From 4a38616f4657e12dc5e45647c44e30c04b6a2ca3 Mon Sep 17 00:00:00 2001 From: Matan Breizman Date: Sun, 7 Apr 2024 09:38:06 +0000 Subject: [PATCH] crimson/osd/object_context_loader: fix with_clone_obc on resolve_oid case Resolve_oid on a clone object may actually return the head: ``` // Because oid.snap > ss.seq, we are trying to read from a snapshot // taken after the most recent write to this object. Read from head. ``` In this case, with_clone_obc should apply `func` same as with_head_obc would have. Note: previously, with_clone_obc_only was called on the resolved head object. While it didn't cause any errors, using the head_obc as clone is wrong. Signed-off-by: Matan Breizman (cherry picked from commit d8aad5576f67c0e255b42c5d26334c87ba705931) --- src/crimson/osd/object_context.cc | 1 + src/crimson/osd/object_context_loader.cc | 16 +++++++++++----- src/crimson/osd/object_context_loader.h | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/crimson/osd/object_context.cc b/src/crimson/osd/object_context.cc index 1ea701c229c15..d7c0c60bc4e61 100644 --- a/src/crimson/osd/object_context.cc +++ b/src/crimson/osd/object_context.cc @@ -53,6 +53,7 @@ std::optional resolve_oid( if (oid.snap > ss.seq) { // Because oid.snap > ss.seq, we are trying to read from a snapshot // taken after the most recent write to this object. Read from head. + logger().debug("{} returning head", __func__); return oid.get_head(); } else { // which clone would it be? diff --git a/src/crimson/osd/object_context_loader.cc b/src/crimson/osd/object_context_loader.cc index b8a6cbceb296c..4f0e2aeff0a4b 100644 --- a/src/crimson/osd/object_context_loader.cc +++ b/src/crimson/osd/object_context_loader.cc @@ -56,18 +56,24 @@ using crimson::common::local_conf; template ObjectContextLoader::load_obc_iertr::future<> ObjectContextLoader::with_clone_obc_only(ObjectContextRef head, - hobject_t oid, + hobject_t clone_oid, with_obc_func_t&& func) { LOG_PREFIX(ObjectContextLoader::with_clone_obc_only); - auto coid = resolve_oid(head->get_head_ss(), oid); - if (!coid) { - ERRORDPP("clone {} not found", dpp, oid); + DEBUGDPP("{}", clone_oid); + assert(!clone_oid.is_head()); + auto resolved_oid = resolve_oid(head->get_head_ss(), clone_oid); + if (!resolved_oid) { + ERRORDPP("clone {} not found", dpp, clone_oid); return load_obc_iertr::future<>{ crimson::ct_error::enoent::make() }; } - auto [clone, existed] = obc_registry.get_cached_obc(*coid); + if (resolved_oid->is_head()) { + // See resolve_oid + return std::move(func)(head, head); + } + auto [clone, existed] = obc_registry.get_cached_obc(*resolved_oid); return clone->template with_lock( [existed=existed, clone=std::move(clone), func=std::move(func), head=std::move(head), this]() mutable diff --git a/src/crimson/osd/object_context_loader.h b/src/crimson/osd/object_context_loader.h index 0cd50623abc25..eeadb9daa92c4 100644 --- a/src/crimson/osd/object_context_loader.h +++ b/src/crimson/osd/object_context_loader.h @@ -43,7 +43,7 @@ public: // with an already locked head. template load_obc_iertr::future<> with_clone_obc_only(ObjectContextRef head, - hobject_t oid, + hobject_t clone_oid, with_obc_func_t&& func); // Use this variant in the case where both the head -- 2.39.5