From: Yingxin Cheng Date: Fri, 25 Apr 2025 07:56:46 +0000 (+0800) Subject: crimson/os/seastore/cached_extent: merge is_clean() and is_dirty() as has_delta() X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=188739f7e929351fa2fbb78f0faa44f6a664b5c8;p=ceph.git 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 --- diff --git a/src/crimson/os/seastore/btree/fixed_kv_btree.h b/src/crimson/os/seastore/btree/fixed_kv_btree.h index 106cf541ba9c..66c28c795157 100644 --- a/src/crimson/os/seastore/btree/fixed_kv_btree.h +++ b/src/crimson/os/seastore/btree/fixed_kv_btree.h @@ -1237,9 +1237,7 @@ private: // 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 @@ private: // 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(); diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 4fd6ee40be72..62bb281aae90 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->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(); diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h index 121ac34c39ca..049df6f82029 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->is_dirty()) { + if (ret->has_delta()) { ++access_stats.trans_dirty; ++stats.access.trans_dirty; } else { @@ -278,7 +278,7 @@ public: 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 @@ 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->is_dirty()) { + if (p_extent->has_delta()) { ++access_stats.cache_dirty; ++stats.access.cache_dirty; } else { @@ -519,7 +519,7 @@ public: } } 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 @@ public: // 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 _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 598afd7543aa..8ba353976dba 100644 --- a/src/crimson/os/seastore/cached_extent.h +++ b/src/crimson/os/seastore/cached_extent.h @@ -590,12 +590,19 @@ public: 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 @@ public: 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 @@ public: /// 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 @@ public: 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; } diff --git a/src/crimson/os/seastore/extent_placement_manager.h b/src/crimson/os/seastore/extent_placement_manager.h index 709c46d968c6..44b58ba1abff 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->is_dirty()) { + if (!extent->has_delta()) { 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 65d70ee5e4d4..8756feaf8e91 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->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); diff --git a/src/test/crimson/seastore/test_object_data_handler.cc b/src/test/crimson/seastore/test_object_data_handler.cc index 1677762fd441..2ab65ac2a6e7 100644 --- a/src/test/crimson/seastore/test_object_data_handler.cc +++ b/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 @@ public: 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; diff --git a/src/test/crimson/seastore/test_seastore_cache.cc b/src/test/crimson/seastore/test_seastore_cache.cc index fa7748861398..922ee496d0e0 100644 --- a/src/test/crimson/seastore/test_seastore_cache.cc +++ b/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());