crimson/os/seastore: get_next_dirty_extents: record in transaction read set

Record the extents in the read set after wait_io() as in get_extent.  This
should ensure that the interruptible_future machinery will handle the
event that one of them gets invalidated prior to beging rewritten.

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 d30a73a1503ec0b9da539f04d50721453c8bcaa8..98e76346d21fa69a166bdb5a14e2990df84fad09 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -1053,40 +1053,72 @@ Cache::replay_delta(
 }
 
 Cache::get_next_dirty_extents_ret Cache::get_next_dirty_extents(
+  Transaction &t,
   journal_seq_t seq,
   size_t max_bytes)
 {
   LOG_PREFIX(Cache::get_next_dirty_extents);
-  std::vector<CachedExtentRef> ret;
+  std::vector<CachedExtentRef> cand;
   size_t bytes_so_far = 0;
   for (auto i = dirty.begin();
        i != dirty.end() && bytes_so_far < max_bytes;
        ++i) {
-    CachedExtentRef cand;
     if (i->get_dirty_from() != journal_seq_t() && i->get_dirty_from() < seq) {
-      DEBUG("next {}", *i);
-      if (!(ret.empty() ||
-           ret.back()->get_dirty_from() <= i->get_dirty_from())) {
-       DEBUG("last {}, next {}", *ret.back(), *i);
+      DEBUGT("next {}", t, *i);
+      if (!(cand.empty() ||
+           cand.back()->get_dirty_from() <= i->get_dirty_from())) {
+       ERRORT("last {}, next {}", t, *cand.back(), *i);
       }
-      assert(ret.empty() || ret.back()->get_dirty_from() <= i->get_dirty_from());
+      assert(cand.empty() || cand.back()->get_dirty_from() <= i->get_dirty_from());
       bytes_so_far += i->get_length();
-      ret.push_back(&*i);
+      cand.push_back(&*i);
+
     } else {
       break;
     }
   }
   return seastar::do_with(
-    std::move(ret),
-    [FNAME](auto &ret) {
-      return seastar::do_for_each(
-       ret,
-       [FNAME](auto &ext) {
+    std::move(cand),
+    decltype(cand)(),
+    [FNAME, this, &t](auto &cand, auto &ret) {
+      return trans_intr::do_for_each(
+       cand,
+       [FNAME, this, &t, &ret](auto &ext) {
          DEBUG("waiting on {}", *ext);
-         return ext->wait_io();
-       }).then([&ret]() mutable {
-         return seastar::make_ready_future<std::vector<CachedExtentRef>>(
-           std::move(ret));
+
+         return trans_intr::make_interruptible(
+           ext->wait_io()
+         ).then_interruptible([FNAME, this, ext, &t, &ret] {
+           if (!ext->is_valid()) {
+             invalidate(t, *ext);
+             return;
+           }
+
+           CachedExtentRef on_transaction;
+           auto result = t.get_extent(ext->get_paddr(), &on_transaction);
+           if (result == Transaction::get_extent_ret::ABSENT) {
+             DEBUGT("{} absent on t", t, *ext);
+             t.add_to_read_set(ext);
+             if (ext->get_type() == extent_types_t::ROOT) {
+               if (t.root) {
+                 assert(&*t.root == &*ext);
+                 assert(0 == "t.root would have to already be in the read set");
+               } else {
+                 assert(&*ext == &*root);
+                 t.root = root;
+               }
+             }
+             ret.push_back(ext);
+           } else if (result == Transaction::get_extent_ret::PRESENT) {
+             DEBUGT("{} present on t as {}", t, *ext, *on_transaction);
+             ret.push_back(on_transaction);
+           } else {
+             assert(result == Transaction::get_extent_ret::RETIRED);
+             DEBUGT("{} retired on t", t, *ext);
+           }
+         });
+       }).then_interruptible([&ret] {
+         return std::move(ret);
        });
     });
 }

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index 0417dbff9f448ffd633b67b1962648048c5ce4d8..d610e0f52d81fc8cc06012b0d29c24e617771556 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -520,6 +520,7 @@ public:
        return t.root;
       } else {
        t.add_to_read_set(extent);
+       t.root = extent->cast<RootBlock>();
        return extent;
       }
     } else {
@@ -545,11 +546,17 @@ public:
     return out;
   }
 
-  /// returns extents with get_dirty_from() < seq
-  using get_next_dirty_extents_ertr = crimson::errorator<>;
-  using get_next_dirty_extents_ret = get_next_dirty_extents_ertr::future<
+  /**
+   * get_next_dirty_extents
+   *
+   * Returns extents with get_dirty_from() < seq and adds to read set of
+   * t.
+   */
+  using get_next_dirty_extents_iertr = base_iertr;
+  using get_next_dirty_extents_ret = get_next_dirty_extents_iertr::future<
     std::vector<CachedExtentRef>>;
   get_next_dirty_extents_ret get_next_dirty_extents(
+    Transaction &t,
     journal_seq_t seq,
     size_t max_bytes);
 

diff --git a/src/crimson/os/seastore/segment_cleaner.cc b/src/crimson/os/seastore/segment_cleaner.cc
index f2e43273152cd3cd05610e375f074ac1211bfc75..283b7eece8953e6881087cc1bb9347e067e827bc 100644 (file)
--- a/src/crimson/os/seastore/segment_cleaner.cc
+++ b/src/crimson/os/seastore/segment_cleaner.cc
@@ -2,6 +2,7 @@
 // vim: ts=8 sw=2 smarttab
 
 #include "crimson/common/log.h"
+#include "crimson/os/seastore/logging.h"
 
 #include "crimson/os/seastore/segment_cleaner.h"
 #include "crimson/os/seastore/transaction_manager.h"
@@ -220,20 +221,19 @@ SegmentCleaner::rewrite_dirty_ret SegmentCleaner::rewrite_dirty(
   Transaction &t,
   journal_seq_t limit)
 {
-  return trans_intr::make_interruptible(
-    ecb->get_next_dirty_extents(
-      limit,
-      config.journal_rewrite_per_cycle)
-  ).then_interruptible([=, &t](auto dirty_list) {
+  LOG_PREFIX(SegmentCleaner::rewrite_dirty);
+  return ecb->get_next_dirty_extents(
+    t,
+    limit,
+    config.journal_rewrite_per_cycle
+  ).si_then([=, &t](auto dirty_list) {
     return seastar::do_with(
       std::move(dirty_list),
-      [this, &t](auto &dirty_list) {
+      [FNAME, this, &t](auto &dirty_list) {
        return trans_intr::do_for_each(
          dirty_list,
-         [this, &t](auto &e) {
-           logger().debug(
-             "SegmentCleaner::rewrite_dirty cleaning {}",
-             *e);
+         [FNAME, this, &t](auto &e) {
+           DEBUGT("cleaning {}", t, *e);
            return ecb->rewrite_extent(t, e);
          });
       });

diff --git a/src/crimson/os/seastore/segment_cleaner.h b/src/crimson/os/seastore/segment_cleaner.h
index 2ff0faccf2183970bc3b659f791c46c78ef426c7..474275d399f20d1def4c7a9aa1c29d20c733544f 100644 (file)
--- a/src/crimson/os/seastore/segment_cleaner.h
+++ b/src/crimson/os/seastore/segment_cleaner.h
@@ -264,20 +264,19 @@ public:
       );
     }
 
-    /**
-     * get_next_dirty_extent
-     *
-     * returns all extents with dirty_from < bound
-     */
-    using get_next_dirty_extents_iertr = crimson::errorator<>;
+    /// See Cache::get_next_dirty_extents
+    using get_next_dirty_extents_iertr = trans_iertr<
+      crimson::errorator<
+        crimson::ct_error::input_output_error>
+      >;
     using get_next_dirty_extents_ret = get_next_dirty_extents_iertr::future<
       std::vector<CachedExtentRef>>;
     virtual get_next_dirty_extents_ret get_next_dirty_extents(
+      Transaction &t,     ///< [in] current transaction
       journal_seq_t bound,///< [in] return extents with dirty_from < bound
       size_t max_bytes    ///< [in] return up to max_bytes of extents
     ) = 0;
 
-
     using extent_mapping_ertr = crimson::errorator<
       crimson::ct_error::input_output_error,
       crimson::ct_error::eagain>;

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index de87f053ba477dce8e71e91e8257a8124633e185..d6fc4a11bd33b841b127624e9da5433eec8d9d38 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -269,10 +269,11 @@ TransactionManager::submit_transaction_direct(
 
 TransactionManager::get_next_dirty_extents_ret
 TransactionManager::get_next_dirty_extents(
+  Transaction &t,
   journal_seq_t seq,
   size_t max_bytes)
 {
-  return cache->get_next_dirty_extents(seq, max_bytes);
+  return cache->get_next_dirty_extents(t, seq, max_bytes);
 }
 
 TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent(

diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index 0a8036967dd3ef54b1cd8e202410ab6c77375e85..db229b12e23505a970fa4a1488f7a70fc8d3e1cc 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -370,6 +370,7 @@ public:
 
   using SegmentCleaner::ExtentCallbackInterface::get_next_dirty_extents_ret;
   get_next_dirty_extents_ret get_next_dirty_extents(
+    Transaction &t,
     journal_seq_t seq,
     size_t max_bytes) final;