From 23edbbd437ce1f060dbb7ae8a2f52f4bae8b0022 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Mon, 16 Jun 2025 11:42:49 +0800 Subject: [PATCH] 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 --- .../os/seastore/btree/fixed_kv_btree.h | 4 +-- src/crimson/os/seastore/cache.cc | 28 ++++++++----------- src/crimson/os/seastore/cache.h | 10 +++---- src/crimson/os/seastore/cached_extent.h | 25 ++++++----------- .../os/seastore/extent_placement_manager.h | 2 +- .../os/seastore/transaction_manager.cc | 8 ++---- .../crimson/seastore/test_seastore_cache.cc | 4 +-- 7 files changed, 33 insertions(+), 48 deletions(-) diff --git a/src/crimson/os/seastore/btree/fixed_kv_btree.h b/src/crimson/os/seastore/btree/fixed_kv_btree.h index fef9a03114d..5b13664bb3d 100644 --- 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 8552ecbaeea..ef0fb233e69 100644 --- 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 d2a03a0b9aa..ad4dd5c6c7b 100644 --- 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 _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 e3fb588f97a..73c6c88b00a 100644 --- 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 44b58ba1abf..bcfe98811b7 100644 --- 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 6b93c3bb438..8a7759a2225 100644 --- 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 922ee496d0e..9eb4073b182 100644 --- 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()); -- 2.47.3