From f0c0040274d1b84ef5e08146500dcf48c9513133 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Fri, 2 Jul 2021 09:28:39 +0800 Subject: [PATCH] crimson/os/seastore/cache: fix retiring mutation-pending extents Mark the retiring mutation-pending extent as INVALID, and add it's prior-instance to retired-set in order to populate transaction invalidation correctly. Signed-off-by: Yingxin Cheng --- src/crimson/os/seastore/cache.cc | 10 ++++++---- src/crimson/os/seastore/transaction.h | 20 ++++++++++++++------ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index e1e557baf40be..1e098f1042ea3 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -393,14 +393,13 @@ void Cache::complete_commit( // Add new copy of mutated blocks, set_io_wait to block until written for (auto &i: t.mutated_block_list) { + if (!i->is_valid()) { + continue; + } DEBUGT("mutated {}", t, *i); assert(i->prior_instance); i->on_delta_write(final_block_start); i->prior_instance = CachedExtentRef(); - if (!i->is_valid()) { - DEBUGT("not dirtying invalid {}", t, *i); - continue; - } i->state = CachedExtent::extent_state_t::DIRTY; if (i->version == 1 || i->get_type() == extent_types_t::ROOT) { i->dirty_from_or_retired_at = seq; @@ -416,6 +415,9 @@ void Cache::complete_commit( } for (auto &i: t.mutated_block_list) { + if (!i->is_valid()) { + continue; + } i->complete_io(); } diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h index 25b437f277041..fb8aad525565c 100644 --- a/src/crimson/os/seastore/transaction.h +++ b/src/crimson/os/seastore/transaction.h @@ -52,16 +52,24 @@ public: void add_to_retired_set(CachedExtentRef ref) { ceph_assert(!is_weak()); - if (!ref->is_initial_pending()) { + if (ref->is_initial_pending()) { + // We decide not to remove it from fresh_block_list because touching this + // will affect relative paddrs, and it should be rare to retire a fresh + // extent. + ref->state = CachedExtent::extent_state_t::INVALID; + write_set.erase(*ref); + } else if (ref->is_mutation_pending()) { + ref->state = CachedExtent::extent_state_t::INVALID; + write_set.erase(*ref); + assert(ref->prior_instance); + retired_set.insert(ref->prior_instance); + assert(read_set.count(ref->prior_instance->get_paddr())); + ref->prior_instance.reset(); + } else { // && retired_set.count(ref->get_paddr()) == 0 // If it's already in the set, insert here will be a noop, // which is what we want. retired_set.insert(ref); - } else { - ref->state = CachedExtent::extent_state_t::INVALID; - } - if (ref->is_pending()) { - write_set.erase(*ref); } } -- 2.39.5