From: Yingxin Cheng Date: Mon, 16 Jun 2025 06:27:03 +0000 (+0800) Subject: crimson/os/seastore: fix is_stable_written/writing() and is_stable() X-Git-Tag: testing/wip-vshankar-testing-20250625.095802-debug~4^2~3 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=64eaa3ecca3fd61c4d2d9dc34ac2abefbfce93a1;p=ceph-ci.git crimson/os/seastore: fix is_stable_written/writing() and is_stable() Signed-off-by: Yingxin Cheng --- diff --git a/src/crimson/os/seastore/btree/fixed_kv_btree.h b/src/crimson/os/seastore/btree/fixed_kv_btree.h index 5b13664bb3d..1390cf63bc8 100644 --- 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(); assert(cnode->has_parent_tracker()); diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h index fd9fa64a75d..cbfd565734c 100644 --- 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 7bb33949d96..3e3b900acb3 100644 --- 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 c26d5eaff50..fe25c7481a5 100644 --- 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 4887a186775..61415c75934 100644 --- 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(child)->is_logical()); assert( dynamic_cast(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(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 49c29710bc2..d2f1ecda73e 100644 --- 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(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(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 e902afd9984..7a1e37c43e9 100644 --- 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 e8c0e19e03c..1bfa444dc38 100644 --- 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());