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

myoungwon · myoungwon · commit ad8b3f29ada5 · 2024-01-05T15:44:44.000+09:00
Signed-off-by: Myoungwon Oh &lt;myoungwon.oh@samsung.com&gt;
diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
@@ -1732,22 +1732,25 @@ 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
   if (delta.type == extent_types_t::ALLOC_INFO) {
     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,28 +1838,25 @@ 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={}",
             journal_seq, record_base, delta, *extent);
 
       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
@@ -1039,7 +1039,8 @@ class Cache {
    */
   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
@@ -611,6 +611,10 @@ class CachedExtent
     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
@@ -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 @@ class Journal {
     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
@@ -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
@@ -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
@@ -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
@@ -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) {

Original file line number	Diff line number	Diff line change
`@@ -246,7 +246,8 @@ struct cbjournal_test_t : public seastar_test_suite_t, JournalTrimmer`
`246`	`246`	`}`
`247`	`247`	`}`
`248`	`248`	`assert(found == true);`
`249`		`- return Journal::replay_ertr::make_ready_future<bool>(true);`
	`249`	`+ return Journal::replay_ertr::make_ready_future<`
	`250`	`+ std::pair<bool, CachedExtentRef>>(true, nullptr);`
`250`	`251`	`});`
`251`	`252`	`}`
`252`	`253`
`@@ -576,7 +577,8 @@ TEST_F(cbjournal_test_t, multiple_submit_at_end)`
`576`	`577`	`auto &dirty_seq,`
`577`	`578`	`auto &alloc_seq,`
`578`	`579`	`auto last_modified) {`
`579`		`- return Journal::replay_ertr::make_ready_future<bool>(true);`
	`580`	`+ return Journal::replay_ertr::make_ready_future<`
	`581`	`+ std::pair<bool, CachedExtentRef>>(true, nullptr);`
`580`	`582`	`}).unsafe_get0();`
`581`	`583`	`assert(get_written_to() == old_written_to);`
`582`	`584`	`});`