crimson/os/seastore: make the updates to backref_entry_mset be consistent with extents

Generally, make alloc/free be consistent with add/commit/retire extents
in Cache.

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Signed-off-by: Zhang Song <zhangsong02@qianxin.com>

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index f40fd32b2bc6d728fce8b61d8cb02b6ce4eff82f..3af5c0d4500caa12f076207bca3e9fdc037991bf 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -1339,6 +1339,7 @@ record_t Cache::prepare_record(
   io_stat_t retire_stat;
   std::vector<alloc_delta_t> alloc_deltas;
   alloc_delta_t rel_delta;
+  backref_entry_refs_t backref_entries;
   rel_delta.op = alloc_delta_t::op_types_t::CLEAR;
   for (auto &i: t.retired_set) {
     auto &extent = i.extent;
@@ -1355,6 +1356,25 @@ record_t Cache::prepare_record(
          extent->get_length(),
          extent->get_type()));
     }
+
+    // Note: commit extents and backref allocations in the same place
+    if (is_backref_mapped_type(extent->get_type()) ||
+       is_retired_placeholder_type(extent->get_type())) {
+      DEBUGT("backref_entry free {} len 0x{:x}",
+            t,
+            extent->get_paddr(),
+            extent->get_length());
+      backref_entries.emplace_back(
+       backref_entry_t::create_retire(
+         extent->get_paddr(),
+         extent->get_length(),
+         extent->get_type()));
+    } else if (is_backref_node(extent->get_type())) {
+      remove_backref_extent(extent->get_paddr());
+    } else {
+      ERRORT("Got unexpected extent type: {}", t, *extent);
+      ceph_abort("imposible");
+    }
   }
   alloc_deltas.emplace_back(std::move(rel_delta));
 
@@ -1529,6 +1549,9 @@ record_t Cache::prepare_record(
     record.push_back(std::move(delta));
   }
 
+  apply_backref_mset(backref_entries);
+  t.set_backref_entries(std::move(backref_entries));
+
   ceph_assert(t.get_fresh_block_stats().num ==
               t.inline_block_list.size() +
               t.ool_block_list.size() +
@@ -1628,20 +1651,17 @@ record_t Cache::prepare_record(
   return record;
 }
 
-void Cache::commit_backref_entries(
+void Cache::apply_backref_byseq(
   backref_entry_refs_t&& backref_entries,
   const journal_seq_t& seq)
 {
-  LOG_PREFIX(Cache::commit_backref_entries);
+  LOG_PREFIX(Cache::apply_backref_byseq);
   DEBUG("backref_entry apply {} entries at {}",
        backref_entries.size(), seq);
   assert(seq != JOURNAL_SEQ_NULL);
   if (backref_entries.empty()) {
     return;
   }
-  for (auto &entry : backref_entries) {
-    backref_entry_mset.insert(*entry);
-  }
   if (backref_entryrefs_by_seq.empty()) {
     backref_entryrefs_by_seq.insert(
       backref_entryrefs_by_seq.end(),
@@ -1703,6 +1723,8 @@ void Cache::complete_commit(
     const auto t_src = t.get_src();
     touch_extent(*i, &t_src);
     epm.commit_space_used(i->get_paddr(), i->get_length());
+
+    // Note: commit extents and backref allocations in the same place
     if (is_backref_mapped_type(i->get_type())) {
       DEBUGT("backref_entry alloc {} len 0x{:x}",
             t,
@@ -1775,23 +1797,6 @@ void Cache::complete_commit(
   for (auto &i: t.retired_set) {
     auto &extent = i.extent;
     extent->dirty_from_or_retired_at = start_seq;
-    if (is_backref_mapped_type(extent->get_type()) ||
-        is_retired_placeholder_type(extent->get_type())) {
-      DEBUGT("backref_entry free {} len 0x{:x}",
-            t,
-            extent->get_paddr(),
-            extent->get_length());
-      backref_entries.emplace_back(
-       backref_entry_t::create_retire(
-         extent->get_paddr(),
-         extent->get_length(),
-         extent->get_type()));
-    } else if (is_backref_node(extent->get_type())) {
-      remove_backref_extent(extent->get_paddr());
-    } else {
-      ERRORT("Got unexpected extent type: {}", t, *extent);
-      ceph_abort("impossible");
-    }
   }
 
   auto existing_stats = t.get_existing_block_stats();
@@ -1808,6 +1813,9 @@ void Cache::complete_commit(
       } else {
        assert(i->state == CachedExtent::extent_state_t::DIRTY);
       }
+      add_extent(i);
+
+      // Note: commit extents and backref allocations in the same place
       DEBUGT("backref_entry alloc existing {} len 0x{:x}",
             t,
             i->get_paddr(),
@@ -1818,7 +1826,6 @@ void Cache::complete_commit(
          i->cast<LogicalCachedExtent>()->get_laddr(),
          i->get_length(),
          i->get_type()));
-      add_extent(i);
       const auto t_src = t.get_src();
       if (i->is_dirty()) {
         add_to_dirty(i, &t_src);
@@ -1827,6 +1834,8 @@ void Cache::complete_commit(
       }
     }
   }
+
+  apply_backref_byseq(t.move_backref_entries(), start_seq);
   commit_backref_entries(std::move(backref_entries), start_seq);
 
   for (auto &i: t.pre_alloc_list) {

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index 9a590746addf0acb6106d12e4426371b9e792492..b2248ff37dd5faed4c03c7a95571711f83609c9f 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -1826,9 +1826,23 @@ private:
   seastar::metrics::metric_group metrics;
   void register_metrics();
 
+  void apply_backref_mset(
+      backref_entry_refs_t& backref_entries) {
+    for (auto& entry : backref_entries) {
+      backref_entry_mset.insert(*entry);
+    }
+  }
+
+  void apply_backref_byseq(
+      backref_entry_refs_t&& backref_entries,
+      const journal_seq_t& seq);
+
   void commit_backref_entries(
-    backref_entry_refs_t&& backref_entries,
-    const journal_seq_t& seq);
+      backref_entry_refs_t&& backref_entries,
+      const journal_seq_t& seq) {
+    apply_backref_mset(backref_entries);
+    apply_backref_byseq(std::move(backref_entries), seq);
+  }
 
   /// Add extent to extents handling dirty and refcounting
   ///

diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h
index 9b95161a404d549e0c6bc1a010ef40bb2f05ebc2..66a9f8965207c73ecb917dfbeeb4def5b6902b75 100644 (file)
--- a/src/crimson/os/seastore/transaction.h
+++ b/src/crimson/os/seastore/transaction.h
@@ -8,11 +8,12 @@
 #include <boost/intrusive/list.hpp>
 
 #include "crimson/common/log.h"
+#include "crimson/os/seastore/backref_entry.h"
+#include "crimson/os/seastore/cached_extent.h"
 #include "crimson/os/seastore/logging.h"
 #include "crimson/os/seastore/ordering_handle.h"
-#include "crimson/os/seastore/seastore_types.h"
-#include "crimson/os/seastore/cached_extent.h"
 #include "crimson/os/seastore/root_block.h"
+#include "crimson/os/seastore/seastore_types.h"
 #include "crimson/os/seastore/transaction_interruptor.h"
 
 namespace crimson::os::seastore {
@@ -460,6 +461,7 @@ public:
     ool_write_stats = {};
     rewrite_stats = {};
     conflicted = false;
+    assert(backref_entries.empty());
     if (!has_reset) {
       has_reset = true;
     }
@@ -575,6 +577,15 @@ private:
   friend class Cache;
   friend Ref make_test_transaction();
 
+  void set_backref_entries(backref_entry_refs_t&& entries) {
+    assert(backref_entries.empty());
+    backref_entries = std::move(entries);
+  }
+
+  backref_entry_refs_t move_backref_entries() {
+    return std::move(backref_entries);
+  }
+
   /**
    * If set, *this may not be used to perform writes and will not provide
    * consistentency allowing operations using to avoid maintaining a read_set.
@@ -669,6 +680,8 @@ private:
   transaction_id_t trans_id = TRANS_ID_NULL;
 
   seastar::lw_shared_ptr<rbm_pending_ool_t> pending_ool;
+
+  backref_entry_refs_t backref_entries;
 };
 using TransactionRef = Transaction::Ref;