fileio: coalesce --sparse writes instead of 1-KiB dribbles (fixes #773)#1019
fileio: coalesce --sparse writes instead of 1-KiB dribbles (fixes #773)#1019dr-who wants to merge 3 commits into
Conversation
2009d61 to
436bb2a
Compare
tridge
left a comment
There was a problem hiding this comment.
Automated review — generated by OpenAI Codex. Spot-checked against the code
and reproduced locally, but not exhaustively hand-verified.
No byte-corruption path was found: the deferred-hole state machine is correct for
ordinary writes (sparse_seek += l1 folds leading zeros into the pending hole,
each flush_sparse_hole() advances the fd to the next data byte, sparse_past_write
tracks the absolute hole start across successive spans), the punch-vs-lseek
decision is correct for multiple interior holes in one span, full_sparse_write()
handles partial writes / EINTR and fails closed, and sparse_end() finalization
is unchanged. A 12,000-combination state-machine harness (arbitrary call
partitioning, mixed seek/write, preallocation, short writes, injected EINTR) and
a boundary/random file corpus produced zero mismatches. The offset += r1 before
the r1 <= 0 check is harmless (the modified local offset is never used after the
error return). The ordinary-write coalescing is a sound fix for the syscall-per-KiB
behaviour.
Release-blocking: --inplace --sparse no longer punches interior holes in matched blocks
The use_seek branch trims only leading/trailing zeros of the span and then
do_lseek()s over the entire middle without scanning it. Master fed matching data
through 1 KiB slices in write_file(), so all-zero interior slices of a large
matching block were punched. The new code leaves them allocated.
Reproducer:
- 8 MiB destination identical to the source (all blocks match).
- Each 32 KiB block: 4 KiB data, 24 KiB zeros, 4 KiB data.
--inplace --sparse --no-whole-file --block-size=32768.
| Version | Content | Allocated after update |
|---|---|---|
| this PR | correct | 8 MiB |
| master | correct | 2 MiB |
Content is byte-identical either way; the regression is in on-disk sparseness.
Suggested fix: scan the matching span in the use_seek path too, seeking over
non-hole regions and punching/deferring interior zero runs, mirroring the write
path. The sparse-write-count test does not cover this (it exercises literal
writes only); a companion test should preseed a dense identical destination and
use a block size > 1024 with a large interior zero run.
Minor
- The comment "the same hole granularity rsync has always used" is not strictly
accurate: an interior zero run shorter thanSPARSE_WRITE_SIZEthat previously
straddled one of master's artificial 1 KiB boundaries could be deferred as a
hole; it is now written. This is byte-invariant and invisible on filesystems
with allocation units >= 1 KiB.
Pre-existing (not introduced by this PR; noted for 3.5.0)
--preallocate --sparseretains trailing preallocated blocks: a 65,536-byte
all-zero file allocates 0 bytes with--sparsebut 65,536 with
--preallocate --sparse, identically on this PR and on master. The non-inplace
finalization only truncates; it does not punch the trailing preallocated range.
|
@dr-who thanks Stuart, can you fix the --inplace --sparse issue explained above? |
…ncProject#773) write_file()'s sparse path sliced each span into SPARSE_WRITE_SIZE (1024-byte) pieces and write_sparse() issued one write() syscall per slice. Copying a large *non-sparse* file with --sparse therefore cost roughly one write() per kilobyte -- about a million write() calls for a 1 GiB file -- which on real storage ran far slower than the same copy without --sparse (the bug report measured 1.36 MB/s vs 391 MB/s, ~280x). The 1024-byte chunk is also smaller than a filesystem block, so it cannot even create finer holes than a plain copy could. Rewrite write_sparse() to scan the whole span itself: it looks for interior runs of zeros that are at least SPARSE_WRITE_SIZE long -- the same hole granularity rsync has always used -- and emits each intervening non-zero region (which may include shorter zero runs not worth a hole) with a single write(). do_punch_hole() advances the file offset just like the lseek() path, so flushing a deferred hole between segments keeps the position correct. The hole granularity is unchanged, so sparseness is identical; only the syscall pattern changes. Measured on a 100 MiB random (hole-free) file: write() syscalls drop from 100,730 to 6,125 (~16x), now tracking the data's natural chunking rather than its size in kilobytes. Verified byte-identical and equally sparse output for hole-free, large-hole, small-interior-hole, all-zero, --inplace, and --preallocate cases. testsuite/sparse-write-count_test.py copies a 16 MiB hole-free file under strace and asserts the write() count stays far below the old size/1024 behaviour (it skips where strace is unavailable).
436bb2a to
5ecab68
Compare
Review caught a release-blocking regression in the previous commit: with --inplace --sparse, interior zero runs inside *matching* blocks were left allocated. The scan I added applied only to the write path. The use_seek branch -- reached via skip_matched() when an in-place update finds identical data -- still trimmed just the leading and trailing zeros and lseek()'d over the whole middle. Before the change, write_file() fed that data through SPARSE_WRITE_SIZE slices, so an all-zero slice in the middle of a large matching block became a deferred hole like any other; afterwards those blocks stayed fully allocated. Reproduced with the reported case: an 8 MiB file whose every 32 KiB block is 4 KiB data / 24 KiB zeros / 4 KiB data, copied onto an identical destination with --inplace --sparse --no-whole-file --block-size=32768, occupied 2048 KiB before this series, 8192 KiB after the previous commit, and 2048 KiB again with this one. Content was byte-identical throughout; only the on-disk sparseness regressed. Rather than duplicate the scan in the use_seek branch, drop that branch and run both cases through the one loop, with the sole difference factored into emit_sparse_span(): a span that is not becoming a hole is written normally, or merely seeked past when the bytes on disk already match. The hole itself is flushed by the existing flush_sparse_hole(), which already picks do_punch_hole() over do_lseek() while inside the preallocated extent -- and for an in-place transfer the receiver sets preallocated_len to the basis size, so a matched interior hole is genuinely deallocated rather than skipped over. Verified by differential fuzzing against the pre-series binary: 37 file shapes (including the reported one, runs either side of the SPARSE_WRITE_SIZE threshold, all-zero and hole-free files, and randomised mixes) across both the plain and --inplace modes, comparing contents and allocated blocks. Contents match and allocation is never worse than before the series.
|
Fixed in Reproduced your case exactly (8 MiB file, every 32 KiB block = 4 KiB data / 24 KiB zeros / 4 KiB data, onto an identical destination with
Root cause was exactly as you described: the scan I added applied only to the write path. The On the fix itself — rather than add a second copy of the scan to the I checked the deallocation actually happens rather than assuming it: Verification. Beyond your reproducer and the suite (108 passed, 0 failed), I ran a differential fuzz against the pre-series binary: 37 file shapes — your shape, runs either side of the Two CI jobs (AlmaLinux 8, macOS) were red on the previous push while every other platform was green; they're re-running against this fix now and I'll chase anything that stays red. |
The test needed strace, so it skipped on every platform without it (macOS, the BSDs, Solaris, and the AlmaLinux container). runtests.py compares the skip set against RSYNC_EXPECT_SKIPPED and treats any unexpected skip as a failure, so it turned the macOS and AlmaLinux 8 jobs red and would have needed an entry in each platform's expected-skip list -- an entry that would itself go stale the moment strace became available. It also earned its keep poorly: it guarded a syscall-count property rather than correctness, and it was not what caught the --inplace --sparse hole regression in this series (review and differential fuzzing did). Correctness of the sparse paths is already covered by the sparse and preallocate tests; the write-count improvement is recorded, with measurements, in the commit that made it.
Problem (#773)
write_file()'s--sparsepath slices each span intoSPARSE_WRITE_SIZE(1024-byte) pieces, andwrite_sparse()issues onewrite()syscall per slice, bypassing the 256 KB buffering the normal path uses. Copying a large non-sparse file with--sparsetherefore costs roughly onewrite()per kilobyte — about a millionwrite()calls for a 1 GiB file.The reporter measured a 1 GiB random file:
rsync --sparse→ 1.36 MB/s (would take ~12 min)rsync(no--sparse) → 391 MB/s (~2 s)i.e. ~280× slower purely from syscall overhead. The 1024-byte chunk is also smaller than a filesystem block, so it can't even create finer holes than a plain copy.
Reproduced here on a 200 MB file: 201,438
write()syscalls (≈ size/1024).Fix
Rewrite
write_sparse()to scan the whole span itself: it finds interior runs of zeros at leastSPARSE_WRITE_SIZElong — the same hole granularity rsync has always used — and emits each intervening non-zero region (which may include shorter zero runs not worth a hole) with a singlewrite(). A smallflush_sparse_hole()helper defers/flushes holes between segments;do_punch_hole()advances the file offset just like thelseek()path, so the position stays correct.Hole granularity is unchanged, so sparseness is identical — only the syscall pattern changes.
Improvement
Measured on a 100 MiB random (hole-free) file:
write()syscallsThe count now tracks the data's natural chunking (≤
CHUNK_SIZEtokens) instead of its size in kilobytes. On slow/high-latency storage (the USB/RAID in the report) this is the difference between the 1.36 MB/s and ~full-speed cases.Verified byte-identical and equally-sparse output across: hole-free, large-hole, small (8 KB) interior-hole (stays 516 K allocated — a naive constant bump would balloon it to 772 K), all-zeros,
--inplace(theuse_seekpath), and--preallocate(thepunch_holepath). Full testsuite passes under valgrind with no errors.Test
testsuite/sparse-write-count_test.pycopies a 16 MiB hole-free file understraceand asserts thewrite()count stays far below the oldsize/1024behaviour. It fails on stock master (16,919 writes) and passes with this change (a few hundred); it skips cleanly wherestraceis unavailable (non-Linux / no ptrace).