scan: hash sparse files whose trailing hole ends at an unaligned EOF - #152
Merged
Conversation
Three different conditions printed the word "changed", with three different meanings and no shared vocabulary. The worst offender fires on any file that is still being written -- a download, a torrent, a log, a VM image -- where "file X changed" reads as damage rather than "come back later": file /srv/dl/temp/Show.S01E04.mkv changed It now says what was seen, why, and that it resolves itself: /srv/dl/temp/Show.S01E04.mkv: size changed while hashing (read 408944640 bytes, expected 4000000000). Skipped - it is probably still being written; the next run will hash it. Also split a counter that was reporting the wrong cause. dedupe_dest_differs was incremented both by our own pre-flight size check and by the kernel's FILE_DEDUPE_RANGE_DIFFERS verdict, then printed as one number labelled "changed since scan" -- so every kernel byte-compare mismatch was attributed to a file change that had not happened. A mismatch is also a property of the pair, not of the destination we happen to name. They are now counted apart, and the summary names only the causes that actually occurred rather than printing three numbers of which two are usually zero: Not deduped 3 changed since scan, 2 content mismatch, 1 failed The dedupe pre-flight message also called its second number "expected" when it is the offset the member must reach, not the size we scanned. Counters are deliberately not exported to --json/run_history here; that is issue #145's job and doing it now would collide with it. Verified by reproducing the scan-phase case: 4 GB file, cold page cache, truncated mid-hash. scripts/verify.sh passes (build, 120 tests, valgrind scan+dedupe+replay smoke). Co-Authored-By: Claude <noreply@anthropic.com>
A file ending in a hole was permanently skipped, on every run, if its size was not a multiple of the hashing block size: Show.S01E04.mkv: size changed while hashing (read 2013921280 bytes, expected 2014013223). Skipped ... Nothing had changed. hole_run_length() floors a hole run to a block boundary, which is right for an *interior* hole -- the block holding the first data byte must still be read whole -- but wrong for a trailing hole, where no data follows. Flooring left a final partial block that maps no data, so fill_buffer() capped its read at zero bytes, the loop exited with off < filesize, and csum_whole_file() concluded the file had changed under it and threw the digest away. The file was then never stored, so the next run repeated it, forever. It hit exactly the files sparseness is used for: preallocated torrent stubs, VM images, database files. The reporter's was a 2 GiB .mkv with 484 KiB allocated and a month-old mtime. Only the e == NULL (trailing) case changes; when EOF happens to be block aligned the new expression is identical to the old, which is why the existing coverage passed -- test_trailing_hole_scans truncates to 4 MiB, exactly 32 blocks. Both new tests fail without the fix. Co-Authored-By: Claude <noreply@anthropic.com>
Review pass over the two preceding commits.
- run_dedupe.c: the "Not deduped" line was three near-identical printf
branches chained by a `sep`, with the counter set named twice (guard and
branch). Drive it from a {count, label} table instead, so a new cause is
one row and the guard is derived rather than restated.
- run_dedupe.c: fold the new counter's comment into the existing block above
it rather than stacking two headers on one declaration group; drop the
changelog prose, which belongs in the commit message.
- file_scan.c: `next_data` had one use left after the trailing-hole early
return; inline it. Trim the comment that restated the message text below it.
- test_sparse.py: use the existing make_trailing_hole() helper instead of
open-coding write()+truncate(), matching its sibling test; shorten a
comment that restated hole_run_length()'s.
No behaviour change; scripts/verify.sh passes (122 tests, valgrind smoke).
Co-Authored-By: Claude <noreply@anthropic.com>
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
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.
The bug
A file ending in a hole was silently and permanently skipped — on every run, forever — if its size was not a multiple of the hashing block size. It was reported as having changed while being read, which was not true:
Nothing had changed. The file was a 2 GiB sparse stub with 484 KiB actually allocated and a month-old mtime. The read stopped exactly on a 128 KiB block boundary, 91943 bytes short of EOF — one partial block.
hole_run_length()floors a hole run to a block boundary. That is right for an interior hole, where the block holding the first data byte must still be read whole, but wrong for a trailing hole, where no data follows. Flooring left a final partial block that maps no data, sofill_buffer()capped its read at zero bytes, the loop exited withoff < filesize, andcsum_whole_file()concluded the file had changed under it and threw the digest away. Nothing was stored, so the next run repeated it.It hits exactly what sparseness is used for: preallocated torrent stubs, VM images, database files.
Only the
e == NULL(trailing) branch changes. When EOF happens to be block-aligned the new expression is numerically identical to the old — which is why existing coverage passed:test_trailing_hole_scanstruncates to 4 MiB, exactly 32 blocks.Why here and not at the EOF check
Relaxing
if (ctxt.off != ctxt.filesize)would only silence the message. The fix makesofflegitimately reachfilesizeand folds the true{offset, length}descriptor into the running checksum. Tolerating a short read instead would leave the final partial hole unaccounted, so two sparse files identical up to the last block boundary but differing in size would produce the same digest.Message clarity (how the bug became findable)
The old text was
file <path> changed— no reason, no numbers, no consequence. It was the numbers in the reworded message that identified the block-boundary stop and led to the root cause.Three different conditions used the word "changed" with three different meanings. They now each state what was observed and what follows from it.
A counter was also reporting the wrong cause.
dedupe_dest_differswas incremented both by our own pre-flight size check and by the kernel'sFILE_DEDUPE_RANGE_DIFFERSverdict, then printed as one number labelledchanged since scan— so every kernel byte-compare mismatch was attributed to a file change that had not happened. A mismatch is also a property of the pair, not of the destination we happen to name. They are now counted apart, and the summary names only the causes that occurred:Counters are deliberately not exported to
--json/run_historyhere — that is #145's job and doing it now would collide.Testing
Two regression tests in
test_sparse.py: an unaligned trailing hole, and a fully-sparse unaligned file. I verified both fail without the fix and pass with it, and that every pre-existing test passes either way.Also verified against the real file that triggered this (read-only
-rscan): warning gone, file hashed and recorded.scripts/verify.shpasses — build with warnings-as-failure, 122 tests, valgrind scan+dedupe+replay smoke.A
/simplifypass is included as the last commit: the summary line is table-driven rather than three copy-pasted branches, and some comments trimmed. No behaviour change.Note for release
Worth a release note — anyone with sparse files has been silently losing them from dedupe, with a message that pointed at the wrong cause.
🤖 Generated with Claude Code