Merge pull request ceph#63218 from xxhdx1985126/wip-seastore-fix-retire-absent

crimson/os/seastore/transaction_manager: Cache::retire_absent_extent_addr should be called immediately after TransactionManager::get_extent_if_linked if necessary

Reviewed-by: Yingxin Cheng <[email protected]>

Author: cyx1231st

src/crimson/os/seastore/btree/fixed_kv_btree.h

@@ -496,12 +496,12 @@ class FixedKVBtree {
           assert(cnode->has_parent_tracker());
           if (node->is_pending()) {
             auto &n = node->get_stable_for_key(i->get_key());
-            assert(cnode->get_parent_node().get() == &n);
+            assert(cnode->peek_parent_node().get() == &n);
             auto pos = n.lower_bound(i->get_key()).get_offset();
             assert(pos < n.get_size());
             assert(n.children[pos] == cnode.get());
           } else {
-            assert(cnode->get_parent_node().get() == node.get());
+            assert(cnode->peek_parent_node().get() == node.get());
             assert(node->children[i->get_offset()] == cnode.get());
           }
         } else if (child_node->is_pending()) {
@@ -511,12 +511,12 @@ class FixedKVBtree {
             assert(prior.is_parent_valid());
             if (node->is_mutation_pending()) {
               auto &n = node->get_stable_for_key(i->get_key());
-              assert(prior.get_parent_node().get() == &n);
+              assert(prior.peek_parent_node().get() == &n);
               auto pos = n.lower_bound(i->get_key()).get_offset();
               assert(pos < n.get_size());
               assert(n.children[pos] == &prior);
             } else {
-              assert(prior.get_parent_node().get() == node.get());
+              assert(prior.peek_parent_node().get() == node.get());
               assert(node->children[i->get_offset()] == &prior);
             }
           } else {
@@ -567,9 +567,9 @@ class FixedKVBtree {
         } else {
           auto c = static_cast<child_node_t*>(child);
           assert(c->has_parent_tracker());
-          assert(c->get_parent_node().get() == node.get()
+          assert(c->peek_parent_node().get() == node.get()
             || (node->is_pending() && c->is_stable()
-                && c->get_parent_node().get() == &node->get_stable_for_key(
+                && c->peek_parent_node().get() == &node->get_stable_for_key(
                   i->get_key())));
         }
       } else {

src/crimson/os/seastore/cache.cc

@@ -1231,6 +1231,7 @@ record_t Cache::prepare_record(
   auto trans_src = t.get_src();
   assert(!t.is_weak());
   assert(trans_src != Transaction::src_t::READ);
+  assert(t.read_set.size() + t.num_replace_placeholder == t.read_items.size());
 
   auto& efforts = get_by_src(stats.committed_efforts_by_src,
                              trans_src);
@@ -1247,7 +1248,7 @@ record_t Cache::prepare_record(
                i.ref->get_type()).increment(i.ref->get_length());
     read_stat.increment(i.ref->get_length());
   }
-  t.read_set.clear();
+  t.clear_read_set();
   t.write_set.clear();
 
   record_t record(record_type_t::JOURNAL, trans_src);
@@ -1365,6 +1366,7 @@ record_t Cache::prepare_record(
     // retiering extents, this is because logical linked tree
     // nodes needs to access their prior instances in this
     // phase if they are rewritten.
+    e->set_io_wait();
     e->prepare_commit();
   });
 
@@ -1796,6 +1798,7 @@ void Cache::complete_commit(
     assert(!i->is_dirty());
     const auto t_src = t.get_src();
     touch_extent(*i, &t_src, t.get_cache_hint());
+    i->complete_io();
     epm.commit_space_used(i->get_paddr(), i->get_length());
 
     // Note: commit extents and backref allocations in the same place

src/crimson/os/seastore/cache.h

@@ -471,11 +471,19 @@ class Cache : public ExtentTransViewRetriever {
 
     const auto t_src = t.get_src();
     auto ext_type = extent->get_type();
+    // FIXME: retired-placeholder isn't linked in the lba tree yet.
+    //   We think it's still working because:
+    //   1. A retired-placeholder must be an ObjectDataBlock
+    //   2. Per rados object, no read is possible during write,
+    //      and write cannot be parallel
+    assert(!is_retired_placeholder_type(ext_type));
     cache_access_stats_t& access_stats = get_by_ext(
       get_by_src(stats.access_by_src_ext, t_src),
       ext_type);
 
     CachedExtent* p_extent;
+    bool needs_step_2 = false;
+    bool needs_touch = false;
     if (extent->is_stable()) {
       p_extent = extent->get_transactional_view(t);
       if (p_extent != extent.get()) {
@@ -495,16 +503,22 @@ class Cache : public ExtentTransViewRetriever {
       } else {
         // stable from trans-view
         assert(!p_extent->is_pending_in_trans(t.get_trans_id()));
-        if (t.maybe_add_to_read_set(p_extent)) {
+        auto ret = t.maybe_add_to_read_set(p_extent);
+        if (ret.added) {
           if (p_extent->is_dirty()) {
             ++access_stats.cache_dirty;
             ++stats.access.cache_dirty;
           } else {
             ++access_stats.cache_lru;
             ++stats.access.cache_lru;
           }
-          touch_extent(*p_extent, &t_src, t.get_cache_hint());
+          if (ret.is_paddr_known) {
+            touch_extent(*p_extent, &t_src, t.get_cache_hint());
+          } else {
+            needs_touch = true;
+          }
         } else {
+          // already exists
           if (p_extent->is_dirty()) {
             ++access_stats.trans_dirty;
             ++stats.access.trans_dirty;
@@ -513,6 +527,9 @@ class Cache : public ExtentTransViewRetriever {
             ++stats.access.trans_lru;
           }
         }
+        // step 2 maybe reordered after wait_io(),
+        // always try step 2 if paddr unknown
+        needs_step_2 = !ret.is_paddr_known;
       }
     } else {
       assert(!extent->is_stable_writting());
@@ -538,7 +555,13 @@ class Cache : public ExtentTransViewRetriever {
 
     return trans_intr::make_interruptible(
       p_extent->wait_io()
-    ).then_interruptible([p_extent] {
+    ).then_interruptible([p_extent, needs_touch, needs_step_2, &t, this, &t_src] {
+      if (needs_step_2) {
+	t.maybe_add_to_read_set_step_2(p_extent);
+      }
+      if (needs_touch) {
+	touch_extent(*p_extent, &t_src, t.get_cache_hint());
+      }
       return get_extent_iertr::make_ready_future<CachedExtentRef>(
         CachedExtentRef(p_extent));
     });

src/crimson/os/seastore/cached_extent.h

@@ -67,11 +67,16 @@ class read_set_item_t {
   using set_hook_t = boost::intrusive::set_member_hook<
     boost::intrusive::link_mode<
       boost::intrusive::auto_unlink>>;
-  set_hook_t trans_hook;
-  using set_hook_options = boost::intrusive::member_hook<
+  set_hook_t trans_hook; // used to attach transactions to extents
+  set_hook_t extent_hook; // used to attach extents to transactions
+  using trans_hook_options = boost::intrusive::member_hook<
     read_set_item_t,
     set_hook_t,
     &read_set_item_t::trans_hook>;
+  using extent_hook_options = boost::intrusive::member_hook<
+    read_set_item_t,
+    set_hook_t,
+    &read_set_item_t::extent_hook>;
 
 public:
   struct extent_cmp_t {
@@ -101,23 +106,34 @@ class read_set_item_t {
 
   using trans_set_t =  boost::intrusive::set<
     read_set_item_t,
-    set_hook_options,
+    trans_hook_options,
     boost::intrusive::constant_time_size<false>,
     boost::intrusive::compare<trans_cmp_t>>;
+  using extent_set_t =  boost::intrusive::set<
+    read_set_item_t,
+    extent_hook_options,
+    boost::intrusive::constant_time_size<false>,
+    boost::intrusive::compare<extent_cmp_t>>;
 
   T *t = nullptr;
   CachedExtentRef ref;
 
+  bool is_extent_attached_to_trans() const {
+    return extent_hook.is_linked();
+  }
+
+  bool is_trans_attached_to_extent() const {
+    return trans_hook.is_linked();
+  }
+
   read_set_item_t(T *t, CachedExtentRef ref);
   read_set_item_t(const read_set_item_t &) = delete;
   read_set_item_t(read_set_item_t &&) = default;
   ~read_set_item_t() = default;
 };
 
 template <typename T>
-using read_extent_set_t = std::set<
-  read_set_item_t<T>,
-  typename read_set_item_t<T>::extent_cmp_t>;
+using read_extent_set_t = typename read_set_item_t<T>::extent_set_t;
 
 template <typename T>
 using read_trans_set_t = typename read_set_item_t<T>::trans_set_t;
@@ -549,12 +565,12 @@ class CachedExtent
   }
 
   bool is_stable_writting() const {
-    // MUTATION_PENDING and under-io extents are already stable and visible,
-    // see prepare_record().
+    // MUTATION_PENDING/INITIAL_WRITE_PENDING and under-io extents are already
+    // stable and visible, see prepare_record().
     //
-    // XXX: It might be good to mark this case as DIRTY from the definition,
+    // XXX: It might be good to mark this case as DIRTY/CLEAN from the definition,
     // which probably can make things simpler.
-    return is_mutation_pending() && is_pending_io();
+    return (is_mutation_pending() || is_initial_pending()) && is_pending_io();
   }
 
   /// Returns true if extent is stable and shared among transactions

src/crimson/os/seastore/lba/btree_lba_manager.cc

@@ -1091,7 +1091,7 @@ BtreeLBAManager::_update_mapping(
 	    }
 	    assert(!nextent || 
 	           (nextent->has_parent_tracker() &&
-		    nextent->get_parent_node().get() == iter.get_leaf_node().get()));
+		    nextent->peek_parent_node().get() == iter.get_leaf_node().get()));
 	    return update_mapping_ret_bare_t(iter.get_cursor(c));
 	  });
 	}

src/crimson/os/seastore/linked_tree_node.h

@@ -196,7 +196,10 @@ class BaseChildNode {
   bool is_parent_valid() const {
     return parent_tracker && parent_tracker->is_valid();
   }
-  TCachedExtentRef<ParentT> get_parent_node() const {
+  // this method should only be used for asserts and logs, because
+  // the parent node might be stable writing and should "wait_io"
+  // before further access
+  TCachedExtentRef<ParentT> peek_parent_node() const {
     assert(parent_tracker);
     return parent_tracker->get_parent();
   }
@@ -999,7 +1002,7 @@ class ChildNode : public BaseChildNode<ParentT, key_t> {
     assert(!down_cast().is_btree_root());
     assert(this->has_parent_tracker());
     auto off = get_parent_pos();
-    auto parent = this->get_parent_node();
+    auto parent = this->peek_parent_node();
     assert(parent->children[off] == &down_cast());
     parent->children[off] = nullptr;
   }
@@ -1018,15 +1021,15 @@ class ChildNode : public BaseChildNode<ParentT, key_t> {
     this->parent_tracker = prior.BaseChildNode<ParentT, key_t>::parent_tracker;
     assert(this->has_parent_tracker());
     auto off = get_parent_pos();
-    auto parent = this->get_parent_node();
+    auto parent = this->peek_parent_node();
     assert(me.get_prior_instance().get() ==
 	   dynamic_cast<CachedExtent*>(parent->children[off]));
     parent->children[off] = &me;
   }
 
   btreenode_pos_t get_parent_pos() const {
     auto &me = down_cast();
-    auto parent = this->get_parent_node();
+    auto parent = this->peek_parent_node();
     assert(parent);
     //TODO: can this search be avoided?
     auto key = me.get_begin();

src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc

@@ -626,7 +626,7 @@ std::ostream &OMapLeafNode::print_detail_l(std::ostream &out) const
 	<< ", end=" << get_end();
   }
   if (this->child_node_t::is_parent_valid())
-    return out << ", parent=" << (void*)this->child_node_t::get_parent_node().get();
+    return out << ", parent=" << (void*)this->child_node_t::peek_parent_node().get();
   else
     return out;
 }