crimson/os/seastore: replace has_delta() by is_stable_dirty()

has_mutation() states are no longer possible after prepare_record().

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

diff --git a/src/crimson/os/seastore/btree/fixed_kv_btree.h b/src/crimson/os/seastore/btree/fixed_kv_btree.h
index fef9a03114d0d74afbb0b9d487f84fafac03bd02..5b13664bb3df6d240d7e60b0c2e8a9bee4749a14 100644 (file)
--- a/src/crimson/os/seastore/btree/fixed_kv_btree.h
+++ b/src/crimson/os/seastore/btree/fixed_kv_btree.h
@@ -1237,7 +1237,7 @@ private:
       // This can only happen during init_cached_extent
       // or when backref extent being rewritten by gc space reclaiming
       if (ret->is_stable() && !ret->is_linked()) {
-        assert(ret->has_delta() || is_backref_node(ret->get_type()));
+        assert(ret->is_stable_dirty() || is_backref_node(ret->get_type()));
         init_internal(*ret);
       }
       auto meta = ret->get_meta();
@@ -1320,7 +1320,7 @@ private:
       // This can only happen during init_cached_extent
       // or when backref extent being rewritten by gc space reclaiming
       if (ret->is_stable() && !ret->is_linked()) {
-        assert(ret->has_delta() || is_backref_node(ret->get_type()));
+        assert(ret->is_stable_dirty() || is_backref_node(ret->get_type()));
         init_leaf(*ret);
       }
       auto meta = ret->get_meta();

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 8552ecbaeead116fe8c9508bca1770922fde16e0..ef0fb233e69a14c87ed85cf505e41aefbc2e38ac 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -739,7 +739,7 @@ void Cache::add_extent(CachedExtentRef ref)
 void Cache::mark_dirty(CachedExtentRef ref)
 {
   assert(ref->get_paddr().is_absolute());
-  if (ref->has_delta()) {
+  if (ref->is_stable_dirty()) {
     assert(ref->primary_ref_list_hook.is_linked());
     return;
   }
@@ -753,15 +753,13 @@ void Cache::add_to_dirty(
     CachedExtentRef ref,
     const Transaction::src_t* p_src)
 {
-  assert(ref->has_delta());
+  assert(ref->is_stable_dirty());
   assert(!ref->primary_ref_list_hook.is_linked());
   ceph_assert(ref->get_modify_time() != NULL_TIME);
   assert(ref->is_fully_loaded());
   assert(ref->get_paddr().is_absolute() ||
          ref->get_paddr().is_root());
 
-  // Note: next might not be at extent_state_t::DIRTY,
-  // also see CachedExtent::is_stable_writting()
   intrusive_ptr_add_ref(&*ref);
   dirty.push_back(*ref);
 
@@ -785,7 +783,7 @@ void Cache::remove_from_dirty(
     CachedExtentRef ref,
     const Transaction::src_t* p_src)
 {
-  assert(ref->has_delta());
+  assert(ref->is_stable_dirty());
   ceph_assert(ref->primary_ref_list_hook.is_linked());
   assert(ref->is_fully_loaded());
   assert(ref->get_paddr().is_absolute() ||
@@ -817,13 +815,11 @@ void Cache::replace_dirty(
     CachedExtentRef prev,
     const Transaction::src_t& src)
 {
-  assert(prev->has_delta());
+  assert(prev->is_stable_dirty());
   ceph_assert(prev->primary_ref_list_hook.is_linked());
   assert(prev->is_fully_loaded());
 
-  // Note: next might not be at extent_state_t::DIRTY,
-  // also see CachedExtent::is_stable_writting()
-  assert(next->has_delta());
+  assert(next->is_stable_dirty());
   assert(!next->primary_ref_list_hook.is_linked());
   ceph_assert(next->get_modify_time() != NULL_TIME);
   assert(next->is_fully_loaded());
@@ -849,7 +845,7 @@ void Cache::clear_dirty()
 {
   for (auto i = dirty.begin(); i != dirty.end(); ) {
     auto ptr = &*i;
-    assert(ptr->has_delta());
+    assert(ptr->is_stable_dirty());
     ceph_assert(ptr->primary_ref_list_hook.is_linked());
     assert(ptr->is_fully_loaded());
 
@@ -873,7 +869,7 @@ void Cache::remove_extent(
   assert(ref->is_valid());
   assert(ref->get_paddr().is_absolute() ||
          ref->get_paddr().is_root());
-  if (ref->has_delta()) {
+  if (ref->is_stable_dirty()) {
     remove_from_dirty(ref, p_src);
   } else if (!ref->is_placeholder()) {
     assert(ref->get_paddr().is_absolute());
@@ -907,12 +903,12 @@ void Cache::commit_replace_extent(
   if (is_root_type(prev->get_type())) {
     assert(prev->is_stable_clean()
       || prev->primary_ref_list_hook.is_linked());
-    if (prev->has_delta()) {
+    if (prev->is_stable_dirty()) {
       // add the new dirty root to front
       remove_from_dirty(prev, nullptr/* exclude root */);
     }
     add_to_dirty(next, nullptr/* exclude root */);
-  } else if (prev->has_delta()) {
+  } else if (prev->is_stable_dirty()) {
     replace_dirty(next, prev, t_src);
   } else {
     lru.remove_from_lru(*prev);
@@ -1573,7 +1569,7 @@ record_t Cache::prepare_record(
     // exist mutation pending extents must be in t.mutated_block_list
     add_extent(i);
     const auto t_src = t.get_src();
-    if (i->has_delta()) {
+    if (i->is_stable_dirty()) {
       add_to_dirty(i, &t_src);
     } else {
       touch_extent(*i, &t_src, t.get_cache_hint());
@@ -1818,10 +1814,10 @@ void Cache::complete_commit(
           t, is_inline, *i);
     i->invalidate_hints();
     add_extent(i);
-    assert(!i->has_delta());
     const auto t_src = t.get_src();
     touch_extent(*i, &t_src, t.get_cache_hint());
     i->complete_io();
+    assert(i->is_stable_clean());
     epm.commit_space_used(i->get_paddr(), i->get_length());
 
     // Note: commit extents and backref allocations in the same place
@@ -1861,7 +1857,7 @@ void Cache::complete_commit(
     if (!i->is_valid()) {
       continue;
     }
-    assert(i->has_delta());
+    assert(i->is_stable_dirty());
     assert(i->is_pending_io());
     assert(i->io_wait->from_state == CachedExtent::extent_state_t::EXIST_MUTATION_PENDING
            || (i->io_wait->from_state == CachedExtent::extent_state_t::MUTATION_PENDING

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index d2a03a0b9aa6fb213eb408f37e6f24136be892a3..ad4dd5c6c7b10bba51102d7c39e6f13699350051 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -222,7 +222,7 @@ public:
       ceph_assert(ret->get_type() == type);
 
       if (ret->is_stable()) {
-        if (ret->has_delta()) {
+        if (ret->is_stable_dirty()) {
           ++access_stats.trans_dirty;
           ++stats.access.trans_dirty;
         } else {
@@ -278,7 +278,7 @@ public:
 
     ceph_assert(ret->get_type() == type);
 
-    if (ret->has_delta()) {
+    if (ret->is_stable_dirty()) {
       ++access_stats.cache_dirty;
       ++stats.access.cache_dirty;
     } else {
@@ -505,7 +505,7 @@ public:
         assert(!p_extent->is_pending_in_trans(t.get_trans_id()));
         auto ret = t.maybe_add_to_read_set(p_extent);
         if (ret.added) {
-          if (p_extent->has_delta()) {
+          if (p_extent->is_stable_dirty()) {
             ++access_stats.cache_dirty;
             ++stats.access.cache_dirty;
           } else {
@@ -519,7 +519,7 @@ public:
           }
         } else {
           // already exists
-          if (p_extent->has_delta()) {
+          if (p_extent->is_stable_dirty()) {
             ++access_stats.trans_dirty;
             ++stats.access.trans_dirty;
           } else {
@@ -1295,7 +1295,7 @@ public:
 
     // journal replay should has been finished at this point,
     // Cache::root should have been inserted to the dirty list
-    assert(root->has_delta());
+    assert(root->is_stable_dirty());
     std::vector<CachedExtentRef> _dirty;
     for (auto &e : extents_index) {
       _dirty.push_back(CachedExtentRef(&e));

diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
index e3fb588f97adea58b735fc739cff12c483230a77..73c6c88b00a7b196b14b1bb20ac1a05a8526ae1c 100644 (file)
--- a/src/crimson/os/seastore/cached_extent.h
+++ b/src/crimson/os/seastore/cached_extent.h
@@ -299,7 +299,7 @@ class CachedExtent
   uint32_t last_committed_crc = 0;
 
   // Points at the prior stable version while in state MUTATION_PENDING
-  // or is rewriting (in state INITIAL_PENDING).
+  // or is rewriting (in state INITIAL_WRITE_PENDING).
   CachedExtentRef prior_instance;
 
   // time of the last modification
@@ -573,7 +573,7 @@ public:
   }
 
   bool is_stable_writting() const {
-    // mutated/INITIAL_PENDING and under-io extents are already
+    // mutated/INITIAL_WRITE_PENDING and under-io extents are already
     // stable and visible, see prepare_record().
     //
     // XXX: It might be good to mark this case as DIRTY/CLEAN from the definition,
@@ -608,19 +608,9 @@ public:
     return state == extent_state_t::INITIAL_WRITE_PENDING;
   }
 
-  /// Returns iff extent has deltas on disk or pending
-  bool has_delta() const {
-    ceph_assert(is_valid());
-    if (state == extent_state_t::INITIAL_WRITE_PENDING
-        || state == extent_state_t::CLEAN
-        || state == extent_state_t::EXIST_CLEAN) {
-      return false;
-    } else {
-      assert(state == extent_state_t::MUTATION_PENDING
-             || state == extent_state_t::DIRTY
-             || state == extent_state_t::EXIST_MUTATION_PENDING);
-      return true;
-    }
+  /// Returns iff extent is DIRTY
+  bool is_stable_dirty() const {
+    return state == extent_state_t::DIRTY;
   }
 
   // Returs true if extent is stable and clean
@@ -665,7 +655,7 @@ public:
 
   /// Return journal location of oldest relevant delta, only valid while DIRTY
   auto get_dirty_from() const {
-    ceph_assert(has_delta());
+    ceph_assert(is_stable_dirty());
     return dirty_from;
   }
 
@@ -726,7 +716,8 @@ public:
     return loaded_length;
   }
 
-  /// Returns version, get_version() == 0 iff !has_delta()
+  /// Returns version, get_version() == 0
+  /// iff CLEAN/EXIST_CLEAN/INITIAL_WRITE_PENDING
   extent_version_t get_version() const {
     return version;
   }

diff --git a/src/crimson/os/seastore/extent_placement_manager.h b/src/crimson/os/seastore/extent_placement_manager.h
index 44b58ba1abff5dce55d0717f5fd8a1c2506921fe..bcfe98811b77e599ed1dbc1389b5adcbe94efb9f 100644 (file)
--- a/src/crimson/os/seastore/extent_placement_manager.h
+++ b/src/crimson/os/seastore/extent_placement_manager.h
@@ -173,7 +173,7 @@ public:
 
   bool can_inplace_rewrite(Transaction& t,
     CachedExtentRef extent) final {
-    if (!extent->has_delta()) {
+    if (!extent->is_stable_dirty()) {
       return false;
     }
     assert(t.get_src() == transaction_type_t::TRIM_DIRTY);

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index 6b93c3bb4384ea049e1131a8329da2c26b04293f..8a7759a2225e6d72120baaa00c0a4127997e1a4b 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -629,7 +629,7 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent(
   }
 
   assert(extent->is_valid() && !extent->is_initial_pending());
-  if (extent->has_delta()) {
+  if (extent->has_mutation() || extent->is_stable_dirty()) {
     assert(extent->get_version() > 0);
     if (is_root_type(extent->get_type())) {
       // pass
@@ -639,10 +639,8 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent(
       // extent->get_version() > 1 or DIRTY
       t.get_rewrite_stats().account_dirty(extent->get_version());
     }
-    if (epm->can_inplace_rewrite(t, extent)) {
-      // FIXME: has_delta() is true for mutation pending extents
-      // which shouldn't do inplace rewrite because a pending transaction
-      // may fail.
+    if (extent->is_stable_dirty()
+        && epm->can_inplace_rewrite(t, extent)) {
       t.add_inplace_rewrite_extent(extent);
       extent->set_inplace_rewrite_generation();
       DEBUGT("rewritten as inplace rewrite -- {}", t, *extent);

diff --git a/src/test/crimson/seastore/test_seastore_cache.cc b/src/test/crimson/seastore/test_seastore_cache.cc
index 922ee496d0e056fed56944670e9d8362b4158179..9eb4073b182908f3f411f3f7feeef2a7b75d821f 100644 (file)
--- a/src/test/crimson/seastore/test_seastore_cache.cc
+++ b/src/test/crimson/seastore/test_seastore_cache.cc
@@ -256,7 +256,7 @@ TEST_F(cache_test_t, test_dirty_extent)
       }
       // submit transaction
       submit_transaction(std::move(t)).get();
-      ASSERT_TRUE(extent->has_delta());
+      ASSERT_TRUE(extent->is_stable_dirty());
       ASSERT_EQ(addr, extent->get_paddr());
       ASSERT_EQ(extent->get_version(), 1);
       ASSERT_EQ(extent->calc_crc32c(), csum2);
@@ -268,7 +268,7 @@ TEST_F(cache_test_t, test_dirty_extent)
        *t,
        addr,
        TestBlockPhysical::SIZE).unsafe_get();
-      ASSERT_TRUE(extent->has_delta());
+      ASSERT_TRUE(extent->is_stable_dirty());
       ASSERT_EQ(addr, extent->get_paddr());
       ASSERT_EQ(extent->get_version(), 1);
       ASSERT_EQ(csum2, extent->calc_crc32c());