From a244d85109153b818ee61081a2057d7ab43f86cf Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Mon, 12 Oct 2026 03:27:56 +0000 Subject: [PATCH 1/6] Avoid loader coroutine frames for owner-local disk GET --- CMakeLists.txt | 1 + docs/architecture/04-storage-and-recovery.md | 7 +- include/lavik/storage/engine.h | 1 + src/storage/engine/impl.h | 55 +++++ src/storage/engine/read.cpp | 152 +++++++++++-- tests/direct_value_read_test.cpp | 220 +++++++++++++++++++ 6 files changed, 418 insertions(+), 18 deletions(-) create mode 100644 tests/direct_value_read_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 784b8cad2..c0f63dcf5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1766,6 +1766,7 @@ if (BUILD_TESTING) tests/rdb_test.cpp tests/rdb_collection_test.cpp tests/record_location_test.cpp + tests/direct_value_read_test.cpp tests/request_gate_test.cpp tests/replication_command_test.cpp tests/replication_group_test.cpp diff --git a/docs/architecture/04-storage-and-recovery.md b/docs/architecture/04-storage-and-recovery.md index 6ba2fdcb6..a6b261158 100644 --- a/docs/architecture/04-storage-and-recovery.md +++ b/docs/architecture/04-storage-and-recovery.md @@ -649,7 +649,12 @@ rereads. Transaction-owned reads always use that locked path directly. Tombstones and expired values are invisible; an expired observation can enqueue a bounded active-expiration candidate. A staged location is copied or framed from its write buffer. A disk location is read on its physical block owner into -an aligned lease. +an aligned lease. For a complete inline String already on its key owner's disk, +GET can submit the read from an awaitable embedded in its existing request task, +without child loader coroutines. The awaitable owns the block pin and buffer +through completion and validates the record and population before returning it. +Buffer contention, staged data, other representations, and stale physical +locations retain the complete loader and its backpressure/relocation handling. Worker-local MGET uses `BatchGetLocked` after the command has acquired its key locks. It classifies index entries in one coroutine and issues ordinary-size diff --git a/include/lavik/storage/engine.h b/include/lavik/storage/engine.h index 2feb57526..c366e8f66 100644 --- a/include/lavik/storage/engine.h +++ b/include/lavik/storage/engine.h @@ -1944,6 +1944,7 @@ class StorageEngine { private: friend class ExpirationAuthorityTestPeer; friend class TombRaiderTestPeer; + friend class DirectValueReadTestPeer; class Impl; std::unique_ptr impl_; }; diff --git a/src/storage/engine/impl.h b/src/storage/engine/impl.h index 17dc6ed40..1690f9906 100644 --- a/src/storage/engine/impl.h +++ b/src/storage/engine/impl.h @@ -3257,6 +3257,7 @@ class StorageEngine::Impl { private: friend class ExpirationAuthorityTestPeer; friend class TombRaiderTestPeer; + friend class DirectValueReadTestPeer; struct DurableSystemState { std::uint64_t generation_ = 0; @@ -3342,6 +3343,54 @@ class StorageEngine::Impl { } }; + // Opportunistic, owner-local disk read embedded in GetWithLockState's frame. + // nullopt selects LoadValue's complete representation/relocation state + // machine. + class DirectValueRead { + public: + using Result = std::optional>; + DirectValueRead(Impl& engine, WorkerStore& store, + WorkerStore::PartitionStore& partition, std::uint8_t db_id, + std::string_view key, RecordLocation location, + ReadLatencyTrace* trace); + DirectValueRead(const DirectValueRead&) = delete; + DirectValueRead& operator=(const DirectValueRead&) = delete; + DirectValueRead(DirectValueRead&&) = delete; + DirectValueRead& operator=(DirectValueRead&&) = delete; + + bool await_ready() const noexcept { return !io_.has_value(); } + bool await_suspend(std::coroutine_handle<> awaiting) { + if (trace_) trace_->io_submit_ns_ = ReadTraceNowNanos(); + return io_->await_suspend(awaiting); + } + Result await_resume(); + + private: + friend class DirectValueReadTestPeer; + struct Pin { + WorkerStore* store_ = nullptr; + BlockState* state_ = nullptr; + ~Pin(); + }; + + Impl& engine_; + WorkerStore::PartitionStore& partition_; + std::string_view key_; + RecordLocation location_; + ReadLatencyTrace* trace_; + std::uint64_t replication_epoch_; + std::uint64_t db_epoch_; + std::size_t record_headroom_ = 0; + std::size_t read_bytes_ = 0; + std::uint8_t db_id_; + Result ready_; + // The completion and lease die before the pin. Runtime shutdown drains + // I/O before destroying suspended frames; no continuation is heap-owned. + Pin pin_; + ReadBufferLease lease_; + std::optional io_; + }; + // Owns the page lease while a grouped reader inspects its checked envelope. // Views into loaded_.value() must not outlive this object. struct LoadedHashGroupPayload { @@ -3361,6 +3410,12 @@ class StorageEngine::Impl { absl::StatusOr EncodeDiskValue(LoadedValue loaded); + // Shared synchronous validation for ordinary and direct disk reads. + absl::StatusOr DecodeLoadedValue( + ReadBufferLease lease, std::uint8_t db_id, std::string_view key, + RecordLocation location, std::uint64_t replication_epoch, + std::size_t record_headroom, std::uint64_t expected_db_epoch); + void QueueExpiredCandidate(WorkerStore& store, std::uint16_t partition_id, std::uint8_t db_id, const RecordIndex::Entry& entry, diff --git a/src/storage/engine/read.cpp b/src/storage/engine/read.cpp index b987a39b1..4c9d1c253 100644 --- a/src/storage/engine/read.cpp +++ b/src/storage/engine/read.cpp @@ -306,9 +306,18 @@ Task> StorageEngine::Impl::GetWithLockState( trace->lookup_done_ns_ = ReadTraceNowNanos(); } - auto loaded = co_await LoadValue(store, partition, db_id, key, digest, - MaterializeIndexLocation(*found), - ExtentsFor(store, found), trace); + // Snapshot all entry-owned state before the direct I/O can suspend. + const auto location = MaterializeIndexLocation(*found); + auto extents = ExtentsFor(store, found); + auto direct = co_await DirectValueRead(*this, store, partition, db_id, key, + location, trace); + absl::StatusOr loaded; + if (direct.has_value()) { + loaded = std::move(*direct); + } else { + loaded = co_await LoadValue(store, partition, db_id, key, digest, + location, std::move(extents), trace); + } if (optimistic_read && !loaded.ok() && (loaded.status().code() == absl::StatusCode::kNotFound || loaded.status().code() == absl::StatusCode::kAborted)) { @@ -328,6 +337,103 @@ Task> StorageEngine::Impl::GetWithLockState( } } +StorageEngine::Impl::DirectValueRead::DirectValueRead( + Impl& engine, WorkerStore& store, WorkerStore::PartitionStore& partition, + std::uint8_t db_id, std::string_view key, RecordLocation location, + ReadLatencyTrace* trace) + : engine_(engine), + partition_(partition), + key_(key), + location_(location), + trace_(trace), + replication_epoch_(partition.replication_epoch_), + db_epoch_(engine.DbEpoch(db_id)), + db_id_(db_id) { + // Keep candidate populations, staged data and other representations on the + // complete loader. The index lookup and this admission never suspend, so a + // validated local block cannot be recycled before its pin is acquired. + if (location.grouped() || location.external() || location.key_indirect() || + location.in_memory() || location.block_owner() != store.worker_->id() || + partition.replica_sync_ != nullptr) + return; + LAVIK_FAULT_INJECT(if (LAVIK_FAULT_MATCHES("LAVIK_FAIL_VALUE_READ_KEY", + key)) { + ready_.emplace(absl::InternalError("injected value payload read failure")); + return; + }); + const auto [file_id, block_offset] = engine.FileOffset(location.block_id()); + const auto absolute_offset = block_offset + location.record_offset(); + const auto mask = static_cast(engine.direct_io_alignment_ - 1); + const auto aligned_offset = absolute_offset & ~mask; + record_headroom_ = absolute_offset - aligned_offset; + read_bytes_ = (record_headroom_ + location.total_disk_bytes() + + engine.direct_io_alignment_ - 1) & + ~mask; + if (trace_) trace_->buffer_acquire_start_ns_ = ReadTraceNowNanos(); + auto acquire = store.buffers_.AcquireReadBuffer(read_bytes_); + // Preserve bounded buffer backpressure through LoadValue's existing waiter + // when exhausted. The common path needs only the caller's I/O continuation. + if (!acquire.await_ready()) return; + auto acquired = acquire.await_resume(); + if (!acquired.ok()) { + ready_.emplace(acquired.status()); + return; + } + lease_ = std::move(*acquired); + if (trace_) { + trace_->buffer_acquired_ns_ = ReadTraceNowNanos(); + trace_->heap_read_buffer_ = lease_.buffer_id() == 0; + } + auto* state = engine.FindBlockState(store, location.block_id()); + if (state == nullptr || !state->allocated_ || state->freeing_ || + state->allocation_epoch_ != location.allocation_epoch()) + return; + ++state->pins_; + pin_.store_ = &store; + pin_.state_ = state; + if (trace_) trace_->disk_read_ = true; + auto buffer = lease_.io_buffer(); + assert(read_bytes_ <= buffer.size_); + buffer.size_ = read_bytes_; + io_.emplace(*store.worker_, store.files_[file_id], buffer, aligned_offset, + lease_.registered() + ? bycorf::SizeIoAwaitable::Operation::kReadFixed + : bycorf::SizeIoAwaitable::Operation::kRead); +} + +StorageEngine::Impl::DirectValueRead::Pin::~Pin() { + if (state_ == nullptr) return; + --state_->pins_; + if (state_->pins_ == 0 && state_->release_pending_) + ReleaseStagingBuffer(*store_, *state_); +} + +StorageEngine::Impl::DirectValueRead::Result +StorageEngine::Impl::DirectValueRead::await_resume() { + if (!io_) return std::move(ready_); + auto read = io_->await_resume(); + if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); + // Replica resets do not acquire individual key locks. Match LoadValue's + // population check before publishing either bytes or an I/O error. + if (partition_.replication_epoch_ != replication_epoch_ || + engine_.DbEpoch(db_id_) != db_epoch_) + return absl::NotFoundError("key not found"); + if (!read.ok()) { + if (absl::IsAborted(read.status())) return std::nullopt; + return read.status(); + } + if (*read != read_bytes_) + return absl::InternalError("short compact record read"); + auto loaded = engine_.DecodeLoadedValue(std::move(lease_), db_id_, key_, + location_, replication_epoch_, + record_headroom_, db_epoch_); + // A relocation or mismatched physical record must go through LoadValue's + // same-mutation retry and corruption checks, also for transaction-owned GET. + if (!loaded.ok() && absl::IsAborted(loaded.status())) return std::nullopt; + if (trace_ && loaded.ok()) trace_->decode_done_ns_ = ReadTraceNowNanos(); + return loaded; +} + Task> StorageEngine::Impl::BatchGetLocked( std::uint8_t db_id, std::span requests) { assert(db_id < kLogicalDatabaseCount); @@ -1467,6 +1573,22 @@ StorageEngine::Impl::LoadValueLocal( "short compact record read"); } + auto loaded = DecodeLoadedValue(std::move(lease), db_id, key, location, + replication_epoch, record_headroom, + expected_db_epoch.value_or(DbEpoch(db_id))); + if (trace != nullptr && loaded.ok()) + trace->decode_done_ns_ = ReadTraceNowNanos(); + co_return loaded; +} + +absl::StatusOr +StorageEngine::Impl::DecodeLoadedValue(ReadBufferLease lease, + std::uint8_t db_id, std::string_view key, + RecordLocation location, + std::uint64_t replication_epoch, + std::size_t record_headroom, + std::uint64_t expected_db_epoch) { + const FixedBuffer io = lease.io_buffer(); RecordHeader record{}; std::string_view disk_key; const std::byte* record_data = io.data_ + record_headroom; @@ -1475,7 +1597,7 @@ StorageEngine::Impl::LoadValueLocal( if (!DecodeRecordHeader(record_bytes, &record, &disk_key) || record.db_id_ != db_id || (!record.key_indirect_ && disk_key != key) || record.kind_ != RecordKind::kValue || - record.db_epoch_ != expected_db_epoch.value_or(DbEpoch(db_id)) || + record.db_epoch_ != expected_db_epoch || record.mutation_sequence_ != location.mutation_sequence_ || record.replication_epoch_ != replication_epoch || record.allocation_epoch_ != location.allocation_epoch() || @@ -1485,37 +1607,33 @@ StorageEngine::Impl::LoadValueLocal( record.key_indirect_ != location.key_indirect() || record.logical_size_ != location.logical_size_ || record.total_disk_bytes_ != location.total_disk_bytes()) { - co_return absl::Status(absl::StatusCode::kAborted, - "record does not match in-memory location"); + return absl::Status(absl::StatusCode::kAborted, + "record does not match in-memory location"); } const std::byte* payload_data = record_data + record.header_bytes_; if (record.key_indirect_ && record.key_bytes_ != key.size()) { - co_return absl::Status(absl::StatusCode::kAborted, - "record key does not match location"); + return absl::Status(absl::StatusCode::kAborted, + "record key does not match location"); } const std::size_t value_bytes = record.payload_bytes_; if (record.value_type_ == ValueType::kString && !record.grouped_ && !record.auxiliary_group_ && value_bytes != record.logical_size_) { - co_return absl::Status(absl::StatusCode::kInternal, - "inline value length does not match metadata"); + return absl::Status(absl::StatusCode::kInternal, + "inline value length does not match metadata"); } if (Crc32c(std::span(payload_data, record.payload_bytes_)) != record.payload_checksum_) { - co_return absl::Status(absl::StatusCode::kInternal, - "record value checksum mismatch"); + return absl::Status(absl::StatusCode::kInternal, + "record value checksum mismatch"); } const std::byte* framed_value = payload_data; if (value_bytes > DirectGetValueLimit()) { std::memmove(io.data_, payload_data, value_bytes); framed_value = io.data_; } - if (trace != nullptr) { - trace->decode_done_ns_ = ReadTraceNowNanos(); - } const std::size_t value_offset = static_cast(framed_value - lease.bytes().data()); - co_return LoadedValue{std::move(lease), value_offset, value_bytes, - record.txid_}; + return LoadedValue{std::move(lease), value_offset, value_bytes, record.txid_}; } } // namespace lavik::storage diff --git a/tests/direct_value_read_test.cpp b/tests/direct_value_read_test.cpp new file mode 100644 index 000000000..dc647abdc --- /dev/null +++ b/tests/direct_value_read_test.cpp @@ -0,0 +1,220 @@ +/* + * Copyright (C) 2026 EloqData Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#include "../src/storage/engine/impl.h" +#include "gtest/gtest.h" + +namespace lavik::storage { + +// Deliver deterministic CQEs to test invalidation and resource ownership at +// exact suspension boundaries. End-to-end storage tests cover real submission. +class DirectValueReadTestPeer : public testing::Test { + protected: + using Awaitable = StorageEngine::Impl::DirectValueRead; + + void SetUp() override { + saved_context_ = std::move(bycorf::MutableThisWorker()); + wake_fd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK); + ASSERT_GE(wake_fd_, 0); + cross_core_.mailbox(0).wake_fd_ = wake_fd_; + worker_.BindCrossCore(0, &cross_core_); + bycorf::SetThisWorker(0, &cross_core_, &worker_); + auto status = worker_.Init({.recv_buffer_count_ = 0}); + ASSERT_TRUE(status.ok()) << status; + auto& engine = *storage_.impl_; + engine.worker_count_ = 1; + engine.stores_.push_back( + std::make_unique()); + auto& store = *engine.stores_[0]; + store.worker_ = &worker_; + store.partitions_.resize(1); + store.partitions_[0].replication_epoch_ = 1; + store.files_.push_back(bycorf::FixedFile{}); + engine.db_epochs_[0].store(1); + engine.devices_.push_back(StorageDevice{.capacity_blocks_ = 2}); + engine.block_state_lookup_.push_back({&block_, 1, 2}); + block_.Reset(0, 7); + block_.allocated_ = true; + + RegisteredBufferPoolOptions options; + options.storage_write_buffer_count_ = 1; + options.registered_bytes_ = + options.write_buffer_bytes_ + options.read_slot_bytes_; + ASSERT_TRUE(Pool().Init(worker_, options).ok()); + ASSERT_EQ(Pool().available_read_buffers(), 1); + + header_.header_bytes_ = RecordHeaderBytes(key_.size()); + header_.key_bytes_ = key_.size(); + header_.value_type_ = ValueType::kString; + header_.logical_size_ = value_.size(); + header_.payload_bytes_ = value_.size(); + header_.total_disk_bytes_ = + AlignRecord(header_.header_bytes_ + value_.size()); + header_.allocation_epoch_ = 7; + header_.mutation_sequence_ = 9; + header_.payload_checksum_ = Crc32c(std::as_bytes(std::span(value_))); + const auto metadata = RecordLocation::PackedMetadata::Encode( + kBlockHeaderBytes + 8, header_.total_disk_bytes_, 0, false, false, + false, false, false, false, RecordKind::kValue, ValueType::kString); + location_ = RecordLocation(1, 9, 7, 0, value_.size(), metadata); + } + + void TearDown() override { + worker_.Shutdown(); + storage_.impl_.reset(); + if (wake_fd_ >= 0) close(wake_fd_); + bycorf::MutableThisWorker() = std::move(saved_context_); + } + + RegisteredBufferPool& Pool() { return storage_.impl_->stores_[0]->buffers_; } + auto Read() { + auto& engine = *storage_.impl_; + auto& store = *engine.stores_[0]; + return Awaitable(engine, store, store.partitions_[0], 0, key_, location_, + nullptr); + } + + void Complete(Awaitable& read, int bytes = -1) { + ASSERT_TRUE(read.io_.has_value()); + const auto io = read.lease_.io_buffer(); + auto record = + std::span(io.data_ + read.record_headroom_, header_.total_disk_bytes_); + header_.db_epoch_ = read.db_epoch_; + header_.replication_epoch_ = read.replication_epoch_; + ASSERT_TRUE( + EncodeRecordHeader(header_, key_, record.first(header_.header_bytes_))); + std::memcpy(record.data() + header_.header_bytes_, value_.data(), + value_.size()); + // No awaiting handle was registered: Complete stores the result, and the + // worker ignores the empty continuation. await_resume is otherwise real. + read.io_->Complete(worker_, bytes == -1 ? read.read_bytes_ : bytes, 0); + } + + void Invalidate(bool database) { + auto& engine = *storage_.impl_; + if (database) + engine.db_epochs_[0].fetch_add(1); + else + ++engine.stores_[0]->partitions_[0].replication_epoch_; + } + + bycorf::CurrentWorker saved_context_; + bycorf::CrossCore cross_core_{1}; + int wake_fd_ = -1; + bycorf::Worker worker_; + BlockState block_; + StorageEngine storage_{StorageEngineOptions{}}; + std::string key_ = "direct-key"; + std::string value_ = "direct value"; + RecordHeader header_; + RecordLocation location_; +}; + +TEST_F(DirectValueReadTestPeer, StagedAndFreeingBlocksFallBackSynchronously) { + location_.set_in_memory(true); + { + auto read = Read(); + ASSERT_TRUE(read.await_ready()); + EXPECT_FALSE(read.await_resume().has_value()); + } + location_.set_in_memory(false); + block_.freeing_ = true; + { + auto read = Read(); + ASSERT_TRUE(read.await_ready()); + EXPECT_FALSE(read.await_resume().has_value()); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(DirectValueReadTestPeer, UnawaitedReadReleasesPinAndBuffer) { + { + auto read = Read(); + EXPECT_FALSE(read.await_ready()); + EXPECT_EQ(block_.pins_, 1); + EXPECT_EQ(Pool().available_read_buffers(), 0); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(DirectValueReadTestPeer, ExhaustedPoolFallsBackWithoutEnqueuingWaiter) { + { + auto held = Pool().AcquireReadBuffer().await_resume(); + ASSERT_TRUE(held.ok()); + auto read = Read(); + ASSERT_TRUE(read.await_ready()); + EXPECT_FALSE(read.await_resume().has_value()); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(DirectValueReadTestPeer, CompletionTransfersLeaseAndReleasesPin) { + Awaitable::Result result; + { + auto read = Read(); + ASSERT_FALSE(read.await_ready()); + ASSERT_NO_FATAL_FAILURE(Complete(read)); + result = read.await_resume(); + ASSERT_TRUE(result.has_value()); + ASSERT_TRUE(result->ok()) << result->status(); + auto bytes = result->value().value(); + EXPECT_EQ(std::string_view(reinterpret_cast(bytes.data()), + bytes.size()), + value_); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 0); + result.reset(); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(DirectValueReadTestPeer, PopulationChangesDiscardCompletedRead) { + for (bool database : {false, true}) { + { + auto read = Read(); + ASSERT_FALSE(read.await_ready()); + ASSERT_NO_FATAL_FAILURE(Complete(read)); + Invalidate(database); + auto result = read.await_resume(); + ASSERT_TRUE(result.has_value()); + EXPECT_TRUE(absl::IsNotFound(result->status())); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); + } +} + +TEST_F(DirectValueReadTestPeer, ErrorsReleaseResourcesAndStaleIdentityRetries) { + for (int completion : {0, -EIO, 4096}) { + { + auto read = Read(); + ASSERT_FALSE(read.await_ready()); + header_.payload_checksum_ ^= 1; + ASSERT_NO_FATAL_FAILURE(Complete(read, completion)); + header_.payload_checksum_ ^= 1; + auto result = read.await_resume(); + ASSERT_TRUE(result.has_value()); + EXPECT_FALSE(result->ok()); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); + } + { + auto read = Read(); + ASSERT_FALSE(read.await_ready()); + ++header_.allocation_epoch_; + ASSERT_NO_FATAL_FAILURE(Complete(read)); + EXPECT_FALSE(read.await_resume().has_value()); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +} // namespace lavik::storage From bc13523c64b463d2e18bb028eade4ca3b17f093d Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Mon, 12 Oct 2026 03:53:42 +0000 Subject: [PATCH 2/6] Unify value loading behind a direct awaitable --- CMakeLists.txt | 2 +- docs/architecture/04-storage-and-recovery.md | 15 +- include/lavik/storage/engine.h | 2 +- src/storage/engine/impl.h | 106 ++-- src/storage/engine/read.cpp | 620 +++++++++---------- tests/direct_value_read_test.cpp | 220 ------- tests/grouped/recovery_e2e_test.cpp | 5 +- tests/load_value_awaitable_test.cpp | 324 ++++++++++ 8 files changed, 699 insertions(+), 595 deletions(-) delete mode 100644 tests/direct_value_read_test.cpp create mode 100644 tests/load_value_awaitable_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index c0f63dcf5..7fbf20472 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1766,7 +1766,7 @@ if (BUILD_TESTING) tests/rdb_test.cpp tests/rdb_collection_test.cpp tests/record_location_test.cpp - tests/direct_value_read_test.cpp + tests/load_value_awaitable_test.cpp tests/request_gate_test.cpp tests/replication_command_test.cpp tests/replication_group_test.cpp diff --git a/docs/architecture/04-storage-and-recovery.md b/docs/architecture/04-storage-and-recovery.md index a6b261158..2621f81f9 100644 --- a/docs/architecture/04-storage-and-recovery.md +++ b/docs/architecture/04-storage-and-recovery.md @@ -649,12 +649,15 @@ rereads. Transaction-owned reads always use that locked path directly. Tombstones and expired values are invisible; an expired observation can enqueue a bounded active-expiration candidate. A staged location is copied or framed from its write buffer. A disk location is read on its physical block owner into -an aligned lease. For a complete inline String already on its key owner's disk, -GET can submit the read from an awaitable embedded in its existing request task, -without child loader coroutines. The awaitable owns the block pin and buffer -through completion and validates the record and population before returning it. -Buffer contention, staged data, other representations, and stale physical -locations retain the complete loader and its backpressure/relocation handling. +an aligned lease. `LoadValue` and its single-location `LoadValueLocal` share an +awaitable embedded in the caller's task. Local inline reads, including staged +copies, use that task directly when a buffer is available. The awaitable owns +the disk-read pin and buffer through completion and validates record identity, +checksums, and population before returning it. Buffer contention retains pool +backpressure; cross-owner, external, grouped, and relocation reads use lazy +helper tasks. A stale logical read resolves the current same-mutation location +before resuming its caller, while a single-location read returns the stale +status to its enclosing representation-specific retry loop. Worker-local MGET uses `BatchGetLocked` after the command has acquired its key locks. It classifies index entries in one coroutine and issues ordinary-size diff --git a/include/lavik/storage/engine.h b/include/lavik/storage/engine.h index c366e8f66..47deb7580 100644 --- a/include/lavik/storage/engine.h +++ b/include/lavik/storage/engine.h @@ -1944,7 +1944,7 @@ class StorageEngine { private: friend class ExpirationAuthorityTestPeer; friend class TombRaiderTestPeer; - friend class DirectValueReadTestPeer; + friend class LoadValueAwaitableTestPeer; class Impl; std::unique_ptr impl_; }; diff --git a/src/storage/engine/impl.h b/src/storage/engine/impl.h index 1690f9906..b1af002cf 100644 --- a/src/storage/engine/impl.h +++ b/src/storage/engine/impl.h @@ -3257,7 +3257,7 @@ class StorageEngine::Impl { private: friend class ExpirationAuthorityTestPeer; friend class TombRaiderTestPeer; - friend class DirectValueReadTestPeer; + friend class LoadValueAwaitableTestPeer; struct DurableSystemState { std::uint64_t generation_ = 0; @@ -3343,52 +3343,69 @@ class StorageEngine::Impl { } }; - // Opportunistic, owner-local disk read embedded in GetWithLockState's frame. - // nullopt selects LoadValue's complete representation/relocation state - // machine. - class DirectValueRead { + // The awaitable lives in the caller's frame. Ordinary local reads suspend + // that caller directly; only buffer waits and multi-step logical reads need + // a lazy helper Task. Local mode returns stale-location errors to its caller, + // while logical mode resolves same-mutation relocations before returning. + class LoadValueAwaitable final : public bycorf::OneShotIoAwaitable { public: - using Result = std::optional>; - DirectValueRead(Impl& engine, WorkerStore& store, - WorkerStore::PartitionStore& partition, std::uint8_t db_id, - std::string_view key, RecordLocation location, - ReadLatencyTrace* trace); - DirectValueRead(const DirectValueRead&) = delete; - DirectValueRead& operator=(const DirectValueRead&) = delete; - DirectValueRead(DirectValueRead&&) = delete; - DirectValueRead& operator=(DirectValueRead&&) = delete; - - bool await_ready() const noexcept { return !io_.has_value(); } - bool await_suspend(std::coroutine_handle<> awaiting) { - if (trace_) trace_->io_submit_ns_ = ReadTraceNowNanos(); - return io_->await_suspend(awaiting); - } + using Result = absl::StatusOr; + LoadValueAwaitable(Impl& engine, WorkerStore& store, + WorkerStore::PartitionStore& partition, + std::uint8_t db_id, std::string_view key, + const Digest& digest, RecordLocation location, + ExtentManifest extents, ReadLatencyTrace* trace, + GroupedObject::Handle grouped_snapshot); + LoadValueAwaitable(Impl& engine, WorkerStore& store, std::uint8_t db_id, + std::string_view key, RecordLocation location, + std::uint64_t replication_epoch, ReadLatencyTrace* trace, + std::optional expected_db_epoch, + ReadBufferLease lease = {}); + ~LoadValueAwaitable(); + + bool await_ready() const noexcept { return ready_ && !slow_.valid(); } + std::coroutine_handle<> await_suspend(std::coroutine_handle<> awaiting); Result await_resume(); + void Complete(bycorf::Worker& worker, int result, unsigned flags) override; private: - friend class DirectValueReadTestPeer; - struct Pin { - WorkerStore* store_ = nullptr; - BlockState* state_ = nullptr; - ~Pin(); + friend class LoadValueAwaitableTestPeer; + struct Payload { + std::uint64_t txid_; + std::uint32_t offset_; + std::uint32_t bytes_; + std::uint32_t checksum_; }; + // False means the pool is exhausted, without registering a buffer waiter. + bool PrepareLocal(); + absl::StatusOr ParseRecord(const std::byte* data) const; + Result FinishRecord(const std::byte* data, Payload payload, bool staged); + bool PopulationChanged() const; + void Unpin(); + void Retry(absl::Status failure); + Impl& engine_; - WorkerStore::PartitionStore& partition_; + WorkerStore& store_; + WorkerStore::PartitionStore* partition_ = nullptr; std::string_view key_; + Digest digest_{}; RecordLocation location_; ReadLatencyTrace* trace_; std::uint64_t replication_epoch_; - std::uint64_t db_epoch_; + std::optional expected_db_epoch_; + std::uint64_t aligned_offset_ = 0; + std::size_t file_id_ = 0; std::size_t record_headroom_ = 0; std::size_t read_bytes_ = 0; std::uint8_t db_id_; - Result ready_; - // The completion and lease die before the pin. Runtime shutdown drains - // I/O before destroying suspended frames; no continuation is heap-owned. - Pin pin_; + BlockState* pinned_ = nullptr; + std::optional payload_; + std::optional ready_; + // Runtime shutdown drains I/O before destroying suspended frames. The + // helper, when present, owns its own physical read and buffer waiter. ReadBufferLease lease_; - std::optional io_; + Task slow_; }; // Owns the page lease while a grouped reader inspects its checked envelope. @@ -3410,12 +3427,6 @@ class StorageEngine::Impl { absl::StatusOr EncodeDiskValue(LoadedValue loaded); - // Shared synchronous validation for ordinary and direct disk reads. - absl::StatusOr DecodeLoadedValue( - ReadBufferLease lease, std::uint8_t db_id, std::string_view key, - RecordLocation location, std::uint64_t replication_epoch, - std::size_t record_headroom, std::uint64_t expected_db_epoch); - void QueueExpiredCandidate(WorkerStore& store, std::uint16_t partition_id, std::uint8_t db_id, const RecordIndex::Entry& entry, @@ -3781,13 +3792,28 @@ class StorageEngine::Impl { WorkerStore& store, RecordIndex& index, const Digest& digest, std::string_view key, const RecordLocation& verified_location); - Task> LoadValue( + LoadValueAwaitable LoadValue( WorkerStore& key_store, WorkerStore::PartitionStore& partition, std::uint8_t db_id, std::string_view key, const Digest& digest, RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace = nullptr, GroupedObject::Handle grouped_snapshot = nullptr); + // Used only for multi-step reads or an aborted direct read. A supplied + // failure enters relocation resolution without reading the stale bytes again. + Task> LoadValueSlow( + WorkerStore& key_store, WorkerStore::PartitionStore& partition, + std::uint8_t db_id, std::string_view key, Digest digest, + RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, + GroupedObject::Handle grouped_snapshot = nullptr, + std::optional failure = std::nullopt); + + Task> LoadValueLocalWaiting( + WorkerStore& store, std::uint8_t db_id, std::string_view key, + RecordLocation location, std::uint64_t replication_epoch, + ReadLatencyTrace* trace, std::optional expected_db_epoch, + std::size_t read_bytes); + // The optional snapshot view must already own physical pins. Ordinary // reads instead retry GC relocation against the same logical root version. Task> LoadHashGroupPayload( @@ -3826,7 +3852,7 @@ class StorageEngine::Impl { ExtentManifest extents, ReadLatencyTrace* trace, bool grouped_payload = false); - Task> LoadValueLocal( + LoadValueAwaitable LoadValueLocal( WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace = nullptr, diff --git a/src/storage/engine/read.cpp b/src/storage/engine/read.cpp index 4c9d1c253..8af8cf976 100644 --- a/src/storage/engine/read.cpp +++ b/src/storage/engine/read.cpp @@ -306,18 +306,9 @@ Task> StorageEngine::Impl::GetWithLockState( trace->lookup_done_ns_ = ReadTraceNowNanos(); } - // Snapshot all entry-owned state before the direct I/O can suspend. - const auto location = MaterializeIndexLocation(*found); - auto extents = ExtentsFor(store, found); - auto direct = co_await DirectValueRead(*this, store, partition, db_id, key, - location, trace); - absl::StatusOr loaded; - if (direct.has_value()) { - loaded = std::move(*direct); - } else { - loaded = co_await LoadValue(store, partition, db_id, key, digest, - location, std::move(extents), trace); - } + auto loaded = co_await LoadValue(store, partition, db_id, key, digest, + MaterializeIndexLocation(*found), + ExtentsFor(store, found), trace); if (optimistic_read && !loaded.ok() && (loaded.status().code() == absl::StatusCode::kNotFound || loaded.status().code() == absl::StatusCode::kAborted)) { @@ -337,103 +328,6 @@ Task> StorageEngine::Impl::GetWithLockState( } } -StorageEngine::Impl::DirectValueRead::DirectValueRead( - Impl& engine, WorkerStore& store, WorkerStore::PartitionStore& partition, - std::uint8_t db_id, std::string_view key, RecordLocation location, - ReadLatencyTrace* trace) - : engine_(engine), - partition_(partition), - key_(key), - location_(location), - trace_(trace), - replication_epoch_(partition.replication_epoch_), - db_epoch_(engine.DbEpoch(db_id)), - db_id_(db_id) { - // Keep candidate populations, staged data and other representations on the - // complete loader. The index lookup and this admission never suspend, so a - // validated local block cannot be recycled before its pin is acquired. - if (location.grouped() || location.external() || location.key_indirect() || - location.in_memory() || location.block_owner() != store.worker_->id() || - partition.replica_sync_ != nullptr) - return; - LAVIK_FAULT_INJECT(if (LAVIK_FAULT_MATCHES("LAVIK_FAIL_VALUE_READ_KEY", - key)) { - ready_.emplace(absl::InternalError("injected value payload read failure")); - return; - }); - const auto [file_id, block_offset] = engine.FileOffset(location.block_id()); - const auto absolute_offset = block_offset + location.record_offset(); - const auto mask = static_cast(engine.direct_io_alignment_ - 1); - const auto aligned_offset = absolute_offset & ~mask; - record_headroom_ = absolute_offset - aligned_offset; - read_bytes_ = (record_headroom_ + location.total_disk_bytes() + - engine.direct_io_alignment_ - 1) & - ~mask; - if (trace_) trace_->buffer_acquire_start_ns_ = ReadTraceNowNanos(); - auto acquire = store.buffers_.AcquireReadBuffer(read_bytes_); - // Preserve bounded buffer backpressure through LoadValue's existing waiter - // when exhausted. The common path needs only the caller's I/O continuation. - if (!acquire.await_ready()) return; - auto acquired = acquire.await_resume(); - if (!acquired.ok()) { - ready_.emplace(acquired.status()); - return; - } - lease_ = std::move(*acquired); - if (trace_) { - trace_->buffer_acquired_ns_ = ReadTraceNowNanos(); - trace_->heap_read_buffer_ = lease_.buffer_id() == 0; - } - auto* state = engine.FindBlockState(store, location.block_id()); - if (state == nullptr || !state->allocated_ || state->freeing_ || - state->allocation_epoch_ != location.allocation_epoch()) - return; - ++state->pins_; - pin_.store_ = &store; - pin_.state_ = state; - if (trace_) trace_->disk_read_ = true; - auto buffer = lease_.io_buffer(); - assert(read_bytes_ <= buffer.size_); - buffer.size_ = read_bytes_; - io_.emplace(*store.worker_, store.files_[file_id], buffer, aligned_offset, - lease_.registered() - ? bycorf::SizeIoAwaitable::Operation::kReadFixed - : bycorf::SizeIoAwaitable::Operation::kRead); -} - -StorageEngine::Impl::DirectValueRead::Pin::~Pin() { - if (state_ == nullptr) return; - --state_->pins_; - if (state_->pins_ == 0 && state_->release_pending_) - ReleaseStagingBuffer(*store_, *state_); -} - -StorageEngine::Impl::DirectValueRead::Result -StorageEngine::Impl::DirectValueRead::await_resume() { - if (!io_) return std::move(ready_); - auto read = io_->await_resume(); - if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); - // Replica resets do not acquire individual key locks. Match LoadValue's - // population check before publishing either bytes or an I/O error. - if (partition_.replication_epoch_ != replication_epoch_ || - engine_.DbEpoch(db_id_) != db_epoch_) - return absl::NotFoundError("key not found"); - if (!read.ok()) { - if (absl::IsAborted(read.status())) return std::nullopt; - return read.status(); - } - if (*read != read_bytes_) - return absl::InternalError("short compact record read"); - auto loaded = engine_.DecodeLoadedValue(std::move(lease_), db_id_, key_, - location_, replication_epoch_, - record_headroom_, db_epoch_); - // A relocation or mismatched physical record must go through LoadValue's - // same-mutation retry and corruption checks, also for transaction-owned GET. - if (!loaded.ok() && absl::IsAborted(loaded.status())) return std::nullopt; - if (trace_ && loaded.ok()) trace_->decode_done_ns_ = ReadTraceNowNanos(); - return loaded; -} - Task> StorageEngine::Impl::BatchGetLocked( std::uint8_t db_id, std::span requests) { assert(db_id < kLogicalDatabaseCount); @@ -985,12 +879,14 @@ absl::StatusOr StorageEngine::Impl::EncodeDiskValue( } Task> -StorageEngine::Impl::LoadValue(WorkerStore& key_store, - WorkerStore::PartitionStore& partition, - std::uint8_t db_id, std::string_view key, - const Digest& digest, RecordLocation location, - ExtentManifest extents, ReadLatencyTrace* trace, - GroupedObject::Handle grouped_snapshot) { +StorageEngine::Impl::LoadValueSlow(WorkerStore& key_store, + WorkerStore::PartitionStore& partition, + std::uint8_t db_id, std::string_view key, + Digest digest, RecordLocation location, + ExtentManifest extents, + ReadLatencyTrace* trace, + GroupedObject::Handle grouped_snapshot, + std::optional failure) { if (location.grouped()) { co_return co_await LoadGroupedValue(key_store, partition, db_id, key, digest, location, @@ -1007,9 +903,8 @@ StorageEngine::Impl::LoadValue(WorkerStore& key_store, : std::nullopt; while (true) { assert(location.block_owner() < worker_count_); - // Every branch below assigns `loaded` before it is observed. Keep the - // required placeholder message-free so the common local GET path does not - // construct and then immediately discard an error payload. + // Every branch assigns `loaded` before it is observed; the placeholder + // does not need an allocated error message. absl::StatusOr loaded; // An external value's manifest is already decoded in this index entry, @@ -1017,7 +912,10 @@ StorageEngine::Impl::LoadValue(WorkerStore& key_store, // and let each extent go straight to its block's owner, rather than // hopping to the record's owner first and having it acquire the output // buffer from its pool and hand the lease back across workers. - if (location.external()) { + if (failure) { + loaded = std::move(*failure); + failure.reset(); + } else if (location.external()) { loaded = co_await LoadExternalValueLocal(key_store, location, std::move(extents), trace); } else if (location.block_owner() == key_store.worker_->id()) { @@ -1405,235 +1303,305 @@ StorageEngine::Impl::LoadExternalValueLocal(WorkerStore& store, co_return LoadedValue{std::move(output), value_offset, output_offset}; } -Task> -StorageEngine::Impl::LoadValueLocal( +StorageEngine::Impl::LoadValueAwaitable StorageEngine::Impl::LoadValue( + WorkerStore& store, WorkerStore::PartitionStore& partition, + std::uint8_t db_id, std::string_view key, const Digest& digest, + RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, + GroupedObject::Handle grouped_snapshot) { + return LoadValueAwaitable(*this, store, partition, db_id, key, digest, + location, std::move(extents), trace, + std::move(grouped_snapshot)); +} + +StorageEngine::Impl::LoadValueAwaitable StorageEngine::Impl::LoadValueLocal( WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, std::optional expected_db_epoch) { - LAVIK_FAULT_INJECT(if (LAVIK_FAULT_MATCHES("LAVIK_FAIL_VALUE_READ_KEY", - key)) co_return absl:: - InternalError("injected value payload read failure");); - if (location.external()) { - co_return absl::Status(absl::StatusCode::kInternal, - "external value was dispatched as inline"); - } - // TODO: Coalesce concurrent reads of the same aligned disk page. Key an - // in-flight table by - // (file_id, aligned offset, aligned length), submit one read, and fan the - // decoded result out to all waiting coroutines. In-flight operations must - // retain values/leases, never flat_hash_map iterators or element pointers. - const auto [file_id, block_offset] = FileOffset(location.block_id()); - const std::uint64_t absolute_offset = block_offset + location.record_offset(); - const std::uint64_t direct_io_mask = - static_cast(direct_io_alignment_ - 1); - const std::uint64_t aligned_offset = absolute_offset & ~direct_io_mask; - const std::size_t record_headroom = - static_cast(absolute_offset - aligned_offset); - const std::size_t record_span = record_headroom + location.total_disk_bytes(); - const std::size_t read_bytes = - (record_span + direct_io_alignment_ - 1) & ~direct_io_mask; - - // Acquire the output buffer before resolving any block state. This is the - // only suspension the staged-copy path would otherwise have, and it used - // to sit between reading the staging pointer and using it, so that path - // needed a pin to stop a flush from handing the staging buffer back. With - // the acquisition hoisted, the staged copy runs straight through on a - // worker that cannot preempt it, and only the disk read below pins. The - // disk sizing covers the staged copy too: read_bytes is at least - // total_disk_bytes, which is header plus payload. - if (trace != nullptr) { - trace->buffer_acquire_start_ns_ = ReadTraceNowNanos(); - } - auto acquired = co_await store.buffers_.AcquireReadBuffer(read_bytes); - if (!acquired.ok()) { - co_return acquired.status(); - } - ReadBufferLease lease = std::move(*acquired); - if (trace != nullptr) { - trace->buffer_acquired_ns_ = ReadTraceNowNanos(); - trace->heap_read_buffer_ = lease.buffer_id() == 0; + return LoadValueAwaitable(*this, store, db_id, key, location, + replication_epoch, trace, expected_db_epoch); +} + +StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( + Impl& engine, WorkerStore& store, WorkerStore::PartitionStore& partition, + std::uint8_t db_id, std::string_view key, const Digest& digest, + RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, + GroupedObject::Handle grouped_snapshot) + : engine_(engine), + store_(store), + // Grouped snapshots have their own population/pin protocol. + partition_(location.grouped() ? nullptr : &partition), + key_(key), + digest_(digest), + location_(location), + trace_(trace), + replication_epoch_(partition.replication_epoch_), + expected_db_epoch_(engine.EffectiveRecordDbEpoch(partition, db_id)), + db_id_(db_id) { + if (location.grouped() || location.external() || + location.block_owner() != store.worker_->id() || !PrepareLocal()) { + slow_ = engine.LoadValueSlow(store, partition, db_id, key, digest, location, + std::move(extents), trace, + std::move(grouped_snapshot)); + } else if (ready_ && absl::IsAborted(ready_->status())) { + Retry(ready_->status()); } +} - BlockState* state = FindBlockState(store, location.block_id()); - if (state == nullptr || !state->allocated_ || state->freeing_ || - state->allocation_epoch_ != location.allocation_epoch()) { - co_return absl::Status(absl::StatusCode::kAborted, - "stale index block epoch"); +StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( + Impl& engine, WorkerStore& store, std::uint8_t db_id, std::string_view key, + RecordLocation location, std::uint64_t replication_epoch, + ReadLatencyTrace* trace, std::optional expected_db_epoch, + ReadBufferLease lease) + : engine_(engine), + store_(store), + key_(key), + location_(location), + trace_(trace), + replication_epoch_(replication_epoch), + expected_db_epoch_(expected_db_epoch), + db_id_(db_id), + lease_(std::move(lease)) { + if (!PrepareLocal()) { + slow_ = engine.LoadValueLocalWaiting(store, db_id, key, location, + replication_epoch, trace, + expected_db_epoch, read_bytes_); } +} - // Only records appended since the last flush live in a staging buffer, so - // on a read-mostly workload this branch is rare. Keep the disk read on the - // straight-line path. - if (location.in_memory() && state->in_memory_) [[unlikely]] { - auto in_mem_buffer = StagingBufferFor(store, *state); - if (!in_mem_buffer.data_ || in_mem_buffer.size_ == 0 || - location.record_offset() + location.total_disk_bytes() > - in_mem_buffer.size_) { - co_return absl::Status(absl::StatusCode::kInternal, - "invalid in-memory location"); - } - if (trace != nullptr) { - trace->io_submit_ns_ = trace->buffer_acquired_ns_; - trace->io_complete_ns_ = trace->buffer_acquired_ns_; - } - FixedBuffer io = lease.io_buffer(); - if (location.external()) { - co_return absl::Status(absl::StatusCode::kInternal, - "external value requires extent loading"); - } - const std::byte* record_bytes = - in_mem_buffer.data_ + location.record_offset(); - RecordHeader record{}; - std::string_view disk_key; - if (!DecodeRecordHeader(std::span( - record_bytes, location.total_disk_bytes()), - &record, &disk_key) || - record.db_id_ != db_id || (!record.key_indirect_ && disk_key != key) || - record.kind_ != RecordKind::kValue || - record.db_epoch_ != expected_db_epoch.value_or(DbEpoch(db_id)) || - record.mutation_sequence_ != location.mutation_sequence_ || - record.replication_epoch_ != replication_epoch || - record.allocation_epoch_ != location.allocation_epoch() || - record.expire_at_ms_ != location.expire_at_ms_ || - record.value_type_ != location.value_type() || - record.external_ != location.external() || - record.key_indirect_ != location.key_indirect() || - location.logical_size_ != record.logical_size_ || - location.total_disk_bytes() != record.total_disk_bytes_) { - co_return absl::Status(absl::StatusCode::kAborted, - "record does not match in-memory location"); - } - const std::byte* payload_data = record_bytes + record.header_bytes_; - if (record.key_indirect_ && record.key_bytes_ != key.size()) { - co_return absl::Status(absl::StatusCode::kAborted, - "record key does not match location"); +StorageEngine::Impl::LoadValueAwaitable::~LoadValueAwaitable() { Unpin(); } + +void StorageEngine::Impl::LoadValueAwaitable::Unpin() { + auto* state = std::exchange(pinned_, nullptr); + if (!state) return; + --state->pins_; + if (state->pins_ == 0 && state->release_pending_) + ReleaseStagingBuffer(store_, *state); +} + +bool StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { + LAVIK_FAULT_INJECT(if (LAVIK_FAULT_MATCHES("LAVIK_FAIL_VALUE_READ_KEY", + key_)) { + ready_.emplace(absl::InternalError("injected value payload read failure")); + return true; + }); + if (location_.external()) { + ready_.emplace( + absl::InternalError("external value was dispatched as inline")); + return true; + } + const auto [file_id, block_offset] = engine_.FileOffset(location_.block_id()); + file_id_ = file_id; + const auto absolute = block_offset + location_.record_offset(); + const auto mask = + static_cast(engine_.direct_io_alignment_ - 1); + aligned_offset_ = absolute & ~mask; + record_headroom_ = absolute - aligned_offset_; + read_bytes_ = + (record_headroom_ + location_.total_disk_bytes() + mask) & ~mask; + if (!lease_.valid()) { + if (trace_) trace_->buffer_acquire_start_ns_ = ReadTraceNowNanos(); + auto acquire = store_.buffers_.AcquireReadBuffer(read_bytes_); + if (!acquire.await_ready()) return false; + auto acquired = acquire.await_resume(); + if (!acquired.ok()) { + ready_.emplace(acquired.status()); + return true; } - const std::size_t value_bytes = record.payload_bytes_; - if (record.value_type_ == ValueType::kString && !record.grouped_ && - !record.auxiliary_group_ && value_bytes != record.logical_size_) { - co_return absl::Status(absl::StatusCode::kInternal, - "inline value length does not match metadata"); + lease_ = std::move(*acquired); + } + if (trace_) { + trace_->buffer_acquired_ns_ = ReadTraceNowNanos(); + trace_->heap_read_buffer_ = lease_.buffer_id() == 0; + } + // Resolve staging memory only after acquiring the buffer: waiting can let + // flush/defrag retire it. The staged copy itself never suspends, so only + // disk I/O needs a physical pin. + auto* state = engine_.FindBlockState(store_, location_.block_id()); + if (!state || !state->allocated_ || state->freeing_ || + state->allocation_epoch_ != location_.allocation_epoch()) { + ready_.emplace(absl::AbortedError("stale index block epoch")); + return true; + } + if (location_.in_memory() && state->in_memory_) [[unlikely]] { + auto buffer = engine_.StagingBufferFor(store_, *state); + if (!buffer.data_ || + location_.record_offset() + location_.total_disk_bytes() > + buffer.size_) { + ready_.emplace(absl::InternalError("invalid in-memory location")); + return true; } - std::memcpy(io.data_, payload_data, value_bytes); - if (Crc32c(std::span( - payload_data, record.payload_bytes_)) != record.payload_checksum_) { - co_return absl::Status(absl::StatusCode::kInternal, - "record value checksum mismatch"); + if (trace_) { + trace_->io_submit_ns_ = trace_->buffer_acquired_ns_; + trace_->io_complete_ns_ = trace_->buffer_acquired_ns_; } - if (trace != nullptr) { - trace->decode_done_ns_ = ReadTraceNowNanos(); - } - const std::size_t value_offset = - static_cast(io.data_ - lease.bytes().data()); - co_return LoadedValue{std::move(lease), value_offset, value_bytes, - record.txid_}; + const auto* data = buffer.data_ + location_.record_offset(); + auto payload = ParseRecord(data); + if (!payload.ok()) + ready_.emplace(payload.status()); + else + ready_.emplace(FinishRecord(data, *payload, true)); + return true; + } + if (read_bytes_ > lease_.io_buffer().size_) { + ready_.emplace(absl::OutOfRangeError( + "record exceeds registered read buffer capacity")); + return true; } - - // Only the disk read suspends while holding the BlockState pointer, so it - // is the only path that has to keep the state alive with a pin. ++state->pins_; - struct PinGuard { - WorkerStore* store_ = nullptr; - BlockState* state_ = nullptr; - ~PinGuard() { - if (state_ == nullptr) { - return; - } - --state_->pins_; - if (state_->pins_ == 0 && state_->release_pending_) { - ReleaseStagingBuffer(*store_, *state_); - } - } - } pin{&store, state}; + pinned_ = state; + if (trace_) trace_->disk_read_ = true; + return true; +} - if (trace != nullptr) { - trace->disk_read_ = true; - } - FixedBuffer io = lease.io_buffer(); - if (read_bytes > io.size_) { - co_return absl::Status(absl::StatusCode::kOutOfRange, - "record exceeds registered read buffer capacity"); - } - FixedBuffer record_buffer = io; - record_buffer.size_ = read_bytes; - if (trace != nullptr) { - trace->io_submit_ns_ = ReadTraceNowNanos(); - } - auto read = co_await ReadStorageBuffer(*store.worker_, store.files_[file_id], - record_buffer, lease.registered(), - aligned_offset); - if (trace != nullptr) { - trace->io_complete_ns_ = ReadTraceNowNanos(); - } - if (!read.ok()) { - co_return read.status(); +bool StorageEngine::Impl::LoadValueAwaitable::PopulationChanged() const { + return partition_ && (partition_->replication_epoch_ != replication_epoch_ || + engine_.EffectiveRecordDbEpoch(*partition_, db_id_) != + *expected_db_epoch_); +} + +void StorageEngine::Impl::LoadValueAwaitable::Retry(absl::Status failure) { + assert(partition_ && absl::IsAborted(failure)); + Unpin(); + lease_.Reset(); + ready_.reset(); + // Inline reads have no extent manifest. Seed the failure so the slow path + // resolves the index before any further I/O and preserves corruption errors + // when the index still endorses the failed physical record. + slow_ = + engine_.LoadValueSlow(store_, *partition_, db_id_, key_, digest_, + location_, {}, trace_, nullptr, std::move(failure)); +} + +std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::await_suspend( + std::coroutine_handle<> awaiting) { + if (slow_.valid()) + return std::move(slow_).operator co_await().await_suspend(awaiting); + auto buffer = lease_.io_buffer(); + buffer.size_ = read_bytes_; + if (trace_) trace_->io_submit_ns_ = ReadTraceNowNanos(); + auto status = + lease_.registered() + ? store_.worker_->SubmitReadFixed(store_.files_[file_id_], buffer, + aligned_offset_, this) + : store_.worker_->SubmitRead(store_.files_[file_id_], + std::span(buffer.data_, buffer.size_), + aligned_offset_, this); + if (Suspend(awaiting, status)) return std::noop_coroutine(); + if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); + if (partition_ && absl::IsAborted(status)) { + Retry(std::move(status)); + return std::move(slow_).operator co_await().await_suspend(awaiting); } - if (*read != read_bytes) { - co_return absl::Status(absl::StatusCode::kInternal, - "short compact record read"); + return awaiting; +} + +void StorageEngine::Impl::LoadValueAwaitable::Complete(bycorf::Worker& worker, + int bytes, + unsigned flags) { + if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); + if (bytes >= 0) worker.RecordStorageReadCompletion(bytes); + if (partition_ && !PopulationChanged() && bytes >= 0 && + static_cast(bytes) == read_bytes_) { + // Only metadata is examined on the completion poller. CRC and large-value + // copies stay in the caller's scheduled slice. A stale identity must select + // the retry continuation before the caller's co_await can finish. + auto parsed = ParseRecord(lease_.io_buffer().data_ + record_headroom_); + if (parsed.ok()) + payload_ = *parsed; + else if (absl::IsAborted(parsed.status())) { + Retry(parsed.status()); + auto child = + std::move(slow_).operator co_await().await_suspend(awaiting_); + // CQEs are polled outside the parent's task class. Preserve background + // scheduling when a backup/maintenance read starts its retry here. + if (worker.IsBackground(awaiting_)) worker.RegisterBackground(child); + worker.Enqueue(child); + return; + } else + ready_.emplace(parsed.status()); + } + OneShotIoAwaitable::Complete(worker, bytes, flags); +} + +StorageEngine::Impl::LoadValueAwaitable::Result +StorageEngine::Impl::LoadValueAwaitable::await_resume() { + if (PopulationChanged()) return absl::NotFoundError("key not found"); + if (slow_.valid()) return std::move(slow_).TakeResult(); + if (ready_) return std::move(*ready_); + auto read = + Resume(lease_.registered() ? "fixed-buffer read failed" : "read failed"); + if (!read.ok()) return read.status(); + if (static_cast(*read) != read_bytes_) + return absl::InternalError("short compact record read"); + const auto* data = lease_.io_buffer().data_ + record_headroom_; + if (!payload_) { + auto parsed = ParseRecord(data); + if (!parsed.ok()) return parsed.status(); + payload_ = *parsed; } + return FinishRecord(data, *payload_, false); +} - auto loaded = DecodeLoadedValue(std::move(lease), db_id, key, location, - replication_epoch, record_headroom, - expected_db_epoch.value_or(DbEpoch(db_id))); - if (trace != nullptr && loaded.ok()) - trace->decode_done_ns_ = ReadTraceNowNanos(); - co_return loaded; +Task> +StorageEngine::Impl::LoadValueLocalWaiting( + WorkerStore& store, std::uint8_t db_id, std::string_view key, + RecordLocation location, std::uint64_t replication_epoch, + ReadLatencyTrace* trace, std::optional expected_db_epoch, + std::size_t read_bytes) { + // Register exactly one waiter and pass its assigned lease into the same + // physical reader. Retrying acquisition would lose the pool's FIFO handoff. + auto acquired = co_await store.buffers_.AcquireReadBuffer(read_bytes); + if (!acquired.ok()) co_return acquired.status(); + co_return co_await LoadValueAwaitable( + *this, store, db_id, key, location, replication_epoch, trace, + expected_db_epoch, std::move(*acquired)); } -absl::StatusOr -StorageEngine::Impl::DecodeLoadedValue(ReadBufferLease lease, - std::uint8_t db_id, std::string_view key, - RecordLocation location, - std::uint64_t replication_epoch, - std::size_t record_headroom, - std::uint64_t expected_db_epoch) { - const FixedBuffer io = lease.io_buffer(); +absl::StatusOr +StorageEngine::Impl::LoadValueAwaitable::ParseRecord( + const std::byte* data) const { RecordHeader record{}; std::string_view disk_key; - const std::byte* record_data = io.data_ + record_headroom; - std::span record_bytes(record_data, - location.total_disk_bytes()); - if (!DecodeRecordHeader(record_bytes, &record, &disk_key) || - record.db_id_ != db_id || (!record.key_indirect_ && disk_key != key) || + if (!DecodeRecordHeader(std::span(data, location_.total_disk_bytes()), + &record, &disk_key) || + record.db_id_ != db_id_ || (!record.key_indirect_ && disk_key != key_) || record.kind_ != RecordKind::kValue || - record.db_epoch_ != expected_db_epoch || - record.mutation_sequence_ != location.mutation_sequence_ || - record.replication_epoch_ != replication_epoch || - record.allocation_epoch_ != location.allocation_epoch() || - record.expire_at_ms_ != location.expire_at_ms_ || - record.value_type_ != location.value_type() || - record.external_ != location.external() || - record.key_indirect_ != location.key_indirect() || - record.logical_size_ != location.logical_size_ || - record.total_disk_bytes_ != location.total_disk_bytes()) { - return absl::Status(absl::StatusCode::kAborted, - "record does not match in-memory location"); - } - const std::byte* payload_data = record_data + record.header_bytes_; - if (record.key_indirect_ && record.key_bytes_ != key.size()) { - return absl::Status(absl::StatusCode::kAborted, - "record key does not match location"); - } - const std::size_t value_bytes = record.payload_bytes_; + record.db_epoch_ != + expected_db_epoch_.value_or(engine_.DbEpoch(db_id_)) || + record.mutation_sequence_ != location_.mutation_sequence_ || + record.replication_epoch_ != replication_epoch_ || + record.allocation_epoch_ != location_.allocation_epoch() || + record.expire_at_ms_ != location_.expire_at_ms_ || + record.value_type_ != location_.value_type() || + record.external_ != location_.external() || + record.key_indirect_ != location_.key_indirect() || + record.logical_size_ != location_.logical_size_ || + record.total_disk_bytes_ != location_.total_disk_bytes()) { + return absl::AbortedError("record does not match in-memory location"); + } + if (record.key_indirect_ && record.key_bytes_ != key_.size()) + return absl::AbortedError("record key does not match location"); if (record.value_type_ == ValueType::kString && !record.grouped_ && - !record.auxiliary_group_ && value_bytes != record.logical_size_) { - return absl::Status(absl::StatusCode::kInternal, - "inline value length does not match metadata"); - } - if (Crc32c(std::span(payload_data, record.payload_bytes_)) != - record.payload_checksum_) { - return absl::Status(absl::StatusCode::kInternal, - "record value checksum mismatch"); - } - const std::byte* framed_value = payload_data; - if (value_bytes > DirectGetValueLimit()) { - std::memmove(io.data_, payload_data, value_bytes); - framed_value = io.data_; - } - const std::size_t value_offset = - static_cast(framed_value - lease.bytes().data()); - return LoadedValue{std::move(lease), value_offset, value_bytes, record.txid_}; + !record.auxiliary_group_ && record.payload_bytes_ != record.logical_size_) + return absl::InternalError("inline value length does not match metadata"); + return Payload{record.txid_, record.header_bytes_, record.payload_bytes_, + record.payload_checksum_}; +} + +StorageEngine::Impl::LoadValueAwaitable::Result +StorageEngine::Impl::LoadValueAwaitable::FinishRecord(const std::byte* data, + Payload payload, + bool staged) { + const auto* value = data + payload.offset_; + if (Crc32c(std::span(value, payload.bytes_)) != payload.checksum_) + return absl::InternalError("record value checksum mismatch"); + if (staged || payload.bytes_ > engine_.DirectGetValueLimit()) { + std::memmove(lease_.io_buffer().data_, value, payload.bytes_); + value = lease_.io_buffer().data_; + } + const auto offset = static_cast(value - lease_.bytes().data()); + if (trace_) trace_->decode_done_ns_ = ReadTraceNowNanos(); + return LoadedValue{std::move(lease_), offset, payload.bytes_, payload.txid_}; } } // namespace lavik::storage diff --git a/tests/direct_value_read_test.cpp b/tests/direct_value_read_test.cpp deleted file mode 100644 index dc647abdc..000000000 --- a/tests/direct_value_read_test.cpp +++ /dev/null @@ -1,220 +0,0 @@ -/* - * Copyright (C) 2026 EloqData Inc. - * SPDX-License-Identifier: Apache-2.0 - */ - -#include - -#include "../src/storage/engine/impl.h" -#include "gtest/gtest.h" - -namespace lavik::storage { - -// Deliver deterministic CQEs to test invalidation and resource ownership at -// exact suspension boundaries. End-to-end storage tests cover real submission. -class DirectValueReadTestPeer : public testing::Test { - protected: - using Awaitable = StorageEngine::Impl::DirectValueRead; - - void SetUp() override { - saved_context_ = std::move(bycorf::MutableThisWorker()); - wake_fd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK); - ASSERT_GE(wake_fd_, 0); - cross_core_.mailbox(0).wake_fd_ = wake_fd_; - worker_.BindCrossCore(0, &cross_core_); - bycorf::SetThisWorker(0, &cross_core_, &worker_); - auto status = worker_.Init({.recv_buffer_count_ = 0}); - ASSERT_TRUE(status.ok()) << status; - auto& engine = *storage_.impl_; - engine.worker_count_ = 1; - engine.stores_.push_back( - std::make_unique()); - auto& store = *engine.stores_[0]; - store.worker_ = &worker_; - store.partitions_.resize(1); - store.partitions_[0].replication_epoch_ = 1; - store.files_.push_back(bycorf::FixedFile{}); - engine.db_epochs_[0].store(1); - engine.devices_.push_back(StorageDevice{.capacity_blocks_ = 2}); - engine.block_state_lookup_.push_back({&block_, 1, 2}); - block_.Reset(0, 7); - block_.allocated_ = true; - - RegisteredBufferPoolOptions options; - options.storage_write_buffer_count_ = 1; - options.registered_bytes_ = - options.write_buffer_bytes_ + options.read_slot_bytes_; - ASSERT_TRUE(Pool().Init(worker_, options).ok()); - ASSERT_EQ(Pool().available_read_buffers(), 1); - - header_.header_bytes_ = RecordHeaderBytes(key_.size()); - header_.key_bytes_ = key_.size(); - header_.value_type_ = ValueType::kString; - header_.logical_size_ = value_.size(); - header_.payload_bytes_ = value_.size(); - header_.total_disk_bytes_ = - AlignRecord(header_.header_bytes_ + value_.size()); - header_.allocation_epoch_ = 7; - header_.mutation_sequence_ = 9; - header_.payload_checksum_ = Crc32c(std::as_bytes(std::span(value_))); - const auto metadata = RecordLocation::PackedMetadata::Encode( - kBlockHeaderBytes + 8, header_.total_disk_bytes_, 0, false, false, - false, false, false, false, RecordKind::kValue, ValueType::kString); - location_ = RecordLocation(1, 9, 7, 0, value_.size(), metadata); - } - - void TearDown() override { - worker_.Shutdown(); - storage_.impl_.reset(); - if (wake_fd_ >= 0) close(wake_fd_); - bycorf::MutableThisWorker() = std::move(saved_context_); - } - - RegisteredBufferPool& Pool() { return storage_.impl_->stores_[0]->buffers_; } - auto Read() { - auto& engine = *storage_.impl_; - auto& store = *engine.stores_[0]; - return Awaitable(engine, store, store.partitions_[0], 0, key_, location_, - nullptr); - } - - void Complete(Awaitable& read, int bytes = -1) { - ASSERT_TRUE(read.io_.has_value()); - const auto io = read.lease_.io_buffer(); - auto record = - std::span(io.data_ + read.record_headroom_, header_.total_disk_bytes_); - header_.db_epoch_ = read.db_epoch_; - header_.replication_epoch_ = read.replication_epoch_; - ASSERT_TRUE( - EncodeRecordHeader(header_, key_, record.first(header_.header_bytes_))); - std::memcpy(record.data() + header_.header_bytes_, value_.data(), - value_.size()); - // No awaiting handle was registered: Complete stores the result, and the - // worker ignores the empty continuation. await_resume is otherwise real. - read.io_->Complete(worker_, bytes == -1 ? read.read_bytes_ : bytes, 0); - } - - void Invalidate(bool database) { - auto& engine = *storage_.impl_; - if (database) - engine.db_epochs_[0].fetch_add(1); - else - ++engine.stores_[0]->partitions_[0].replication_epoch_; - } - - bycorf::CurrentWorker saved_context_; - bycorf::CrossCore cross_core_{1}; - int wake_fd_ = -1; - bycorf::Worker worker_; - BlockState block_; - StorageEngine storage_{StorageEngineOptions{}}; - std::string key_ = "direct-key"; - std::string value_ = "direct value"; - RecordHeader header_; - RecordLocation location_; -}; - -TEST_F(DirectValueReadTestPeer, StagedAndFreeingBlocksFallBackSynchronously) { - location_.set_in_memory(true); - { - auto read = Read(); - ASSERT_TRUE(read.await_ready()); - EXPECT_FALSE(read.await_resume().has_value()); - } - location_.set_in_memory(false); - block_.freeing_ = true; - { - auto read = Read(); - ASSERT_TRUE(read.await_ready()); - EXPECT_FALSE(read.await_resume().has_value()); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); -} - -TEST_F(DirectValueReadTestPeer, UnawaitedReadReleasesPinAndBuffer) { - { - auto read = Read(); - EXPECT_FALSE(read.await_ready()); - EXPECT_EQ(block_.pins_, 1); - EXPECT_EQ(Pool().available_read_buffers(), 0); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); -} - -TEST_F(DirectValueReadTestPeer, ExhaustedPoolFallsBackWithoutEnqueuingWaiter) { - { - auto held = Pool().AcquireReadBuffer().await_resume(); - ASSERT_TRUE(held.ok()); - auto read = Read(); - ASSERT_TRUE(read.await_ready()); - EXPECT_FALSE(read.await_resume().has_value()); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); -} - -TEST_F(DirectValueReadTestPeer, CompletionTransfersLeaseAndReleasesPin) { - Awaitable::Result result; - { - auto read = Read(); - ASSERT_FALSE(read.await_ready()); - ASSERT_NO_FATAL_FAILURE(Complete(read)); - result = read.await_resume(); - ASSERT_TRUE(result.has_value()); - ASSERT_TRUE(result->ok()) << result->status(); - auto bytes = result->value().value(); - EXPECT_EQ(std::string_view(reinterpret_cast(bytes.data()), - bytes.size()), - value_); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 0); - result.reset(); - EXPECT_EQ(Pool().available_read_buffers(), 1); -} - -TEST_F(DirectValueReadTestPeer, PopulationChangesDiscardCompletedRead) { - for (bool database : {false, true}) { - { - auto read = Read(); - ASSERT_FALSE(read.await_ready()); - ASSERT_NO_FATAL_FAILURE(Complete(read)); - Invalidate(database); - auto result = read.await_resume(); - ASSERT_TRUE(result.has_value()); - EXPECT_TRUE(absl::IsNotFound(result->status())); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); - } -} - -TEST_F(DirectValueReadTestPeer, ErrorsReleaseResourcesAndStaleIdentityRetries) { - for (int completion : {0, -EIO, 4096}) { - { - auto read = Read(); - ASSERT_FALSE(read.await_ready()); - header_.payload_checksum_ ^= 1; - ASSERT_NO_FATAL_FAILURE(Complete(read, completion)); - header_.payload_checksum_ ^= 1; - auto result = read.await_resume(); - ASSERT_TRUE(result.has_value()); - EXPECT_FALSE(result->ok()); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); - } - { - auto read = Read(); - ASSERT_FALSE(read.await_ready()); - ++header_.allocation_epoch_; - ASSERT_NO_FATAL_FAILURE(Complete(read)); - EXPECT_FALSE(read.await_resume().has_value()); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); -} - -} // namespace lavik::storage diff --git a/tests/grouped/recovery_e2e_test.cpp b/tests/grouped/recovery_e2e_test.cpp index a0458dee0..0622ff957 100644 --- a/tests/grouped/recovery_e2e_test.cpp +++ b/tests/grouped/recovery_e2e_test.cpp @@ -1170,7 +1170,10 @@ TEST(GroupedRecoveryE2e, CheckpointAccountsForReassignedPhysicalOwners) { EXPECT_NE(recovered.Log().find("loaded shutdown checkpoint generation="), std::string::npos) << recovered.Log(); - EXPECT_EQ(recovered.Log().find("falling back"), std::string::npos) + // A kernel may independently fall back from provided-buffer receive. + // Only checkpoint recovery falling back to a record scan is a failure. + EXPECT_EQ(recovered.Log().find("falling back to record scan"), + std::string::npos) << recovered.Log(); EXPECT_EQ(recovered.Command({"HGET", "hash", "field"}), value); EXPECT_EQ(recovered.Command({"SET", "string", value}), "+OK"); diff --git a/tests/load_value_awaitable_test.cpp b/tests/load_value_awaitable_test.cpp new file mode 100644 index 000000000..f5a93cfa0 --- /dev/null +++ b/tests/load_value_awaitable_test.cpp @@ -0,0 +1,324 @@ +/* + * Copyright (C) 2026 EloqData Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#include "../src/storage/engine/impl.h" +#include "gtest/gtest.h" + +namespace lavik::storage { + +// Deliver deterministic CQEs at exact suspension boundaries. End-to-end +// storage tests cover real submission through the same LoadValue entry point. +class LoadValueAwaitableTestPeer : public testing::Test { + protected: + using Awaitable = StorageEngine::Impl::LoadValueAwaitable; + using Result = Awaitable::Result; + + void SetUp() override { + saved_context_ = std::move(bycorf::MutableThisWorker()); + wake_fd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK); + ASSERT_GE(wake_fd_, 0); + cross_core_.mailbox(0).wake_fd_ = wake_fd_; + worker_.BindCrossCore(0, &cross_core_); + bycorf::SetThisWorker(0, &cross_core_, &worker_); + ASSERT_TRUE(worker_.Init({.recv_buffer_count_ = 0}).ok()); + auto& engine = *storage_.impl_; + engine.worker_count_ = 1; + engine.stores_.push_back( + std::make_unique()); + auto& store = *engine.stores_[0]; + store.worker_ = &worker_; + store.partitions_.resize(1); + store.partition_indexes_.resize(kLogicalDatabaseCount); + store.partitions_[0].indexes_ = store.partition_indexes_; + store.partitions_[0].replication_epoch_ = 1; + store.files_.push_back(bycorf::FixedFile{}); + engine.db_epochs_[0].store(1); + engine.devices_.push_back(StorageDevice{.capacity_blocks_ = 2}); + engine.block_state_lookup_.push_back({&block_, 1, 2}); + block_.Reset(0, 7); + block_.allocated_ = true; + + RegisteredBufferPoolOptions options; + options.storage_write_buffer_count_ = 1; + options.registered_bytes_ = + options.write_buffer_bytes_ + options.read_slot_bytes_; + ASSERT_TRUE(Pool().Init(worker_, options).ok()); + ASSERT_EQ(Pool().available_read_buffers(), 1); + + header_.header_bytes_ = RecordHeaderBytes(key_.size()); + header_.key_bytes_ = key_.size(); + header_.value_type_ = ValueType::kString; + header_.logical_size_ = value_.size(); + header_.payload_bytes_ = value_.size(); + header_.total_disk_bytes_ = + AlignRecord(header_.header_bytes_ + value_.size()); + header_.allocation_epoch_ = 7; + header_.mutation_sequence_ = 9; + header_.payload_checksum_ = Crc32c(std::as_bytes(std::span(value_))); + const auto metadata = RecordLocation::PackedMetadata::Encode( + kBlockHeaderBytes + 8, header_.total_disk_bytes_, 0, false, false, + false, false, false, false, RecordKind::kValue, ValueType::kString); + location_ = RecordLocation(1, 9, 7, 0, value_.size(), metadata); + } + + void TearDown() override { + worker_.Shutdown(); + storage_.impl_.reset(); + if (wake_fd_ >= 0) close(wake_fd_); + bycorf::MutableThisWorker() = std::move(saved_context_); + } + + auto& Store() { return *storage_.impl_->stores_[0]; } + RegisteredBufferPool& Pool() { return Store().buffers_; } + auto Read() { + return storage_.impl_->LoadValue(Store(), Store().partitions_[0], 0, key_, + digest_, location_, {}); + } + auto ReadLocal() { + return storage_.impl_->LoadValueLocal(Store(), 0, key_, location_, 1); + } + bool HasHelper(const Awaitable& read) { return read.slow_.valid(); } + + void Encode(std::byte* data) { + ASSERT_TRUE(EncodeRecordHeader(header_, key_, + std::span(data, header_.header_bytes_))); + std::memcpy(data + header_.header_bytes_, value_.data(), value_.size()); + } + void Complete(Awaitable& read, int bytes = -1) { + ASSERT_NE(read.pinned_, nullptr); + Encode(read.lease_.io_buffer().data_ + read.record_headroom_); + read.Complete(worker_, bytes == -1 ? read.read_bytes_ : bytes, 0); + } + void Stage() { + location_.set_in_memory(true); + block_.in_memory_ = true; + if (block_.staging_slot_ == 0) { + block_.staging_slot_ = Store().staging_slots_.size(); + Store().staging_slots_.emplace_back(); + ASSERT_TRUE(Pool().TryAcquireWriteBuffer( + &Store().staging_slots_.back().write_buffer_id_)); + } + auto buffer = storage_.impl_->StagingBufferFor(Store(), block_); + Encode(buffer.data_ + location_.record_offset()); + } + void IndexLocation() { + ASSERT_NE(Store() + .partitions_[0] + .indexes_[0] + .InsertOrAssign(digest_, key_, location_) + .entry_, + nullptr); + } + void Invalidate(bool database) { + if (database) + storage_.impl_->db_epochs_[0].fetch_add(1); + else + ++Store().partitions_[0].replication_epoch_; + } + void CandidatePopulation() { + storage_.impl_->replica_loading_.store(true); + auto& partition = Store().partitions_[0]; + partition.replica_sync_ = std::make_unique< + StorageEngine::Impl::WorkerStore::PartitionStore::ReplicaSyncState>(); + partition.replica_sync_->local_db_epochs_[0] = 4; + header_.db_epoch_ = 4; + } + void ExpectValue(const Result& result) { + ASSERT_TRUE(result.ok()) << result.status(); + auto bytes = result->value(); + EXPECT_EQ(std::string_view(reinterpret_cast(bytes.data()), + bytes.size()), + value_); + } + + // Simulate only the submission, retaining the real completion -> retry -> + // parent continuation chain, with an owned Task to catch early/double resume. + Task AwaitCompletion(Awaitable& read) { + struct Arm { + Awaitable& read_; + bool await_ready() const { return false; } + void await_suspend(std::coroutine_handle<> parent) { + read_.awaiting_ = parent; + } + void await_resume() const {} + }; + co_await Arm{read}; + co_return read.await_resume(); + } + Task AwaitRead() { co_return co_await Read(); } + auto Start(Task& task, bool background = false) { + auto handle = std::move(task).operator co_await().await_suspend( + std::noop_coroutine()); + if (background) worker_.RegisterBackground(handle); + handle.resume(); + return handle; + } + void Drain(Task& task) { + for (int i = 0; i < 100 && !task.done(); ++i) worker_.RunOnce(false); + ASSERT_TRUE(task.done()); + } + bool HelperIsBackground(Awaitable& read) { + return worker_.IsBackground( + std::move(read.slow_).operator co_await().handle_); + } + + bycorf::CurrentWorker saved_context_; + bycorf::CrossCore cross_core_{1}; + int wake_fd_ = -1; + bycorf::Worker worker_; + BlockState block_; + StorageEngine storage_{StorageEngineOptions{}}; + std::string key_ = "direct-key"; + std::string value_ = "direct value"; + Digest digest_{17}; + RecordHeader header_; + RecordLocation location_; +}; + +TEST_F(LoadValueAwaitableTestPeer, StagedReadNeedsNoPinOrHelper) { + Stage(); + auto read = Read(); + ASSERT_TRUE(read.await_ready()); + EXPECT_FALSE(HasHelper(read)); + ExpectValue(read.await_resume()); + EXPECT_EQ(block_.pins_, 0); +} + +TEST_F(LoadValueAwaitableTestPeer, UnawaitedReadReleasesPinAndBuffer) { + { + auto read = Read(); + EXPECT_FALSE(read.await_ready()); + EXPECT_FALSE(HasHelper(read)); + EXPECT_EQ(block_.pins_, 1); + EXPECT_EQ(Pool().available_read_buffers(), 0); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, BufferWaitResumesWithAssignedLease) { + Stage(); + auto held = Pool().AcquireReadBuffer().await_resume(); + ASSERT_TRUE(held.ok()); + auto task = AwaitRead(); + Start(task); + EXPECT_FALSE(task.done()); + EXPECT_EQ(block_.pins_, 0); + held->Reset(); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + ExpectValue(std::move(task).TakeResult()); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, CompletionTransfersLeaseAndReleasesPin) { + std::optional result; + { + auto read = Read(); + ASSERT_FALSE(read.await_ready()); + ASSERT_NO_FATAL_FAILURE(Complete(read)); + result = read.await_resume(); + ExpectValue(*result); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 0); + result.reset(); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, PopulationChangesDiscardCompletedRead) { + for (bool database : {false, true}) { + { + auto read = Read(); + ASSERT_FALSE(read.await_ready()); + ASSERT_NO_FATAL_FAILURE(Complete(read)); + Invalidate(database); + EXPECT_TRUE(absl::IsNotFound(read.await_resume().status())); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); + ++header_.replication_epoch_; + } +} + +TEST_F(LoadValueAwaitableTestPeer, CandidatePopulationUsesMappedEpoch) { + CandidatePopulation(); + auto read = Read(); + EXPECT_FALSE(HasHelper(read)); + ASSERT_NO_FATAL_FAILURE(Complete(read)); + ExpectValue(read.await_resume()); +} + +TEST_F(LoadValueAwaitableTestPeer, ErrorsReleaseResources) { + for (int completion : {0, -EIO, 4096}) { + { + auto read = Read(); + ASSERT_FALSE(read.await_ready()); + header_.payload_checksum_ ^= 1; + ASSERT_NO_FATAL_FAILURE(Complete(read, completion)); + header_.payload_checksum_ ^= 1; + EXPECT_FALSE(read.await_resume().ok()); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); + } +} + +TEST_F(LoadValueAwaitableTestPeer, LocalReadReturnsStaleLocationToItsCaller) { + block_.freeing_ = true; + auto read = ReadLocal(); + ASSERT_TRUE(read.await_ready()); + EXPECT_FALSE(HasHelper(read)); + EXPECT_TRUE(absl::IsAborted(read.await_resume().status())); +} + +TEST_F(LoadValueAwaitableTestPeer, + RelocationRetriesBeforeResumingBackgroundCaller) { + auto read = Read(); + auto task = AwaitCompletion(read); + Start(task, true); + ++header_.allocation_epoch_; + ASSERT_NO_FATAL_FAILURE(Complete(read)); + EXPECT_FALSE(task.done()); + EXPECT_TRUE(HelperIsBackground(read)); + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); + --header_.allocation_epoch_; + // The new physical record has the same logical mutation and lives in staging. + location_.metadata_ = RecordLocation::PackedMetadata::Encode( + location_.record_offset() + 512, location_.total_disk_bytes(), 0, false, + false, false, false, false, false, RecordKind::kValue, + ValueType::kString); + Stage(); + IndexLocation(); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + ExpectValue(std::move(task).TakeResult()); +} + +TEST_F(LoadValueAwaitableTestPeer, UnchangedBadLocationIsCorruption) { + IndexLocation(); + auto read = Read(); + auto task = AwaitCompletion(read); + Start(task); + ++header_.allocation_epoch_; + ASSERT_NO_FATAL_FAILURE(Complete(read)); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + EXPECT_TRUE(absl::IsInternal(std::move(task).TakeResult().status())); +} + +TEST_F(LoadValueAwaitableTestPeer, ConcurrentLogicalMutationIsNotRelocation) { + auto read = Read(); + auto task = AwaitCompletion(read); + Start(task); + ++header_.mutation_sequence_; + ASSERT_NO_FATAL_FAILURE(Complete(read)); + ++location_.mutation_sequence_; + IndexLocation(); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + EXPECT_TRUE(absl::IsNotFound(std::move(task).TakeResult().status())); +} + +} // namespace lavik::storage From cc50c68ddf172810fba0a63a73b2c5c8c5d082dc Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Mon, 12 Oct 2026 04:34:03 +0000 Subject: [PATCH 3/6] Remove grouped forwarding and buffered extent loader frames --- docs/architecture/04-storage-and-recovery.md | 26 +- include/lavik/storage/buffer_pool.h | 9 + src/storage/buffer_pool.cpp | 20 +- src/storage/engine/impl.h | 94 +++-- src/storage/engine/read.cpp | 356 +++++++++++-------- tests/load_value_awaitable_test.cpp | 232 +++++++++++- 6 files changed, 530 insertions(+), 207 deletions(-) diff --git a/docs/architecture/04-storage-and-recovery.md b/docs/architecture/04-storage-and-recovery.md index 2621f81f9..56a06b7e7 100644 --- a/docs/architecture/04-storage-and-recovery.md +++ b/docs/architecture/04-storage-and-recovery.md @@ -651,13 +651,15 @@ a bounded active-expiration candidate. A staged location is copied or framed from its write buffer. A disk location is read on its physical block owner into an aligned lease. `LoadValue` and its single-location `LoadValueLocal` share an awaitable embedded in the caller's task. Local inline reads, including staged -copies, use that task directly when a buffer is available. The awaitable owns -the disk-read pin and buffer through completion and validates record identity, -checksums, and population before returning it. Buffer contention retains pool -backpressure; cross-owner, external, grouped, and relocation reads use lazy -helper tasks. A stale logical read resolves the current same-mutation location -before resuming its caller, while a single-location read returns the stale -status to its enclosing representation-specific retry loop. +copies, use that task directly. Buffer contention retains the pool's FIFO +handoff and schedules an owner-local continuation under the caller's task class; +it does not require a waiter coroutine. The awaitable owns the disk-read pin +and buffer through completion and validates record identity, checksums, and +population before returning it. Grouped reads enter their graph reader directly; +cross-owner, external, and relocation flows use lazy helper tasks. A stale +logical read resolves the current same-mutation location before resuming its +caller, while a single-location read returns the stale status to its enclosing +representation-specific retry loop. Worker-local MGET uses `BatchGetLocked` after the command has acquired its key locks. It classifies index entries in one coroutine and issues ordinary-size @@ -676,9 +678,13 @@ taking the reader's key lock; a stale physical read aborts and follows the current same-sequence relocation, while a true logical mutation becomes a normal miss. -External values read and validate every extent on its current owner in bounded -parallel waves, then assemble the logical value. Every read in a wave finishes -before its shared output buffer can be released, including on error. A move-only +External values read and validate every extent on its current owner and +assemble the logical value in manifest order. Whole-extent and slice reads +share a physical-read awaitable that retains its pin across buffer waiting, +I/O, and validation. Slices validate the entire extent before copying any +requested bytes. Local extent routing and physical reads need no child task; +cross-owner dispatch retains the runtime's task bridge. The output buffer +outlives every extent read, including on error. A move-only `ReadBufferLease` may cross to the connection worker; destruction returns a registered slot to its storage owner. diff --git a/include/lavik/storage/buffer_pool.h b/include/lavik/storage/buffer_pool.h index 759d2acb4..44bcd9f10 100644 --- a/include/lavik/storage/buffer_pool.h +++ b/include/lavik/storage/buffer_pool.h @@ -32,6 +32,7 @@ namespace bycorf { class CrossCore; class Worker; +struct RemoteWork; } // namespace bycorf namespace lavik::storage { @@ -208,12 +209,20 @@ class RegisteredBufferPool { bool await_suspend(std::coroutine_handle<> awaiting); absl::StatusOr await_resume(); + // Queue an owner-local continuation when a slot is handed off. This uses + // the worker's scheduled work queue (including its task-class budget), + // never runs inline in Release(), and sends no cross-worker reply. The + // caller supplies run_fn_/task_class_ and keeps both objects alive until + // that callback consumes the assigned lease through await_resume(). + bool SuspendWith(bycorf::RemoteWork& continuation); + private: friend class RegisteredBufferPool; RegisteredBufferPool* pool_ = nullptr; std::size_t minimum_payload_bytes_ = 0; std::coroutine_handle<> awaiting_{}; + bycorf::RemoteWork* continuation_ = nullptr; std::uint16_t assigned_buffer_id_ = 0; }; diff --git a/src/storage/buffer_pool.cpp b/src/storage/buffer_pool.cpp index 9a9ddf292..8d5f34ef1 100644 --- a/src/storage/buffer_pool.cpp +++ b/src/storage/buffer_pool.cpp @@ -468,16 +468,22 @@ bool RegisteredBufferPool::AcquireReadAwaiter::await_ready() const noexcept { bool RegisteredBufferPool::AcquireReadAwaiter::await_suspend( std::coroutine_handle<> awaiting) { - if (pool_ == nullptr || !pool_->initialized() || - minimum_payload_bytes_ > pool_->options_.read_payload_bytes() || - !pool_->free_read_buffers_.empty()) { - return false; - } + if (await_ready()) return false; awaiting_ = awaiting; pool_->read_waiters_.push_back(this); return true; } +bool RegisteredBufferPool::AcquireReadAwaiter::SuspendWith( + bycorf::RemoteWork& continuation) { + if (await_ready()) return false; + continuation_ = &continuation; + continuation.origin_ = pool_->owner_worker_; + continuation.reply_deferred_ = true; + pool_->read_waiters_.push_back(this); + return true; +} + absl::StatusOr RegisteredBufferPool::AcquireReadAwaiter::await_resume() { if (pool_ == nullptr || !pool_->initialized()) { @@ -717,7 +723,9 @@ void RegisteredBufferPool::ReleaseLocal(std::uint16_t buffer_id) noexcept { AcquireReadAwaiter* waiter = read_waiters_.front(); read_waiters_.pop_front(); waiter->assigned_buffer_id_ = buffer_id; - if (worker_ != nullptr) { + if (waiter->continuation_ != nullptr) { + bycorf::PostRequest(cross_core_, owner_worker_, waiter->continuation_); + } else if (worker_ != nullptr) { worker_->Enqueue(waiter->awaiting_); } return; diff --git a/src/storage/engine/impl.h b/src/storage/engine/impl.h index b1af002cf..83f39e248 100644 --- a/src/storage/engine/impl.h +++ b/src/storage/engine/impl.h @@ -3344,10 +3344,12 @@ class StorageEngine::Impl { }; // The awaitable lives in the caller's frame. Ordinary local reads suspend - // that caller directly; only buffer waits and multi-step logical reads need - // a lazy helper Task. Local mode returns stale-location errors to its caller, - // while logical mode resolves same-mutation relocations before returning. - class LoadValueAwaitable final : public bycorf::OneShotIoAwaitable { + // that caller directly, including through pool backpressure. Only multi-step + // logical reads need a lazy helper Task. Local mode returns stale-location + // errors to its caller, while logical mode resolves same-mutation relocations + // before returning. + class LoadValueAwaitable final : public bycorf::OneShotIoAwaitable, + private bycorf::RemoteWork { public: using Result = absl::StatusOr; LoadValueAwaitable(Impl& engine, WorkerStore& store, @@ -3359,8 +3361,7 @@ class StorageEngine::Impl { LoadValueAwaitable(Impl& engine, WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, - std::optional expected_db_epoch, - ReadBufferLease lease = {}); + std::optional expected_db_epoch); ~LoadValueAwaitable(); bool await_ready() const noexcept { return ready_ && !slow_.valid(); } @@ -3377,8 +3378,10 @@ class StorageEngine::Impl { std::uint32_t checksum_; }; - // False means the pool is exhausted, without registering a buffer waiter. - bool PrepareLocal(); + void PrepareLocal(); + void FinishBufferWait(); + static void BufferReady(bycorf::RemoteWork* work); + std::coroutine_handle<> SuspendRead(std::coroutine_handle<> awaiting); absl::StatusOr ParseRecord(const std::byte* data) const; Result FinishRecord(const std::byte* data, Payload payload, bool staged); bool PopulationChanged() const; @@ -3403,11 +3406,55 @@ class StorageEngine::Impl { std::optional payload_; std::optional ready_; // Runtime shutdown drains I/O before destroying suspended frames. The - // helper, when present, owns its own physical read and buffer waiter. + // waiter retains its assigned slot until the scheduled continuation runs. ReadBufferLease lease_; + std::optional buffer_wait_; Task slow_; }; + // One physical extent read, including buffer waiting and slice validation, + // embedded in the caller. Routing adds a runtime task bridge only when the + // block belongs to another worker; local reads need no child coroutine. + class ReadExtentAwaitable final : public bycorf::OneShotIoAwaitable, + private bycorf::RemoteWork { + public: + ReadExtentAwaitable(Impl& engine, WorkerStore& store, ExtentRef ref, + std::uint32_t extent_index, std::size_t source_offset, + std::span destination, bool route = false); + ~ReadExtentAwaitable(); + bool await_ready() const noexcept { return ready_.has_value(); } + bool await_suspend(std::coroutine_handle<> awaiting); + absl::Status await_resume(); + + private: + friend class LoadValueAwaitableTestPeer; + struct RemoteRead { + Impl* engine_; + unsigned owner_; + ExtentRef ref_; + std::uint32_t extent_index_; + std::size_t source_offset_; + std::span destination_; + Task operator()(); + }; + void FinishBufferWait(); + static void BufferReady(bycorf::RemoteWork* work); + bool SubmitRead(std::coroutine_handle<> awaiting); + + Impl& engine_; + WorkerStore& store_; + ExtentRef ref_; + std::uint32_t extent_index_; + std::size_t source_offset_; + std::span destination_; + std::size_t read_bytes_ = 0; + BlockState* pinned_ = nullptr; + std::optional ready_; + ReadBufferLease lease_; + std::optional buffer_wait_; + std::optional> remote_; + }; + // Owns the page lease while a grouped reader inspects its checked envelope. // Views into loaded_.value() must not outlive this object. struct LoadedHashGroupPayload { @@ -3805,15 +3852,8 @@ class StorageEngine::Impl { WorkerStore& key_store, WorkerStore::PartitionStore& partition, std::uint8_t db_id, std::string_view key, Digest digest, RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, - GroupedObject::Handle grouped_snapshot = nullptr, std::optional failure = std::nullopt); - Task> LoadValueLocalWaiting( - WorkerStore& store, std::uint8_t db_id, std::string_view key, - RecordLocation location, std::uint64_t replication_epoch, - ReadLatencyTrace* trace, std::optional expected_db_epoch, - std::size_t read_bytes); - // The optional snapshot view must already own physical pins. Ordinary // reads instead retry GC relocation against the same logical root version. Task> LoadHashGroupPayload( @@ -3833,19 +3873,15 @@ class StorageEngine::Impl { std::uint8_t db_id, std::string_view key, const Digest& digest, RecordLocation location, GroupedObject::Handle snapshot); - // Reads one extent block's payload into `destination`. Runs on the worker - // that owns that block, which is not necessarily the one holding the - // manifest, so everything it needs is passed by value. - Task ReadExtentInto(WorkerStore& store, ExtentRef ref, - std::uint32_t extent_index, - std::byte* destination); - Task ReadExtentOnOwner(ExtentRef ref, - std::uint32_t extent_index, - std::byte* destination); - Task ReadExtentSlice(WorkerStore& store, ExtentRef ref, - std::uint32_t extent_index, - std::size_t source_offset, - std::span destination); + // Reads an entire extent on its physical owner into caller-owned storage. + // Local routing returns the physical awaitable without a wrapper task. + ReadExtentAwaitable ReadExtentOnOwner(ExtentRef ref, + std::uint32_t extent_index, + std::byte* destination); + ReadExtentAwaitable ReadExtentSlice(WorkerStore& store, ExtentRef ref, + std::uint32_t extent_index, + std::size_t source_offset, + std::span destination); Task> LoadExternalValueLocal( WorkerStore& store, const RecordLocation& location, diff --git a/src/storage/engine/read.cpp b/src/storage/engine/read.cpp index 8af8cf976..917113ea6 100644 --- a/src/storage/engine/read.cpp +++ b/src/storage/engine/read.cpp @@ -885,13 +885,8 @@ StorageEngine::Impl::LoadValueSlow(WorkerStore& key_store, Digest digest, RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, - GroupedObject::Handle grouped_snapshot, std::optional failure) { - if (location.grouped()) { - co_return co_await LoadGroupedValue(key_store, partition, db_id, key, - digest, location, - std::move(grouped_snapshot)); - } + assert(!location.grouped()); // The caller already resolved the key's partition for the index lookup. // Reuse it across retries instead of recomputing the Redis slot. const std::uint64_t replication_epoch = partition.replication_epoch_; @@ -977,131 +972,155 @@ StorageEngine::Impl::LoadValueSlow(WorkerStore& key_store, } } -Task StorageEngine::Impl::ReadExtentInto( - WorkerStore& store, ExtentRef ref, std::uint32_t extent_index, - std::byte* destination) { - BlockState* state = FindBlockState(store, ref.block_id_); - if (state == nullptr || !state->allocated_ || state->freeing_ || - state->kind_ != BlockKind::kPayloadExtent || - state->allocation_epoch_ != ref.allocation_epoch_) { - co_return absl::Status(absl::StatusCode::kAborted, - "stale or missing external extent"); - } - ++state->pins_; - struct ExtentPin { - BlockState* state_; - ~ExtentPin() { --state_->pins_; } - } pin{state}; - const std::size_t read_bytes = - AlignDirect(kBlockHeaderBytes + ref.payload_bytes_); - auto temp_acquired = co_await store.buffers_.AcquireReadBuffer(read_bytes); - if (!temp_acquired.ok()) { - co_return temp_acquired.status(); - } - ReadBufferLease temp = std::move(*temp_acquired); - FixedBuffer io = temp.io_buffer(); - io.size_ = read_bytes; - const auto [file_id, block_offset] = FileOffset(ref.block_id_); - auto read = co_await ReadStorageBuffer(*store.worker_, store.files_[file_id], - io, temp.registered(), block_offset); - if (!read.ok()) { - co_return read.status(); - } - if (*read != read_bytes) { - co_return absl::Status(absl::StatusCode::kInternal, - "short extent block read"); - } - BlockHeader header{}; - if (!DecodeBlockHeaderPages(std::span( - io.data_, kBlockHeaderBytes), - &header) || - header.kind_ != BlockKind::kPayloadExtent || - header.block_id_ != ref.block_id_ || - header.allocation_epoch_ != ref.allocation_epoch_ || - header.extent_index_ != extent_index || - header.extent_payload_bytes_ != ref.payload_bytes_ || - header.extent_payload_checksum_ != ref.payload_checksum_) { - co_return absl::Status(absl::StatusCode::kInternal, - "extent header does not match manifest"); - } - const auto payload = std::span(io.data_ + kBlockHeaderBytes, - ref.payload_bytes_); - if (Crc32c(payload) != ref.payload_checksum_) { - co_return absl::Status(absl::StatusCode::kInternal, - "extent payload checksum mismatch"); - } - std::memcpy(destination, payload.data(), payload.size()); - co_return absl::OkStatus(); -} - -Task StorageEngine::Impl::ReadExtentOnOwner( +StorageEngine::Impl::ReadExtentAwaitable StorageEngine::Impl::ReadExtentOnOwner( ExtentRef ref, std::uint32_t extent_index, std::byte* destination) { - const unsigned owner = BlockOwner(ref.block_id_); - // The block may be recycled after manifest validation. Recheck its owner - // before routing, and let ReadExtentInto validate the allocation epoch. - if (owner >= worker_count_) - co_return absl::AbortedError("stale or missing external extent"); - if (owner == bycorf::ThisWorker().id_) - co_return co_await ReadExtentInto(*stores_[owner], ref, extent_index, - destination); - co_return co_await bycorf::SubmitTaskTo( - owner, [this, owner, ref, extent_index, destination]() { - return ReadExtentInto(*stores_[owner], ref, extent_index, destination); - }); + return ReadExtentAwaitable(*this, CurrentStore(), ref, extent_index, 0, + std::span(destination, ref.payload_bytes_), true); } -Task StorageEngine::Impl::ReadExtentSlice( +StorageEngine::Impl::ReadExtentAwaitable StorageEngine::Impl::ReadExtentSlice( WorkerStore& store, ExtentRef ref, std::uint32_t extent_index, std::size_t source_offset, std::span destination) { + return ReadExtentAwaitable(*this, store, ref, extent_index, source_offset, + destination); +} + +Task +StorageEngine::Impl::ReadExtentAwaitable::RemoteRead::operator()() { + // SubmitTaskTo requires a Task on the physical owner. The local awaitable + // below supplies the I/O and pool continuation without another loader frame. + co_return co_await engine_->ReadExtentSlice(*engine_->stores_[owner_], ref_, + extent_index_, source_offset_, + destination_); +} + +StorageEngine::Impl::ReadExtentAwaitable::ReadExtentAwaitable( + Impl& engine, WorkerStore& store, ExtentRef ref, std::uint32_t extent_index, + std::size_t source_offset, std::span destination, bool route) + : engine_(engine), + store_(store), + ref_(ref), + extent_index_(extent_index), + source_offset_(source_offset), + destination_(destination) { if (source_offset > ref.payload_bytes_ || destination.size() > ref.payload_bytes_ - source_offset) { - co_return absl::InvalidArgumentError("extent slice is out of range"); + ready_ = absl::InvalidArgumentError("extent slice is out of range"); + return; } - BlockState* state = FindBlockState(store, ref.block_id_); - if (state == nullptr || !state->allocated_ || state->freeing_ || + if (route) { + const unsigned owner = engine.BlockOwner(ref.block_id_); + if (owner >= engine.worker_count_) { + ready_ = absl::AbortedError("stale or missing external extent"); + return; + } + if (owner != store.worker_->id()) { + remote_.emplace(bycorf::SubmitTaskTo( + owner, RemoteRead{&engine, owner, ref, extent_index, source_offset, + destination})); + return; + } + } + // Recheck the physical owner and incarnation after routing. Retain the + // extent pin across pool waiting as well as I/O, matching manifest reads. + auto* state = engine.FindBlockState(store, ref.block_id_); + if (!state || !state->allocated_ || state->freeing_ || state->kind_ != BlockKind::kPayloadExtent || state->allocation_epoch_ != ref.allocation_epoch_) { - co_return absl::AbortedError("stale or missing external extent"); + ready_ = absl::AbortedError("stale or missing external extent"); + return; } ++state->pins_; - struct ExtentPin { - BlockState* state_; - ~ExtentPin() { --state_->pins_; } - } pin{state}; - const std::size_t read_bytes = - AlignDirect(kBlockHeaderBytes + ref.payload_bytes_); - auto acquired = co_await store.buffers_.AcquireReadBuffer(read_bytes); - if (!acquired.ok()) co_return acquired.status(); - ReadBufferLease buffer = std::move(*acquired); - FixedBuffer io = buffer.io_buffer(); - io.size_ = read_bytes; - const auto [file_id, block_offset] = FileOffset(ref.block_id_); - auto read = co_await ReadStorageBuffer(*store.worker_, store.files_[file_id], - io, buffer.registered(), block_offset); - if (!read.ok()) co_return read.status(); - if (*read != read_bytes) { - co_return absl::InternalError("short extent block read"); + pinned_ = state; + read_bytes_ = AlignDirect(kBlockHeaderBytes + ref.payload_bytes_); + buffer_wait_.emplace(&store.buffers_, read_bytes_); + if (buffer_wait_->await_ready()) FinishBufferWait(); +} + +StorageEngine::Impl::ReadExtentAwaitable::~ReadExtentAwaitable() { + if (pinned_) --pinned_->pins_; +} + +void StorageEngine::Impl::ReadExtentAwaitable::FinishBufferWait() { + auto acquired = buffer_wait_->await_resume(); + buffer_wait_.reset(); + if (!acquired.ok()) + ready_ = acquired.status(); + else + lease_ = std::move(*acquired); +} + +void StorageEngine::Impl::ReadExtentAwaitable::BufferReady( + bycorf::RemoteWork* work) { + auto& read = *static_cast(work); + read.FinishBufferWait(); + if (read.ready_ || !read.SubmitRead(read.awaiting_)) + read.store_.worker_->Enqueue(read.awaiting_); +} + +bool StorageEngine::Impl::ReadExtentAwaitable::await_suspend( + std::coroutine_handle<> awaiting) { + if (remote_) { + remote_->await_suspend(awaiting); + return true; } + if (buffer_wait_) { + awaiting_ = awaiting; + run_fn_ = &BufferReady; + task_class_ = store_.worker_->IsBackground(awaiting) + ? bycorf::TaskClass::kBackground + : bycorf::CurrentTaskClass(); + if (buffer_wait_->SuspendWith(*this)) return true; + FinishBufferWait(); + if (ready_) return false; + } + return SubmitRead(awaiting); +} + +bool StorageEngine::Impl::ReadExtentAwaitable::SubmitRead( + std::coroutine_handle<> awaiting) { + auto buffer = lease_.io_buffer(); + buffer.size_ = read_bytes_; + const auto [file_id, offset] = engine_.FileOffset(ref_.block_id_); + return Suspend( + awaiting, lease_.registered() + ? store_.worker_->SubmitReadFixed(store_.files_[file_id], + buffer, offset, this) + : store_.worker_->SubmitRead( + store_.files_[file_id], + std::span(buffer.data_, buffer.size_), offset, this)); +} + +absl::Status StorageEngine::Impl::ReadExtentAwaitable::await_resume() { + if (remote_) return remote_->await_resume(); + if (ready_) return std::move(*ready_); + auto read = + Resume(lease_.registered() ? "fixed-buffer read failed" : "read failed"); + if (!read.ok()) return read.status(); + store_.worker_->RecordStorageReadCompletion(*read); + if (static_cast(*read) != read_bytes_) + return absl::InternalError("short extent block read"); + const auto buffer = lease_.io_buffer(); BlockHeader header{}; if (!DecodeBlockHeaderPages(std::span( - io.data_, kBlockHeaderBytes), + buffer.data_, kBlockHeaderBytes), &header) || header.kind_ != BlockKind::kPayloadExtent || - header.block_id_ != ref.block_id_ || - header.allocation_epoch_ != ref.allocation_epoch_ || - header.extent_index_ != extent_index || - header.extent_payload_bytes_ != ref.payload_bytes_ || - header.extent_payload_checksum_ != ref.payload_checksum_) { - co_return absl::InternalError("extent header does not match manifest"); - } - const auto payload = std::span(io.data_ + kBlockHeaderBytes, - ref.payload_bytes_); - if (Crc32c(payload) != ref.payload_checksum_) { - co_return absl::InternalError("extent payload checksum mismatch"); - } - std::memcpy(destination.data(), payload.data() + source_offset, - destination.size()); - co_return absl::OkStatus(); + header.block_id_ != ref_.block_id_ || + header.allocation_epoch_ != ref_.allocation_epoch_ || + header.extent_index_ != extent_index_ || + header.extent_payload_bytes_ != ref_.payload_bytes_ || + header.extent_payload_checksum_ != ref_.payload_checksum_) + return absl::InternalError("extent header does not match manifest"); + const auto payload = std::span( + buffer.data_ + kBlockHeaderBytes, ref_.payload_bytes_); + // Even a partial read validates the entire extent before publishing bytes. + // This runs in the resumed task's budget, never on the I/O completion poller. + if (Crc32c(payload) != ref_.payload_checksum_) + return absl::InternalError("extent payload checksum mismatch"); + std::memcpy(destination_.data(), payload.data() + source_offset_, + destination_.size()); + return absl::OkStatus(); } Task> StorageEngine::Impl::LoadOutOfIndexKey( @@ -1337,21 +1356,25 @@ StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( replication_epoch_(partition.replication_epoch_), expected_db_epoch_(engine.EffectiveRecordDbEpoch(partition, db_id)), db_id_(db_id) { - if (location.grouped() || location.external() || - location.block_owner() != store.worker_->id() || !PrepareLocal()) { + if (location.grouped()) { + // The grouped task owns its graph/version protocol. Keep its borrowed + // digest in this awaitable, not in the constructor's argument lifetime. + slow_ = engine.LoadGroupedValue(store, partition, db_id, key, digest_, + location, std::move(grouped_snapshot)); + } else if (location.external() || + location.block_owner() != store.worker_->id()) { slow_ = engine.LoadValueSlow(store, partition, db_id, key, digest, location, - std::move(extents), trace, - std::move(grouped_snapshot)); - } else if (ready_ && absl::IsAborted(ready_->status())) { - Retry(ready_->status()); + std::move(extents), trace); + } else { + PrepareLocal(); + if (ready_ && absl::IsAborted(ready_->status())) Retry(ready_->status()); } } StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( Impl& engine, WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, - ReadLatencyTrace* trace, std::optional expected_db_epoch, - ReadBufferLease lease) + ReadLatencyTrace* trace, std::optional expected_db_epoch) : engine_(engine), store_(store), key_(key), @@ -1359,13 +1382,8 @@ StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( trace_(trace), replication_epoch_(replication_epoch), expected_db_epoch_(expected_db_epoch), - db_id_(db_id), - lease_(std::move(lease)) { - if (!PrepareLocal()) { - slow_ = engine.LoadValueLocalWaiting(store, db_id, key, location, - replication_epoch, trace, - expected_db_epoch, read_bytes_); - } + db_id_(db_id) { + PrepareLocal(); } StorageEngine::Impl::LoadValueAwaitable::~LoadValueAwaitable() { Unpin(); } @@ -1378,16 +1396,16 @@ void StorageEngine::Impl::LoadValueAwaitable::Unpin() { ReleaseStagingBuffer(store_, *state); } -bool StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { +void StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { LAVIK_FAULT_INJECT(if (LAVIK_FAULT_MATCHES("LAVIK_FAIL_VALUE_READ_KEY", key_)) { ready_.emplace(absl::InternalError("injected value payload read failure")); - return true; + return; }); if (location_.external()) { ready_.emplace( absl::InternalError("external value was dispatched as inline")); - return true; + return; } const auto [file_id, block_offset] = engine_.FileOffset(location_.block_id()); file_id_ = file_id; @@ -1401,11 +1419,14 @@ bool StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { if (!lease_.valid()) { if (trace_) trace_->buffer_acquire_start_ns_ = ReadTraceNowNanos(); auto acquire = store_.buffers_.AcquireReadBuffer(read_bytes_); - if (!acquire.await_ready()) return false; + if (!acquire.await_ready()) { + buffer_wait_.emplace(&store_.buffers_, read_bytes_); + return; + } auto acquired = acquire.await_resume(); if (!acquired.ok()) { ready_.emplace(acquired.status()); - return true; + return; } lease_ = std::move(*acquired); } @@ -1420,7 +1441,7 @@ bool StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { if (!state || !state->allocated_ || state->freeing_ || state->allocation_epoch_ != location_.allocation_epoch()) { ready_.emplace(absl::AbortedError("stale index block epoch")); - return true; + return; } if (location_.in_memory() && state->in_memory_) [[unlikely]] { auto buffer = engine_.StagingBufferFor(store_, *state); @@ -1428,7 +1449,7 @@ bool StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { location_.record_offset() + location_.total_disk_bytes() > buffer.size_) { ready_.emplace(absl::InternalError("invalid in-memory location")); - return true; + return; } if (trace_) { trace_->io_submit_ns_ = trace_->buffer_acquired_ns_; @@ -1440,17 +1461,16 @@ bool StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { ready_.emplace(payload.status()); else ready_.emplace(FinishRecord(data, *payload, true)); - return true; + return; } if (read_bytes_ > lease_.io_buffer().size_) { ready_.emplace(absl::OutOfRangeError( "record exceeds registered read buffer capacity")); - return true; + return; } ++state->pins_; pinned_ = state; if (trace_) trace_->disk_read_ = true; - return true; } bool StorageEngine::Impl::LoadValueAwaitable::PopulationChanged() const { @@ -1467,15 +1487,52 @@ void StorageEngine::Impl::LoadValueAwaitable::Retry(absl::Status failure) { // Inline reads have no extent manifest. Seed the failure so the slow path // resolves the index before any further I/O and preserves corruption errors // when the index still endorses the failed physical record. - slow_ = - engine_.LoadValueSlow(store_, *partition_, db_id_, key_, digest_, - location_, {}, trace_, nullptr, std::move(failure)); + slow_ = engine_.LoadValueSlow(store_, *partition_, db_id_, key_, digest_, + location_, {}, trace_, std::move(failure)); +} + +void StorageEngine::Impl::LoadValueAwaitable::FinishBufferWait() { + auto acquired = buffer_wait_->await_resume(); + buffer_wait_.reset(); + if (PopulationChanged()) { + ready_.emplace(absl::NotFoundError("key not found")); + } else if (!acquired.ok()) { + ready_.emplace(acquired.status()); + } else { + lease_ = std::move(*acquired); + PrepareLocal(); + if (partition_ && ready_ && absl::IsAborted(ready_->status())) + Retry(ready_->status()); + } +} + +void StorageEngine::Impl::LoadValueAwaitable::BufferReady( + bycorf::RemoteWork* work) { + auto& read = *static_cast(work); + read.FinishBufferWait(); + auto next = read.SuspendRead(read.awaiting_); + if (next != std::noop_coroutine()) read.store_.worker_->Enqueue(next); } std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::await_suspend( std::coroutine_handle<> awaiting) { + if (buffer_wait_) { + awaiting_ = awaiting; + run_fn_ = &BufferReady; + task_class_ = store_.worker_->IsBackground(awaiting) + ? bycorf::TaskClass::kBackground + : bycorf::CurrentTaskClass(); + if (buffer_wait_->SuspendWith(*this)) return std::noop_coroutine(); + FinishBufferWait(); + } + return SuspendRead(awaiting); +} + +std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::SuspendRead( + std::coroutine_handle<> awaiting) { if (slow_.valid()) return std::move(slow_).operator co_await().await_suspend(awaiting); + if (ready_) return awaiting; auto buffer = lease_.io_buffer(); buffer.size_ = read_bytes_; if (trace_) trace_->io_submit_ns_ = ReadTraceNowNanos(); @@ -1542,21 +1599,6 @@ StorageEngine::Impl::LoadValueAwaitable::await_resume() { return FinishRecord(data, *payload_, false); } -Task> -StorageEngine::Impl::LoadValueLocalWaiting( - WorkerStore& store, std::uint8_t db_id, std::string_view key, - RecordLocation location, std::uint64_t replication_epoch, - ReadLatencyTrace* trace, std::optional expected_db_epoch, - std::size_t read_bytes) { - // Register exactly one waiter and pass its assigned lease into the same - // physical reader. Retrying acquisition would lose the pool's FIFO handoff. - auto acquired = co_await store.buffers_.AcquireReadBuffer(read_bytes); - if (!acquired.ok()) co_return acquired.status(); - co_return co_await LoadValueAwaitable( - *this, store, db_id, key, location, replication_epoch, trace, - expected_db_epoch, std::move(*acquired)); -} - absl::StatusOr StorageEngine::Impl::LoadValueAwaitable::ParseRecord( const std::byte* data) const { diff --git a/tests/load_value_awaitable_test.cpp b/tests/load_value_awaitable_test.cpp index f5a93cfa0..f84497d2f 100644 --- a/tests/load_value_awaitable_test.cpp +++ b/tests/load_value_awaitable_test.cpp @@ -5,6 +5,8 @@ #include +#include + #include "../src/storage/engine/impl.h" #include "gtest/gtest.h" @@ -16,6 +18,7 @@ class LoadValueAwaitableTestPeer : public testing::Test { protected: using Awaitable = StorageEngine::Impl::LoadValueAwaitable; using Result = Awaitable::Result; + using ExtentRead = StorageEngine::Impl::ReadExtentAwaitable; void SetUp() override { saved_context_ = std::move(bycorf::MutableThisWorker()); @@ -150,15 +153,98 @@ class LoadValueAwaitableTestPeer : public testing::Test { co_return read.await_resume(); } Task AwaitRead() { co_return co_await Read(); } - auto Start(Task& task, bool background = false) { + Task AwaitExisting(Awaitable& read) { co_return co_await read; } + Task AwaitExtent(ExtentRead& read) { co_return co_await read; } + bool BufferWaitIsBackground(Awaitable& read) { + return read.task_class_ == bycorf::TaskClass::kBackground; + } + bool HasRemote(const ExtentRead& read) { return read.remote_.has_value(); } + auto ExtentSlice(ExtentRef ref, std::size_t offset, + std::span destination) { + return storage_.impl_->ReadExtentSlice(Store(), ref, 0, offset, + destination); + } + auto ExtentOnOwner(ExtentRef ref, std::span destination) { + return storage_.impl_->ReadExtentOnOwner(ref, 0, destination.data()); + } + + // Use a real fixed-file read for pool -> I/O handoff, rather than completing + // the request before the continuation actually submits it. + void InstallDiskImage(std::span bytes) { + char path[] = "/tmp/lavik-awaitable-XXXXXX"; + const int fd = mkstemp(path); + ASSERT_GE(fd, 0); + const auto offset = storage_.impl_->FileOffset(1).second; + const auto written = pwrite(fd, bytes.data(), bytes.size(), offset); + close(fd); + ASSERT_EQ(written, static_cast(bytes.size())); + struct Opened : bycorf::IoCompletion { + bool done_ = false; + int result_ = -1; + void Complete(bycorf::Worker&, int result, unsigned) override { + result_ = result; + done_ = true; + } + } opened; + ASSERT_TRUE(worker_.RegisterFixedFiles(1).ok()); + auto submitted = + worker_.SubmitOpenDirect(path, O_RDONLY | O_DIRECT, 0, {0}, &opened); + if (submitted.ok()) { + const auto until = + std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!opened.done_ && std::chrono::steady_clock::now() < until) + worker_.RunOnce(false); + } + unlink(path); + ASSERT_TRUE(submitted.ok()) << submitted; + ASSERT_TRUE(opened.done_); + ASSERT_EQ(opened.result_, 0); + } + void InstallRecord() { + std::vector bytes( + AlignDirect(location_.record_offset() + location_.total_disk_bytes())); + Encode(bytes.data() + location_.record_offset()); + InstallDiskImage(bytes); + } + ExtentRef ExtentReference() { + return {1, 7, static_cast(value_.size()), + Crc32c(std::as_bytes(std::span(value_)))}; + } + void InstallExtent(bool corrupt = false) { + block_.kind_ = BlockKind::kPayloadExtent; + auto ref = ExtentReference(); + std::vector bytes( + AlignDirect(kBlockHeaderBytes + value_.size())); + BlockHeader header; + header.block_id_ = 1; + header.allocation_epoch_ = 7; + header.kind_ = BlockKind::kPayloadExtent; + header.layout_worker_count_ = 1; + header.committed_bytes_ = kBlockHeaderBytes + value_.size(); + header.extent_payload_bytes_ = value_.size(); + header.extent_payload_checksum_ = ref.payload_checksum_; + EncodeBlockHeader(header, std::span( + bytes.data(), kBlockHeaderSlotBytes)); + std::memcpy(bytes.data() + kBlockHeaderBytes, value_.data(), value_.size()); + // Corrupt beyond the slice returned by the test, inside the actual payload. + if (corrupt) bytes[kBlockHeaderBytes + value_.size() - 1] ^= std::byte{1}; + InstallDiskImage(bytes); + } + + template + auto Start(Task& task, bool background = false) { auto handle = std::move(task).operator co_await().await_suspend( std::noop_coroutine()); if (background) worker_.RegisterBackground(handle); handle.resume(); return handle; } - void Drain(Task& task) { - for (int i = 0; i < 100 && !task.done(); ++i) worker_.RunOnce(false); + template + void Drain(Task& task) { + const auto until = + std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (!task.done() && std::chrono::steady_clock::now() < until) + worker_.RunOnce(false); ASSERT_TRUE(task.done()); } bool HelperIsBackground(Awaitable& read) { @@ -200,20 +286,156 @@ TEST_F(LoadValueAwaitableTestPeer, UnawaitedReadReleasesPinAndBuffer) { EXPECT_EQ(Pool().available_read_buffers(), 1); } -TEST_F(LoadValueAwaitableTestPeer, BufferWaitResumesWithAssignedLease) { +TEST_F(LoadValueAwaitableTestPeer, BufferWaitUsesAssignedLeaseWithoutHelper) { Stage(); auto held = Pool().AcquireReadBuffer().await_resume(); ASSERT_TRUE(held.ok()); - auto task = AwaitRead(); + auto read = Read(); + EXPECT_FALSE(HasHelper(read)); + auto task = AwaitExisting(read); + Start(task, true); + EXPECT_FALSE(task.done()); + EXPECT_TRUE(BufferWaitIsBackground(read)); + EXPECT_EQ(block_.pins_, 0); + held->Reset(); + // Handoff schedules the continuation; no decoding or nested resume occurs + // inside the buffer owner's release stack. + EXPECT_FALSE(task.done()); + EXPECT_EQ(Pool().available_read_buffers(), 0); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + ExpectValue(std::move(task).TakeResult()); + EXPECT_FALSE(HasHelper(read)); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, BufferWaitersPreserveFifoHandoff) { + Stage(); + auto held = Pool().AcquireReadBuffer().await_resume(); + ASSERT_TRUE(held.ok()); + auto first = Read(); + auto second = Read(); + auto first_task = AwaitExisting(first); + auto second_task = AwaitExisting(second); + Start(first_task); + Start(second_task); + held->Reset(); + ASSERT_NO_FATAL_FAILURE(Drain(first_task)); + EXPECT_FALSE(second_task.done()); + // Taking/destroying the first result hands its sole slot to the second. + ExpectValue(std::move(first_task).TakeResult()); + ASSERT_NO_FATAL_FAILURE(Drain(second_task)); + ExpectValue(std::move(second_task).TakeResult()); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, BufferWaitContinuesIntoRealDiskIo) { + ASSERT_NO_FATAL_FAILURE(InstallRecord()); + auto held = Pool().AcquireReadBuffer().await_resume(); + ASSERT_TRUE(held.ok()); + auto read = Read(); + auto task = AwaitExisting(read); Start(task); EXPECT_FALSE(task.done()); + held->Reset(); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + ExpectValue(std::move(task).TakeResult()); + EXPECT_FALSE(HasHelper(read)); +} + +TEST_F(LoadValueAwaitableTestPeer, PopulationChangeDuringBufferWaitSkipsIo) { + auto held = Pool().AcquireReadBuffer().await_resume(); + ASSERT_TRUE(held.ok()); + auto read = Read(); + auto task = AwaitExisting(read); + Start(task); + Invalidate(true); + held->Reset(); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + EXPECT_TRUE(absl::IsNotFound(std::move(task).TakeResult().status())); + EXPECT_FALSE(HasHelper(read)); EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, RelocationDuringBufferWaitUsesNewLocation) { + auto held = Pool().AcquireReadBuffer().await_resume(); + ASSERT_TRUE(held.ok()); + auto read = Read(); + auto task = AwaitExisting(read); + Start(task, true); + EXPECT_EQ(block_.pins_, 0); + // No physical pointer was retained while waiting: recycle before handoff. + block_.Reset(0, 8); + block_.allocated_ = true; + header_.allocation_epoch_ = 8; + location_ = RecordLocation(1, 9, 8, 0, value_.size(), location_.metadata_); + Stage(); + IndexLocation(); held->Reset(); ASSERT_NO_FATAL_FAILURE(Drain(task)); ExpectValue(std::move(task).TakeResult()); + EXPECT_TRUE(HasHelper(read)); + EXPECT_EQ(block_.pins_, 0); EXPECT_EQ(Pool().available_read_buffers(), 1); } +TEST_F(LoadValueAwaitableTestPeer, ExtentReadWaitsAndRoutesLocallyWithoutTask) { + ASSERT_NO_FATAL_FAILURE(InstallExtent()); + auto held = Pool().AcquireReadBuffer().await_resume(); + ASSERT_TRUE(held.ok()); + std::vector output(value_.size()); + { + auto read = ExtentOnOwner(ExtentReference(), output); + EXPECT_FALSE(HasRemote(read)); + EXPECT_FALSE(read.await_ready()); + EXPECT_EQ(block_.pins_, 1); + auto task = AwaitExtent(read); + Start(task, true); + held->Reset(); + EXPECT_FALSE(task.done()); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + auto status = std::move(task).TakeResult(); + EXPECT_TRUE(status.ok()) << status; + EXPECT_EQ(std::string_view(reinterpret_cast(output.data()), + output.size()), + value_); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, ExtentSliceChecksWholePayloadBeforeCopying) { + ASSERT_NO_FATAL_FAILURE(InstallExtent(true)); + std::array output{std::byte{0x55}, std::byte{0x55}}; + { + auto read = ExtentSlice(ExtentReference(), 0, output); + auto task = AwaitExtent(read); + Start(task); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + auto status = std::move(task).TakeResult(); + EXPECT_TRUE(absl::IsInternal(status)) << status; + EXPECT_EQ(status.message(), "extent payload checksum mismatch"); + EXPECT_EQ(output[0], std::byte{0x55}); + EXPECT_EQ(output[1], std::byte{0x55}); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); +} + +TEST_F(LoadValueAwaitableTestPeer, ExtentSliceReturnsRequestedBytes) { + ASSERT_NO_FATAL_FAILURE(InstallExtent()); + std::array output{}; + auto read = ExtentSlice(ExtentReference(), 2, output); + auto task = AwaitExtent(read); + Start(task); + ASSERT_NO_FATAL_FAILURE(Drain(task)); + auto status = std::move(task).TakeResult(); + EXPECT_TRUE(status.ok()) << status; + EXPECT_EQ(std::string_view(reinterpret_cast(output.data()), + output.size()), + std::string_view(value_).substr(2, output.size())); +} + TEST_F(LoadValueAwaitableTestPeer, CompletionTransfersLeaseAndReleasesPin) { std::optional result; { From fcad839675b5e0aa5d962ac81f19ddbfe336ee82 Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Mon, 12 Oct 2026 04:44:16 +0000 Subject: [PATCH 4/6] Trim awaitable tests and remove redundant read epoch checks --- src/storage/engine/read.cpp | 10 ++- tests/load_value_awaitable_test.cpp | 129 ++++++++-------------------- 2 files changed, 41 insertions(+), 98 deletions(-) diff --git a/src/storage/engine/read.cpp b/src/storage/engine/read.cpp index 917113ea6..6d9401122 100644 --- a/src/storage/engine/read.cpp +++ b/src/storage/engine/read.cpp @@ -1557,7 +1557,7 @@ void StorageEngine::Impl::LoadValueAwaitable::Complete(bycorf::Worker& worker, unsigned flags) { if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); if (bytes >= 0) worker.RecordStorageReadCompletion(bytes); - if (partition_ && !PopulationChanged() && bytes >= 0 && + if (partition_ && bytes >= 0 && static_cast(bytes) == read_bytes_) { // Only metadata is examined on the completion poller. CRC and large-value // copies stay in the caller's scheduled slice. A stale identity must select @@ -1565,7 +1565,7 @@ void StorageEngine::Impl::LoadValueAwaitable::Complete(bycorf::Worker& worker, auto parsed = ParseRecord(lease_.io_buffer().data_ + record_headroom_); if (parsed.ok()) payload_ = *parsed; - else if (absl::IsAborted(parsed.status())) { + else if (absl::IsAborted(parsed.status()) && !PopulationChanged()) { Retry(parsed.status()); auto child = std::move(slow_).operator co_await().await_suspend(awaiting_); @@ -1582,6 +1582,8 @@ void StorageEngine::Impl::LoadValueAwaitable::Complete(bycorf::Worker& worker, StorageEngine::Impl::LoadValueAwaitable::Result StorageEngine::Impl::LoadValueAwaitable::await_resume() { + // Fence successful reads here, after the scheduling gap following the CQE. + // Completion only needs this check when deciding whether to start a retry. if (PopulationChanged()) return absl::NotFoundError("key not found"); if (slow_.valid()) return std::move(slow_).TakeResult(); if (ready_) return std::move(*ready_); @@ -1608,8 +1610,8 @@ StorageEngine::Impl::LoadValueAwaitable::ParseRecord( &record, &disk_key) || record.db_id_ != db_id_ || (!record.key_indirect_ && disk_key != key_) || record.kind_ != RecordKind::kValue || - record.db_epoch_ != - expected_db_epoch_.value_or(engine_.DbEpoch(db_id_)) || + record.db_epoch_ != (expected_db_epoch_ ? *expected_db_epoch_ + : engine_.DbEpoch(db_id_)) || record.mutation_sequence_ != location_.mutation_sequence_ || record.replication_epoch_ != replication_epoch_ || record.allocation_epoch_ != location_.allocation_epoch() || diff --git a/tests/load_value_awaitable_test.cpp b/tests/load_value_awaitable_test.cpp index f84497d2f..d5e1a4302 100644 --- a/tests/load_value_awaitable_test.cpp +++ b/tests/load_value_awaitable_test.cpp @@ -12,8 +12,8 @@ namespace lavik::storage { -// Deliver deterministic CQEs at exact suspension boundaries. End-to-end -// storage tests cover real submission through the same LoadValue entry point. +// Deliver deterministic CQEs for identity races and real I/O for buffer +// handoff. class LoadValueAwaitableTestPeer : public testing::Test { protected: using Awaitable = StorageEngine::Impl::LoadValueAwaitable; @@ -152,21 +152,16 @@ class LoadValueAwaitableTestPeer : public testing::Test { co_await Arm{read}; co_return read.await_resume(); } - Task AwaitRead() { co_return co_await Read(); } Task AwaitExisting(Awaitable& read) { co_return co_await read; } Task AwaitExtent(ExtentRead& read) { co_return co_await read; } bool BufferWaitIsBackground(Awaitable& read) { return read.task_class_ == bycorf::TaskClass::kBackground; } - bool HasRemote(const ExtentRead& read) { return read.remote_.has_value(); } auto ExtentSlice(ExtentRef ref, std::size_t offset, std::span destination) { return storage_.impl_->ReadExtentSlice(Store(), ref, 0, offset, destination); } - auto ExtentOnOwner(ExtentRef ref, std::span destination) { - return storage_.impl_->ReadExtentOnOwner(ref, 0, destination.data()); - } // Use a real fixed-file read for pool -> I/O handoff, rather than completing // the request before the continuation actually submits it. @@ -265,83 +260,33 @@ class LoadValueAwaitableTestPeer : public testing::Test { RecordLocation location_; }; -TEST_F(LoadValueAwaitableTestPeer, StagedReadNeedsNoPinOrHelper) { - Stage(); - auto read = Read(); - ASSERT_TRUE(read.await_ready()); - EXPECT_FALSE(HasHelper(read)); - ExpectValue(read.await_resume()); - EXPECT_EQ(block_.pins_, 0); -} - -TEST_F(LoadValueAwaitableTestPeer, UnawaitedReadReleasesPinAndBuffer) { - { - auto read = Read(); - EXPECT_FALSE(read.await_ready()); - EXPECT_FALSE(HasHelper(read)); - EXPECT_EQ(block_.pins_, 1); - EXPECT_EQ(Pool().available_read_buffers(), 0); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); -} - -TEST_F(LoadValueAwaitableTestPeer, BufferWaitUsesAssignedLeaseWithoutHelper) { - Stage(); - auto held = Pool().AcquireReadBuffer().await_resume(); - ASSERT_TRUE(held.ok()); - auto read = Read(); - EXPECT_FALSE(HasHelper(read)); - auto task = AwaitExisting(read); - Start(task, true); - EXPECT_FALSE(task.done()); - EXPECT_TRUE(BufferWaitIsBackground(read)); - EXPECT_EQ(block_.pins_, 0); - held->Reset(); - // Handoff schedules the continuation; no decoding or nested resume occurs - // inside the buffer owner's release stack. - EXPECT_FALSE(task.done()); - EXPECT_EQ(Pool().available_read_buffers(), 0); - ASSERT_NO_FATAL_FAILURE(Drain(task)); - ExpectValue(std::move(task).TakeResult()); - EXPECT_FALSE(HasHelper(read)); - EXPECT_EQ(Pool().available_read_buffers(), 1); -} - TEST_F(LoadValueAwaitableTestPeer, BufferWaitersPreserveFifoHandoff) { - Stage(); + ASSERT_NO_FATAL_FAILURE(InstallRecord()); auto held = Pool().AcquireReadBuffer().await_resume(); ASSERT_TRUE(held.ok()); auto first = Read(); auto second = Read(); auto first_task = AwaitExisting(first); auto second_task = AwaitExisting(second); - Start(first_task); + Start(first_task, true); Start(second_task); + EXPECT_TRUE(BufferWaitIsBackground(first)); + EXPECT_EQ(block_.pins_, 0); held->Reset(); + // Handoff schedules I/O; it must not resume inside the release stack. + EXPECT_FALSE(first_task.done()); + EXPECT_EQ(Pool().available_read_buffers(), 0); ASSERT_NO_FATAL_FAILURE(Drain(first_task)); EXPECT_FALSE(second_task.done()); // Taking/destroying the first result hands its sole slot to the second. ExpectValue(std::move(first_task).TakeResult()); ASSERT_NO_FATAL_FAILURE(Drain(second_task)); ExpectValue(std::move(second_task).TakeResult()); + EXPECT_FALSE(HasHelper(first)); + EXPECT_FALSE(HasHelper(second)); EXPECT_EQ(Pool().available_read_buffers(), 1); } -TEST_F(LoadValueAwaitableTestPeer, BufferWaitContinuesIntoRealDiskIo) { - ASSERT_NO_FATAL_FAILURE(InstallRecord()); - auto held = Pool().AcquireReadBuffer().await_resume(); - ASSERT_TRUE(held.ok()); - auto read = Read(); - auto task = AwaitExisting(read); - Start(task); - EXPECT_FALSE(task.done()); - held->Reset(); - ASSERT_NO_FATAL_FAILURE(Drain(task)); - ExpectValue(std::move(task).TakeResult()); - EXPECT_FALSE(HasHelper(read)); -} - TEST_F(LoadValueAwaitableTestPeer, PopulationChangeDuringBufferWaitSkipsIo) { auto held = Pool().AcquireReadBuffer().await_resume(); ASSERT_TRUE(held.ok()); @@ -379,14 +324,13 @@ TEST_F(LoadValueAwaitableTestPeer, RelocationDuringBufferWaitUsesNewLocation) { EXPECT_EQ(Pool().available_read_buffers(), 1); } -TEST_F(LoadValueAwaitableTestPeer, ExtentReadWaitsAndRoutesLocallyWithoutTask) { +TEST_F(LoadValueAwaitableTestPeer, ExtentWaitRetainsPinAndReturnsSlice) { ASSERT_NO_FATAL_FAILURE(InstallExtent()); auto held = Pool().AcquireReadBuffer().await_resume(); ASSERT_TRUE(held.ok()); - std::vector output(value_.size()); + std::array output{}; { - auto read = ExtentOnOwner(ExtentReference(), output); - EXPECT_FALSE(HasRemote(read)); + auto read = ExtentSlice(ExtentReference(), 2, output); EXPECT_FALSE(read.await_ready()); EXPECT_EQ(block_.pins_, 1); auto task = AwaitExtent(read); @@ -398,7 +342,7 @@ TEST_F(LoadValueAwaitableTestPeer, ExtentReadWaitsAndRoutesLocallyWithoutTask) { EXPECT_TRUE(status.ok()) << status; EXPECT_EQ(std::string_view(reinterpret_cast(output.data()), output.size()), - value_); + std::string_view(value_).substr(2, output.size())); } EXPECT_EQ(block_.pins_, 0); EXPECT_EQ(Pool().available_read_buffers(), 1); @@ -422,20 +366,6 @@ TEST_F(LoadValueAwaitableTestPeer, ExtentSliceChecksWholePayloadBeforeCopying) { EXPECT_EQ(Pool().available_read_buffers(), 1); } -TEST_F(LoadValueAwaitableTestPeer, ExtentSliceReturnsRequestedBytes) { - ASSERT_NO_FATAL_FAILURE(InstallExtent()); - std::array output{}; - auto read = ExtentSlice(ExtentReference(), 2, output); - auto task = AwaitExtent(read); - Start(task); - ASSERT_NO_FATAL_FAILURE(Drain(task)); - auto status = std::move(task).TakeResult(); - EXPECT_TRUE(status.ok()) << status; - EXPECT_EQ(std::string_view(reinterpret_cast(output.data()), - output.size()), - std::string_view(value_).substr(2, output.size())); -} - TEST_F(LoadValueAwaitableTestPeer, CompletionTransfersLeaseAndReleasesPin) { std::optional result; { @@ -453,16 +383,27 @@ TEST_F(LoadValueAwaitableTestPeer, CompletionTransfersLeaseAndReleasesPin) { TEST_F(LoadValueAwaitableTestPeer, PopulationChangesDiscardCompletedRead) { for (bool database : {false, true}) { - { - auto read = Read(); - ASSERT_FALSE(read.await_ready()); - ASSERT_NO_FATAL_FAILURE(Complete(read)); - Invalidate(database); - EXPECT_TRUE(absl::IsNotFound(read.await_resume().status())); + // Exercise valid/mismatched metadata at completion and the scheduling gap. + for (int timing : {0, 1, 2}) { + SCOPED_TRACE(testing::Message() + << "database=" << database << " timing=" << timing); + { + auto read = Read(); + if (timing != 2) Invalidate(database); + if (timing == 1) ++header_.allocation_epoch_; + ASSERT_NO_FATAL_FAILURE(Complete(read)); + if (timing == 1) --header_.allocation_epoch_; + if (timing == 2) Invalidate(database); + EXPECT_FALSE(HasHelper(read)); + EXPECT_TRUE(absl::IsNotFound(read.await_resume().status())); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); + if (database) + ++header_.db_epoch_; + else + ++header_.replication_epoch_; } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); - ++header_.replication_epoch_; } } From 6af2d397d9503e2eed664a7575e3d54423191664 Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Mon, 12 Oct 2026 04:58:06 +0000 Subject: [PATCH 5/6] Split physical value reads from logical retry state --- docs/architecture/04-storage-and-recovery.md | 27 +- src/storage/engine/impl.h | 57 +++-- src/storage/engine/read.cpp | 248 +++++++++++++------ tests/load_value_awaitable_test.cpp | 45 +++- 4 files changed, 253 insertions(+), 124 deletions(-) diff --git a/docs/architecture/04-storage-and-recovery.md b/docs/architecture/04-storage-and-recovery.md index 56a06b7e7..f3edd74bf 100644 --- a/docs/architecture/04-storage-and-recovery.md +++ b/docs/architecture/04-storage-and-recovery.md @@ -649,17 +649,22 @@ rereads. Transaction-owned reads always use that locked path directly. Tombstones and expired values are invisible; an expired observation can enqueue a bounded active-expiration candidate. A staged location is copied or framed from its write buffer. A disk location is read on its physical block owner into -an aligned lease. `LoadValue` and its single-location `LoadValueLocal` share an -awaitable embedded in the caller's task. Local inline reads, including staged -copies, use that task directly. Buffer contention retains the pool's FIFO -handoff and schedules an owner-local continuation under the caller's task class; -it does not require a waiter coroutine. The awaitable owns the disk-read pin -and buffer through completion and validates record identity, checksums, and -population before returning it. Grouped reads enter their graph reader directly; -cross-owner, external, and relocation flows use lazy helper tasks. A stale -logical read resolves the current same-mutation location before resuming its -caller, while a single-location read returns the stale status to its enclosing -representation-specific retry loop. +an aligned lease. `LoadValue` and its single-location `LoadValueLocal` use +separate awaitable types embedded in the caller's task, sharing physical I/O +and validation code. Local inline reads, including staged copies, use that +task directly. Buffer contention retains the pool's FIFO handoff and schedules +an owner-local continuation under the caller's task class; it does not require +a waiter coroutine. Each awaitable owns its disk-read pin and buffer through +completion. + +Ordinary owner-local GET uses the single-location reader. Its I/O completion +only wakes the caller; the resumed task validates the record and fences the +population, entering logical relocation resolution only on a stale-location +error. Candidate replica populations and other representations use the full +logical loader, which resolves same-mutation relocations before returning to +its caller. Representation-specific readers use the single-location result +inside their own retry loops. Grouped reads enter their graph reader directly; +cross-owner, external, and relocation flows use lazy helper tasks. Worker-local MGET uses `BatchGetLocked` after the command has acquired its key locks. It classifies index entries in one coroutine and issues ordinary-size diff --git a/src/storage/engine/impl.h b/src/storage/engine/impl.h index 83f39e248..acf06b35b 100644 --- a/src/storage/engine/impl.h +++ b/src/storage/engine/impl.h @@ -3343,28 +3343,34 @@ class StorageEngine::Impl { } }; - // The awaitable lives in the caller's frame. Ordinary local reads suspend - // that caller directly, including through pool backpressure. Only multi-step - // logical reads need a lazy helper Task. Local mode returns stale-location - // errors to its caller, while logical mode resolves same-mutation relocations - // before returning. - class LoadValueAwaitable final : public bycorf::OneShotIoAwaitable, + // Both types share physical I/O and pool handoff, but single-location reads + // have no logical dispatch/retry state or completion-side parsing. Their + // caller fences the population and decides whether to retry a stale location. + template + class ValueReadAwaitable final : public bycorf::OneShotIoAwaitable, private bycorf::RemoteWork { public: using Result = absl::StatusOr; - LoadValueAwaitable(Impl& engine, WorkerStore& store, + ValueReadAwaitable(Impl& engine, WorkerStore& store, WorkerStore::PartitionStore& partition, std::uint8_t db_id, std::string_view key, const Digest& digest, RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, - GroupedObject::Handle grouped_snapshot); - LoadValueAwaitable(Impl& engine, WorkerStore& store, std::uint8_t db_id, + GroupedObject::Handle grouped_snapshot) + requires(Logical); + ValueReadAwaitable(Impl& engine, WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, - std::optional expected_db_epoch); - ~LoadValueAwaitable(); - - bool await_ready() const noexcept { return ready_ && !slow_.valid(); } + std::optional expected_db_epoch) + requires(!Logical); + ~ValueReadAwaitable(); + + bool await_ready() const noexcept { + if constexpr (Logical) + return ready_ && !logical_.slow_.valid(); + else + return ready_.has_value(); + } std::coroutine_handle<> await_suspend(std::coroutine_handle<> awaiting); Result await_resume(); void Complete(bycorf::Worker& worker, int result, unsigned flags) override; @@ -3384,15 +3390,25 @@ class StorageEngine::Impl { std::coroutine_handle<> SuspendRead(std::coroutine_handle<> awaiting); absl::StatusOr ParseRecord(const std::byte* data) const; Result FinishRecord(const std::byte* data, Payload payload, bool staged); - bool PopulationChanged() const; + bool PopulationChanged() const + requires(Logical); void Unpin(); - void Retry(absl::Status failure); + void Retry(absl::Status failure) + requires(Logical); + + struct LogicalState { + WorkerStore::PartitionStore* partition_ = nullptr; + Digest digest_{}; + std::optional payload_; + Task slow_; + }; + struct PhysicalState {}; + [[no_unique_address]] std::conditional_t logical_; Impl& engine_; WorkerStore& store_; - WorkerStore::PartitionStore* partition_ = nullptr; std::string_view key_; - Digest digest_{}; RecordLocation location_; ReadLatencyTrace* trace_; std::uint64_t replication_epoch_; @@ -3403,15 +3419,16 @@ class StorageEngine::Impl { std::size_t read_bytes_ = 0; std::uint8_t db_id_; BlockState* pinned_ = nullptr; - std::optional payload_; std::optional ready_; // Runtime shutdown drains I/O before destroying suspended frames. The // waiter retains its assigned slot until the scheduled continuation runs. ReadBufferLease lease_; std::optional buffer_wait_; - Task slow_; }; + using LoadValueAwaitable = ValueReadAwaitable; + using LoadValueLocalAwaitable = ValueReadAwaitable; + // One physical extent read, including buffer waiting and slice validation, // embedded in the caller. Routing adds a runtime task bridge only when the // block belongs to another worker; local reads need no child coroutine. @@ -3888,7 +3905,7 @@ class StorageEngine::Impl { ExtentManifest extents, ReadLatencyTrace* trace, bool grouped_payload = false); - LoadValueAwaitable LoadValueLocal( + LoadValueLocalAwaitable LoadValueLocal( WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace = nullptr, diff --git a/src/storage/engine/read.cpp b/src/storage/engine/read.cpp index 6d9401122..c00fc1aad 100644 --- a/src/storage/engine/read.cpp +++ b/src/storage/engine/read.cpp @@ -306,9 +306,43 @@ Task> StorageEngine::Impl::GetWithLockState( trace->lookup_done_ns_ = ReadTraceNowNanos(); } - auto loaded = co_await LoadValue(store, partition, db_id, key, digest, - MaterializeIndexLocation(*found), - ExtentsFor(store, found), trace); + const auto location = MaterializeIndexLocation(*found); + const auto replication_epoch = partition.replication_epoch_; + const bool candidate = partition.replica_sync_ != nullptr; + const auto db_epoch = + candidate ? EffectiveRecordDbEpoch(partition, db_id) : DbEpoch(db_id); + auto population_changed = [&] { + // Starting a candidate changes the partition epoch, so a read admitted + // outside recovery needs only the served DB epoch on its success path. + return partition.replication_epoch_ != replication_epoch || + (candidate ? EffectiveRecordDbEpoch(partition, db_id) + : DbEpoch(db_id)) != db_epoch; + }; + absl::StatusOr loaded; + if (!location.grouped() && !location.external() && + location.block_owner() == store.worker_->id() && !candidate) { + // This single-location type has no logical dispatcher or completion-side + // parsing. The temporary releases its pin before an exceptional retry. + loaded = co_await LoadValueLocal(store, db_id, key, location, + replication_epoch, trace, db_epoch); + if (absl::IsAborted(loaded.status()) && !population_changed()) + [[unlikely]] { + loaded = co_await LoadValueSlow(store, partition, db_id, key, digest, + location, {}, trace, loaded.status()); + } + } else if (location.grouped()) { + loaded = co_await LoadGroupedValue(store, partition, db_id, key, digest, + location, nullptr); + } else { + // Only cold representations create a task. Keeping the full logical + // awaitable out of this branch also keeps its state out of GET's frame. + loaded = + co_await LoadValueSlow(store, partition, db_id, key, digest, location, + ExtentsFor(store, found), trace); + } + // FLUSHDB/reset can run during I/O or any multi-step representation/retry. + if (population_changed()) [[unlikely]] + loaded = absl::NotFoundError("key not found"); if (optimistic_read && !loaded.ok() && (loaded.status().code() == absl::StatusCode::kNotFound || loaded.status().code() == absl::StatusCode::kAborted)) { @@ -1332,25 +1366,29 @@ StorageEngine::Impl::LoadValueAwaitable StorageEngine::Impl::LoadValue( std::move(grouped_snapshot)); } -StorageEngine::Impl::LoadValueAwaitable StorageEngine::Impl::LoadValueLocal( +StorageEngine::Impl::LoadValueLocalAwaitable +StorageEngine::Impl::LoadValueLocal( WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, std::optional expected_db_epoch) { - return LoadValueAwaitable(*this, store, db_id, key, location, - replication_epoch, trace, expected_db_epoch); + return LoadValueLocalAwaitable(*this, store, db_id, key, location, + replication_epoch, trace, expected_db_epoch); } -StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( +template +StorageEngine::Impl::ValueReadAwaitable::ValueReadAwaitable( Impl& engine, WorkerStore& store, WorkerStore::PartitionStore& partition, std::uint8_t db_id, std::string_view key, const Digest& digest, RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, GroupedObject::Handle grouped_snapshot) - : engine_(engine), + requires(Logical) + : logical_{.partition_ = location.grouped() ? nullptr : &partition, + .digest_ = digest, + .payload_ = {}, + .slow_ = {}}, + engine_(engine), store_(store), - // Grouped snapshots have their own population/pin protocol. - partition_(location.grouped() ? nullptr : &partition), key_(key), - digest_(digest), location_(location), trace_(trace), replication_epoch_(partition.replication_epoch_), @@ -1359,22 +1397,25 @@ StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( if (location.grouped()) { // The grouped task owns its graph/version protocol. Keep its borrowed // digest in this awaitable, not in the constructor's argument lifetime. - slow_ = engine.LoadGroupedValue(store, partition, db_id, key, digest_, - location, std::move(grouped_snapshot)); + logical_.slow_ = + engine.LoadGroupedValue(store, partition, db_id, key, logical_.digest_, + location, std::move(grouped_snapshot)); } else if (location.external() || location.block_owner() != store.worker_->id()) { - slow_ = engine.LoadValueSlow(store, partition, db_id, key, digest, location, - std::move(extents), trace); + logical_.slow_ = engine.LoadValueSlow(store, partition, db_id, key, digest, + location, std::move(extents), trace); } else { PrepareLocal(); if (ready_ && absl::IsAborted(ready_->status())) Retry(ready_->status()); } } -StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( +template +StorageEngine::Impl::ValueReadAwaitable::ValueReadAwaitable( Impl& engine, WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, std::optional expected_db_epoch) + requires(!Logical) : engine_(engine), store_(store), key_(key), @@ -1386,9 +1427,13 @@ StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( PrepareLocal(); } -StorageEngine::Impl::LoadValueAwaitable::~LoadValueAwaitable() { Unpin(); } +template +StorageEngine::Impl::ValueReadAwaitable::~ValueReadAwaitable() { + Unpin(); +} -void StorageEngine::Impl::LoadValueAwaitable::Unpin() { +template +void StorageEngine::Impl::ValueReadAwaitable::Unpin() { auto* state = std::exchange(pinned_, nullptr); if (!state) return; --state->pins_; @@ -1396,7 +1441,8 @@ void StorageEngine::Impl::LoadValueAwaitable::Unpin() { ReleaseStagingBuffer(store_, *state); } -void StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { +template +void StorageEngine::Impl::ValueReadAwaitable::PrepareLocal() { LAVIK_FAULT_INJECT(if (LAVIK_FAULT_MATCHES("LAVIK_FAIL_VALUE_READ_KEY", key_)) { ready_.emplace(absl::InternalError("injected value payload read failure")); @@ -1473,48 +1519,67 @@ void StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { if (trace_) trace_->disk_read_ = true; } -bool StorageEngine::Impl::LoadValueAwaitable::PopulationChanged() const { - return partition_ && (partition_->replication_epoch_ != replication_epoch_ || - engine_.EffectiveRecordDbEpoch(*partition_, db_id_) != - *expected_db_epoch_); +template +bool StorageEngine::Impl::ValueReadAwaitable::PopulationChanged() const + requires(Logical) +{ + return logical_.partition_ && + (logical_.partition_->replication_epoch_ != replication_epoch_ || + engine_.EffectiveRecordDbEpoch(*logical_.partition_, db_id_) != + *expected_db_epoch_); } -void StorageEngine::Impl::LoadValueAwaitable::Retry(absl::Status failure) { - assert(partition_ && absl::IsAborted(failure)); +template +void StorageEngine::Impl::ValueReadAwaitable::Retry( + absl::Status failure) + requires(Logical) +{ + assert(logical_.partition_ && absl::IsAborted(failure)); Unpin(); lease_.Reset(); ready_.reset(); // Inline reads have no extent manifest. Seed the failure so the slow path // resolves the index before any further I/O and preserves corruption errors // when the index still endorses the failed physical record. - slow_ = engine_.LoadValueSlow(store_, *partition_, db_id_, key_, digest_, - location_, {}, trace_, std::move(failure)); + logical_.slow_ = engine_.LoadValueSlow(store_, *logical_.partition_, db_id_, + key_, logical_.digest_, location_, {}, + trace_, std::move(failure)); } -void StorageEngine::Impl::LoadValueAwaitable::FinishBufferWait() { +template +void StorageEngine::Impl::ValueReadAwaitable::FinishBufferWait() { auto acquired = buffer_wait_->await_resume(); buffer_wait_.reset(); - if (PopulationChanged()) { - ready_.emplace(absl::NotFoundError("key not found")); - } else if (!acquired.ok()) { + if constexpr (Logical) { + if (PopulationChanged()) { + ready_.emplace(absl::NotFoundError("key not found")); + return; + } + } + if (!acquired.ok()) { ready_.emplace(acquired.status()); } else { lease_ = std::move(*acquired); PrepareLocal(); - if (partition_ && ready_ && absl::IsAborted(ready_->status())) - Retry(ready_->status()); + if constexpr (Logical) { + if (logical_.partition_ && ready_ && absl::IsAborted(ready_->status())) + Retry(ready_->status()); + } } } -void StorageEngine::Impl::LoadValueAwaitable::BufferReady( +template +void StorageEngine::Impl::ValueReadAwaitable::BufferReady( bycorf::RemoteWork* work) { - auto& read = *static_cast(work); + auto& read = *static_cast(work); read.FinishBufferWait(); auto next = read.SuspendRead(read.awaiting_); if (next != std::noop_coroutine()) read.store_.worker_->Enqueue(next); } -std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::await_suspend( +template +std::coroutine_handle<> +StorageEngine::Impl::ValueReadAwaitable::await_suspend( std::coroutine_handle<> awaiting) { if (buffer_wait_) { awaiting_ = awaiting; @@ -1528,10 +1593,16 @@ std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::await_suspend( return SuspendRead(awaiting); } -std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::SuspendRead( +template +std::coroutine_handle<> +StorageEngine::Impl::ValueReadAwaitable::SuspendRead( std::coroutine_handle<> awaiting) { - if (slow_.valid()) - return std::move(slow_).operator co_await().await_suspend(awaiting); + if constexpr (Logical) { + if (logical_.slow_.valid()) + return std::move(logical_.slow_) + .operator co_await() + .await_suspend(awaiting); + } if (ready_) return awaiting; auto buffer = lease_.io_buffer(); buffer.size_ = read_bytes_; @@ -1545,47 +1616,56 @@ std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::SuspendRead( aligned_offset_, this); if (Suspend(awaiting, status)) return std::noop_coroutine(); if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); - if (partition_ && absl::IsAborted(status)) { - Retry(std::move(status)); - return std::move(slow_).operator co_await().await_suspend(awaiting); + if constexpr (Logical) { + if (logical_.partition_ && absl::IsAborted(status)) { + Retry(std::move(status)); + return std::move(logical_.slow_) + .operator co_await() + .await_suspend(awaiting); + } } return awaiting; } -void StorageEngine::Impl::LoadValueAwaitable::Complete(bycorf::Worker& worker, - int bytes, - unsigned flags) { +template +void StorageEngine::Impl::ValueReadAwaitable::Complete( + bycorf::Worker& worker, int bytes, unsigned flags) { if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); if (bytes >= 0) worker.RecordStorageReadCompletion(bytes); - if (partition_ && bytes >= 0 && - static_cast(bytes) == read_bytes_) { - // Only metadata is examined on the completion poller. CRC and large-value - // copies stay in the caller's scheduled slice. A stale identity must select - // the retry continuation before the caller's co_await can finish. - auto parsed = ParseRecord(lease_.io_buffer().data_ + record_headroom_); - if (parsed.ok()) - payload_ = *parsed; - else if (absl::IsAborted(parsed.status()) && !PopulationChanged()) { - Retry(parsed.status()); - auto child = - std::move(slow_).operator co_await().await_suspend(awaiting_); - // CQEs are polled outside the parent's task class. Preserve background - // scheduling when a backup/maintenance read starts its retry here. - if (worker.IsBackground(awaiting_)) worker.RegisterBackground(child); - worker.Enqueue(child); - return; - } else - ready_.emplace(parsed.status()); + if constexpr (Logical) { + if (logical_.partition_ && bytes >= 0 && + static_cast(bytes) == read_bytes_) { + // Only metadata is examined on the completion poller. CRC and large-value + // copies stay in the caller's scheduled slice. A stale identity must + // select the retry continuation before the caller's co_await can finish. + auto parsed = ParseRecord(lease_.io_buffer().data_ + record_headroom_); + if (parsed.ok()) + logical_.payload_ = *parsed; + else if (absl::IsAborted(parsed.status()) && !PopulationChanged()) { + Retry(parsed.status()); + auto child = std::move(logical_.slow_) + .operator co_await() + .await_suspend(awaiting_); + // CQEs are polled outside the parent's task class. Preserve background + // scheduling when a backup/maintenance read starts its retry here. + if (worker.IsBackground(awaiting_)) worker.RegisterBackground(child); + worker.Enqueue(child); + return; + } else + ready_.emplace(parsed.status()); + } } OneShotIoAwaitable::Complete(worker, bytes, flags); } -StorageEngine::Impl::LoadValueAwaitable::Result -StorageEngine::Impl::LoadValueAwaitable::await_resume() { - // Fence successful reads here, after the scheduling gap following the CQE. - // Completion only needs this check when deciding whether to start a retry. - if (PopulationChanged()) return absl::NotFoundError("key not found"); - if (slow_.valid()) return std::move(slow_).TakeResult(); +template +typename StorageEngine::Impl::ValueReadAwaitable::Result +StorageEngine::Impl::ValueReadAwaitable::await_resume() { + if constexpr (Logical) { + // Fence after the scheduling gap; completion only fences before retries. + if (PopulationChanged()) return absl::NotFoundError("key not found"); + if (logical_.slow_.valid()) return std::move(logical_.slow_).TakeResult(); + } if (ready_) return std::move(*ready_); auto read = Resume(lease_.registered() ? "fixed-buffer read failed" : "read failed"); @@ -1593,16 +1673,18 @@ StorageEngine::Impl::LoadValueAwaitable::await_resume() { if (static_cast(*read) != read_bytes_) return absl::InternalError("short compact record read"); const auto* data = lease_.io_buffer().data_ + record_headroom_; - if (!payload_) { - auto parsed = ParseRecord(data); - if (!parsed.ok()) return parsed.status(); - payload_ = *parsed; + if constexpr (Logical) { + if (logical_.payload_) return FinishRecord(data, *logical_.payload_, false); } - return FinishRecord(data, *payload_, false); + auto parsed = ParseRecord(data); + if (!parsed.ok()) return parsed.status(); + return FinishRecord(data, *parsed, false); } -absl::StatusOr -StorageEngine::Impl::LoadValueAwaitable::ParseRecord( +template +absl::StatusOr< + typename StorageEngine::Impl::ValueReadAwaitable::Payload> +StorageEngine::Impl::ValueReadAwaitable::ParseRecord( const std::byte* data) const { RecordHeader record{}; std::string_view disk_key; @@ -1632,10 +1714,10 @@ StorageEngine::Impl::LoadValueAwaitable::ParseRecord( record.payload_checksum_}; } -StorageEngine::Impl::LoadValueAwaitable::Result -StorageEngine::Impl::LoadValueAwaitable::FinishRecord(const std::byte* data, - Payload payload, - bool staged) { +template +typename StorageEngine::Impl::ValueReadAwaitable::Result +StorageEngine::Impl::ValueReadAwaitable::FinishRecord( + const std::byte* data, Payload payload, bool staged) { const auto* value = data + payload.offset_; if (Crc32c(std::span(value, payload.bytes_)) != payload.checksum_) return absl::InternalError("record value checksum mismatch"); @@ -1648,4 +1730,8 @@ StorageEngine::Impl::LoadValueAwaitable::FinishRecord(const std::byte* data, return LoadedValue{std::move(lease_), offset, payload.bytes_, payload.txid_}; } +// Keep the shared implementation out of every loader caller's translation unit. +template class StorageEngine::Impl::ValueReadAwaitable; +template class StorageEngine::Impl::ValueReadAwaitable; + } // namespace lavik::storage diff --git a/tests/load_value_awaitable_test.cpp b/tests/load_value_awaitable_test.cpp index d5e1a4302..0726c3230 100644 --- a/tests/load_value_awaitable_test.cpp +++ b/tests/load_value_awaitable_test.cpp @@ -17,6 +17,8 @@ namespace lavik::storage { class LoadValueAwaitableTestPeer : public testing::Test { protected: using Awaitable = StorageEngine::Impl::LoadValueAwaitable; + using LocalAwaitable = StorageEngine::Impl::LoadValueLocalAwaitable; + static_assert(sizeof(LocalAwaitable) < sizeof(Awaitable)); using Result = Awaitable::Result; using ExtentRead = StorageEngine::Impl::ReadExtentAwaitable; @@ -84,18 +86,22 @@ class LoadValueAwaitableTestPeer : public testing::Test { auto ReadLocal() { return storage_.impl_->LoadValueLocal(Store(), 0, key_, location_, 1); } - bool HasHelper(const Awaitable& read) { return read.slow_.valid(); } + bool HasHelper(const Awaitable& read) { return read.logical_.slow_.valid(); } void Encode(std::byte* data) { ASSERT_TRUE(EncodeRecordHeader(header_, key_, std::span(data, header_.header_bytes_))); std::memcpy(data + header_.header_bytes_, value_.data(), value_.size()); } - void Complete(Awaitable& read, int bytes = -1) { + template + void Complete(Read& read, int bytes = -1) { ASSERT_NE(read.pinned_, nullptr); Encode(read.lease_.io_buffer().data_ + read.record_headroom_); read.Complete(worker_, bytes == -1 ? read.read_bytes_ : bytes, 0); } + void RewriteRecord(LocalAwaitable& read) { + Encode(read.lease_.io_buffer().data_ + read.record_headroom_); + } void Stage() { location_.set_in_memory(true); block_.in_memory_ = true; @@ -152,9 +158,13 @@ class LoadValueAwaitableTestPeer : public testing::Test { co_await Arm{read}; co_return read.await_resume(); } - Task AwaitExisting(Awaitable& read) { co_return co_await read; } + template + Task AwaitExisting(Read& read) { + co_return co_await read; + } Task AwaitExtent(ExtentRead& read) { co_return co_await read; } - bool BufferWaitIsBackground(Awaitable& read) { + template + bool BufferWaitIsBackground(Read& read) { return read.task_class_ == bycorf::TaskClass::kBackground; } auto ExtentSlice(ExtentRef ref, std::size_t offset, @@ -244,7 +254,7 @@ class LoadValueAwaitableTestPeer : public testing::Test { } bool HelperIsBackground(Awaitable& read) { return worker_.IsBackground( - std::move(read.slow_).operator co_await().handle_); + std::move(read.logical_.slow_).operator co_await().handle_); } bycorf::CurrentWorker saved_context_; @@ -264,7 +274,7 @@ TEST_F(LoadValueAwaitableTestPeer, BufferWaitersPreserveFifoHandoff) { ASSERT_NO_FATAL_FAILURE(InstallRecord()); auto held = Pool().AcquireReadBuffer().await_resume(); ASSERT_TRUE(held.ok()); - auto first = Read(); + auto first = ReadLocal(); auto second = Read(); auto first_task = AwaitExisting(first); auto second_task = AwaitExisting(second); @@ -282,7 +292,6 @@ TEST_F(LoadValueAwaitableTestPeer, BufferWaitersPreserveFifoHandoff) { ExpectValue(std::move(first_task).TakeResult()); ASSERT_NO_FATAL_FAILURE(Drain(second_task)); ExpectValue(std::move(second_task).TakeResult()); - EXPECT_FALSE(HasHelper(first)); EXPECT_FALSE(HasHelper(second)); EXPECT_EQ(Pool().available_read_buffers(), 1); } @@ -431,11 +440,23 @@ TEST_F(LoadValueAwaitableTestPeer, ErrorsReleaseResources) { } TEST_F(LoadValueAwaitableTestPeer, LocalReadReturnsStaleLocationToItsCaller) { - block_.freeing_ = true; - auto read = ReadLocal(); - ASSERT_TRUE(read.await_ready()); - EXPECT_FALSE(HasHelper(read)); - EXPECT_TRUE(absl::IsAborted(read.await_resume().status())); + { + block_.freeing_ = true; + auto read = ReadLocal(); + ASSERT_TRUE(read.await_ready()); + EXPECT_TRUE(absl::IsAborted(read.await_resume().status())); + } + block_.freeing_ = false; + { + auto read = ReadLocal(); + ASSERT_NO_FATAL_FAILURE(Complete(read)); + // Physical reads parse in the caller, not on the completion poller. + ++header_.allocation_epoch_; + RewriteRecord(read); + EXPECT_TRUE(absl::IsAborted(read.await_resume().status())); + } + EXPECT_EQ(block_.pins_, 0); + EXPECT_EQ(Pool().available_read_buffers(), 1); } TEST_F(LoadValueAwaitableTestPeer, From bac1d2d3593b9a5711538f2316f30571107139a8 Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Mon, 12 Oct 2026 05:12:35 +0000 Subject: [PATCH 6/6] Revert "Split physical value reads from logical retry state" This reverts commit 6af2d397d9503e2eed664a7575e3d54423191664. --- docs/architecture/04-storage-and-recovery.md | 27 +- src/storage/engine/impl.h | 57 ++--- src/storage/engine/read.cpp | 248 ++++++------------- tests/load_value_awaitable_test.cpp | 45 +--- 4 files changed, 124 insertions(+), 253 deletions(-) diff --git a/docs/architecture/04-storage-and-recovery.md b/docs/architecture/04-storage-and-recovery.md index f3edd74bf..56a06b7e7 100644 --- a/docs/architecture/04-storage-and-recovery.md +++ b/docs/architecture/04-storage-and-recovery.md @@ -649,22 +649,17 @@ rereads. Transaction-owned reads always use that locked path directly. Tombstones and expired values are invisible; an expired observation can enqueue a bounded active-expiration candidate. A staged location is copied or framed from its write buffer. A disk location is read on its physical block owner into -an aligned lease. `LoadValue` and its single-location `LoadValueLocal` use -separate awaitable types embedded in the caller's task, sharing physical I/O -and validation code. Local inline reads, including staged copies, use that -task directly. Buffer contention retains the pool's FIFO handoff and schedules -an owner-local continuation under the caller's task class; it does not require -a waiter coroutine. Each awaitable owns its disk-read pin and buffer through -completion. - -Ordinary owner-local GET uses the single-location reader. Its I/O completion -only wakes the caller; the resumed task validates the record and fences the -population, entering logical relocation resolution only on a stale-location -error. Candidate replica populations and other representations use the full -logical loader, which resolves same-mutation relocations before returning to -its caller. Representation-specific readers use the single-location result -inside their own retry loops. Grouped reads enter their graph reader directly; -cross-owner, external, and relocation flows use lazy helper tasks. +an aligned lease. `LoadValue` and its single-location `LoadValueLocal` share an +awaitable embedded in the caller's task. Local inline reads, including staged +copies, use that task directly. Buffer contention retains the pool's FIFO +handoff and schedules an owner-local continuation under the caller's task class; +it does not require a waiter coroutine. The awaitable owns the disk-read pin +and buffer through completion and validates record identity, checksums, and +population before returning it. Grouped reads enter their graph reader directly; +cross-owner, external, and relocation flows use lazy helper tasks. A stale +logical read resolves the current same-mutation location before resuming its +caller, while a single-location read returns the stale status to its enclosing +representation-specific retry loop. Worker-local MGET uses `BatchGetLocked` after the command has acquired its key locks. It classifies index entries in one coroutine and issues ordinary-size diff --git a/src/storage/engine/impl.h b/src/storage/engine/impl.h index acf06b35b..83f39e248 100644 --- a/src/storage/engine/impl.h +++ b/src/storage/engine/impl.h @@ -3343,34 +3343,28 @@ class StorageEngine::Impl { } }; - // Both types share physical I/O and pool handoff, but single-location reads - // have no logical dispatch/retry state or completion-side parsing. Their - // caller fences the population and decides whether to retry a stale location. - template - class ValueReadAwaitable final : public bycorf::OneShotIoAwaitable, + // The awaitable lives in the caller's frame. Ordinary local reads suspend + // that caller directly, including through pool backpressure. Only multi-step + // logical reads need a lazy helper Task. Local mode returns stale-location + // errors to its caller, while logical mode resolves same-mutation relocations + // before returning. + class LoadValueAwaitable final : public bycorf::OneShotIoAwaitable, private bycorf::RemoteWork { public: using Result = absl::StatusOr; - ValueReadAwaitable(Impl& engine, WorkerStore& store, + LoadValueAwaitable(Impl& engine, WorkerStore& store, WorkerStore::PartitionStore& partition, std::uint8_t db_id, std::string_view key, const Digest& digest, RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, - GroupedObject::Handle grouped_snapshot) - requires(Logical); - ValueReadAwaitable(Impl& engine, WorkerStore& store, std::uint8_t db_id, + GroupedObject::Handle grouped_snapshot); + LoadValueAwaitable(Impl& engine, WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, - std::optional expected_db_epoch) - requires(!Logical); - ~ValueReadAwaitable(); - - bool await_ready() const noexcept { - if constexpr (Logical) - return ready_ && !logical_.slow_.valid(); - else - return ready_.has_value(); - } + std::optional expected_db_epoch); + ~LoadValueAwaitable(); + + bool await_ready() const noexcept { return ready_ && !slow_.valid(); } std::coroutine_handle<> await_suspend(std::coroutine_handle<> awaiting); Result await_resume(); void Complete(bycorf::Worker& worker, int result, unsigned flags) override; @@ -3390,25 +3384,15 @@ class StorageEngine::Impl { std::coroutine_handle<> SuspendRead(std::coroutine_handle<> awaiting); absl::StatusOr ParseRecord(const std::byte* data) const; Result FinishRecord(const std::byte* data, Payload payload, bool staged); - bool PopulationChanged() const - requires(Logical); + bool PopulationChanged() const; void Unpin(); - void Retry(absl::Status failure) - requires(Logical); - - struct LogicalState { - WorkerStore::PartitionStore* partition_ = nullptr; - Digest digest_{}; - std::optional payload_; - Task slow_; - }; - struct PhysicalState {}; - [[no_unique_address]] std::conditional_t logical_; + void Retry(absl::Status failure); Impl& engine_; WorkerStore& store_; + WorkerStore::PartitionStore* partition_ = nullptr; std::string_view key_; + Digest digest_{}; RecordLocation location_; ReadLatencyTrace* trace_; std::uint64_t replication_epoch_; @@ -3419,16 +3403,15 @@ class StorageEngine::Impl { std::size_t read_bytes_ = 0; std::uint8_t db_id_; BlockState* pinned_ = nullptr; + std::optional payload_; std::optional ready_; // Runtime shutdown drains I/O before destroying suspended frames. The // waiter retains its assigned slot until the scheduled continuation runs. ReadBufferLease lease_; std::optional buffer_wait_; + Task slow_; }; - using LoadValueAwaitable = ValueReadAwaitable; - using LoadValueLocalAwaitable = ValueReadAwaitable; - // One physical extent read, including buffer waiting and slice validation, // embedded in the caller. Routing adds a runtime task bridge only when the // block belongs to another worker; local reads need no child coroutine. @@ -3905,7 +3888,7 @@ class StorageEngine::Impl { ExtentManifest extents, ReadLatencyTrace* trace, bool grouped_payload = false); - LoadValueLocalAwaitable LoadValueLocal( + LoadValueAwaitable LoadValueLocal( WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace = nullptr, diff --git a/src/storage/engine/read.cpp b/src/storage/engine/read.cpp index c00fc1aad..6d9401122 100644 --- a/src/storage/engine/read.cpp +++ b/src/storage/engine/read.cpp @@ -306,43 +306,9 @@ Task> StorageEngine::Impl::GetWithLockState( trace->lookup_done_ns_ = ReadTraceNowNanos(); } - const auto location = MaterializeIndexLocation(*found); - const auto replication_epoch = partition.replication_epoch_; - const bool candidate = partition.replica_sync_ != nullptr; - const auto db_epoch = - candidate ? EffectiveRecordDbEpoch(partition, db_id) : DbEpoch(db_id); - auto population_changed = [&] { - // Starting a candidate changes the partition epoch, so a read admitted - // outside recovery needs only the served DB epoch on its success path. - return partition.replication_epoch_ != replication_epoch || - (candidate ? EffectiveRecordDbEpoch(partition, db_id) - : DbEpoch(db_id)) != db_epoch; - }; - absl::StatusOr loaded; - if (!location.grouped() && !location.external() && - location.block_owner() == store.worker_->id() && !candidate) { - // This single-location type has no logical dispatcher or completion-side - // parsing. The temporary releases its pin before an exceptional retry. - loaded = co_await LoadValueLocal(store, db_id, key, location, - replication_epoch, trace, db_epoch); - if (absl::IsAborted(loaded.status()) && !population_changed()) - [[unlikely]] { - loaded = co_await LoadValueSlow(store, partition, db_id, key, digest, - location, {}, trace, loaded.status()); - } - } else if (location.grouped()) { - loaded = co_await LoadGroupedValue(store, partition, db_id, key, digest, - location, nullptr); - } else { - // Only cold representations create a task. Keeping the full logical - // awaitable out of this branch also keeps its state out of GET's frame. - loaded = - co_await LoadValueSlow(store, partition, db_id, key, digest, location, - ExtentsFor(store, found), trace); - } - // FLUSHDB/reset can run during I/O or any multi-step representation/retry. - if (population_changed()) [[unlikely]] - loaded = absl::NotFoundError("key not found"); + auto loaded = co_await LoadValue(store, partition, db_id, key, digest, + MaterializeIndexLocation(*found), + ExtentsFor(store, found), trace); if (optimistic_read && !loaded.ok() && (loaded.status().code() == absl::StatusCode::kNotFound || loaded.status().code() == absl::StatusCode::kAborted)) { @@ -1366,29 +1332,25 @@ StorageEngine::Impl::LoadValueAwaitable StorageEngine::Impl::LoadValue( std::move(grouped_snapshot)); } -StorageEngine::Impl::LoadValueLocalAwaitable -StorageEngine::Impl::LoadValueLocal( +StorageEngine::Impl::LoadValueAwaitable StorageEngine::Impl::LoadValueLocal( WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, std::optional expected_db_epoch) { - return LoadValueLocalAwaitable(*this, store, db_id, key, location, - replication_epoch, trace, expected_db_epoch); + return LoadValueAwaitable(*this, store, db_id, key, location, + replication_epoch, trace, expected_db_epoch); } -template -StorageEngine::Impl::ValueReadAwaitable::ValueReadAwaitable( +StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( Impl& engine, WorkerStore& store, WorkerStore::PartitionStore& partition, std::uint8_t db_id, std::string_view key, const Digest& digest, RecordLocation location, ExtentManifest extents, ReadLatencyTrace* trace, GroupedObject::Handle grouped_snapshot) - requires(Logical) - : logical_{.partition_ = location.grouped() ? nullptr : &partition, - .digest_ = digest, - .payload_ = {}, - .slow_ = {}}, - engine_(engine), + : engine_(engine), store_(store), + // Grouped snapshots have their own population/pin protocol. + partition_(location.grouped() ? nullptr : &partition), key_(key), + digest_(digest), location_(location), trace_(trace), replication_epoch_(partition.replication_epoch_), @@ -1397,25 +1359,22 @@ StorageEngine::Impl::ValueReadAwaitable::ValueReadAwaitable( if (location.grouped()) { // The grouped task owns its graph/version protocol. Keep its borrowed // digest in this awaitable, not in the constructor's argument lifetime. - logical_.slow_ = - engine.LoadGroupedValue(store, partition, db_id, key, logical_.digest_, - location, std::move(grouped_snapshot)); + slow_ = engine.LoadGroupedValue(store, partition, db_id, key, digest_, + location, std::move(grouped_snapshot)); } else if (location.external() || location.block_owner() != store.worker_->id()) { - logical_.slow_ = engine.LoadValueSlow(store, partition, db_id, key, digest, - location, std::move(extents), trace); + slow_ = engine.LoadValueSlow(store, partition, db_id, key, digest, location, + std::move(extents), trace); } else { PrepareLocal(); if (ready_ && absl::IsAborted(ready_->status())) Retry(ready_->status()); } } -template -StorageEngine::Impl::ValueReadAwaitable::ValueReadAwaitable( +StorageEngine::Impl::LoadValueAwaitable::LoadValueAwaitable( Impl& engine, WorkerStore& store, std::uint8_t db_id, std::string_view key, RecordLocation location, std::uint64_t replication_epoch, ReadLatencyTrace* trace, std::optional expected_db_epoch) - requires(!Logical) : engine_(engine), store_(store), key_(key), @@ -1427,13 +1386,9 @@ StorageEngine::Impl::ValueReadAwaitable::ValueReadAwaitable( PrepareLocal(); } -template -StorageEngine::Impl::ValueReadAwaitable::~ValueReadAwaitable() { - Unpin(); -} +StorageEngine::Impl::LoadValueAwaitable::~LoadValueAwaitable() { Unpin(); } -template -void StorageEngine::Impl::ValueReadAwaitable::Unpin() { +void StorageEngine::Impl::LoadValueAwaitable::Unpin() { auto* state = std::exchange(pinned_, nullptr); if (!state) return; --state->pins_; @@ -1441,8 +1396,7 @@ void StorageEngine::Impl::ValueReadAwaitable::Unpin() { ReleaseStagingBuffer(store_, *state); } -template -void StorageEngine::Impl::ValueReadAwaitable::PrepareLocal() { +void StorageEngine::Impl::LoadValueAwaitable::PrepareLocal() { LAVIK_FAULT_INJECT(if (LAVIK_FAULT_MATCHES("LAVIK_FAIL_VALUE_READ_KEY", key_)) { ready_.emplace(absl::InternalError("injected value payload read failure")); @@ -1519,67 +1473,48 @@ void StorageEngine::Impl::ValueReadAwaitable::PrepareLocal() { if (trace_) trace_->disk_read_ = true; } -template -bool StorageEngine::Impl::ValueReadAwaitable::PopulationChanged() const - requires(Logical) -{ - return logical_.partition_ && - (logical_.partition_->replication_epoch_ != replication_epoch_ || - engine_.EffectiveRecordDbEpoch(*logical_.partition_, db_id_) != - *expected_db_epoch_); +bool StorageEngine::Impl::LoadValueAwaitable::PopulationChanged() const { + return partition_ && (partition_->replication_epoch_ != replication_epoch_ || + engine_.EffectiveRecordDbEpoch(*partition_, db_id_) != + *expected_db_epoch_); } -template -void StorageEngine::Impl::ValueReadAwaitable::Retry( - absl::Status failure) - requires(Logical) -{ - assert(logical_.partition_ && absl::IsAborted(failure)); +void StorageEngine::Impl::LoadValueAwaitable::Retry(absl::Status failure) { + assert(partition_ && absl::IsAborted(failure)); Unpin(); lease_.Reset(); ready_.reset(); // Inline reads have no extent manifest. Seed the failure so the slow path // resolves the index before any further I/O and preserves corruption errors // when the index still endorses the failed physical record. - logical_.slow_ = engine_.LoadValueSlow(store_, *logical_.partition_, db_id_, - key_, logical_.digest_, location_, {}, - trace_, std::move(failure)); + slow_ = engine_.LoadValueSlow(store_, *partition_, db_id_, key_, digest_, + location_, {}, trace_, std::move(failure)); } -template -void StorageEngine::Impl::ValueReadAwaitable::FinishBufferWait() { +void StorageEngine::Impl::LoadValueAwaitable::FinishBufferWait() { auto acquired = buffer_wait_->await_resume(); buffer_wait_.reset(); - if constexpr (Logical) { - if (PopulationChanged()) { - ready_.emplace(absl::NotFoundError("key not found")); - return; - } - } - if (!acquired.ok()) { + if (PopulationChanged()) { + ready_.emplace(absl::NotFoundError("key not found")); + } else if (!acquired.ok()) { ready_.emplace(acquired.status()); } else { lease_ = std::move(*acquired); PrepareLocal(); - if constexpr (Logical) { - if (logical_.partition_ && ready_ && absl::IsAborted(ready_->status())) - Retry(ready_->status()); - } + if (partition_ && ready_ && absl::IsAborted(ready_->status())) + Retry(ready_->status()); } } -template -void StorageEngine::Impl::ValueReadAwaitable::BufferReady( +void StorageEngine::Impl::LoadValueAwaitable::BufferReady( bycorf::RemoteWork* work) { - auto& read = *static_cast(work); + auto& read = *static_cast(work); read.FinishBufferWait(); auto next = read.SuspendRead(read.awaiting_); if (next != std::noop_coroutine()) read.store_.worker_->Enqueue(next); } -template -std::coroutine_handle<> -StorageEngine::Impl::ValueReadAwaitable::await_suspend( +std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::await_suspend( std::coroutine_handle<> awaiting) { if (buffer_wait_) { awaiting_ = awaiting; @@ -1593,16 +1528,10 @@ StorageEngine::Impl::ValueReadAwaitable::await_suspend( return SuspendRead(awaiting); } -template -std::coroutine_handle<> -StorageEngine::Impl::ValueReadAwaitable::SuspendRead( +std::coroutine_handle<> StorageEngine::Impl::LoadValueAwaitable::SuspendRead( std::coroutine_handle<> awaiting) { - if constexpr (Logical) { - if (logical_.slow_.valid()) - return std::move(logical_.slow_) - .operator co_await() - .await_suspend(awaiting); - } + if (slow_.valid()) + return std::move(slow_).operator co_await().await_suspend(awaiting); if (ready_) return awaiting; auto buffer = lease_.io_buffer(); buffer.size_ = read_bytes_; @@ -1616,56 +1545,47 @@ StorageEngine::Impl::ValueReadAwaitable::SuspendRead( aligned_offset_, this); if (Suspend(awaiting, status)) return std::noop_coroutine(); if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); - if constexpr (Logical) { - if (logical_.partition_ && absl::IsAborted(status)) { - Retry(std::move(status)); - return std::move(logical_.slow_) - .operator co_await() - .await_suspend(awaiting); - } + if (partition_ && absl::IsAborted(status)) { + Retry(std::move(status)); + return std::move(slow_).operator co_await().await_suspend(awaiting); } return awaiting; } -template -void StorageEngine::Impl::ValueReadAwaitable::Complete( - bycorf::Worker& worker, int bytes, unsigned flags) { +void StorageEngine::Impl::LoadValueAwaitable::Complete(bycorf::Worker& worker, + int bytes, + unsigned flags) { if (trace_) trace_->io_complete_ns_ = ReadTraceNowNanos(); if (bytes >= 0) worker.RecordStorageReadCompletion(bytes); - if constexpr (Logical) { - if (logical_.partition_ && bytes >= 0 && - static_cast(bytes) == read_bytes_) { - // Only metadata is examined on the completion poller. CRC and large-value - // copies stay in the caller's scheduled slice. A stale identity must - // select the retry continuation before the caller's co_await can finish. - auto parsed = ParseRecord(lease_.io_buffer().data_ + record_headroom_); - if (parsed.ok()) - logical_.payload_ = *parsed; - else if (absl::IsAborted(parsed.status()) && !PopulationChanged()) { - Retry(parsed.status()); - auto child = std::move(logical_.slow_) - .operator co_await() - .await_suspend(awaiting_); - // CQEs are polled outside the parent's task class. Preserve background - // scheduling when a backup/maintenance read starts its retry here. - if (worker.IsBackground(awaiting_)) worker.RegisterBackground(child); - worker.Enqueue(child); - return; - } else - ready_.emplace(parsed.status()); - } + if (partition_ && bytes >= 0 && + static_cast(bytes) == read_bytes_) { + // Only metadata is examined on the completion poller. CRC and large-value + // copies stay in the caller's scheduled slice. A stale identity must select + // the retry continuation before the caller's co_await can finish. + auto parsed = ParseRecord(lease_.io_buffer().data_ + record_headroom_); + if (parsed.ok()) + payload_ = *parsed; + else if (absl::IsAborted(parsed.status()) && !PopulationChanged()) { + Retry(parsed.status()); + auto child = + std::move(slow_).operator co_await().await_suspend(awaiting_); + // CQEs are polled outside the parent's task class. Preserve background + // scheduling when a backup/maintenance read starts its retry here. + if (worker.IsBackground(awaiting_)) worker.RegisterBackground(child); + worker.Enqueue(child); + return; + } else + ready_.emplace(parsed.status()); } OneShotIoAwaitable::Complete(worker, bytes, flags); } -template -typename StorageEngine::Impl::ValueReadAwaitable::Result -StorageEngine::Impl::ValueReadAwaitable::await_resume() { - if constexpr (Logical) { - // Fence after the scheduling gap; completion only fences before retries. - if (PopulationChanged()) return absl::NotFoundError("key not found"); - if (logical_.slow_.valid()) return std::move(logical_.slow_).TakeResult(); - } +StorageEngine::Impl::LoadValueAwaitable::Result +StorageEngine::Impl::LoadValueAwaitable::await_resume() { + // Fence successful reads here, after the scheduling gap following the CQE. + // Completion only needs this check when deciding whether to start a retry. + if (PopulationChanged()) return absl::NotFoundError("key not found"); + if (slow_.valid()) return std::move(slow_).TakeResult(); if (ready_) return std::move(*ready_); auto read = Resume(lease_.registered() ? "fixed-buffer read failed" : "read failed"); @@ -1673,18 +1593,16 @@ StorageEngine::Impl::ValueReadAwaitable::await_resume() { if (static_cast(*read) != read_bytes_) return absl::InternalError("short compact record read"); const auto* data = lease_.io_buffer().data_ + record_headroom_; - if constexpr (Logical) { - if (logical_.payload_) return FinishRecord(data, *logical_.payload_, false); + if (!payload_) { + auto parsed = ParseRecord(data); + if (!parsed.ok()) return parsed.status(); + payload_ = *parsed; } - auto parsed = ParseRecord(data); - if (!parsed.ok()) return parsed.status(); - return FinishRecord(data, *parsed, false); + return FinishRecord(data, *payload_, false); } -template -absl::StatusOr< - typename StorageEngine::Impl::ValueReadAwaitable::Payload> -StorageEngine::Impl::ValueReadAwaitable::ParseRecord( +absl::StatusOr +StorageEngine::Impl::LoadValueAwaitable::ParseRecord( const std::byte* data) const { RecordHeader record{}; std::string_view disk_key; @@ -1714,10 +1632,10 @@ StorageEngine::Impl::ValueReadAwaitable::ParseRecord( record.payload_checksum_}; } -template -typename StorageEngine::Impl::ValueReadAwaitable::Result -StorageEngine::Impl::ValueReadAwaitable::FinishRecord( - const std::byte* data, Payload payload, bool staged) { +StorageEngine::Impl::LoadValueAwaitable::Result +StorageEngine::Impl::LoadValueAwaitable::FinishRecord(const std::byte* data, + Payload payload, + bool staged) { const auto* value = data + payload.offset_; if (Crc32c(std::span(value, payload.bytes_)) != payload.checksum_) return absl::InternalError("record value checksum mismatch"); @@ -1730,8 +1648,4 @@ StorageEngine::Impl::ValueReadAwaitable::FinishRecord( return LoadedValue{std::move(lease_), offset, payload.bytes_, payload.txid_}; } -// Keep the shared implementation out of every loader caller's translation unit. -template class StorageEngine::Impl::ValueReadAwaitable; -template class StorageEngine::Impl::ValueReadAwaitable; - } // namespace lavik::storage diff --git a/tests/load_value_awaitable_test.cpp b/tests/load_value_awaitable_test.cpp index 0726c3230..d5e1a4302 100644 --- a/tests/load_value_awaitable_test.cpp +++ b/tests/load_value_awaitable_test.cpp @@ -17,8 +17,6 @@ namespace lavik::storage { class LoadValueAwaitableTestPeer : public testing::Test { protected: using Awaitable = StorageEngine::Impl::LoadValueAwaitable; - using LocalAwaitable = StorageEngine::Impl::LoadValueLocalAwaitable; - static_assert(sizeof(LocalAwaitable) < sizeof(Awaitable)); using Result = Awaitable::Result; using ExtentRead = StorageEngine::Impl::ReadExtentAwaitable; @@ -86,22 +84,18 @@ class LoadValueAwaitableTestPeer : public testing::Test { auto ReadLocal() { return storage_.impl_->LoadValueLocal(Store(), 0, key_, location_, 1); } - bool HasHelper(const Awaitable& read) { return read.logical_.slow_.valid(); } + bool HasHelper(const Awaitable& read) { return read.slow_.valid(); } void Encode(std::byte* data) { ASSERT_TRUE(EncodeRecordHeader(header_, key_, std::span(data, header_.header_bytes_))); std::memcpy(data + header_.header_bytes_, value_.data(), value_.size()); } - template - void Complete(Read& read, int bytes = -1) { + void Complete(Awaitable& read, int bytes = -1) { ASSERT_NE(read.pinned_, nullptr); Encode(read.lease_.io_buffer().data_ + read.record_headroom_); read.Complete(worker_, bytes == -1 ? read.read_bytes_ : bytes, 0); } - void RewriteRecord(LocalAwaitable& read) { - Encode(read.lease_.io_buffer().data_ + read.record_headroom_); - } void Stage() { location_.set_in_memory(true); block_.in_memory_ = true; @@ -158,13 +152,9 @@ class LoadValueAwaitableTestPeer : public testing::Test { co_await Arm{read}; co_return read.await_resume(); } - template - Task AwaitExisting(Read& read) { - co_return co_await read; - } + Task AwaitExisting(Awaitable& read) { co_return co_await read; } Task AwaitExtent(ExtentRead& read) { co_return co_await read; } - template - bool BufferWaitIsBackground(Read& read) { + bool BufferWaitIsBackground(Awaitable& read) { return read.task_class_ == bycorf::TaskClass::kBackground; } auto ExtentSlice(ExtentRef ref, std::size_t offset, @@ -254,7 +244,7 @@ class LoadValueAwaitableTestPeer : public testing::Test { } bool HelperIsBackground(Awaitable& read) { return worker_.IsBackground( - std::move(read.logical_.slow_).operator co_await().handle_); + std::move(read.slow_).operator co_await().handle_); } bycorf::CurrentWorker saved_context_; @@ -274,7 +264,7 @@ TEST_F(LoadValueAwaitableTestPeer, BufferWaitersPreserveFifoHandoff) { ASSERT_NO_FATAL_FAILURE(InstallRecord()); auto held = Pool().AcquireReadBuffer().await_resume(); ASSERT_TRUE(held.ok()); - auto first = ReadLocal(); + auto first = Read(); auto second = Read(); auto first_task = AwaitExisting(first); auto second_task = AwaitExisting(second); @@ -292,6 +282,7 @@ TEST_F(LoadValueAwaitableTestPeer, BufferWaitersPreserveFifoHandoff) { ExpectValue(std::move(first_task).TakeResult()); ASSERT_NO_FATAL_FAILURE(Drain(second_task)); ExpectValue(std::move(second_task).TakeResult()); + EXPECT_FALSE(HasHelper(first)); EXPECT_FALSE(HasHelper(second)); EXPECT_EQ(Pool().available_read_buffers(), 1); } @@ -440,23 +431,11 @@ TEST_F(LoadValueAwaitableTestPeer, ErrorsReleaseResources) { } TEST_F(LoadValueAwaitableTestPeer, LocalReadReturnsStaleLocationToItsCaller) { - { - block_.freeing_ = true; - auto read = ReadLocal(); - ASSERT_TRUE(read.await_ready()); - EXPECT_TRUE(absl::IsAborted(read.await_resume().status())); - } - block_.freeing_ = false; - { - auto read = ReadLocal(); - ASSERT_NO_FATAL_FAILURE(Complete(read)); - // Physical reads parse in the caller, not on the completion poller. - ++header_.allocation_epoch_; - RewriteRecord(read); - EXPECT_TRUE(absl::IsAborted(read.await_resume().status())); - } - EXPECT_EQ(block_.pins_, 0); - EXPECT_EQ(Pool().available_read_buffers(), 1); + block_.freeing_ = true; + auto read = ReadLocal(); + ASSERT_TRUE(read.await_ready()); + EXPECT_FALSE(HasHelper(read)); + EXPECT_TRUE(absl::IsAborted(read.await_resume().status())); } TEST_F(LoadValueAwaitableTestPeer,