IoRingLogDevice2 trim bug and remove atomic slot range tokens. (#160)

* Add more metrics to IoRingLogDevice2 and verify them in the sim test.

* Remove atomic slot range tokens.

* Fix for issue/159.

conanfile.py

@@ -69,7 +69,7 @@ def configure(self):
 
 
     def requirements(self):
-        self.requires("batteries/0.53.0", **VISIBLE)
+        self.requires("batteries/0.55.1", **VISIBLE)
         self.requires("boost/1.83.0", **VISIBLE)
         self.requires("cli11/2.3.2", **VISIBLE)
         self.requires("glog/0.6.0", **VISIBLE)

src/llfs/basic_ring_buffer_log_device.hpp

@@ -245,7 +245,11 @@ class BasicRingBufferLogDevice<Impl>::WriterImpl : public LogDevice::Writer
     const usize space_required = byte_count + head_room;
 
     if (this->space() < space_required) {
-      return ::llfs::make_status(StatusCode::kPrepareFailedTrimRequired);
+      BATT_REQUIRE_OK(::llfs::make_status(StatusCode::kPrepareFailedTrimRequired))
+          << BATT_INSPECT(space_required) << BATT_INSPECT(this->space())
+          << BATT_INSPECT(this->device_->driver_.get_trim_pos())
+          << BATT_INSPECT(this->device_->driver_.get_commit_pos())
+          << boost::stacktrace::stacktrace{};
     }
 
     const slot_offset_type commit_pos = this->device_->driver_.get_commit_pos();

src/llfs/ioring_log_device2.hpp

@@ -83,6 +83,7 @@ class IoRingLogDriver2
   Status set_trim_pos(slot_offset_type trim_pos)
   {
     this->observed_watch_[kTargetTrimPos].set_value(trim_pos);
+    BATT_REQUIRE_OK(this->await_trim_pos(trim_pos));
     return OkStatus();
   }
 

src/llfs/ioring_log_device2.test.cpp

@@ -36,7 +36,7 @@ using llfs::StatusOr;
 //     the confirmed trim and flush never go backwards, and that all confirmed flushed data can be
 //     read.
 //
-class IoringLogDevice2SimTest : public ::testing::Test
+class IoRingLogDevice2SimTest : public ::testing::Test
 {
  public:
   static constexpr usize kNumSeeds = 250 * 1000;
@@ -326,6 +326,8 @@ class IoringLogDevice2SimTest : public ::testing::Test
         if (!trim_status.ok()) {
           break;
         }
+        BATT_CHECK_EQ(this->log_device->slot_range(llfs::LogReadMode::kDurable).lower_bound,
+                      trimmed_slot.upper_bound);
 
         Status await_status = this->log_writer->await(llfs::BytesAvailable{
             .size = trimmed_slot.size() + observed_space,
@@ -503,11 +505,11 @@ class IoringLogDevice2SimTest : public ::testing::Test
 
   };  // struct Scenario
 
-};  // class IoringLogDevice2SimTest
+};  // class IoRingLogDevice2SimTest
 
 //=#=#==#==#===============+=+=+=+=++=++++++++++++++-++-+--+-+----+---------------
 
-TEST_F(IoringLogDevice2SimTest, Simulation)
+TEST_F(IoRingLogDevice2SimTest, Simulation)
 {
   const usize kNumThreads = std::thread::hardware_concurrency();
   const usize kUpdateInterval = kNumSeeds / 20;
@@ -558,7 +560,7 @@ TEST_F(IoringLogDevice2SimTest, Simulation)
 
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
-TEST(IoringLogDevice2Test, Benchmark)
+TEST(IoRingLogDevice2Test, Benchmark)
 {
   //+++++++++++-+-+--+----- --- -- -  -  -   -
   // Set configuration and options.

src/llfs/slot_writer.cpp

@@ -57,23 +57,6 @@ void SlotWriter::halt()
   this->pool_.close();
 }
 
-//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
-//
-/*static*/ Slice<const u8> SlotWriter::WriterLock::begin_atomic_range_token() noexcept
-{
-  const static std::array<u8, Self::kBeginAtomicRangeTokenSize> token_ = {0b10000000, 0b10000000,
-                                                                          0b00000000};
-  return as_const_slice(token_);
-}
-
-//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
-//
-/*static*/ Slice<const u8> SlotWriter::WriterLock::end_atomic_range_token() noexcept
-{
-  const static std::array<u8, Self::kEndAtomicRangeTokenSize> token_ = {0b10000000, 0b00000000};
-  return as_const_slice(token_);
-}
-
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
 /*explicit*/ SlotWriter::WriterLock::WriterLock(SlotWriter& slot_writer) noexcept
@@ -83,45 +66,6 @@ void SlotWriter::halt()
   BATT_CHECK_NOT_NULLPTR(*this->writer_lock_);
 }
 
-//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
-//
-Status SlotWriter::WriterLock::append_token_impl(const Slice<const u8>& token,
-                                                 const batt::Grant& caller_grant) noexcept
-{
-  BATT_CHECK_EQ(this->prepare_size_, 0u);
-
-  const usize prepare_size = this->deferred_commit_size_ + token.size();
-
-  if (caller_grant.size() < prepare_size) {
-    return ::llfs::make_status(StatusCode::kSlotGrantTooSmall);
-  }
-
-  BATT_ASSIGN_OK_RESULT(MutableBuffer buffer,
-                        (*this->writer_lock_)->prepare(prepare_size, /*head_room=*/0));
-
-  buffer += this->deferred_commit_size_;
-
-  std::memcpy(buffer.data(), token.begin(), token.size());
-
-  this->deferred_commit_size_ += token.size();
-
-  return batt::OkStatus();
-}
-
-//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
-//
-Status SlotWriter::WriterLock::begin_atomic_range(const batt::Grant& caller_grant) noexcept
-{
-  return this->append_token_impl(Self::begin_atomic_range_token(), caller_grant);
-}
-
-//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
-//
-Status SlotWriter::WriterLock::end_atomic_range(const batt::Grant& caller_grant) noexcept
-{
-  return this->append_token_impl(Self::end_atomic_range_token(), caller_grant);
-}
-
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
 StatusOr<MutableBuffer> SlotWriter::WriterLock::prepare(usize slot_payload_size,

src/llfs/slot_writer.hpp

@@ -136,12 +136,6 @@ class SlotWriter::WriterLock
  public:
   using Self = WriterLock;
 
-  static constexpr usize kBeginAtomicRangeTokenSize = 3;
-  static constexpr usize kEndAtomicRangeTokenSize = 2;
-
-  static Slice<const u8> begin_atomic_range_token() noexcept;
-  static Slice<const u8> end_atomic_range_token() noexcept;
-
   //+++++++++++-+-+--+----- --- -- -  -  -   -
 
   /** \brief Acquires an exclusive lock on writing to the log managed by `slot_writer`, preparing
@@ -159,19 +153,6 @@ class SlotWriter::WriterLock
 
   //+++++++++++-+-+--+----- --- -- -  -  -   -
 
-  /** \brief Appends special token to the log to indicate the start of a sequence of slots that
-   * must commit/recover atomically.
-   *
-   * The token commit is deferred, requiring later calls to end_atomic_range and commit.
-   */
-  Status begin_atomic_range(const batt::Grant& caller_grant) noexcept;
-
-  /** \brief Appends special sequence to the log indicating the end of the current atomic range.
-   *
-   * The token commit is deferred, requiring a later call to commit.
-   */
-  Status end_atomic_range(const batt::Grant& caller_grant) noexcept;
-
   /** \brief Verifies that the passed grant can accomodate a new slot with the passed payload size
    * (in addition to any currently deferred slots), then allocates space in the log and writes a
    * header for the new slot, returning a mutable buffer for _just_ the payload portion.
@@ -206,14 +187,6 @@ class SlotWriter::WriterLock
 
   //+++++++++++-+-+--+----- --- -- -  -  -   -
  private:
-  /** \brief Appends the specified byte range to the prepared data.  `token` is just a raw byte
-   * sequence, not a full slot.  This function is used to implement the `begin_atomic_range` and
-   * `end_atomic_range` tokens.
-   */
-  Status append_token_impl(const Slice<const u8>& token, const batt::Grant& caller_grant) noexcept;
-
-  //+++++++++++-+-+--+----- --- -- -  -  -   -
-
   /** \brief The SlotWriter object passed in at construction time.
    */
   SlotWriter& slot_writer_;
@@ -289,16 +262,6 @@ class TypedSlotWriter<PackedVariant<Ts...>> : public SlotWriter
 
     //----- --- -- -  -  -   -
 
-    Status begin_atomic_range(const batt::Grant& caller_grant) noexcept
-    {
-      return this->writer_lock_.begin_atomic_range(caller_grant);
-    }
-
-    Status end_atomic_range(const batt::Grant& caller_grant) noexcept
-    {
-      return this->writer_lock_.end_atomic_range(caller_grant);
-    }
-
     /** \brief Packs a single slot into the log device buffer.  Does not commit the slot; all slots
      * are committed atomically when this->finalize() is called.
      */

src/llfs/slot_writer.test.cpp

@@ -19,27 +19,4 @@ namespace {
 
 using namespace llfs::int_types;
 
-TEST(SlotWriterTest, BeginEndAtomicRangeTokens)
-{
-  llfs::Slice<const u8> begin_token = llfs::SlotWriter::WriterLock::begin_atomic_range_token();
-  llfs::Slice<const u8> end_token = llfs::SlotWriter::WriterLock::end_atomic_range_token();
-
-  EXPECT_EQ(begin_token.size(), llfs::SlotWriter::WriterLock::kBeginAtomicRangeTokenSize);
-  EXPECT_EQ(end_token.size(), llfs::SlotWriter::WriterLock::kEndAtomicRangeTokenSize);
-  EXPECT_NE(begin_token.size(), end_token.size());
-  EXPECT_GT(begin_token.size(), 1u);
-  EXPECT_GT(end_token.size(), 1u);
-
-  const auto verify_parses_as_zero = [](const llfs::Slice<const u8>& token) {
-    auto [parsed_n, end_of_parse] = llfs::unpack_varint_from(token.begin(), token.end());
-
-    ASSERT_TRUE(parsed_n);
-    EXPECT_EQ(*parsed_n, 0u);
-    EXPECT_EQ(end_of_parse, token.end());
-  };
-
-  verify_parses_as_zero(begin_token);
-  verify_parses_as_zero(end_token);
-}
-
 }  // namespace

src/llfs/volume.cpp

@@ -39,6 +39,13 @@ const VolumeOptions& Volume::options() const
   return this->options_;
 }
 
+//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
+//
+const std::string& Volume::name() const noexcept
+{
+  return this->options().name;
+}
+
 //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- -  -  -   -
 //
 const boost::uuids::uuid& Volume::get_volume_uuid() const
@@ -374,7 +381,7 @@ void Volume::start()
           Status result = this->trimmer_->run();
           LLFS_VLOG(1) << "Volume::trimmer_task_ exited with status=" << result;
         },
-        "Volume::trimmer_task_");
+        /*task_name=*/batt::to_string(this->name(), "_Volume.trimmer_task"));
   }
 }
 

src/llfs/volume.hpp

@@ -88,6 +88,10 @@ class Volume
   //
   const VolumeOptions& options() const;
 
+  /** \brief The name for this Volume, as specified by the VolumeOptions.
+   */
+  const std::string& name() const noexcept;
+
   // Returns the UUID for this volume.
   //
   const boost::uuids::uuid& get_volume_uuid() const;

src/llfs/volume_multi_append.hpp

@@ -35,8 +35,7 @@ class VolumeMultiAppend
    */
   static constexpr u64 calculate_grant_size(u64 slots_total_size) noexcept
   {
-    return slots_total_size + SlotWriter::WriterLock::kBeginAtomicRangeTokenSize +
-           SlotWriter::WriterLock::kEndAtomicRangeTokenSize;
+    return slots_total_size;
   }
 
   //+++++++++++-+-+--+----- --- -- -  -  -   -
@@ -85,11 +84,6 @@ class VolumeMultiAppend
   {
     BATT_CHECK(!this->closed_);
 
-    if (this->first_) {
-      this->first_ = false;
-      BATT_REQUIRE_OK(this->op_.begin_atomic_range(grant));
-    }
-
     llfs::PackObjectAsRawData<const T&> packed_obj_as_raw{payload};
 
     return this->op_.append(grant, packed_obj_as_raw);
@@ -115,10 +109,6 @@ class VolumeMultiAppend
   {
     BATT_CHECK(!this->closed_);
 
-    if (!this->first_) {
-      BATT_REQUIRE_OK(this->op_.end_atomic_range(grant));
-    }
-
     StatusOr<SlotRange> result = this->op_.finalize(grant);
     BATT_REQUIRE_OK(result);
 
@@ -142,11 +132,6 @@ class VolumeMultiAppend
   //
   TypedSlotWriter<VolumeEventVariant>::MultiAppend op_;
 
-  // true iff no call append has yet been made; used to determine when we should write a begin
-  // atomic range token to the log.
-  //
-  bool first_ = true;
-
   // true iff commit or cancel has been called.
   //
   bool closed_ = false;

src/llfs/volume_trimmer.cpp

@@ -217,12 +217,12 @@ Status VolumeTrimmer::run()
     //+++++++++++-+-+--+----- --- -- -  -  -   -
     // Wait for the trim point to advance.
     //
-    BATT_DEBUG_INFO("waiting for trim target to advance; "
-                    << BATT_INSPECT(this->trim_control_.get_lower_bound())
-                    << BATT_INSPECT(least_upper_bound) << BATT_INSPECT(bytes_trimmed)
-                    << BATT_INSPECT(trim_lower_bound) << std::endl
-                    << BATT_INSPECT(this->trim_control_.debug_info()) << BATT_INSPECT(loop_counter)
-                    << BATT_INSPECT(this->trim_control_.is_closed()));
+    BATT_DEBUG_INFO(
+        "waiting for trim target to advance; "
+        << BATT_INSPECT(this->trim_control_.get_lower_bound()) << BATT_INSPECT(least_upper_bound)
+        << BATT_INSPECT(bytes_trimmed) << BATT_INSPECT(trim_lower_bound) << std::endl
+        << BATT_INSPECT(this->trim_control_.debug_info()) << BATT_INSPECT(loop_counter)
+        << BATT_INSPECT(this->trim_control_.is_closed()) << BATT_INSPECT(this->trim_delay_));
 
     StatusOr<slot_offset_type> trim_upper_bound = this->await_trim_target(least_upper_bound);