From fdad2c066b257599eac8b8a7e773c406334c3a40 Mon Sep 17 00:00:00 2001 From: Xuehan Xu Date: Wed, 8 Jul 2026 16:05:20 +0800 Subject: [PATCH] crimson/os/seastore/transaction: new conflict/no_conflict trans synchronization Use seastar::shared_mutex to synchronize transactions when committing no_conflict ones. Specifically, this commit achieves conflict/no_conflict trans synchronization by: 1. The no_conflict trans acquires locks on all the stable extents which it modifies. 2. The conflicting trans acquires shared locks on all the stable extents which it modifies. 3. The no_conflict trans releases the locks after its committing. 4. The conflicting trans releases the shared locks once it enter the prepare pipeline phase. This commits makes sure that: 1. only transactions accessing the extents that the no_conflict trans is committing will be blocked. 2. avoid persisting extents that a committing no_conflict trans is modified Compared to the old approach, this commit's strategy is much more straightforward and understandable. This commit is more performant as the no_conflict trans only blocks the trans the modified extent set of which intersects with it, while, in the old approach, all other transactions will be blocked once a conflicting trans is blocked because it's blocked after it enters the prepare pipeline phase Signed-off-by: Xuehan Xu --- src/crimson/os/seastore/cached_extent.h | 2 + src/crimson/os/seastore/transaction.h | 21 ++++++- .../os/seastore/transaction_manager.cc | 59 ++++++++++++++++++- src/crimson/os/seastore/transaction_manager.h | 46 +++++++++++++++ 4 files changed, 126 insertions(+), 2 deletions(-) diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h index e0325c6bf60..91416f87060 100644 --- a/src/crimson/os/seastore/cached_extent.h +++ b/src/crimson/os/seastore/cached_extent.h @@ -1012,6 +1012,8 @@ private: void new_committer(Transaction &t); + seastar::shared_mutex commit_lock; + protected: trans_view_set_t mutation_pending_extents; trans_view_set_t retired_transactions; diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h index 99caa62dd80..c7ff9071af9 100644 --- a/src/crimson/os/seastore/transaction.h +++ b/src/crimson/os/seastore/transaction.h @@ -444,6 +444,26 @@ public: std::for_each(existing_block_list.begin(), existing_block_list.end(), f); } + template + void for_each_mutated_extent(F &&f) { + std::for_each( + retired_set.begin(), + retired_set.end(), + [&f](auto &link) { + std::invoke(f, *link.extent); + }); + std::for_each( + mutated_block_list.begin(), + mutated_block_list.end(), + [&f](auto &e) { + if (!e->is_valid() || + e->is_exist_mutation_pending()) { + return; + } + std::invoke(f, *e->get_prior_instance()); + }); + } + const io_stat_t& get_fresh_block_stats() const { return fresh_block_stats; } @@ -972,7 +992,6 @@ constexpr bool should_use_no_conflict_publish(const Transaction &t, return !t.force_rewrite_conflict && is_rewrite_transaction(t.get_src()); } - } #if FMT_VERSION >= 90000 diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index ae2b32cfd0d..a8d28e276f3 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -593,6 +593,43 @@ TransactionManager::update_lba_mappings( }); } +seastar::future +TransactionManager::lock_mutated_nodes( + Transaction &t) +{ + mutated_extents_set_t mutated_extents; + LOG_PREFIX(TransactionManager::lock_mutated_nodes); + t.for_each_mutated_extent( + [&mutated_extents](auto &extent) { + if (!is_root_type(extent.get_type())) { + std::ignore = mutated_extents.emplace(&extent); + } + }); + if (unlikely(mutated_extents.empty())) { + co_return mutated_extents_locker(); + } + std::vector> unique_locks; + std::vector> shared_locks; + if (auto &e = *mutated_extents.begin(); + should_use_no_conflict_publish(t, e->get_type())) { + for (auto &ext : mutated_extents) { + assert(should_use_no_conflict_publish(t, ext->get_type())); + TRACET("locking {}", t, *ext); + auto lock = co_await seastar::get_unique_lock(ext->commit_lock); + unique_locks.emplace_back(std::move(lock)); + } + } else { + for (auto &ext : mutated_extents) { + assert(!should_use_no_conflict_publish(t, ext->get_type())); + TRACET("shared locking {}", t, *ext); + auto lock = co_await seastar::get_shared_lock(ext->commit_lock); + shared_locks.emplace_back(std::move(lock)); + } + } + co_return mutated_extents_locker( + std::move(mutated_extents), std::move(unique_locks), std::move(shared_locks)); +} + TransactionManager::submit_transaction_direct_ret TransactionManager::do_submit_transaction( Transaction &tref, @@ -619,6 +656,18 @@ TransactionManager::do_submit_transaction( tref.get_phase_durations().lba_update += std::chrono::steady_clock::now() - lba_start; + // TODO: For now, we lock mutated extents after delayed ool writes + // and lba mappings updating, this is ok because: + // 1. at present, only lba/backref nodes might be modified by + // no_conflict trans; + // 2. only ool lba/backref extents' persistence needs to be sync'd + // + // In the future, when no_conflict transactions may also modify + // logical extents, we should add something like "lock_logical_mutated_extents" + // and invoke it before writing ool extents. + auto locker = co_await trans_intr::make_interruptible( + lock_mutated_nodes(tref)); + auto num_extents = allocated_extents.size(); SUBTRACET(seastore_t, "process {} allocated extents", tref, num_extents); ool_start = std::chrono::steady_clock::now(); @@ -631,6 +680,13 @@ TransactionManager::do_submit_transaction( co_await trans_intr::make_interruptible( tref.get_handle().enter(write_pipeline.prepare) ); + + // For conflicting transactions, we can release the lock + // now. Because other transactions accessing the same + // extents as the current one would be invalidated later + // in Cache::prepare_record() + locker.release_shared_lock(); + tref.get_phase_durations().prepare_enter += std::chrono::steady_clock::now() - prepare_enter_start; @@ -659,7 +715,7 @@ TransactionManager::do_submit_transaction( std::move(record), tref.get_handle(), tref.get_src(), - [this, FNAME, &tref](record_locator_t submit_result) { + [&locker, this, FNAME, &tref](record_locator_t submit_result) { SUBDEBUGT(seastore_t, "committed with {}", tref, submit_result); auto start_seq = submit_result.write_result.start_seq; journal->get_trimmer().set_journal_head(start_seq); @@ -667,6 +723,7 @@ TransactionManager::do_submit_transaction( tref, submit_result.record_block_base, start_seq); + locker.release_lock(); journal->get_trimmer().update_journal_tails( cache->get_oldest_dirty_from().value_or(start_seq), cache->get_oldest_backref_dirty_from().value_or(start_seq)); diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index 3e22411f080..a08734f0d21 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -1303,6 +1303,52 @@ private: return cache->can_drop_backref(); } + // use memory addresses as the key for comparing extents, + // this makes sure that mutexes of extents are always + // acquired in the same order, avoiding dead locks. + struct mem_addr_cmp_t { + bool operator()(const CachedExtentRef &lhs, + const CachedExtentRef &rhs) const { + return lhs.get() < rhs.get(); + } + }; + + using mutated_extents_set_t = + std::set; + struct mutated_extents_locker { + mutated_extents_set_t mutated_extents; + std::vector> unique_locks; + std::vector> shared_locks; + + mutated_extents_locker( + mutated_extents_set_t &&mutated_extents, + std::vector> &&unique_locks, + std::vector> &&shared_locks) + : mutated_extents(mutated_extents), + unique_locks(std::move(unique_locks)), + shared_locks(std::move(shared_locks)) {} + mutated_extents_locker() = default; + mutated_extents_locker(mutated_extents_locker&&) = default; + + void release_lock() { + unique_locks.clear(); + } + + void release_shared_lock() { + shared_locks.clear(); + } + + void release() { + unique_locks.clear(); + shared_locks.clear(); + } + ~mutated_extents_locker() { + release(); + } + }; + seastar::future lock_mutated_nodes( + Transaction &t); + using resolve_cursor_to_mapping_iertr = base_iertr; resolve_cursor_to_mapping_iertr::future resolve_cursor_to_mapping( -- 2.47.3