crimson/os/seastore/transaction: cleanup add_absent/present_to_retired_set

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 4dd49caf8ca388bff9c00d405bb3581b36a931e9..42407284d663447d811b38927ffe9bcaa7b59774 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -62,7 +62,7 @@ Cache::retire_extent_ret Cache::retire_extent_addr(
   auto result = t.get_extent(addr, &ext);
   if (result == Transaction::get_extent_ret::PRESENT) {
     DEBUGT("retire {}~0x{:x} on t -- {}", t, addr, length, *ext);
-    t.add_to_retired_set(CachedExtentRef(&*ext));
+    t.add_present_to_retired_set(CachedExtentRef(&*ext));
     return retire_extent_iertr::now();
   } else if (result == Transaction::get_extent_ret::RETIRED) {
     ERRORT("retire {}~0x{:x} failed, already retired -- {}", t, addr, length, *ext);
@@ -90,8 +90,7 @@ Cache::retire_extent_ret Cache::retire_extent_addr(
            t, addr, length, *ext);
     add_extent(ext);
   }
-  t.add_to_read_set(ext);
-  t.add_to_retired_set(ext);
+  t.add_absent_to_retired_set(ext);
   return retire_extent_iertr::now();
 }
 
@@ -117,8 +116,7 @@ void Cache::retire_absent_extent_addr(
   DEBUGT("retire {}~0x{:x} as placeholder, add extent -- {}",
         t, addr, length, *ext);
   add_extent(ext);
-  t.add_to_read_set(ext);
-  t.add_to_retired_set(ext);
+  t.add_absent_to_retired_set(ext);
 }
 
 void Cache::dump_contents()

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index 5be4cb97a35b38956463608a5a96281326819397..0339d6af6744dfb14e456c89005ed12c3453a01b 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -153,7 +153,7 @@ public:
   void retire_extent(Transaction &t, CachedExtentRef ref) {
     LOG_PREFIX(Cache::retire_extent);
     SUBDEBUGT(seastore_cache, "retire extent -- {}", t, *ref);
-    t.add_to_retired_set(ref);
+    t.add_present_to_retired_set(ref);
   }
 
   /// Declare paddr retired in t

diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h
index a3dfa7261a07d7adc9e4976bb71f9906db8de49a..28a28fdfadcae92c1e3f889562aa10f595211865 100644 (file)
--- a/src/crimson/os/seastore/transaction.h
+++ b/src/crimson/os/seastore/transaction.h
@@ -98,6 +98,17 @@ struct rbm_pending_ool_t {
  * - seastore_cache logs
  */
 class Transaction {
+private:
+  auto lookup_read_set(CachedExtentRef ref) const {
+    assert(ref->is_valid());
+    assert(!is_weak());
+    auto it = ref->read_transactions.lower_bound(
+      this, read_set_item_t<Transaction>::trans_cmp_t());
+    bool exists =
+      (it != ref->read_transactions.end() && it->t == this);
+    return std::make_pair(exists, it);
+  }
+
 public:
   using Ref = std::unique_ptr<Transaction>;
   using on_destruct_func_t = std::function<void(Transaction&)>;
@@ -107,39 +118,29 @@ public:
     RETIRED
   };
   get_extent_ret get_extent(paddr_t addr, CachedExtentRef *out) {
-    LOG_PREFIX(Transaction::get_extent);
-    // it's possible that both write_set and retired_set contain
-    // this addr at the same time when addr is absolute and the
-    // corresponding extent is used to map existing extent on disk.
-    // So search write_set first.
-    if (auto iter = write_set.find_offset(addr);
-       iter != write_set.end()) {
-      if (out)
-       *out = CachedExtentRef(&*iter);
-      SUBTRACET(seastore_cache, "{} is present in write_set -- {}",
-                *this, addr, *iter);
-      assert(!out || (*out)->is_valid());
-      return get_extent_ret::PRESENT;
-    } else if (retired_set.count(addr)) {
-      return get_extent_ret::RETIRED;
-    } else if (
-      auto iter = read_set.find(addr);
-      iter != read_set.end()) {
-      // placeholder in read-set should be in the retired-set
-      // at the same time.
-      assert(!is_retired_placeholder_type(iter->ref->get_type()));
-      if (out)
-       *out = iter->ref;
-      SUBTRACET(seastore_cache, "{} is present in read_set -- {}",
-                *this, addr, *(iter->ref));
-      return get_extent_ret::PRESENT;
-    } else {
-      return get_extent_ret::ABSENT;
+    auto [result, ext] = do_get_extent(addr);
+    // placeholder in read-set must be in the retired-set
+    // at the same time, user should not see a placeholder.
+    assert(result != get_extent_ret::PRESENT ||
+           !is_retired_placeholder_type(ext->get_type()));
+    if (out && result == get_extent_ret::PRESENT) {
+      *out = ext;
     }
+    return result;
   }
 
-  void add_to_retired_set(CachedExtentRef ref) {
-    ceph_assert(!is_weak());
+  void add_absent_to_retired_set(CachedExtentRef ref) {
+    add_to_read_set(ref);
+    add_present_to_retired_set(ref);
+  }
+
+  void add_present_to_retired_set(CachedExtentRef ref) {
+    assert(!is_weak());
+#ifndef NDEBUG
+    auto [result, ext] = do_get_extent(ref->get_paddr());
+    assert(result == get_extent_ret::PRESENT);
+    assert(ext == ref);
+#endif
     if (ref->is_exist_clean() ||
        ref->is_exist_mutation_pending()) {
       existing_block_stats.dec(ref);
@@ -169,11 +170,8 @@ public:
       return false;
     }
 
-    assert(ref->is_valid());
-
-    auto it = ref->read_transactions.lower_bound(
-      this, read_set_item_t<Transaction>::trans_cmp_t());
-    if (it != ref->read_transactions.end() && it->t == this) {
+    auto [exists, it] = lookup_read_set(ref);
+    if (exists) {
       return false;
     }
 
@@ -189,11 +187,8 @@ public:
       return;
     }
 
-    assert(ref->is_valid());
-
-    auto it = ref->read_transactions.lower_bound(
-      this, read_set_item_t<Transaction>::trans_cmp_t());
-    assert(it == ref->read_transactions.end() || it->t != this);
+    auto [exists, it] = lookup_read_set(ref);
+    assert(!exists);
 
     auto [iter, inserted] = read_set.emplace(this, ref);
     ceph_assert(inserted);
@@ -583,6 +578,33 @@ private:
   friend class Cache;
   friend Ref make_test_transaction();
 
+  std::pair<get_extent_ret, CachedExtentRef> do_get_extent(paddr_t addr) {
+    LOG_PREFIX(Transaction::do_get_extent);
+    // it's possible that both write_set and retired_set contain
+    // this addr at the same time when addr is absolute and the
+    // corresponding extent is used to map existing extent on disk.
+    // So search write_set first.
+    if (auto iter = write_set.find_offset(addr);
+       iter != write_set.end()) {
+      auto ret = CachedExtentRef(&*iter);
+      SUBTRACET(seastore_cache, "{} is present in write_set -- {}",
+                *this, addr, *ret);
+      assert(ret->is_valid());
+      return {get_extent_ret::PRESENT, ret};
+    } else if (retired_set.count(addr)) {
+      return {get_extent_ret::RETIRED, nullptr};
+    } else if (
+      auto iter = read_set.find(addr);
+      iter != read_set.end()) {
+      auto ret = iter->ref;
+      SUBTRACET(seastore_cache, "{} is present in read_set -- {}",
+                *this, addr, *ret);
+      return {get_extent_ret::PRESENT, ret};
+    } else {
+      return {get_extent_ret::ABSENT, nullptr};
+    }
+  }
+
   void set_backref_entries(backref_entry_refs_t&& entries) {
     assert(backref_entries.empty());
     backref_entries = std::move(entries);