Add null check to PackedPointer. (#176)

Add ScenarioRunner (testing); more static checks for cache slots/refs.

Fix header double-inclusion guard name.

CR feedback.

Author: tonyastolfi

script

@@ -89,6 +89,10 @@ BATT_STRONG_TYPEDEF(bool, HasOutgoingRefs);
  */
 BATT_STRONG_TYPEDEF(usize, ItemOffset);
 
+/** \brief A pseudo-random number generator seed.
+ */
+BATT_STRONG_TYPEDEF(u32, RandomSeed);
+
 }  // namespace llfs
 
 #endif  // LLFS_API_TYPES_HPP

src/llfs/api_types.hpp

@@ -19,6 +19,8 @@
 #include <llfs/ioring_log_device.test.hpp>
 #include <llfs/storage_simulation.hpp>
 
+#include <llfs/testing/scenario_runner.hpp>
+
 namespace {
 
 using namespace llfs::constants;
@@ -511,9 +513,6 @@ class IoRingLogDevice2SimTest : public ::testing::Test
 
 TEST_F(IoRingLogDevice2SimTest, Simulation)
 {
-  const usize kNumThreads = std::thread::hardware_concurrency();
-  const usize kUpdateInterval = kNumSeeds / 20;
-
   llfs::testing::TestConfig test_config;
 
   LLFS_LOG_INFO() << BATT_INSPECT(kNumSeeds);
@@ -527,28 +526,10 @@ TEST_F(IoRingLogDevice2SimTest, Simulation)
   EXPECT_FALSE(goals.tail_collision);
   EXPECT_FALSE(goals.tail_rewrite);
 
-  std::vector<std::thread> threads;
-  usize start_seed = test_config.get_random_seed();
-  usize seeds_remaining = kNumSeeds;
-  for (usize i = 0; i < kNumThreads; ++i) {
-    usize n_seeds = seeds_remaining / (kNumThreads - i);
-    usize end_seed = start_seed + n_seeds;
-    threads.emplace_back([&, start_seed, end_seed] {
-      for (usize seed = start_seed; seed < end_seed; seed += 1) {
-        LLFS_LOG_INFO_EVERY_N(kUpdateInterval)
-            << "progress=" << (seed - start_seed) * 100 / (end_seed - start_seed) << "%";
-
-        Scenario scenario{seed, goals};
-        ASSERT_NO_FATAL_FAILURE(scenario.run());
-      }
-    });
-    start_seed += n_seeds;
-    seeds_remaining -= n_seeds;
-  }
-
-  for (std::thread& t : threads) {
-    t.join();
-  }
+  llfs::testing::ScenarioRunner{}  //
+      .n_seeds(kNumSeeds)
+      .n_updates(20)
+      .run(batt::StaticType<Scenario>{}, goals);
 
   EXPECT_TRUE(goals.concurrent_writes);
   EXPECT_TRUE(goals.burst_mode_checked);

src/llfs/ioring_log_device2.test.cpp

@@ -43,6 +43,7 @@ struct PackedPointer {
   const T* get() const
   {
     BATT_STATIC_ASSERT_EQ(sizeof(PackedPointer), sizeof(Offset));
+    BATT_CHECK_NE(this->offset, 0);
 
     return reinterpret_cast<const T*>(reinterpret_cast<const u8*>(this) + this->offset);
   }

src/llfs/packed_pointer.hpp

@@ -76,7 +76,7 @@ bool PageCacheSlot::is_pinned() const noexcept
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
-u32 PageCacheSlot::pin_count() const noexcept
+u64 PageCacheSlot::pin_count() const noexcept
 {
   return Self::get_pin_count(this->state_.load(std::memory_order_acquire));
 }
@@ -116,11 +116,11 @@ auto PageCacheSlot::acquire_pin(PageId key, bool ignore_key) noexcept -> PinnedR
 {
   const auto old_state = this->state_.fetch_add(kPinCountDelta, std::memory_order_acquire);
   const auto new_state = old_state + kPinCountDelta;
-  const bool newly_pinned = !this->is_pinned(old_state);
+  const bool newly_pinned = !Self::is_pinned(old_state);
 
   BATT_CHECK_EQ(new_state & Self::kOverflowMask, 0);
 
-  BATT_CHECK(this->is_pinned(new_state));
+  BATT_CHECK(Self::is_pinned(new_state));
 
   BATT_SUPPRESS_IF_GCC("-Wmaybe-uninitialized")
 
@@ -134,7 +134,7 @@ auto PageCacheSlot::acquire_pin(PageId key, bool ignore_key) noexcept -> PinnedR
   // If the pin_count > 1 (because of the fetch_add above) and the slot is valid, it is safe to read
   // the key.  If the key doesn't match, release the ref and return failure.
   //
-  if (!this->is_valid(old_state) ||
+  if (!Self::is_valid(old_state) ||
       (!ignore_key && (!this->key_.is_valid() || this->key_ != key))) {
     this->release_pin();
     return PinnedRef{};
@@ -149,7 +149,7 @@ auto PageCacheSlot::acquire_pin(PageId key, bool ignore_key) noexcept -> PinnedR
     BATT_CHECK(this->value_);
   }
 
-  return PinnedRef{this};
+  return PinnedRef{this, CallerPromisesTheyAcquiredPinCount{}};
 }
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
@@ -199,7 +199,7 @@ void PageCacheSlot::release_pin() noexcept
       << BATT_INSPECT(old_state);
 
   const auto new_state = old_state - kPinCountDelta;
-  const bool newly_unpinned = !this->is_pinned(new_state);
+  const bool newly_unpinned = !Self::is_pinned(new_state);
 
   BATT_CHECK_EQ(new_state & Self::kOverflowMask, 0);
 
@@ -221,7 +221,7 @@ bool PageCacheSlot::evict() noexcept
   //
   auto observed_state = this->state_.load(std::memory_order_acquire);
   for (;;) {
-    if (Self::is_pinned(observed_state) || !this->is_valid(observed_state)) {
+    if (Self::is_pinned(observed_state) || !Self::is_valid(observed_state)) {
       return false;
     }
 
@@ -244,7 +244,7 @@ bool PageCacheSlot::evict_if_key_equals(PageId key) noexcept
   const auto old_state = this->state_.fetch_add(kPinCountDelta, std::memory_order_acquire);
   auto observed_state = old_state + kPinCountDelta;
 
-  const bool newly_pinned = !this->is_pinned(old_state);
+  const bool newly_pinned = !Self::is_pinned(old_state);
   if (newly_pinned) {
     this->add_ref();
   }
@@ -257,7 +257,7 @@ bool PageCacheSlot::evict_if_key_equals(PageId key) noexcept
   for (;;) {
     // To succeed, we must be holding the only pin, the slot must be valid, and the key must match.
     //
-    if (!(Self::get_pin_count(observed_state) == 1 && this->is_valid(observed_state) &&
+    if (!(Self::get_pin_count(observed_state) == 1 && Self::is_valid(observed_state) &&
           this->key_ == key)) {
       this->release_pin();
       return false;
@@ -300,7 +300,7 @@ auto PageCacheSlot::fill(PageId key) noexcept -> PinnedRef
   this->add_ref();
   this->set_valid();
 
-  return PinnedRef{this};
+  return PinnedRef{this, CallerPromisesTheyAcquiredPinCount{}};
 }
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
@@ -333,7 +333,7 @@ void PageCacheSlot::notify_last_ref_released()
 void PageCacheSlot::set_valid()
 {
   const auto observed_state = this->state_.fetch_or(kValidMask, std::memory_order_release);
-  BATT_CHECK(!this->is_valid(observed_state)) << "Must go from an invalid state to valid!";
+  BATT_CHECK(!Self::is_valid(observed_state)) << "Must go from an invalid state to valid!";
 }
 
 }  //namespace llfs

src/llfs/page_cache_slot.cpp

@@ -107,7 +107,7 @@ class PageCacheSlot
 
   /** \brief Returns the pin count bit field within the passed state integer.
    */
-  static constexpr u32 get_pin_count(u64 state)
+  static constexpr u64 get_pin_count(u64 state)
   {
     return state >> kPinCountShift;
   }
@@ -202,7 +202,7 @@ class PageCacheSlot
 
   /** \brief Returns the current pin count of the slot; if this is 0, the slot is not pinned.
    */
-  u32 pin_count() const noexcept;
+  u64 pin_count() const noexcept;
 
   /** \brief Conditionally pins the slot so it can't be evicted.
    *

src/llfs/page_cache_slot.hpp

@@ -68,7 +68,7 @@ class PageCacheSlotTest : public ::testing::Test
 //  1. Create slots with different index values in a Pool; verify index()
 //     - also verify initial is_valid state
 //
-TEST_F(PageCacheSlotTest, CreateSlotsDeath)
+TEST_F(PageCacheSlotTest, CreateSlots)
 {
   for (usize i = 0; i < kNumTestSlots; ++i) {
     llfs::PageCacheSlot* slot = this->pool_->allocate();
@@ -152,7 +152,8 @@ TEST_F(PageCacheSlotTest, StateTransitions)
   //     i. Valid + Cleared --(acquire_pin)--> Valid + Cleared + Pinned
   //
   {
-    llfs::PageCacheSlot::PinnedRef valid_cleared_ref = slot->acquire_pin(llfs::PageId{}, /*ignore_key=*/true);
+    llfs::PageCacheSlot::PinnedRef valid_cleared_ref =
+        slot->acquire_pin(llfs::PageId{}, /*ignore_key=*/true);
     EXPECT_EQ(valid_cleared_ref.value(), nullptr);
     EXPECT_EQ(slot->pin_count(), 1u);
     EXPECT_EQ(slot->ref_count(), 1u);
@@ -421,7 +422,8 @@ TEST_F(PageCacheSlotTest, FillFailureClearedDeath)
   slot->clear();
 
   {
-    llfs::PageCacheSlot::PinnedRef valid_cleared_ref = slot->acquire_pin(llfs::PageId{}, /*ignore_key=*/true);
+    llfs::PageCacheSlot::PinnedRef valid_cleared_ref =
+        slot->acquire_pin(llfs::PageId{}, /*ignore_key=*/true);
     EXPECT_TRUE(slot->is_valid());
     EXPECT_DEATH(slot->fill(llfs::PageId{2}), "Assert.*fail.*is.*valid");
   }
@@ -473,12 +475,13 @@ TEST_F(PageCacheSlotTest, RefCounting)
         continue;
       }
 
-      // Split the workload: let half the threads perform acquire_pin calls and 
+      // Split the workload: let half the threads perform acquire_pin calls and
       // let the other half perform evictions.
       //
       if (i % 2) {
         {
-          llfs::PageCacheSlot::PinnedRef pinned = slot->acquire_pin(llfs::PageId{}, /*ignore_key=*/true);
+          llfs::PageCacheSlot::PinnedRef pinned =
+              slot->acquire_pin(llfs::PageId{}, /*ignore_key=*/true);
         }
       } else {
         slot->evict_if_key_equals(llfs::PageId{1});

src/llfs/page_cache_slot.test.cpp

@@ -12,6 +12,26 @@
 
 namespace llfs {
 
+//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
+//
+class CallerPromisesTheyAcquiredPinCount
+{
+ private:
+  CallerPromisesTheyAcquiredPinCount() = default;
+
+  //+++++++++++-+-+--+----- --- -- -  -  -   -
+  // The following declared "friend" functions are the only places that should be acquiring a pin on
+  // a cache slot and using it to create a PinnedRef!
+  //+++++++++++-+-+--+----- --- -- -  -  -   -
+
+  friend auto PageCacheSlot::acquire_pin(PageId key,
+                                         bool ignore_key) noexcept -> PageCacheSlot::PinnedRef;
+
+  friend auto PageCacheSlot::fill(PageId key) noexcept -> PageCacheSlot::PinnedRef;
+};
+
+//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
+//
 class PageCacheSlot::PinnedRef : public boost::equality_comparable<PageCacheSlot::PinnedRef>
 {
  public:
@@ -22,7 +42,7 @@ class PageCacheSlot::PinnedRef : public boost::equality_comparable<PageCacheSlot
   PinnedRef() = default;
 
  private:
-  explicit PinnedRef(PageCacheSlot* slot) noexcept
+  explicit PinnedRef(PageCacheSlot* slot, CallerPromisesTheyAcquiredPinCount) noexcept
       : slot_{slot}
       , value_{slot ? slot->value() : nullptr}
   {

src/llfs/page_cache_slot_pinned_ref.hpp

@@ -49,6 +49,18 @@ class StorageSimulation::TaskSchedulerImpl : public batt::TaskScheduler
 
 //=#=#==#==#===============+=+=+=+=++=++++++++++++++-++-+--+-+----+---------------
 
+//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
+//
+/*explicit*/ StorageSimulation::StorageSimulation(RandomSeed seed) noexcept
+    : StorageSimulation{batt::StateMachineEntropySource{
+          /*entropy_fn=*/[rng = std::make_shared<std::default_random_engine>(seed)](
+                             usize min_value, usize max_value) -> usize {
+            std::uniform_int_distribution<usize> pick_value{min_value, max_value};
+            return pick_value(*rng);
+          }}}
+{
+}
+
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
 /*explicit*/ StorageSimulation::StorageSimulation(
@@ -423,6 +435,16 @@ const batt::SharedPtr<PageCache>& StorageSimulation::init_cache() noexcept
   return this->cache_;
 }
 
+//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
+//
+std::unique_ptr<LogDeviceFactory> StorageSimulation::get_log_device_factory(const std::string& name,
+                                                                            Optional<u64> capacity)
+{
+  return std::make_unique<BasicLogDeviceFactory>([this, name, capacity] {
+    return this->get_log_device(name, capacity);
+  });
+}
+
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
 StatusOr<std::unique_ptr<Volume>> StorageSimulation::get_volume(

src/llfs/storage_simulation.cpp

@@ -54,6 +54,11 @@ class StorageSimulation
 
   //+++++++++++-+-+--+----- --- -- -  -  -   -
 
+  /** \brief Creates a new StorageSimulation using a default random engine seeded with the passed
+   * value.
+   */
+  explicit StorageSimulation(RandomSeed seed) noexcept;
+
   /** \brief Creates a new StorageSimulation context that draws from the passed entropy source.
    */
   explicit StorageSimulation(batt::StateMachineEntropySource&& entropy_source) noexcept;
@@ -121,13 +126,42 @@ class StorageSimulation
    */
   batt::TaskScheduler& task_scheduler() noexcept;
 
+  /** \brief Convenience: invokes this->task_scheduler().schedule_task() and returns the resulting
+   * executor.
+   */
+  boost::asio::any_io_executor schedule_task() noexcept
+  {
+    return this->task_scheduler().schedule_task();
+  }
+
+  /** \brief Convenience: invokes this->entropy_source().pick_int(min_value, max_value).
+   */
+  usize pick_int(usize min_value, usize max_value) const
+  {
+    return this->entropy_source().pick_int(min_value, max_value);
+  }
+
+  /** \brief Convenience: invokes this->entropy_source().pick_branch().
+   */
+  bool pick_branch() const
+  {
+    return this->entropy_source().pick_branch();
+  }
+
   /** \brief Returns a reference to the entropy source passed in at construction time.
    */
   batt::StateMachineEntropySource& entropy_source() noexcept
   {
     return this->entropy_source_;
   }
 
+  /** \brief Returns a reference to the entropy source passed in at construction time.
+   */
+  const batt::StateMachineEntropySource& entropy_source() const noexcept
+  {
+    return this->entropy_source_;
+  }
+
   /** \brief Returns whether the simulation is currently injecting failures randomly into system
    * components.
    *
@@ -203,6 +237,15 @@ class StorageSimulation
    */
   std::unique_ptr<LogDevice> get_log_device(const std::string& name, Optional<u64> capacity = None);
 
+  /** \brief Creates/accesses a simulated LogDevice via the LogDeviceFactory interface.
+   *
+   * \param name A unique name used to identify the LogDevice in the context of this simulation
+   * \param capacity The maximum size in bytes of the log; must be specified the first time this
+   * function is called for a given name (i.e., when the log is initially created)
+   */
+  std::unique_ptr<LogDeviceFactory> get_log_device_factory(const std::string& name,
+                                                           Optional<u64> capacity = None);
+
   /** \brief Creates/accesses a simulated PageDevice.
    *
    * The args `page_count` and `page_size` must be specified the first time this
@@ -253,6 +296,19 @@ class StorageSimulation
     return this->step_.get_value();
   }
 
+  /** \brief Blocks the calling task until the simulation step has reached or exceeded the specified
+   * value.
+   *
+   * \return the new step value, or error code if the simulation was interrupted/ended before
+   * the target step was reached.
+   */
+  StatusOr<u64> await_step(u64 minimum_step) noexcept
+  {
+    return this->step_.await_true([minimum_step](u64 observed_step) {
+      return observed_step >= minimum_step;
+    });
+  }
+
   /** \brief Returns true if all blocks for a given page are present in a simulated PageDevice,
    * false if they are all absent, error status code otherwise.
    */