dd: page-align the copy buffer used for direct I/O#13373
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
ff5279f to
d57f06a
Compare
|
Updated the description to properly credit #12143, which I had missed when opening this (my issue search didn't match its title). See the "Relation to existing PRs" section. |
Merging this PR will degrade performance by 3.24%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | uniq_check_chars[(10000, "1")] |
14.4 ms | 14.9 ms | -3.24% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing relative23:dd-odirect-buffer-alignment (3ca0248) with main (4977964)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
|
Thanks for the report — breakdown of the two kinds of deltas: Memory (all six: exactly +4.0 KB): this is the intentional over-allocation by one page that provides the alignment guarantee ( Simulation (−4…−5%): per-iteration bookkeeping of the new buffer wrapper (capacity check + slice re-borrow), which adds up at
Worth keeping in mind when weighing the remaining instruction-count delta: these benchmarks don't use direct I/O, and CodSpeed doesn't measure syscall cost — while the bug this PR fixes currently makes every full-block (The Android job failure is unrelated — it died in "Create and cache emulator image".) |
|
Verified the fast path against the gate's own metric (instruction counts, single-pass analysis-mode bench binaries under
Down from the reported 4–5% to well under 1.5% (≈8 instructions per copied block, from the remaining slice re-borrows). The constant +4 KiB in the memory-mode numbers is the page over-allocation that provides the alignment guarantee itself. |
|
Round 3, following up on the flame graphs from the last run: Rather than keep shaving at wrapper overhead, b859333 removes it structurally by consolidating with the read path from #12143 (credit to @chrboe — this is essentially his slice-based design on top of the safe runtime-page-size allocation):
The diff is net −75 lines. This run's CodSpeed result reflects it: no simulation benchmark is flagged anymore (previously
That leaves the headline number (−16.32%), which is now purely the seven Memory entries averaged: peak memory is up by exactly one page (+4,096 B, constant, not proportional to the buffer size) because the buffer over-allocates one alignment unit to place the data page-aligned. That is the fix itself — the same trade GNU dd makes via Re-verified after the refactor: The other two failing checks look unrelated to this diff: the macOS job failed on a |
b859333 to
5868242
Compare
|
Round 4: the seven Memory flags are gone as well — 5868242 removes the one-page over-allocation instead of asking for it to be acknowledged.
Being explicit about the trade-off, since the PR previously advertised "no unsafe": this adds three small unsafe blocks ( Side effect on the hot loop:
Re-verified after the change: 91 unit + 123 integration tests pass, outputs byte-identical to main across the 18-configuration sweep, and on a |
|
Hardened the buffer internals a bit further ahead of review: One correction to my earlier phrasing for precision: the unsafe surface is four |
804cb6f to
79bc8d5
Compare
|
i stopped reviewing it because i became tired ;) A few comments:
I will stop there too but maybe i should start providing an AGENTS.md and skills to help contributors |
8624f83 to
42f8ea2
Compare
42f8ea2 to
95d9fe9
Compare
|
Reworked this around a safe, |
The copy buffer was aligned only for iflag=direct, so oflag=direct on its own handed a misaligned buffer to the output descriptor. Every block then took the EINVAL retry in handle_o_direct_write, which clears O_DIRECT, writes buffered and restores the flag, so oflag=direct performed no direct I/O at all. Measured on a loop device with dma_alignment=511, copying 4 MiB with bs=64K oflag=direct: 128 fcntl(F_SETFL) calls before, none after.
Fixes #12085.
Problem
Vec<u8>does not guarantee the alignmentO_DIRECTrequires, sodd iflag=directfails withEINVALon devices with a strict DMA alignment (sd,loop,virtio_blk). It is invisible on NVMe, wheredma_alignmentis 3.The write side is affected too, but silently.
handle_o_direct_writecatches theEINVAL, clearsO_DIRECTwithfcntl(F_SETFL), writes buffered and restores the flag, once per block, sooflag=directperformed no direct I/O at all.Fix
Use a safe
Vec-backed buffer that exposes a page-aligned fixed-size slice, and request that alignment when eitheriflag=directoroflag=directis given. Read helpers return the valid prefix instead of resizing the buffer, and the initial buffer size respectscountwhen less data is requested. This adds no dependency and nounsafecode.Scope
Writes that go through
BufferedOutput, which is the case wheneverbs=is absent or anyconv=is given, still stage through an unaligned buffer and keep taking the fallback. Fixing that touchesbufferedoutput.rsand the conversion scratch buffer, so it is a separate change that I will open once this one lands.Validation
cargo test -p uu_dd, plus the dd integration suite including FIFO partial-read coverage and the bounded-allocation regressiondma_alignment=511:iflag=directfails before the change and succeeds after it, andbs=64K oflag=directdrops from 128fcntl(F_SETFL)calls to nonerecords in/outuniq, which this PR does not touch, and CodSpeed reports it as compared across different runtime environments.