Addressed second set of feedback

src/llfs/committable_page_cache_job.cpp

@@ -517,21 +517,12 @@ auto CommittablePageCacheJob::get_page_ref_count_updates(u64 /*callers*/)
     batt::StatusOr<batt::BoxedSeq<PageId>> 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<PageId> CommittablePageCacheJob::finalized_deleted_page_ids() const
-{
-  return BoxedSeq<PageId>{
-      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);
 }
 

src/llfs/committable_page_cache_job.hpp

@@ -206,15 +206,13 @@ class CommittablePageCacheJob
 
   Status drop_deleted_pages(u64 callers);
 
-  BoxedSeq<PageId> finalized_deleted_page_ids() const;
-
   //+++++++++++-+-+--+----- --- -- -  -  -   -
 
   std::shared_ptr<const PageCacheJob> job_;
   boost::intrusive_ptr<FinalizedJobTracker> tracker_;
   PageRefCountUpdates ref_count_updates_;
   std::unique_ptr<WriteNewPagesContext> write_new_pages_context_;
-  std::unordered_set<PageId, PageId::Hash> finalized_deleted_pages_;
+  std::unordered_set<PageId, PageId::Hash> not_found_deleted_pages_;
 };
 
 /** \brief Write all changes in `job` to durable storage.  This is guaranteed to be atomic.

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<OutgoingRefsStatus>(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

src/llfs/no_outgoing_refs_cache.hpp

@@ -19,11 +19,6 @@
 #include <vector>
 
 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.
    */

src/llfs/page_tracer.cpp

@@ -93,7 +93,7 @@ batt::StatusOr<batt::BoxedSeq<PageId>> 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;