From: Samuel Just Date: Mon, 10 Jun 2024 20:40:43 +0000 (+0000) Subject: crimson/.../object_context_loader: simplify obc loading X-Git-Tag: v20.0.0~1674^2~8 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=be92834ebd779d3c4ef6484e4612f8df17bef63c;p=ceph.git crimson/.../object_context_loader: simplify obc loading Because we just constructed the obc, we know that we can get an exclusive lock without blocking. Introduce ObjectContext::load_then_with_lock to take an exclusive lock unconditionally, load, downgrade (which we also know must be safe), and then run the passed function. Signed-off-by: Samuel Just --- diff --git a/src/crimson/osd/object_context.h b/src/crimson/osd/object_context.h index b1a1e8938276..ee2edaefe1dc 100644 --- a/src/crimson/osd/object_context.h +++ b/src/crimson/osd/object_context.h @@ -269,6 +269,85 @@ public: } } + /** + * load_then_with_lock + * + * Takes two functions as arguments -- load_func to be invoked + * with an exclusive lock, and func to be invoked under the + * lock type specified by the Type template argument. + * + * Caller must ensure that *this is not already locked, presumably + * by invoking load_then_with_lock immediately after construction. + * + * @param [in] load_func Function to be invoked under excl lock + * @param [in] func Function to be invoked after load_func under + * lock of type Type. + */ + template + auto load_then_with_lock(Func &&load_func, Func2 &&func) { + class lock_state_t { + tri_mutex *lock = nullptr; + bool excl = false; + + public: + lock_state_t(tri_mutex &lock) : lock(&lock), excl(true) { + ceph_assert(lock.try_lock_for_excl()); + } + lock_state_t(lock_state_t &&o) : lock(o.lock), excl(o.excl) { + o.lock = nullptr; + o.excl = false; + } + lock_state_t() = delete; + lock_state_t &operator=(lock_state_t &&o) = delete; + lock_state_t(const lock_state_t &o) = delete; + lock_state_t &operator=(const lock_state_t &o) = delete; + + void demote() { + ceph_assert(excl); + ceph_assert(lock); + if constexpr (Type == RWState::RWWRITE) { + lock->demote_to_write(); + } else if constexpr (Type == RWState::RWREAD) { + lock->demote_to_read(); + } else if constexpr (Type == RWState::RWNONE) { + lock->unlock_for_excl(); + } + excl = false; + } + + ~lock_state_t() { + if (!lock) + return; + + if constexpr (Type == RWState::RWEXCL) { + lock->unlock_for_excl(); + } else { + if (excl) { + lock->unlock_for_excl(); + return; + } + + if constexpr (Type == RWState::RWWRITE) { + lock->unlock_for_write(); + } else if constexpr (Type == RWState::RWREAD) { + lock->unlock_for_read(); + } + } + } + }; + + return seastar::do_with( + lock_state_t{lock}, + [load_func=std::move(load_func), func=std::move(func)](auto &ls) mutable { + return std::invoke( + std::move(load_func) + ).si_then([func=std::move(func), &ls]() mutable { + ls.demote(); + return std::invoke(std::move(func)); + }); + }); + } + bool empty() const { return !lock.is_acquired(); } diff --git a/src/crimson/osd/object_context_loader.cc b/src/crimson/osd/object_context_loader.cc index 03bc1e1e349f..d099f41e1581 100644 --- a/src/crimson/osd/object_context_loader.cc +++ b/src/crimson/osd/object_context_loader.cc @@ -105,28 +105,43 @@ using crimson::common::local_conf; if constexpr (track) { obc->append_to(obc_set_accessing); } - return obc->with_lock( - [existed=existed, obc=obc, func=std::move(func), this]() mutable { - return get_or_load_obc( - obc, existed - ).safe_then_interruptible(std::move(func)); - }).finally([FNAME, this, obc=std::move(obc)] { - DEBUGDPP("released object {}", dpp, obc->get_oid()); - if constexpr (track) { - obc->remove_from(obc_set_accessing); - } - }); + if (existed) { + return obc->with_lock( + [func=std::move(func), obc=ObjectContextRef(obc)] { + return std::invoke(std::move(func), obc); + } + ).finally([FNAME, this, obc=ObjectContextRef(obc)] { + DEBUGDPP("released object {}", dpp, obc->get_oid()); + if constexpr (track) { + obc->remove_from(obc_set_accessing); + } + }); + } else { + return obc->load_then_with_lock ( + [this, obc=ObjectContextRef(obc)] { + return load_obc(obc); + }, + [func=std::move(func), obc=ObjectContextRef(obc)] { + return std::invoke(std::move(func), obc); + } + ).finally([FNAME, this, obc=ObjectContextRef(obc)] { + DEBUGDPP("released object {}", dpp, obc->get_oid()); + if constexpr (track) { + obc->remove_from(obc_set_accessing); + } + }); + } } - ObjectContextLoader::load_obc_iertr::future + ObjectContextLoader::load_obc_iertr::future<> ObjectContextLoader::load_obc(ObjectContextRef obc) { LOG_PREFIX(ObjectContextLoader::load_obc); return backend.load_metadata(obc->get_oid()) .safe_then_interruptible( [FNAME, this, obc=std::move(obc)](auto md) - -> load_obc_ertr::future { + -> load_obc_ertr::future<> { const hobject_t& oid = md->os.oi.soid; DEBUGDPP("loaded obs {} for {}", dpp, md->os.oi, oid); if (oid.is_head()) { @@ -142,8 +157,8 @@ using crimson::common::local_conf; // See set_clone_ssc obc->set_clone_state(std::move(md->os)); } - DEBUGDPP("returning obc {} for {}", dpp, obc->obs.oi, obc->obs.oi.soid); - return load_obc_ertr::make_ready_future(obc); + DEBUGDPP("loaded obc {} for {}", dpp, obc->obs.oi, obc->obs.oi.soid); + return seastar::now(); }); } diff --git a/src/crimson/osd/object_context_loader.h b/src/crimson/osd/object_context_loader.h index 0d7d7fdd9359..db27e81a3e48 100644 --- a/src/crimson/osd/object_context_loader.h +++ b/src/crimson/osd/object_context_loader.h @@ -96,7 +96,6 @@ private: return load_obc_iertr::make_ready_future(obc); } - load_obc_iertr::future - load_obc(ObjectContextRef obc); + load_obc_iertr::future<> load_obc(ObjectContextRef obc); }; }