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 664da03faf0c3601aae905d73ad3df96d9c91d9a..b1fbd4bf2a859bc1cc131702fad8ab3cb2014327 100644 (file)
--- 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 b7fba8399be7151e7c847b649f0ac41a253ab160..92706ab90373369e86ca018779d7a68d13ebef7d 100644 (file)
--- 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 7b52fef4f484fad98ae3ebc11d9e90de44f8e4b5..8e8086c85955a5254465a4d4fdfd14cb24260d35 100644 (file)
--- 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)