feat(filesystem): add DirectFile with O_DIRECT/F_NOCACHE support#35
feat(filesystem): add DirectFile with O_DIRECT/F_NOCACHE support#35bneb wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hi bneb,
Thanks for the PR — I can see the effort and thought put into it. I have a few concerns, and I'm happy to iterate on them with you. I'd like to start with the following.
Copy-through strategy doesn't seem right to me. We're avoiding changes to the upper layers by creating a local region of alignment, when we should be propagating the alignment assumption through the stack. As maintainer, I'm open to a larger blast radius if it cleans up the code.
Changes I'd like to see:
-
Make 4 KiB (4096) alignment a property of the buffer type, not a per-I/O fixup. Introduce an aligned
Block:#[repr(align(4096))] struct Block([u8; BLOCK_SIZE]); // aligned + fixed-size by construction, zero unsafe
-
Propagate
Blockthrough the stack — the page-cache frames, the superblock buffer, andBlockStore::read/write— so an unaligned or wrong-sized buffer can't be passed in the first place.
This simplifies the code by deleting the special-casing the current approach needs:
AlignedBufand itsunsafe— the bounce buffer indirect.rs— gone; aligned frames are DMA'd directly.- The per-I/O
if direct { bounce } else { … }branch — gone. - The
check_len/BadBlockSizeruntime size checks infile.rs— gone; the type fixes the size.
The alignment assumption then lives in one type, instead of being implied across the I/O path.
Thanks!
Nevin
PS: Added an implementation note / hint. A Options Struct + single object would be helpful. #34 (comment) #34 (comment)
|
Putting this here to be direct — I wrote the fuller version in #34, but here's what I'd like, plainly:
happy to talk through scope, split it, or pair. |
… stack
Replace the copy-through bounce-buffer strategy with a #[repr(align(4096))]
Block type that makes alignment a compile-time property. Propagate Block
through BlockStore, PageCache frames, and the superblock so DirectFile can
DMA directly — no per-I/O allocation, no branch.
- Add Block([u8; BLOCK_SIZE]) with Deref/DerefMut, zero unsafe
- Propagate Block through BlockStore::read/write, PageCache frames,
superblock I/O, and DirectFile (which now takes &Block/&mut Block)
- Remove AlignedBuf and its unsafe alloc/dealloc (53 lines gone)
- Remove check_len / BadBlockSize — the type system enforces size
- Remove the per-I/O if-direct-bounce-else branch in DirectFile
- Consolidate Heap constructors with HeapOptions { frames, direct_io }
- Consolidate StorageEngine constructors with EngineBuilder
- Update benchmarks to use Block-aligned buffers
- Remove dead #[ignore]d cross-journal durability test
This implements the design requested in nevzheng#35 review: alignment lives in
one type instead of being implied across the I/O path.
|
Thanks for the review. Rewritten to propagate alignment as you described.
I did not propagate Re: |
|
Hi @bneb I decided to close this in favor of #36. And while this PR added benchmarks, I decided that I'm not ready to accept and maintain them, so I'd rather not pull them in right now. Thanks for the work here. I'm sorry for any confusion. |
Summary
Direct I/O support (#34) via
O_DIRECT(Linux) /F_NOCACHE(macOS), with alignment enforced at the type level.Design
A
#[repr(align(4096))] struct Block([u8; BLOCK_SIZE])carries the alignment invariant. It propagates throughBlockStore::read/write→PageCacheframes →FileBlockStoresuperblock →DirectFile. The type system rejects unaligned or wrong-sized buffers at compile time — nounsafe, no runtime size checks, no bounce buffer.DirectFileprovides two constructors:open()(attempts O_DIRECT/F_NOCACHE, falls back silently) andopen_buffered(). Both take&Block/&mut Blockfor I/O.HeapOptions { frames, direct_io }andEngineBuilderconsolidate the constructor surface.When to use
Benchmark sweep (16K rows, ~300 pages, pool sizes 32–512 frames) places the inversion point at 384 frames (3 MiB). When
pool_frames ≥ dataset_pages, direct I/O matches buffered throughput without double-buffering in the OS page cache.Scope
Does not propagate
Blockto LSM SSTable reads (the LSM usesCache, notPageCache). Does not change the default I/O path.Verification
Clippy clean, 291 tests pass, benchmarks compile and run.