From: Matan Breizman <mbreizma@redhat.com>
Date: Mon, 8 Dec 2025 12:54:36 +0000 (+0000)
Subject: crimson/os/seastore/lba_mapping: Introduce co_refresh
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F66506%2Fhead;p=ceph.git

crimson/os/seastore/lba_mapping: Introduce co_refresh

See comment:
```
  //TODO: should be changed to return future<> once all calls
  //	  to refresh are through co_await. We return LBAMapping
  //	  for now to avoid mandating the callers to make sure
  //	  the life of the lba mapping survives the refresh.
```

For now introduce co_refresh and mark the existing refresh as
deprecated. Following work will audit all the existing users of
refresh and move them to the new method. This change is not trivial
so I prefer to follow up on this as a separate PR.

This should help avoiding UAR in suspension points:
```
==103588==ERROR: AddressSanitizer: stack-use-after-return on address 0xffff80197e90 at pc 0xaaaacb941b24 bp 0xffff7e48dd80 sp 0xffff7e48dd78
READ of size 8 at 0xffff80197e90 thread T1
    #0 0xaaaacb941b20 in boost::intrusive_ptr<crimson::os::seastore::LBACursor>::swap(boost::intrusive_ptr<crimson::os::seastore::LBACursor>&) /opt/ceph/include/boost/smart_ptr/intrusive_ptr.hpp:172:18
    #1 0xaaaacb941998 in boost::intrusive_ptr<crimson::os::seastore::LBACursor>::operator=(boost::intrusive_ptr<crimson::os::seastore::LBACursor>&&) /opt/ceph/include/boost/smart_ptr/intrusive_ptr.hpp:93:61
    #2 0xaaaacb933758 in crimson::os::seastore::LBAMapping::operator=(crimson::os::seastore::LBAMapping&&) /ceph/src/crimson/os/seastore/lba_mapping.h:46:48
    #3 0xaaaacde2fa54 in ... crimson::os::seastore::LBAMapping&&, std::array<crimson::os::seastore::LBAManager::remap_entry_t, 1ul>) (.resume) /ceph/src/crimson/os/seastore/transaction_manager.h:1282:11
```

Deprecate is commented out since otherwise make check would fail.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
---

diff --git a/src/crimson/os/seastore/lba_mapping.cc b/src/crimson/os/seastore/lba_mapping.cc
index 664da03faf0c..b1fbd4bf2a85 100644
--- a/src/crimson/os/seastore/lba_mapping.cc
+++ b/src/crimson/os/seastore/lba_mapping.cc
@@ -129,6 +129,19 @@ base_iertr::future<LBAMapping> LBAMapping::refresh()
   });
 }
 
+base_iertr::future<> LBAMapping::co_refresh()
+{
+  if (is_viewable()) {
+    co_return;
+  }
+  if (direct_cursor) {
+    co_await direct_cursor->refresh();
+  }
+  if (indirect_cursor) {
+    co_await indirect_cursor->refresh();
+  }
+}
+
 bool LBAMapping::is_initial_pending() const {
   assert(is_linked_direct());
   ceph_assert(direct_cursor->is_viewable());
diff --git a/src/crimson/os/seastore/lba_mapping.h b/src/crimson/os/seastore/lba_mapping.h
index b7fba8399be7..92706ab90373 100644
--- a/src/crimson/os/seastore/lba_mapping.h
+++ b/src/crimson/os/seastore/lba_mapping.h
@@ -182,12 +182,16 @@ public:
 
   LogicalChildNodeRef peek_logical_extent(Transaction &t) const;
 
+  // [[deprecated]]
   //TODO: should be changed to return future<> once all calls
   //	  to refresh are through co_await. We return LBAMapping
   //	  for now to avoid mandating the callers to make sure
   //	  the life of the lba mapping survives the refresh.
   base_iertr::future<LBAMapping> refresh();
 
+  // once the deprecated refresh is removed we can rename this to refresh
+  base_iertr::future<> co_refresh();
+
   base_iertr::future<LBAMapping> next();
 
 private:
diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index 7b52fef4f484..8e8086c85955 100644
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -299,7 +299,7 @@ public:
     // must be user-oriented required by maybe_init
     assert(is_user_transaction(t.get_src()));
 
-    pin = co_await pin.refresh();
+    co_await pin.co_refresh();
 
     if (pin.is_indirect()) {
       pin = co_await lba_manager->complete_indirect_lba_mapping(
@@ -640,7 +640,7 @@ public:
     SUBDEBUGT(seastore_tm,
       "src_base={}, dst_base={}, {}~{}, mapping={}, pos={}, updateref={}",
       t, src_base, dst_base, offset, len, mapping, pos, updateref);
-    pos = co_await pos.refresh();
+    co_await pos.co_refresh();
     mapping = co_await mapping.refresh();
     auto left = len;
     bool shared_direct = false;
@@ -1279,7 +1279,7 @@ private:
     if (pin.is_indirect()) {
       SUBDEBUGT(seastore_tm, "{} into {} remaps ...",
         t, pin, remaps.size());
-      pin = co_await pin.refresh();
+      co_await pin.co_refresh();
       pin = co_await lba_manager->complete_indirect_lba_mapping(t, pin);
     } else {
       laddr_t original_laddr = pin.get_key();
@@ -1290,7 +1290,7 @@ private:
       ceph_assert(!pin.is_clone());
 
       TCachedExtentRef<T> extent;
-      pin = co_await pin.refresh();
+      co_await pin.co_refresh();
       if (full_extent_integrity_check) {
         SUBTRACET(seastore_tm, "{} reading pin...", t, pin);
         // read the entire extent from disk (See: pin_to_extent)