From f74b8bb0f6ae7d636faaf4df2b91b913a000fdf8 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Thu, 22 Aug 2024 10:34:47 +0800 Subject: [PATCH] crimson/os/seastore: refine documents related to inplace rewrite Signed-off-by: Yingxin Cheng --- src/crimson/os/seastore/cache.cc | 15 ++++++++++----- src/crimson/os/seastore/transaction_manager.cc | 3 +++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 8c388c3d3ccf4..f96fe244e1c3d 100644 --- 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 7e75b8bc4975b..62c88b49be3b3 100644 --- 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(); -- 2.39.5