fix(overlaybd): derive direct I/O alignment from each file - #279
fix(overlaybd): derive direct I/O alignment from each file#279ranxi2001 wants to merge 3 commits into
Conversation
|
🔍 OpenCodeReview found 4 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let alignment = if self.direct_io { | ||
| sys::direct_io_alignment(&file) | ||
| .with_context(|| format!("query direct io alignment on {}", path.display()))? | ||
| } else { | ||
| sys::DirectIoAlignment::default() | ||
| }; |
There was a problem hiding this comment.
The values returned by statx are stored and then used as divisors and Layout alignments without validating them. In particular, stx_dio_offset_align is allowed to be zero when the filesystem does not report an offset requirement, which will panic in is_multiple_of/the alignment helpers; non-power-of-two or overflowing values are also invalid for AlignedBuffer (and non-power-of-two values make the bit-mask helpers incorrect in release builds). Normalize an incomplete result to the fallback or reject it before constructing LocalFileInner, and validate both fields as nonzero, representable, and allocator-compatible.
Suggestion:
| let alignment = if self.direct_io { | |
| sys::direct_io_alignment(&file) | |
| .with_context(|| format!("query direct io alignment on {}", path.display()))? | |
| } else { | |
| sys::DirectIoAlignment::default() | |
| }; | |
| let alignment = if self.direct_io { | |
| sys::direct_io_alignment(&file) | |
| .with_context(|| format!("query direct io alignment on {}", path.display()))? | |
| } else { | |
| sys::DirectIoAlignment::default() | |
| }; | |
| ensure!( | |
| alignment.memory.is_power_of_two() | |
| && alignment.memory != 0 | |
| && alignment.offset != 0, | |
| "invalid direct I/O alignment: memory {}, offset {}", | |
| alignment.memory, | |
| alignment.offset | |
| ); |
| let aligned_len = usize::try_from(aligned_end.saturating_sub(aligned_offset)) | ||
| .context("aligned read length overflow")?; | ||
| let mut buffer = AlignedBuffer::new(aligned_len, DIRECT_IO_ALIGNMENT)?; | ||
| let mut buffer = AlignedBuffer::new(aligned_len, self.alignment.memory)?; |
There was a problem hiding this comment.
The new separation of memory and offset alignment exposes a retry-path bug: both read_into_at and io_ring::read_exact_at reslice the buffer after a short read, so if a direct-I/O operation returns a byte count that is a multiple of offset but not memory (for example 512 bytes on a file requiring 4-KiB memory alignment), the next submission starts at an unaligned pointer and fails with EINVAL. The analogous write helpers have the same issue after a short write. Retries need to preserve an aligned base (e.g. use an aligned allocation plus an aligned offset into it, or ensure each chunk advance satisfies the memory alignment), rather than blindly advancing the slice pointer.
Suggestion:
| let mut buffer = AlignedBuffer::new(aligned_len, self.alignment.memory)?; | |
| let mut buffer = AlignedBuffer::new(aligned_len, self.alignment.memory)?; |
| // 512-aligned memory still requires a bounce on a 4K device. | ||
| let mut dst = AlignedBuffer::new(len, 512).unwrap(); | ||
| let n = file | ||
| .read_at_into_with_ctx(ctx, offset as u64, dst.as_mut()) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
This ignored test does not deterministically exercise the newly supported distinction between memory and offset alignment: it hardcodes 512-byte buffer alignment and 4096-byte operations, and uses whatever alignment the host reports. On a typical 512-byte device all of these paths can pass without testing the 4 KiB bounce/alignment behavior. Inject or otherwise mock DirectIoAlignment (or use a controlled backing device) and derive the operation sizes from the injected values; otherwise this test should not be relied on to catch regressions in the new logic.
| if stat.stx_dio_mem_align == 0 { | ||
| return Err(SysError::Unsupported("direct_io on this file")); | ||
| } | ||
| Ok(super::DirectIoAlignment { | ||
| memory: stat.stx_dio_mem_align as usize, | ||
| offset: stat.stx_dio_offset_align as usize, | ||
| }) |
There was a problem hiding this comment.
Validate stx_dio_offset_align as well as the memory alignment before returning. This value is subsequently passed to align_down/align_up and is_multiple_of, so returning zero can cause division-by-zero panics during ordinary reads or writes. Treat either zero field as unsupported (or use the fallback if that is the intended policy).
Suggestion:
| if stat.stx_dio_mem_align == 0 { | |
| return Err(SysError::Unsupported("direct_io on this file")); | |
| } | |
| Ok(super::DirectIoAlignment { | |
| memory: stat.stx_dio_mem_align as usize, | |
| offset: stat.stx_dio_offset_align as usize, | |
| }) | |
| if stat.stx_dio_mem_align == 0 || stat.stx_dio_offset_align == 0 { | |
| return Err(SysError::Unsupported("direct_io on this file")); | |
| } | |
| Ok(super::DirectIoAlignment { | |
| memory: stat.stx_dio_mem_align as usize, | |
| offset: stat.stx_dio_offset_align as usize, | |
| }) |
|
Thanks for catching these issues. Fixed in |
| Ok( | ||
| io_ring::write_exact_at_aligned(submitter, fd, buf, offset, self.alignment.memory) | ||
| .await?, | ||
| ) |
There was a problem hiding this comment.
This branch handles buffered files (direct_io has already been excluded), but it now uses the alignment-aware helper with the default 512-byte alignment. A buffered short write whose count is not divisible by 512 will therefore be returned as a successful partial write rather than retried; for example, a partial write caused by an exhausted filesystem can hide the subsequent error that the previous exact-write loop would expose. Keep io_ring::write_exact_at for buffered I/O and reserve the aligned helper for O_DIRECT.
| // if dst is already aligned, just read into dst | ||
| Ok(io_ring::read_exact_at(submitter, fd, dst, offset).await?) |
There was a problem hiding this comment.
This direct-I/O fast path still uses the generic retry helper. If the first read ends at a non-sector-aligned EOF, it may return a short, unaligned byte count; read_exact_at then advances the pointer and offset and submits another O_DIRECT read that fails with EINVAL, instead of returning the available bytes. Use the alignment-aware read helper here as in read_direct_at_via.
| pub async fn read_exact_at_aligned<S: IoUringSubmitter + ?Sized>( | ||
| submitter: &S, | ||
| fd: RawFd, | ||
| mut buf: &mut [u8], | ||
| mut offset: u64, | ||
| alignment: usize, |
There was a problem hiding this comment.
A single alignment is insufficient for O_DIRECT retries because memory alignment and offset/length alignment can differ (and statx explicitly reports them separately). Current callers pass stx_dio_mem_align; if a short completion is memory-aligned but not stx_dio_offset_align, this helper advances to an invalid file offset/remaining length and the retry fails with EINVAL. Accept both requirements (or a validated retry alignment such as their LCM) and only resubmit when the completed count preserves both. The write helper below has the same issue.
| pub async fn write_exact_at_aligned<S: IoUringSubmitter + ?Sized>( | ||
| submitter: &S, | ||
| fd: RawFd, | ||
| mut buf: &[u8], | ||
| mut offset: u64, | ||
| alignment: usize, |
There was a problem hiding this comment.
This public helper accepts zero without validation, so a short completion reaches n.is_multiple_of(alignment) and panics. The read counterpart has the same problem. Represent this as NonZeroUsize or reject zero before submitting I/O so invalid input produces a normal InvalidInput error.
1 participant
What
Query each direct-I/O file’s memory and offset alignment with Linux STATX_DIOALIGN and use those limits for LocalFile reads and writes, including io_uring. This allows sub-sector reads from local lower layers on filesystems requiring 4096-byte direct I/O.
Why
The fixed 512-byte alignment causes EINVAL when the backing file requires 4096-byte requests. The libaio image path opens local lower layers with direct I/O, so even reading their headers can fail.
Related issue
Related to #274. This addresses lower-layer reads and aligned LocalFile I/O; writable LSMT operations that issue 512-byte requests on a 4K-required file remain unsupported.
Scope and non-goals
The change is limited to LocalFile and its existing platform syscall boundary. The LSMT data/index layout remains unchanged; it does not add read-modify-write or buffered fallback.
Design and behavior changes
Alignment is queried once using the opened descriptor. Read ranges use the reported offset alignment, while bounce buffers use the memory alignment. Writes still require aligned offsets and lengths, and invalid requests report the required alignment. If the kernel/filesystem cannot report the limits, or on macOS, the existing 512-byte behavior is retained. Other query errors are propagated.
Compatibility and operations
Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetCommands and results:
Full workspace clippy passed after supplying libclang/LLVM and Clang builtin headers from locally extracted distribution packages; no host packages were installed.
The no-default-features library suite also passed (299 tests, 2 ignored). A process-local syscall probe reporting and enforcing 4K constraints makes the same new lower-read regression fail on base with EINVAL and pass with this patch. The libaio image-open test passes under that probe. This models alignment constraints; it is not native 4K hardware validation, and it does not intercept io_uring submissions.
Skipped checks and reasons:
Risks and reviewer notes
This is a partial fix for #274. LSMT uses 512-byte data and seal-index/padding writes, which cannot be submitted as direct I/O to a file requiring 4096-byte requests. Such writes now fail with the required alignment in the error. Supporting them needs a separate storage design. Filesystems that cannot report STATX_DIOALIGN retain the existing 512-byte assumption.
Checklist