From: Shraddha Agrawal Date: Mon, 27 Apr 2026 06:31:17 +0000 (+0530) Subject: seastore/omap_manager/btree: change omap manager funcs to coroutines X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=eae6ee94c88a2b43dfbd9968f78bf87ba022bfb9;p=ceph.git seastore/omap_manager/btree: change omap manager funcs to coroutines This commit changes funcs in BTree OMap manager to coroutines. Apart from cleaner code that's easier to follow this is done to fix ASan heap-use-after-free asserts. Example QA job with the error: https://pulpito.ceph.com/shraddhaag-2026-04-20_07:04:25-crimson-rados-main-distro-debug-trial/164374/ Signed-off-by: Shraddha Agrawal --- diff --git a/src/crimson/os/seastore/omap_manager.h b/src/crimson/os/seastore/omap_manager.h index ee2ed7624c20..8f75fc6eec2d 100644 --- a/src/crimson/os/seastore/omap_manager.h +++ b/src/crimson/os/seastore/omap_manager.h @@ -75,7 +75,7 @@ public: virtual omap_get_value_ret omap_get_value( const omap_root_t &omap_root, Transaction &t, - const std::string &key) = 0; + std::string key) = 0; /** * set key value mapping in omap @@ -91,15 +91,15 @@ public: virtual omap_set_key_ret omap_set_key( omap_root_t &omap_root, Transaction &t, - const std::string &key, - const ceph::bufferlist &value) = 0; + std::string key, + ceph::bufferlist value) = 0; using omap_set_keys_iertr = omap_set_key_iertr; using omap_set_keys_ret = omap_set_keys_iertr::future<>; virtual omap_set_keys_ret omap_set_keys( omap_root_t &omap_root, Transaction &t, - std::map&& keys) = 0; + std::map keys) = 0; /** * remove key value mapping in omap tree @@ -113,14 +113,14 @@ public: virtual omap_rm_key_ret omap_rm_key( omap_root_t &omap_root, Transaction &t, - const std::string &key) = 0; + std::string key) = 0; using omap_rm_keys_iertr = base_iertr; using omap_rm_keys_ret = omap_rm_keys_iertr::future<>; virtual omap_rm_keys_ret omap_rm_keys( omap_root_t& root, Transaction& t, - std::set& keys) = 0; + std::set keys) = 0; /** * omap_iterate diff --git a/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc b/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc index ef5ed822185d..5e004597111f 100644 --- a/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc +++ b/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.cc @@ -130,117 +130,79 @@ BtreeOMapManager::omap_get_value_ret BtreeOMapManager::omap_get_value( const omap_root_t &omap_root, Transaction &t, - const std::string &key) + std::string key) { LOG_PREFIX(BtreeOMapManager::omap_get_value); DEBUGT("key={}", t, key); - return get_omap_root( - get_omap_context(t, omap_root), - omap_root - ).si_then([this, &t, &key, &omap_root](auto&& extent) { - return extent->get_value( - get_omap_context(t, omap_root), key); - }).si_then([](auto &&e) { - return omap_get_value_ret( - interruptible::ready_future_marker{}, - std::move(e)); - }); + auto extent = co_await get_omap_root(get_omap_context(t, omap_root), omap_root); + co_return co_await extent->get_value(get_omap_context(t, omap_root), key); } BtreeOMapManager::omap_set_keys_ret BtreeOMapManager::omap_set_keys( omap_root_t &omap_root, Transaction &t, - std::map&& keys) + std::map keys) { - return seastar::do_with(std::move(keys), [&, this](auto& keys) { - return trans_intr::do_for_each( - keys.begin(), - keys.end(), - [&, this](auto &p) { - return omap_set_key(omap_root, t, p.first, p.second); - }); - }); + LOG_PREFIX(BtreeOMapManager::omap_set_keys); + for (auto &p: keys) { + DEBUGT("set key={}", t, p.first); + co_await omap_set_key(omap_root, t, p.first, p.second); + } } BtreeOMapManager::omap_set_key_ret BtreeOMapManager::omap_set_key( omap_root_t &omap_root, Transaction &t, - const std::string &key, - const ceph::bufferlist &value) + std::string key, + ceph::bufferlist value) { LOG_PREFIX(BtreeOMapManager::omap_set_key); DEBUGT("{} -> 0x{:x} value", t, key, value.length()); - // #FIXME: heap buffer overflow during logging if value is long (e.g. 1020B) + // #FIXME: heap buffer overflow during logging if value is long (e.g. 1020B) // https://tracker.ceph.com/issues/71524 // DEBUGT("{} -> {}", t, key, value); - return get_omap_root( - get_omap_context(t, omap_root), - omap_root - ).si_then([this, &t, &key, &value, &omap_root](auto root) { - return root->insert(get_omap_context( - t, omap_root), key, value); - }).si_then([this, &omap_root, &t](auto mresult) -> omap_set_key_ret { - if (mresult.status == mutation_status_t::SUCCESS) - return seastar::now(); - else if (mresult.status == mutation_status_t::WAS_SPLIT) - return handle_root_split( - get_omap_context(t, omap_root), omap_root, mresult); - else - return seastar::now(); - }); + auto root = co_await get_omap_root(get_omap_context(t, omap_root), omap_root); + auto mresult = co_await root->insert(get_omap_context(t, omap_root), key, value); + if (mresult.status == mutation_status_t::WAS_SPLIT) { + co_await handle_root_split(get_omap_context(t, omap_root), omap_root, mresult); + } } BtreeOMapManager::omap_rm_key_ret BtreeOMapManager::omap_rm_key( omap_root_t &omap_root, Transaction &t, - const std::string &key) + std::string key) { LOG_PREFIX(BtreeOMapManager::omap_rm_key); DEBUGT("{}", t, key); - return get_omap_root( - get_omap_context(t, omap_root), - omap_root - ).si_then([this, &t, &key, &omap_root](auto root) { - return root->rm_key(get_omap_context(t, omap_root), key); - }).si_then([this, &omap_root, &t](auto mresult) -> omap_rm_key_ret { - if (mresult.status == mutation_status_t::SUCCESS) { - return seastar::now(); - } else if (mresult.status == mutation_status_t::WAS_SPLIT) { - return handle_root_split( - get_omap_context(t, omap_root), omap_root, mresult); - } else if (mresult.status == mutation_status_t::NEED_MERGE) { - auto root = *(mresult.need_merge); - if (root->get_node_size() == 1 && omap_root.depth != 1) { - return handle_root_merge( - get_omap_context(t, omap_root), omap_root, mresult); - } else { - return seastar::now(); - } - } else { - return seastar::now(); + auto root = co_await get_omap_root(get_omap_context(t, omap_root), omap_root); + auto mresult = co_await root->rm_key(get_omap_context(t, omap_root), key); + + if (mresult.status == mutation_status_t::WAS_SPLIT) { + co_await handle_root_split(get_omap_context(t, omap_root), omap_root, mresult); + } else if (mresult.status == mutation_status_t::NEED_MERGE) { + auto root = *(mresult.need_merge); + if (root->get_node_size() == 1 && omap_root.depth != 1) { + co_await handle_root_merge(get_omap_context(t, omap_root), omap_root, mresult); } - }); + } } BtreeOMapManager::omap_rm_keys_ret BtreeOMapManager::omap_rm_keys( omap_root_t &omap_root, Transaction &t, - std::set& keys) + std::set keys) { + LOG_PREFIX(BtreeOMapManager::omap_rm_keys); auto type = omap_root.get_type(); - return trans_intr::do_for_each( - keys.begin(), - keys.end(), - [&t, &omap_root, type, this](auto &p) - { - LOG_PREFIX(BtreeOMapManager::omap_rm_keys); + for (auto &p: keys) { DEBUGT("{} remove key={} ...", t, type, p); - return omap_rm_key(omap_root, t, p); - }); + co_await omap_rm_key(omap_root, t, p); + } } BtreeOMapManager::omap_rm_key_range_ret diff --git a/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.h b/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.h index aa9231acf621..bb2505cc9d17 100644 --- a/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.h +++ b/src/crimson/os/seastore/omap_manager/btree/btree_omap_manager.h @@ -72,22 +72,22 @@ public: omap_get_value_ret omap_get_value( const omap_root_t &omap_root, Transaction &t, - const std::string &key) final; + std::string key) final; omap_set_key_ret omap_set_key( omap_root_t &omap_root, Transaction &t, - const std::string &key, const ceph::bufferlist &value) final; + std::string key, ceph::bufferlist value) final; omap_set_keys_ret omap_set_keys( omap_root_t &omap_root, Transaction &t, - std::map&& keys) final; + std::map keys) final; omap_rm_key_ret omap_rm_key( omap_root_t &omap_root, Transaction &t, - const std::string &key) final; + std::string key) final; omap_rm_key_range_ret omap_rm_key_range( omap_root_t &omap_root, @@ -115,7 +115,7 @@ public: omap_rm_keys_ret omap_rm_keys( omap_root_t &omap_root, Transaction &t, - std::set& keys) final; + std::set keys) final; }; using BtreeOMapManagerRef = std::unique_ptr; diff --git a/src/crimson/os/seastore/omap_manager/log/log_manager.cc b/src/crimson/os/seastore/omap_manager/log/log_manager.cc index 59e182c07094..c2833e06b996 100644 --- a/src/crimson/os/seastore/omap_manager/log/log_manager.cc +++ b/src/crimson/os/seastore/omap_manager/log/log_manager.cc @@ -55,13 +55,12 @@ LogManager::initialize_omap(Transaction &t, laddr_t hint, omap_type_t omap_type) LogManager::omap_set_keys_ret LogManager::omap_set_keys( omap_root_t &log_root, - Transaction &t, std::map&& _kvs) + Transaction &t, std::map kvs) { LOG_PREFIX(LogManager::omap_set_keys); - DEBUGT("enter kv size {}", t, _kvs.size()); + DEBUGT("enter kv size {}", t, kvs.size()); assert(log_root.get_type() == omap_type_t::LOG); - auto kvs = std::move(_kvs); auto ext = co_await log_load_extent( t, log_root.addr, BEGIN_KEY, END_KEY); ceph_assert(ext); @@ -242,7 +241,7 @@ LogManager::omap_set_key_ret LogManager::omap_set_key( omap_root_t &log_root, Transaction &t, - const std::string &key, const ceph::bufferlist &value) + std::string key, ceph::bufferlist value) { LOG_PREFIX(LogManager::omap_set_key); DEBUGT("enter k={}", t, key); @@ -401,7 +400,7 @@ LogManager::log_load_extent( LogManager::omap_get_value_ret LogManager::omap_get_value( - const omap_root_t &log_root, Transaction &t, const std::string &key) + const omap_root_t &log_root, Transaction &t, std::string key) { LOG_PREFIX(LogManager::omap_get_value); DEBUGT("key={}", t, key); @@ -673,7 +672,7 @@ LogManager::omap_rm_key_ret LogManager::omap_rm_key( omap_root_t &log_root, Transaction &t, - const std::string &key) + std::string key) { LOG_PREFIX(LogManager::omap_rm_key); DEBUGT("key={}", t, key); @@ -734,7 +733,7 @@ LogManager::omap_rm_keys_ret LogManager::omap_rm_keys( omap_root_t& log_root, Transaction& t, - std::set& keys) + std::set keys) { LOG_PREFIX(LogManager::omap_rm_keys); DEBUGT("key size={}", t, keys.size()); diff --git a/src/crimson/os/seastore/omap_manager/log/log_manager.h b/src/crimson/os/seastore/omap_manager/log/log_manager.h index b073611ba948..10a8c52ff3cb 100644 --- a/src/crimson/os/seastore/omap_manager/log/log_manager.h +++ b/src/crimson/os/seastore/omap_manager/log/log_manager.h @@ -71,14 +71,14 @@ public: * @param _kvs Batch of keys to set */ omap_set_keys_ret omap_set_keys(omap_root_t &log_root, - Transaction &t, std::map&& _kvs) final; + Transaction &t, std::map kvs) final; // see omap_set_keys omap_set_key_ret omap_set_key( omap_root_t &log_root, Transaction &t, - const std::string &key, - const ceph::bufferlist &value) final; + std::string key, + ceph::bufferlist value) final; /** * omap_get_value @@ -92,7 +92,7 @@ public: */ omap_get_value_ret omap_get_value(const omap_root_t &log_root, Transaction &t, - const std::string &key) final; + std::string key) final; /** * omap_list @@ -151,13 +151,13 @@ public: omap_rm_key_ret omap_rm_key( omap_root_t &log_root, Transaction &t, - const std::string &key) final; + std::string key) final; omap_rm_keys_ret omap_rm_keys( omap_root_t &omap_root, Transaction &t, - std::set& keys) final; + std::set keys) final; /** * omap_clear