7791: fs: Implement io-uring::Op<Statx> and use it to implement fs::try_exists and complete the todo's in read_uring#39
Conversation
WalkthroughThe PR introduces comprehensive changes to Tokio's io-uring integration for file operations. It adds linux-raw-sys as a dependency, introduces a new statx module for metadata retrieval via io-uring, refactors file opening to work with OwnedFd instead of File objects directly, and updates filesystem operations (read, write, try_exists) to leverage the new paths. Test utilities are enhanced with bidirectional OwnedFd/MockFile conversions, and lower-level utilities are added for managing uninitialized boxed values. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request Review: Implement io-uring OpOverviewThis PR implements Code Quality & Best PracticesStrengths:
Issues & Concerns:
Potential Bugs & IssuesCritical:
Minor:
Performance ConsiderationsPositive:
Observations:
Security ConcernsNo major security issues identified. Notes:
Test CoverageCurrent state:
Gaps:
Additional Recommendations
SummaryThis is a solid implementation that follows Tokio's established patterns well. The main concerns are:
The code is production-ready after addressing the doc comment issue, with the other items being nice-to-haves for robustness. Recommendation: Approve with minor changes requested. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tokio/src/io/uring/utils.rs (1)
15-19: Consider adding safety documentation tobox_assume_init.The implementation of
box_assume_initis correct, but the function would benefit from explicit safety documentation explaining the caller's responsibility to ensure the value is properly initialized before calling this function.🔎 Suggested safety documentation
+/// # Safety +/// +/// The caller must ensure that `boxed` contains a fully initialized value of type `T`. +/// Calling this function on a `Box<MaybeUninit<T>>` that has not been properly initialized +/// results in undefined behavior. // TODO(MSRV 1.82): When bumping MSRV, switch to `Box::<MaybeUninit<T>>::assume_init()`. pub(crate) unsafe fn box_assume_init<T>(boxed: Box<MaybeUninit<T>>) -> Box<T> {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
tokio/Cargo.tomltokio/src/fs/mocks.rstokio/src/fs/open_options.rstokio/src/fs/open_options/uring_open_options.rstokio/src/fs/read.rstokio/src/fs/read_uring.rstokio/src/fs/try_exists.rstokio/src/fs/write.rstokio/src/io/uring/mod.rstokio/src/io/uring/open.rstokio/src/io/uring/statx.rstokio/src/io/uring/utils.rstokio/src/runtime/driver/op.rs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-28T15:30:50.368Z
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 9
File: tokio/src/fs/file.rs:824-855
Timestamp: 2025-10-28T15:30:50.368Z
Learning: In io-uring write implementations for tokio::fs::File, writes must loop until the buffer is fully drained. A single Op::write_at call may perform a partial write. The code must: (1) loop calling Op::write_at until buf.is_empty(), (2) return io::ErrorKind::WriteZero when 0 bytes are written, (3) only return Operation::Write(Ok(())) after the buffer is completely empty. This applies to both poll_write and poll_write_vectored implementations.
Applied to files:
tokio/src/fs/write.rstokio/src/fs/read_uring.rstokio/src/runtime/driver/op.rs
📚 Learning: 2025-10-28T15:30:54.516Z
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 9
File: tokio/src/fs/file.rs:970-981
Timestamp: 2025-10-28T15:30:54.516Z
Learning: In tokio fs file io-uring write operations (both vectored and non-vectored), the write must loop until the buffer is fully drained. Operation::Write(Ok()) should only be returned when buf.is_empty() is true. If 0 bytes are written, return Operation::Write(Err(io::ErrorKind::WriteZero.into())). Otherwise, continue executing new writes until the buffer is empty or an error occurs.
Applied to files:
tokio/src/fs/write.rstokio/src/runtime/driver/op.rs
📚 Learning: 2025-10-30T14:46:01.443Z
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 10
File: tokio-util/src/io/mod.rs:17-17
Timestamp: 2025-10-30T14:46:01.443Z
Learning: In tokio-util, the entire `io` module at `tokio-util/src/lib.rs` is already gated with `cfg_io!` macro, which applies `#[cfg(feature = "io")]` to all contents. Submodules declared within `tokio-util/src/io/mod.rs` inherit this feature gate and don't need additional `cfg_io!` wrapping unless they have specific requirements. The `cfg_io_util!` macro in the same file is for items that specifically require the `io-util` feature, not the general `io` feature.
Applied to files:
tokio/src/fs/write.rstokio/src/fs/open_options.rstokio/Cargo.toml
📚 Learning: 2025-11-05T14:24:03.551Z
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 13
File: tokio-util/src/io/simplex.rs:184-209
Timestamp: 2025-11-05T14:24:03.551Z
Learning: In AsyncWrite implementations, zero-length writes (when buf.is_empty()) should return Poll::Ready(Ok(0)) immediately without blocking or registering wakers. Only non-empty buffers that cannot be written due to backpressure should return Poll::Pending. This prevents indefinite hangs when empty slices are written.
Applied to files:
tokio/src/runtime/driver/op.rs
🧬 Code graph analysis (11)
tokio/src/io/uring/mod.rs (1)
tokio/src/io/uring/statx.rs (1)
statx(57-75)
tokio/src/fs/mocks.rs (1)
tokio/src/fs/file.rs (2)
from(888-890)from_raw_fd(919-923)
tokio/src/fs/open_options/uring_open_options.rs (2)
tokio/src/fs/open_options.rs (1)
open(519-521)tokio/src/io/uring/open.rs (1)
open(40-58)
tokio/src/io/uring/open.rs (3)
tokio/src/io/uring/statx.rs (2)
complete(39-42)complete(95-101)tokio/src/runtime/driver/op.rs (1)
complete(118-118)tokio/src/io/uring/write.rs (1)
complete(16-18)
tokio/src/fs/try_exists.rs (2)
tokio/src/runtime/handle.rs (1)
current(144-148)tokio/src/io/uring/statx.rs (1)
metadata(77-79)
tokio/src/io/uring/utils.rs (1)
tokio/src/io/uring/statx.rs (2)
box_new_uninit(59-59)box_new_uninit(116-116)
tokio/src/fs/read_uring.rs (3)
tokio/src/fs/open_options/uring_open_options.rs (1)
new(24-35)tokio/src/runtime/driver/op.rs (1)
new(67-74)tokio/src/io/uring/statx.rs (1)
metadata_fd(115-135)
tokio/src/fs/open_options.rs (3)
tokio/src/fs/file.rs (2)
from_std(277-287)from(888-890)tokio/src/fs/open_options/uring_open_options.rs (1)
from(120-134)tokio/src/fs/mod.rs (1)
asyncify(312-324)
tokio/src/fs/read.rs (1)
tokio/src/fs/read_uring.rs (1)
read_uring(19-35)
tokio/src/io/uring/statx.rs (3)
tokio/src/io/uring/utils.rs (3)
box_assume_init(16-19)box_new_uninit(10-13)cstr(5-7)tokio/src/io/uring/open.rs (3)
complete(22-25)complete_with_error(27-29)cancel(33-35)tokio/src/runtime/driver/op.rs (5)
complete(118-118)complete_with_error(124-124)cancel(129-129)from(104-112)new(67-74)
tokio/src/runtime/driver/op.rs (1)
tokio/src/io/uring/statx.rs (1)
statx(57-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (19)
tokio/src/io/uring/mod.rs (1)
3-3: LGTM! Module addition looks correct.The new
statxmodule is properly declared aspub(crate), making it available for internal use within the io-uring subsystem.tokio/src/runtime/driver/op.rs (1)
6-6: LGTM! Cancellation support properly extended.The addition of
StatxandStatxFdvariants to theCancelDataenum correctly extends cancellation support to the new statx operations, following the established pattern for other io-uring operation types.Also applies to: 23-24
tokio/src/fs/open_options/uring_open_options.rs (1)
114-116: LGTM! Clean async interface for io-uring file opening.The new
open()method provides a clean async interface that delegates to the io-uring operation and returns anOwnedFd, maintaining clear ownership semantics.tokio/src/fs/mocks.rs (1)
109-116: LGTM! Test-only conversion properly implemented.The
From<OwnedFd>implementation correctly transfers ownership of the file descriptor fromOwnedFdtoMockFileusingIntoRawFdandFromRawFd. This complements the existing reverse conversion and is properly scoped to test code only.tokio/src/io/uring/utils.rs (1)
10-13: LGTM! Clean implementation with MSRV tracking.The
box_new_uninithelper correctly creates an uninitialized boxed value and includes a TODO comment for migrating to the standard library method when the MSRV is raised.tokio/src/fs/read.rs (1)
57-57: LGTM! Improved path handling and code organization.The refactoring improves efficiency by deferring the
PathBufallocation until it's actually needed (in the blocking path). The extraction ofread_spawn_blockingalso improves code modularity and separation of concerns.Also applies to: 72-72, 76-82
tokio/src/io/uring/open.rs (1)
21-25: OwnedFd improves ownership semantics across io-uring integration.The change from returning
io::Result<crate::fs::File>toio::Result<OwnedFd>provides clearer ownership semantics and aligns with the broader refactoring across the io-uring integration. The implementation correctly usesOwnedFd::from_raw_fdto take ownership of the file descriptor. All call sites (read_uring.rsandwrite.rs) properly handle the new return type, passing OwnedFd to the corresponding io-uring operations.tokio/Cargo.toml (1)
88-88: Version 0.12.1 is the latest stable release and has no known security vulnerabilities.Verification confirms that linux-raw-sys 0.12.1 is the current stable version (as of December 23, 2025) with no public CVEs or security advisories. The dependency is properly gated behind the unstable
tokio_unstablefeature and the Linux-only target configuration, ensuring it only compiles when explicitly enabled.tokio/src/fs/try_exists.rs (3)
25-44: LGTM! Clean io-uring path selection with efficient path handling.The implementation correctly:
- Borrows the path reference upfront, avoiding unnecessary cloning for the io-uring path
- Only clones to
PathBufin the blocking fallback where ownership is required- Properly checks driver readiness before using the io-uring path
46-56: LGTM! Correct error mapping for existence check.The io-uring path properly maps
NotFoundtoOk(false)while propagating other errors, matching the semantics ofstd::path::Path::try_exists.
58-61: LGTM!The blocking fallback correctly clones the path to satisfy the
'staticbound required byasyncify.tokio/src/fs/read_uring.rs (2)
19-35: LGTM! Clean integration with UringOpenOptions and metadata_fd.The flow correctly:
- Opens the file with io-uring via
UringOpenOptions- Retrieves metadata using
Op::metadata_fdwhich returns both the result and the fd- Safely handles
u64tousizeconversion overflow by defaulting tousize::MAX
107-132: LGTM! Proper EINTR retry handling.The
op_readfunction correctly loops onErrorKind::Interrupted, which is the expected behavior for io-uring operations that may be interrupted by signals.tokio/src/fs/open_options.rs (1)
519-558: LGTM! Clean separation between io-uring and standard paths.The implementation:
- Properly delegates to
open_innerfor path handling- Correctly checks driver readiness before using io-uring
- Falls back to the standard path when io-uring is not available
- Uses test-mockable
StdFilefor testabilitytokio/src/fs/write.rs (1)
48-80: LGTM! Correctly implements partial write handling for io-uring.The implementation properly:
- Loops until the entire buffer is written (
while buf_offset < total)- Returns
WriteZeroerror whenOk(0)is received (per retrieved learnings)- Continues on
Interruptederrors- Tracks both buffer and file offsets for correct positioning
Based on learnings, this matches the required pattern for io-uring write operations.
tokio/src/io/uring/statx.rs (4)
11-25: LGTM! Clean Metadata wrapper.The
Metadatastruct properly wraps the rawstatxstructure and provides a safe accessor for the file size viastx_size.
36-53: LGTM! Correct Completable and Cancellable implementations.The
completemethod correctly usesbox_assume_initafter the kernel has written to the buffer. The safety invariant is maintained because the kernel completes the statx operation beforecompleteis called.
55-84: LGTM! Well-structured statx operation setup.The implementation:
- Correctly converts the path to CString and stores it to keep it alive during the operation
- Uses
AT_FDCWDfor path-relative operations- Properly applies
AT_SYMLINK_NOFOLLOWflag based onfollow_symlinksparameter- Documents the commented-out
symlink_metadatafor future use
86-136: LGTM! StatxFd correctly returns both metadata and fd.The
StatxFdvariant:
- Preserves the
OwnedFdthrough the completion, returning it as part of the tuple- Uses
AT_EMPTY_PATHflag correctly with an empty C string literal (c"")- Properly documents the io-uring/statx man page reference for the empty path usage
🤖 Augment PR SummarySummary: Adds a new io-uring-backed Changes:
Technical Notes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| // https://man7.org/linux/man-pages/man2/statx.2.html | ||
| let statx_op = opcode::Statx::new( | ||
| types::Fd(fd.as_raw_fd()), | ||
| c"".as_ptr(), |
There was a problem hiding this comment.
c"" C-string literals require a newer Rust version than this crate’s MSRV (1.71), so this will break builds that respect rust-version.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:useful; category:bug; feedback:The Augment AI reviewer is correct! The build will fail with Rust .171 because c-string literals are stable since version 1.77. This should be reworked to not use c-string literals.
| /// Submit a request to open a file. | ||
| fn statx(path: &Path, follow_symlinks: bool) -> io::Result<Op<Statx>> { | ||
| let path = cstr(path)?; | ||
| let mut buffer = box_new_uninit::<statx>(); |
There was a problem hiding this comment.
This allocates the output statx buffer as MaybeUninit and later reads the whole struct via assume_init; if the kernel ever leaves padding/unused fields uninitialized, that becomes UB. Consider ensuring the buffer is fully initialized (e.g., zeroed) before passing it to the kernel.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:useful; category:bug; feedback:The Augment AI reviewer is correct! A check should be added that the returned buffer by the kernel is fully initialized or alternatively the buffer should be pre-initialized before being passed to the kernel. Prevents passing a corrected data to the user application in case of a bug in the kernel.
value:good-to-have; category:documentation; feedback:The Claude AI reviewer is correct! The docstring is copy/pasted from somewhere else. The statx module is about reading the file's metadata, so the docstring should be fixed. Prevents releasing wrong documentation for the new method. |
value:good-to-have; category:bug; feedback:The Claude AI reviewer is correct! There is no point in adding commented out code in a new module. It should be removed and an issue should be filled in the issue tracker for implementing it when the prerequisites are available. Prevents adding technical dept without explanation of the reason. |
value:useful; category:bug; feedback:The Claude AI reviewer is correct! There is no problem with truncation here since this is just a hint that is used to decide how many bytes to pre-allocate for the buffer capacity. |
value:annoying; category:bug; feedback:The Claude AI reviewer is not correct! There is no such code at try_exists.rs:21 at all! The AI model is hallucinating! |
2 participants
7791: To review by AI