From c32300258d2e6a599083dca4b36a2f0282fd82f7 Mon Sep 17 00:00:00 2001 From: Samuel Just Date: Wed, 1 Dec 2021 19:25:15 -0800 Subject: [PATCH] crimson/os/seastore: initialize logical pins before exposing to cache Otherwise, another task may get a reference to the extent before we've set the pin. Fixes: https://tracker.ceph.com/issues/53267 Signed-off-by: Samuel Just --- src/crimson/os/seastore/cache.cc | 30 ++-- src/crimson/os/seastore/cache.h | 128 +++++++++++++++--- .../os/seastore/transaction_manager.cc | 23 ++-- src/crimson/os/seastore/transaction_manager.h | 18 ++- 4 files changed, 145 insertions(+), 54 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 22c8af25d73..b5ce1cf2371 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -1161,12 +1161,13 @@ Cache::replay_delta( }; auto extent_fut = (delta.pversion == 0 ? // replay is not included by the cache hit metrics - get_extent_by_type( + _get_extent_by_type( delta.type, delta.paddr, delta.laddr, delta.length, - nullptr) : + nullptr, + [](CachedExtent &) {}) : _get_extent_if_cached( delta.paddr) ).handle_error( @@ -1298,14 +1299,15 @@ 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::_get_extent_by_type( extent_types_t type, paddr_t offset, laddr_t laddr, segment_off_t length, - const Transaction::src_t* p_src) + const Transaction::src_t* p_src, + extent_init_func_t &&extent_init_func) { - return [=] { + return [=, extent_init_func=std::move(extent_init_func)]() mutable { src_ext_t* p_metric_key = nullptr; src_ext_t metric_key; if (p_src) { @@ -1319,43 +1321,43 @@ Cache::get_extent_ertr::future Cache::get_extent_by_type( return get_extent_ertr::make_ready_future(); case extent_types_t::LADDR_INTERNAL: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::LADDR_LEAF: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::OMAP_INNER: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::OMAP_LEAF: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::COLL_BLOCK: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::ONODE_BLOCK_STAGED: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::OBJECT_DATA_BLOCK: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); @@ -1364,13 +1366,13 @@ Cache::get_extent_ertr::future Cache::get_extent_by_type( return get_extent_ertr::make_ready_future(); case extent_types_t::TEST_BLOCK: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).safe_then([](auto extent) { return CachedExtentRef(extent.detach(), false /* add_ref */); }); case extent_types_t::TEST_BLOCK_PHYSICAL: return get_extent( - offset, length, p_metric_key + offset, length, p_metric_key, std::move(extent_init_func) ).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 ffd7cacc50d..6ebfef1c95d 100644 --- a/src/crimson/os/seastore/cache.h +++ b/src/crimson/os/seastore/cache.h @@ -195,11 +195,12 @@ public: using get_extent_ertr = base_ertr; template using get_extent_ret = get_extent_ertr::future>; - template + template get_extent_ret get_extent( paddr_t offset, ///< [in] starting addr segment_off_t length, ///< [in] length - const src_ext_t* p_metric_key ///< [in] cache query metric key + const src_ext_t* p_metric_key, ///< [in] cache query metric key + Func &&extent_init_func ///< [in] init func for extent ) { auto cached = query_cache(offset, p_metric_key); if (!cached) { @@ -208,7 +209,8 @@ public: ret->set_paddr(offset); ret->state = CachedExtent::extent_state_t::CLEAN; add_extent(ret); - return read_extent(std::move(ret)); + return read_extent( + std::move(ret), std::forward(extent_init_func)); } // extent PRESENT in cache @@ -226,11 +228,14 @@ public: } cached->state = CachedExtent::extent_state_t::INVALID; - return read_extent(std::move(ret)); + return read_extent( + std::move(ret), std::forward(extent_init_func)); } else { auto ret = TCachedExtentRef(static_cast(cached.get())); return ret->wait_io( - ).then([ret=std::move(ret)]() mutable -> get_extent_ret { + ).then([ret=std::move(ret), + extent_init_func=std::forward(extent_init_func)]() mutable + -> get_extent_ret { // ret may be invalid, caller must check return get_extent_ret( get_extent_ertr::ready_future_marker{}, @@ -238,6 +243,16 @@ public: }); } } + template + get_extent_ret get_extent( + paddr_t offset, ///< [in] starting addr + segment_off_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 &){}); + } /** * get_extent_if_cached @@ -298,11 +313,12 @@ public: * t *must not* have retired offset */ using get_extent_iertr = base_iertr; - template + template get_extent_iertr::future> get_extent( Transaction &t, paddr_t offset, - segment_off_t length) { + segment_off_t length, + Func &&extent_init_func) { CachedExtentRef ret; LOG_PREFIX(Cache::get_extent); auto result = t.get_extent(offset, &ret); @@ -316,7 +332,9 @@ public: } else { auto metric_key = std::make_pair(t.get_src(), T::TYPE); return trans_intr::make_interruptible( - get_extent(offset, length, &metric_key) + get_extent( + offset, length, &metric_key, + std::forward(extent_init_func)) ).si_then([this, FNAME, offset, &t](auto ref) { (void)this; // silence incorrect clang warning about capture if (!ref->is_valid()) { @@ -334,6 +352,13 @@ public: }); } } + template + get_extent_iertr::future> get_extent( + Transaction &t, + paddr_t offset, + segment_off_t length) { + return get_extent(t, offset, length, [](T &){}); + } /** @@ -342,23 +367,52 @@ public: * Based on type, instantiate the correct concrete type * and read in the extent at location offset~length. */ - get_extent_ertr::future get_extent_by_type( - extent_types_t type, ///< [in] type tag - paddr_t offset, ///< [in] starting addr - laddr_t laddr, ///< [in] logical address if logical - segment_off_t length, ///< [in] length - const Transaction::src_t* p_src ///< [in] src tag for cache query metric +private: + // This is a workaround std::move_only_function not being available, + // not really worth generalizing at this time. + class extent_init_func_t { + struct callable_i { + virtual void operator()(CachedExtent &extent) = 0; + virtual ~callable_i() = default; + }; + template + struct callable_wrapper final : callable_i { + Func func; + callable_wrapper(Func &&func) : func(std::forward(func)) {} + void operator()(CachedExtent &extent) final { + return func(extent); + } + ~callable_wrapper() final = default; + }; + public: + std::unique_ptr wrapped; + template + extent_init_func_t(Func &&func) : wrapped( + std::make_unique>(std::forward(func))) + {} + void operator()(CachedExtent &extent) { + return (*wrapped)(extent); + } + }; + get_extent_ertr::future _get_extent_by_type( + extent_types_t type, + paddr_t offset, + laddr_t laddr, + segment_off_t length, + const Transaction::src_t* p_src, + extent_init_func_t &&extent_init_func ); 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_extent_by_type( Transaction &t, extent_types_t type, paddr_t offset, laddr_t laddr, - segment_off_t length) { + segment_off_t length, + extent_init_func_t &&extent_init_func) { CachedExtentRef ret; auto status = t.get_extent(offset, &ret); if (status == Transaction::get_extent_ret::RETIRED) { @@ -368,7 +422,9 @@ public: } else { auto src = t.get_src(); return trans_intr::make_interruptible( - get_extent_by_type(type, offset, laddr, length, &src) + _get_extent_by_type( + type, offset, laddr, length, &src, + std::move(extent_init_func)) ).si_then([=, &t](CachedExtentRef ret) { if (!ret->is_valid()) { LOG_PREFIX(Cache::get_extent_by_type); @@ -384,6 +440,36 @@ public: } } +public: + 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 + segment_off_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_extent_by_type_ret get_extent_by_type( + Transaction &t, + extent_types_t type, + paddr_t offset, + laddr_t laddr, + segment_off_t length + ) { + return get_extent_by_type( + t, type, offset, laddr, length, [](CachedExtent &) {}); + } + + /** * alloc_new_extent * @@ -786,9 +872,10 @@ private: /// Introspect transaction when it is being destructed void on_transaction_destruct(Transaction& t); - template + template get_extent_ret read_extent( - TCachedExtentRef&& extent + TCachedExtentRef&& extent, + Func &&func ) { extent->set_io_wait(); return reader.read( @@ -796,11 +883,12 @@ private: extent->get_length(), extent->get_bptr() ).safe_then( - [extent=std::move(extent)]() mutable { + [extent=std::move(extent), func=std::forward(func)]() mutable { /* TODO: crc should be checked against LBA manager */ extent->last_committed_crc = extent->get_crc32c(); extent->on_clean_read(); + func(*extent); extent->complete_io(); return get_extent_ertr::make_ready_future>( std::move(extent)); diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index 643becf009b..08527ff8ec3 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -437,19 +437,16 @@ TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_liv type, addr, laddr, - len).si_then( - [this, pin=std::move(pin)](CachedExtentRef ret) mutable { - auto lref = ret->cast(); - if (!lref->has_pin()) { - assert(!(pin->has_been_invalidated() || - lref->has_been_invalidated())); - lref->set_pin(std::move(pin)); - lba_manager->add_pin(lref->get_pin()); - } - return inner_ret( - interruptible::ready_future_marker{}, - ret); - }); + len, + [this, pin=std::move(pin)](CachedExtent &extent) mutable { + auto lref = extent.cast(); + if (!lref->has_pin()) { + assert(!(pin->has_been_invalidated() || + lref->has_been_invalidated())); + lref->set_pin(std::move(pin)); + lba_manager->add_pin(lref->get_pin()); + } + }); } else { return inner_ret( interruptible::ready_future_marker{}, diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index 50c5c1c1206..eede86737a6 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -148,16 +148,20 @@ public: LOG_PREFIX(TransactionManager::pin_to_extent); using ret = pin_to_extent_ret; DEBUGT("getting extent {}", t, *pin); + auto &pref = *pin; return cache->get_extent( t, - pin->get_paddr(), - pin->get_length() - ).si_then([this, FNAME, &t, pin=std::move(pin)](auto ref) mutable -> ret { - if (!ref->has_pin()) { - assert(!(pin->has_been_invalidated() || ref->has_been_invalidated())); - ref->set_pin(std::move(pin)); - lba_manager->add_pin(ref->get_pin()); + pref.get_paddr(), + pref.get_length(), + [this, pin=std::move(pin)](T &extent) mutable { + if (!extent.has_pin()) { + assert(!(extent.has_been_invalidated() || + pin->has_been_invalidated())); + extent.set_pin(std::move(pin)); + lba_manager->add_pin(extent.get_pin()); + } } + ).si_then([FNAME, &t](auto ref) mutable -> ret { DEBUGT("got extent {}", t, *ref); return pin_to_extent_ret( interruptible::ready_future_marker{}, -- 2.39.5