Skip to content

SDSTOR-20656: clear wbc chunk selector - #177

Closed
nnastonen wants to merge 4 commits into
eBay:dev/v6.xfrom
nnastonen:reset_block_allocator
Closed

SDSTOR-20656: clear wbc chunk selector#177
nnastonen wants to merge 4 commits into
eBay:dev/v6.xfrom
nnastonen:reset_block_allocator

Conversation

@nnastonen

@nnastonen nnastonen commented Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown
  1. Create homestore with chunk selector
  2. Create btree1 and populate it
  3. Nodes get into wbc cache
  4. Destroy btree1 (wbc purge is not done, also volume's chunk bitmap is not cleared)
  5. Create btree2 and populate it
  6. Nodes get into wbc cache and we get cache corruption, duplicate inserts

This change makes chunk release asynchronous and defers returning the chunk to the free pool until cleanup is complete.

New flow:

  • detach the volume's chunk list from m_volume_chunks
  • run asynchronous cleanup for all the chunks
  • in cleanup, evict all allocated blkids for the chunks from WBC
  • [reset the chunks' block allocator state (bitmap)]
  • reinsert the chunks into m_per_dev_chunks

@nnastonen nnastonen changed the title SDSTOR-25404: clear wbc chunk selector SDSTOR-20656: clear wbc chunk selector Sep 10, 2026
@nnastonen
nnastonen force-pushed the reset_block_allocator branch from 5c20f36 to f617e74 Compare September 10, 2026 14:13
sbinmalek and others added 3 commits September 10, 2026 07:20
@sbinmalek @nnastonen
craft: S5 RAFT entry apply — SyncRSCommitLSN + InternalLogin (SDSTOR-…
01ff1c0 
…22886, SDSTOR-22887) (#176)

* SDSTOR-22886 craft: SyncRSCommitLSN RAFT entry apply
Implement the apply side of the SyncRSCommitLSN RAFT entry
@shosseinimotlagh @nnastonen
S3: Commit + Read path: advance commit_lsn, serve reads, watchdog (#175)
627b5e1 
* craft: S3 commit + read path — advance commit_lsn, serve reads, watchdog

Squashed from 11 incremental commits (checksum-array blob format, real read_slot(),
VolumeIndexTable::delete_lba_range, index handle + journal-tail overlay, commit()
wired into write()/keep_alive(), CraftReplDev::read(), client-liveness watchdog,
heavy integration + concurrency test coverage, overlay rebuild on restart, and two
rounds of robustness-gap fixes) into one commit for a clean rebase onto the updated
dev/v6.x base (PR #176 / S5 landed upstream).

Implements S3 (SDSTOR-22733): CraftReplDev::commit() advances commit_lsn by applying
journaled entries to the index in strict dLSN order, stalling (not erroring) at the
first gap; keep_alive() and write() both piggyback it. CraftReplDev::read() serves
[addr, addr+len) as of a client-supplied horizon from the index or the journal-tail
overlay (never fetches from a peer), CRC-verifying every LBA and collapsing all-zero
content to holes at read time. A per-partition watchdog timer proposes a
SyncRSCommitLSN entry when a client goes quiet. rebuild_overlay() repopulates the
overlay from the journal on restart. Full unit + HomeStore-integration + concurrency
test coverage across test_craft_commit.cpp, test_craft_commit_hs.cpp,
test_craft_concurrency.cpp, test_craft_homestore_backend.cpp, and test_craft_watchdog.cpp.

* craft: fix 13 review findings from PR #175 (horizon, TOCTOU, block leaks, validation)

Addresses sbinmalek/Copilot/szmyd/independent-agent review findings on PR #175, all
build-verified (100% tests passing, 12/12 suites).

szmyd (blocking) — read()'s piggyback commit can advance commit_lsn past its own
read_lsn before read_impl() snapshots it, serving a too-new value with no error. Added
volume_error::HORIZON_STALE; read_impl() rejects read_lsn < commit_lsn_snapshot instead
of silently serving the too-new value.

Residual TOCTOU in read_impl — the horizon check above only guards the snapshot taken
BEFORE the unlocked index query; a concurrent commit_impl can still advance commit_lsn
while that query runs. Re-snapshot and re-check after the index read closes this second
instance of the same class of bug.

Copilot — old_blkid == new_blkid frees a live index block on crash-replay of an
already-applied slot (index already holds the exact blkid the replay reports as "old").
Skip the free when they match.

Copilot — read_slot() never cross-checked the on-disk record's self-describing hdr.lsn
against the seq_num actually requested; a corrupt/misplaced record was silently
trusted. Now rejected.

sbinmalek/Copilot — the all-zero read-time collapse ran before the CRC check, so a
bit-flip that zeroed real data was reported as a hole instead of CRC_MISMATCH. CRC is
now computed unconditionally before the collapse decision.

Copilot — read_slot() derived nlbas from hdr.len without validating alignment/non-zero,
letting a corrupt record silently misalign the csum-array and blkid offsets that
follow. Validated up front.

Copilot — write()'s ack snapshotted state_ BEFORE the piggyback commit() call, so a
successful write's own commit progress was never reflected in its own response.
Re-snapshot after.

Independent-agent finding — hdr.all_committed_lsn (client-controlled) had no
validation; a malformed negative value (not the -1 "unset" sentinel) could corrupt this
long-lived floor. Now ignored. An unbounded-positive-value poisoning vector remains --
no valid per-call bound exists without breaking the legitimate lagging-replica catch-up
signal; documented as a TODO(S8) on the field itself, since only S8's real reclaim
implementation has the context to validate it.

sbinmalek/Copilot — dest iovec capacity in the read path only checked for emptiness,
not whether iov_len actually covered the requested range; an undersized buffer would be
written past its end.

Copilot — write() allowed a multi-iovec sg_list whose iovs[0] alone happened to satisfy
the size/alignment checks, carrying dead trailing iovecs. Rejected as hygiene (the
CRC-corruption scenario Copilot originally described was already closed by the
pre-existing iov_len check).

szmyd (unfiled) — a single contiguous read run could exceed blk_count_t's max (65535
LBAs) at small lba_size/large craft_max_io_len_mb, silently truncating in the
run_blkid construction. Capped the run-extension loop at that limit; larger runs now
split into multiple read_data batches (invisible to the caller -- adjacent same-type
extents still merge).

szmyd (unfiled) — commit_impl's is_empty branch skipped overlay retirement entirely,
including the case where THIS replica already journaled real data for an lsn the
cluster-wide resolution still verdicts Empty ("Empty beats data" reconciliation). Left
a live overlay entry that would keep being served on reads, contradicting the verdict.
Now attempts a best-effort read_slot() purely to discover the LBA range to retire, even
when skipping the index apply.

Independent-agent finding (S4 scope, not S3, bundled here per review discussion) —
truncate() never freed the data blocks referenced by journal entries it rolled back,
permanently leaking them on every login that drops a stale tail. Reads each dropped
entry's blkid before the rollback destroys the record, frees it after.

Also: two review comments acked but never actually implemented -- explicit
is_hole(nlbas, false) initialization (style, sbinmalek) and a shortened TODO-style
comment on the logout/watchdog interaction (sbinmalek's "leave a TODO instead of a
large comment" request).

Deferred to a follow-up PR (design discussion in progress with szmyd): the overlay's
single-version-per-LBA limitation, a read_impl/commit_impl atomicity gap distinct from
the two fixed above, and asymmetric failure handling for a corrupt journal record hit
live vs. at restart.

* build: bump version to 6.0.8

* craft: fix build breakage from S5 (PR #176) rebase integration

S5 landed independently on dev/v6.x with no visibility into S3's own changes (and vice
versa), so several integration points compiled/linked/ran incorrectly even after the
rebase's textual conflicts were resolved -- none of these were merge conflicts, they were
semantic mismatches between the two branches' independent designs:

- HomeStoreCraftJournalBackend::free_slot (S5) computed the blkid offset assuming the
  pre-S3 [header][blkid] blob layout; S3 inserted a csum array in between
  ([header][csums][blkid]). Fixed to skip past the csum array via blkid_offset(nlbas),
  same as read_slot() already does.
- apply_sync_rs_commit_lsn's (S5) own write_slot call site never passed csums (added by
  S3 after S5 branched) -- now passes the fetched slot's own slot.csums.
- CraftReplDev moved from a public constructor to a private constructor + create()
  factory (S5, for shared_from_this()) with S3's lba_size/indx_tbl parameters folded in.
  Four S3-only test files never conflicted during the rebase (S5 never touched them) but
  still called the old make_unique<CraftReplDev> pattern directly: test_craft_watchdog.cpp,
  test_craft_commit.cpp (two call sites), test_craft_concurrency.cpp, test_craft_commit_hs.cpp.
  All switched to CraftReplDev::create(), with dev_ members changed from unique_ptr to
  shared_ptr to match create()'s return type.
- Three MockCraftJournalBackend mocks (test_craft_commit/concurrency/watchdog) never
  implemented S5's new free_slot pure virtual, making them abstract classes; added stub
  overrides. test_craft_raft_entries.cpp's own mock was also missing S3's read_data pure
  virtual and had a stale 6-arg write_slot override (no csums parameter) -- both fixed.
- test_craft_raft_entries's CMakeLists.txt target was missing the generated
  HB_CONFIG_BINDUMP source and its core-library dependency, causing an undefined
  reference to home_blks_config_fbs at link time (every other light CRAFT test target
  already needed this before S5's own tests ever required HB_DYNAMIC_CONFIG).
- test_craft_raft_entries.cpp's main() never called SISL_OPTIONS_LOAD -- harmless before
  S3's watchdog work made CraftReplDev's constructor unconditionally read
  HB_DYNAMIC_CONFIG(craft_watchdog_timeout_ms), which segfaults against an uninitialized
  settings registry. Added SISL_OPTIONS_LOAD plus craft_watchdog_timeout_ms=0 (this suite
  never starts iomgr, so a live recurring timer would crash it regardless).

All 13 test suites pass post-fix (100%), including S5's own CraftRaftEntriesTest (28
tests) running for the first time alongside every S3 fix from this review pass.

* craft: fix clang-format violation in test_craft_raft_entries.cpp

CI's GccAddressSanitize job runs a formatting check against origin/dev/v6.x for
PR-modified files and failed on this one line -- pre-existing from the rebase's
auto-merge (never touched by any of this session's actual edits to this file, just
caught in the same formatting-check scope since the file was modified). Joins the
JournalSlot initializer onto one line, matching clang-format -style=file's output.

* bump craft client
@nnastonen
asdf
2ad2ab7
@nnastonen
nnastonen force-pushed the reset_block_allocator branch from b008726 to 2ad2ab7 Compare September 10, 2026 14:21
@nnastonen nnastonen closed this Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szmyd szmyd Awaiting requested review from szmyd
@shosseinimotlagh shosseinimotlagh Awaiting requested review from shosseinimotlagh
@xiaoxichen xiaoxichen Awaiting requested review from xiaoxichen
@JacksonYao287 JacksonYao287 Awaiting requested review from JacksonYao287

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nnastonen @sbinmalek @shosseinimotlagh