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 <[email protected]>

Author: xxhdx1985126

src/crimson/os/seastore/btree/btree_range_pin.h

@@ -194,6 +194,27 @@ class BtreeNodeMapping : public PhysicalNodeMapping<key_t, val_t> {
     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<LogicalCachedExtent> get_logical_extent(Transaction&) final;
   bool is_stable() const final;
   bool is_data_stable() const final;

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<backref_entry_t>(
-	  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");
     }
   }

src/crimson/os/seastore/cached_extent.h

@@ -651,6 +651,7 @@ class CachedExtent
   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 @@ class CachedExtent
 
 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
   >;
 

src/crimson/os/seastore/transaction.h

@@ -126,14 +126,14 @@ class Transaction {
       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 @@ class Transaction {
     {
       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 @@ class Transaction {
 
   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;
     }
   }