Merge pull request ceph#46526 from cyx1231st/wip-seastore-fix-cache

crimson/os/seastore/cache: fix a potential leak when replace placeholder Reviewed-by: Xuehan Xu <[email protected]>
selroc · Jun 8, 2022 · fbd7c77 · fbd7c77
2 parents b985f3a + e569319
commit fbd7c77
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 31 deletions.
diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
@@ -77,12 +77,8 @@ Cache::retire_extent_ret Cache::retire_extent_addr(
     DEBUGT("retire {}~{} in cache -- {}", t, addr, length, *ext);
     if (ext->get_type() != extent_types_t::RETIRED_PLACEHOLDER) {
       t.add_to_read_set(ext);
-      return trans_intr::make_interruptible(
-        ext->wait_io()
-      ).then_interruptible([&t, ext=std::move(ext)]() mutable {
-        t.add_to_retired_set(ext);
-        return retire_extent_iertr::now();
-      });
+      t.add_to_retired_set(ext);
+      return retire_extent_iertr::now();
     }
     // the retired-placeholder exists
   } else {

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
@@ -325,6 +325,7 @@ class Cache {
           "{} {}~{} is absent(placeholder), reading ... -- {}",
           T::TYPE, offset, length, *ret);
       extents.replace(*ret, *cached);
+      on_cache(*ret);
 
       // replace placeholder in transactions
       while (!cached->transactions.empty()) {
@@ -451,20 +452,16 @@ class Cache {
     } else {
       SUBTRACET(seastore_cache, "{} {}~{} is absent on t, query cache ...",
                 t, T::TYPE, offset, length);
-      auto f = [&t](CachedExtent &ext) {
+      auto f = [&t, this](CachedExtent &ext) {
 	t.add_to_read_set(CachedExtentRef(&ext));
+	touch_extent(ext);
       };
       auto metric_key = std::make_pair(t.get_src(), T::TYPE);
       return trans_intr::make_interruptible(
 	get_extent<T>(
 	  offset, length, &metric_key,
 	  std::forward<Func>(extent_init_func), std::move(f))
-      ).si_then([this](auto ref) {
-	(void)this; // silence incorrect clang warning about captur
-	touch_extent(*ref);
-	return get_extent_iertr::make_ready_future<TCachedExtentRef<T>>(
-	  std::move(ref));
-      });
+      );
     }
   }
   template <typename T>
@@ -539,19 +536,16 @@ class Cache {
     } else {
       SUBTRACET(seastore_cache, "{} {}~{} {} is absent on t, query cache ...",
                 t, type, offset, length, laddr);
-      auto f = [&t](CachedExtent &ext) {
+      auto f = [&t, this](CachedExtent &ext) {
 	t.add_to_read_set(CachedExtentRef(&ext));
+	touch_extent(ext);
       };
       auto src = t.get_src();
       return trans_intr::make_interruptible(
 	_get_extent_by_type(
 	  type, offset, laddr, length, &src,
 	  std::move(extent_init_func), std::move(f))
-      ).si_then([this](CachedExtentRef ret) {
-	touch_extent(*ret);
-	return get_extent_ertr::make_ready_future<CachedExtentRef>(
-	  std::move(ret));
-      });
+      );
     }
   }
 
@@ -1048,10 +1042,7 @@ class Cache {
     }
 
     void add_to_lru(CachedExtent &extent) {
-      assert(
-	extent.is_clean() &&
-	!extent.is_pending() &&
-	!extent.is_placeholder());
+      assert(extent.is_clean() && !extent.is_placeholder());
 
       if (!extent.primary_ref_list_hook.is_linked()) {
 	contents += extent.get_length();
@@ -1077,9 +1068,7 @@ class Cache {
     }
 
     void remove_from_lru(CachedExtent &extent) {
-      assert(extent.is_clean());
-      assert(!extent.is_pending());
-      assert(!extent.is_placeholder());
+      assert(extent.is_clean() && !extent.is_placeholder());
 
       if (extent.primary_ref_list_hook.is_linked()) {
 	lru.erase(lru.s_iterator_to(extent));
@@ -1090,10 +1079,7 @@ class Cache {
     }
 
     void move_to_top(CachedExtent &extent) {
-      assert(
-	extent.is_clean() &&
-	!extent.is_pending() &&
-	!extent.is_placeholder());
+      assert(extent.is_clean() && !extent.is_placeholder());
 
       if (extent.primary_ref_list_hook.is_linked()) {
 	lru.erase(lru.s_iterator_to(extent));
@@ -1263,7 +1249,6 @@ class Cache {
 
   /// Update lru for access to ref
   void touch_extent(CachedExtent &ext) {
-    assert(!ext.is_pending());
     if (ext.is_clean() && !ext.is_placeholder()) {
       lru.move_to_top(ext);
     }

diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
@@ -317,6 +317,10 @@ class CachedExtent : public boost::intrusive_ref_counter<
     return get_type() == extent_types_t::RETIRED_PLACEHOLDER;
   }
 
+  bool is_pending_io() const {
+    return !!io_wait_promise;
+  }
+
   /// Return journal location of oldest relevant delta, only valid while DIRTY
   auto get_dirty_from() const {
     ceph_assert(is_dirty());

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
@@ -501,6 +501,7 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent(
       return rewrite_extent_iertr::now();
     }
     extent = updated;
+    ceph_assert(!extent->is_pending_io());
   }
 
   t.get_rewrite_version_stats().increment(extent->get_version());