crimson/os/seastore/cache: fix retiring mutation-pending extents

Mark the retiring mutation-pending extent as INVALID, and add it's
prior-instance to retired-set in order to populate transaction
invalidation correctly.

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 e1e557baf40bea8652779d7db14019d5e67d92cf..1e098f1042ea35cc461ca0d3149e6531d28a6fe9 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -393,14 +393,13 @@ void Cache::complete_commit(
 
   // Add new copy of mutated blocks, set_io_wait to block until written
   for (auto &i: t.mutated_block_list) {
+    if (!i->is_valid()) {
+      continue;
+    }
     DEBUGT("mutated {}", t, *i);
     assert(i->prior_instance);
     i->on_delta_write(final_block_start);
     i->prior_instance = CachedExtentRef();
-    if (!i->is_valid()) {
-      DEBUGT("not dirtying invalid {}", t, *i);
-      continue;
-    }
     i->state = CachedExtent::extent_state_t::DIRTY;
     if (i->version == 1 || i->get_type() == extent_types_t::ROOT) {
       i->dirty_from_or_retired_at = seq;
@@ -416,6 +415,9 @@ void Cache::complete_commit(
   }
 
   for (auto &i: t.mutated_block_list) {
+    if (!i->is_valid()) {
+      continue;
+    }
     i->complete_io();
   }
 

diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h
index 25b437f2770413abbc5245d5ef9f6d43017c9ccf..fb8aad525565c534a86c26960c297737c50cfec8 100644 (file)
--- a/src/crimson/os/seastore/transaction.h
+++ b/src/crimson/os/seastore/transaction.h
@@ -52,16 +52,24 @@ public:
 
   void add_to_retired_set(CachedExtentRef ref) {
     ceph_assert(!is_weak());
-    if (!ref->is_initial_pending()) {
+    if (ref->is_initial_pending()) {
+      // We decide not to remove it from fresh_block_list because touching this
+      // will affect relative paddrs, and it should be rare to retire a fresh
+      // extent.
+      ref->state = CachedExtent::extent_state_t::INVALID;
+      write_set.erase(*ref);
+    } else if (ref->is_mutation_pending()) {
+      ref->state = CachedExtent::extent_state_t::INVALID;
+      write_set.erase(*ref);
+      assert(ref->prior_instance);
+      retired_set.insert(ref->prior_instance);
+      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);
-    } else {
-      ref->state = CachedExtent::extent_state_t::INVALID;
-    }
-    if (ref->is_pending()) {
-      write_set.erase(*ref);
     }
   }