From 8a73bcfecc45e1c488c5a6966b77a5bdc318bbbb Mon Sep 17 00:00:00 2001 From: Kautilya Tripathi Date: Mon, 22 Jun 2026 08:58:43 +0530 Subject: [PATCH] crimson/osd: guard recovery clone overlap with try_lock_for_read Crimson recovery could use a neighbor clone as a clone_subset source during osd_recover_clone_overlap without checking whether that clone was safe to read. Classic OSD takes a read lock on the neighbor clone via try_lock_for_read() before using it. Under snap trim + recovery stress, recovery could clone_range from a clone that snap trim was modifying, producing wrong object content (ceph_test_rados: incorrect buffer at pos 1462272). calc_clone_subsets() and calc_head_subsets() now return clone overlap candidates in preference order only. ReplicatedRecoveryBackend locks the first usable neighbor clone from each list and holds those read locks until push/pull recovery completes. RecoveryCloneLockManager releases locks in its destructor so move-assign cannot leak a held lock and block snap trim. Fixes: https://tracker.ceph.com/issues/76499 Signed-off-by: Kautilya Tripathi --- src/crimson/osd/object_context.h | 6 + src/crimson/osd/object_metadata_helper.cc | 149 +++++++++--------- src/crimson/osd/object_metadata_helper.h | 57 ++++--- src/crimson/osd/recovery_backend.cc | 6 + src/crimson/osd/recovery_backend.h | 52 ++++++ .../osd/replicated_recovery_backend.cc | 119 +++++++++++--- src/crimson/osd/replicated_recovery_backend.h | 14 +- src/test/crimson/test_calc_subsets.cc | 64 ++++---- 8 files changed, 318 insertions(+), 149 deletions(-) diff --git a/src/crimson/osd/object_context.h b/src/crimson/osd/object_context.h index 1845cb179b3..cbf43fc38a7 100644 --- a/src/crimson/osd/object_context.h +++ b/src/crimson/osd/object_context.h @@ -234,6 +234,12 @@ public: return lock.is_acquired(); } + bool try_get_read_lock() { + return lock.try_lock_for_read(); + } + void put_read_lock() { + lock.unlock_for_read(); + } bool get_recovery_read() { if (lock.try_lock_for_read()) { recovery_read_marker = true; diff --git a/src/crimson/osd/object_metadata_helper.cc b/src/crimson/osd/object_metadata_helper.cc index 352ede567b3..9766e7117a0 100644 --- a/src/crimson/osd/object_metadata_helper.cc +++ b/src/crimson/osd/object_metadata_helper.cc @@ -16,14 +16,17 @@ namespace crimson::osd { * next older and the next newest clone obejct. * Use the existing (next) clones object overlaps instead * of pushing the whole clone object to the replica. + * + * Returns candidates in preference order (closest first). + * The caller locks the first usable candidate from each list. */ -subsets_t calc_clone_subsets( +clone_overlap_plan_t calc_clone_subsets( SnapSet& snapset, const hobject_t& soid, const pg_missing_t& missing, const hobject_t &last_backfill) { - subsets_t subsets; + clone_overlap_plan_t plan; logger().debug("{}: {} clone_overlap {} ", __func__, soid, snapset.clone_overlap); assert(missing.get_items().contains(soid)); @@ -34,17 +37,17 @@ subsets_t calc_clone_subsets( "{} {} not touched, no need to recover, skipping", __func__, soid); - return subsets; + return plan; } uint64_t size = snapset.clone_size[soid.snap]; if (size) { - subsets.data_subset.insert(0, size); + plan.data_subset.insert(0, size); } // let data_subset store only the modified content of the object. - subsets.data_subset.intersection_of(dirty_regions); + plan.data_subset.intersection_of(dirty_regions); logger().debug("{} {} data_subset {}", - __func__, soid, subsets.data_subset); + __func__, soid, plan.data_subset); // TODO: make sure CEPH_FEATURE_OSD_CACHEPOOL is not supported in Crimson // Skips clone subsets if caching was enabled (allow_incomplete_clones). @@ -52,14 +55,14 @@ subsets_t calc_clone_subsets( #ifndef UNIT_TESTS_BUILT if (!crimson::common::local_conf()->osd_recover_clone_overlap) { logger().debug("{} {} -- osd_recover_clone_overlap is disabled", - __func__, soid); ; - return subsets; + __func__, soid); + return plan; } #endif if (snapset.clones.empty()) { logger().debug("{} {} -- no clones", __func__, soid); - return subsets; + return plan; } auto soid_snap_iter = find(snapset.clones.begin(), @@ -68,8 +71,7 @@ subsets_t calc_clone_subsets( assert(soid_snap_iter != snapset.clones.end()); auto soid_snap_index = soid_snap_iter - snapset.clones.begin(); - // any overlap with next older clone? - interval_set cloning; + // older clones, closest first interval_set prev; if (size) { prev.insert(0, size); @@ -80,54 +82,42 @@ subsets_t calc_clone_subsets( // clone_overlap of i holds the overlap between i to i+1 prev.intersection_of(snapset.clone_overlap[snapset.clones[i]]); if (!missing.is_missing(clone) && clone < last_backfill) { - logger().debug("{} {} has prev {} overlap {}", + logger().debug("{} {} candidate prev {} overlap {}", + __func__, soid, clone, prev); + plan.older_candidates.push_back( + clone_candidate_t{clone, prev}); + } else { + logger().debug("{} {} does not have prev {} overlap {}", __func__, soid, clone, prev); - subsets.clone_subsets[clone] = prev; - cloning.union_of(prev); - break; } - logger().debug("{} {} does not have prev {} overlap {}", - __func__, soid, clone, prev); } - // overlap with next newest? + // newer clones, closest first interval_set next; if (size) { next.insert(0, size); } - for (unsigned i = soid_snap_index+1; + for (unsigned i = soid_snap_index + 1; i < snapset.clones.size(); i++) { hobject_t clone = soid; clone.snap = snapset.clones[i]; // clone_overlap of i-1 holds the overlap between i-1 to i next.intersection_of(snapset.clone_overlap[snapset.clones[i - 1]]); if (!missing.is_missing(clone) && clone < last_backfill) { - logger().debug("{} {} has next {} overlap {}", + logger().debug("{} {} candidate next {} overlap {}", + __func__, soid, clone, next); + plan.newer_candidates.push_back( + clone_candidate_t{clone, next}); + } else { + logger().debug("{} {} does not have next {} overlap {}", __func__, soid, clone, next); - subsets.clone_subsets[clone] = next; - cloning.union_of(next); - break; } - logger().debug("{} {} does not have next {} overlap {}", - __func__, soid, clone, next); } -#ifndef UNIT_TESTS_BUILT - if (cloning.num_intervals() > - crimson::common::local_conf().get_val - ("osd_recover_clone_overlap_limit")) { - logger().debug("skipping clone, too many holes"); - subsets.clone_subsets.clear(); - cloning.clear(); - } -#endif - - // what's left for us to push? - subsets.data_subset.subtract(cloning); - logger().debug("{} {} data_subsets {}" - "clone_subsets {}", - __func__, soid, subsets.data_subset, subsets.clone_subsets); - return subsets; + logger().debug("{} {} data_subset {} older_candidates {} newer_candidates {}", + __func__, soid, plan.data_subset, + plan.older_candidates.size(), plan.newer_candidates.size()); + return plan; } /* @@ -138,9 +128,11 @@ subsets_t calc_clone_subsets( * 2) The modified content may already overlap with the * next older clone obejct. Use the existing clone * object overlap as well. + * + * Returns older candidates in preference order (newest clone first). */ -subsets_t calc_head_subsets( +clone_overlap_plan_t calc_head_subsets( uint64_t obj_size, SnapSet& snapset, const hobject_t& head, @@ -150,18 +142,18 @@ subsets_t calc_head_subsets( logger().debug("{}: {} clone_overlap {} ", __func__, head, snapset.clone_overlap); - subsets_t subsets; + clone_overlap_plan_t plan; // 1) Calculate modified content only if (obj_size) { - subsets.data_subset.insert(0, obj_size); + plan.data_subset.insert(0, obj_size); } assert(missing.get_items().contains(head)); const pg_missing_item &missing_item = missing.get_items().at(head); // let data_subset store only the modified content of the object. - subsets.data_subset.intersection_of(missing_item.clean_regions.get_dirty_regions()); + plan.data_subset.intersection_of(missing_item.clean_regions.get_dirty_regions()); logger().debug("{} {} data_subset {}", - __func__, head, subsets.data_subset); + __func__, head, plan.data_subset); // TODO: make sure CEPH_FEATURE_OSD_CACHEPOOL is not supported in Crimson // Skips clone subsets if caching was enabled (allow_incomplete_clones). @@ -170,17 +162,16 @@ subsets_t calc_head_subsets( if (!crimson::common::local_conf()->osd_recover_clone_overlap) { logger().debug("{} {} -- osd_recover_clone_overlap is disabled", __func__, head); - return subsets; + return plan; } #endif if (snapset.clones.empty()) { logger().debug("{} {} -- no clones", __func__, head); - return subsets; + return plan; } - // 2) Find any overlap with next older clone - interval_set cloning; + // 2) Candidates: next older clones, newest first interval_set prev; hobject_t clone = head; if (obj_size) { @@ -191,39 +182,45 @@ subsets_t calc_head_subsets( // let prev store only the overlap with clone i prev.intersection_of(snapset.clone_overlap[snapset.clones[i]]); if (!missing.is_missing(clone) && clone < last_backfill) { - logger().debug("{} {} has prev {} overlap {}", + interval_set overlap = prev; + overlap.intersection_of(plan.data_subset); + if (overlap.empty()) { + logger().debug("{} {} candidate prev {} overlap empty after data_subset", + __func__, head, clone); + } else { + logger().debug("{} {} candidate prev {} overlap {}", + __func__, head, clone, overlap); + plan.older_candidates.push_back( + clone_candidate_t{clone, std::move(overlap)}); + } + } else { + logger().debug("{} {} does not have prev {} overlap {}", __func__, head, clone, prev); - cloning = prev; - break; } - logger().debug("{} {} does not have prev {} overlap {}", - __func__, head, clone, prev); } - // let cloning store only the overlap with data_subset - cloning.intersection_of(subsets.data_subset); - if (cloning.empty()) { - logger().debug("skipping clone, nothing needs to clone"); - return subsets; - } + logger().debug("{} {} data_subset {} older_candidates {}", + __func__, head, plan.data_subset, + plan.older_candidates.size()); + return plan; +} -#ifndef UNIT_TESTS_BUILT - if (cloning.num_intervals() > - crimson::common::local_conf().get_val - ("osd_recover_clone_overlap_limit")) { - logger().debug("skipping clone, too many holes"); - subsets.clone_subsets.clear(); - cloning.clear(); +subsets_t commit_subsets_unlocked(clone_overlap_plan_t plan) +{ + subsets_t subsets; + subsets.data_subset = std::move(plan.data_subset); + interval_set cloning; + if (!plan.older_candidates.empty()) { + const auto& c = plan.older_candidates.front(); + subsets.clone_subsets[c.clone] = c.overlap; + cloning.union_of(c.overlap); + } + if (!plan.newer_candidates.empty()) { + const auto& c = plan.newer_candidates.front(); + subsets.clone_subsets[c.clone] = c.overlap; + cloning.union_of(c.overlap); } -#endif - - // what's left for us to push? - subsets.clone_subsets[clone] = cloning; subsets.data_subset.subtract(cloning); - logger().debug("{} {} data_subsets {}" - "clone_subsets {}", - __func__, head, subsets.data_subset, subsets.clone_subsets); - return subsets; } diff --git a/src/crimson/osd/object_metadata_helper.h b/src/crimson/osd/object_metadata_helper.h index 927fc488811..1e6fc13444e 100644 --- a/src/crimson/osd/object_metadata_helper.h +++ b/src/crimson/osd/object_metadata_helper.h @@ -1,24 +1,45 @@ #pragma once +#include + #include "osd/osd_types_fmt.h" namespace crimson::osd { - struct subsets_t { - interval_set data_subset; - std::map> clone_subsets; - }; - - subsets_t calc_clone_subsets( - SnapSet& snapset, const hobject_t& soid, - const pg_missing_t& missing, - const hobject_t &last_backfill); - subsets_t calc_head_subsets( - uint64_t obj_size, - SnapSet& snapset, - const hobject_t& head, - const pg_missing_t& missing, - const hobject_t &last_backfill); - void set_subsets( - const subsets_t& subsets, - ObjectRecoveryInfo& recovery_info); + +struct clone_candidate_t { + hobject_t clone; + interval_set overlap; +}; + +/// Pure overlap plan: candidates in preference order (best-first). +/// Locking / committing a candidate is the caller's responsibility. +struct clone_overlap_plan_t { + interval_set data_subset; + std::vector older_candidates; + std::vector newer_candidates; +}; + +struct subsets_t { + interval_set data_subset; + std::map> clone_subsets; +}; + +clone_overlap_plan_t calc_clone_subsets( + SnapSet& snapset, const hobject_t& soid, + const pg_missing_t& missing, + const hobject_t &last_backfill); +clone_overlap_plan_t calc_head_subsets( + uint64_t obj_size, + SnapSet& snapset, + const hobject_t& head, + const pg_missing_t& missing, + const hobject_t &last_backfill); + +/// Build final subsets from a plan, taking the first candidate in each +/// list (no locking). Used by unit tests. +subsets_t commit_subsets_unlocked(clone_overlap_plan_t plan); + +void set_subsets( + const subsets_t& subsets, + ObjectRecoveryInfo& recovery_info); } diff --git a/src/crimson/osd/recovery_backend.cc b/src/crimson/osd/recovery_backend.cc index 1709f56c034..d40fb1704d3 100644 --- a/src/crimson/osd/recovery_backend.cc +++ b/src/crimson/osd/recovery_backend.cc @@ -60,6 +60,12 @@ void RecoveryBackend::clean_up(ceph::os::Transaction& t, replica_push_targets.clear(); for (auto& [soid, recovery_waiter] : recovering) { + for (auto& kv : recovery_waiter->pushing) { + kv.second.clone_lock_manager.release_locks(); + } + if (recovery_waiter->pull_info) { + recovery_waiter->pull_info->clone_lock_manager.release_locks(); + } if (recovery_waiter->obc) { recovery_waiter->obc->interrupt( ::crimson::common::actingset_changed( diff --git a/src/crimson/osd/recovery_backend.h b/src/crimson/osd/recovery_backend.h index 5e164384e43..9ef47858573 100644 --- a/src/crimson/osd/recovery_backend.h +++ b/src/crimson/osd/recovery_backend.h @@ -24,6 +24,56 @@ class PG; class PGRecovery; +/// holds read locks on neighbor clones used for clone overlap recovery +class RecoveryCloneLockManager { + std::map locks; + + void unlock_all() { + for (auto& kv : locks) { + kv.second->put_read_lock(); + } + locks.clear(); + } +public: + RecoveryCloneLockManager() = default; + RecoveryCloneLockManager(RecoveryCloneLockManager&& o) noexcept + : locks(std::move(o.locks)) {} + RecoveryCloneLockManager(const RecoveryCloneLockManager&) = delete; + RecoveryCloneLockManager& operator=(RecoveryCloneLockManager&& o) noexcept { + if (this != &o) { + unlock_all(); + locks = std::move(o.locks); + } + return *this; + } + RecoveryCloneLockManager& operator=(const RecoveryCloneLockManager&) = delete; + + bool try_lock_for_read( + const hobject_t& hoid, + ObjectContextRegistry& registry) { + if (locks.count(hoid)) { + return false; + } + auto obc = registry.maybe_get_cached_obc(hoid); + if (!obc || !obc->try_get_read_lock()) { + return false; + } + locks.emplace(hoid, std::move(obc)); + return true; + } + + void release_locks() { + unlock_all(); + } + + ~RecoveryCloneLockManager() { + // Safe to unlock from the dtor: unlock_for_read() only decrements + // and wakes waiters. Also covers move-assign replacing a non-empty + // target without an explicit release_locks() call. + unlock_all(); + } +}; + class RecoveryBackend { public: class WaitForObjectRecovery; @@ -151,6 +201,7 @@ protected: crimson::osd::ObjectContextRef head_ctx; crimson::osd::ObjectContextRef obc; object_stat_sum_t stat; + RecoveryCloneLockManager clone_lock_manager; bool is_complete() const { return recovery_progress.is_complete(recovery_info); } @@ -161,6 +212,7 @@ protected: ObjectRecoveryInfo recovery_info; crimson::osd::ObjectContextRef obc; object_stat_sum_t stat; + RecoveryCloneLockManager clone_lock_manager; }; public: diff --git a/src/crimson/osd/replicated_recovery_backend.cc b/src/crimson/osd/replicated_recovery_backend.cc index 6d43707e14f..445f753b26e 100644 --- a/src/crimson/osd/replicated_recovery_backend.cc +++ b/src/crimson/osd/replicated_recovery_backend.cc @@ -134,6 +134,12 @@ ReplicatedRecoveryBackend::maybe_push_shards( if (recovery.obc) { recovery.obc->drop_recovery_read(); } + for (auto& kv : recovery.pushing) { + kv.second.clone_lock_manager.release_locks(); + } + if (recovery.pull_info) { + recovery.pull_info->clone_lock_manager.release_locks(); + } recovering.erase(soid); return seastar::make_exception_future<>(e); }); @@ -386,6 +392,7 @@ ReplicatedRecoveryBackend::prep_push_to_replica( auto& obc = recovery_waiter.obc; SnapSet push_info_ss; // only populated if soid is_snap() crimson::osd::subsets_t subsets; + RecoveryCloneLockManager clone_lock_manager; const auto& missing = pg.get_shard_missing().find(pg_shard)->second; @@ -413,30 +420,35 @@ ReplicatedRecoveryBackend::prep_push_to_replica( push_info_ss = ssc->snapset; DEBUGDPP("snapset is {}", pg, ssc->snapset); - subsets = crimson::osd::calc_clone_subsets( - ssc->snapset, soid, - missing, - // get_peer_info() asserts `peer_info` existence. - pg.get_peering_state().get_peer_info( - pg_shard).last_backfill); + subsets = commit_clone_overlap_plan( + crimson::osd::calc_clone_subsets( + ssc->snapset, soid, + missing, + // get_peer_info() asserts `peer_info` existence. + pg.get_peering_state().get_peer_info( + pg_shard).last_backfill), + clone_lock_manager); } else if (soid.snap == CEPH_NOSNAP) { // pushing head or unversioned object. // base this on partially on replica's clones? auto ssc = obc->ssc; ceph_assert(ssc); DEBUGDPP("snapset is {}", pg, ssc->snapset); - subsets = crimson::osd::calc_head_subsets( - obc->obs.oi.size, - ssc->snapset, soid, - missing, - pg.get_peering_state().get_peer_info( - pg_shard).last_backfill); + subsets = commit_clone_overlap_plan( + crimson::osd::calc_head_subsets( + obc->obs.oi.size, + ssc->snapset, soid, + missing, + pg.get_peering_state().get_peer_info( + pg_shard).last_backfill), + clone_lock_manager); } return prep_push(soid, need, pg_shard, subsets, - push_info_ss); + push_info_ss, + std::move(clone_lock_manager)); } RecoveryBackend::interruptible_future @@ -445,7 +457,8 @@ ReplicatedRecoveryBackend::prep_push( eversion_t need, pg_shard_t pg_shard, const crimson::osd::subsets_t& subsets, - const SnapSet push_info_ss) + const SnapSet push_info_ss, + RecoveryCloneLockManager&& clone_lock_manager) { LOG_PREFIX(ReplicatedRecoveryBackend::prep_push); DEBUGDPP("{}, {}", pg, soid, need); @@ -459,6 +472,7 @@ ReplicatedRecoveryBackend::prep_push( assert(missing_iter != pmissing_iter->second.get_items().end()); push_info.obc = obc; + push_info.clone_lock_manager = std::move(clone_lock_manager); push_info.recovery_info.size = obc->obs.oi.size; push_info.recovery_info.copy_subset = subsets.data_subset; push_info.recovery_info.clone_subset = subsets.clone_subsets; @@ -503,7 +517,7 @@ void ReplicatedRecoveryBackend::prepare_pull( pg_shard_t fromshard = *(iter); pull_op.recovery_info = - set_recovery_info(soid, head_obc->ssc); + set_recovery_info(soid, head_obc->ssc, pull_info.clone_lock_manager); pull_op.soid = soid; pull_op.recovery_progress.data_complete = false; pull_op.recovery_progress.omap_complete = @@ -520,7 +534,8 @@ void ReplicatedRecoveryBackend::prepare_pull( ObjectRecoveryInfo ReplicatedRecoveryBackend::set_recovery_info( const hobject_t& soid, - const crimson::osd::SnapSetContextRef ssc) + const crimson::osd::SnapSetContextRef ssc, + RecoveryCloneLockManager& clone_lock_manager) { LOG_PREFIX(ReplicatedRecoveryBackend::set_recovery_info); pg_missing_tracker_t local_missing = pg.get_local_missing(); @@ -530,8 +545,10 @@ ObjectRecoveryInfo ReplicatedRecoveryBackend::set_recovery_info( assert(!local_missing.is_missing(soid.get_head())); assert(ssc); recovery_info.ss = ssc->snapset; - auto subsets = crimson::osd::calc_clone_subsets( - ssc->snapset, soid, local_missing, pg.get_info().last_backfill); + auto subsets = commit_clone_overlap_plan( + crimson::osd::calc_clone_subsets( + ssc->snapset, soid, local_missing, pg.get_info().last_backfill), + clone_lock_manager); crimson::osd::set_subsets(subsets, recovery_info); DEBUGDPP("pulling {}", pg, recovery_info); ceph_assert(ssc->snapset.clone_size.count(soid.snap)); @@ -892,7 +909,8 @@ ReplicatedRecoveryBackend::_handle_pull_response( if (pull_info.recovery_info.soid.snap && pull_info.recovery_info.soid.snap < CEPH_NOSNAP) { recalc_subsets(pull_info.recovery_info, - ssc); + ssc, + pull_info.clone_lock_manager); } pull_info.obc = recovery_waiter.obc = @@ -931,6 +949,7 @@ ReplicatedRecoveryBackend::_handle_pull_response( if (complete) { pull_info.stat.num_objects_recovered++; + pull_info.clone_lock_manager.release_locks(); auto manager = pg.obc_loader.get_obc_manager( recovery_waiter.obc); manager.lock_excl_sync(); /* cannot already be locked */ @@ -959,15 +978,67 @@ ReplicatedRecoveryBackend::_handle_pull_response( void ReplicatedRecoveryBackend::recalc_subsets( ObjectRecoveryInfo& recovery_info, - crimson::osd::SnapSetContextRef ssc) + crimson::osd::SnapSetContextRef ssc, + RecoveryCloneLockManager& clone_lock_manager) { assert(ssc); - auto subsets = crimson::osd::calc_clone_subsets( - ssc->snapset, recovery_info.soid, pg.get_local_missing(), - pg.get_info().last_backfill); + clone_lock_manager.release_locks(); + auto subsets = commit_clone_overlap_plan( + crimson::osd::calc_clone_subsets( + ssc->snapset, recovery_info.soid, pg.get_local_missing(), + pg.get_info().last_backfill), + clone_lock_manager); crimson::osd::set_subsets(subsets, recovery_info); } +subsets_t ReplicatedRecoveryBackend::commit_clone_overlap_plan( + clone_overlap_plan_t plan, + RecoveryCloneLockManager& clone_lock_manager) +{ + LOG_PREFIX(ReplicatedRecoveryBackend::commit_clone_overlap_plan); + subsets_t subsets; + subsets.data_subset = std::move(plan.data_subset); + interval_set cloning; + + auto try_commit = [&](std::vector& candidates) { + for (auto& c : candidates) { + // missing was already filtered in calc_*; re-check local missing + // in case the set changed, then try the read lock. + if (pg.get_local_missing().is_missing(c.clone)) { + DEBUGDPP("skip candidate {}, now missing", pg, c.clone); + continue; + } + if (!clone_lock_manager.try_lock_for_read(c.clone, pg.obc_registry)) { + DEBUGDPP("skip candidate {}, cannot lock for read", pg, c.clone); + continue; + } + DEBUGDPP("locked candidate {} overlap {}", pg, c.clone, c.overlap); + subsets.clone_subsets[c.clone] = c.overlap; + cloning.union_of(c.overlap); + return; + } + }; + + try_commit(plan.older_candidates); + try_commit(plan.newer_candidates); + +#ifndef UNIT_TESTS_BUILT + if (cloning.num_intervals() > + crimson::common::local_conf().get_val( + "osd_recover_clone_overlap_limit")) { + DEBUGDPP("skipping clone, too many holes", pg); + clone_lock_manager.release_locks(); + subsets.clone_subsets.clear(); + cloning.clear(); + } +#endif + + subsets.data_subset.subtract(cloning); + DEBUGDPP("data_subset {} clone_subsets {}", + pg, subsets.data_subset, subsets.clone_subsets); + return subsets; +} + RecoveryBackend::interruptible_future<> ReplicatedRecoveryBackend::handle_pull_response( Ref m) @@ -1122,6 +1193,7 @@ ReplicatedRecoveryBackend::_handle_push_reply( }).handle_exception_interruptible( [recovering_iter, &push_info, peer] (auto e) { push_info.recovery_progress.error = true; + push_info.clone_lock_manager.release_locks(); recovering_iter->second->set_push_failed(peer, e); return seastar::make_ready_future>(); }); @@ -1131,6 +1203,7 @@ ReplicatedRecoveryBackend::_handle_push_reply( soid, push_info.recovery_info); } + push_info.clone_lock_manager.release_locks(); recovering_iter->second->set_pushed(peer); return seastar::make_ready_future>(); } diff --git a/src/crimson/osd/replicated_recovery_backend.h b/src/crimson/osd/replicated_recovery_backend.h index 2b144a08b1b..ce9563ccb69 100644 --- a/src/crimson/osd/replicated_recovery_backend.h +++ b/src/crimson/osd/replicated_recovery_backend.h @@ -49,7 +49,8 @@ protected: eversion_t need, pg_shard_t pg_shard, const crimson::osd::subsets_t& subsets, - const SnapSet push_info_ss); + const SnapSet push_info_ss, + RecoveryCloneLockManager&& clone_lock_manager = {}); void prepare_pull( const crimson::osd::ObjectContextRef &head_obc, PullOp& pull_op, @@ -58,7 +59,13 @@ protected: eversion_t need); ObjectRecoveryInfo set_recovery_info( const hobject_t& soid, - const crimson::osd::SnapSetContextRef ssc); + const crimson::osd::SnapSetContextRef ssc, + RecoveryCloneLockManager& clone_lock_manager); + /// Lock the first usable candidate from each preference list and + /// build final recovery subsets. Lock policy stays in the backend. + subsets_t commit_clone_overlap_plan( + clone_overlap_plan_t plan, + RecoveryCloneLockManager& clone_lock_manager); std::vector get_shards_to_push( const hobject_t& soid) const; interruptible_future build_push_op( @@ -73,7 +80,8 @@ protected: PullOp* response); void recalc_subsets( ObjectRecoveryInfo& recovery_info, - crimson::osd::SnapSetContextRef ssc); + crimson::osd::SnapSetContextRef ssc, + RecoveryCloneLockManager& clone_lock_manager); std::pair, ceph::bufferlist> trim_pushed_data( const interval_set ©_subset, const interval_set &intervals_received, diff --git a/src/test/crimson/test_calc_subsets.cc b/src/test/crimson/test_calc_subsets.cc index 96888495a84..44b628f931f 100644 --- a/src/test/crimson/test_calc_subsets.cc +++ b/src/test/crimson/test_calc_subsets.cc @@ -24,11 +24,12 @@ TEST(head_subsets, dirty_region) // **** crimson::osd::subsets_t result = - crimson::osd::calc_head_subsets(obj_size, - empty_ss, - head, - missing, - last_backfill); + crimson::osd::commit_subsets_unlocked( + crimson::osd::calc_head_subsets(obj_size, + empty_ss, + head, + missing, + last_backfill)); EXPECT_TRUE(result.clone_subsets.empty()); EXPECT_TRUE(result.data_subset == expect_data_region); @@ -47,11 +48,12 @@ TEST(head_subsets, head_all_clean) // **** crimson::osd::subsets_t result = - crimson::osd::calc_head_subsets(obj_size, - empty_ss, - head, - missing, - last_backfill); + crimson::osd::commit_subsets_unlocked( + crimson::osd::calc_head_subsets(obj_size, + empty_ss, + head, + missing, + last_backfill)); EXPECT_TRUE(result.clone_subsets.empty()); EXPECT_TRUE(result.data_subset.empty()); @@ -71,11 +73,12 @@ TEST(head_subsets, all_dirty) // **** crimson::osd::subsets_t result = - crimson::osd::calc_head_subsets(obj_size, - empty_ss, - head, - missing, - last_backfill); + crimson::osd::commit_subsets_unlocked( + crimson::osd::calc_head_subsets(obj_size, + empty_ss, + head, + missing, + last_backfill)); EXPECT_TRUE(result.clone_subsets.empty()); EXPECT_TRUE(result.data_subset.size() == obj_size); @@ -118,11 +121,12 @@ TEST(head_subsets, clone_overlap) // **** crimson::osd::subsets_t result = - crimson::osd::calc_head_subsets(obj_size, - ss, - head, - missing, - last_backfill); + crimson::osd::commit_subsets_unlocked( + crimson::osd::calc_head_subsets(obj_size, + ss, + head, + missing, + last_backfill)); EXPECT_TRUE(result.clone_subsets[clone] == expect_clone_subset); } @@ -170,11 +174,12 @@ TEST(head_subsets, dirty_region_and_clone_overlap) // **** crimson::osd::subsets_t result = - crimson::osd::calc_head_subsets(obj_size, - ss, - head, - missing, - last_backfill); + crimson::osd::commit_subsets_unlocked( + crimson::osd::calc_head_subsets(obj_size, + ss, + head, + missing, + last_backfill)); EXPECT_TRUE(result.clone_subsets[clone] == expect_clone_subset); EXPECT_TRUE(result.data_subset == expect_data_region); } @@ -247,10 +252,11 @@ TEST(clone_subsets, overlap) // **** crimson::osd::subsets_t result = - crimson::osd::calc_clone_subsets(ss, - clone, - missing, - last_backfill); + crimson::osd::commit_subsets_unlocked( + crimson::osd::calc_clone_subsets(ss, + clone, + missing, + last_backfill)); EXPECT_TRUE(result.clone_subsets[older_clone] == expect_clone_subset1); EXPECT_TRUE(result.clone_subsets[newest_clone] == expect_clone_subset2); } -- 2.47.3