From: Yingxin Cheng Date: Fri, 21 Mar 2025 01:26:52 +0000 (+0800) Subject: crimson/os/seastore/transaction: cleanup add_absent/present_to_retired_set X-Git-Tag: v20.3.0~283^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=45f62dd91dea16a0ea4e95e8cea258b92a0fbd8a;p=ceph.git crimson/os/seastore/transaction: cleanup add_absent/present_to_retired_set Signed-off-by: Yingxin Cheng --- diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 4dd49caf8ca38..42407284d6634 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -62,7 +62,7 @@ Cache::retire_extent_ret Cache::retire_extent_addr( auto result = t.get_extent(addr, &ext); if (result == Transaction::get_extent_ret::PRESENT) { DEBUGT("retire {}~0x{:x} on t -- {}", t, addr, length, *ext); - t.add_to_retired_set(CachedExtentRef(&*ext)); + t.add_present_to_retired_set(CachedExtentRef(&*ext)); return retire_extent_iertr::now(); } else if (result == Transaction::get_extent_ret::RETIRED) { ERRORT("retire {}~0x{:x} failed, already retired -- {}", t, addr, length, *ext); @@ -90,8 +90,7 @@ Cache::retire_extent_ret Cache::retire_extent_addr( t, addr, length, *ext); add_extent(ext); } - t.add_to_read_set(ext); - t.add_to_retired_set(ext); + t.add_absent_to_retired_set(ext); return retire_extent_iertr::now(); } @@ -117,8 +116,7 @@ void Cache::retire_absent_extent_addr( DEBUGT("retire {}~0x{:x} as placeholder, add extent -- {}", t, addr, length, *ext); add_extent(ext); - t.add_to_read_set(ext); - t.add_to_retired_set(ext); + t.add_absent_to_retired_set(ext); } void Cache::dump_contents() diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h index 5be4cb97a35b3..0339d6af6744d 100644 --- a/src/crimson/os/seastore/cache.h +++ b/src/crimson/os/seastore/cache.h @@ -153,7 +153,7 @@ public: void retire_extent(Transaction &t, CachedExtentRef ref) { LOG_PREFIX(Cache::retire_extent); SUBDEBUGT(seastore_cache, "retire extent -- {}", t, *ref); - t.add_to_retired_set(ref); + t.add_present_to_retired_set(ref); } /// Declare paddr retired in t diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h index a3dfa7261a07d..28a28fdfadcae 100644 --- a/src/crimson/os/seastore/transaction.h +++ b/src/crimson/os/seastore/transaction.h @@ -98,6 +98,17 @@ struct rbm_pending_ool_t { * - seastore_cache logs */ class Transaction { +private: + auto lookup_read_set(CachedExtentRef ref) const { + assert(ref->is_valid()); + assert(!is_weak()); + auto it = ref->read_transactions.lower_bound( + this, read_set_item_t::trans_cmp_t()); + bool exists = + (it != ref->read_transactions.end() && it->t == this); + return std::make_pair(exists, it); + } + public: using Ref = std::unique_ptr; using on_destruct_func_t = std::function; @@ -107,39 +118,29 @@ public: RETIRED }; get_extent_ret get_extent(paddr_t addr, CachedExtentRef *out) { - LOG_PREFIX(Transaction::get_extent); - // it's possible that both write_set and retired_set contain - // this addr at the same time when addr is absolute and the - // corresponding extent is used to map existing extent on disk. - // So search write_set first. - if (auto iter = write_set.find_offset(addr); - iter != write_set.end()) { - if (out) - *out = CachedExtentRef(&*iter); - SUBTRACET(seastore_cache, "{} is present in write_set -- {}", - *this, addr, *iter); - assert(!out || (*out)->is_valid()); - return get_extent_ret::PRESENT; - } else if (retired_set.count(addr)) { - return get_extent_ret::RETIRED; - } else if ( - auto iter = read_set.find(addr); - iter != read_set.end()) { - // placeholder in read-set should be in the retired-set - // at the same time. - assert(!is_retired_placeholder_type(iter->ref->get_type())); - if (out) - *out = iter->ref; - SUBTRACET(seastore_cache, "{} is present in read_set -- {}", - *this, addr, *(iter->ref)); - return get_extent_ret::PRESENT; - } else { - return get_extent_ret::ABSENT; + auto [result, ext] = do_get_extent(addr); + // placeholder in read-set must be in the retired-set + // at the same time, user should not see a placeholder. + assert(result != get_extent_ret::PRESENT || + !is_retired_placeholder_type(ext->get_type())); + if (out && result == get_extent_ret::PRESENT) { + *out = ext; } + return result; } - void add_to_retired_set(CachedExtentRef ref) { - ceph_assert(!is_weak()); + void add_absent_to_retired_set(CachedExtentRef ref) { + add_to_read_set(ref); + add_present_to_retired_set(ref); + } + + void add_present_to_retired_set(CachedExtentRef ref) { + assert(!is_weak()); +#ifndef NDEBUG + auto [result, ext] = do_get_extent(ref->get_paddr()); + assert(result == get_extent_ret::PRESENT); + assert(ext == ref); +#endif if (ref->is_exist_clean() || ref->is_exist_mutation_pending()) { existing_block_stats.dec(ref); @@ -169,11 +170,8 @@ public: return false; } - assert(ref->is_valid()); - - auto it = ref->read_transactions.lower_bound( - this, read_set_item_t::trans_cmp_t()); - if (it != ref->read_transactions.end() && it->t == this) { + auto [exists, it] = lookup_read_set(ref); + if (exists) { return false; } @@ -189,11 +187,8 @@ public: return; } - assert(ref->is_valid()); - - auto it = ref->read_transactions.lower_bound( - this, read_set_item_t::trans_cmp_t()); - assert(it == ref->read_transactions.end() || it->t != this); + auto [exists, it] = lookup_read_set(ref); + assert(!exists); auto [iter, inserted] = read_set.emplace(this, ref); ceph_assert(inserted); @@ -583,6 +578,33 @@ private: friend class Cache; friend Ref make_test_transaction(); + std::pair do_get_extent(paddr_t addr) { + LOG_PREFIX(Transaction::do_get_extent); + // it's possible that both write_set and retired_set contain + // this addr at the same time when addr is absolute and the + // corresponding extent is used to map existing extent on disk. + // So search write_set first. + if (auto iter = write_set.find_offset(addr); + iter != write_set.end()) { + auto ret = CachedExtentRef(&*iter); + SUBTRACET(seastore_cache, "{} is present in write_set -- {}", + *this, addr, *ret); + assert(ret->is_valid()); + return {get_extent_ret::PRESENT, ret}; + } else if (retired_set.count(addr)) { + return {get_extent_ret::RETIRED, nullptr}; + } else if ( + auto iter = read_set.find(addr); + iter != read_set.end()) { + auto ret = iter->ref; + SUBTRACET(seastore_cache, "{} is present in read_set -- {}", + *this, addr, *ret); + return {get_extent_ret::PRESENT, ret}; + } else { + return {get_extent_ret::ABSENT, nullptr}; + } + } + void set_backref_entries(backref_entry_refs_t&& entries) { assert(backref_entries.empty()); backref_entries = std::move(entries);