crimson/os/seastore/cached_extent: merge is_clean() and is_dirty() as has_delta()

The original names were misleading because they don't correspond to
CLEAN and DIRTY states.

Signed-off-by: Yingxin Cheng <[email protected]>

Author: cyx1231st

src/crimson/os/seastore/btree/fixed_kv_btree.h

@@ -1237,9 +1237,7 @@ class FixedKVBtree {
       // This can only happen during init_cached_extent
       // or when backref extent being rewritten by gc space reclaiming
       if (!ret->is_pending() && !ret->is_linked()) {
-        assert(ret->is_dirty()
-          || (is_backref_node(ret->get_type())
-            && ret->is_clean()));
+        assert(ret->has_delta() || is_backref_node(ret->get_type()));
         init_internal(*ret);
       }
       auto meta = ret->get_meta();
@@ -1322,9 +1320,7 @@ class FixedKVBtree {
       // This can only happen during init_cached_extent
       // or when backref extent being rewritten by gc space reclaiming
       if (!ret->is_pending() && !ret->is_linked()) {
-        assert(ret->is_dirty()
-          || (is_backref_node(ret->get_type())
-            && ret->is_clean()));
+        assert(ret->has_delta() || is_backref_node(ret->get_type()));
         init_leaf(*ret);
       }
       auto meta = ret->get_meta();

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->is_dirty()) {
+  if (ref->has_delta()) {
     assert(ref->primary_ref_list_hook.is_linked());
     return;
   }
@@ -753,7 +753,7 @@ void Cache::add_to_dirty(
     CachedExtentRef ref,
     const Transaction::src_t* p_src)
 {
-  assert(ref->is_dirty());
+  assert(ref->has_delta());
   assert(!ref->primary_ref_list_hook.is_linked());
   ceph_assert(ref->get_modify_time() != NULL_TIME);
   assert(ref->is_fully_loaded());
@@ -785,7 +785,7 @@ void Cache::remove_from_dirty(
     CachedExtentRef ref,
     const Transaction::src_t* p_src)
 {
-  assert(ref->is_dirty());
+  assert(ref->has_delta());
   ceph_assert(ref->primary_ref_list_hook.is_linked());
   assert(ref->is_fully_loaded());
   assert(ref->get_paddr().is_absolute() ||
@@ -817,13 +817,13 @@ void Cache::replace_dirty(
     CachedExtentRef prev,
     const Transaction::src_t& src)
 {
-  assert(prev->is_dirty());
+  assert(prev->has_delta());
   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->is_dirty());
+  assert(next->has_delta());
   assert(!next->primary_ref_list_hook.is_linked());
   ceph_assert(next->get_modify_time() != NULL_TIME);
   assert(next->is_fully_loaded());
@@ -849,7 +849,7 @@ void Cache::clear_dirty()
 {
   for (auto i = dirty.begin(); i != dirty.end(); ) {
     auto ptr = &*i;
-    assert(ptr->is_dirty());
+    assert(ptr->has_delta());
     ceph_assert(ptr->primary_ref_list_hook.is_linked());
     assert(ptr->is_fully_loaded());
 
@@ -873,7 +873,7 @@ void Cache::remove_extent(
   assert(ref->is_valid());
   assert(ref->get_paddr().is_absolute() ||
          ref->get_paddr().is_root());
-  if (ref->is_dirty()) {
+  if (ref->has_delta()) {
     remove_from_dirty(ref, p_src);
   } else if (!ref->is_placeholder()) {
     assert(ref->get_paddr().is_absolute());
@@ -907,12 +907,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->is_dirty()) {
+    if (prev->has_delta()) {
       // add the new dirty root to front
       remove_from_dirty(prev, nullptr/* exclude root */);
     }
     add_to_dirty(next, nullptr/* exclude root */);
-  } else if (prev->is_dirty()) {
+  } else if (prev->has_delta()) {
     replace_dirty(next, prev, t_src);
   } else {
     lru.remove_from_lru(*prev);
@@ -1548,7 +1548,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->is_dirty()) {
+    if (i->has_delta()) {
       add_to_dirty(i, &t_src);
     } else {
       touch_extent(*i, &t_src, t.get_cache_hint());
@@ -1795,7 +1795,7 @@ void Cache::complete_commit(
 	   t, is_inline, *i);
     i->invalidate_hints();
     add_extent(i);
-    assert(!i->is_dirty());
+    assert(!i->has_delta());
     const auto t_src = t.get_src();
     touch_extent(*i, &t_src, t.get_cache_hint());
     i->complete_io();

src/crimson/os/seastore/cache.h

@@ -222,7 +222,7 @@ class Cache : public ExtentTransViewRetriever {
       ceph_assert(ret->get_type() == type);
 
       if (ret->is_stable()) {
-        if (ret->is_dirty()) {
+        if (ret->has_delta()) {
           ++access_stats.trans_dirty;
           ++stats.access.trans_dirty;
         } else {
@@ -278,7 +278,7 @@ class Cache : public ExtentTransViewRetriever {
 
     ceph_assert(ret->get_type() == type);
 
-    if (ret->is_dirty()) {
+    if (ret->has_delta()) {
       ++access_stats.cache_dirty;
       ++stats.access.cache_dirty;
     } else {
@@ -505,7 +505,7 @@ class Cache : public ExtentTransViewRetriever {
         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->is_dirty()) {
+          if (p_extent->has_delta()) {
             ++access_stats.cache_dirty;
             ++stats.access.cache_dirty;
           } else {
@@ -519,7 +519,7 @@ class Cache : public ExtentTransViewRetriever {
           }
         } else {
           // already exists
-          if (p_extent->is_dirty()) {
+          if (p_extent->has_delta()) {
             ++access_stats.trans_dirty;
             ++stats.access.trans_dirty;
           } else {
@@ -1295,7 +1295,7 @@ class Cache : public ExtentTransViewRetriever {
 
     // journal replay should has been finished at this point,
     // Cache::root should have been inserted to the dirty list
-    assert(root->is_dirty());
+    assert(root->has_delta());
     std::vector<CachedExtentRef> _dirty;
     for (auto &e : extents_index) {
       _dirty.push_back(CachedExtentRef(&e));

src/crimson/os/seastore/cached_extent.h

@@ -590,12 +590,19 @@ class CachedExtent
     return state == extent_state_t::INITIAL_WRITE_PENDING;
   }
 
-  /// Returns true if extent is clean (does not have deltas on disk)
-  bool is_clean() const {
+  /// Returns iff extent has deltas on disk or pending
+  bool has_delta() const {
     ceph_assert(is_valid());
-    return state == extent_state_t::INITIAL_WRITE_PENDING ||
-           state == extent_state_t::CLEAN ||
-           state == extent_state_t::EXIST_CLEAN;
+    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;
+    }
   }
 
   // Returs true if extent is stable and clean
@@ -619,12 +626,6 @@ class CachedExtent
     return state == extent_state_t::EXIST_MUTATION_PENDING;
   }
 
-  /// Returns true if extent is dirty (has deltas on disk)
-  bool is_dirty() const {
-    ceph_assert(is_valid());
-    return !is_clean();
-  }
-
   /// Returns true if extent has not been superceded or retired
   bool is_valid() const {
     return state != extent_state_t::INVALID;
@@ -646,7 +647,7 @@ class CachedExtent
 
   /// Return journal location of oldest relevant delta, only valid while DIRTY
   auto get_dirty_from() const {
-    ceph_assert(is_dirty());
+    ceph_assert(has_delta());
     return dirty_from_or_retired_at;
   }
 
@@ -713,7 +714,7 @@ class CachedExtent
     return loaded_length;
   }
 
-  /// Returns version, get_version() == 0 iff is_clean()
+  /// Returns version, get_version() == 0 iff !has_delta()
   extent_version_t get_version() const {
     return version;
   }

src/crimson/os/seastore/extent_placement_manager.h

@@ -173,7 +173,7 @@ class RandomBlockOolWriter : public ExtentOolWriter {
 
   bool can_inplace_rewrite(Transaction& t,
     CachedExtentRef extent) final {
-    if (!extent->is_dirty()) {
+    if (!extent->has_delta()) {
       return false;
     }
     assert(t.get_src() == transaction_type_t::TRIM_DIRTY);

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->is_dirty()) {
+  if (extent->has_delta()) {
     assert(extent->get_version() > 0);
     if (is_root_type(extent->get_type())) {
       // pass
@@ -639,7 +639,7 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent(
       t.get_rewrite_stats().account_dirty(extent->get_version());
     }
     if (epm->can_inplace_rewrite(t, extent)) {
-      // FIXME: is_dirty() is true for mutation pending extents
+      // FIXME: has_delta() is true for mutation pending extents
       // which shouldn't do inplace rewrite because a pending transaction
       // may fail.
       t.add_inplace_rewrite_extent(extent);

src/test/crimson/seastore/test_object_data_handler.cc

@@ -23,7 +23,6 @@ namespace {
 
 class TestOnode final : public Onode {
   onode_layout_t layout;
-  bool dirty = false;
 
 public:
   TestOnode(uint32_t ddr, uint32_t dmr) : Onode(ddr, dmr, hobject_t()) {}
@@ -34,10 +33,9 @@ class TestOnode final : public Onode {
   void with_mutable_layout(Transaction &t, Func&& f) {
     f(layout);
   }
-  bool is_alive() const {
+  bool is_alive() const final {
     return true;
   }
-  bool is_dirty() const { return dirty; }
   laddr_t get_hint() const final {return L_ADDR_MIN; }
   ~TestOnode() final = default;
 

src/test/crimson/seastore/test_seastore_cache.cc

@@ -196,8 +196,7 @@ TEST_F(cache_test_t, test_dirty_extent)
 	  *t,
 	  reladdr,
 	  TestBlockPhysical::SIZE).unsafe_get();
-	ASSERT_TRUE(extent->is_clean());
-	ASSERT_TRUE(extent->is_pending());
+	ASSERT_TRUE(extent->is_initial_pending());
 	ASSERT_TRUE(extent->get_paddr().is_relative());
 	ASSERT_EQ(extent->get_version(), 0);
 	ASSERT_EQ(csum, extent->calc_crc32c());
@@ -239,8 +238,7 @@ TEST_F(cache_test_t, test_dirty_extent)
 	  *t2,
 	  addr,
 	  TestBlockPhysical::SIZE).unsafe_get();
-	ASSERT_TRUE(extent->is_clean());
-	ASSERT_FALSE(extent->is_pending());
+	ASSERT_FALSE(extent->is_initial_pending());
 	ASSERT_EQ(addr, extent->get_paddr());
 	ASSERT_EQ(extent->get_version(), 0);
 	ASSERT_EQ(csum, extent->calc_crc32c());
@@ -251,15 +249,14 @@ TEST_F(cache_test_t, test_dirty_extent)
 	  *t,
 	  addr,
 	  TestBlockPhysical::SIZE).unsafe_get();
-	ASSERT_TRUE(extent->is_dirty());
-	ASSERT_TRUE(extent->is_pending());
+	ASSERT_TRUE(extent->is_mutation_pending());
 	ASSERT_EQ(addr, extent->get_paddr());
 	ASSERT_EQ(extent->get_version(), 1);
 	ASSERT_EQ(csum2, extent->calc_crc32c());
       }
       // submit transaction
       submit_transaction(std::move(t)).get();
-      ASSERT_TRUE(extent->is_dirty());
+      ASSERT_TRUE(extent->has_delta());
       ASSERT_EQ(addr, extent->get_paddr());
       ASSERT_EQ(extent->get_version(), 1);
       ASSERT_EQ(extent->calc_crc32c(), csum2);
@@ -271,7 +268,7 @@ TEST_F(cache_test_t, test_dirty_extent)
 	*t,
 	addr,
 	TestBlockPhysical::SIZE).unsafe_get();
-      ASSERT_TRUE(extent->is_dirty());
+      ASSERT_TRUE(extent->has_delta());
       ASSERT_EQ(addr, extent->get_paddr());
       ASSERT_EQ(extent->get_version(), 1);
       ASSERT_EQ(csum2, extent->calc_crc32c());