crimson/os/seastore: fix is_stable_written/writing() and is_stable()

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 5b13664bb3df6d240d7e60b0c2e8a9bee4749a14..1390cf63bc8c0af1753a0763c971e0b364a83671 100644 (file)
--- a/src/crimson/os/seastore/btree/fixed_kv_btree.h
+++ b/src/crimson/os/seastore/btree/fixed_kv_btree.h
@@ -490,7 +490,7 @@ public:
         }
       }
       if (ret == Transaction::get_extent_ret::PRESENT) {
-        if (child_node->is_stable_written()) {
+        if (child_node->is_stable_ready()) {
           assert(child_node->is_valid());
           auto cnode = child_node->template cast<child_node_t>();
           assert(cnode->has_parent_tracker());

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index fd9fa64a75dfca80c5652fe8d1ff772774c2e5e8..cbfd565734c9427f8d01a6876c9cbce63929e346 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -489,9 +489,9 @@ public:
     if (extent->is_stable()) {
       p_extent = extent->get_transactional_view(t);
       if (p_extent != extent.get()) {
-        assert(!extent->is_stable_writting());
+        assert(!extent->is_pending_io());
         assert(p_extent->is_pending_in_trans(t.get_trans_id()));
-        assert(!p_extent->is_stable_writting());
+        assert(!p_extent->is_pending_io());
         ++access_stats.trans_pending;
         ++stats.access.trans_pending;
         if (p_extent->is_mutable()) {
@@ -536,7 +536,7 @@ public:
         needs_step_2 = !ret.is_paddr_known;
       }
     } else {
-      assert(!extent->is_stable_writting());
+      assert(!extent->is_pending_io());
       assert(extent->is_pending_in_trans(t.get_trans_id()));
       ++access_stats.trans_pending;
       ++stats.access.trans_pending;

diff --git a/src/crimson/os/seastore/cached_extent.cc b/src/crimson/os/seastore/cached_extent.cc
index 7bb33949d96abcb59e7bd7836700d87b634ba373..3e3b900acb36ac5b9ffaeed49b4a7be5d5bf4ccd 100644 (file)
--- a/src/crimson/os/seastore/cached_extent.cc
+++ b/src/crimson/os/seastore/cached_extent.cc
@@ -116,7 +116,7 @@ CachedExtent::is_viewable_by_trans(Transaction &t) {
 
   // shared by multiple transactions
   assert(t.is_in_read_set(this));
-  assert(is_stable_written());
+  assert(is_stable_ready());
 
   auto cmp = trans_spec_view_t::cmp_t();
   if (mutation_pending_extents.find(trans_id, cmp) !=

diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
index c26d5eaff50e84e0143955e8a371f616d771de76..fe25c7481a582917d37c05b16eca8743c2249bb1 100644 (file)
--- a/src/crimson/os/seastore/cached_extent.h
+++ b/src/crimson/os/seastore/cached_extent.h
@@ -549,7 +549,7 @@ public:
   }
 
   /// Returns true if extent can be mutated in an open transaction,
-  /// normally equivalent to !is_data_stable.
+  /// equivalent to !is_data_stable.
   bool is_mutable() const {
     return state == extent_state_t::INITIAL_WRITE_PENDING ||
       state == extent_state_t::MUTATION_PENDING ||
@@ -557,55 +557,56 @@ public:
   }
 
   /// Returns true if extent is part of an open transaction,
-  /// normally equivalent to !is_stable.
+  /// equivalent to !is_stable.
   bool is_pending() const {
     return is_mutable() || state == extent_state_t::EXIST_CLEAN;
   }
 
-  bool is_rewrite() {
-    return is_initial_pending() && get_prior_instance();
+  /// Returns true if extent has a pending delta
+  bool has_mutation() const {
+    return state == extent_state_t::MUTATION_PENDING
+      || state == extent_state_t::EXIST_MUTATION_PENDING;
   }
 
-  /// Returns true if extent is stable, written and shared among transactions
-  bool is_stable_written() const {
-    return state == extent_state_t::CLEAN
-           || state == extent_state_t::DIRTY;
+  bool is_mutation_pending() const {
+    return state == extent_state_t::MUTATION_PENDING;
   }
 
-  bool is_stable_writting() const {
-    // 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,
-    // which probably can make things simpler.
-    return (has_mutation() || is_initial_pending()) && is_pending_io();
+  /// Returns true if extent is a fresh extent
+  bool is_initial_pending() const {
+    return state == extent_state_t::INITIAL_WRITE_PENDING;
   }
 
-  /// Returns true if extent is stable and shared among transactions,
-  /// normally equivalent to !is_pending
-  bool is_stable() const {
-    return is_stable_written() || is_stable_writting();
+  bool is_rewrite() {
+    return is_initial_pending() && get_prior_instance();
   }
 
-  /// Returns true if extent can not be mutated,
-  /// normally equivalent to !is_mutable.
-  bool is_data_stable() const {
-    return is_stable() || is_exist_clean();
+  /// Ruturns true if data is persisted while metadata isn't
+  bool is_exist_clean() const {
+    return state == extent_state_t::EXIST_CLEAN;
   }
 
-  /// Returns true if extent has a pending delta
-  bool has_mutation() const {
-    return state == extent_state_t::MUTATION_PENDING
-      || state == extent_state_t::EXIST_MUTATION_PENDING;
+  /// Returns true if the extent with EXTIST_CLEAN is modified
+  bool is_exist_mutation_pending() const {
+    return state == extent_state_t::EXIST_MUTATION_PENDING;
   }
 
-  bool is_mutation_pending() const {
-    return state == extent_state_t::MUTATION_PENDING;
+  /// Returns true iff extent is stable (shared among transactions),
+  /// equivalent to !is_pending()
+  bool is_stable() const {
+    return state == extent_state_t::CLEAN
+        || state == extent_state_t::DIRTY;
   }
 
-  /// Returns true if extent is a fresh extent
-  bool is_initial_pending() const {
-    return state == extent_state_t::INITIAL_WRITE_PENDING;
+  /// Returns true iff extent is stable and not io-pending
+  bool is_stable_ready() const {
+    return is_stable() && !is_pending_io();
+  }
+
+  /// Returns true if extent can not be mutated,
+  /// equivalent to !is_mutable.
+  bool is_data_stable() const {
+    return is_stable() || is_exist_clean();
   }
 
   /// Returns iff extent is DIRTY
@@ -618,21 +619,11 @@ public:
     return state == extent_state_t::CLEAN;
   }
 
-  /// Returns iff extent is CLEAN and pending
+  /// Returns iff extent is CLEAN and io-pending
   bool is_stable_clean_pending() const {
     return is_stable_clean() && is_pending_io();
   }
 
-  /// Ruturns true if data is persisted while metadata isn't
-  bool is_exist_clean() const {
-    return state == extent_state_t::EXIST_CLEAN;
-  }
-
-  /// Returns true if the extent with EXTIST_CLEAN is modified
-  bool is_exist_mutation_pending() const {
-    return state == extent_state_t::EXIST_MUTATION_PENDING;
-  }
-
   /// Returns true if extent has not been superceded or retired
   bool is_valid() const {
     return state != extent_state_t::INVALID;

diff --git a/src/crimson/os/seastore/linked_tree_node.h b/src/crimson/os/seastore/linked_tree_node.h
index 4887a186775355a2d300064071e3d155f5a1fc38..61415c75934567ae8ea23d4649e3667702e7a91b 100644 (file)
--- a/src/crimson/os/seastore/linked_tree_node.h
+++ b/src/crimson/os/seastore/linked_tree_node.h
@@ -887,7 +887,7 @@ protected:
       assert(dynamic_cast<CachedExtent*>(child)->is_logical());
       assert(
        dynamic_cast<CachedExtent*>(child)->is_pending_in_trans(t.get_trans_id())
-       || me.is_stable_written());
+       || me.is_stable_ready());
       if (data_only) {
        return etvr.is_viewable_extent_data_stable(
          t, dynamic_cast<CachedExtent*>(child));

diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h
index 49c29710bc29c434ef01be51f9d253ed07451176..d2f1ecda73e2e843bfb923ab52354647d0a47982 100644 (file)
--- a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h
+++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_accessor.h
@@ -306,6 +306,7 @@ class NodeExtentAccessorT {
       assert(p_recorder->field_type() == FIELD_TYPE);
       recorder = static_cast<recorder_t*>(p_recorder);
     } else if (extent->is_stable()) {
+      assert(extent->is_stable_ready());
       state = nextent_state_t::READ_ONLY;
       // mut is empty
       assert(extent->get_recorder() == nullptr ||
@@ -356,6 +357,7 @@ class NodeExtentAccessorT {
     assert(!is_retired());
     if (state == nextent_state_t::READ_ONLY) {
       assert(extent->is_stable());
+      assert(extent->is_stable_ready());
       auto ref_recorder = recorder_t::create_for_encode(c.vb);
       recorder = static_cast<recorder_t*>(ref_recorder.get());
       extent = extent->mutate(c, std::move(ref_recorder));

diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h
index e902afd998419fd71cb6f7d6ebf8e0875df70408..7a1e37c43e96271845defa26cccdac4cbe9ac3ed 100644 (file)
--- a/src/crimson/os/seastore/transaction.h
+++ b/src/crimson/os/seastore/transaction.h
@@ -151,8 +151,8 @@ public:
       assert(read_set.count(ref->prior_instance->get_paddr(), extent_cmp_t{}));
       ref->reset_prior_instance();
     } else {
-      assert(ref->is_stable_written());
-      // && retired_set.count(ref->get_paddr()) == 0
+      ceph_assert(ref->is_stable_ready());
+      // XXX: prevent double retire -- 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.emplace(ref, trans_id);

diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc
index e8c0e19e03cb700e345213573d358de4940ed6b7..1bfa444dc38d537e7256a1b0345c7d6c442fde50 100644 (file)
--- a/src/test/crimson/seastore/test_transaction_manager.cc
+++ b/src/test/crimson/seastore/test_transaction_manager.cc
@@ -1150,7 +1150,7 @@ struct transaction_manager_test_t :
        test_mappings.alloced(pin->get_key(), *extent, t.mapping_delta);
        EXPECT_TRUE(extent->is_exist_clean());
       } else {
-       EXPECT_TRUE(extent->is_stable_written());
+       EXPECT_TRUE(extent->is_stable_ready());
       }
     } else {
       ceph_assert(t.t->is_conflicted());