7847: fs: implement tokio::fs::create_dir[_all] via io_uring#71
7847: fs: implement tokio::fs::create_dir[_all] via io_uring#71martin-augment wants to merge 1 commit intomasterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Tokio's file system capabilities by integrating Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThis change introduces io-uring optimized directory creation operations for Tokio on Linux. The ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces io_uring support for tokio::fs::create_dir and tokio::fs::create_dir_all on Linux, which is a significant performance improvement. The implementation correctly integrates with the io_uring driver and includes a new create_dir_all_uring module to handle recursive directory creation. Comprehensive tests have been added to cover various scenarios, including runtime shutdown and edge cases for create_dir_all.
However, there are a few areas that could be improved for robustness, performance consistency, and idiomatic Rust path handling.
| Ok(()) => Ok(true), | ||
| Err(ref e) if e.kind() == io::ErrorKind::NotFound => Ok(false), | ||
| // TODO: replace with uring-based statx | ||
| Err(_) if crate::fs::metadata(path).await?.is_dir() => Ok(true), |
There was a problem hiding this comment.
The fallback to crate::fs::metadata here, and similarly on line 88, can introduce blocking operations if io-uring is not enabled for metadata itself. This would negate the performance benefits of using io-uring for mkdir in these specific error handling paths. While the TODO comment acknowledges the need for uring-based statx, the current implementation can lead to unexpected blocking.
Consider ensuring that all file system operations within the io-uring path are non-blocking or handle the blocking fallback explicitly with spawn_blocking to avoid blocking the io-uring driver thread.
There was a problem hiding this comment.
value:good-but-wont-fix; category:bug; feedback: The Gemini AI reviewer is correct! The statx (file metadata) is not yet implemented for io-uring, but IO_Uring support is introduced into Tokio feature by feature. This PR is about create_dir. A follow-up PR should be about statx
| let path_bytes = path.as_os_str().as_bytes(); | ||
| for (separator_pos, _) in path_bytes | ||
| .iter() | ||
| .enumerate() | ||
| .rev() | ||
| .filter(|(_, &byte)| byte == b'/') | ||
| { | ||
| let parent_bytes = &path_bytes[..separator_pos]; | ||
| let parent_str = OsStr::from_bytes(parent_bytes); | ||
| let parent_path = Path::new(parent_str); | ||
| if mkdir_parent_missing(parent_path).await? { |
There was a problem hiding this comment.
The manual byte-level manipulation of paths using as_os_str().as_bytes() and OsStr::from_bytes() for splitting and rejoining path components is less idiomatic and potentially more error-prone than using Path's built-in methods like parent() and components(). While it might work correctly for typical Unix paths, using Path methods would improve readability, maintainability, and robustness against edge cases (e.g., root paths, multiple slashes, or future platform changes).
For example, path.parent() and then iterating over path.components() could provide a more robust way to handle path segments.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! There is no need to do this manual parsing of the file path. The PR author should use std::fs::Path and PathBuf instead.
| // TODO: We're attempting to create all directories sequentially. | ||
| // This would benefit from batching. |
There was a problem hiding this comment.
The TODO comment correctly identifies a significant optimization opportunity. Creating directories sequentially can be inefficient, especially for deep paths. Batching these operations, if io-uring supports it (e.g., via IORING_OP_MKDIRAT with IORING_ASYNC_BATCH), would greatly improve performance for create_dir_all.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! If there are many directories to be created using batching may be better but this should be benchmarked first and implemented in a follow-up if needed.
PR Review: fs: implement tokio::fs::create_dir[_all] via io_uringGood overall structure - the implementation follows the existing patterns for io_uring operations ( Bug: Error information is lost in
|
🤖 Augment PR SummarySummary: Adds io_uring-backed implementations for Changes:
Technical Notes: Uses 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| ))] | ||
| { | ||
| let handle = crate::runtime::Handle::current(); | ||
| let driver_handle = handle.inner.driver().io(); |
There was a problem hiding this comment.
handle.inner.driver().io() will panic if the runtime was built without IO enabled (i.e., enable_io/enable_all not called), which would be a behavior change vs the asyncify fallback. Consider guarding this so create_dir still falls back cleanly when IO is disabled at runtime.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-but-wont-fix; category:bug; feedback: The Augment AI reviewer is correct! io_uring support is experimental/unstable in Tokio, so the developer should opt-in for it. It is an application error if io_uring is requested but the IO driver is not enabled. Returning the error would tell the developer what (s)he needs to do. Falling back to asyncify() will hide the problem and leave the developer believing that IO Uring is in use.
| use crate::fs::create_dir_all_uring; | ||
|
|
||
| let handle = crate::runtime::Handle::current(); | ||
| let driver_handle = handle.inner.driver().io(); |
There was a problem hiding this comment.
Same concern as create_dir: calling handle.inner.driver().io() can panic when the runtime has IO disabled, preventing the intended asyncify fallback. It may be worth ensuring create_dir_all doesn’t require IO-enabled runtimes just to decide whether to use io_uring.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-but-wont-fix; category:bug; feedback: The Augment AI reviewer is correct! io_uring support is experimental/unstable in Tokio, so the developer should opt-in for it. It is an application error if io_uring is requested but the IO driver is not enabled. Returning the error would tell the developer what (s)he needs to do. Falling back to asyncify() will hide the problem and leave the developer believing that IO Uring is in use.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tokio/src/fs/create_dir_all_uring.rs`:
- Around line 73-79: The metadata fallback in mkdir_parent_missing currently
uses the `?` operator inside an `Err(...) if ...` guard, causing any metadata
error to replace the original `Op::mkdir` error; change the guard to capture the
original mkdir error (from `Op::mkdir(path)?.await`), then explicitly call
`crate::fs::metadata(path).await` in a separate match/if-let so you can inspect
its Result: on Ok(meta) if meta.is_dir() return Ok(true), on Err(_) return the
original mkdir error (Err(original_err)); update both occurrences that use this
pattern so metadata failures do not clobber the original `Op::mkdir` error.
In `@tokio/tests/fs_uring_mkdir.rs`:
- Around line 146-153: The test recreates the create_dir future on every poll
because `fut` is declared inside the `poll_fn` closure; move the one-shot
operation out so the same in-flight future is polled across invocations.
Concretely, allocate and pin the `tokio::fs::create_dir(&child)` future once
(e.g., a `let mut fut = Box::pin(...)` or `Option<Pin<_>>`), then have the
`poll_fn` closure poll that pinned `fut` via `poll_unpin` instead of reassigning
a new `fut` each poll; update the `cancel_op_future` test harness to reference
this single pinned future so the cancellation behavior is exercised correctly.
- Around line 60-67: The test currently re-executes assert_pending! on every
re-poll inside the poll_fn closure, which can panic when the underlying
create_dir future becomes ready and the task is detached; fix by ensuring the
“pending” assertion runs only once before readiness detection—e.g., capture a
local mutable bool (e.g., asserted) in the poll_fn closure or perform a single
assert_pending! call before entering the poll loop, then on subsequent polls
check fut.as_mut().poll(cx) == Poll::Pending without reasserting; reference the
poll_fn closure, assert_pending!, and fut (the create_dir future) when applying
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6887a0c8-72b5-4051-82ee-211c908ee3ca
📒 Files selected for processing (8)
tokio/src/fs/create_dir.rstokio/src/fs/create_dir_all.rstokio/src/fs/create_dir_all_uring.rstokio/src/fs/mod.rstokio/src/io/uring/mkdir.rstokio/src/io/uring/mod.rstokio/src/runtime/driver/op.rstokio/tests/fs_uring_mkdir.rs
| async fn mkdir_parent_missing(path: &Path) -> io::Result<bool> { | ||
| match Op::mkdir(path)?.await { | ||
| Ok(()) => Ok(true), | ||
| Err(ref e) if e.kind() == io::ErrorKind::NotFound => Ok(false), | ||
| // TODO: replace with uring-based statx | ||
| Err(_) if crate::fs::metadata(path).await?.is_dir() => Ok(true), | ||
| Err(e) => Err(e), |
There was a problem hiding this comment.
Metadata fallback currently clobbers the original mkdir error.
In Lines 78 and 87, metadata is evaluated with ? in a guard. If metadata fails, that metadata error is returned instead of the original Op::mkdir failure.
Suggested fix
async fn mkdir_parent_missing(path: &Path) -> io::Result<bool> {
match Op::mkdir(path)?.await {
Ok(()) => Ok(true),
Err(ref e) if e.kind() == io::ErrorKind::NotFound => Ok(false),
// TODO: replace with uring-based statx
- Err(_) if crate::fs::metadata(path).await?.is_dir() => Ok(true),
- Err(e) => Err(e),
+ Err(e) => match crate::fs::metadata(path).await {
+ Ok(metadata) if metadata.is_dir() => Ok(true),
+ _ => Err(e),
+ },
}
}
async fn mkdir_parent_created(path: &Path) -> io::Result<()> {
match Op::mkdir(path)?.await {
Ok(()) => Ok(()),
// TODO: replace with uring-based statx
- Err(_) if crate::fs::metadata(path).await?.is_dir() => Ok(()),
- Err(e) => Err(e),
+ Err(e) => match crate::fs::metadata(path).await {
+ Ok(metadata) if metadata.is_dir() => Ok(()),
+ _ => Err(e),
+ },
}
}Also applies to: 83-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tokio/src/fs/create_dir_all_uring.rs` around lines 73 - 79, The metadata
fallback in mkdir_parent_missing currently uses the `?` operator inside an
`Err(...) if ...` guard, causing any metadata error to replace the original
`Op::mkdir` error; change the guard to capture the original mkdir error (from
`Op::mkdir(path)?.await`), then explicitly call
`crate::fs::metadata(path).await` in a separate match/if-let so you can inspect
its Result: on Ok(meta) if meta.is_dir() return Ok(true), on Err(_) return the
original mkdir error (Err(original_err)); update both occurrences that use this
pattern so metadata failures do not clobber the original `Op::mkdir` error.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! If the metadata() calls errors then it will hide the original error. Better error handling is needed in this case to combine the errors or at least show the original error.
| poll_fn(|cx| { | ||
| assert_pending!(fut.as_mut().poll(cx)); | ||
| Poll::<()>::Pending | ||
| }) | ||
| .await; | ||
|
|
||
| fut.await.unwrap(); | ||
| }); |
There was a problem hiding this comment.
assert_pending! is re-run on wake and can silently panic detached tasks.
At Line 61, the assertion is executed on every poll_fn re-poll. Once create_dir becomes ready, this panics inside a detached task, so the test can still pass while work panics.
Suggested fix
- poll_fn(|cx| {
- assert_pending!(fut.as_mut().poll(cx));
- Poll::<()>::Pending
- })
- .await;
-
- fut.await.unwrap();
+ let mut first_poll = true;
+ poll_fn(|cx| {
+ if first_poll {
+ assert_pending!(fut.as_mut().poll(cx));
+ first_poll = false;
+ }
+ Poll::<()>::Pending
+ })
+ .await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tokio/tests/fs_uring_mkdir.rs` around lines 60 - 67, The test currently
re-executes assert_pending! on every re-poll inside the poll_fn closure, which
can panic when the underlying create_dir future becomes ready and the task is
detached; fix by ensuring the “pending” assertion runs only once before
readiness detection—e.g., capture a local mutable bool (e.g., asserted) in the
poll_fn closure or perform a single assert_pending! call before entering the
poll loop, then on subsequent polls check fut.as_mut().poll(cx) == Poll::Pending
without reasserting; reference the poll_fn closure, assert_pending!, and fut
(the create_dir future) when applying the change.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The test assumes that the poll_fn will be called just once because the handle should be aborted after the first poll but due to timings the poll_fn may be called more than once and the assertion will fail on the second run. The test should be improved to not depend on racing.
| poll_fn(|cx| { | ||
| let child = workdir.path().join("child"); | ||
| let fut = tokio::fs::create_dir(&child); | ||
|
|
||
| // If io_uring is enabled (and not falling back to the thread pool), | ||
| // the first poll should return Pending. | ||
| let _pending = pin!(fut).poll_unpin(cx); | ||
|
|
There was a problem hiding this comment.
cancel_op_future recreates/drops the op each poll instead of holding one in-flight future.
Because fut is local to the closure, it is dropped every poll cycle. This weakens the cancellation test intent for a single operation future.
Suggested fix
- poll_fn(|cx| {
- let child = workdir.path().join("child");
- let fut = tokio::fs::create_dir(&child);
-
- // If io_uring is enabled (and not falling back to the thread pool),
- // the first poll should return Pending.
- let _pending = pin!(fut).poll_unpin(cx);
-
- tx.send(()).unwrap();
-
- Poll::<()>::Pending
- })
+ let child = workdir.path().join("child");
+ let mut fut = pin!(tokio::fs::create_dir(&child));
+ let mut first_poll = true;
+ poll_fn(|cx| {
+ if first_poll {
+ assert_pending!(fut.as_mut().poll(cx));
+ first_poll = false;
+ tx.send(()).unwrap();
+ }
+ Poll::<()>::Pending
+ })
.await;📝 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.
| poll_fn(|cx| { | |
| let child = workdir.path().join("child"); | |
| let fut = tokio::fs::create_dir(&child); | |
| // If io_uring is enabled (and not falling back to the thread pool), | |
| // the first poll should return Pending. | |
| let _pending = pin!(fut).poll_unpin(cx); | |
| let child = workdir.path().join("child"); | |
| let mut fut = pin!(tokio::fs::create_dir(&child)); | |
| let mut first_poll = true; | |
| poll_fn(|cx| { | |
| if first_poll { | |
| assert_pending!(fut.as_mut().poll(cx)); | |
| first_poll = false; | |
| tx.send(()).unwrap(); | |
| } | |
| Poll::<()>::Pending | |
| }) | |
| .await; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tokio/tests/fs_uring_mkdir.rs` around lines 146 - 153, The test recreates the
create_dir future on every poll because `fut` is declared inside the `poll_fn`
closure; move the one-shot operation out so the same in-flight future is polled
across invocations. Concretely, allocate and pin the
`tokio::fs::create_dir(&child)` future once (e.g., a `let mut fut =
Box::pin(...)` or `Option<Pin<_>>`), then have the `poll_fn` closure poll that
pinned `fut` via `poll_unpin` instead of reassigning a new `fut` each poll;
update the `cancel_op_future` test harness to reference this single pinned
future so the cancellation behavior is exercised correctly.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The test assumes that the poll_fn will be called just once because the handle should be aborted after the first poll but due to timings the poll_fn may be called more than once and then the a new future will be created. This does not do any harm but it is confusing
value:useful; category:bug; feedback: The Claude AI reviewer is correct! If the metadata read operation fails it will hide the original error and the developer won't have enough information to debug the problem. The logic should be improved to return both errors if the metadata read fails. |
2 participants
7847: To review by AI