Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions tokio/src/fs/create_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,21 @@ use std::path::Path;
/// ```
pub async fn create_dir(path: impl AsRef<Path>) -> io::Result<()> {
let path = path.as_ref().to_owned();

#[cfg(all(
tokio_unstable,
feature = "io-uring",
feature = "rt",
feature = "fs",
target_os = "linux"
))]
{
let handle = crate::runtime::Handle::current();
let driver_handle = handle.inner.driver().io();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if driver_handle.check_and_init(io_uring::opcode::MkDirAt::CODE)? {
return crate::runtime::driver::op::Op::mkdir(&path)?.await;
}
}

asyncify(move || std::fs::create_dir(path)).await
}
18 changes: 18 additions & 0 deletions tokio/src/fs/create_dir_all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,23 @@ use std::path::Path;
/// ```
pub async fn create_dir_all(path: impl AsRef<Path>) -> io::Result<()> {
let path = path.as_ref().to_owned();

#[cfg(all(
tokio_unstable,
feature = "io-uring",
feature = "rt",
feature = "fs",
target_os = "linux"
))]
{
use crate::fs::create_dir_all_uring;

let handle = crate::runtime::Handle::current();
let driver_handle = handle.inner.driver().io();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if driver_handle.check_and_init(io_uring::opcode::MkDirAt::CODE)? {
return create_dir_all_uring(&path).await;
}
}

asyncify(move || std::fs::create_dir_all(path)).await
}
90 changes: 90 additions & 0 deletions tokio/src/fs/create_dir_all_uring.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
#[cfg(all(
tokio_unstable,
feature = "io-uring",
feature = "rt",
feature = "fs",
target_os = "linux"
))]
use crate::runtime::driver::op::Op;
use std::ffi::OsStr;
use std::io;
use std::os::unix::ffi::OsStrExt;
use std::path::Path;

pub(crate) async fn create_dir_all_uring(path: &Path) -> io::Result<()> {
if path == Path::new("") {
return Ok(());
}

// First, check if the path exists.
if mkdir_parent_missing(path).await? {
return Ok(());
}

// Otherwise, we must create its parents.
// For /a/b/c/d, we must first try /a/b/c, /a/b, /a, and /.
// Hence, we can iterate over the positions of / in reverse,
// finding the first / that appears after a directory that already exists.
//
// For example, suppose /a exists, but none of its children.
// The creation of /a/b will be successful.
// Hence, first_valid_separator_pos = 4.
let mut first_valid_separator_pos = None;
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? {
Comment on lines +33 to +43
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

first_valid_separator_pos = Some(separator_pos);
break;
}
}
let first_valid_separator_pos = first_valid_separator_pos.unwrap_or(0);

// Once we have found the correct /,
// we can iterate the remaining components in the forward direction.
//
// In the example /a/b/c/d path, there is only one remaining /, after c.
// Hence, we first create /a/b/c.
//
// TODO: We're attempting to create all directories sequentially.
// This would benefit from batching.
Comment on lines +56 to +57
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for (separator_pos, _) in path_bytes
.iter()
.enumerate()
.skip(first_valid_separator_pos + 1)
.filter(|(_, &byte)| byte == b'/')
{
let parent_path = Path::new(OsStr::from_bytes(&path_bytes[..separator_pos]));
mkdir_parent_created(parent_path).await?;
}

// We must finally create the last path (/a/b/c/d in our example).
mkdir_parent_created(path).await?;
Ok(())
}

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),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Err(e) => Err(e),
Comment on lines +73 to +79
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

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),
}
}
2 changes: 2 additions & 0 deletions tokio/src/fs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,9 @@ cfg_windows! {
}

cfg_io_uring! {
pub(crate) mod create_dir_all_uring;
pub(crate) mod read_uring;
pub(crate) use self::create_dir_all_uring::create_dir_all_uring;
pub(crate) use self::read_uring::read_uring;

pub(crate) use self::open_options::UringOpenOptions;
Expand Down
49 changes: 49 additions & 0 deletions tokio/src/io/uring/mkdir.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
use super::utils::cstr;

use crate::runtime::driver::op::{CancelData, Cancellable, Completable, CqeResult, Op};

use io_uring::{opcode, types};
use std::ffi::CString;
use std::io;
use std::io::Error;
use std::path::Path;

#[derive(Debug)]
pub(crate) struct Mkdir {
/// This field will be read by the kernel during the operation, so we
/// need to ensure it is valid for the entire duration of the operation.
#[allow(dead_code)]
path: CString,
}

impl Completable for Mkdir {
type Output = io::Result<()>;

fn complete(self, cqe: CqeResult) -> Self::Output {
cqe.result.map(|_| ())
}

fn complete_with_error(self, err: Error) -> Self::Output {
Err(err)
}
}

impl Cancellable for Mkdir {
fn cancel(self) -> CancelData {
CancelData::Mkdir(self)
}
}

impl Op<Mkdir> {
/// Submit a request to create a directory.
pub(crate) fn mkdir(path: &Path) -> io::Result<Self> {
let path = cstr(path)?;

let mkdir_op = opcode::MkDirAt::new(types::Fd(libc::AT_FDCWD), path.as_ptr())
.mode(0o777)
.build();

// SAFETY: Parameters are valid for the entire duration of the operation
Ok(unsafe { Op::new(mkdir_op, Mkdir { path }) })
}
}
1 change: 1 addition & 0 deletions tokio/src/io/uring/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub(crate) mod mkdir;
pub(crate) mod open;
pub(crate) mod read;
pub(crate) mod utils;
Expand Down
2 changes: 2 additions & 0 deletions tokio/src/runtime/driver/op.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::io::uring::mkdir::Mkdir;
use crate::io::uring::open::Open;
use crate::io::uring::read::Read;
use crate::io::uring::write::Write;
Expand All @@ -16,6 +17,7 @@ use std::task::{Context, Poll, Waker};
#[allow(dead_code)]
#[derive(Debug)]
pub(crate) enum CancelData {
Mkdir(Mkdir),
Open(Open),
Write(Write),
Read(Read),
Expand Down
168 changes: 168 additions & 0 deletions tokio/tests/fs_uring_mkdir.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
//! Uring mkdir operations tests.

#![cfg(all(
tokio_unstable,
feature = "io-uring",
feature = "rt",
feature = "fs",
target_os = "linux"
))]

use futures::FutureExt;
use std::future::poll_fn;
use std::future::Future;
use std::pin::pin;
use std::sync::mpsc;
use std::task::Poll;
use std::time::Duration;
use tokio::runtime::{Builder, Runtime};
use tokio_test::assert_pending;
use tokio_util::task::TaskTracker;

fn multi_rt(n: usize) -> Box<dyn Fn() -> Runtime> {
Box::new(move || {
Builder::new_multi_thread()
.worker_threads(n)
.enable_all()
.build()
.unwrap()
})
}

fn current_rt() -> Box<dyn Fn() -> Runtime> {
Box::new(|| Builder::new_current_thread().enable_all().build().unwrap())
}

fn rt_combinations() -> Vec<Box<dyn Fn() -> Runtime>> {
vec![
current_rt(),
multi_rt(1),
multi_rt(2),
multi_rt(8),
multi_rt(64),
multi_rt(256),
]
}

#[test]
fn shutdown_runtime_while_performing_io_uring_ops() {
fn run(rt: Runtime) {
let (done_tx, done_rx) = mpsc::channel();
let workdir = tempfile::tempdir().unwrap();

rt.spawn(async move {
// spawning a bunch of uring operations.
for i in 0..usize::MAX {
let child = workdir.path().join(format!("{i}"));
tokio::spawn(async move {
let mut fut = pin!(tokio::fs::create_dir(&child));

poll_fn(|cx| {
assert_pending!(fut.as_mut().poll(cx));
Poll::<()>::Pending
})
.await;

fut.await.unwrap();
});
Comment on lines +60 to +67
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Avoid busy looping.
tokio::task::yield_now().await;
}
});

std::thread::spawn(move || {
rt.shutdown_timeout(Duration::from_millis(300));
done_tx.send(()).unwrap();
});

done_rx.recv().unwrap();
}

for rt in rt_combinations() {
run(rt());
}
}

#[test]
fn create_many_directories() {
fn run(rt: Runtime) {
let workdir = tempfile::tempdir().unwrap();

rt.block_on(async move {
const N_CHILDREN: usize = 10_000;

let tracker = TaskTracker::new();

for i in 0..N_CHILDREN {
let child = workdir.path().join(format!("{i}"));
tracker.spawn(async move {
tokio::fs::create_dir(&child).await.unwrap();
});
}
tracker.close();
tracker.wait().await;

let mut dir_iter = tokio::fs::read_dir(workdir.path()).await.unwrap();
let mut child_count = 0;
while dir_iter.next_entry().await.unwrap().is_some() {
child_count += 1;
}
assert_eq!(child_count, N_CHILDREN);
});
}

for rt in rt_combinations() {
run(rt());
}
}

#[tokio::test]
async fn create_dir_all_edge_cases() {
let workdir = tempfile::tempdir().unwrap();
let workdir_path = workdir.path();

tokio::fs::create_dir_all(workdir.path()).await.unwrap();

let nested_path = workdir_path.join("foo").join("bar");
tokio::fs::create_dir_all(&nested_path).await.unwrap();
assert!(nested_path.is_dir());
tokio::fs::create_dir_all(nested_path.parent().unwrap())
.await
.unwrap();
tokio::fs::create_dir_all(&nested_path).await.unwrap();

let slash_trailing = workdir_path.join("./baz/qux//");
tokio::fs::create_dir_all(&slash_trailing).await.unwrap();
assert!(slash_trailing.is_dir());
}

#[tokio::test]
async fn cancel_op_future() {
let workdir = tempfile::tempdir().unwrap();

let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel();
let handle = tokio::spawn(async move {
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);

Comment on lines +146 to +153
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tx.send(()).unwrap();

Poll::<()>::Pending
})
.await;
});

// Wait for the first poll
rx.recv().await.unwrap();

handle.abort();

let res = handle.await.unwrap_err();
assert!(res.is_cancelled());
}