From 44ccf2e30c0cc5a6243ad597251834dd99dc6adc Mon Sep 17 00:00:00 2001 From: Vidya Silai Date: Mon, 4 Nov 2024 08:58:44 -0500 Subject: [PATCH] Addressed second set of feedback --- src/llfs/committable_page_cache_job.cpp | 27 +++------- src/llfs/committable_page_cache_job.hpp | 4 +- src/llfs/no_outgoing_refs_cache.cpp | 50 ++++++++++++----- src/llfs/no_outgoing_refs_cache.hpp | 29 ++++++---- src/llfs/page_tracer.cpp | 2 +- src/llfs/page_tracer.test.cpp | 71 ++++++++++++++----------- 6 files changed, 105 insertions(+), 78 deletions(-) diff --git a/src/llfs/committable_page_cache_job.cpp b/src/llfs/committable_page_cache_job.cpp index d3bc578..c8b023a 100644 --- a/src/llfs/committable_page_cache_job.cpp +++ b/src/llfs/committable_page_cache_job.cpp @@ -517,21 +517,12 @@ auto CommittablePageCacheJob::get_page_ref_count_updates(u64 /*callers*/) batt::StatusOr> outgoing_refs = caching_tracer.trace_page_refs(deleted_page_id); if (outgoing_refs.status() == batt::StatusCode::kNotFound) { + this->not_found_deleted_pages_.insert(deleted_page_id); continue; } BATT_REQUIRE_OK(outgoing_refs); ref_count_delta[deleted_page_id] = kRefCount_1_to_0; - // TODO [vsilai 2024-10-28] not sure if finalized_deleted_pages_ is needed. Given how we - // restructured this function and `delete_page` in PageCacheJob, I think that we have to - // ensure that we don't accidentally drop a page that wasn't found when `trace_page_refs` was - // called above, because we keep this page id in the job's deleted_pages_ set and simply call - // continue in the loop. Before, `drop_pages` looked into the job's deleted_pages_ set to create - // a vector of all the ids to drop, which may cause this accidental drop. However, since drops - // should be idempotent (as described by the comment on line 275 of this file), we may not need - // this... - // - this->finalized_deleted_pages_.insert(deleted_page_id); *outgoing_refs | seq::for_each([&ref_count_delta, deleted_page_id](PageId id) { if (id) { @@ -599,21 +590,19 @@ Status CommittablePageCacheJob::recycle_dead_pages(const JobCommitParams& params // the page recycler's log. } -//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - -// -BoxedSeq CommittablePageCacheJob::finalized_deleted_page_ids() const -{ - return BoxedSeq{ - as_seq(this->finalized_deleted_pages_.begin(), this->finalized_deleted_pages_.end())}; -} - //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // Status CommittablePageCacheJob::drop_deleted_pages(u64 callers) { LLFS_VLOG(1) << "commit(PageCacheJob): dropping deleted pages"; - return parallel_drop_pages(this->finalized_deleted_page_ids() | seq::collect_vec(), + // From the set of all deleted pages, filter out those that were not found during the attempt to + // trace their outgoing refs. + // + return parallel_drop_pages(this->deleted_page_ids() | seq::filter([this](const PageId& id) { + auto iter = this->not_found_deleted_pages_.find(id); + return iter == this->not_found_deleted_pages_.end(); + }) | seq::collect_vec(), this->job_->cache(), this->job_->job_id, callers); } diff --git a/src/llfs/committable_page_cache_job.hpp b/src/llfs/committable_page_cache_job.hpp index 4012ac3..3e51503 100644 --- a/src/llfs/committable_page_cache_job.hpp +++ b/src/llfs/committable_page_cache_job.hpp @@ -206,15 +206,13 @@ class CommittablePageCacheJob Status drop_deleted_pages(u64 callers); - BoxedSeq finalized_deleted_page_ids() const; - //+++++++++++-+-+--+----- --- -- - - - - std::shared_ptr job_; boost::intrusive_ptr tracker_; PageRefCountUpdates ref_count_updates_; std::unique_ptr write_new_pages_context_; - std::unordered_set finalized_deleted_pages_; + std::unordered_set not_found_deleted_pages_; }; /** \brief Write all changes in `job` to durable storage. This is guaranteed to be atomic. diff --git a/src/llfs/no_outgoing_refs_cache.cpp b/src/llfs/no_outgoing_refs_cache.cpp index 241f35f..9b0797c 100644 --- a/src/llfs/no_outgoing_refs_cache.cpp +++ b/src/llfs/no_outgoing_refs_cache.cpp @@ -20,7 +20,7 @@ NoOutgoingRefsCache::NoOutgoingRefsCache(const PageIdFactory& page_ids) noexcept //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // void NoOutgoingRefsCache::set_page_state(PageId page_id, - batt::BoolStatus new_has_outgoing_refs_state) noexcept + HasOutgoingRefs new_has_outgoing_refs_state) noexcept { BATT_CHECK_EQ(PageIdFactory::get_device_id(page_id), this->page_ids_.get_device_id()); const u64 physical_page = this->page_ids_.get_physical_page(page_id); @@ -33,21 +33,26 @@ void NoOutgoingRefsCache::set_page_state(PageId page_id, // new_cache_entry |= this->kValidBitMask; - // If new_has_outgoing_refs_state has value kFalse, page_id has no outgoing references. + // If new_has_outgoing_refs_state has value false, page_id has no outgoing references. // - if (new_has_outgoing_refs_state == batt::BoolStatus::kFalse) { + if (!new_has_outgoing_refs_state) { // Set the "has no outgoing references" bit to 1. // - new_cache_entry |= u64{1}; + new_cache_entry |= this->kHasNoOutgoingRefsBitMask; } - u64 old_cache_entry = - this->cache_[physical_page].exchange(new_cache_entry, std::memory_order_acq_rel); + u64 old_cache_entry = this->cache_[physical_page].exchange(new_cache_entry); - // Sanity check: we are not setting the bits for the same generation more than once. + // Two sanity checks: + // 1) We are not going backwards in generation. + // 2) If the cache entry is set of the same generation multiple times, the same value should be + // set. // page_generation_int old_generation = old_cache_entry >> this->kGenerationShift; - BATT_CHECK_NE(generation, old_generation); + BATT_CHECK_GE(generation, old_generation); + if (generation == old_generation) { + BATT_CHECK_EQ(new_cache_entry, old_cache_entry); + } } //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - @@ -59,17 +64,36 @@ batt::BoolStatus NoOutgoingRefsCache::has_outgoing_refs(PageId page_id) const no const page_generation_int generation = this->page_ids_.get_generation(page_id); BATT_CHECK_LT((usize)physical_page, this->cache_.size()); - u64 current_cache_entry = this->cache_[physical_page].load(std::memory_order_acquire); + u64 current_cache_entry = this->cache_[physical_page].load(); page_generation_int stored_generation = current_cache_entry >> this->kGenerationShift; - OutgoingRefsStatus outgoing_refs_status = - static_cast(current_cache_entry & this->kOutgoingRefsBitMask); + u64 outgoing_refs_status = current_cache_entry & this->kOutgoingRefsStatusBitsMask; // If the generation that is currently stored in the cache is not the same as the generation we // are querying for, this cache entry is invalid. Thus, we return a "unknown" status. // - if (stored_generation != generation || outgoing_refs_status == OutgoingRefsStatus::kNotTraced) { + if (stored_generation != generation) { return batt::BoolStatus::kUnknown; } - return batt::bool_status_from(outgoing_refs_status == OutgoingRefsStatus::kHasOutgoingRefs); + + switch (outgoing_refs_status) { + case 0: + // Bit status 00, not traced yet. + // + return batt::BoolStatus::kUnknown; + case 1: + BATT_PANIC() << "The lower two outgoing refs bits in a cache entry can never be 01!"; + BATT_UNREACHABLE(); + case 2: + // Bit status 10, has outgoing refs. + // + return batt::BoolStatus::kTrue; + case 3: + // Bit status 11, no outgoing refs. + // + return batt::BoolStatus::kFalse; + default: + BATT_PANIC() << "Impossible outgoing refs bits state: " << outgoing_refs_status; + BATT_UNREACHABLE(); + } } } // namespace llfs \ No newline at end of file diff --git a/src/llfs/no_outgoing_refs_cache.hpp b/src/llfs/no_outgoing_refs_cache.hpp index e040c00..7d7410f 100644 --- a/src/llfs/no_outgoing_refs_cache.hpp +++ b/src/llfs/no_outgoing_refs_cache.hpp @@ -19,11 +19,6 @@ #include namespace llfs { -enum struct OutgoingRefsStatus : u8 { - kNotTraced = 0, // bit state 00, invalid - kHasOutgoingRefs = 2, // bit state 10, valid with outgoing refs - kNoOutgoingRefs = 3, // bit state 11, valid with no outgoing refs -}; //=#=#==#==#===============+=+=+=+=++=++++++++++++++-++-+--+-+----+--------------- /** \brief A cache to store information about a page's outgoing references to other pages. Can be @@ -33,6 +28,12 @@ enum struct OutgoingRefsStatus : u8 { * The lowest bit in an element represents if the page has no outgoing refs. The second lowest bit * represents the validity of the page's state to help determine if a page's outgoing refs have ever * been traced. The remaining upper 62 bits are used to store the generation of the physical page. + * + * Example cache entry: 0000000000000000000000000000000000000000000000000000000000000110 + * The upper 62 bits represent generation, which in this case is 1. The second lowest bit is the + * validity bit, which is 1 here. This indicates that the page has been traced and therefore the + * cache entry contains valid information for the page of this generation. The lowest bit is 0, + * which indicates that the page has outgoing references to other pages. */ class NoOutgoingRefsCache { @@ -51,10 +52,10 @@ class NoOutgoingRefsCache * @param page_id The id of the page whose outgoing refs information we are storing. * * @param new_has_outgoing_refs_state The status to store, indicating whether or not the page has - * outgoing refs. A value of `kFalse` indicates that the page does not have any outgoing refs, and - * a value of `kTrue` indicates that a page does have outgoing refs. + * outgoing refs. A value of `false` indicates that the page does not have any outgoing refs, and + * a value of `true` indicates that a page does have outgoing refs. */ - void set_page_state(PageId page_id, batt::BoolStatus new_has_outgoing_refs_state) noexcept; + void set_page_state(PageId page_id, HasOutgoingRefs new_has_outgoing_refs_state) noexcept; //----- --- -- - - - - /** \brief Queries for whether or not the page with id `page_id` contains outgoing references to @@ -68,14 +69,20 @@ class NoOutgoingRefsCache batt::BoolStatus has_outgoing_refs(PageId page_id) const noexcept; private: - /** \brief A mask to retrieve the two outgoing refs state bits. + /** \brief A mask to retrieve the lowest two outgoing refs state bits. */ - static constexpr u64 kOutgoingRefsBitMask = 0b11; + static constexpr u64 kOutgoingRefsStatusBitsMask = 0b11; - /** \brief A mask to access the "valid" bit of the two outgoing refs state bits. + /** \brief A mask to access the "valid" bit of the two outgoing refs state bits. This is the + * higher of the two bits. */ static constexpr u64 kValidBitMask = 0b10; + /** \brief A mask to access the "has no outgoing references" bit of the two outgoing refs state + * bits. This is the lower of the two bits. + */ + static constexpr u64 kHasNoOutgoingRefsBitMask = 0b01; + /** \brief A constant representing the number of bits to shift a cache entry in order retrieve the * generation stored. */ diff --git a/src/llfs/page_tracer.cpp b/src/llfs/page_tracer.cpp index f53142a..aea58e0 100644 --- a/src/llfs/page_tracer.cpp +++ b/src/llfs/page_tracer.cpp @@ -93,7 +93,7 @@ batt::StatusOr> CachingPageTracer::trace_page_refs( new_status_has_refs = true; } this->page_devices_[device_id]->no_outgoing_refs_cache.set_page_state( - from_page_id, batt::bool_status_from(HasOutgoingRefs{new_status_has_refs})); + from_page_id, HasOutgoingRefs{new_status_has_refs}); } return outgoing_refs; diff --git a/src/llfs/page_tracer.test.cpp b/src/llfs/page_tracer.test.cpp index e718ca7..6c53493 100644 --- a/src/llfs/page_tracer.test.cpp +++ b/src/llfs/page_tracer.test.cpp @@ -237,10 +237,10 @@ class PageTracerTest : public ::testing::Test /** \brief Get the outgoing reference status of the given page with id `page_id`. * - * \return An OutgoingRefsStatus value telling the caller if the page has outgoing references to + * \return An u64 value telling the caller if the page has outgoing references to * other pages, does not have any outgoing references, or has never been traced yet. */ - llfs::OutgoingRefsStatus get_outgoing_refs_status(llfs::PageId page_id) + batt::BoolStatus get_outgoing_refs_status(llfs::PageId page_id) { const std::vector>& page_devices = this->page_cache->devices_by_id(); @@ -248,16 +248,7 @@ class PageTracerTest : public ::testing::Test batt::BoolStatus has_outgoing_refs = page_devices[device]->no_outgoing_refs_cache.has_outgoing_refs(page_id); - switch (has_outgoing_refs) { - case batt::BoolStatus::kFalse: - return llfs::OutgoingRefsStatus::kNoOutgoingRefs; - case batt::BoolStatus::kTrue: - return llfs::OutgoingRefsStatus::kHasOutgoingRefs; - case batt::BoolStatus::kUnknown: - return llfs::OutgoingRefsStatus::kNotTraced; - default: - return llfs::OutgoingRefsStatus::kNotTraced; - } + return has_outgoing_refs; } /** \brief A utility function that triggers a call to `trace_refs_recursive` with the PageCache as @@ -311,7 +302,7 @@ TEST_F(PageTracerTest, NoOutgoingRefsCacheDeath) { const std::unique_ptr& page_device1 = this->page_cache->devices_by_id()[0]; - batt::BoolStatus no_outgoing_refs = batt::bool_status_from(llfs::HasOutgoingRefs{false}); + llfs::HasOutgoingRefs no_outgoing_refs{false}; llfs::PageId page = page_device1->arena.device().page_ids().make_page_id(1, 1); // Create a page with physical page id 1 and generation 1, then set and get its page bits in the @@ -326,10 +317,11 @@ TEST_F(PageTracerTest, NoOutgoingRefsCacheDeath) // EXPECT_EQ(has_outgoing_refs, batt::BoolStatus::kFalse); - // We can't set the outgoing refs bits for the same generation of a page twice! + // We can't set different outgoing refs bits for the same generation of a page! // - EXPECT_DEATH(page_device1->no_outgoing_refs_cache.set_page_state(page, no_outgoing_refs), - "Assertion failed: generation != old_generation"); + EXPECT_DEATH( + page_device1->no_outgoing_refs_cache.set_page_state(page, llfs::HasOutgoingRefs{true}), + "Assertion failed: new_cache_entry == old_cache_entry"); page = page_device1->arena.device().page_ids().make_page_id(2, 1); page_device1->no_outgoing_refs_cache.set_page_state(page, no_outgoing_refs); @@ -355,7 +347,11 @@ TEST_F(PageTracerTest, CachingPageTracerTest) // std::unordered_set not_traced_set; std::vector leaves; - std::unordered_map + + // BoolStatus kFalse: page has no outgoing refs. + // BoolStatus kTrue: page has outgoing refs. + // + std::unordered_map expected_outgoing_refs_status; // Use half of the PageDevice's capacity to make leaf pages, i.e. pages with no outgoing refs. @@ -366,7 +362,7 @@ TEST_F(PageTracerTest, CachingPageTracerTest) ASSERT_TRUE(leaf.ok()) << BATT_INSPECT(leaf.status()); not_traced_set.insert(*leaf); leaves.emplace_back(*leaf); - expected_outgoing_refs_status[*leaf] = llfs::OutgoingRefsStatus::kNoOutgoingRefs; + expected_outgoing_refs_status[*leaf] = batt::BoolStatus::kFalse; } // Use (half of PageDevice's capacity - 1) number of pages for pages with outgoing refs. @@ -381,7 +377,7 @@ TEST_F(PageTracerTest, CachingPageTracerTest) batt::StatusOr root = this->build_page_with_refs_to(outgoing_refs, llfs::PageSize{256}, *job); job->new_root(*root); - expected_outgoing_refs_status[*root] = llfs::OutgoingRefsStatus::kHasOutgoingRefs; + expected_outgoing_refs_status[*root] = batt::BoolStatus::kTrue; ++num_pages_with_outgoing_refs; // Those leaves that were initially placed in the not_traced set are now traceable since they @@ -399,7 +395,10 @@ TEST_F(PageTracerTest, CachingPageTracerTest) this->build_page_with_refs_to({}, llfs::PageSize{256}, *job); not_traced_set.insert(*island); for (const llfs::PageId& id : not_traced_set) { - expected_outgoing_refs_status[id] = llfs::OutgoingRefsStatus::kNotTraced; + // All non traceable pages will never has their outgoing refs information set, so their status + // will be kUnknown. + // + expected_outgoing_refs_status[id] = batt::BoolStatus::kUnknown; } batt::Status trace_status = job->trace_new_roots(/*page_loader=*/*job, /*page_id_fn=*/ @@ -411,7 +410,7 @@ TEST_F(PageTracerTest, CachingPageTracerTest) // Verify the OutgoingRefsStatus for each page. // for (const auto& [page_id, expected_status] : expected_outgoing_refs_status) { - llfs::OutgoingRefsStatus actual_status = this->get_outgoing_refs_status(page_id); + batt::BoolStatus actual_status = this->get_outgoing_refs_status(page_id); EXPECT_EQ(actual_status, expected_status); } @@ -436,8 +435,11 @@ TEST_F(PageTracerTest, VolumeAndRecyclerTest) return llfs::OkStatus(); }); - std::unordered_map - expected_outgoing_refs; + // BoolStatus kFalse: page has no outgoing refs. + // BoolStatus kTrue: page has outgoing refs. + // + std::unordered_map expected_outgoing_refs; + u64 num_pages_created = 0; // Create the first job with some new pages and commit it. // @@ -446,14 +448,18 @@ TEST_F(PageTracerTest, VolumeAndRecyclerTest) batt::StatusOr leaf1 = this->build_page_with_refs_to({}, llfs::PageSize{256}, *job1); ASSERT_TRUE(leaf1.ok()) << BATT_INSPECT(leaf1.status()); + ++num_pages_created; batt::StatusOr leaf2 = this->build_page_with_refs_to({}, llfs::PageSize{256}, *job1); ASSERT_TRUE(leaf2.ok()) << BATT_INSPECT(leaf2.status()); + ++num_pages_created; batt::StatusOr root1 = this->build_page_with_refs_to({*leaf1, *leaf2}, llfs::PageSize{256}, *job1); ASSERT_TRUE(root1.ok()) << BATT_INSPECT(root1.status()); + ++num_pages_created; + llfs::StatusOr commit_root1 = this->commit_job(*test_volume, std::move(job1), *root1); ASSERT_TRUE(commit_root1.ok()) << BATT_INSPECT(commit_root1.status()); @@ -465,24 +471,27 @@ TEST_F(PageTracerTest, VolumeAndRecyclerTest) batt::StatusOr root2 = this->build_page_with_refs_to({*leaf1}, llfs::PageSize{256}, *job2); ASSERT_TRUE(root2.ok()) << BATT_INSPECT(root2.status()); + ++num_pages_created; + llfs::StatusOr commit_root2 = this->commit_job(*test_volume, std::move(job2), *root2); ASSERT_TRUE(commit_root2.ok()) << BATT_INSPECT(commit_root2.status()); - expected_outgoing_refs[*leaf1] = llfs::OutgoingRefsStatus::kNoOutgoingRefs; - expected_outgoing_refs[*leaf2] = llfs::OutgoingRefsStatus::kNoOutgoingRefs; - expected_outgoing_refs[*root1] = llfs::OutgoingRefsStatus::kHasOutgoingRefs; - expected_outgoing_refs[*root2] = llfs::OutgoingRefsStatus::kHasOutgoingRefs; + expected_outgoing_refs[*leaf1] = batt::BoolStatus::kFalse; + expected_outgoing_refs[*leaf2] = batt::BoolStatus::kFalse; + expected_outgoing_refs[*root1] = batt::BoolStatus::kTrue; + expected_outgoing_refs[*root2] = batt::BoolStatus::kTrue; // Create the last job to fill up the PageDevice. // std::unique_ptr job3 = test_volume->new_job(); batt::StatusOr page = this->build_page_with_refs_to({}, llfs::PageSize{256}, *job3); - expected_outgoing_refs[*page] = llfs::OutgoingRefsStatus::kNoOutgoingRefs; - for (usize i = 0; i < (this->page_device_capacity - 5); ++i) { + ++num_pages_created; + expected_outgoing_refs[*page] = batt::BoolStatus::kFalse; + for (usize i = 0; i < (this->page_device_capacity - num_pages_created); ++i) { llfs::PageId prev_page{*page}; page = this->build_page_with_refs_to({prev_page}, llfs::PageSize{256}, *job3); - expected_outgoing_refs[*page] = llfs::OutgoingRefsStatus::kHasOutgoingRefs; + expected_outgoing_refs[*page] = batt::BoolStatus::kTrue; } llfs::StatusOr commit_remaining = this->commit_job(*test_volume, std::move(job3), *page); @@ -495,7 +504,7 @@ TEST_F(PageTracerTest, VolumeAndRecyclerTest) // Verify the outgoing refs status. // for (const auto& [page_id, expected_status] : expected_outgoing_refs) { - llfs::OutgoingRefsStatus actual_status = this->get_outgoing_refs_status(page_id); + batt::BoolStatus actual_status = this->get_outgoing_refs_status(page_id); EXPECT_EQ(actual_status, expected_status); }