From: Xuehan Xu Date: Fri, 14 Jun 2024 10:35:05 +0000 (+0800) Subject: crimson/os/seastore/cache: add an efficient method to check if extents are X-Git-Tag: v20.0.0~1647^2~5 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=2721169f3285eb132d4a6f2792ec490c72129dd3;p=ceph.git crimson/os/seastore/cache: add an efficient method to check if extents are viewable to transactions Instead of searching the transaction's retired_set to determine whether an extent has been retired, we add the transaction that's retiring an extent to that extent's retired_transactions field and search that field to do the check. Since the probability of multiple transactions retiring the same extent is very low, this approach should be more cpu efficient. Signed-off-by: Xuehan Xu --- diff --git a/src/crimson/os/seastore/btree/btree_range_pin.h b/src/crimson/os/seastore/btree/btree_range_pin.h index a2d7455873354..49773d98d7461 100644 --- a/src/crimson/os/seastore/btree/btree_range_pin.h +++ b/src/crimson/os/seastore/btree/btree_range_pin.h @@ -194,6 +194,27 @@ public: return parent->has_been_invalidated(); } + bool is_unviewable_by_trans(CachedExtent& extent, Transaction &t) const { + if (!extent.is_valid()) { + return true; + } + if (extent.is_pending()) { + assert(extent.is_pending_in_trans(t.get_trans_id())); + return false; + } + auto &pendings = extent.mutation_pendings; + auto trans_id = t.get_trans_id(); + bool unviewable = (pendings.find(trans_id, trans_spec_view_t::cmp_t()) != + pendings.end()); + if (!unviewable) { + auto &trans = extent.retired_transactions; + unviewable = (trans.find(trans_id, trans_spec_view_t::cmp_t()) != + trans.end()); + assert(unviewable == t.is_retired(extent.get_paddr(), extent.get_length())); + } + return unviewable; + } + get_child_ret_t get_logical_extent(Transaction&) final; bool is_stable() const final; bool is_data_stable() const final; diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index a737b2be29ca7..7ade49827a041 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -884,7 +884,7 @@ void Cache::mark_transaction_conflicted( if (t.get_src() != Transaction::src_t::READ) { io_stat_t retire_stat; for (auto &i: t.retired_set) { - retire_stat.increment(i->get_length()); + retire_stat.increment(i.extent->get_length()); } efforts.retire.increment_stat(retire_stat); @@ -1249,18 +1249,19 @@ record_t Cache::prepare_record( alloc_delta_t rel_delta; rel_delta.op = alloc_delta_t::op_types_t::CLEAR; for (auto &i: t.retired_set) { + auto &extent = i.extent; get_by_ext(efforts.retire_by_ext, - i->get_type()).increment(i->get_length()); - retire_stat.increment(i->get_length()); - DEBUGT("retired and remove extent -- {}", t, *i); - commit_retire_extent(t, i); - if (is_backref_mapped_extent_node(i) - || is_retired_placeholder(i->get_type())) { + extent->get_type()).increment(extent->get_length()); + retire_stat.increment(extent->get_length()); + DEBUGT("retired and remove extent -- {}", t, *extent); + commit_retire_extent(t, extent); + if (is_backref_mapped_extent_node(extent) + || is_retired_placeholder(extent->get_type())) { rel_delta.alloc_blk_ranges.emplace_back( - i->get_paddr(), + extent->get_paddr(), L_ADDR_NULL, - i->get_length(), - i->get_type()); + extent->get_length(), + extent->get_type()); } } alloc_deltas.emplace_back(std::move(rel_delta)); @@ -1621,7 +1622,8 @@ void Cache::complete_commit( } for (auto &i: t.retired_set) { - epm.mark_space_free(i->get_paddr(), i->get_length()); + auto &extent = i.extent; + epm.mark_space_free(extent->get_paddr(), extent->get_length()); } for (auto &i: t.existing_block_list) { if (i->is_valid()) { @@ -1638,24 +1640,25 @@ void Cache::complete_commit( last_commit = start_seq; for (auto &i: t.retired_set) { - i->dirty_from_or_retired_at = start_seq; - if (is_backref_mapped_extent_node(i) - || is_retired_placeholder(i->get_type())) { + auto &extent = i.extent; + extent->dirty_from_or_retired_at = start_seq; + if (is_backref_mapped_extent_node(extent) + || is_retired_placeholder(extent->get_type())) { DEBUGT("backref_list free {} len {}", t, - i->get_paddr(), - i->get_length()); + extent->get_paddr(), + extent->get_length()); backref_list.emplace_back( std::make_unique( - i->get_paddr(), + extent->get_paddr(), L_ADDR_NULL, - i->get_length(), - i->get_type(), + extent->get_length(), + extent->get_type(), start_seq)); - } else if (is_backref_node(i->get_type())) { - remove_backref_extent(i->get_paddr()); + } else if (is_backref_node(extent->get_type())) { + remove_backref_extent(extent->get_paddr()); } else { - ERRORT("{}", t, *i); + ERRORT("{}", t, *extent); ceph_abort("not possible"); } } diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h index 4778117c8a66f..c81304668fb38 100644 --- a/src/crimson/os/seastore/cached_extent.h +++ b/src/crimson/os/seastore/cached_extent.h @@ -651,6 +651,7 @@ private: friend struct paddr_cmp; friend struct ref_paddr_cmp; friend class ExtentIndex; + friend struct trans_retired_extent_link_t; /// Pointer to containing index (or null) ExtentIndex *parent_index = nullptr; @@ -735,6 +736,7 @@ private: protected: trans_view_set_t mutation_pendings; + trans_view_set_t retired_transactions; CachedExtent(CachedExtent &&other) = delete; CachedExtent(ceph::bufferptr &&_ptr) : ptr(std::move(_ptr)) { @@ -884,17 +886,54 @@ struct paddr_cmp { } }; +// trans_retired_extent_link_t is used to link stable extents with +// the transactions that retired them. With this link, we can find +// out whether an extent has been retired by a specific transaction +// in a way that's more efficient than searching through the transaction's +// retired_set (Transaction::is_retired()) +struct trans_retired_extent_link_t { + CachedExtentRef extent; + // We use trans_spec_view_t instead of transaction_id_t, so that, + // when a transaction is deleted or reset, we can efficiently remove + // that transaction from the extents' extent-transaction link set. + // Otherwise, we have to search through each extent's "retired_transactions" + // to remove the transaction + trans_spec_view_t trans_view; + trans_retired_extent_link_t(CachedExtentRef extent, transaction_id_t id) + : extent(extent), trans_view{id} + { + assert(extent->is_stable()); + extent->retired_transactions.insert(trans_view); + } +}; + /// Compare extent refs by paddr struct ref_paddr_cmp { using is_transparent = paddr_t; - bool operator()(const CachedExtentRef &lhs, const CachedExtentRef &rhs) const { - return lhs->poffset < rhs->poffset; - } - bool operator()(const paddr_t &lhs, const CachedExtentRef &rhs) const { - return lhs < rhs->poffset; - } - bool operator()(const CachedExtentRef &lhs, const paddr_t &rhs) const { - return lhs->poffset < rhs; + bool operator()( + const trans_retired_extent_link_t &lhs, + const trans_retired_extent_link_t &rhs) const { + return lhs.extent->poffset < rhs.extent->poffset; + } + bool operator()( + const paddr_t &lhs, + const trans_retired_extent_link_t &rhs) const { + return lhs < rhs.extent->poffset; + } + bool operator()( + const trans_retired_extent_link_t &lhs, + const paddr_t &rhs) const { + return lhs.extent->poffset < rhs; + } + bool operator()( + const CachedExtentRef &lhs, + const trans_retired_extent_link_t &rhs) const { + return lhs->poffset < rhs.extent->poffset; + } + bool operator()( + const trans_retired_extent_link_t &lhs, + const CachedExtentRef &rhs) const { + return lhs.extent->poffset < rhs->poffset; } }; @@ -910,7 +949,7 @@ class addr_extent_set_base_t using pextent_set_t = addr_extent_set_base_t< paddr_t, - CachedExtentRef, + trans_retired_extent_link_t, ref_paddr_cmp >; diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h index 849c025009ccd..90a9fc80883d8 100644 --- a/src/crimson/os/seastore/transaction.h +++ b/src/crimson/os/seastore/transaction.h @@ -126,14 +126,14 @@ public: ref->set_invalid(*this); write_set.erase(*ref); assert(ref->prior_instance); - retired_set.insert(ref->prior_instance); + retired_set.emplace(ref->prior_instance, trans_id); assert(read_set.count(ref->prior_instance->get_paddr())); ref->prior_instance.reset(); } else { // && retired_set.count(ref->get_paddr()) == 0 // If it's already in the set, insert here will be a noop, // which is what we want. - retired_set.insert(ref); + retired_set.emplace(ref, trans_id); } } @@ -262,9 +262,9 @@ public: { auto where = retired_set.find(&placeholder); assert(where != retired_set.end()); - assert(where->get() == &placeholder); + assert(where->extent.get() == &placeholder); where = retired_set.erase(where); - retired_set.emplace_hint(where, &extent); + retired_set.emplace_hint(where, &extent, trans_id); } } @@ -318,11 +318,14 @@ public: bool is_retired(paddr_t paddr, extent_len_t len) { auto iter = retired_set.lower_bound(paddr); - if (iter == retired_set.end() || - (*iter)->get_paddr() != paddr) { + if (iter == retired_set.end()) { + return false; + } + auto &extent = iter->extent; + if (extent->get_paddr() != paddr) { return false; } else { - assert(len == (*iter)->get_length()); + assert(len == extent->get_length()); return true; } }