From 110ae7cde5e58e05cf52c69c7c006a5e64816905 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Thu, 7 Mar 2024 15:34:06 +0800 Subject: [PATCH] crimson/os/cache: cleanup get_extent(_by_type) Distinguish caching vs absent get_extent interfaces, and misc related cleanups. After the lba parent-child pointer optimization, the absent path should be used. Signed-off-by: Yingxin Cheng (cherry picked from commit 05c916ba46b9edeb0551e1629373c64c91bcdd5f) --- src/crimson/os/seastore/cache.cc | 27 +- src/crimson/os/seastore/cache.h | 301 +++++++----------- .../crimson/seastore/test_seastore_cache.cc | 2 +- 3 files changed, 138 insertions(+), 192 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 0e5eeb96c17e2..880967031544a 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -1850,7 +1850,7 @@ Cache::replay_delta( }; auto extent_fut = (delta.pversion == 0 ? // replay is not included by the cache hit metrics - _get_extent_by_type( + do_get_caching_extent_by_type( delta.type, delta.paddr, delta.laddr, @@ -2012,7 +2012,8 @@ Cache::get_root_ret Cache::get_root(Transaction &t) } } -Cache::get_extent_ertr::future Cache::_get_extent_by_type( +Cache::get_extent_ertr::future +Cache::do_get_caching_extent_by_type( extent_types_t type, paddr_t offset, laddr_t laddr, @@ -2034,55 +2035,55 @@ Cache::get_extent_ertr::future Cache::_get_extent_by_type( ceph_assert(0 == "ROOT is never directly read"); return get_extent_ertr::make_ready_future(); case extent_types_t::BACKREF_INTERNAL: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::BACKREF_LEAF: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::LADDR_INTERNAL: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::LADDR_LEAF: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::OMAP_INNER: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::OMAP_LEAF: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::COLL_BLOCK: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::ONODE_BLOCK_STAGED: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::OBJECT_DATA_BLOCK: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); @@ -2091,13 +2092,13 @@ Cache::get_extent_ertr::future Cache::_get_extent_by_type( ceph_assert(0 == "impossible"); return get_extent_ertr::make_ready_future(); case extent_types_t::TEST_BLOCK: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::TEST_BLOCK_PHYSICAL: - return get_extent( + return do_get_caching_extent( offset, length, p_metric_key, std::move(extent_init_func), std::move(on_cache) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h index 86e63aa6c11e9..082241d408fa4 100644 --- a/src/crimson/os/seastore/cache.h +++ b/src/crimson/os/seastore/cache.h @@ -267,109 +267,6 @@ public: return t.root; } - /** - * get_extent - * - * returns ref to extent at offset~length of type T either from - * - extent_set if already in cache - * - disk - */ - using src_ext_t = std::pair; - using get_extent_ertr = base_ertr; - template - using get_extent_ret = get_extent_ertr::future>; - template - get_extent_ret get_extent( - paddr_t offset, ///< [in] starting addr - extent_len_t length, ///< [in] length - const src_ext_t* p_src_ext, ///< [in] cache query metric key - Func &&extent_init_func, ///< [in] init func for extent - OnCache &&on_cache - ) { - LOG_PREFIX(Cache::get_extent); - auto cached = query_cache(offset, p_src_ext); - if (!cached) { - auto ret = CachedExtent::make_cached_extent_ref( - alloc_cache_buf(length)); - ret->init(CachedExtent::extent_state_t::CLEAN_PENDING, - offset, - PLACEMENT_HINT_NULL, - NULL_GENERATION, - TRANS_ID_NULL); - SUBDEBUG(seastore_cache, - "{} {}~{} is absent, add extent and reading ... -- {}", - T::TYPE, offset, length, *ret); - const auto p_src = p_src_ext ? &p_src_ext->first : nullptr; - add_extent(ret, p_src); - on_cache(*ret); - extent_init_func(*ret); - return read_extent( - std::move(ret)); - } - - // extent PRESENT in cache - if (cached->get_type() == extent_types_t::RETIRED_PLACEHOLDER) { - auto ret = CachedExtent::make_cached_extent_ref( - alloc_cache_buf(length)); - ret->init(CachedExtent::extent_state_t::CLEAN_PENDING, - offset, - PLACEMENT_HINT_NULL, - NULL_GENERATION, - TRANS_ID_NULL); - SUBDEBUG(seastore_cache, - "{} {}~{} is absent(placeholder), reading ... -- {}", - T::TYPE, offset, length, *ret); - extents.replace(*ret, *cached); - on_cache(*ret); - - // replace placeholder in transactions - while (!cached->transactions.empty()) { - auto t = cached->transactions.begin()->t; - t->replace_placeholder(*cached, *ret); - } - - cached->state = CachedExtent::extent_state_t::INVALID; - extent_init_func(*ret); - return read_extent( - std::move(ret)); - } else if (!cached->is_fully_loaded()) { - auto ret = TCachedExtentRef(static_cast(cached.get())); - on_cache(*ret); - SUBDEBUG(seastore_cache, - "{} {}~{} is present without been fully loaded, reading ... -- {}", - T::TYPE, offset, length, *ret); - auto bp = alloc_cache_buf(length); - ret->set_bptr(std::move(bp)); - return read_extent( - std::move(ret)); - } else { - SUBTRACE(seastore_cache, - "{} {}~{} is present in cache -- {}", - T::TYPE, offset, length, *cached); - auto ret = TCachedExtentRef(static_cast(cached.get())); - on_cache(*ret); - return ret->wait_io( - ).then([ret=std::move(ret)]() mutable - -> get_extent_ret { - // ret may be invalid, caller must check - return get_extent_ret( - get_extent_ertr::ready_future_marker{}, - std::move(ret)); - }); - } - } - template - get_extent_ret get_extent( - paddr_t offset, ///< [in] starting addr - extent_len_t length, ///< [in] length - const src_ext_t* p_metric_key ///< [in] cache query metric key - ) { - return get_extent( - offset, length, p_metric_key, - [](T &){}, [](T &) {}); - } - - /** * get_extent_if_cached * @@ -435,7 +332,7 @@ public: } /** - * get_extent + * get_caching_extent * * returns ref to extent at offset~length of type T either from * - t if modified by t @@ -443,16 +340,19 @@ public: * - disk * * t *must not* have retired offset + * + * Note, the current implementation leverages parent-child + * pointers in LBA instead, so it should only be called in tests. */ using get_extent_iertr = base_iertr; - template - get_extent_iertr::future> get_extent( + template + get_extent_iertr::future> + get_caching_extent( Transaction &t, paddr_t offset, - extent_len_t length, - Func &&extent_init_func) { + extent_len_t length) { CachedExtentRef ret; - LOG_PREFIX(Cache::get_extent); + LOG_PREFIX(Cache::get_caching_extent); auto result = t.get_extent(offset, &ret); if (result == Transaction::get_extent_ret::RETIRED) { SUBERRORT(seastore_cache, "{} {}~{} is retired on t -- {}", @@ -485,9 +385,9 @@ public: }; auto metric_key = std::make_pair(t.get_src(), T::TYPE); return trans_intr::make_interruptible( - get_extent( - offset, length, &metric_key, - std::forward(extent_init_func), std::move(f)) + do_get_caching_extent( + offset, length, &metric_key, + [](T &){}, std::move(f)) ); } } @@ -495,9 +395,7 @@ public: /* * get_absent_extent * - * Mostly the same as Cache::get_extent(), with the only difference - * that get_absent_extent won't search the transaction's context for - * the specific CachedExtent + * The extent in query is supposed to be absent in Cache. */ template get_extent_iertr::future> get_absent_extent( @@ -524,20 +422,12 @@ public: }; auto metric_key = std::make_pair(t.get_src(), T::TYPE); return trans_intr::make_interruptible( - get_extent( + do_get_caching_extent( offset, length, &metric_key, std::forward(extent_init_func), std::move(f)) ); } - template - get_extent_iertr::future> get_extent( - Transaction &t, - paddr_t offset, - extent_len_t length) { - return get_extent(t, offset, length, [](T &){}); - } - /* * get_absent_extent * @@ -553,7 +443,9 @@ public: return get_absent_extent(t, offset, length, [](T &){}); } - get_extent_ertr::future get_extent_viewable_by_trans( + using get_extent_ertr = base_ertr; + get_extent_ertr::future + get_extent_viewable_by_trans( Transaction &t, CachedExtentRef extent) { @@ -586,7 +478,10 @@ public: } template - get_extent_ertr::future> get_extent_viewable_by_trans( + using read_extent_ret = get_extent_ertr::future>; + + template + read_extent_ret get_extent_viewable_by_trans( Transaction &t, TCachedExtentRef extent) { @@ -607,6 +502,96 @@ public: } private: + /** + * do_get_caching_extent + * + * returns ref to extent at offset~length of type T either from + * - extent_set if already in cache + * - disk + */ + using src_ext_t = std::pair; + template + read_extent_ret do_get_caching_extent( + paddr_t offset, ///< [in] starting addr + extent_len_t length, ///< [in] length + const src_ext_t* p_src_ext, ///< [in] cache query metric key + Func &&extent_init_func, ///< [in] init func for extent + OnCache &&on_cache + ) { + LOG_PREFIX(Cache::do_get_caching_extent); + auto cached = query_cache(offset, p_src_ext); + if (!cached) { + auto ret = CachedExtent::make_cached_extent_ref( + alloc_cache_buf(length)); + ret->init(CachedExtent::extent_state_t::CLEAN_PENDING, + offset, + PLACEMENT_HINT_NULL, + NULL_GENERATION, + TRANS_ID_NULL); + SUBDEBUG(seastore_cache, + "{} {}~{} is absent, add extent and reading ... -- {}", + T::TYPE, offset, length, *ret); + const auto p_src = p_src_ext ? &p_src_ext->first : nullptr; + add_extent(ret, p_src); + on_cache(*ret); + extent_init_func(*ret); + return read_extent( + std::move(ret)); + } + + // extent PRESENT in cache + if (cached->get_type() == extent_types_t::RETIRED_PLACEHOLDER) { + auto ret = CachedExtent::make_cached_extent_ref( + alloc_cache_buf(length)); + ret->init(CachedExtent::extent_state_t::CLEAN_PENDING, + offset, + PLACEMENT_HINT_NULL, + NULL_GENERATION, + TRANS_ID_NULL); + SUBDEBUG(seastore_cache, + "{} {}~{} is absent(placeholder), reading ... -- {}", + T::TYPE, offset, length, *ret); + extents.replace(*ret, *cached); + on_cache(*ret); + + // replace placeholder in transactions + while (!cached->transactions.empty()) { + auto t = cached->transactions.begin()->t; + t->replace_placeholder(*cached, *ret); + } + + cached->state = CachedExtent::extent_state_t::INVALID; + extent_init_func(*ret); + return read_extent( + std::move(ret)); + } else if (!cached->is_fully_loaded()) { + auto ret = TCachedExtentRef(static_cast(cached.get())); + on_cache(*ret); + SUBDEBUG(seastore_cache, + "{} {}~{} is present without been fully loaded, reading ... -- {}", + T::TYPE, offset, length, *ret); + auto bp = alloc_cache_buf(length); + ret->set_bptr(std::move(bp)); + return read_extent( + std::move(ret)); + } else { + SUBTRACE(seastore_cache, + "{} {}~{} is present in cache -- {}", + T::TYPE, offset, length, *cached); + auto ret = TCachedExtentRef(static_cast(cached.get())); + on_cache(*ret); + return ret->wait_io( + ).then([ret=std::move(ret)]() mutable + -> read_extent_ret { + // ret may be invalid, caller must check + return read_extent_ret( + get_extent_ertr::ready_future_marker{}, + std::move(ret)); + }); + } + } + + // This is a workaround std::move_only_function not being available, // not really worth generalizing at this time. class extent_init_func_t { @@ -633,7 +618,9 @@ private: return (*wrapped)(extent); } }; - get_extent_ertr::future _get_extent_by_type( + + get_extent_ertr::future + do_get_caching_extent_by_type( extent_types_t type, paddr_t offset, laddr_t laddr, @@ -646,7 +633,7 @@ private: using get_extent_by_type_iertr = get_extent_iertr; using get_extent_by_type_ret = get_extent_by_type_iertr::future< CachedExtentRef>; - get_extent_by_type_ret _get_extent_by_type( + get_extent_by_type_ret get_caching_extent_by_type( Transaction &t, extent_types_t type, paddr_t offset, @@ -654,7 +641,7 @@ private: extent_len_t length, extent_init_func_t &&extent_init_func ) { - LOG_PREFIX(Cache::get_extent_by_type); + LOG_PREFIX(Cache::get_caching_extent_by_type); CachedExtentRef ret; auto status = t.get_extent(offset, &ret); if (status == Transaction::get_extent_ret::RETIRED) { @@ -687,7 +674,7 @@ private: }; auto src = t.get_src(); return trans_intr::make_interruptible( - _get_extent_by_type( + do_get_caching_extent_by_type( type, offset, laddr, length, &src, std::move(extent_init_func), std::move(f)) ); @@ -721,7 +708,7 @@ private: }; auto src = t.get_src(); return trans_intr::make_interruptible( - _get_extent_by_type( + do_get_caching_extent_by_type( type, offset, laddr, length, &src, std::move(extent_init_func), std::move(f)) ); @@ -768,36 +755,13 @@ private: } public: - /** - * get_extent_by_type + /* + * get_absent_extent_by_type * * Based on type, instantiate the correct concrete type * and read in the extent at location offset~length. - */ - template - get_extent_by_type_ret get_extent_by_type( - Transaction &t, ///< [in] transaction - extent_types_t type, ///< [in] type tag - paddr_t offset, ///< [in] starting addr - laddr_t laddr, ///< [in] logical address if logical - extent_len_t length, ///< [in] length - Func &&extent_init_func ///< [in] extent init func - ) { - return _get_extent_by_type( - t, - type, - offset, - laddr, - length, - extent_init_func_t(std::forward(extent_init_func))); - } - - /* - * get_absent_extent_by_type * - * Mostly the same as Cache::get_extent_by_type(), with the only difference - * that get_absent_extent_by_type won't search the transaction's context for - * the specific CachedExtent + * The extent in query is supposed to be absent in Cache. */ template get_extent_by_type_ret get_absent_extent_by_type( @@ -817,25 +781,6 @@ public: extent_init_func_t(std::forward(extent_init_func))); } - get_extent_by_type_ret get_extent_by_type( - Transaction &t, - extent_types_t type, - paddr_t offset, - laddr_t laddr, - extent_len_t length - ) { - return get_extent_by_type( - t, type, offset, laddr, length, [](CachedExtent &) {}); - } - - - /* - * get_absent_extent_by_type - * - * Mostly the same as Cache::get_extent_by_type(), with the only difference - * that get_absent_extent_by_type won't search the transaction's context for - * the specific CachedExtent - */ get_extent_by_type_ret get_absent_extent_by_type( Transaction &t, extent_types_t type, @@ -1678,7 +1623,7 @@ private: void on_transaction_destruct(Transaction& t); template - get_extent_ret read_extent( + read_extent_ret read_extent( TCachedExtentRef&& extent ) { assert(extent->state == CachedExtent::extent_state_t::CLEAN_PENDING || @@ -1712,7 +1657,7 @@ private: }, get_extent_ertr::pass_further{}, crimson::ct_error::assert_all{ - "Cache::get_extent: invalid error" + "Cache::read_extent: invalid error" } ); } diff --git a/src/test/crimson/seastore/test_seastore_cache.cc b/src/test/crimson/seastore/test_seastore_cache.cc index 66c9899538c85..2b0b546b159a0 100644 --- a/src/test/crimson/seastore/test_seastore_cache.cc +++ b/src/test/crimson/seastore/test_seastore_cache.cc @@ -76,7 +76,7 @@ struct cache_test_t : public seastar_test_suite_t { return with_trans_intr( t, [this](auto &&... args) { - return cache->get_extent(args...); + return cache->get_caching_extent(args...); }, std::forward(args)...); } -- 2.39.5