From 231c7acc55071afa3c42241dcbd20bf5e9f84acc Mon Sep 17 00:00:00 2001 From: Xuehan Xu Date: Mon, 16 Oct 2023 11:58:32 +0800 Subject: [PATCH] crimson/os/seastore/lba_manager: hide lba mapping ref count update away from users of TransactionManager Signed-off-by: Xuehan Xu --- .../os/seastore/btree/btree_range_pin.cc | 10 +++ .../os/seastore/btree/btree_range_pin.h | 1 + src/crimson/os/seastore/btree/fixed_kv_node.h | 38 +++++++++- src/crimson/os/seastore/cached_extent.cc | 11 ++- src/crimson/os/seastore/cached_extent.h | 4 +- .../flat_collection_manager.cc | 2 +- .../lba_manager/btree/btree_lba_manager.cc | 62 +++++++-------- .../lba_manager/btree/btree_lba_manager.h | 75 ++++++++++++------- .../os/seastore/object_data_handler.cc | 4 +- .../omap_manager/btree/btree_omap_manager.cc | 4 +- .../btree/omap_btree_node_impl.cc | 2 +- .../node_extent_manager/seastore.h | 2 +- .../os/seastore/transaction_manager.cc | 10 +-- src/crimson/os/seastore/transaction_manager.h | 39 +++++----- .../seastore/test_transaction_manager.cc | 2 +- 15 files changed, 173 insertions(+), 93 deletions(-) diff --git a/src/crimson/os/seastore/btree/btree_range_pin.cc b/src/crimson/os/seastore/btree/btree_range_pin.cc index 2f801dcf1ec..1fe79eafa81 100644 --- a/src/crimson/os/seastore/btree/btree_range_pin.cc +++ b/src/crimson/os/seastore/btree/btree_range_pin.cc @@ -22,6 +22,16 @@ BtreeNodeMapping::get_logical_extent( return v; } +template +bool BtreeNodeMapping::is_stable() const +{ + assert(parent); + assert(parent->is_valid()); + assert(pos != std::numeric_limits::max()); + auto &p = (FixedKVNode&)*parent; + return p.is_child_stable(pos); +} + template class BtreeNodeMapping; template class BtreeNodeMapping; } // namespace crimson::os::seastore diff --git a/src/crimson/os/seastore/btree/btree_range_pin.h b/src/crimson/os/seastore/btree/btree_range_pin.h index 7a08b6d8947..b23a50bf4ba 100644 --- a/src/crimson/os/seastore/btree/btree_range_pin.h +++ b/src/crimson/os/seastore/btree/btree_range_pin.h @@ -195,6 +195,7 @@ public: } get_child_ret_t get_logical_extent(Transaction&) final; + bool is_stable() const final; }; } diff --git a/src/crimson/os/seastore/btree/fixed_kv_node.h b/src/crimson/os/seastore/btree/fixed_kv_node.h index 956a1824e2a..0ae23b2f4de 100644 --- a/src/crimson/os/seastore/btree/fixed_kv_node.h +++ b/src/crimson/os/seastore/btree/fixed_kv_node.h @@ -157,7 +157,7 @@ struct FixedKVNode : ChildableCachedExtent { (get_node_size() - offset - 1) * sizeof(ChildableCachedExtent*)); } - FixedKVNode& get_stable_for_key(node_key_t key) { + FixedKVNode& get_stable_for_key(node_key_t key) const { ceph_assert(is_pending()); if (is_mutation_pending()) { return (FixedKVNode&)*get_prior_instance(); @@ -229,6 +229,8 @@ struct FixedKVNode : ChildableCachedExtent { virtual get_child_ret_t get_logical_child(op_context_t c, uint16_t pos) = 0; + virtual bool is_child_stable(uint16_t pos) const = 0; + template get_child_ret_t get_child(op_context_t c, iter_t iter) { auto pos = iter.get_offset(); @@ -592,6 +594,11 @@ struct FixedKVInternalNode return get_child_ret_t(child_pos_t(nullptr, 0)); } + bool is_child_stable(uint16_t pos) const final { + ceph_abort("impossible"); + return false; + } + bool validate_stable_children() final { LOG_PREFIX(FixedKVInternalNode::validate_stable_children); if (this->children.empty()) { @@ -984,6 +991,35 @@ struct FixedKVLeafNode } } + // children are considered stable if any of the following case is true: + // 1. Not in memory + // 2. being stable + // 3. being mutation pending and under-io + bool is_child_stable(uint16_t pos) const final { + auto child = this->children[pos]; + if (is_valid_child_ptr(child)) { + ceph_assert(child->is_logical()); + return child->is_stable() || + (child->is_mutation_pending() && + child->is_pending_io()); + } else if (this->is_pending()) { + auto key = this->iter_idx(pos).get_key(); + auto &sparent = this->get_stable_for_key(key); + auto spos = sparent.child_pos_for_key(key); + auto child = sparent.children[spos]; + if (is_valid_child_ptr(child)) { + ceph_assert(child->is_logical()); + return child->is_stable() || + (child->is_mutation_pending() && + child->is_pending_io()); + } else { + return true; + } + } else { + return true; + } + } + bool validate_stable_children() override { return true; } diff --git a/src/crimson/os/seastore/cached_extent.cc b/src/crimson/os/seastore/cached_extent.cc index 769b0446a5d..37884227186 100644 --- a/src/crimson/os/seastore/cached_extent.cc +++ b/src/crimson/os/seastore/cached_extent.cc @@ -158,8 +158,15 @@ parent_tracker_t::~parent_tracker_t() { std::ostream &operator<<(std::ostream &out, const LBAMapping &rhs) { - return out << "LBAMapping(" << rhs.get_key() << "~" << rhs.get_length() - << "->" << rhs.get_val(); + out << "LBAMapping(" << rhs.get_key() << "~" << rhs.get_length() + << "->" << rhs.get_val(); + if (rhs.is_indirect()) { + out << " indirect(" << rhs.get_intermediate_base() << "~" + << rhs.get_intermediate_key() << "~" + << rhs.get_intermediate_length() << ")"; + } + out << ")"; + return out; } std::ostream &operator<<(std::ostream &out, const lba_pin_list_t &rhs) diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h index 02f8ae46c95..c73839cf1fe 100644 --- a/src/crimson/os/seastore/cached_extent.h +++ b/src/crimson/os/seastore/cached_extent.h @@ -595,7 +595,7 @@ public: // a rewrite extent has an invalid prior_instance, // and a mutation_pending extent has a valid prior_instance - CachedExtentRef get_prior_instance() { + CachedExtentRef get_prior_instance() const { return prior_instance; } @@ -1046,6 +1046,8 @@ public: child_pos->link_child(c); } + virtual bool is_stable() const = 0; + virtual ~PhysicalNodeMapping() {} protected: std::optional child_pos = std::nullopt; diff --git a/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc b/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc index decb095f6f9..3c65ed0e2c1 100644 --- a/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc +++ b/src/crimson/os/seastore/collection_manager/flat_collection_manager.cc @@ -84,7 +84,7 @@ FlatCollectionManager::create(coll_root_t &coll_root, Transaction &t, get_coll_context(t), cid, info.split_bits ).si_then([=, this, &t](auto result) { assert(result == CollectionNode::create_result_t::SUCCESS); - return tm.dec_ref(t, extent->get_laddr()); + return tm.remove(t, extent->get_laddr()); }).si_then([] (auto) { return create_iertr::make_ready_future<>(); }); diff --git a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc index bb43bdb2c4f..1b7f927ec0f 100644 --- a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc +++ b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc @@ -197,7 +197,7 @@ BtreeLBAManager::_get_original_mappings( pin->get_key(), pin->get_length(), pin->get_raw_val().get_laddr()); auto &btree_new_pin = static_cast(*new_pin); - btree_new_pin.set_key_for_indirect( + btree_new_pin.make_indirect( pin->get_key(), pin->get_length(), pin->get_raw_val().get_laddr()); @@ -287,7 +287,7 @@ BtreeLBAManager::_get_mapping( c.trans, pin->get_raw_val().get_laddr() ).si_then([&pin](auto new_pin) { ceph_assert(pin->get_length() == new_pin->get_length()); - new_pin->set_key_for_indirect( + new_pin->make_indirect( pin->get_key(), pin->get_length()); return new_pin; @@ -307,7 +307,6 @@ BtreeLBAManager::_alloc_extent( extent_len_t len, pladdr_t addr, paddr_t actual_addr, - laddr_t intermediate_base, LogicalCachedExtent* nextent) { struct state_t { @@ -321,6 +320,8 @@ BtreeLBAManager::_alloc_extent( LOG_PREFIX(BtreeLBAManager::_alloc_extent); TRACET("{}~{}, hint={}", t, addr, len, hint); + + ceph_assert(actual_addr != P_ADDR_NULL ? addr.is_laddr() : addr.is_paddr()); auto c = get_context(t); ++stats.num_alloc_extents; auto lookup_attempts = stats.num_alloc_extents_iter_nexts; @@ -384,17 +385,9 @@ BtreeLBAManager::_alloc_extent( state.ret = iter; }); }); - }).si_then([c, actual_addr, addr, intermediate_base](auto &&state) { - auto ret_pin = state.ret->get_pin(c); - if (actual_addr != P_ADDR_NULL) { - ceph_assert(addr.is_laddr()); - ret_pin->set_paddr(actual_addr); - ret_pin->set_intermediate_base(intermediate_base); - } else { - ceph_assert(addr.is_paddr()); - } - return alloc_extent_iertr::make_ready_future( - std::move(ret_pin)); + }).si_then([c](auto &&state) { + return alloc_extent_iertr::make_ready_future< + LBAMappingRef>(state.ret->get_pin(c)); }); } @@ -556,7 +549,8 @@ BtreeLBAManager::update_mapping( return ret; }, nextent - ).si_then([&t, laddr, prev_addr, addr, FNAME](auto result) { + ).si_then([&t, laddr, prev_addr, addr, FNAME](auto p) { + auto &result = p.first; DEBUGT("laddr={}, paddr {} => {} done -- {}", t, laddr, prev_addr, addr, result); }, @@ -687,7 +681,9 @@ BtreeLBAManager::update_refcount( return out; }, nullptr - ).si_then([&t, addr, delta, FNAME, this, cascade_remove](auto result) { + ).si_then([&t, addr, delta, FNAME, this, cascade_remove](auto p) { + auto &result = p.first; + auto &mapping = p.second; DEBUGT("laddr={}, delta={} done -- {}", t, addr, delta, result); auto fut = ref_iertr::make_ready_future< std::optional>>(); @@ -698,19 +694,23 @@ BtreeLBAManager::update_refcount( result.len ); } - return fut.si_then([result](auto removed) { + return fut.si_then([result, mapping=std::move(mapping)] + (auto removed) mutable { if (result.pladdr.is_laddr() && removed) { - return ref_update_result_t{ - result.refcount, - removed->first, - removed->second}; + return std::make_pair( + ref_update_result_t{ + result.refcount, + removed->first, + removed->second}, + std::move(mapping)); } else { - return ref_update_result_t{ - result.refcount, - result.pladdr, - result.len - }; + return std::make_pair( + ref_update_result_t{ + result.refcount, + result.pladdr, + result.len}, + std::move(mapping)); } }); }); @@ -724,7 +724,7 @@ BtreeLBAManager::_update_mapping( LogicalCachedExtent* nextent) { auto c = get_context(t); - return with_btree_ret( + return with_btree_ret( cache, c, [f=std::move(f), c, addr, nextent](auto &btree) mutable { @@ -744,7 +744,8 @@ BtreeLBAManager::_update_mapping( c, iter ).si_then([ret] { - return ret; + return std::make_pair( + std::move(ret), BtreeLBAMappingRef(nullptr)); }); } else { return btree.update( @@ -752,8 +753,9 @@ BtreeLBAManager::_update_mapping( iter, ret, nextent - ).si_then([ret](auto) { - return ret; + ).si_then([c, ret](auto iter) { + return std::make_pair( + std::move(ret), iter.get_pin(c)); }); } }); diff --git a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h index 5496a4b1968..7895f806abd 100644 --- a/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h +++ b/src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h @@ -51,7 +51,8 @@ class BtreeLBAMapping : public BtreeNodeMapping { // 3. intermediate_base: the laddr key of the physical lba mapping, intermediate_key // and intermediate_base should be the same when doing cloning // 4. intermediate_offset: intermediate_key - intermediate_base -// 5. paddr: the paddr recorded in the physical lba mapping pointed to by the +// 5. intermediate_length: the length of the actual physical lba mapping +// 6. paddr: the paddr recorded in the physical lba mapping pointed to by the // indirect lba mapping being queried; // // NOTE THAT, for direct BtreeLBAMappings, their intermediate_keys are the same as @@ -73,7 +74,7 @@ public: val.len, meta), key(meta.begin), - indirect(val.pladdr.is_laddr() ? true : false), + indirect(val.pladdr.is_laddr()), intermediate_key(indirect ? val.pladdr.get_laddr() : L_ADDR_NULL), intermediate_length(indirect ? val.len : 0), raw_val(val.pladdr), @@ -88,12 +89,16 @@ public: return indirect; } - void set_key_for_indirect( + void make_indirect( laddr_t new_key, extent_len_t length, laddr_t interkey = L_ADDR_NULL) { - turn_indirect(interkey); + assert(!indirect); + assert(value.is_paddr()); + intermediate_base = key; + intermediate_key = (interkey == L_ADDR_NULL ? key : interkey); + indirect = true; key = new_key; intermediate_length = len; len = length; @@ -107,10 +112,6 @@ public: return raw_val; } - void set_paddr(paddr_t addr) { - value = addr; - } - laddr_t get_intermediate_key() const final { assert(is_indirect()); assert(intermediate_key != L_ADDR_NULL); @@ -136,10 +137,6 @@ public: return intermediate_length; } - void set_intermediate_base(laddr_t base) { - intermediate_base = base; - } - protected: std::unique_ptr> _duplicate( op_context_t ctx) const final { @@ -154,12 +151,6 @@ protected: return pin; } private: - void turn_indirect(laddr_t interkey) { - assert(value.is_paddr()); - intermediate_base = key; - intermediate_key = (interkey == L_ADDR_NULL ? key : interkey); - indirect = true; - } laddr_t key = L_ADDR_NULL; bool indirect = false; laddr_t intermediate_key = L_ADDR_NULL; @@ -226,7 +217,6 @@ public: len, P_ADDR_ZERO, P_ADDR_NULL, - L_ADDR_NULL, nullptr); } @@ -238,14 +228,32 @@ public: paddr_t actual_addr, laddr_t intermediate_base) { + assert(intermediate_key != L_ADDR_NULL); + assert(intermediate_base != L_ADDR_NULL); return _alloc_extent( t, hint, len, intermediate_key, actual_addr, - intermediate_base, - nullptr); + nullptr + ).si_then([&t, this, intermediate_base](auto indirect_mapping) { + assert(indirect_mapping->is_indirect()); + return update_refcount(t, intermediate_base, 1, false + ).si_then([imapping=std::move(indirect_mapping)](auto p) mutable { + auto mapping = std::move(p.second); + ceph_assert(mapping->is_stable()); + mapping->make_indirect( + imapping->get_key(), + imapping->get_length(), + imapping->get_intermediate_key()); + return seastar::make_ready_future< + LBAMappingRef>(std::move(mapping)); + }); + }).handle_error_interruptible( + crimson::ct_error::input_output_error::pass_further{}, + crimson::ct_error::assert_all{"unexpect enoent"} + ); } alloc_extent_ret alloc_extent( @@ -261,7 +269,6 @@ public: len, addr, P_ADDR_NULL, - L_ADDR_NULL, &ext); } @@ -269,13 +276,19 @@ public: Transaction &t, laddr_t addr, bool cascade_remove) final { - return update_refcount(t, addr, -1, cascade_remove); + return update_refcount(t, addr, -1, cascade_remove + ).si_then([](auto p) { + return std::move(p.first); + }); } ref_ret incref_extent( Transaction &t, laddr_t addr) final { - return update_refcount(t, addr, 1, false); + return update_refcount(t, addr, 1, false + ).si_then([](auto p) { + return std::move(p.first); + }); } ref_ret incref_extent( @@ -283,7 +296,10 @@ public: laddr_t addr, int delta) final { ceph_assert(delta > 0); - return update_refcount(t, addr, delta, false); + return update_refcount(t, addr, delta, false + ).si_then([](auto p) { + return std::move(p.first); + }); } /** @@ -344,7 +360,10 @@ private: * * Updates refcount, returns resulting refcount */ - using update_refcount_ret = ref_ret; + using update_refcount_ret_bare = std::pair; + using update_refcount_iertr = ref_iertr; + using update_refcount_ret = update_refcount_iertr::future< + update_refcount_ret_bare>; update_refcount_ret update_refcount( Transaction &t, laddr_t addr, @@ -357,7 +376,8 @@ private: * Updates mapping, removes if f returns nullopt */ using _update_mapping_iertr = ref_iertr; - using _update_mapping_ret = ref_iertr::future; + using _update_mapping_ret_bare = std::pair; + using _update_mapping_ret = ref_iertr::future<_update_mapping_ret_bare>; using update_func_t = std::function< lba_map_val_t(const lba_map_val_t &v) >; @@ -373,7 +393,6 @@ private: extent_len_t len, pladdr_t addr, paddr_t actual_addr, - laddr_t intermediate_base, LogicalCachedExtent*); using _get_mapping_ret = get_mapping_iertr::future; diff --git a/src/crimson/os/seastore/object_data_handler.cc b/src/crimson/os/seastore/object_data_handler.cc index 025f91993ef..1b0ae5c814a 100644 --- a/src/crimson/os/seastore/object_data_handler.cc +++ b/src/crimson/os/seastore/object_data_handler.cc @@ -387,7 +387,7 @@ ObjectDataHandler::write_ret do_removals( DEBUGT("decreasing ref: {}", ctx.t, pin->get_key()); - return ctx.tm.dec_ref( + return ctx.tm.remove( ctx.t, pin->get_key() ).si_then( @@ -1524,7 +1524,7 @@ ObjectDataHandler::clone_ret ObjectDataHandler::clone_extents( object_data.get_reserved_data_base(), object_data.get_reserved_data_len(), data_base); - return ctx.tm.dec_ref( + return ctx.tm.remove( ctx.t, object_data.get_reserved_data_base() ).si_then( diff --git a/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc b/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc index 1782d7ee66e..77dc270a532 100644 --- a/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc +++ b/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc @@ -84,7 +84,7 @@ BtreeOMapManager::handle_root_merge( omap_root.hint); oc.t.get_omap_tree_stats().depth = omap_root.depth; oc.t.get_omap_tree_stats().extents_num_delta--; - return oc.tm.dec_ref(oc.t, root->get_laddr() + return oc.tm.remove(oc.t, root->get_laddr() ).si_then([](auto &&ret) -> handle_root_merge_ret { return seastar::now(); }).handle_error_interruptible( @@ -274,7 +274,7 @@ BtreeOMapManager::omap_clear( ).si_then([this, &t, &omap_root](auto extent) { return extent->clear(get_omap_context(t, omap_root.hint)); }).si_then([this, &omap_root, &t] { - return tm.dec_ref( + return tm.remove( t, omap_root.get_location() ).si_then([&omap_root] (auto ret) { omap_root.update( diff --git a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc index 4db58414a6e..96115f13237 100644 --- a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc +++ b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.cc @@ -36,7 +36,7 @@ using dec_ref_iertr = OMapInnerNode::base_iertr; using dec_ref_ret = dec_ref_iertr::future<>; template dec_ref_ret dec_ref(omap_context_t oc, T&& addr) { - return oc.tm.dec_ref(oc.t, std::forward(addr)).handle_error_interruptible( + return oc.tm.remove(oc.t, std::forward(addr)).handle_error_interruptible( dec_ref_iertr::pass_further{}, crimson::ct_error::assert_all{ "Invalid error in OMapInnerNode helper dec_ref" diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h index f7cfa8c2112..c12e583bd56 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h @@ -165,7 +165,7 @@ class SeastoreNodeExtentManager final: public TransactionManagerHandle { return retire_iertr::now(); } } - return tm.dec_ref(t, extent).si_then([addr, len, &t] (unsigned cnt) { + return tm.remove(t, extent).si_then([addr, len, &t] (unsigned cnt) { assert(cnt == 0); SUBTRACET(seastore_onode, "retired {}B at {:#x} ...", t, len, addr); }); diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index ad8e5f1a65f..7fbe119fcdd 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -210,11 +210,11 @@ TransactionManager::ref_ret TransactionManager::inc_ref( }); } -TransactionManager::ref_ret TransactionManager::dec_ref( +TransactionManager::ref_ret TransactionManager::remove( Transaction &t, LogicalCachedExtentRef &ref) { - LOG_PREFIX(TransactionManager::dec_ref); + LOG_PREFIX(TransactionManager::remove); TRACET("{}", t, *ref); return lba_manager->decref_extent(t, ref->get_laddr(), true ).si_then([this, FNAME, &t, ref](auto result) { @@ -253,17 +253,17 @@ TransactionManager::ref_ret TransactionManager::_dec_ref( }); } -TransactionManager::refs_ret TransactionManager::dec_ref( +TransactionManager::refs_ret TransactionManager::remove( Transaction &t, std::vector offsets) { - LOG_PREFIX(TransactionManager::dec_ref); + LOG_PREFIX(TransactionManager::remove); DEBUG("{} offsets", offsets.size()); return seastar::do_with(std::move(offsets), std::vector(), [this, &t] (auto &&offsets, auto &refcnt) { return trans_intr::do_for_each(offsets.begin(), offsets.end(), [this, &t, &refcnt] (auto &laddr) { - return this->dec_ref(t, laddr).si_then([&refcnt] (auto ref) { + return this->remove(t, laddr).si_then([&refcnt] (auto ref) { refcnt.push_back(ref); return ref_iertr::now(); }); diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index dfce85c5e1b..f30dea3bd77 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -235,6 +235,7 @@ public: using ref_iertr = LBAManager::ref_iertr; using ref_ret = ref_iertr::future; +#ifdef UNIT_TESTS_BUILT /// Add refcount for ref ref_ret inc_ref( Transaction &t, @@ -244,14 +245,28 @@ public: ref_ret inc_ref( Transaction &t, laddr_t offset); +#endif - /// Remove refcount for ref - ref_ret dec_ref( + /** + * remove + * + * Remove the extent and the corresponding lba mapping, + * users must make sure that lba mapping's refcount is 1 + */ + ref_ret remove( Transaction &t, LogicalCachedExtentRef &ref); - /// Remove refcount for offset - ref_ret dec_ref( + /** + * remove + * + * 1. Remove the indirect mapping(s), and if refcount drops to 0, + * also remove the direct mapping and retire the extent. + * + * 2. Remove the direct mapping(s) and retire the extent if + * refcount drops to 0. + */ + ref_ret remove( Transaction &t, laddr_t offset) { return _dec_ref(t, offset, true); @@ -259,7 +274,7 @@ public: /// remove refcount for list of offset using refs_ret = ref_iertr::future>; - refs_ret dec_ref( + refs_ret remove( Transaction &t, std::vector offsets); @@ -487,10 +502,6 @@ public: mapping.is_indirect() ? mapping.get_intermediate_key() : mapping.get_key(); - auto intermediate_base = - mapping.is_indirect() - ? mapping.get_intermediate_base() - : mapping.get_key(); LOG_PREFIX(TransactionManager::clone_pin); SUBDEBUGT(seastore_tm, "len={}, laddr_hint={}, clone_offset {}", @@ -503,15 +514,7 @@ public: intermediate_key, mapping.get_val(), intermediate_key - ).si_then([this, &t, intermediate_base](auto pin) { - return inc_ref(t, intermediate_base - ).si_then([pin=std::move(pin)](auto) mutable { - return std::move(pin); - }).handle_error_interruptible( - crimson::ct_error::input_output_error::pass_further(), - crimson::ct_error::assert_all("not possible") - ); - }); + ); } /* alloc_extents diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc index 914eea9bbbf..54bd27b8c18 100644 --- a/src/test/crimson/seastore/test_transaction_manager.cc +++ b/src/test/crimson/seastore/test_transaction_manager.cc @@ -636,7 +636,7 @@ struct transaction_manager_test_t : ceph_assert(test_mappings.get(offset, t.mapping_delta).refcount > 0); auto refcnt = with_trans_intr(*(t.t), [&](auto& trans) { - return tm->dec_ref(trans, offset); + return tm->remove(trans, offset); }).unsafe_get0(); auto check_refcnt = test_mappings.dec_ref(offset, t.mapping_delta); EXPECT_EQ(refcnt, check_refcnt); -- 2.39.5