crimson/os/seastore: check correct crc for inplace-rewritten extents after replay...

Signed-off-by: Myoungwon Oh <myoungwon.oh@samsung.com>

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 24fa9788fe89ee1abae29cac951093283592d7e5..7513cd601871d025e1c9af7a160db78deaa243d0 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -1732,14 +1732,16 @@ Cache::replay_delta(
               segment_seq_printer_t{delta_paddr_segment_seq},
               delta_paddr_segment_type,
               delta);
-        return replay_delta_ertr::make_ready_future<bool>(false);
+        return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+         std::make_pair(false, nullptr));
       }
     }
   }
 
   if (delta.type == extent_types_t::JOURNAL_TAIL) {
     // this delta should have been dealt with during segment cleaner mounting
-    return replay_delta_ertr::make_ready_future<bool>(false);
+    return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+      std::make_pair(false, nullptr));
   }
 
   // replay alloc
@@ -1747,7 +1749,8 @@ Cache::replay_delta(
     if (journal_seq < alloc_tail) {
       DEBUG("journal_seq {} < alloc_tail {}, don't replay {}",
        journal_seq, alloc_tail, delta);
-      return replay_delta_ertr::make_ready_future<bool>(false);
+      return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+       std::make_pair(false, nullptr));
     }
 
     alloc_delta_t alloc_delta;
@@ -1771,14 +1774,16 @@ Cache::replay_delta(
     if (!backref_list.empty()) {
       backref_batch_update(std::move(backref_list), journal_seq);
     }
-    return replay_delta_ertr::make_ready_future<bool>(true);
+    return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+      std::make_pair(true, nullptr));
   }
 
   // replay dirty
   if (journal_seq < dirty_tail) {
     DEBUG("journal_seq {} < dirty_tail {}, don't replay {}",
       journal_seq, dirty_tail, delta);
-    return replay_delta_ertr::make_ready_future<bool>(false);
+    return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+      std::make_pair(false, nullptr));
   }
 
   if (delta.type == extent_types_t::ROOT) {
@@ -1792,7 +1797,8 @@ Cache::replay_delta(
           journal_seq, record_base, delta, *root);
     root->set_modify_time(modify_time);
     add_extent(root);
-    return replay_delta_ertr::make_ready_future<bool>(true);
+    return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+      std::make_pair(true, root));
   } else {
     auto _get_extent_if_cached = [this](paddr_t addr)
       -> get_extent_ertr::future<CachedExtentRef> {
@@ -1832,7 +1838,8 @@ Cache::replay_delta(
        DEBUG("replay extent is not present, so delta is obsolete at {} {} -- {}",
              journal_seq, record_base, delta);
        assert(delta.pversion > 0);
-       return replay_delta_ertr::make_ready_future<bool>(true);
+       return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+         std::make_pair(true, nullptr));
       }
 
       DEBUG("replay extent delta at {} {} ... -- {}, prv_extent={}",
@@ -1840,20 +1847,16 @@ Cache::replay_delta(
 
       if (delta.paddr.get_addr_type() == paddr_types_t::SEGMENT ||
          !can_inplace_rewrite(delta.type)) {
-       assert(extent->last_committed_crc == delta.prev_crc);
+       ceph_assert_always(extent->last_committed_crc == delta.prev_crc);
        assert(extent->version == delta.pversion);
        extent->apply_delta_and_adjust_crc(record_base, delta.bl);
        extent->set_modify_time(modify_time);
-       assert(extent->last_committed_crc == delta.final_crc);
+       ceph_assert_always(extent->last_committed_crc == delta.final_crc);
       } else {
        assert(delta.paddr.get_addr_type() == paddr_types_t::RANDOM_BLOCK);
        extent->apply_delta_and_adjust_crc(record_base, delta.bl);
        extent->set_modify_time(modify_time);
-       // Since rewrite_dirty can conflict with other transaction after 
-       // inplace rewrite is complete, crc may not be matched
-       if (delta.final_crc == extent->last_committed_crc) {
-         assert(extent->version == delta.pversion);
-       }
+       // crc will be checked after journal replay is done
       }
 
       extent->version++;
@@ -1866,7 +1869,8 @@ Cache::replay_delta(
               journal_seq, record_base, delta, *extent);
       }
       mark_dirty(extent);
-      return replay_delta_ertr::make_ready_future<bool>(true);
+      return replay_delta_ertr::make_ready_future<std::pair<bool, CachedExtentRef>>(
+       std::make_pair(true, extent));
     });
   }
 }

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index c79473f98ba28993fcc6bcddf06c168fde449f7b..0f32bfe87e66b547a0a00929013635ec6929bea6 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -1039,7 +1039,8 @@ public:
    */
   using replay_delta_ertr = crimson::errorator<
     crimson::ct_error::input_output_error>;
-  using replay_delta_ret = replay_delta_ertr::future<bool>;
+  using replay_delta_ret = replay_delta_ertr::future<
+    std::pair<bool, CachedExtentRef>>;
   replay_delta_ret replay_delta(
     journal_seq_t seq,
     paddr_t record_block_base,

diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
index 487b0f3555e1f111d302f5e390f10c1d32a43961..4cb7e2b490cbdbf6a6fd11c20c99d44f148f8f70 100644 (file)
--- a/src/crimson/os/seastore/cached_extent.h
+++ b/src/crimson/os/seastore/cached_extent.h
@@ -611,6 +611,10 @@ public:
     return prior_instance;
   }
 
+  uint32_t get_last_committed_crc() const {
+    return last_committed_crc;
+  }
+
 private:
   template <typename T>
   friend class read_set_item_t;

diff --git a/src/crimson/os/seastore/journal.h b/src/crimson/os/seastore/journal.h
index 18c0797a8b8b80d13c8d56a6167080e95ccc3988..633aa84d7dbcda2b45d933945b30a9a522855198 100644 (file)
--- a/src/crimson/os/seastore/journal.h
+++ b/src/crimson/os/seastore/journal.h
@@ -8,6 +8,7 @@
 #include "crimson/os/seastore/ordering_handle.h"
 #include "crimson/os/seastore/seastore_types.h"
 #include "crimson/os/seastore/segment_seq_allocator.h"
+#include "crimson/os/seastore/cached_extent.h"
 
 namespace crimson::os::seastore {
 
@@ -88,7 +89,7 @@ public:
     crimson::ct_error::erange>;
   using replay_ret = replay_ertr::future<>;
   using delta_handler_t = std::function<
-    replay_ertr::future<bool>(
+    replay_ertr::future<std::pair<bool, CachedExtentRef>>(
       const record_locator_t&,
       const delta_info_t&,
       const journal_seq_t&, // dirty_tail

diff --git a/src/crimson/os/seastore/journal/circular_bounded_journal.cc b/src/crimson/os/seastore/journal/circular_bounded_journal.cc
index ec41bfab142649d8ce36678003b3ca86669064ce..a02c745084174e4c1b9ba0f7237c03d616d9fd90 100644 (file)
--- a/src/crimson/os/seastore/journal/circular_bounded_journal.cc
+++ b/src/crimson/os/seastore/journal/circular_bounded_journal.cc
@@ -316,7 +316,8 @@ Journal::replay_ret CircularBoundedJournal::replay(
     return seastar::do_with(
       std::move(delta_handler),
       std::map<paddr_t, journal_seq_t>(),
-      [this](auto &d_handler, auto &map) {
+      std::map<paddr_t, std::pair<CachedExtentRef, uint32_t>>(),
+      [this](auto &d_handler, auto &map, auto &crc_info) {
       auto build_paddr_seq_map = [&map](
         const auto &offsets,
         const auto &e,
@@ -339,8 +340,8 @@ Journal::replay_ret CircularBoundedJournal::replay(
       // The first pass to build the paddr->journal_seq_t map 
       // from extent allocations
       return scan_valid_record_delta(std::move(build_paddr_seq_map), tail
-      ).safe_then([this, &map, &d_handler, tail]() {
-       auto call_d_handler_if_valid = [this, &map, &d_handler](
+      ).safe_then([this, &map, &d_handler, tail, &crc_info]() {
+       auto call_d_handler_if_valid = [this, &map, &d_handler, &crc_info](
          const auto &offsets,
          const auto &e,
          sea_time_point modify_time)
@@ -353,12 +354,27 @@ Journal::replay_ret CircularBoundedJournal::replay(
              get_dirty_tail(),
              get_alloc_tail(),
              modify_time
-           );
+           ).safe_then([&e, &crc_info](auto ret) {
+             auto [applied, ext] = ret;
+             if (applied && ext && can_inplace_rewrite(
+                 ext->get_type())) {
+               crc_info[ext->get_paddr()] =
+                 std::make_pair(ext, e.final_crc);
+             }
+             return replay_ertr::make_ready_future<bool>(applied);
+           });
          }
          return replay_ertr::make_ready_future<bool>(true);
        };
        // The second pass to replay deltas
-       return scan_valid_record_delta(std::move(call_d_handler_if_valid), tail);
+       return scan_valid_record_delta(std::move(call_d_handler_if_valid), tail
+       ).safe_then([&crc_info]() {
+         for (auto p : crc_info) {
+           ceph_assert_always(p.second.first->get_last_committed_crc() == p.second.second);    
+         }
+         crc_info.clear();
+         return replay_ertr::now();
+       });
       });
     }).safe_then([this]() {
       // make sure that committed_to is JOURNAL_SEQ_NULL if jounal is the initial state

diff --git a/src/crimson/os/seastore/journal/segmented_journal.cc b/src/crimson/os/seastore/journal/segmented_journal.cc
index 58df913749321abe97332fc840e5be816d466123..c40295dbacef4b32da4b1a3e01a9b6e039db0d1e 100644 (file)
--- a/src/crimson/os/seastore/journal/segmented_journal.cc
+++ b/src/crimson/os/seastore/journal/segmented_journal.cc
@@ -291,7 +291,8 @@ SegmentedJournal::replay_segment(
              trimmer.get_dirty_tail(),
              trimmer.get_alloc_tail(),
               modify_time
-            ).safe_then([&stats, delta_type=delta.type](bool is_applied) {
+            ).safe_then([&stats, delta_type=delta.type](auto ret) {
+             auto [is_applied, ext] = ret;
               if (is_applied) {
                 // see Cache::replay_delta()
                 assert(delta_type != extent_types_t::JOURNAL_TAIL);

diff --git a/src/test/crimson/seastore/test_cbjournal.cc b/src/test/crimson/seastore/test_cbjournal.cc
index 0bf2d41358bfc7fa19dba27cd4d47f8358873438..bacb3cd2f78ec8044ba4ea0aefbb90d1a3c053af 100644 (file)
--- a/src/test/crimson/seastore/test_cbjournal.cc
+++ b/src/test/crimson/seastore/test_cbjournal.cc
@@ -246,7 +246,8 @@ struct cbjournal_test_t : public seastar_test_suite_t, JournalTrimmer
        }
       }
       assert(found == true);
-      return Journal::replay_ertr::make_ready_future<bool>(true);
+      return Journal::replay_ertr::make_ready_future<
+       std::pair<bool, CachedExtentRef>>(true, nullptr);
     });
   }
 
@@ -576,7 +577,8 @@ TEST_F(cbjournal_test_t, multiple_submit_at_end)
             auto &dirty_seq,
             auto &alloc_seq,
             auto last_modified) {
-      return Journal::replay_ertr::make_ready_future<bool>(true);
+      return Journal::replay_ertr::make_ready_future<
+       std::pair<bool, CachedExtentRef>>(true, nullptr);
     }).unsafe_get0();
     assert(get_written_to() == old_written_to);
   });

diff --git a/src/test/crimson/seastore/test_seastore_journal.cc b/src/test/crimson/seastore/test_seastore_journal.cc
index 46ec723a3524fb5fb3d3180e20d24cb6052697df..ddd894349d21e6e88ad3dd16fd1e2b0bc371e704 100644 (file)
--- a/src/test/crimson/seastore/test_seastore_journal.cc
+++ b/src/test/crimson/seastore/test_seastore_journal.cc
@@ -218,7 +218,8 @@ struct journal_test_t : seastar_test_suite_t, SegmentProvider, JournalTrimmer {
          delta_checker = std::nullopt;
          advance();
        }
-       return Journal::replay_ertr::make_ready_future<bool>(true);
+       return Journal::replay_ertr::make_ready_future<
+         std::pair<bool, CachedExtentRef>>(true, nullptr);
       }).unsafe_get0();
     ASSERT_EQ(record_iter, records.end());
     for (auto &i : records) {