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 <xuxuehan@qianxin.com>
(cherry picked from commit 2721169f3285eb132d4a6f2792ec490c72129dd3)

diff --git a/src/crimson/os/seastore/btree/btree_range_pin.h b/src/crimson/os/seastore/btree/btree_range_pin.h
index a2d74558733548ff7fbed2e1521699faf041c257..49773d98d7461d542f821b525e66f3f479f4aac0 100644 (file)
--- 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<LogicalCachedExtent> 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 a737b2be29ca7cf6faf18215868dec2cabce6b39..7ade49827a041dd6481a45e0f39b9d844f778a0a 100644 (file)
--- 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<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");
     }
   }

diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
index 4778117c8a66fa381f2e9428b376d8e856567d86..c81304668fb3843af0d2459307c08c5c017fef1b 100644 (file)
--- 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 849c025009ccd76d09c8299148b34d0abe839389..90a9fc80883d88aae976587ee3e751481a74cfd0 100644 (file)
--- 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;
     }
   }