From 2652de984166e36666c1dc8cd1c5ba560c8fced6 Mon Sep 17 00:00:00 2001 From: Xuehan Xu Date: Tue, 19 Jul 2022 15:48:47 +0800 Subject: [PATCH] crimson/os/seastore/async_cleaner: don't trim backref when reclaiming space Since the current backref cache doesn't invalidate duplicated backrefs any more and backrefs get trimmed in the exact order of journal seqs, there's no need to trim backrefs when reclaiming space Fixes: https://tracker.ceph.com/issues/56535 Signed-off-by: Xuehan Xu --- src/crimson/os/seastore/async_cleaner.cc | 130 +++++++----------- src/crimson/os/seastore/async_cleaner.h | 3 +- src/crimson/os/seastore/cache.cc | 26 ++-- src/crimson/os/seastore/transaction.h | 16 --- .../os/seastore/transaction_manager.cc | 35 +---- src/crimson/os/seastore/transaction_manager.h | 3 +- 6 files changed, 66 insertions(+), 147 deletions(-) diff --git a/src/crimson/os/seastore/async_cleaner.cc b/src/crimson/os/seastore/async_cleaner.cc index a7be225f69f..6009ceea3c2 100644 --- a/src/crimson/os/seastore/async_cleaner.cc +++ b/src/crimson/os/seastore/async_cleaner.cc @@ -1000,90 +1000,58 @@ AsyncCleaner::gc_reclaim_space_ret AsyncCleaner::gc_reclaim_space() [this, &reclaimed, &runs, &pin_list]() mutable { reclaimed = 0; runs++; - return seastar::do_with( - JOURNAL_SEQ_NULL, - [this, &reclaimed, &pin_list]( - auto &seq) { - return ecb->with_transaction_intr( - Transaction::src_t::CLEANER_RECLAIM, - "reclaim_space", - [this, &seq, &reclaimed, &pin_list](auto &t) { - return seastar::do_with( - std::vector(), - [this, &reclaimed, &t, &seq, &pin_list] - (auto &extents) { - return backref_manager.retrieve_backref_extents( - t, - backref_manager.get_cached_backref_extents_in_range( - reclaim_state->start_pos, reclaim_state->end_pos), - extents - ).si_then([this, &extents, &t, &pin_list] { - // calculate live extents - auto backref_set = - backref_manager.get_cached_backrefs_in_range( - reclaim_state->start_pos, reclaim_state->end_pos); - std::set< - backref_buf_entry_t, - backref_buf_entry_t::cmp_t> backrefs; - for (auto &pin : pin_list) { - backrefs.emplace(pin->get_key(), pin->get_val(), - pin->get_length(), pin->get_type(), journal_seq_t()); - } - for (auto &backref : backref_set) { - if (backref.laddr == L_ADDR_NULL) { - auto it = backrefs.find(backref.paddr); - assert(it->len == backref.len); - backrefs.erase(it); - } else { - backrefs.emplace(backref.paddr, backref.laddr, - backref.len, backref.type, backref.seq); - } - } - // retrieve live extents - return _retrieve_live_extents( - t, std::move(backrefs), extents); - }).si_then([this, &t, &seq] { - // we need to get the backref_set in range again, because - // it can change during live extents retrieval - auto backref_set = - backref_manager.get_cached_backrefs_in_range( - reclaim_state->start_pos, reclaim_state->end_pos); - // calculate the journal seq up to which the backref merge - // should run - for (auto &backref : backref_set) { - if (backref.seq != JOURNAL_SEQ_NULL && - (backref.seq > seq || seq == JOURNAL_SEQ_NULL)) { - seq = backref.seq; - } - } - auto fut = BackrefManager::merge_cached_backrefs_iertr::now(); - if (seq != JOURNAL_SEQ_NULL) { - fut = backref_manager.merge_cached_backrefs( - t, seq, std::numeric_limits::max() - ).si_then([](auto) { - return BackrefManager::merge_cached_backrefs_iertr::now(); - }); + return ecb->with_transaction_intr( + Transaction::src_t::CLEANER_RECLAIM, + "reclaim_space", + [this, &reclaimed, &pin_list](auto &t) { + return seastar::do_with( + std::vector(), + [this, &reclaimed, &t, &pin_list] + (auto &extents) { + return backref_manager.retrieve_backref_extents( + t, + backref_manager.get_cached_backref_extents_in_range( + reclaim_state->start_pos, reclaim_state->end_pos), + extents + ).si_then([this, &extents, &t, &pin_list] { + // calculate live extents + auto cached_backrefs = + backref_manager.get_cached_backrefs_in_range( + reclaim_state->start_pos, reclaim_state->end_pos); + std::set< + backref_buf_entry_t, + backref_buf_entry_t::cmp_t> backrefs; + for (auto &pin : pin_list) { + backrefs.emplace(pin->get_key(), pin->get_val(), + pin->get_length(), pin->get_type(), journal_seq_t()); + } + for (auto &backref : cached_backrefs) { + if (backref.laddr == L_ADDR_NULL) { + auto it = backrefs.find(backref.paddr); + assert(it->len == backref.len); + backrefs.erase(it); + } else { + backrefs.emplace(backref.paddr, backref.laddr, + backref.len, backref.type, backref.seq); } - return fut; - }).si_then([&extents, this, &t, &reclaimed] { - auto modify_time = segments[reclaim_state->get_segment_id()].modify_time; - return trans_intr::do_for_each( - extents, - [this, modify_time, &t, &reclaimed](auto &ext) { - reclaimed += ext->get_length(); - return ecb->rewrite_extent( - t, ext, reclaim_state->target_generation, modify_time); - }); - }); - }).si_then([this, &t, &seq] { - if (reclaim_state->is_complete()) { - t.mark_segment_to_release(reclaim_state->get_segment_id()); } - return ecb->submit_transaction_direct( - t, std::make_optional(std::move(seq)), - std::make_optional>( - {reclaim_state->start_pos, reclaim_state->end_pos})); + return _retrieve_live_extents( + t, std::move(backrefs), extents); + }).si_then([&extents, this, &t, &reclaimed] { + auto modify_time = segments[reclaim_state->get_segment_id()].modify_time; + return trans_intr::do_for_each( + extents, + [this, modify_time, &t, &reclaimed](auto &ext) { + reclaimed += ext->get_length(); + return ecb->rewrite_extent( + t, ext, reclaim_state->target_generation, modify_time); + }); }); + }).si_then([this, &t] { + if (reclaim_state->is_complete()) { + t.mark_segment_to_release(reclaim_state->get_segment_id()); + } + return ecb->submit_transaction_direct(t); }); }); }); diff --git a/src/crimson/os/seastore/async_cleaner.h b/src/crimson/os/seastore/async_cleaner.h index 94791486ff9..986c13268d4 100644 --- a/src/crimson/os/seastore/async_cleaner.h +++ b/src/crimson/os/seastore/async_cleaner.h @@ -666,8 +666,7 @@ public: submit_transaction_direct_iertr::future<>; virtual submit_transaction_direct_ret submit_transaction_direct( Transaction &t, - std::optional seq_to_trim = std::nullopt, - std::optional> gc_range = std::nullopt) = 0; + std::optional seq_to_trim = std::nullopt) = 0; }; private: diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc index 54922cd02c7..4e808b40e79 100644 --- a/src/crimson/os/seastore/cache.cc +++ b/src/crimson/os/seastore/cache.cc @@ -1144,9 +1144,8 @@ record_t Cache::prepare_record( retire_stat.increment(i->get_length()); DEBUGT("retired and remove extent -- {}", t, *i); commit_retire_extent(t, i); - if ((is_backref_mapped_extent_node(i) - || is_retired_placeholder(i->get_type())) - && t.should_record_release(i->get_paddr())) { + if (is_backref_mapped_extent_node(i) + || is_retired_placeholder(i->get_type())) { rel_delta.alloc_blk_ranges.emplace_back( i->get_paddr(), L_ADDR_NULL, @@ -1482,20 +1481,17 @@ void Cache::complete_commit( i->dirty_from_or_retired_at = last_commit; if (is_backref_mapped_extent_node(i) || is_retired_placeholder(i->get_type())) { - DEBUGT("backref_list free {} len {} should release {}", + DEBUGT("backref_list free {} len {}", t, i->get_paddr(), - i->get_length(), - t.should_record_release(i->get_paddr())); - if (t.should_record_release(i->get_paddr())) { - backref_list.emplace_back( - std::make_unique( - i->get_paddr(), - L_ADDR_NULL, - i->get_length(), - i->get_type(), - seq)); - } + i->get_length()); + backref_list.emplace_back( + std::make_unique( + i->get_paddr(), + L_ADDR_NULL, + i->get_length(), + i->get_type(), + seq)); } else if (is_backref_node(i->get_type())) { remove_backref_extent(i->get_paddr()); } else { diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h index 4cb6273217e..f81bc62e5f5 100644 --- a/src/crimson/os/seastore/transaction.h +++ b/src/crimson/os/seastore/transaction.h @@ -277,19 +277,6 @@ public: retired_paddr.add_offset(retired_length) >= paddr.add_offset(len); } - bool should_record_release(paddr_t addr) { - auto count = no_release_delta_retired_set.count(addr); -#ifndef NDEBUG - if (count) - assert(retired_set.count(addr)); -#endif - return count == 0; - } - - void dont_record_release(CachedExtentRef ref) { - no_release_delta_retired_set.insert(ref); - } - template auto for_each_fresh_block(F &&f) const { std::for_each(ool_block_list.begin(), ool_block_list.end(), f); @@ -376,7 +363,6 @@ public: inline_block_list.clear(); ool_block_list.clear(); retired_set.clear(); - no_release_delta_retired_set.clear(); existing_block_list.clear(); existing_block_stats = {}; onode_tree_stats = {}; @@ -533,8 +519,6 @@ private: */ pextent_set_t retired_set; - pextent_set_t no_release_delta_retired_set; - /// stats to collect when commit or invalidate tree_stats_t onode_tree_stats; tree_stats_t omap_tree_stats; // exclude omap tree depth diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc index 1224114b276..963a78e6630 100644 --- a/src/crimson/os/seastore/transaction_manager.cc +++ b/src/crimson/os/seastore/transaction_manager.cc @@ -326,8 +326,7 @@ TransactionManager::submit_transaction( TransactionManager::submit_transaction_direct_ret TransactionManager::submit_transaction_direct( Transaction &tref, - std::optional seq_to_trim, - std::optional> gc_range) + std::optional seq_to_trim) { LOG_PREFIX(TransactionManager::submit_transaction_direct); SUBTRACET(seastore_t, "start", tref); @@ -360,26 +359,12 @@ TransactionManager::submit_transaction_direct( }).si_then([this, FNAME, &tref] { SUBTRACET(seastore_t, "about to prepare", tref); return tref.get_handle().enter(write_pipeline.prepare); - }).si_then([this, FNAME, &tref, seq_to_trim=std::move(seq_to_trim), - gc_range=std::move(gc_range)]() mutable + }).si_then([this, FNAME, &tref, seq_to_trim=std::move(seq_to_trim)]() mutable -> submit_transaction_iertr::future<> { if (seq_to_trim && *seq_to_trim != JOURNAL_SEQ_NULL) { cache->trim_backref_bufs(*seq_to_trim); } -#ifndef NDEBUG - if (gc_range) { - auto backref_set = - backref_manager->get_cached_backrefs_in_range( - gc_range->first, gc_range->second); - for (auto &backref : backref_set) { - ERRORT("unexpected backref: {}~{}, {}, {}, {}", - tref, backref.paddr, backref.len, backref.laddr, - backref.type, backref.seq); - ceph_abort("impossible"); - } - } -#endif auto record = cache->prepare_record(tref, async_cleaner.get()); tref.get_handle().maybe_release_collection_lock(); @@ -558,24 +543,12 @@ TransactionManager::rewrite_extent_ret TransactionManager::rewrite_extent( return rewrite_extent_iertr::now(); } - auto fut = rewrite_extent_iertr::now(); if (extent->is_logical()) { - fut = rewrite_logical_extent(t, extent->cast()); + return rewrite_logical_extent(t, extent->cast()); } else { DEBUGT("rewriting physical extent -- {}", t, *extent); - fut = lba_manager->rewrite_extent(t, extent); + return lba_manager->rewrite_extent(t, extent); } - - return fut.si_then([this, extent, &t] { - t.dont_record_release(extent); - return backref_manager->remove_mapping( - t, extent->get_paddr()).si_then([](auto) { - return seastar::now(); - }).handle_error_interruptible( - crimson::ct_error::input_output_error::pass_further(), - crimson::ct_error::assert_all() - ); - }); } TransactionManager::get_extents_if_live_ret TransactionManager::get_extents_if_live( diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h index 295ba8f35ca..d5c153df43c 100644 --- a/src/crimson/os/seastore/transaction_manager.h +++ b/src/crimson/os/seastore/transaction_manager.h @@ -484,8 +484,7 @@ public: using AsyncCleaner::ExtentCallbackInterface::submit_transaction_direct_ret; submit_transaction_direct_ret submit_transaction_direct( Transaction &t, - std::optional seq_to_trim = std::nullopt, - std::optional> gc_range = std::nullopt) final; + std::optional seq_to_trim = std::nullopt) final; /** * flush -- 2.39.5