Addressed first set of review feedback

src/llfs/api_types.hpp

@@ -81,7 +81,9 @@ BATT_STRONG_TYPEDEF(usize, BufferCount);
  */
 BATT_STRONG_TYPEDEF(usize, BufferSize);
 
-BATT_STRONG_TYPEDEF(bool, HasNoOutgoingRefs);
+/** \brief True if a page contains outgoing references to other pages.
+ */
+BATT_STRONG_TYPEDEF(bool, HasOutgoingRefs);
 
 }  // namespace llfs
 

src/llfs/committable_page_cache_job.cpp

@@ -509,11 +509,9 @@ auto CommittablePageCacheJob::get_page_ref_count_updates(u64 /*callers*/)
   // Trace deleted pages non-recursively, decrementing the ref counts of all pages they directly
   // reference.
   //
-  LoadingPageTracer loading_tracer{loader, true};
+  LoadingPageTracer loading_tracer{loader, /*ok_if_not_found=*/true};
   CachingPageTracer caching_tracer{this->job_->cache().devices_by_id(), loading_tracer};
-  for (const auto& p : this->job_->get_deleted_pages()) {
-    const PageId deleted_page_id = p;
-
+  for (const PageId& deleted_page_id : this->job_->get_deleted_pages()) {
     // Decrement ref counts.
     //
     batt::StatusOr<batt::BoxedSeq<PageId>> outgoing_refs =
@@ -523,7 +521,7 @@ auto CommittablePageCacheJob::get_page_ref_count_updates(u64 /*callers*/)
     }
     BATT_REQUIRE_OK(outgoing_refs);
 
-    ref_count_delta[p] = kRefCount_1_to_0;
+    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
@@ -533,7 +531,7 @@ auto CommittablePageCacheJob::get_page_ref_count_updates(u64 /*callers*/)
     // should be idempotent (as described by the comment on line 275 of this file), we may not need
     // this...
     //
-    this->finalized_deleted_pages_.insert(p);
+    this->finalized_deleted_pages_.insert(deleted_page_id);
 
     *outgoing_refs | seq::for_each([&ref_count_delta, deleted_page_id](PageId id) {
       if (id) {

src/llfs/no_outgoing_refs_cache.cpp

@@ -11,49 +11,65 @@ namespace llfs {
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
-NoOutgoingRefsCache::NoOutgoingRefsCache(u64 physical_page_count) noexcept
-    : cache_(physical_page_count)
+NoOutgoingRefsCache::NoOutgoingRefsCache(const PageIdFactory& page_ids) noexcept
+    : page_ids_{page_ids}
+    , cache_(this->page_ids_.get_physical_page_count().value())
 {
 }
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
-void NoOutgoingRefsCache::set_page_bits(i64 physical_page_id, page_generation_int generation,
-                                        HasNoOutgoingRefs has_no_out_going_refs)
+void NoOutgoingRefsCache::set_page_state(PageId page_id,
+                                         batt::BoolStatus new_has_outgoing_refs_state) noexcept
 {
-  BATT_CHECK_LT((usize)physical_page_id, this->cache_.size());
+  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);
+  const page_generation_int generation = this->page_ids_.get_generation(page_id);
+  BATT_CHECK_LT((usize)physical_page, this->cache_.size());
+
+  u64 new_cache_entry = generation << this->kGenerationShift;
 
-  u64 new_state = generation << this->generation_shift_;
   // Set the "valid" bit to 1.
   //
-  new_state |= (u64{1} << 1);
-  if (has_no_out_going_refs) {
+  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 == batt::BoolStatus::kFalse) {
     // Set the "has no outgoing references" bit to 1.
     //
-    new_state |= u64{1};
+    new_cache_entry |= u64{1};
   }
 
-  u64 old_state = this->cache_[physical_page_id].exchange(new_state, std::memory_order_acq_rel);
+  u64 old_cache_entry =
+      this->cache_[physical_page].exchange(new_cache_entry, std::memory_order_acq_rel);
+
   // Sanity check: we are not setting the bits for the same generation more than once.
   //
-  page_generation_int old_generation = old_state >> this->generation_shift_;
+  page_generation_int old_generation = old_cache_entry >> this->kGenerationShift;
   BATT_CHECK_NE(generation, old_generation);
 }
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
-u64 NoOutgoingRefsCache::get_page_bits(i64 physical_page_id, page_generation_int generation) const
+batt::BoolStatus NoOutgoingRefsCache::has_outgoing_refs(PageId page_id) const noexcept
 {
-  BATT_CHECK_LT((usize)physical_page_id, this->cache_.size());
+  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);
+  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);
+  page_generation_int stored_generation = current_cache_entry >> this->kGenerationShift;
+  OutgoingRefsStatus outgoing_refs_status =
+      static_cast<OutgoingRefsStatus>(current_cache_entry & this->kOutgoingRefsBitMask);
 
-  u64 current_state = this->cache_[physical_page_id].load(std::memory_order_acquire);
-  page_generation_int stored_generation = current_state >> this->generation_shift_;
   // 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 "not traced" status.
+  // are querying for, this cache entry is invalid. Thus, we return a "unknown" status.
   //
-  if (stored_generation != generation) {
-    return u64{0};
+  if (stored_generation != generation || outgoing_refs_status == OutgoingRefsStatus::kNotTraced) {
+    return batt::BoolStatus::kUnknown;
   }
-  return current_state & this->bit_mask_;
+  return batt::bool_status_from(outgoing_refs_status == OutgoingRefsStatus::kHasOutgoingRefs);
 }
 }  // namespace llfs

src/llfs/no_outgoing_refs_cache.hpp

@@ -13,10 +13,18 @@
 #include <llfs/api_types.hpp>
 #include <llfs/page_id_factory.hpp>
 
+#include <batteries/bool_status.hpp>
+
 #include <atomic>
 #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
  * used by an implementer of PageTracer as a way to organize and look up this information on the
@@ -29,29 +37,58 @@ namespace llfs {
 class NoOutgoingRefsCache
 {
  public:
-  explicit NoOutgoingRefsCache(u64 physical_page_count) noexcept;
+  explicit NoOutgoingRefsCache(const PageIdFactory& page_ids) noexcept;
 
   NoOutgoingRefsCache(const NoOutgoingRefsCache&) = delete;
   NoOutgoingRefsCache& operator=(const NoOutgoingRefsCache&) = delete;
 
   //----- --- -- -  -  -   -
-  /** \brief Sets the two outgoing refs state bits for the given `physical_page_id` based on
-   * whether the page has outgoing refs or not, as indicated by `has_outgoing_refs`.
+  /** \brief Sets the two outgoing refs state bits for the given `page_id` based on
+   * whether the page has outgoing refs or not, as indicated by `new_has_outgoing_refs_state`. This
+   * function also updates the generation stored in the cache for the physical page associated with
+   * `page_id`.
+   *
+   * @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.
    */
-  void set_page_bits(i64 physical_page_id, page_generation_int generation,
-                     HasNoOutgoingRefs has_no_outgoing_refs);
+  void set_page_state(PageId page_id, batt::BoolStatus new_has_outgoing_refs_state) noexcept;
 
   //----- --- -- -  -  -   -
-  /** \brief Retrieves the two outgoing refs state bits for the given `physical_page_id`.
+  /** \brief Queries for whether or not the page with id `page_id` contains outgoing references to
+   * other pages.
    *
-   * \return Can return a value of 0 (00), 2 (10), or 3 (11).
+   * \return Returns a `batt::BoolStatus` type, where `kFalse` indicates that the page has no
+   * outgoing refs, `kTrue` indicates that the page does have outgoing refs, and `kUnknown`
+   * indicates that the cache entry for the given page does not have any valid information stored in
+   * it currently.
    */
-  u64 get_page_bits(i64 physical_page_id, page_generation_int generation) const;
+  batt::BoolStatus has_outgoing_refs(PageId page_id) const noexcept;
 
  private:
+  /** \brief A mask to retrieve the two outgoing refs state bits.
+   */
+  static constexpr u64 kOutgoingRefsBitMask = 0b11;
+
+  /** \brief A mask to access the "valid" bit of the two outgoing refs state bits.
+   */
+  static constexpr u64 kValidBitMask = 0b10;
+
+  /** \brief A constant representing the number of bits to shift a cache entry in order retrieve the
+   * generation stored.
+   */
+  static constexpr u8 kGenerationShift = 2;
+
+  /** \brief A PageIdFactory object to help resolve physical page and generation values from a
+   * PageId object.
+   */
+  const PageIdFactory page_ids_;
+
+  /** \brief The vector storing the cache entries, indexed by physical page id.
+   */
   std::vector<std::atomic<u64>> cache_;
-  static constexpr u64 bit_mask_ = 0b11;
-  static constexpr u8 generation_shift_ = 2;
 };
 }  // namespace llfs
 

src/llfs/page_device_entry.hpp

@@ -15,14 +15,15 @@
 #include <llfs/page_device_cache.hpp>
 
 namespace llfs {
+
 /** \brief All the per-PageDevice state for a single device in the storage pool.
  */
 struct PageDeviceEntry {
   explicit PageDeviceEntry(PageArena&& arena,
                            boost::intrusive_ptr<PageCacheSlot::Pool>&& slot_pool) noexcept
       : arena{std::move(arena)}
       , cache{this->arena.device().page_ids(), std::move(slot_pool)}
-      , no_outgoing_refs_cache{this->arena.device().capacity().value()}
+      , no_outgoing_refs_cache{this->arena.device().page_ids()}
   {
   }
 

src/llfs/page_tracer.cpp

@@ -16,10 +16,9 @@ namespace llfs {
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
-LoadingPageTracer::LoadingPageTracer(PageLoader& page_loader,
-                                     batt::Optional<bool> ok_if_not_found) noexcept
+LoadingPageTracer::LoadingPageTracer(PageLoader& page_loader, bool ok_if_not_found) noexcept
     : page_loader_{page_loader}
-    , ok_if_not_found_{*ok_if_not_found}
+    , ok_if_not_found_{ok_if_not_found}
 {
 }
 
@@ -70,36 +69,31 @@ batt::StatusOr<batt::BoxedSeq<PageId>> CachingPageTracer::trace_page_refs(
 {
   const page_device_id_int device_id = PageIdFactory::get_device_id(from_page_id);
   BATT_CHECK_LT(device_id, this->page_devices_.size());
-  const u64 physical_page =
-      this->page_devices_[device_id]->arena.device().page_ids().get_physical_page(from_page_id);
-  const page_generation_int generation =
-      this->page_devices_[device_id]->arena.device().page_ids().get_generation(from_page_id);
 
-  const OutgoingRefsStatus outgoing_refs_status = static_cast<OutgoingRefsStatus>(
-      this->page_devices_[device_id]->no_outgoing_refs_cache.get_page_bits(physical_page,
-                                                                           generation));
+  const batt::BoolStatus has_outgoing_refs =
+      this->page_devices_[device_id]->no_outgoing_refs_cache.has_outgoing_refs(from_page_id);
 
   // If we already have information that this page has no outgoing refs, do not load the page and
   // call trace refs; there's nothing we need to do.
   //
-  if (outgoing_refs_status == OutgoingRefsStatus::kNoOutgoingRefs) {
+  if (has_outgoing_refs == batt::BoolStatus::kFalse) {
     return batt::seq::Empty<PageId>{} | batt::seq::boxed();
   }
 
-  // If outgoing_refs_status has any other status, we must call trace_refs and set
+  // If has_outgoing_refs has any other value, we must call trace_refs and set
   // the outgoing refs information if it hasn't ever been traced before.
   //
   batt::StatusOr<batt::BoxedSeq<PageId>> outgoing_refs =
       this->loader_.trace_page_refs(from_page_id);
   BATT_REQUIRE_OK(outgoing_refs);
 
-  if (outgoing_refs_status == OutgoingRefsStatus::kNotTraced) {
-    bool new_status_no_refs = true;
+  if (has_outgoing_refs == batt::BoolStatus::kUnknown) {
+    bool new_status_has_refs = false;
     if ((*outgoing_refs).peek()) {
-      new_status_no_refs = false;
+      new_status_has_refs = true;
     }
-    this->page_devices_[device_id]->no_outgoing_refs_cache.set_page_bits(
-        physical_page, generation, HasNoOutgoingRefs{new_status_no_refs});
+    this->page_devices_[device_id]->no_outgoing_refs_cache.set_page_state(
+        from_page_id, batt::bool_status_from(HasOutgoingRefs{new_status_has_refs}));
   }
 
   return outgoing_refs;

src/llfs/page_tracer.hpp

@@ -51,10 +51,9 @@ class PageTracer
 class LoadingPageTracer : public PageTracer
 {
  public:
-  explicit LoadingPageTracer(PageLoader& page_loader,
-                             batt::Optional<bool> ok_if_not_found = false) noexcept;
+  explicit LoadingPageTracer(PageLoader& page_loader, bool ok_if_not_found = false) noexcept;
 
-  ~LoadingPageTracer();
+  ~LoadingPageTracer() noexcept;
 
   LoadingPageTracer(const LoadingPageTracer&) = delete;
   LoadingPageTracer& operator=(const LoadingPageTracer&) = delete;
@@ -79,12 +78,6 @@ class LoadingPageTracer : public PageTracer
   bool ok_if_not_found_;
 };
 
-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 PageTracer with the ability to load a page and trace its outgoing references to other
  * pages, as well as look up cached information about a page's outgoing references to other pages.
@@ -101,7 +94,7 @@ class CachingPageTracer : public PageTracer
   CachingPageTracer(const CachingPageTracer&) = delete;
   CachingPageTracer operator=(const CachingPageTracer&) = delete;
 
-  ~CachingPageTracer();
+  ~CachingPageTracer() noexcept;
 
   /** \brief Traces the outgoing references of the page with page id `from_page_id`. First, this
    * function will attempt to see if there exists any cached information about the page's outgoing