From: Xuehan Xu Date: Wed, 19 Mar 2025 09:06:12 +0000 (+0800) Subject: crimson/os/seastore/lba_mapping: make LBAMapping duplicate X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=51225f4087b3229e28a9113b5db187fd8ef7bfe1;p=ceph.git 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 --- diff --git a/src/crimson/os/seastore/btree/btree_types.h b/src/crimson/os/seastore/btree/btree_types.h index fb4e465a05dd..2a756ce8e79e 100644 --- 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 -struct BtreeCursor { +struct BtreeCursor + : public boost::intrusive_ref_counter< + BtreeCursor, boost::thread_unsafe_counter> { BtreeCursor( op_context_t &ctx, CachedExtentRef parent, @@ -281,7 +283,6 @@ struct LBACursor : BtreeCursor { assert(!is_indirect()); return val->refcount; } - std::unique_ptr duplicate() const { return std::make_unique(*this); } @@ -291,7 +292,7 @@ struct LBACursor : BtreeCursor { using base_iertr = trans_iertr; base_iertr::future<> refresh(); }; -using LBACursorRef = std::unique_ptr; +using LBACursorRef = boost::intrusive_ptr; struct BackrefCursor : BtreeCursor { using Base = BtreeCursor; @@ -309,7 +310,7 @@ struct BackrefCursor : BtreeCursor { return val->type; } }; -using BackrefCursorRef = std::unique_ptr; +using BackrefCursorRef = boost::intrusive_ptr; template 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 9a847a890270..954cca7fd692 100644 --- 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 get_cursor(op_context_t ctx) const { - assert(!is_end()); - return std::make_unique( + boost::intrusive_ptr 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 get_cursor( + boost::intrusive_ptr get_cursor( op_context_t c, TCachedExtentRef 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( - 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 458bd1323e35..a4bbf4a41ff5 100644 --- 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 -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 162c39c45ab9..ad099e6e94df 100644 --- 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 - 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 0ffa865a0fcf..993f24f6fa60 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -257,7 +257,7 @@ TransactionManager::ref_iertr::future TransactionManager::remove( return mapping.refresh().si_then([&t, this, FNAME](auto mapping) { auto fut = base_iertr::make_ready_future(); if (!mapping.is_indirect() && mapping.get_val().is_real_location()) { - auto ret = get_extent_if_linked(t, mapping.duplicate()); + auto ret = get_extent_if_linked(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 00dadef00238..a912ef7ddb0e 100644 --- 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(); auto pin_laddr = pin.get_intermediate_base(); @@ -1055,7 +1055,7 @@ private: }); } else { return unlinked_child_t{ - std::move(pin), + std::move(const_cast(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; 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 536d3b145296..d2deac07a91d 100644 --- 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; 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(trans, std::move(pin)); + return tm->read_pin(trans, pin.duplicate()); }).safe_then([](auto ret) { return ertr::make_ready_future(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) {