crimson/os/seastore: refine documents related to inplace rewrite

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 8c388c3d3ccf41e5cbec72b66baf8c74a7a21b9e..f96fe244e1c3df7a3993d07709d118dfd27887bf 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -1148,11 +1148,14 @@ record_t Cache::prepare_record(
     if (!i->is_exist_mutation_pending()) {
       DEBUGT("commit replace extent ... -- {}, prior={}",
             t, *i, *i->prior_instance);
-      // If inplace rewrite occurs during mutation, prev->version will
-      // be zero. Although this results in the version mismatch here, we can
-      // correct this by changing version to 1. This is because the inplace rewrite
-      // does not introduce any actual modification that could negatively
-      // impact system reliability
+
+      // If inplace rewrite happens from a concurrent transaction,
+      // i->prior_instance will be changed from DIRTY to CLEAN implicitly, thus
+      // i->prior_instance->version become 0. This won't cause conflicts
+      // intentionally because inplace rewrite won't modify the shared extent.
+      //
+      // However, this leads to version mismatch below, thus we reset the
+      // version to 1 in this case.
       if (i->prior_instance->version == 0 && i->version > 1) {
        assert(can_inplace_rewrite(i->get_type()));
        assert(can_inplace_rewrite(i->prior_instance->get_type()));
@@ -1162,6 +1165,7 @@ record_t Cache::prepare_record(
          paddr_types_t::RANDOM_BLOCK);
        i->version = 1;
       }
+
       // extent with EXIST_MUTATION_PENDING doesn't have
       // prior_instance field so skip these extents.
       // the existing extents should be added into Cache
@@ -1926,6 +1930,7 @@ Cache::replay_delta(
        ceph_assert_always(extent->last_committed_crc == delta.final_crc);
       } else {
        assert(delta.paddr.get_addr_type() == paddr_types_t::RANDOM_BLOCK);
+       // see prepare_record(), inplace rewrite might cause version mismatch
        extent->apply_delta_and_adjust_crc(record_base, delta.bl);
        extent->set_modify_time(modify_time);
        // crc will be checked after journal replay is done

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index 7e75b8bc4975bbfda12ee71331155bbd9efae33a..62c88b49be3b3183ea94a0cb697a74396ebfe694 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -639,6 +639,9 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent(
   assert(extent->is_valid() && !extent->is_initial_pending());
   if (extent->is_dirty()) {
     if (epm->can_inplace_rewrite(t, extent)) {
+      // FIXME: is_dirty() is true for mutation pending extents
+      // which shouldn't do inplace rewrite because a pending transaction
+      // may fail.
       DEBUGT("delta overwriting extent -- {}", t, *extent);
       t.add_inplace_rewrite_extent(extent);
       extent->set_inplace_rewrite_generation();