S3: Commit + Read path: advance commit_lsn, serve reads, watchdog - #175
S3: Commit + Read path: advance commit_lsn, serve reads, watchdog#175shosseinimotlagh wants to merge 11 commits into
Conversation
Each journal slot's on-disk blob gains a csum_t array between the fixed 34-byte header and the serialized multi_blk_id, one crc16_t10dif entry per LBA in the write's range. write() computes the array while the payload is still in memory (same seed/routine volume.cpp's non-CRAFT path already uses), and CraftReplDev now takes the volume's lba_size at construction to derive the array length.
Parses a slot's blob back into a JournalSlot: read_sync's std::out_of_range (truncated or never-appended lsn) maps to result_out_of_range; the header, csum array, and blkid regions are each size-validated before their length fields are trusted, so a truncated or corrupt record fails cleanly rather than overreading. HomeStoreCraftJournalBackend and its factory now take the volume's lba_size to derive nlbas from a slot's on-disk byte length.
Removes every index entry in [start_lba, end_lba] via a single BtreeRangeRemoveRequest, using the remove filter callback to capture each removed entry's blkid so the caller can reclaim those blocks. not_found is treated as success (unmap of an already-absent range is a no-op). Needed by CRAFT's upcoming all_zeros commit-apply path, which has no other way to clear index entries for a WRITE_ZEROES/unmap range.
CraftReplDev now takes the volume's shared<VolumeIndexTable> at construction (nullptr in write-path-only tests) and maintains an overlay_ map of the highest-dLSN unapplied entry per LBA in (commit_lsn, last_append_lsn]. write()'s post-flight success path populates it: all_zeros writes record a zero-fill marker per LBA, data writes decompose the multi_blk_id's pieces (multi_blk_id::iterate()) into per-LBA single- block entries, same shape as volume.cpp's non-CRAFT write path but generalized since a CRAFT blkid may span more than one piece. Highest-dLSN wins per LBA so an out-of-order lower-dLSN write can't clobber a later one.
commit(upto_lsn) advances commit_lsn by applying each committable slot to the index: stalls (not an error) at the first gap in missing_lsns_, skips Empty-verdicted slots, and otherwise reads the slot back and applies it -- all_zeros via delete_lba_range, data via write_to_index with the slot's multi_blk_id decomposed into per-LBA BlockInfo entries. Reclaims any freed or superseded old blkid inline (same as write()'s existing free_data call sites). Retires the overlay entry per applied LBA only when its recorded lsn matches the lsn just applied, so a higher-dLSN entry for the same LBA survives. At most one commit() run is ever active (commit_running_); a concurrent caller is a safe no-op since the in-flight run covers the same ground and every subsequent write()/keep_alive() retries the advance. write()'s post-flight block calls commit() best-effort (every outcome ignored); keep_alive() is now implemented for real and propagates a genuine commit() fault (not a stall) to its caller, since advancing the frontier is its entire purpose. The core apply algorithm is parameterized (commit_impl) over the index write/delete operations so test_craft_commit.cpp can exercise it against a fake std::map-backed index via the test-only commit_with() seam, without a real VolumeIndexTable or HomeStore bring-up.
write() now rejects addr/len that aren't a positive multiple of lba_size_. Without this, a client-supplied len=0 (or a len that rounds nlbas down to 0) would journal a slot that later reaches commit_impl's end_lba = start_lba + nlbas - 1, underflowing lba_t (unsigned) into a near-UINT64_MAX range applied to the real index -- an effectively unbounded loop reachable straight from the client wire. commit_impl also gets a defense-in-depth nlbas==0 guard for a stale/legacy on-disk record that predates this validation. Closes coverage gaps found in a full review of Commits 1-5: - write()'s own CRC16 computation is now checked against an independently computed value (previously only checked via array length); added a multi-LBA case so the per-LBA loop isn't only ever exercised with nlbas=1. - commit_impl's multi-piece multi_blk_id decomposition, its commit_running_ concurrency guard (via a reentrant test double), real write_fn/delete_fn error propagation, upto_lsn clamping to last_append_lsn, and a repeated-commit no-op are now all covered. - write()'s overlay population is now exercised for all_zeros writes too (previously data writes only). - HomeStoreCraftJournalBackend::read_slot's validate-before-trust paths (bad magic, truncated header/csum/blkid regions) and an all_zeros round-trip are now covered against a real log store. The new corruption-injection tests initially used home_log_store:: write_and_flush() to inject raw blobs directly, which left the logdev's completion bookkeeping inconsistent with what a graceful homestore shutdown waits on, hanging test teardown indefinitely. Fixed by routing through the same write_async/value_awaitable completion bridge write_slot itself already uses.
Serves [addr, addr+len) as of read_lsn by merging the LBA index's committed state with the journal-tail overlay, horizon-clamped to (commit_lsn, read_lsn] so an overlay entry above read_lsn is held but never served -- the index's older, still-valid value is used instead. Absent-from-both and all_zeros entries both read as holes; a data entry whose actual payload happens to be all-zero bytes collapses to a hole at read time only, never at write time. Every data-carrying LBA is checksum-verified against its stored per-LBA CRC16, matching volume.cpp's non-CRAFT read precedent (verify-before-trust). Adds CraftJournalBackend::read_data (mirroring alloc_write_data/ free_data's existing shape) so the byte-level read stays mockable -- read()'s own logic (index/overlay merge, horizon clamp, checksum verification, read-time zero-collapse) is covered by 8 new light tests in test_craft_commit.cpp with no real HomeStore, via a commit_impl-style read_impl()/read_with() test seam.
reset_watchdog() cancels-and-reschedules a one-shot iomgr timer on every successful write()/keep_alive(); on expiry (no activity from the current session for a full interval) on_watchdog_expiry() fires append() via detail::detach (a plain timer-callback context, not a coroutine caller) and reschedules itself -- a permanently-silent client keeps getting append() attempts, not just one. No-ops before the first successful login (state_.term == 0) or when disabled (enable_watchdog_). enable_watchdog defaults false and the timeout is constructor-injected (uint64_t nanoseconds) rather than read internally via HB_DYNAMIC_CONFIG: scheduling a real timer needs a running iomgr reactor pool that no existing light CRAFT test starts, and reading the config value directly from craft_repl_dev.cpp would force those tests (which compile this file standalone) to link the flatbuffers settings-generated object just to build. The craft_watchdog_timeout_secs key in home_blks_config.fbs is wired in by CraftReplDev's real construction site once one exists. The destructor now cancels any pending timer (wait_to_cancel=true) so a self-rescheduling callback capturing `this` can never fire against a destroyed object. New test_craft_watchdog binary: starts a minimal, HomeStore-free iomgr instance (the only light CRAFT test that needs one) so schedule_global_timer/cancel_timer work for real, while still compiling craft_repl_dev.cpp directly with _PRERELEASE like the other light tests.
test_craft_commit_hs.cpp: heavy integration test for commit()/read()
against a REAL VolumeIndexTable and real data blocks -- closing the gap
between test_craft_commit.cpp's fake-index unit tests and production
reality. Creates an ordinary volume purely to get a real, chunk-allocated
index (repl_mode::CRAFT isn't wired to anything yet), reuses its index
table for a standalone CraftReplDev with its own fresh journal. Covers
write+commit+read round trip, overlay-before-commit visibility, all_zeros
unmap reclaiming a real index entry (the only exercise of
delete_lba_range against a real index anywhere in the suite), multi-LBA
round trip, and the keep_alive()-driven commit-to-read transition via
the real public API (not just the commit_with()/read_with() test seams).
Also closes gaps found in a full review of Commits 1-8:
- 9 new tests in test_craft_commit.cpp: overlay winning over an existing
committed index entry for the same LBA, the horizon clamp's inclusive
boundary (lsn == read_lsn), a single read spanning hole + index +
overlay sources together, a write partially overlapping an existing
committed range, the end-to-end overlay-to-index visibility
transition, and read()'s own term/boundary validation (previously
only write() had these).
- Two new real multi-threaded test suites, closing the concurrency gap:
every other CRAFT test drove calls from a single thread via sync_get(),
so none of missing_mu_/overlay_mu_/commit_running_ had ever been
exercised under genuine contention.
- test_craft_concurrency.cpp (light, no HomeStore): concurrent
disjoint-dLSN writes converging correctly, concurrent write+read
with no torn reads, concurrent commit_with() calls proving
commit_running_ serializes with zero double-applies.
- test_craft_commit_hs.cpp gains two more tests: concurrent writes
from multiple threads to disjoint LBAs against the real
backend/index, and concurrent keep_alive() calls serializing
correctly against the real index.
…x a real watchdog UAF
Closes out S3 (Commit 9: async_status rebuild_overlay() walks (commit_lsn,
last_append_lsn] and repopulates the journal-tail overlay from CraftRaftListener::
on_restart(), sharing write()'s populate_overlay() helper).
Jira/wiki audit against SDSTOR-22733 and its subtasks found two real gaps versus
docs/craft/subtasks.md and the CRAFT-Design wiki, both fixed: truncate() wasn't
pruning journal-tail overlay entries for rolled-back dLSNs, and keep_alive() was
discarding hdr.all_committed_lsn instead of capturing it max-monotonically.
A five-lens parallel review then surfaced further issues, all fixed here:
- A reproduced SEGFAULT: on_watchdog_expiry()'s self-reschedule called
cancel_timer() on its own already-fired handle, corrupting iomgr's timer
heap. Fixed by nulling watchdog_hdl_ under the generation-verified lock
before rescheduling.
- write()/read() length-bound and buffer-size validation against malformed
client-wire input (OOM, over-read, and nlbas==0 stall vectors).
- populate_overlay()'s csum-array access is now bounds-checked against a
corrupted/truncated on-disk journal record.
- read() rejects a negative read_lsn instead of silently clamping every
overlay entry out.
- Concurrent write() calls for the same dLSN could double-allocate real
blocks; added an in-flight-dlsn dedup guard so only one wins and the rest
are rejected rather than racing the allocator.
- Corrected several stale/overclaiming comments (mislabeled section header,
truncate()'s step count, a stale stubs banner, and on_restart()'s comment,
which now honestly flags the missing recovery-time I/O gate instead of
asserting correctness).
- Strengthened KeepAliveAdvancesCommitThenReadServesIndex so it actually
distinguishes overlay- from index-sourced reads by content.
New stress tests: ConcurrentSameDlsnWritesNoDoubleAllocation and
DestructorRacesFiringManyIterations (200 iterations racing watchdog teardown
against a real firing timer -- the exact class that caught the SEGFAULT above).
All 11 CRAFT/volume test binaries pass, verified with repeated runs of the new
concurrency-sensitive tests to rule out flakiness.
C1 – commit_impl(): csum-array bounds guard If a journal slot's csums[] is shorter than the blkid piece count (corrupt on-disk record), abort with INTERNAL_ERROR instead of accessing out-of-bounds. C2 – rebuild_overlay(): nlbas==0 guard A slot with len_bytes==0 produces start_lba+0-1 underflow inside populate_overlay(). On the restart path, skip and continue rather than aborting the entire rebuild. R1-1 – write() and read(): capture all_committed_lsn Both entry points now update state_.all_committed_lsn with max-monotonic semantics under missing_mu_, matching keep_alive()'s existing pattern. C5 – read_impl(): empty sg_list guard Guard dest.iovs[0] access with an explicit dest.iovs.empty() check; return invalid_argument on a malformed caller sg_list. UTs (test_craft_commit.cpp +10, test_craft_write.cpp +1): DeleteFnErrorAbortsCommit, ReadFnErrorPropagates, ReadNegativeLsnRejected, KeepAliveRejectsStaleTerm, CommitCsumShortArrayAborts (C1), RebuildOverlaySkipsNlbasZeroSlot (C2), ReadDestEmptyIovsRejected (C5), WriteCapturesAllCommittedLsn (R1-1), ReadCapturesAllCommittedLsn (R1-1), ReadMaxIoLenEnforced, MaxIoLenEnforced (write). Both files' main() sets craft_max_io_len_mb=1 so MaxIoLen tests work without allocating >1 MiB buffers.
f101cf5 to
3f4a438
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new read/write paths assume single-iovec sg_lists but don’t fully validate/enforce buffer shape/capacity, which can lead to checksum mismatches and potential out-of-bounds writes on the wire-reachable read path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements the CRAFT S3 “commit + read path” for CraftReplDev, including advancing commit_lsn by applying journal entries to the index, serving reads from an overlay+index merge (with CRC validation), and adding a client-liveness watchdog plus configuration knobs to bound request sizes.
Changes:
- Add commit/apply machinery (including
VolumeIndexTable::delete_lba_range) and overlay rebuild on restart, with read-path overlay/index resolution and CRC verification. - Introduce watchdog timer behavior and new settings (
craft_watchdog_timeout_ms,craft_max_io_len_mb) with corresponding test coverage. - Expand/adjust CRAFT API and test build wiring to support the new behaviors (including new unit/integration tests).
File summaries
| File | Description |
|---|---|
| src/lib/volume/index_fixed_table.hpp | Adds delete_lba_range to support unmap/all-zero apply via range remove and blkid reclamation. |
| src/lib/home_blks_config.fbs | Adds watchdog timeout and max I/O length configuration keys. |
| src/lib/craft/tests/test_craft_write.cpp | Updates write tests for new checksum behavior, argument changes, and max-IO enforcement. |
| src/lib/craft/tests/test_craft_watchdog.cpp | New watchdog timer test binary with minimal iomgr bring-up. |
| src/lib/craft/tests/test_craft_truncate.cpp | Updates mocks/constructors and settings initialization for new config usage. |
| src/lib/craft/tests/test_craft_peer_exchange.cpp | Updates mocks/constructors and settings initialization for new journal API shape. |
| src/lib/craft/tests/test_craft_journal_slot_wire.cpp | Adjusts wire-compat assertions now that JournalSlot is a superset internally. |
| src/lib/craft/tests/test_craft_homestore_backend.cpp | Extends HomeStore backend coverage (read_slot parsing/validation; updated backend ctor). |
| src/lib/craft/tests/test_craft_concurrency.cpp | New multi-threaded tests for internal locking, overlay/read behavior, and commit serialization. |
| src/lib/craft/tests/test_craft_commit.cpp | New comprehensive unit tests for commit/read/rebuild overlay behavior and error paths. |
| src/lib/craft/tests/test_craft_commit_hs.cpp | New heavy integration tests against a real VolumeIndexTable + real HomeStore data. |
| src/lib/craft/tests/CMakeLists.txt | Wires new test targets and ensures config bindump is linked into “light” tests. |
| src/lib/craft/craft_repl_dev.hpp | Extends interfaces/state (checksums, read_data, overlay, watchdog, commit/read seams). |
| src/lib/craft/craft_repl_dev.cpp | Implements on-disk slot format changes, commit/read/overlay logic, watchdog, and recovery gate. |
| src/lib/craft/craft_api.cpp | Updates async_write signature to match “empty data means unmap” semantics. |
| src/include/homeblks/home_blocks.hpp | Updates public API documentation/signature for CRAFT write/unmap semantics. |
| conanfile.py | Bumps package version to 6.0.7. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (dest.iovs.empty()) { | ||
| LOGW("read rejected: dest sg_list has no iovecs (size={})", dest.size); | ||
| co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); | ||
| } | ||
| auto* dest_buf = static_cast< uint8_t* >(dest.iovs[0].iov_base); |
| uint32_t nlbas = slot.all_zeros ? 0 : (hdr.len / lba_size_); | ||
| uint32_t csum_bytes = nlbas * static_cast< uint32_t >(sizeof(homestore::csum_t)); |
| if (!all_zeros && (data.iovs.empty() || data.size != len || data.iovs[0].iov_len < len)) { | ||
| LOGW("write rejected: data.size={} does not match len={} dlsn={}", data.size, len, dlsn); | ||
| co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/v6.x #175 +/- ##
===========================================
Coverage ? 46.97%
===========================================
Files ? 18
Lines ? 1141
Branches ? 501
===========================================
Hits ? 536
Misses ? 266
Partials ? 339 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| bool is_missing, is_empty; | ||
| { | ||
| std::lock_guard lk{missing_mu_}; | ||
| is_missing = missing_lsns_.contains(lsn); |
There was a problem hiding this comment.
is_missing does not need to be variable. The check can be done within the block
{
std::lock_guard lk{missing_mu_};
if (missing_lsns_.contains(lsn)) { is_empty = true; break/continue;}
is_empty = empty_lsns_.contains(lsn);
}
| is_missing = missing_lsns_.contains(lsn); | ||
| is_empty = empty_lsns_.contains(lsn); | ||
| } | ||
| if (is_missing) break; // stall at the first hole -- not an error |
There was a problem hiding this comment.
Good point. it is intentional
hen the loop hits an LSN that is in missing_lsns_:
if (is_missing) break; // stall at the first hole -- not an error
It stops applying and returns the current commit_lsn unchanged. It does NOT return an error — the caller just gets "nothing advanced yet, try again later."
This is correct because the index must be applied in strict LSN order. The index is a persistent, durable data structure. If you applied LSN 8 before LSN 7, the index would have a version of LBA X from write 8, then write 7 would overwrite it with stale data. That's data corruption.
The "not an error" part
A hole is a normal, expected transient state — the missing write is in-flight on the network. The right response is:
- Stop now (don't skip the hole and apply 8 without 7).
- Return cleanly — the next write() or keep_alive() that fills the hole will call commit() again, and this time the loop will advance through it.
If it returned an error, the caller would have to do something special. Instead it returns commit_lsn (the current watermark, unchanged), which is a valid successful result meaning "committed up to here."
Concrete example
missing_lsns_ = {7} ← write for LSN 7 hasn't arrived yet
commit_lsn = 5
target = 10
Loop iteration lsn=6: not missing, not empty → apply → commit_lsn advances to 6
Loop iteration lsn=7: is_missing → break
Return: commit_lsn = 6 ← clean return, not an error
... later, write(lsn=7) arrives, fills the hole, removes 7 from missing_lsns_,
calls commit(10) again ...
Loop resumes from lsn=7: apply 7 → 8 → 9 → 10 → commit_lsn = 10
TL;DR: It's a deliberate stall point that enforces serial in-order index application. A missing LSN means "data not yet received" — that's a normal race condition, not a fault, so the loop breaks silently and the caller retries implicitly via the next incoming write.
There was a problem hiding this comment.
I mixed up the empty with missing.
There should be a short circuit if we get a confirmation on a lsn is empty. which is missing.
There was a problem hiding this comment.
🟡 Changes recommended
Horizon reads, checksum verification, buffer validation, recovery compatibility, and peer data fetching contain correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/lib/craft/craft_repl_dev.cpp:945
- Checking only that an iovec exists does not establish that the flat destination has
lenwritable bytes. A malformed public call withdest.size < len, a short first iovec, or a null base reaches thememset/async_readbelow and writes past or through the supplied buffer. Validate the destination exactly as the write path validates its source before dereferencing it.
if (dest.iovs.empty()) {
LOGW("read rejected: dest sg_list has no iovecs (size={})", dest.size);
co_return std::unexpected(make_error_condition(std::errc::invalid_argument));
}
auto* dest_buf = static_cast< uint8_t* >(dest.iovs[0].iov_base);
src/lib/craft/craft_repl_dev.cpp:240
- After deserialization there is no structural check that a data slot's blkid covers exactly
hdr.len / lba_size_blocks (or that a zero slot has an empty blkid). If it covers fewer blocks,commit_impl()builds fewerBlockInfovalues but still callswrite_to_index()for the full header range, which inserts default/invalid mappings for the missing LBAs and advancescommit_lsn. Validate alignment and exact block coverage here before returning the slot.
sisl::blob blkid_blob{buf.bytes() + blkid_off, buf.size() - blkid_off};
slot.blkid.deserialize(blkid_blob, /* copy = */ true);
- Files reviewed: 17/17 changed files
- Comments generated: 8
- Review effort level: Balanced
| // Each log slot is: [CraftJournalEntry header][csum_t per LBA][serialized multi_blk_id bytes]. | ||
| // The payload (HS_DATA_LINKED) is written directly to the data service; only the | ||
| // block reference is stored here. | ||
| // block reference is stored here. The checksum array is empty for all_zeros slots | ||
| // (no data, nothing to sum) and otherwise has exactly len/lba_size entries, computed | ||
| // on the write path while the data is still in memory (see CraftReplDev::write()). |
| for (auto const& [_, info] : blocks_info) { | ||
| if (!info.old_blkid.is_valid()) continue; | ||
| if (auto fr = co_await journal_->free_data(homestore::multi_blk_id{info.old_blkid}); !fr) | ||
| LOGE("free_data failed reclaiming blk={} lsn={}: {}", info.old_blkid.to_string(), lsn, | ||
| fr.error().message()); | ||
| } |
| if (it != overlay_.end() && it->second.lsn > commit_lsn_snapshot && it->second.lsn <= read_lsn) { | ||
| if (!it->second.all_zeros) sources[i] = Source{false, it->second.blkid, it->second.csum}; | ||
| continue; // all_zeros overlay entry -> hole (Source's default) | ||
| } | ||
| if (auto idx_it = index_map.find(lba); idx_it != index_map.end()) | ||
| sources[i] = Source{false, idx_it->second.blkid(), idx_it->second.checksum()}; |
| bool const all_zero = std::all_of(lba_buf, lba_buf + lba_size_, [](uint8_t b) { return b == 0; }); | ||
| if (all_zero) { | ||
| // Read-time-only collapse: a data write whose payload happened to be all-zero bytes | ||
| // reads back as a hole. This scan must never run on the write path. | ||
| is_hole[k] = true; | ||
| continue; | ||
| } | ||
| auto const computed = crc16_t10dif(k_craft_crc16_seed, lba_buf, lba_size_); | ||
| if (computed != sources[k].csum) { | ||
| LOGE("read: crc mismatch lba={} expected={} actual={}", start_lba + k, sources[k].csum, computed); | ||
| co_return std::unexpected(make_error_condition(volume_error::CRC_MISMATCH)); | ||
| } | ||
| is_hole[k] = false; |
| homestore::multi_blk_id blkid{}; // block reference (empty for all_zeros slots) | ||
| std::vector< homestore::csum_t > csums{}; // one per LBA in range; empty for all_zeros |
| // Read the data payload referenced by blkid into dest (dest.size already set by the caller to | ||
| // blkid's byte length). Used by CraftReplDev::read() to fetch the bytes an index/overlay entry | ||
| // only stores a block reference for. Mockable so read()'s tests stay light (no real HomeStore). | ||
| virtual async_status read_data(homestore::multi_blk_id blkid, sisl::sg_list& dest) = 0; |
| JournalSlot slot; | ||
| slot.lsn = hdr.lsn; | ||
| slot.all_zeros = hdr.all_zeros != 0; | ||
| slot.lba_off_bytes = hdr.lba; | ||
| slot.len_bytes = hdr.len; |
| touch_watchdog(); | ||
|
|
||
| LOGT("write ok dlsn={} addr={} len={} all_zeros={}", dlsn, addr, len, all_zeros); | ||
| co_return snapshot; |
| is_missing = missing_lsns_.contains(lsn); | ||
| is_empty = empty_lsns_.contains(lsn); | ||
| } | ||
| if (is_missing || is_empty) continue; |
There was a problem hiding this comment.
Variable is not needed. check and continue within lock. Short-circuit for the check can also be leveraged
|
|
||
| for (uint32_t k = i; k < j; ++k) { | ||
| uint8_t const* lba_buf = dest_buf + k * lba_size_; | ||
| bool const all_zero = std::all_of(lba_buf, lba_buf + lba_size_, [](uint8_t b) { return b == 0; }); |
There was a problem hiding this comment.
We can optimize this. We can compare it against a vector<uint_8> k_zero_block(lba_size_, 0). memcmp is more performant. Downside is additional memory footprint of lba_size
There was a problem hiding this comment.
nice ! Thank you
| for (uint32_t k = i; k < j; ++k) { | ||
| uint8_t const* lba_buf = dest_buf + k * lba_size_; | ||
| bool const all_zero = std::all_of(lba_buf, lba_buf + lba_size_, [](uint8_t b) { return b == 0; }); | ||
| if (all_zero) { |
There was a problem hiding this comment.
If there is a bit flip resulting in this getting marked as a hole. Should we also do a crc check just to be safe?
We dont need to calc the crc again and again. crc for zero_block can be precalculated
There was a problem hiding this comment.
ack! nice catch
| co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); | ||
| } | ||
| auto* dest_buf = static_cast< uint8_t* >(dest.iovs[0].iov_base); | ||
| std::vector< bool > is_hole(nlbas); |
There was a problem hiding this comment.
ack!
The constructor argument that sets the initial number of elements in the vector to the value stored in the variable nlbas. All elements are automatically initialized to false. C++ guarantees this.
C++: "Otherwise, the object is zero-initialized." link
I agree with you that for sake of clarity and style, it's better to initialize it
|
|
||
| // dest is a single flat iovec -- same assumption write()'s CRC computation already makes; every | ||
| // sg_list CRAFT builds anywhere in this codebase is a single iovec. | ||
| if (dest.iovs.empty()) { |
There was a problem hiding this comment.
Do we need to do the same check as the write path?
| // Not yet logged in -- no session to watch. NOTE this only prevents ARMING before the first | ||
| // login; it does not DISARM an already-armed timer once a session ends. logout() is still a | ||
| // stub today (see the "stubs" section) and never actually resets state_.term back to 0, so this | ||
| // is currently unreachable in practice -- but whoever implements real logout() should also stop |
There was a problem hiding this comment.
Lets leave a TODO. We will miss the hints of things to do in coming days from large comments
| }; | ||
| std::vector< Source > sources(nlbas); | ||
| { | ||
| std::lock_guard lk{overlay_mu_}; |
There was a problem hiding this comment.
Comments from claude
Possible stale-read race between read_impl and a concurrent commit_impl — same LBA, no error, watermark says fresh.
read_impl snapshots the index via read_fn (line 908) outside overlay_mu_, then separately locks
overlay_mu_ here (line 925) to consult the overlay. commit_impl does the mirror-image split: it
writes the index via write_fn (line 718), then separately locks overlay_mu_ to erase the matching
entry (line 731) — with a blocking I/O call (read_slot, sometimes free_data) in between the two.
Neither pair is atomic, and nothing serializes one against the other — commit_running_ only
prevents two commit_impl runs from overlapping; it doesn't make a concurrent reader wait.
(read()'s own commit() call can't help either: if another commit is already in flight, commit()'s
guard makes this call an instant no-op rather than a wait.)
Repro shape:
- Thread A (commit_impl) is applying lsn=6 for LBA X, currently blocked in read_slot/free_data.
- Thread B (read_impl, read_lsn >= 6, same LBA X) calls read_fn — lands before A's write_fn,
capturing the pre-commit value. - A finishes: index now updated, and A erases overlay_[X] (line 731).
- B locks overlay_mu_ (line 925) — finds nothing (A already erased it) — falls back to the stale
index snapshot from step 2.
B returns data older than what's already fully committed, with no error, and its own
{commit_lsn, last_append_lsn} response watermark can already show the advanced value by the time
B returns — so the response looks authoritative while the payload is stale.
This isn't a hairline-timing race: commit_impl holds no lock across two real blocking I/O calls
before it reaches the overlay erase, so the window is roughly as wide as one slot's I/O latency,
not a few instructions. It's also not caught by the existing concurrency suite — the light tests
(test_craft_concurrency.cpp) use fake index/journal doubles that complete near-instantly, and the
heavy HomeStore suite (test_craft_commit_hs.cpp) only exercises concurrent writes to disjoint LBAs
and concurrent keep_alive vs keep_alive — never a read racing a commit on the same LBA.
Am I missing a synchronization mechanism that closes this? If not, one direction is extending
overlay_mu_'s scope on both sides to cover "index op + overlay op" as one critical section —
read_fn/write_fn/delete_fn are all synchronous (no co_await inside), so there's no
suspension-under-lock hazard — though that would serialize every read/commit on the instance
against every other, regardless of LBA overlap, which is worth weighing against real B-tree
latency before committing to it.
| blocks_info.emplace(lba, BlockInfo{single_bid, homestore::blk_id{}, slot.csums[csum_idx]}); | ||
| } | ||
| } | ||
| if (auto r = write_fn(start_lba, end_lba, blocks_info); !r) co_return std::unexpected(r.error()); |
There was a problem hiding this comment.
Companion to the comment on read_impl's overlay lock (line 925) — this is the other half of that
race. write_fn here and the overlay erase at line 731 aren't atomic with each other either.
Flagging here too since a fix likely needs to touch both sides symmetrically.
| // a stall at a gap, or even a genuine commit() fault -- is ignored here; the write itself already | ||
| // succeeded, and the next write/keep_alive retries the advance. keep_alive() is where a real | ||
| // commit() error actually surfaces (advancing the frontier is its entire purpose). | ||
| co_await commit(hdr.commit_lsn); |
There was a problem hiding this comment.
Comments from claude
Asymmetry between restart-time and runtime failure handling for a corrupt journal record.
run_recovery() (line 1304) treats a rebuild_overlay() failure as permanent: it latches
recovery_faulted_ (line 1310), and per its own doc comment, every client-facing entry point
checks this first and rejects with INTERNAL_ERROR forever after -- "a stuck-rejecting partition
is a visible, actionable failure; a partition silently serving stale data is not."
The same underlying fault -- commit_impl hitting a corrupt record (bad magic, truncated csum
array, nlbas==0) -- has no equivalent when it happens live instead of at restart:
- write() (here, line 620) and read() (line 844) both discard commit()'s result unconditionally.
The write/read itself still succeeds and returns normally to the client. - keep_alive() (line 1036) does propagate the error -- but only for that one call, with nothing
latched. The next keep_alive() retries, hits the same corrupt slot, fails again.
Since commit_lsn never advances past the bad lsn, every future commit() call re-enters the loop
at the same corrupt record and fails the same way -- indefinitely. There's no counter, flag, or
metric recording this; just a LOGE line each time. overlay_ also keeps growing unbounded above
that point, since nothing past the stuck lsn ever gets applied-and-retired -- exactly the growth
scenario the watchdog's periodic commit() nudging is supposed to keep in check, except progress
is now permanently blocked.
Is this intentionally out of scope for S3 (i.e., "operator observability for a stuck partition"
belongs to a later subtask)? If so, could you point me at the tracking ticket? If not, this seems
like it wants the same treatment as the restart path -- e.g. commit_impl setting recovery_faulted_
(or a dedicated commit_faulted_) on INTERNAL_ERROR, checked the same way at every entry point, so
runtime and restart handling of "the journal has a record we can't apply" are symmetric.
Separately: none of the corrupt-record injection tests added in this PR (CommitCsumShortArrayAborts,
RebuildOverlaySkipsNlbasZeroSlot, etc.) check what happens on a subsequent write()/read() call
after the injected corruption -- worth adding an assertion that commit_lsn stays frozen and the
failure keeps recurring, so this behavior (whichever way it's resolved) is pinned down in the suite.
szmyd
left a comment
szmyd
left a comment
There was a problem hiding this comment.
Blocking on one finding, inline at read()'s piggyback commit.
Most of what I would otherwise raise is already on this thread -- the overlay's single-version limitation and the read_impl/commit_impl interleaving in particular. What follows is a separate half of the first of those, and the thing worth saying about it is that the fix proposed there does not close it.
Two smaller items I looked at and did not file, so they are at least on the record: the blk_count_t narrowing in read_impl's contiguous-run merge (line 965 -- run_nlbas is bounded only by craft_max_io_len_mb / lba_size, which exceeds 65535 at a 512-byte page_size under the default cap), and commit_impl's is_empty branch skipping overlay retirement alongside the apply. Say the word if either is worth writing up properly.
| // Best-effort piggyback, same reasoning as write()'s: read()'s own success does not depend on | ||
| // whether commit() advances further, so a genuine commit() fault here is swallowed rather than | ||
| // failing an otherwise-servable read. | ||
| co_await commit(hdr.commit_lsn); |
There was a problem hiding this comment.
This advances the frontier past the horizon the same call is about to serve.
read() commits to hdr.commit_lsn here, then read_impl() snapshots state_.commit_lsn (line 903) and merges the index into the result. The overlay half is horizon-clamped (line 928, it->second.lsn > commit_lsn_snapshot && it->second.lsn <= read_lsn). The index half, line 934, is not clamped to anything.
The assumption is stated in the code and never checked, line 917:
// index's value (committed as of commit_lsn <= read_lsn's caller-assumed frontier) is used instead.read_lsn and hdr.commit_lsn arrive in the same frame with nothing coupling them, so read_lsn < commit_lsn is reachable straight from the wire -- and this line can produce the condition itself, by advancing the frontier past read_lsn before the snapshot is taken.
CRAFT-Design is unambiguous about what that should return: "The replica returns the latest version <= H for the range", and "writes above H are ignored even if the replica holds them". This is not only a malformed-frame concern. The read-eligibility deep-dive has the client deliberately drive H down: an unresolved slot M overlapping the range means the read either "clamps H below M (serving the old data, which is stable under both futures)" or routes around it. A clamped-H read is exactly the case served wrong here, and it is the case the protocol leans on to stay correct across an unresolved write's two possible futures.
Worth separating this from the overlay-versions point already raised on line 934. That one is the index having too little -- the fallback finds nothing and returns a hole. This is the index having too much, and retaining more per-LBA overlay versions does not touch it. It cannot: apply is blind overwrite by design -- "the index applies entries only at the contiguous commit frontier (commit_lsn), in dLSN order [...] Blind overwrite is then correct by construction" -- so once commit_lsn passes read_lsn, the pre-read_lsn version is gone from the index and is not reconstructible from anything this replica still holds.
Which leaves rejecting the read as the only correct answer. read_impl() already validates alignment, craft_max_io_len_mb, a negative read_lsn, and an empty dest.iovs, on the stated grounds that read() is "reachable straight from the client wire, so len is untrusted input". The same argument applies here, and this is the one where passing the check is what makes the returned bytes correct rather than merely well-formed. Either reject read_lsn < state_.commit_lsn, or clamp the piggyback to min(hdr.commit_lsn, read_lsn) so a read can never push the frontier past its own horizon. The first is the honest one: a horizon below an already-applied frontier is unanswerable, not merely awkward.
| co_return std::unexpected(make_error_condition(std::errc::invalid_argument)); | ||
| } | ||
| auto* dest_buf = static_cast< uint8_t* >(dest.iovs[0].iov_base); | ||
| std::vector< bool > is_hole(nlbas); |
There was a problem hiding this comment.
ack!
The constructor argument that sets the initial number of elements in the vector to the value stored in the variable nlbas. All elements are automatically initialized to false. C++ guarantees this.
C++: "Otherwise, the object is zero-initialized." link
I agree with you that for sake of clarity and style, it's better to initialize it
|
|
||
| for (uint32_t k = i; k < j; ++k) { | ||
| uint8_t const* lba_buf = dest_buf + k * lba_size_; | ||
| bool const all_zero = std::all_of(lba_buf, lba_buf + lba_size_, [](uint8_t b) { return b == 0; }); |
There was a problem hiding this comment.
nice ! Thank you
| for (uint32_t k = i; k < j; ++k) { | ||
| uint8_t const* lba_buf = dest_buf + k * lba_size_; | ||
| bool const all_zero = std::all_of(lba_buf, lba_buf + lba_size_, [](uint8_t b) { return b == 0; }); | ||
| if (all_zero) { |
There was a problem hiding this comment.
ack! nice catch
|
|
||
| // dest is a single flat iovec -- same assumption write()'s CRC computation already makes; every | ||
| // sg_list CRAFT builds anywhere in this codebase is a single iovec. | ||
| if (dest.iovs.empty()) { |
| // Advance the reclaim floor from ordinary IO -- same max-monotonic pattern as keep_alive(). | ||
| // Without this, all_committed_lsn only moves via keep_alive() messages, so quiet partitions | ||
| // (write-only, no explicit keep_alive) never advance the reclaim floor at all. | ||
| state_.all_committed_lsn = std::max(state_.all_committed_lsn, hdr.all_committed_lsn); |
There was a problem hiding this comment.
@sbinmalek shouldn't this state update (L465- L744) occur before L442?
Summary
Implements the S3 story:
commit(),keep_alive(), andread()onCraftReplDev— the machinery that advancescommit_lsn, applies journal entries to the index, and serves client reads from the journal-tail overlay and committed index without fetching from a peer.What's implemented (across 11 commits)
CraftJournalEntryblob format — per-LBA checksum array appended after the header;write()computes onecrc16_t10difper LBA at append timeHomeStoreCraftJournalBackend::read_slot()— real log-store read: validate, parse header, deserialize blkid, extract csumsVolumeIndexTable::delete_lba_range()— new method forall_zerosunmap on applystd::unordered_map<lba_t, OverlayEntry>covering(commit_lsn, last_append_lsn]; highest-dLSN-wins per LBA; populated onwrite(), rebuilt on restart, pruned ontruncate()and commitcommit(upto_lsn)— walks the journal in order, stalls at the first missing gap (never skips holes), applies each slot to the index (write_to_indexfor data,delete_lba_rangeforall_zeros), retires overlay entries only when their recorded LSN exactly matches the applied slotkeep_alive()— term check → watchdog reset →commit(hdr.commit_lsn)→ returns{commit_lsn, last_append_lsn}read()— term check → piggyback commit → snapshot watermarks → per-LBA resolution: overlay (horizon-clamped toread_lsn) wins over index; absent-from-both is a hole; read-time all-zero collapse; CRC verification; adjacent same-type extents mergedrebuild_overlay()— walks(commit_lsn, last_append_lsn]on restart, skips missing/empty LSNs (not a stop condition), populates overlay with the same highest-dLSN-wins rulewrite()usesiomgrtimer; reset on every successfulwrite()/keep_alive(); fires intoappend()when the session goes quiet;craft_watchdog_timeout_msconfig key (default 30 s, 0 = disabled)craft_max_io_len_mbconfig key (default 128 MiB) — enforced in bothwrite()andread()to bound unbounded-len attacks from malformed framesUnit test coverage
All tests in
test_craft_commit.cpp(light, no HomeStore) andtest_craft_commit_hs.cpp(real HomeStore) pass. Coverage maps to AC requirements:InOrderApply+WriteCommitReadRoundTrip(hs)ThreeWayMixedExtentReadInOrderApplyAfterHoleFillsMatchesNoGapRunRestartRecoveryGatesClientIoUntilOverlayRebuilt,RebuildOverlaySkipsNlbasZeroSlottest_craft_watchdog.cppAllZerosOverlayIsHole,AllZerosUnmapReclaimsRealBlock(hs)DataWriteOfAllZeroBytesCollapsesAtReadTimeNotWriteTimeCommitCsumShortArrayAbortsRebuildOverlaySkipsNlbasZeroSlotWriteCapturesAllCommittedLsn,ReadCapturesAllCommittedLsnReadDestEmptyIovsRejectedReadMaxIoLenEnforced,MaxIoLenEnforced(write)HorizonClampServesIndexNotOverlayAboveReadLsnOverlayWinsOverCommittedIndexForSameLbaCrcMismatchFailsTruncateRemovesOverlayEntriesAboveLsncommit_running_ConcurrentCommitIsNoOp,ConcurrentKeepAliveSerializesCommitAgainstRealIndex(hs)Not in scope (future stories)
SyncRSCommitLSNentry dispatch (watchdog fires into the existing stubappend())all_committed_lsn(captured here; reclaim is S8)CraftPartitionStatesuperblock recovery (overlay rebuild walk is empty until state recovery is wired)