crimson/os/seastore/lba_mapping: make LBAMapping duplicate

light-weighted and safe

This commit removes LBAMapping::child_pos, forces TransactioManager
methods to link children directly through child_pos_t, so that
LBAMapping::duplicate() can be a shallow one.

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

diff --git a/src/crimson/os/seastore/btree/btree_types.h b/src/crimson/os/seastore/btree/btree_types.h
index fb4e465a05ddfd1621baa7a700c14431c5d560ab..2a756ce8e79ea467ea1e1bde249072cc2e5cf9ff 100644 (file)
--- a/src/crimson/os/seastore/btree/btree_types.h
+++ b/src/crimson/os/seastore/btree/btree_types.h
@@ -198,7 +198,9 @@ struct __attribute__((packed)) backref_map_val_le_t {
  * time.
  */
 template <typename key_t, typename val_t>
-struct BtreeCursor {
+struct BtreeCursor
+  : public boost::intrusive_ref_counter<
+      BtreeCursor<key_t, val_t>, boost::thread_unsafe_counter> {
   BtreeCursor(
     op_context_t &ctx,
     CachedExtentRef parent,
@@ -281,7 +283,6 @@ struct LBACursor : BtreeCursor<laddr_t, lba::lba_map_val_t> {
     assert(!is_indirect());
     return val->refcount;
   }
-
   std::unique_ptr<LBACursor> duplicate() const {
     return std::make_unique<LBACursor>(*this);
   }
@@ -291,7 +292,7 @@ struct LBACursor : BtreeCursor<laddr_t, lba::lba_map_val_t> {
   using base_iertr = trans_iertr<base_ertr>;
   base_iertr::future<> refresh();
 };
-using LBACursorRef = std::unique_ptr<LBACursor>;
+using LBACursorRef = boost::intrusive_ptr<LBACursor>;
 
 struct BackrefCursor : BtreeCursor<paddr_t, backref::backref_map_val_t> {
   using Base = BtreeCursor<paddr_t, backref::backref_map_val_t>;
@@ -309,7 +310,7 @@ struct BackrefCursor : BtreeCursor<paddr_t, backref::backref_map_val_t> {
     return val->type;
   }
 };
-using BackrefCursorRef = std::unique_ptr<BackrefCursor>;
+using BackrefCursorRef = boost::intrusive_ptr<BackrefCursor>;
 
 template <typename key_t, typename val_t>
 std::ostream &operator<<(

diff --git a/src/crimson/os/seastore/btree/fixed_kv_btree.h b/src/crimson/os/seastore/btree/fixed_kv_btree.h
index 9a847a890270d414befdcaa2372e014ddc39a007..954cca7fd692417c2bbed4e7fa269f38aae22346 100644 (file)
--- a/src/crimson/os/seastore/btree/fixed_kv_btree.h
+++ b/src/crimson/os/seastore/btree/fixed_kv_btree.h
@@ -286,9 +286,8 @@ public:
     }
 
     // handle_boundary() must be called before get_cursor
-    std::unique_ptr<cursor_t> get_cursor(op_context_t ctx) const {
-      assert(!is_end());
-      return std::make_unique<cursor_t>(
+    boost::intrusive_ptr<cursor_t> get_cursor(op_context_t ctx) const {
+      return new cursor_t(
         ctx,
        leaf.node,
         leaf.node->modifications,
@@ -500,7 +499,7 @@ public:
       cursor.pos);
   }
 
-  std::unique_ptr<cursor_t> get_cursor(
+  boost::intrusive_ptr<cursor_t> get_cursor(
     op_context_t c,
     TCachedExtentRef<leaf_node_t> leaf,
     node_key_t key)
@@ -521,8 +520,7 @@ public:
     assert(leaf->get_size() != pos);
     auto it = leaf->iter_idx(pos);
     assert(it.get_key() == key);
-    return std::make_unique<cursor_t>(
-      c, leaf, leaf->modifications, key, it.get_val(), pos);
+    return new cursor_t(c, leaf, leaf->modifications, key, it.get_val(), pos);
   }
 
   /**

diff --git a/src/crimson/os/seastore/lba_mapping.cc b/src/crimson/os/seastore/lba_mapping.cc
index 458bd1323e35e074736d957827e82ee179fb94e8..a4bbf4a41ff52069b66d8ca011ccad214daf09d9 100644 (file)
--- a/src/crimson/os/seastore/lba_mapping.cc
+++ b/src/crimson/os/seastore/lba_mapping.cc
@@ -43,7 +43,7 @@ std::ostream &operator<<(std::ostream &out, const lba_mapping_list_t &rhs)
 using lba::LBALeafNode;
 
 get_child_ret_t<LBALeafNode, LogicalChildNode>
-LBAMapping::get_logical_extent(Transaction &t)
+LBAMapping::get_logical_extent(Transaction &t) const
 {
   assert(is_linked_direct());
   ceph_assert(direct_cursor->is_viewable());

diff --git a/src/crimson/os/seastore/lba_mapping.h b/src/crimson/os/seastore/lba_mapping.h
index 162c39c45ab95695c4a86439851dce673eda4d96..ad099e6e94dfd7dcf1b6a19e0c6cca335bb18a05 100644 (file)
--- a/src/crimson/os/seastore/lba_mapping.h
+++ b/src/crimson/os/seastore/lba_mapping.h
@@ -40,9 +40,9 @@ public:
   }
 
   LBAMapping() = delete;
-  LBAMapping(const LBAMapping &) = delete;
+  LBAMapping(const LBAMapping &) = default;
   LBAMapping(LBAMapping &&) = default;
-  LBAMapping &operator=(const LBAMapping &) = delete;
+  LBAMapping &operator=(const LBAMapping &) = default;
   LBAMapping &operator=(LBAMapping &&) = default;
   ~LBAMapping() = default;
 
@@ -144,9 +144,10 @@ public:
   }
 
   get_child_ret_t<lba::LBALeafNode, LogicalChildNode>
-  get_logical_extent(Transaction &t);
+  get_logical_extent(Transaction &t) const;
 
   LBAMapping duplicate() const {
+    assert(!is_null());
     auto dup_iter = [](const LBACursorRef &iter) -> LBACursorRef {
       if (iter) {
        return iter->duplicate();

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index 0ffa865a0fcfdb8f29c2f64c728382d7d26b6ba6..993f24f6fa60dc9356f6d3576b864696b0ace483 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -257,7 +257,7 @@ TransactionManager::ref_iertr::future<LBAMapping> TransactionManager::remove(
   return mapping.refresh().si_then([&t, this, FNAME](auto mapping) {
     auto fut = base_iertr::make_ready_future<LogicalChildNodeRef>();
     if (!mapping.is_indirect() && mapping.get_val().is_real_location()) {
-      auto ret = get_extent_if_linked<LogicalChildNode>(t, mapping.duplicate());
+      auto ret = get_extent_if_linked<LogicalChildNode>(t, mapping);
       if (ret.index() == 1) {
         fut = std::move(std::get<1>(ret));
       }

diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index 00dadef002382800ca12aaf7adf17f4cb3c4ec83..a912ef7ddb0eb02f6b0ecf5e573221c186ae8f76 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -1045,7 +1045,7 @@ private:
     auto v = pin.get_logical_extent(t);
     if (v.has_child()) {
       return v.get_child_fut(
-      ).si_then([pin=std::move(pin)](auto extent) {
+      ).si_then([pin=pin.duplicate()](auto extent) {
 #ifndef NDEBUG
         auto lextent = extent->template cast<LogicalChildNode>();
         auto pin_laddr = pin.get_intermediate_base();
@@ -1055,7 +1055,7 @@ private:
       });
     } else {
       return unlinked_child_t{
-       std::move(pin),
+       std::move(const_cast<LBAMapping&>(pin)),
        v.get_child_pos()};
     }
   }
@@ -1078,7 +1078,7 @@ private:
         return ext;
       });
     } else {
-      return pin_to_extent_by_type(t, std::move(pin), v.get_child_pos(), type);
+      return pin_to_extent_by_type(t, pin.duplicate(), v.get_child_pos(), type);
     }
   }
 
@@ -1117,6 +1117,7 @@ private:
     static_assert(is_logical_type(T::TYPE));
     // must be user-oriented required by maybe_init
     assert(is_user_transaction(t.get_src()));
+    assert(pin.is_viewable());
     using ret = pin_to_extent_ret<T>;
     auto direct_length = pin.get_intermediate_length();
     if (full_extent_integrity_check) {
@@ -1144,7 +1145,7 @@ private:
        maybe_init(extent);
        extent.set_seen_by_users();
       }
-    ).si_then([FNAME, &t, pin=std::move(pin), this](auto ref) mutable -> ret {
+    ).si_then([FNAME, &t, pin=pin.duplicate(), this](auto ref) mutable -> ret {
       if (ref->is_fully_loaded()) {
         auto crc = ref->calc_crc32c();
         SUBTRACET(
@@ -1195,6 +1196,7 @@ private:
     LOG_PREFIX(TransactionManager::pin_to_extent_by_type);
     SUBTRACET(seastore_tm, "getting absent extent from pin {} type {} ...",
               t, pin, type);
+    assert(pin.is_viewable());
     assert(is_logical_type(type));
     assert(is_background_transaction(t.get_src()));
     laddr_t direct_key = pin.get_intermediate_base();
@@ -1215,7 +1217,7 @@ private:
         // 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) {
+    ).si_then([FNAME, &t, pin=pin.duplicate(), this](auto ref) {
       auto crc = ref->calc_crc32c();
       SUBTRACET(
        seastore_tm,

diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc
index 536d3b1452969afd3914a5deb637a1b450c78769..d2deac07a91d9092e55ba33622c11c237f3b4fc6 100644 (file)
--- a/src/test/crimson/seastore/test_transaction_manager.cc
+++ b/src/test/crimson/seastore/test_transaction_manager.cc
@@ -633,13 +633,13 @@ struct transaction_manager_test_t :
 
   TestBlockRef try_read_pin(
     test_transaction_t &t,
-    LBAMapping &&pin) {
+    const LBAMapping pin) {
     using ertr = with_trans_ertr<TransactionManager::base_iertr>;
     bool indirect = pin.is_indirect();
     auto addr = pin.get_key();
     auto im_addr = pin.get_intermediate_base();
     auto ext = with_trans_intr(*(t.t), [&](auto& trans) {
-      return tm->read_pin<TestBlock>(trans, std::move(pin));
+      return tm->read_pin<TestBlock>(trans, pin.duplicate());
     }).safe_then([](auto ret) {
       return ertr::make_ready_future<TestBlockRef>(ret.extent);
     }).handle_error(
@@ -1608,13 +1608,12 @@ struct transaction_manager_test_t :
            }
 
            auto t = create_transaction();
-            auto pin0 = try_get_pin(t, offset);
-           if (!pin0 || pin0->get_length() != length) {
+            auto last_pin = try_get_pin(t, offset);
+           if (!last_pin || last_pin->get_length() != length) {
              early_exit++;
              return;
            }
 
-            auto last_pin = pin0->duplicate();
            ASSERT_TRUE(!split_points.empty());
            for (auto off : split_points) {
              if (off == 0 || off >= 255) {
@@ -1629,7 +1628,7 @@ struct transaction_manager_test_t :
                conflicted++;
                return;
              }
-              last_pin = pin->duplicate();
+              last_pin = std::move(pin);
            }
             auto last_ext = try_get_extent(t, last_pin.get_key());
             if (!last_ext) {
@@ -1743,7 +1742,7 @@ struct transaction_manager_test_t :
                }
               }
               ASSERT_TRUE(rpin);
-              last_rpin = rpin->duplicate();
+              last_rpin = std::move(*rpin);
            }
             auto last_rext = try_get_extent(t, last_rpin.get_key());
             if (!last_rext) {