crimson/os/seastore/transaction_manager: complete dec_ref

Previously, dec_ref didn't handle actually retiring the
extent from the cache.  dec_ref will now reach into the
cache and mark the extent retired if it exists either
in the cache or in the current transaction.

Signed-off-by: Samuel Just <sjust@redhat.com>

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 2e7855651abbd7a196c1fc8611a1c49896775a4b..7b130c71216a8edda7217bf2f19aa7525d6292b6 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -28,6 +28,24 @@ Cache::~Cache()
   ceph_assert(extents.empty());
 }
 
+Cache::retire_extent_ret Cache::retire_extent_if_cached(
+  Transaction &t, paddr_t addr)
+{
+  if (auto ext = t.write_set.find_offset(addr); ext != t.write_set.end()) {
+    t.add_to_retired_set(CachedExtentRef(&*ext));
+    return retire_extent_ertr::now();
+  } else if (auto iter = extents.find_offset(addr);
+      iter != extents.end()) {
+    auto ret = CachedExtentRef(&*iter);
+    return ret->wait_io().then([&t, ret=std::move(ret)]() mutable {
+      t.add_to_retired_set(ret);
+      return retire_extent_ertr::now();
+    });
+  } else {
+    return retire_extent_ertr::now();
+  }
+}
+
 void Cache::add_extent(CachedExtentRef ref)
 {
   assert(ref->is_valid());

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index d8859b3e7fb7ffd084f9945d001017ec058f5405..a31e044e1d60f436c432341963818e12e118f152 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -90,6 +90,13 @@ public:
     t.add_to_retired_set(ref);
   }
 
+  /// Declare paddr retired in t, noop if not cached
+  using retire_extent_ertr = crimson::errorator<
+    crimson::ct_error::input_output_error>;
+  using retire_extent_ret = retire_extent_ertr::future<>;
+  retire_extent_ret retire_extent_if_cached(
+    Transaction &t, paddr_t addr);
+
   /**
    * get_root
    *

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index 05859a3626a3093029b5683c6a319b77f6cda116..82a4d3de327ed183928261670af268c0aa7e88b7 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -117,21 +117,32 @@ TransactionManager::ref_ret TransactionManager::dec_ref(
   Transaction &t,
   LogicalCachedExtentRef &ref)
 {
-  return dec_ref(t, ref->get_laddr()
-  ).handle_error(
-    ref_ertr::pass_further{},
-    ct_error::all_same_way([](auto e) {
-      ceph_assert(0 == "unhandled error, TODO");
-    }));
+  return lba_manager.decref_extent(t, ref->get_laddr()
+  ).safe_then([this, &t, ref](auto ret) {
+    if (ret.refcount == 0) {
+      cache.retire_extent(t, ref);
+    }
+    return ret.refcount;
+  });
 }
 
 TransactionManager::ref_ret TransactionManager::dec_ref(
   Transaction &t,
   laddr_t offset)
 {
-  // TODO: need to retire the extent (only) if it's live, will need cache call
-  return lba_manager.decref_extent(t, offset).safe_then([](auto ret) {
-    return ret.refcount;
+  return lba_manager.decref_extent(t, offset
+  ).safe_then([this, &t](auto result) -> ref_ret {
+    if (result.refcount == 0) {
+      return cache.retire_extent_if_cached(t, result.addr).safe_then([] {
+       return ref_ret(
+         ref_ertr::ready_future_marker{},
+         0);
+      });
+    } else {
+      return ref_ret(
+       ref_ertr::ready_future_marker{},
+       result.refcount);
+    }
   });
 }
 

diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc
index d086c4ca34818b3970db098bc2492b92861ace63..076d7ad2dceedf7342bc7cdb895a3252e9c53a3b 100644 (file)
--- a/src/test/crimson/seastore/test_transaction_manager.cc
+++ b/src/test/crimson/seastore/test_transaction_manager.cc
@@ -278,6 +278,38 @@ TEST_F(transaction_manager_test_t, mutate)
   });
 }
 
+TEST_F(transaction_manager_test_t, create_remove_same_transaction)
+{
+  constexpr laddr_t SIZE = 4096;
+  run_async([this] {
+    constexpr laddr_t ADDR = 0xFF * SIZE;
+    {
+      auto t = create_transaction();
+      auto extent = alloc_extent(
+       t,
+       ADDR,
+       SIZE,
+       'a');
+      ASSERT_EQ(ADDR, extent->get_laddr());
+      check_mappings(t);
+      dec_ref(t, ADDR);
+      check_mappings(t);
+
+      extent = alloc_extent(
+       t,
+       ADDR,
+       SIZE,
+       'a');
+
+      submit_transaction(std::move(t));
+      check_mappings();
+    }
+    replay();
+    check_mappings();
+  });
+}
+
+
 TEST_F(transaction_manager_test_t, inc_dec_ref)
 {
   constexpr laddr_t SIZE = 4096;