dd: align the I/O buffer to the page size for O_DIRECT#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 not alter performance
Comparing Footnotes
|
|
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
| use std::ptr::{self, NonNull}; | ||
| use std::slice; | ||
|
|
||
| /// A fixed-size, zero-initialized byte buffer aligned to a given power of two. |
There was a problem hiding this comment.
I guess this long comment is AI generated
could you please make it shorter ?
stuff like
/// The buffer is allocated directly with the requested alignment via
/// [std::alloc], so it is exactly len bytes — no memory overhead compared
/// to a Vec<u8> of the same size.
isn't super interesting
| if !alignment.is_power_of_two() { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "buffer alignment must be a power of two", |
There was a problem hiding this comment.
needs to use the translate!() macro
There was a problem hiding this comment.
Agreed. The safe replacement should remove this error path; otherwise I will add a locale key.
| let raw = unsafe { alloc::alloc_zeroed(layout) }; | ||
| match NonNull::new(raw) { | ||
| Some(ptr) => { | ||
| debug_assert_eq!(ptr.as_ptr().addr() % layout.align(), 0); |
There was a problem hiding this comment.
we rarely use debug_assert_eq in this project
is it really interesting ?
| if self.layout.size() != 0 { | ||
| // SAFETY: `ptr` was returned by `alloc_zeroed` for exactly | ||
| // `self.layout` and has not been freed before (`self` owns it). | ||
| unsafe { alloc::dealloc(self.ptr.as_ptr(), self.layout) }; |
There was a problem hiding this comment.
do we really need unsafe ?
can't we use something from rustix or another crate to avoid this ?
There was a problem hiding this comment.
rustix::mm::mmap is itself unsafe in rustix 1.1.4. I can use safe memmap2::MmapMut::map_anon, already a workspace dependency, or restore the safe over-allocated Vec. Which would you prefer? I also proposed splitting the probe/strace tests in the PR discussion.
| /// Reads in increments of 'self.ibs'. | ||
| /// The start of each ibs-sized read follows the previous one. | ||
| fn fill_consecutive(&mut self, buf: &mut Vec<u8>) -> io::Result<ReadStat> { | ||
| /// Returns the read statistics and the number of valid bytes in `buf`. |
There was a problem hiding this comment.
please use the rustdoc format
thanks
| /// Reads in increments of 'self.ibs'. | ||
| /// The start of each ibs-sized read is aligned to multiples of ibs; remaining space is filled with the 'pad' byte. | ||
| fn fill_blocks(&mut self, buf: &mut Vec<u8>, pad: u8) -> io::Result<ReadStat> { | ||
| /// Returns the read statistics and the number of valid bytes in `buf` (reads plus padding). |
| fn probe_direct_io(at: &uutests::util::AtPath, write: bool) -> DirectIo { | ||
| use std::os::unix::ffi::OsStrExt; | ||
|
|
||
| let page_size = usize::try_from(unsafe { libc::sysconf(libc::_SC_PAGESIZE) }).unwrap(); |
There was a problem hiding this comment.
can we avoid the unsafe here?
| // EOPNOTSUPP (== ENOTSUP on Linux): explicitly not supported. | ||
| return match err.raw_os_error() { | ||
| Some(e) if e == libc::EINVAL || e == libc::EOPNOTSUPP => DirectIo::Unsupported, | ||
| _ => panic!("unexpected error opening the O_DIRECT {direction} probe: {err}"), |
There was a problem hiding this comment.
please read the contribution doc (or pass it to your LLM)
we have a don't panic policy
https://github.com/uutils/coreutils/blob/main/CONTRIBUTING.md#dont-panic
There was a problem hiding this comment.
I have read the policy. I will remove the new panic and unwrap paths from this PR.
| } | ||
|
|
||
| let mut buf: *mut libc::c_void = std::ptr::null_mut(); | ||
| let rc = libc::posix_memalign(&raw mut buf, page_size, page_size); |
There was a problem hiding this comment.
please try to use rustix
There was a problem hiding this comment.
Agreed. If the probe remains, I will replace the libc calls with rustix; otherwise it will move to a follow-up.
|
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 |
|
Thanks, understood. Before reworking this: should I keep #13373 to the minimal alignment fix and move the probe/strace tests to a follow-up? |
8624f83 to
42f8ea2
Compare
42f8ea2 to
95d9fe9
Compare
Fixes #12085.
dd iflag=directcurrently fails immediately withdd: IO error: Invalid inputwhen reading from any block device whosequeue/dma_alignmentis stricter than the allocator's default alignment — which is most of them (sd,loop,virtio_blk,nbd,zram, … report511; NVMe reports3, which is why the bug does not reproduce there). Downstream impact: LXD has started working around this (canonical/lxd#18429).Root cause
The copy loop's I/O buffer is a plain
Vec<u8>; the allocator only guarantees small alignment for it (16 bytes with glibc):The write path has the same misalignment, but it is masked by
handle_o_direct_write(): every full-block write first fails withEINVAL, is then retried buffered withO_DIRECTstripped viafcntl, and the flag is restored afterwards — three extra syscalls per block, whileoflag=directsilently does no direct I/O at all:Fix
Allocate the copy-loop buffer page-aligned, matching GNU dd (
ibuf = ptr_align (…, page_size)).AlignedBufferallocates exactlybsizebytes at the runtime page size viastd::alloc::alloc_zeroed(Layout)— no memory overhead over the previousVec<u8>of the same size, and the alignment covers the strictest DMA alignment Linux block drivers advertise in practice (O_DIRECTcan additionally constrain I/O length and file offset, which follow from the chosen block sizes rather than from the buffer). No new dependencies.The unsafe surface consists of four small, documented expressions covering three responsibilities — allocation, deallocation, and shared/mutable slice construction; the pointer is owned by
AlignedBufferalone, freed with the sameLayoutit was allocated with, and never handed toVecor any other allocator-aware container — which is precisely what made the earliermemalign+Vec::from_raw_partsattempt (#9104) unsound. The unit tests pass under Miri.The copy loop reads into a prefix of that fixed-size buffer through a plain
&mut [u8]taken once outside the loop, adopting the slice-based read-path structure of #12143:fill_consecutive/fill_blocksreport the number of valid bytes instead of truncating the buffer, andconv=block/unblockoutput goes to a separate scratchVec, since those conversions can change the byte count. This keeps the hot loop free of per-iteration buffer bookkeeping (see #11544).With this change, full-block
oflag=directwrites also no longer take theEINVALfallback path; the fallback keeps handling the final partial block, as GNU does. As a drive-by, thefill_blockspadding now usesslice::fillinstead of allocating a paddingVecand splicing it in.Relation to existing PRs
I only found these after opening this PR — apologies for the overlap:
#[repr(align(4096))]page approach. This PR adopts its slice-based read-path structure (no per-iteration resize/truncate, separate conv scratch); compared to it, the allocation here uses the runtime page size instead of a compile-time 4096 (16K/64K-page systems), allocates exactlybsizebytes (no rounding to page multiples), restores true direct I/O on the write path, and adds unit and integration tests.libc::memalign+Vec::from_raw_parts, which hands memory not obtained throughVecback toVecand violates the allocator contract (UB, e.g. when building with a non-libc global allocator). Here the raw allocation never crosses into allocator-aware containers.Related follow-up
Writing an additional short-read regression test for this area surfaced a pre-existing data-corruption bug in the shared read path (#13458, fix in #13459). It reproduces identically on
mainand is orthogonal to this PR; the refactor here inherits that behavior unchanged (only the visible filler byte changes from0xDDto0x00, since the aligned buffer is zero-initialized). The textual overlap infill_consecutiveis trivial — whichever PR lands second gets rebased.Testing
AlignedBuffer(alignment across sizes and alignments for empty and non-empty buffers, zero-initialization, pinned error kinds for the rejection paths); they pass under Miri (run standalone — the uu_dd test binary itself cannot run under Miri because uucore's startup code callssigaction).O_DIRECTcapability probe (open withO_DIRECT+ one page-alignedposix_memaligntransfer): read and write support are probed separately, and only a failing open reports unsupported, for the errnos that mean the filesystem has no direct I/O at all (EINVAL/EOPNOTSUPP). Once the descriptor exists, anEINVALfrom the aligned transfer means the filesystem demands stricter alignment than a page — a finding worth surfacing rather than a reason to skip — so that, a short transfer, and every other unexpected outcome panic instead. Where the probe reports unsupported the tests skip and say so; once it succeeds the dd invocations are required to succeed, so a failure can no longer be excused as a filesystem limitation.iflag=direct(across 4K/64K/512K block sizes),oflag=direct, and the combined round-trip are covered separately, over position-dependent data whose every aligned 8-byte chunk is a distinct SplitMix64 value, so swapped, duplicated, or stale blocks cannot cancel out.oflag=directtest proves the write stays direct rather than only successful: it traces the open andfcntlunder strace and asserts the output file was opened withO_DIRECTand that noF_SETFLfollowed on that descriptor while it was open (the scan is bounded by the matchingclose(), so a later reuse of the descriptor number cannot produce a false positive), because the buffered-retry fallback (handle_o_direct_write) is the only path that togglesO_DIRECTafteropen()— a plain success check cannot tell the two apart, since the fallback also produces correct output. Confirmed to discriminate on a strict-alignment loop device (dma_alignment=511): the aligned buffer opens withO_DIRECTand issues zeroF_SETFL, while the previous misalignedVectriggers the fallback (fourF_SETFLtoggles on the output fd for two blocks). The test skips cleanly when strace is absent or cannot ptrace.O_DIRECTon this kernel (7.0; tmpfs has since 6.1), so the tests really ran everywhere rather than skipping: tmpfs, ext4 on NVMe (dma_alignment=3), and an ext4 loop device (dma_alignment=511, where the strace test is the discriminating one, run 10× to rule out timing flakiness). The skip path was exercised separately by hidingstracefromPATH.--no-default-features --features dd, and ran clippy with-D warningsover that minimal build,dd,printf,uu_dd, and the default feature set.dma_alignment=511): beforedd: IO error: Invalid input, after the read succeeds; strace shows zeroEINVALand nofcntltoggling for full-block direct writes.cargo test -p uu_dd(95 tests) and the dd integration suite (130 tests) pass; outputs are byte-identical to main across an 18-configuration sweep (odd ibs/obs, swab, sync, block/unblock, count_bytes, pipes).main, same toolchain): all nine dd benchmarks are at or below theVec<u8>baseline —dd_copy_1m_blocks−11.4%,dd_copy_partial−2.7%,dd_copy_4k_blocks−2.5%,dd_copy_8k_blocks−2.4%,dd_copy_with_skip/with_seek−1.9%,dd_copy_64k_blocks−1.6%,dd_copy_default−0.9%,dd_copy_separate_blocks±0%. Peak heap (DHAT) is identical tomain, sinceAlignedBufferallocates exactlybsizebytes.