From 6217c2c0bb89d0e5d7dfb6981d20d29f9c57ed3e Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Wed, 21 May 2025 11:33:26 +0800 Subject: [PATCH] crimson/os/seastore: distinguish between is_mutation_pending() and has_mutation() Signed-off-by: Yingxin Cheng --- src/crimson/os/seastore/cache.cc | 4 +++- src/crimson/os/seastore/cached_extent.h | 18 +++++++++++------- src/crimson/os/seastore/object_data_handler.h | 2 +- src/crimson/os/seastore/transaction.h | 4 +++- src/crimson/os/seastore/transaction_manager.cc | 3 ++- 5 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 03cc55781383f..db2ff1ff0a615 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -1273,7 +1273,7 @@ record_t Cache::prepare_record( i->set_modify_time(commit_time); DEBUGT("mutated extent with {}B delta -- {}", t, delta_length, *i); - if (!i->is_exist_mutation_pending()) { + if (i->is_mutation_pending()) { DEBUGT("commit replace extent ... -- {}, prior={}", t, *i, *i->prior_instance); @@ -1298,6 +1298,8 @@ record_t Cache::prepare_record( // the existing extents should be added into Cache // during complete_commit to sync with gc transaction. commit_replace_extent(t, i, i->prior_instance); + } else { + assert(i->is_exist_mutation_pending()); } i->prepare_write(); diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h index 22082a48360a0..8775a69444a5d 100644 --- a/src/crimson/os/seastore/cached_extent.h +++ b/src/crimson/os/seastore/cached_extent.h @@ -439,7 +439,7 @@ public: if (!e.is_pending()) { set_prior_instance(&e); } else { - assert(e.is_mutation_pending()); + assert(e.has_mutation()); set_prior_instance(e.prior_instance); } e.get_bptr().copy_out( @@ -565,12 +565,12 @@ public: } bool is_stable_writting() const { - // MUTATION_PENDING/INITIAL_WRITE_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, // which probably can make things simpler. - return (is_mutation_pending() || is_initial_pending()) && is_pending_io(); + return (has_mutation() || is_initial_pending()) && is_pending_io(); } /// Returns true if extent is stable and shared among transactions @@ -583,11 +583,15 @@ public: } /// Returns true if extent has a pending delta - bool is_mutation_pending() const { + bool has_mutation() const { return state == extent_state_t::MUTATION_PENDING || state == extent_state_t::EXIST_MUTATION_PENDING; } + bool is_mutation_pending() const { + return state == extent_state_t::MUTATION_PENDING; + } + /// Returns true if extent is a fresh extent bool is_initial_pending() const { return state == extent_state_t::INITIAL_WRITE_PENDING; @@ -636,10 +640,10 @@ public: /// Returns true if extent or prior_instance has been invalidated bool has_been_invalidated() const { - return !is_valid() || (is_mutation_pending() && !prior_instance->is_valid()); + return !is_valid() || (prior_instance && !prior_instance->is_valid()); } - /// Returns true if extent is a plcaeholder + /// Returns true if extent is a placeholder bool is_placeholder() const { return is_retired_placeholder_type(get_type()); } @@ -1077,7 +1081,7 @@ protected: if (is_initial_pending() && addr.is_record_relative()) { return addr.block_relative_to(get_paddr()); } else { - ceph_assert(!addr.is_record_relative() || is_mutation_pending()); + ceph_assert(!addr.is_record_relative() || has_mutation()); return addr; } } diff --git a/src/crimson/os/seastore/object_data_handler.h b/src/crimson/os/seastore/object_data_handler.h index 123a7889b9157..2c3e41bf4d1d8 100644 --- a/src/crimson/os/seastore/object_data_handler.h +++ b/src/crimson/os/seastore/object_data_handler.h @@ -128,7 +128,7 @@ struct ObjectDataBlock : crimson::os::seastore::LogicalChildNode { } void prepare_commit() final { - if (is_mutation_pending() || is_exist_mutation_pending()) { + if (has_mutation()) { ceph_assert(!cached_overwrites.is_empty()); if (cached_overwrites.has_cached_bptr()) { set_bptr(cached_overwrites.move_cached_bptr()); diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h index 3a1899b41f935..e902afd998419 100644 --- a/src/crimson/os/seastore/transaction.h +++ b/src/crimson/os/seastore/transaction.h @@ -151,6 +151,7 @@ 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 // If it's already in the set, insert here will be a noop, // which is what we want. @@ -285,9 +286,10 @@ public: assert(ref->is_exist_mutation_pending() || read_set.count(ref->prior_instance->get_paddr(), extent_cmp_t{})); mutated_block_list.push_back(ref); - if (!ref->is_exist_mutation_pending()) { + if (ref->is_mutation_pending()) { write_set.insert(*ref); } else { + assert(ref->is_exist_mutation_pending()); // already added as fresh extent in write_set assert(write_set.exists(*ref)); } diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index 8756feaf8e915..6b93c3bb4384e 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -633,9 +633,10 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent( assert(extent->get_version() > 0); if (is_root_type(extent->get_type())) { // pass - } else if (extent->get_version() == 1 && extent->is_mutation_pending()) { + } else if (extent->get_version() == 1 && extent->has_mutation()) { t.get_rewrite_stats().account_n_dirty(); } else { + // extent->get_version() > 1 or DIRTY t.get_rewrite_stats().account_dirty(extent->get_version()); } if (epm->can_inplace_rewrite(t, extent)) { -- 2.39.5