From 90d99742c2a58a4d26874f8030446c501e21cd2f Mon Sep 17 00:00:00 2001 From: Tony Astolfi Date: Thu, 4 Jun 2026 09:12:58 -0400 Subject: [PATCH 1/5] wip - minor cleanups --- conan.lock | 2 +- src/llfs/buffer.hpp | 11 +---------- src/llfs/packed_pointer.hpp | 5 +++++ 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/conan.lock b/conan.lock index 5e0c520f..411612ad 100644 --- a/conan.lock +++ b/conan.lock @@ -1,7 +1,7 @@ { "version": "0.5", "requires": [ - "batteries/0.70.1", + "batteries/0.70.4.dev2", "boost/1.88.0", "bzip2/1.0.8", "cli11/2.5.0", diff --git a/src/llfs/buffer.hpp b/src/llfs/buffer.hpp index 14ee91db..7dc68683 100644 --- a/src/llfs/buffer.hpp +++ b/src/llfs/buffer.hpp @@ -20,22 +20,13 @@ namespace llfs { using batt::buffer_from_struct; +using batt::byte_distance; using batt::ConstBuffer; using batt::make_buffer; using batt::mutable_buffer_from_struct; using batt::MutableBuffer; using batt::resize_buffer; -//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - -// -/** \brief Returns the distance, in bytes, from `begin` to `end`. If `end` is less than `begin`, - * the result is negative. - */ -inline isize byte_distance(const void* begin, const void* end) -{ - return static_cast(end) - static_cast(begin); -} - //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // /** \brief Returns an empty sequence of ConstBuffer objects. diff --git a/src/llfs/packed_pointer.hpp b/src/llfs/packed_pointer.hpp index 34fc3e1d..41964a16 100644 --- a/src/llfs/packed_pointer.hpp +++ b/src/llfs/packed_pointer.hpp @@ -48,6 +48,11 @@ struct PackedPointer { return reinterpret_cast(reinterpret_cast(this) + this->offset); } + void reset_unsafe(T* ptr) + { + this->offset = byte_distance(this, ptr); + } + template void reset(T* ptr, Dst* dst) { From e47a25e74c2eec28c9c3a1ee7c7d3371dd1c856e Mon Sep 17 00:00:00 2001 From: Tony Astolfi Date: Thu, 4 Jun 2026 11:26:57 -0400 Subject: [PATCH 2/5] Fixes #202 Use-after-free on shutdown when a pending Volume::append. --- src/llfs/committable_page_cache_job.cpp | 41 +++++++++---- src/llfs/committable_page_cache_job.hpp | 8 ++- src/llfs/page_write_op.cpp | 53 ++++++++++++----- src/llfs/page_write_op.hpp | 78 ++++++++++++++++++++----- src/llfs/volume.cpp | 3 + src/llfs/volume.test.cpp | 2 + 6 files changed, 147 insertions(+), 38 deletions(-) diff --git a/src/llfs/committable_page_cache_job.cpp b/src/llfs/committable_page_cache_job.cpp index ff03856b..75ccced3 100644 --- a/src/llfs/committable_page_cache_job.cpp +++ b/src/llfs/committable_page_cache_job.cpp @@ -314,13 +314,23 @@ Status CommittablePageCacheJob::start_writing_new_pages() return OkStatus(); } +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +Status CommittablePageCacheJob::await_new_page_writes() noexcept +{ + if (this->write_new_pages_context_) { + BATT_REQUIRE_OK(this->write_new_pages_context_->await_finish()); + } + return OkStatus(); +} + //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // /*explicit*/ CommittablePageCacheJob::WriteNewPagesContext::WriteNewPagesContext( CommittablePageCacheJob* that) noexcept : that{that} , job{that->job_.get()} - , op_count{0} + , normalized_iop_count{0} , used_byte_count{0} , total_byte_count{0} , done_counter{0} @@ -329,6 +339,15 @@ Status CommittablePageCacheJob::start_writing_new_pages() { } +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +CommittablePageCacheJob::WriteNewPagesContext::~WriteNewPagesContext() noexcept +{ + // Wait for any pending asynchronous writes to finish. + // + this->await_finish().IgnoreError(); +} + //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // Status CommittablePageCacheJob::WriteNewPagesContext::start() @@ -346,18 +365,22 @@ Status CommittablePageCacheJob::WriteNewPagesContext::start() // throughput. // this->n_ops = this->job->get_new_pages().size(); - this->ops = PageWriteOp::allocate_array(this->n_ops, this->done_counter); + this->ops = std::make_unique(n_ops); LLFS_VLOG(1) << "commit(PageCacheJob): writing new pages"; { usize i = 0; for (auto& p : this->job->get_new_pages()) { + auto increment_i = batt::finally([&i] { + ++i; + }); + const PageId page_id = p.first; // There's no need to write recovered pages, since they are already durable; skip. // if (this->job->is_recovered_page(page_id)) { LLFS_VLOG(1) << "commit(PageCacheJob): skipping recovered page " << page_id; - this->ops[i].get_handler()(batt::OkStatus()); + this->ops[i].get_handler(page_id, &this->done_counter)(batt::OkStatus()); continue; } @@ -382,14 +405,12 @@ Status CommittablePageCacheJob::WriteNewPagesContext::start() const usize page_size = page_header.size; const usize used_size = page_header.used_size(); - this->ops[i].page_id = page_id; - - this->job->cache().async_write_new_page(std::move(*pinned_page), this->ops[i].get_handler()); + this->job->cache().async_write_new_page( + std::move(*pinned_page), this->ops[i].get_handler(page_id, &this->done_counter)); this->total_byte_count += page_size; this->used_byte_count += used_size; - this->op_count += page_size / 4096; - ++i; + this->normalized_iop_count += page_size / 4096; } } @@ -422,13 +443,13 @@ Status CommittablePageCacheJob::WriteNewPagesContext::await_finish() }); #endif // LLFS_TRACK_NEW_PAGE_EVENTS - all_ops_status.Update(op.result); + all_ops_status.Update(op.result()); } BATT_REQUIRE_OK(all_ops_status); this->job->cache().metrics().total_bytes_written += this->total_byte_count; this->job->cache().metrics().used_bytes_written += this->used_byte_count; - this->job->cache().metrics().total_write_ops += this->op_count; + this->job->cache().metrics().total_write_ops += this->normalized_iop_count; return OkStatus(); } diff --git a/src/llfs/committable_page_cache_job.hpp b/src/llfs/committable_page_cache_job.hpp index 0a253b87..7fa0be67 100644 --- a/src/llfs/committable_page_cache_job.hpp +++ b/src/llfs/committable_page_cache_job.hpp @@ -152,6 +152,10 @@ class CommittablePageCacheJob */ Status start_writing_new_pages(); + /** \brief Waits for asynchronous writes of new pages to complete. + */ + Status await_new_page_writes() noexcept; + //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - private: struct DeviceUpdateState { @@ -172,7 +176,7 @@ class CommittablePageCacheJob struct WriteNewPagesContext { CommittablePageCacheJob* const that; const PageCacheJob* const job; - u64 op_count; + u64 normalized_iop_count; u64 used_byte_count; u64 total_byte_count; batt::Watch done_counter; @@ -186,6 +190,8 @@ class CommittablePageCacheJob WriteNewPagesContext(const WriteNewPagesContext&) = delete; WriteNewPagesContext& operator=(const WriteNewPagesContext&) = delete; + ~WriteNewPagesContext() noexcept; + Status start(); Status await_finish(); diff --git a/src/llfs/page_write_op.cpp b/src/llfs/page_write_op.cpp index 51d43cf1..80fd16db 100644 --- a/src/llfs/page_write_op.cpp +++ b/src/llfs/page_write_op.cpp @@ -13,26 +13,51 @@ namespace llfs { //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // -/*static*/ std::unique_ptr PageWriteOp::allocate_array( - usize n, batt::Watch& done_counter) +/*explicit*/ PageWriteOp::PageWriteOp() noexcept { - auto ops = std::make_unique(n); - for (auto& op : as_slice(ops.get(), n)) { - op.done_counter = &done_counter; - } - return ops; } //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // -/*explicit*/ PageWriteOp::PageWriteOp() noexcept +PageWriteOp::~PageWriteOp() noexcept { + BATT_CHECK_EQ(this->is_pending(), false) << "This write op has an uninvoked handler!"; } //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - // -PageWriteOp::~PageWriteOp() noexcept +batt::CustomAllocHandler PageWriteOp::get_handler( + PageId id, batt::Watch* done_counter) noexcept +{ + BATT_CHECK_EQ(std::exchange(this->pending_, true), false) + << "Only one handler per PageWriteOp is allowed at a time!"; + + BATT_CHECK_NOT_NULLPTR(this->done_counter_); + + this->page_id_ = id; + this->done_counter_ = done_counter; + + return make_custom_alloc_handler(this->handler_memory_, HandlerImpl{.op_ = this}); +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +void PageWriteOp::HandlerImpl::operator()(PageDevice::WriteResult result) const noexcept +{ + this->op_->handle_write(std::move(result)); +} + +//==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - +// +void PageWriteOp::handle_write(PageDevice::WriteResult result) noexcept { + BATT_CHECK_EQ(std::exchange(this->pending_, false), true) + << "PageWriteOp handlers may only be invoked once!"; + + BATT_CHECK_NOT_NULLPTR(this->done_counter_); + + this->result_ = std::move(result); + this->done_counter_->fetch_add(1); } //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - @@ -48,12 +73,12 @@ Status parallel_drop_pages(const std::vector& deleted_pages, PageCache& } batt::Watch done_counter{0}; - std::unique_ptr ops = PageWriteOp::allocate_array(n_ops, done_counter); + auto ops = std::make_unique(n_ops); { usize i = 0; for (const PageId page_id : deleted_pages) { - ops[i].page_id = page_id; - cache.arena_for_page_id(page_id).device().drop(page_id, ops[i].get_handler()); + cache.arena_for_page_id(page_id).device().drop(page_id, + ops[i].get_handler(page_id, &done_counter)); ++i; #if LLFS_TRACK_NEW_PAGE_EVENTS @@ -79,10 +104,10 @@ Status parallel_drop_pages(const std::vector& deleted_pages, PageCache& // Status overall_status; for (PageWriteOp& op : as_slice(ops.get(), n_ops)) { - Status page_status = op.result; + Status page_status = op.result(); overall_status.Update(page_status); if (page_status.ok()) { - cache.purge(op.page_id, callers | Caller::PageCacheJob_commit_2, job_id); + cache.purge(op.page_id(), callers | Caller::PageCacheJob_commit_2, job_id); } } diff --git a/src/llfs/page_write_op.hpp b/src/llfs/page_write_op.hpp index db06a76b..263fb49b 100644 --- a/src/llfs/page_write_op.hpp +++ b/src/llfs/page_write_op.hpp @@ -24,30 +24,82 @@ namespace llfs { -// Represents/tracks a single async PageDevice::write/drop operation. +//=#=#==#==#===============+=+=+=+=++=++++++++++++++-++-+--+-+----+--------------- // +/** \brief Represents/tracks a single async PageDevice::write/drop operation. + */ + struct PageWriteOp { - PageWriteOp(const PageWriteOp&) = delete; - PageWriteOp& operator=(const PageWriteOp&) = delete; + struct HandlerImpl { + PageWriteOp* op_; - batt::HandlerMemory<256> handler_memory; - PageId page_id; - batt::Watch* done_counter = nullptr; - PageDevice::WriteResult result; + void operator()(PageDevice::WriteResult result) const noexcept; + }; - static std::unique_ptr allocate_array(usize n, batt::Watch& done_counter); + //+++++++++++-+-+--+----- --- -- - - - - explicit PageWriteOp() noexcept; + PageWriteOp(const PageWriteOp&) = delete; + PageWriteOp& operator=(const PageWriteOp&) = delete; + ~PageWriteOp() noexcept; - auto get_handler() + //+++++++++++-+-+--+----- --- -- - - - - + + /** \brief Returns a write handler for this op; there must only be one active handler per op at a + * time! + */ + batt::CustomAllocHandler get_handler( + PageId id, batt::Watch* done_counter) noexcept; + + /** \brief Returns the most recent result that was passed to the handler; only valid after + * get_handler has been called and the returned handler has been invoked. + */ + const PageDevice::WriteResult& result() const noexcept + { + return this->result_; + } + + /** \brief Returns the most recent PageId passed to get_handler. + */ + PageId page_id() const noexcept { - return make_custom_alloc_handler(this->handler_memory, [this](PageDevice::WriteResult result) { - this->result = std::move(result); - this->done_counter->fetch_add(1); - }); + return this->page_id_; } + + /** \brief Returns true iff this op has a handler which hasn't been invoked. + */ + bool is_pending() const noexcept + { + return this->pending_; + } + + //+++++++++++-+-+--+----- --- -- - - - - + private: + void handle_write(PageDevice::WriteResult result) noexcept; + + //+++++++++++-+-+--+----- --- -- - - - - + + /** \brief Pre-allocated memory for the write handler. + */ + batt::HandlerMemory<256> handler_memory_; + + /** \brief The Watch to increment when this operation's handler is invoked. + */ + batt::Watch* done_counter_ = nullptr; + + /** \brief The most recent page associated with this op. + */ + PageId page_id_; + + /** \brief The result of the last operation. + */ + PageDevice::WriteResult result_; + + /** \brief Set to true when get_handler is called; set to false when that handler is invoked. + */ + bool pending_ = false; }; // Drops a range of PageIds in parallel. diff --git a/src/llfs/volume.cpp b/src/llfs/volume.cpp index 6fc91dae..009f0219 100644 --- a/src/llfs/volume.cpp +++ b/src/llfs/volume.cpp @@ -520,6 +520,9 @@ StatusOr Volume::append(AppendableJob&& appendable, batt::Grant& gran if (write_new_pages_asap()) { BATT_REQUIRE_OK(appendable.job.start_writing_new_pages()); } + auto wait_for_async_ops = batt::finally([&] { + appendable.job.await_new_page_writes().IgnoreError(); + }); StatusOr result; diff --git a/src/llfs/volume.test.cpp b/src/llfs/volume.test.cpp index b6dfeabe..0bbbea96 100644 --- a/src/llfs/volume.test.cpp +++ b/src/llfs/volume.test.cpp @@ -1692,6 +1692,8 @@ void VolumeSimTest::run_recovery_sim(u32 seed) llfs::PageGraphNodeView::page_reader()); const auto main_task_fn = [&] { + BATT_DEBUG_INFO(BATT_INSPECT(seed)); + sim.set_inject_failures_mode(false); LLFS_VLOG(1) << "Entered main task;" << BATT_INSPECT(seed) << BATT_INSPECT(sim.is_running()); From a0d663dc69829455004588acecdb8b8e06d7d0dd Mon Sep 17 00:00:00 2001 From: Tony Astolfi Date: Thu, 4 Jun 2026 11:32:10 -0400 Subject: [PATCH 3/5] Fix conan.lock. --- conan.lock | 2 +- src/llfs/page_write_op.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/conan.lock b/conan.lock index 411612ad..18597a46 100644 --- a/conan.lock +++ b/conan.lock @@ -1,7 +1,7 @@ { "version": "0.5", "requires": [ - "batteries/0.70.4.dev2", + "batteries/0.70.3", "boost/1.88.0", "bzip2/1.0.8", "cli11/2.5.0", diff --git a/src/llfs/page_write_op.cpp b/src/llfs/page_write_op.cpp index 80fd16db..5a2ebb8f 100644 --- a/src/llfs/page_write_op.cpp +++ b/src/llfs/page_write_op.cpp @@ -32,7 +32,7 @@ batt::CustomAllocHandler PageWriteOp::get_handler( BATT_CHECK_EQ(std::exchange(this->pending_, true), false) << "Only one handler per PageWriteOp is allowed at a time!"; - BATT_CHECK_NOT_NULLPTR(this->done_counter_); + BATT_CHECK_NOT_NULLPTR(done_counter); this->page_id_ = id; this->done_counter_ = done_counter; From c214f9f024c585468b65c3acc0537e1e2c255241 Mon Sep 17 00:00:00 2001 From: Tony Astolfi Date: Tue, 9 Jun 2026 09:14:18 -0400 Subject: [PATCH 4/5] magic number 4096 -> named constant --- src/llfs/committable_page_cache_job.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/llfs/committable_page_cache_job.cpp b/src/llfs/committable_page_cache_job.cpp index 75ccced3..ff87952c 100644 --- a/src/llfs/committable_page_cache_job.cpp +++ b/src/llfs/committable_page_cache_job.cpp @@ -410,7 +410,7 @@ Status CommittablePageCacheJob::WriteNewPagesContext::start() this->total_byte_count += page_size; this->used_byte_count += used_size; - this->normalized_iop_count += page_size / 4096; + this->normalized_iop_count += page_size / kDirectIOBlockSize; } } From b6c70cd0816d253d6de5372a294a7c03fb68974d Mon Sep 17 00:00:00 2001 From: Tony Astolfi Date: Tue, 9 Jun 2026 09:17:46 -0400 Subject: [PATCH 5/5] Add doc comment for CR. --- src/llfs/page_write_op.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/llfs/page_write_op.hpp b/src/llfs/page_write_op.hpp index 263fb49b..03f11d72 100644 --- a/src/llfs/page_write_op.hpp +++ b/src/llfs/page_write_op.hpp @@ -48,7 +48,8 @@ struct PageWriteOp { //+++++++++++-+-+--+----- --- -- - - - - /** \brief Returns a write handler for this op; there must only be one active handler per op at a - * time! + * time! If `this->get_handler()` is called twice before invoking the first returned handler (or + * a copy of it), this function will panic. */ batt::CustomAllocHandler get_handler( PageId id, batt::Watch* done_counter) noexcept;