Merge pull request ceph#62895 from cyx1231st/wip-seastore-omap-link-init

crimson/os/seastore/omap_manager: simplify maybe_init from tolerating duplicated calls

Reviewed-by: Xuehan Xu <[email protected]>

Author: cyx1231st

src/crimson/os/seastore/cache.cc

@@ -1164,13 +1164,19 @@ CachedExtentRef Cache::duplicate_for_write(
   LOG_PREFIX(Cache::duplicate_for_write);
   assert(i->is_fully_loaded());
 
+#ifndef NDEBUG
+  if (i->is_logical()) {
+    assert(static_cast<LogicalCachedExtent&>(*i).has_laddr());
+    assert(static_cast<LogicalCachedExtent&>(*i).is_seen_by_users());
+  }
+#endif
+
   if (i->is_mutable()) {
     return i;
   }
 
   if (i->is_exist_clean()) {
     assert(i->is_logical());
-    assert(static_cast<LogicalCachedExtent&>(*i).has_laddr());
     i->version++;
     i->state = CachedExtent::extent_state_t::EXIST_MUTATION_PENDING;
     i->last_committed_crc = i->calc_crc32c();
@@ -1201,10 +1207,11 @@ CachedExtentRef Cache::duplicate_for_write(
   ret->version++;
   ret->state = CachedExtent::extent_state_t::MUTATION_PENDING;
   if (i->is_logical()) {
-    auto& lextent = static_cast<LogicalCachedExtent&>(*i);
-    assert(lextent.has_laddr());
+    auto& stable_extent = static_cast<LogicalCachedExtent&>(*i);
     assert(ret->is_logical());
-    static_cast<LogicalCachedExtent&>(*ret).set_laddr(lextent.get_laddr());
+    auto& mutate_extent = static_cast<LogicalCachedExtent&>(*ret);
+    mutate_extent.set_laddr(stable_extent.get_laddr());
+    assert(mutate_extent.is_seen_by_users());
   }
 
   t.add_mutated_extent(ret);

src/crimson/os/seastore/cached_extent.cc

@@ -91,7 +91,8 @@ CachedExtent* CachedExtent::get_transactional_view(transaction_id_t tid) {
 
 std::ostream &LogicalCachedExtent::print_detail(std::ostream &out) const
 {
-  out << ", laddr=" << laddr;
+  out << ", laddr=" << laddr
+      << ", seen=" << seen_by_users;
   return print_detail_l(out);
 }
 

src/crimson/os/seastore/cached_extent.h

@@ -857,10 +857,12 @@ class CachedExtent
 
   /// used to wait while in-progress commit completes
   std::optional<seastar::shared_promise<>> io_wait_promise;
+
   void set_io_wait() {
     ceph_assert(!io_wait_promise);
     io_wait_promise = seastar::shared_promise<>();
   }
+
   void complete_io() {
     ceph_assert(io_wait_promise);
     io_wait_promise->set_value();
@@ -1385,18 +1387,37 @@ class LBAMapping;
  */
 class LogicalCachedExtent : public CachedExtent {
 public:
-  template <typename... T>
-  LogicalCachedExtent(T&&... t) : CachedExtent(std::forward<T>(t)...) {}
+  using CachedExtent::CachedExtent;
+
+  LogicalCachedExtent(const LogicalCachedExtent& other)
+    : CachedExtent(other),
+     seen_by_users(other.seen_by_users) {}
+
+  LogicalCachedExtent(const LogicalCachedExtent& other, share_buffer_t s)
+    : CachedExtent(other, s),
+     seen_by_users(other.seen_by_users) {}
 
   void on_rewrite(Transaction &t, CachedExtent &extent, extent_len_t off) final {
     assert(get_type() == extent.get_type());
     auto &lextent = (LogicalCachedExtent&)extent;
     set_laddr((lextent.get_laddr() + off).checked_to_laddr());
+    seen_by_users = lextent.seen_by_users;
     do_on_rewrite(t, lextent);
   }
 
   virtual void do_on_rewrite(Transaction &t, LogicalCachedExtent &extent) {}
 
+  bool is_seen_by_users() const {
+    return seen_by_users;
+  }
+
+  // handled under TransactionManager,
+  // user should not set this by themselves.
+  void set_seen_by_users() {
+    assert(!seen_by_users);
+    seen_by_users = true;
+  }
+
   bool has_laddr() const {
     return laddr != L_ADDR_NULL;
   }
@@ -1461,6 +1482,12 @@ class LogicalCachedExtent : public CachedExtent {
   // the logical address of the extent, and if shared,
   // it is the intermediate_base, see BtreeLBAMapping comments.
   laddr_t laddr = L_ADDR_NULL;
+
+  // whether the extent has been seen by users since OSD startup,
+  // otherwise, an extent can still be alive if it's dirty or pinned by lru.
+  //
+  // user must initialize the logical extent upon the first access.
+  bool seen_by_users = false;
 };
 
 using LogicalCachedExtentRef = TCachedExtentRef<LogicalCachedExtent>;

src/crimson/os/seastore/logical_child_node.h

@@ -40,6 +40,7 @@ class LogicalChildNode : public LogicalCachedExtent,
   }
 protected:
   void on_replace_prior() final {
+    assert(is_seen_by_users());
     lba_child_node_t::on_replace_prior();
     do_on_replace_prior();
   }

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

@@ -76,7 +76,6 @@ BtreeOMapManager::handle_root_split(
     omap_node_meta_t meta{omap_root.depth + 1};
     nroot->init_range(BEGIN_KEY, END_KEY);
     nroot->set_meta(meta);
-    left->init_range(BEGIN_KEY, pivot);
     nroot->insert_child_ptr(
       nroot->iter_begin().get_offset(),
       dynamic_cast<BaseChildNode<OMapInnerNode, std::string>*>(left.get()));
@@ -85,7 +84,6 @@ BtreeOMapManager::handle_root_split(
     nroot->insert_child_ptr(
       (nroot->iter_begin() + 1).get_offset(),
       dynamic_cast<BaseChildNode<OMapInnerNode, std::string>*>(right.get()));
-    right->init_range(pivot, END_KEY);
     nroot->journal_inner_insert(nroot->iter_begin() + 1, right->get_laddr(),
                                 pivot, nroot->maybe_get_delta_buffer());
     omap_root.update(nroot->get_laddr(), omap_root.get_depth() + 1, omap_root.hint,

src/crimson/os/seastore/omap_manager/btree/omap_btree_node.h

@@ -116,17 +116,14 @@ struct OMapNode : LogicalChildNode {
 
   virtual ~OMapNode() = default;
 
-  void init_range(std::string begin, std::string end) {
-    if (!this->end.empty()) {
-      // this is a duplicated init.
-      assert(end == this->end);
-      return;
-    }
-    if (begin == BEGIN_KEY && end == END_KEY) {
+  void init_range(std::string _begin, std::string _end) {
+    assert(begin.empty());
+    assert(end.empty());
+    if (_begin == BEGIN_KEY && _end == END_KEY) {
       root = true;
     }
-    this->begin = std::move(begin);
-    this->end = std::move(end);
+    begin = std::move(_begin);
+    end = std::move(_end);
   }
   const std::string &get_begin() const {
     return begin;

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

@@ -80,10 +80,15 @@ struct OMapInnerNode
     if (is_rewrite() && !is_btree_root()) {
       auto &prior = *get_prior_instance()->template cast<OMapInnerNode>();
       if (prior.base_child_t::has_parent_tracker()) {
+        assert(prior.is_seen_by_users());
 	// unlike fixed-kv nodes, rewriting child nodes of the omap tree
 	// won't affect parent nodes, so we have to manually take prior
 	// instances' parent trackers here.
 	this->child_node_t::take_parent_from_prior();
+      } else {
+        // dirty omap extent may not be accessed yet during rewrite,
+        // this means the extent may not be initalized yet as linked.
+        assert(!prior.is_seen_by_users());
       }
     }
   }
@@ -231,6 +236,7 @@ struct OMapInnerNode
     if (this->is_valid()
 	&& !this->is_pending()
 	&& !this->is_btree_root()
+	// dirty omap extent may not be accessed/linked yet
 	&& this->base_child_t::has_parent_tracker()) {
       this->child_node_t::destroy();
     }
@@ -282,10 +288,15 @@ struct OMapLeafNode
     if (is_rewrite() && !is_btree_root()) {
       auto &prior = *get_prior_instance()->template cast<OMapLeafNode>();
       if (prior.base_child_t::has_parent_tracker()) {
+        assert(prior.is_seen_by_users());
 	// unlike fixed-kv nodes, rewriting child nodes of the omap tree
 	// won't affect parent nodes, so we have to manually take prior
 	// instances' parent trackers here.
 	this->child_node_t::take_parent_from_prior();
+      } else {
+        // dirty omap extent may not be accessed yet during rewrite,
+        // this means the extent may not be initalized yet as linked.
+        assert(!prior.is_seen_by_users());
       }
     }
   }
@@ -303,6 +314,7 @@ struct OMapLeafNode
     if (this->is_valid()
 	&& !this->is_pending()
 	&& !this->is_btree_root()
+	// dirty omap extent may not be accessed/linked yet
 	&& this->base_child_t::has_parent_tracker()) {
       this->child_node_t::destroy();
     }
@@ -434,14 +446,15 @@ omap_load_extent(
     oc.t, laddr, size,
     [begin=std::move(begin), end=std::move(end), FNAME,
     oc, chp=std::move(chp)](T &extent) mutable {
+      assert(!extent.is_seen_by_users());
       extent.init_range(std::move(begin), std::move(end));
-      if (extent.T::base_child_t::is_parent_valid()
-	  || extent.is_btree_root()) {
-	return;
+      if (extent.is_btree_root()) {
+        return;
       }
-      assert(chp);
       SUBDEBUGT(seastore_omap, "linking {} to {}",
-	oc.t, extent, *chp->get_parent());
+        oc.t, extent, *chp->get_parent());
+      assert(chp);
+      assert(!extent.T::base_child_t::is_parent_valid());
       chp->link_child(&extent);
     }
   ).handle_error_interruptible(

src/crimson/os/seastore/seastore_types.h

@@ -2214,6 +2214,10 @@ constexpr bool is_valid_transaction(transaction_type_t type) {
   return type < transaction_type_t::MAX;
 }
 
+constexpr bool is_user_transaction(transaction_type_t type) {
+  return type <= transaction_type_t::READ;
+}
+
 constexpr bool is_background_transaction(transaction_type_t type) {
   return (type >= transaction_type_t::TRIM_DIRTY &&
           type < transaction_type_t::MAX);

src/crimson/os/seastore/transaction_manager.h

@@ -260,6 +260,8 @@ class TransactionManager : public ExtentCallbackInterface {
     static_assert(is_logical_type(T::TYPE));
     assert(is_aligned(partial_off, get_block_size()));
     assert(is_aligned(partial_len, get_block_size()));
+    // must be user-oriented required by maybe_init
+    assert(is_user_transaction(t.get_src()));
 
     extent_len_t direct_partial_off = partial_off;
     bool is_clone = pin->is_clone();
@@ -302,9 +304,12 @@ class TransactionManager : public ExtentCallbackInterface {
           return cache->read_extent_maybe_partial(
             t, std::move(extent), direct_partial_off, partial_len);
         }).si_then([maybe_init=std::move(maybe_init)](auto extent) {
-	  maybe_init(*extent);
-	  return extent;
-	});
+          if (!extent->is_seen_by_users()) {
+            maybe_init(*extent);
+            extent->set_seen_by_users();
+          }
+          return std::move(extent);
+        });
       } else {
 	return this->pin_to_extent<T>(
           t, std::move(std::get<0>(ret)),
@@ -381,6 +386,7 @@ class TransactionManager : public ExtentCallbackInterface {
     laddr_t laddr_hint,
     extent_len_t len,
     placement_hint_t placement_hint = placement_hint_t::HOT) {
+    static_assert(is_logical_metadata_type(T::TYPE));
     LOG_PREFIX(TransactionManager::alloc_non_data_extent);
     SUBDEBUGT(seastore_tm, "{} hint {}~0x{:x} phint={} ...",
               t, T::TYPE, laddr_hint, len, placement_hint);
@@ -389,6 +395,9 @@ class TransactionManager : public ExtentCallbackInterface {
       len,
       placement_hint,
       INIT_GENERATION);
+    // user must initialize the logical extent themselves.
+    assert(is_user_transaction(t.get_src()));
+    ext->set_seen_by_users();
     return lba_manager->alloc_extent(
       t,
       laddr_hint,
@@ -416,6 +425,7 @@ class TransactionManager : public ExtentCallbackInterface {
     laddr_t laddr_hint,
     extent_len_t len,
     placement_hint_t placement_hint = placement_hint_t::HOT) {
+    static_assert(is_data_type(T::TYPE));
     LOG_PREFIX(TransactionManager::alloc_data_extents);
     SUBDEBUGT(seastore_tm, "{} hint {}~0x{:x} phint={} ...",
               t, T::TYPE, laddr_hint, len, placement_hint);
@@ -424,6 +434,11 @@ class TransactionManager : public ExtentCallbackInterface {
       len,
       placement_hint,
       INIT_GENERATION);
+    // user must initialize the logical extent themselves
+    assert(is_user_transaction(t.get_src()));
+    for (auto& ext : exts) {
+      ext->set_seen_by_users();
+    }
     return lba_manager->alloc_extents(
       t,
       laddr_hint,
@@ -479,6 +494,10 @@ class TransactionManager : public ExtentCallbackInterface {
     LBAMappingRef &&pin,
     std::array<remap_entry, N> remaps) {
     static_assert(std::is_base_of_v<LogicalChildNode, T>);
+    // data extents don't need maybe_init yet, currently,
+    static_assert(is_data_type(T::TYPE));
+    // must be user-oriented required by (the potential) maybe_init
+    assert(is_user_transaction(t.get_src()));
 
 #ifndef NDEBUG
     std::sort(remaps.begin(), remaps.end(),
@@ -550,7 +569,14 @@ class TransactionManager : public ExtentCallbackInterface {
 	  } else {
 	    auto ret = get_extent_if_linked<T>(t, pin->duplicate());
 	    if (ret.index() == 1) {
-	      return std::move(std::get<1>(ret));
+	      return std::get<1>(ret
+	      ).si_then([](auto extent) {
+	        if (!extent->is_seen_by_users()) {
+	          // Note, no maybe_init available for data extents
+	          extent->set_seen_by_users();
+	        }
+	        return std::move(extent);
+	      });
 	    } else {
 	      // absent
 	      return base_iertr::make_ready_future<TCachedExtentRef<T>>();
@@ -571,6 +597,7 @@ class TransactionManager : public ExtentCallbackInterface {
 	    original_bptr = ext->get_bptr();
 	  }
 	  if (ext) {
+	    assert(ext->is_seen_by_users());
 	    cache->retire_extent(t, ext);
 	  } else {
 	    cache->retire_absent_extent_addr(t, original_paddr, original_len);
@@ -594,6 +621,8 @@ class TransactionManager : public ExtentCallbackInterface {
 	      remap_len,
 	      original_laddr,
 	      original_bptr);
+	    // user must initialize the logical extent themselves.
+	    extent->set_seen_by_users();
 	    extents.emplace_back(std::move(extent));
 	  }
 	});
@@ -1034,6 +1063,8 @@ class TransactionManager : public ExtentCallbackInterface {
     extent_len_t partial_len,
     lextent_init_func_t<T> &&maybe_init) {
     static_assert(is_logical_type(T::TYPE));
+    // must be user-oriented required by maybe_init
+    assert(is_user_transaction(t.get_src()));
     using ret = pin_to_extent_ret<T>;
     auto &pref = *pin;
     auto direct_length = pref.is_indirect() ?
@@ -1062,6 +1093,7 @@ class TransactionManager : public ExtentCallbackInterface {
 	pref.link_child(&extent);
 	extent.maybe_set_intermediate_laddr(pref);
 	maybe_init(extent);
+	extent.set_seen_by_users();
       }
     ).si_then([FNAME, &t, pin=std::move(pin), this](auto ref) mutable -> ret {
       if (ref->is_fully_loaded()) {
@@ -1114,6 +1146,7 @@ class TransactionManager : public ExtentCallbackInterface {
     SUBTRACET(seastore_tm, "getting absent extent from pin {} type {} ...",
               t, *pin, type);
     assert(is_logical_type(type));
+    assert(is_background_transaction(t.get_src()));
     auto &pref = *pin;
     laddr_t direct_key;
     extent_len_t direct_length;
@@ -1140,6 +1173,8 @@ class TransactionManager : public ExtentCallbackInterface {
 	assert(!pref.get_parent()->is_pending());
 	pref.link_child(&lextent);
 	lextent.maybe_set_intermediate_laddr(pref);
+        // No change to extent::seen_by_user because this path is only
+        // for background cleaning.
       }
     ).si_then([FNAME, &t, pin=std::move(pin), this](auto ref) {
       auto crc = ref->calc_crc32c();