7799: implement unlink io-uring call to implement remove_file and remove_dir#43
7799: implement unlink io-uring call to implement remove_file and remove_dir#43martin-augment wants to merge 1 commit intomasterfrom
Conversation
WalkthroughThis pull request extends tokio's filesystem operations with io-uring support for file and directory removal. The changes add a new ✨ 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 |
Code Review: io-uring unlink implementationThank you for implementing io-uring support for ✅ Strengths
🔍 Issues & Recommendations1. Missing Test Coverage (High Priority)The PR does not include any tests for the new io-uring unlink functionality. Looking at Recommendation: Add tests similar to the existing
Example test structure: #[test]
fn remove_file_with_uring() {
fn run(rt: Runtime) {
let (_tmp_files, paths) = create_tmp_files(10);
rt.block_on(async move {
for path in paths {
tokio::fs::remove_file(&path).await.unwrap();
assert!(!tokio::fs::try_exists(&path).await.unwrap());
}
});
}
for rt in rt_combinations() {
run(rt());
}
}2. Incomplete Documentation (Medium Priority)In - /// Submit a request to open a file.
+ /// Submit a request to unlink a file or directory.
3. Error Handling Consideration (Low Priority)The Common errors to consider:
The current implementation should handle these correctly via 4. Code Consistency: CancelData Pattern (Low Priority)The 🔒 Security Considerations✅ TOCTOU Handling: The implementation correctly uses ✅ Path Handling: Using ✅ No Unsafe Beyond io-uring: The ⚡ Performance Considerations✅ Path Ownership: Good optimization in ✅ Zero-Copy Path: The io-uring path passes 📝 Style & Best Practices✅ Follows tokio conventions: The conditional compilation, module structure, and trait implementations all follow established tokio patterns. ✅ Proper use of SummaryThis is a solid implementation that correctly follows tokio's io-uring patterns. The main issue is the lack of test coverage for the new functionality. Once tests are added and the documentation comment is fixed, this should be ready to merge. Required before merge:
Nice to have:
Review performed with analysis of tokio codebase patterns, io-uring safety requirements, and POSIX |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tokio/src/fs/remove_file.rs (1)
34-44: LGTM!The helper functions are correctly implemented with appropriate feature gating.
The io-uring availability check logic (lines 16-29) is duplicated between
remove_file.rsandremove_dir.rs. Consider extracting a helper macro or function if this pattern will be reused in additional fs operations (e.g.,rename,mkdir, etc.). For now, this duplication is acceptable given only two call sites.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tokio/src/fs/remove_dir.rstokio/src/fs/remove_file.rstokio/src/io/uring/mod.rstokio/src/io/uring/open.rstokio/src/io/uring/unlink.rstokio/src/runtime/driver/op.rs
🧰 Additional context used
🧠 Learnings (1)
📚 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/remove_file.rs
🧬 Code graph analysis (4)
tokio/src/io/uring/mod.rs (1)
tokio/src/io/uring/unlink.rs (1)
unlink(37-48)
tokio/src/io/uring/unlink.rs (3)
tokio/src/io/uring/utils.rs (1)
cstr(4-6)tokio/src/io/uring/open.rs (3)
complete(22-25)complete_with_error(27-29)cancel(33-35)tokio/src/runtime/driver/op.rs (4)
complete(115-115)complete_with_error(121-121)cancel(126-126)new(64-71)
tokio/src/fs/remove_file.rs (2)
tokio/src/runtime/handle.rs (1)
current(144-148)tokio/src/io/uring/unlink.rs (1)
remove_file(50-52)
tokio/src/fs/remove_dir.rs (2)
tokio/src/runtime/handle.rs (1)
current(144-148)tokio/src/io/uring/unlink.rs (1)
remove_dir(54-56)
⏰ 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 (10)
tokio/src/runtime/driver/op.rs (1)
4-4: LGTM! Clean generalization of cancellation data.The refactoring from
CancelData::Open(Open)toCancelData::CString(CString)is a sensible abstraction. BothOpenand the newUnlinkoperations need to keep their path alive during cancellation, so usingCStringdirectly in the cancellation data avoids duplication and allows multiple operation types to share the same cancellation mechanism.Also applies to: 21-21
tokio/src/io/uring/open.rs (1)
32-36: LGTM!The cancellation now correctly preserves only the
CStringpath, which is what the kernel needs to remain valid. This aligns with theCancelDatarefactoring and is more precise than keeping the entireOpenstruct.tokio/src/io/uring/mod.rs (1)
1-5: LGTM!Clean module addition following the established pattern for io-uring operations.
tokio/src/fs/remove_dir.rs (2)
9-28: LGTM! Clean runtime-conditional io-uring path.The implementation correctly:
- Checks feature flags at compile time via
#[cfg(...)]- Verifies io-uring availability at runtime via
check_and_init()?- Falls back to blocking implementation when io-uring is unavailable
30-39: Straightforward helper functions.The separation between
remove_dir_uringandremove_dir_blockingis clean. The blocking path correctly clones the path to move into the closure.tokio/src/io/uring/unlink.rs (4)
9-15: LGTM! Struct design follows the established pattern.The
Unlinkstruct correctly mirrors theOpenstruct design, keeping theCStringpath alive for the kernel to read during the operation.
17-27: LGTM!The
Completableimplementation is correct. Using.map(drop)elegantly converts theResult<u32>(whereu32is 0 on success) toResult<()>.
29-33: LGTM!Cancellation correctly preserves the path
CStringso the kernel can safely complete or cancel the in-flight operation.
50-56: LGTM! Clean convenience wrappers.The
remove_fileandremove_dirmethods provide clear, intention-revealing names that delegate to the underlyingunlinkwith appropriate flags.tokio/src/fs/remove_file.rs (1)
13-32: LGTM! Consistent withremove_dirimplementation.The runtime-conditional io-uring path follows the same pattern as
remove_dir.rs, which is good for consistency and maintainability.
| impl Op<Unlink> { | ||
| /// Submit a request to open a file. | ||
| pub(crate) fn unlink(path: &Path, directory: bool) -> io::Result<Op<Unlink>> { | ||
| let path = cstr(path)?; | ||
|
|
||
| let flags = if directory { libc::AT_REMOVEDIR } else { 0 }; | ||
|
|
||
| let unlink_op = opcode::UnlinkAt::new(types::Fd(libc::AT_FDCWD), path.as_ptr()) | ||
| .flags(flags) | ||
| .build(); | ||
|
|
||
| // SAFETY: Parameters are valid for the entire duration of the operation | ||
| Ok(unsafe { Op::new(unlink_op, Unlink { path }) }) | ||
| } |
There was a problem hiding this comment.
Fix the doc comment - it describes the wrong operation.
The doc comment says "Submit a request to open a file" but this function performs an unlink operation.
Proposed fix
impl Op<Unlink> {
- /// Submit a request to open a file.
+ /// Submit a request to unlink a file or directory.
pub(crate) fn unlink(path: &Path, directory: bool) -> io::Result<Op<Unlink>> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Op<Unlink> { | |
| /// Submit a request to open a file. | |
| pub(crate) fn unlink(path: &Path, directory: bool) -> io::Result<Op<Unlink>> { | |
| let path = cstr(path)?; | |
| let flags = if directory { libc::AT_REMOVEDIR } else { 0 }; | |
| let unlink_op = opcode::UnlinkAt::new(types::Fd(libc::AT_FDCWD), path.as_ptr()) | |
| .flags(flags) | |
| .build(); | |
| // SAFETY: Parameters are valid for the entire duration of the operation | |
| Ok(unsafe { Op::new(unlink_op, Unlink { path }) }) | |
| } | |
| impl Op<Unlink> { | |
| /// Submit a request to unlink a file or directory. | |
| pub(crate) fn unlink(path: &Path, directory: bool) -> io::Result<Op<Unlink>> { | |
| let path = cstr(path)?; | |
| let flags = if directory { libc::AT_REMOVEDIR } else { 0 }; | |
| let unlink_op = opcode::UnlinkAt::new(types::Fd(libc::AT_FDCWD), path.as_ptr()) | |
| .flags(flags) | |
| .build(); | |
| // SAFETY: Parameters are valid for the entire duration of the operation | |
| Ok(unsafe { Op::new(unlink_op, Unlink { path }) }) | |
| } |
🤖 Prompt for AI Agents
In tokio/src/io/uring/unlink.rs around lines 35 to 48, the doc comment
incorrectly reads "Submit a request to open a file"; update it to accurately
describe the function (e.g., "Submit a request to unlink (remove) a file or
directory") and mention that the `directory` flag toggles AT_REMOVEDIR to remove
directories; keep the rest of the implementation unchanged.
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback:The CodeRabbit AI reviewer is correct! The docstring is copy/pasted from another file (uring/read.rs) and it is not correct. Prevents releasing a new version with wrong documentation.
🤖 Augment PR SummarySummary: Adds an io-uring-backed unlink operation to support async file and directory removal on Linux. Changes:
Technical Notes: The io-uring path is gated behind 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| } | ||
|
|
||
| impl Op<Unlink> { | ||
| /// Submit a request to open a file. |
There was a problem hiding this comment.
This doc comment says “open a file”, but this function submits an UnlinkAt request (used for remove_file/remove_dir); updating it would avoid misleading readers.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback:The Augment AI reviewer is correct! The docstring is copy/pasted from another file (uring/read.rs) and it is not correct. Prevents releasing a new version with wrong documentation.
value:useful; category:bug; feedback:The Claude AI reviewer is correct! New integration tests should be added to prevent regressions in the future. There are some old tests for the non-io_uring file system operations which are automatically enabled when io_uring is enabled. |
value:good-to-have; category:documentation; feedback:The Claude AI reviewer is correct! The docstring is copy/pasted from another file (uring/read.rs) and it is not correct. Prevents releasing a new version with wrong documentation. |
value:incorrect-but-reasonable; category:bug; feedback:The Claude AI reviewer is correct! Actually the new enum variant should be named |
2 participants
7799: To review by AI