S3: read+commit - follow up - #178
Closed
shosseinimotlagh wants to merge 1 commit into
Closed
Conversation
…ay race Independent post-eBay#175 review findings (build-verified, 26/26 test suites pass): - free_slot() was missing the same magic/version/hdr.lsn==lsn/hdr.len validation read_slot() already has -- a corrupt record could deserialize a garbage blkid and free the wrong blocks. - write()'s post-flight block erased dlsn from missing_lsns_ and released missing_mu_ before calling populate_overlay(), letting a concurrent commit_impl (another write's/keep_alive's piggyback) apply and retire dlsn before the overlay entry existed -- leaving a permanently-orphaned (harmless but never-retired) overlay entry. Both mutations now happen under the same missing_mu_ critical section commit_impl's is_missing check also takes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #175 (S3: Commit + Read path, SDSTOR-22733). Two of the items found by an independent post-merge review pass are fixed here; the rest are tracked as design-decision items under a new story, SDSTOR-25614, rather than blocking on them here.
Fixed in this PR
free_slot()was missing the magic/version/hdr.lsn==lsn/hdr.lenvalidation thatread_slot()already has right next to it — a corrupt on-disk record could deserialize a garbage blkid and free the wrong blocks.read_slot()uses.write()'s post-flight block eraseddlsnfrommissing_lsns_and releasedmissing_mu_before callingpopulate_overlay(). A concurrentcommit_implrun (anotherwrite()'s orkeep_alive()'s piggyback) could seedlsnalready resolved, apply + retire it, and advancecommit_lsnpast it — all before this call got around to inserting the overlay entry. Net effect: a permanently-orphaned (harmless, but never-retired) overlay entry until the same LBA is next overwritten.missing_mu_critical section thatcommit_impl'sis_missingcheck also takes —commit_impleither still seesdlsnas missing (stalls) or sees it resolved with the overlay already populated; the in-between state no longer exists.Tracked, not fixed here (needs a design decision)
Filed under SDSTOR-25614 ("CRAFT S3 follow-up: overlay versioning, read/commit atomicity, and corrupt-record failure handling"), carried over from #175's review:
read_impl/commit_implnon-atomicity: a read's index-check and overlay-check can straddle a concurrent commit, returning stale data with a fresh watermark.commit_impl'sis_emptysnapshot is stale-able by a concurrentapply_sync_rs_commit_lsnrun, letting Empty-verdicted data get applied to the index.Out of scope (separate ticket, different story's code)
SDSTOR-25620 —
apply_sync_rs_commit_lsn(S5, PR #176) advancescommit_lsnfor peer-fetched/resolved slots without ever applying them to the index or overlay, so a lagging replica's catch-up path can leavecommit_lsnahead of what's actually servable — subsequent reads of those LBAs return holes. Confirmed via diff that this PR (and #175) never touched that function's logic. Filed as a standalone bug against S5/S6, not a subtask of the S3 follow-up story.Testing
Built and ran the full suite on
sdsbuild06(Debug, C++23): bothcraft_client(13/13) andhomeblocks(13/13) test binaries pass, including all CRAFT suites — no regressions from these two fixes.