7846: fs: implement tokio::fs::hard_link via io_uring#50
7846: fs: implement tokio::fs::hard_link via io_uring#50martin-augment wants to merge 1 commit intomasterfrom
Conversation
WalkthroughThis change introduces io-uring support for the ✨ Finishing touches
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: fs: implement tokio::fs::hard_link via io_uringSummaryThis PR implements ✅ Code Quality & Best PracticesStrengths:
Code structure (tokio/src/io/uring/link.rs):
🐛 Potential Issues1. Return Type Consistency (tokio/src/io/uring/link.rs:26)The Current: fn complete(self, cqe: CqeResult) -> Self::Output {
cqe.result.map(|_| ())
}This is actually fine - consistent with how other void operations would work. 2. Test Implementation Detail (tokio/tests/fs_uring_link.rs:61-70)In Suggestion: Add a clarifying comment: // Spawn operations and poll once to register them with io_uring,
// then drop the futures to test cancellation during shutdown⚡ Performance ConsiderationsPositive:
Note:
🔒 Security ConcernsOverall: Good security posture ✅ Proper lifetime management: CString fields are held for the operation duration with Minor consideration:
🧪 Test CoverageExcellent test coverage: ✅ Runtime shutdown during operations (
✅ High concurrency test (
✅ Cancellation test (
Test quality observations:
📝 Minor Suggestions
✨ SummaryThis is a high-quality implementation that:
Recommendation: Approve with minor suggestions The implementation is production-ready. The suggestions above are minor enhancements rather than blocking issues. Great work! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tokio/tests/fs_uring_link.rs (2)
40-90:shutdown_runtime_while_performing_io_uring_opslikely doesn’t exercisecurrent_threadruntime, and may be too “stress-y” for CI.Line 40–90:
rt.spawn(...)without ablock_on/enterdriver means thecurrent_threadruntime (Line 42) may never actually run the spawned task; shutdown then “passes” without covering io_uring ops. Also,multi_rt(256)+ the unboundedfor i in 0..usize::MAXspawn loop can be flaky/heavy.Possible tightening (example approach)
- Exclude
current_rt()from this test (or restructure to actively drive it).- Bound the work by time or a fixed task count (e.g., 5k) and/or lower max worker threads for CI stability.
132-160:cancel_op_futurecurrently proves task abortion, not necessarily io_uring op cancellation.Line 132–160: since you don’t assert the first poll is
Pending(andhard_linkcould complete immediately or fall back to the threadpool), the test can pass without ever having an in-flight uring op to cancel. If the intent is “uring cancellation path works”, it likely needs a stronger signal that the operation was submitted and is still pending.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tokio/src/fs/hard_link.rstokio/src/io/uring/link.rstokio/src/io/uring/mod.rstokio/src/runtime/driver/op.rstokio/tests/fs_uring_link.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/io/uring/mod.rs
🧬 Code graph analysis (5)
tokio/src/io/uring/mod.rs (1)
tokio/src/io/uring/link.rs (1)
link(43-57)
tokio/src/runtime/driver/op.rs (1)
tokio/src/io/uring/link.rs (1)
link(43-57)
tokio/src/io/uring/link.rs (2)
tokio/src/io/uring/utils.rs (1)
cstr(4-6)tokio/src/runtime/driver/op.rs (4)
complete(117-117)complete_with_error(123-123)cancel(128-128)new(66-73)
tokio/src/fs/hard_link.rs (2)
tokio/src/runtime/handle.rs (1)
current(144-148)tokio/src/io/uring/link.rs (1)
link(43-57)
tokio/tests/fs_uring_link.rs (3)
tokio/src/runtime/driver/op.rs (2)
new(66-73)poll(136-208)tokio/src/io/uring/link.rs (1)
link(43-57)tokio/src/fs/hard_link.rs (1)
hard_link(39-59)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
tokio/tests/fs_uring_link.rs (2)
92-130:hard_link_many_filesis solid; consider limiting concurrency if CI shows sporadic failures.Line 92–130: this is a good correctness check (dev+ino equality). If CI gets noisy, consider batching the 10k metadata tasks to avoid saturating the scheduler/file system.
162-172: Helper looks fine.Line 162–172:
create_tmp_diris straightforward and creates the target file in a temp directory as expected.tokio/src/io/uring/mod.rs (1)
1-1: The newlinkmodule is properly cfg-gated.The entire
uringmodule is wrapped withcfg_io_uring!at the parent level intokio/src/io/mod.rs, so all submodules—includinglink—automatically inherit this feature gate. The sibling modules (open,read,utils,write) follow the same pattern without additional cfg wrapping, confirming that this approach is consistent throughout the codebase.tokio/src/runtime/driver/op.rs (1)
1-4: The imports inop.rsare properly gated and safe. Bothop.rsand theuringmodule (includinglink.rs) are conditionally compiled withcfg_io_uring!, which expands to#[cfg(all(tokio_unstable, feature = "io-uring", feature = "rt", feature = "fs", target_os = "linux"))]. Since both the importer and the imported modules share identical cfg guards, there is no risk of a build break from missing definitions.Likely an incorrect or invalid review comment.
tokio/src/io/uring/link.rs (1)
11-58: Implementation is correct; cfg-gating and LinkAt opcode API verified.The
Linkstruct properly storesCStrings to keep kernel pointers valid throughout the operation lifetime, andLinkAt::new()parameter order (olddirfd, oldpath, newdirfd, newpath) matches both the code usage and thelinkat(2)system call. Theuringmodule is already feature-gated withcfg_io_uring!intokio/src/io/mod.rs, solink.rsinherits this protection without needing additional cfg directives. TheCompletableandCancellableimplementations correctly handle operation completion and cancellation.tokio/src/fs/hard_link.rs (1)
39-56: Implementation is correct.io_uring::opcode::LinkAt::CODEexists in io-uring 0.7.11 and properly corresponds tolinkat(2). Thecheck_and_initstate machine handles supported/unsupported scenarios correctly and is idempotent (cheap on subsequent calls).The fast-path wiring is sound. The conditional io-uring block correctly:
- Uses
LinkAt::CODEconstant from io-uring 0.7.11- Calls
check_and_initto verify io_uring is available and thelinkatopcode is supported- Returns
Ok(true)only when both are available;Ok(false)gracefully when either is unsupported (ENOSYS for kernel too old, EPERM for seccomp)- Falls back cleanly to
asyncifyfor blocking I/O when io_uring is unavailableThe state machine is idempotent: first call performs setup; subsequent calls are fast checks. This pattern is consistently used across filesystem operations (read, write, open).
🤖 Augment PR SummarySummary: Implements an io_uring-backed fast path for Changes:
Technical Notes: Uses 🤖 Was this summary useful? React with 👍 or 👎 |
|
|
||
| rt.spawn(async move { | ||
| // spawning a bunch of uring operations. | ||
| for i in 0..usize::MAX { |
There was a problem hiding this comment.
This loop can create an unbounded number of distinct hard-link directory entries before shutdown (bounded only by timing), which risks inode/disk exhaustion or CI flakiness. Consider bounding the number of iterations/links (similar to other uring shutdown stress tests) so the test remains robust across slower machines.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:incorrect-but-reasonable; category:bug; feedback:The Augment AI reviewer is partially correct! Indeed using usize::MAX would lead to a huge number of spawned tasks but the test also spawns a new OS thread that shuts down the Tokio runtime after 300ms. The test actually tests that the shutdown works even when there are active tasks. So, the number of tasks might become big but it won't really loon until usize::MAX.
| poll_fn(|cx| { | ||
| assert_pending!(fut.as_mut().poll(cx)); | ||
| Poll::<()>::Pending | ||
| }) |
There was a problem hiding this comment.
Test assertion fails if io_uring operation completes quickly
Medium Severity
The poll_fn closure always returns Poll::Pending, but when the io_uring hard_link operation completes, the task gets woken and poll_fn is polled again. On subsequent polls, fut.as_mut().poll(cx) returns Poll::Ready (since the operation completed), causing assert_pending! to panic. This can cause intermittent test failures when filesystem operations complete quickly between polls.
There was a problem hiding this comment.
value:useful; category:bug; feedback:The Augment AI reviewer is correct! The test would not be very stable because it assumes that the io_uring operation is still being processed by the Linux kernel and the first poll will be always Pending. But it might be that the issued io_uring operation finishes before the first assertion and in this case the result will be Poll::Ready and it will fail. Prevents introducing a flacky test that will fail randomly.
value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct! The CurrentThreadRuntime needs to wrap the tested logic in block_on() call to actually execute async tasks. The same change would work for the multi thread runtime too. Prevents introducing new tests which actually do not execute and do not test anything. |
value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct! There is no easy way to test that an io_uring operation executing in the Linux kernel can be cancelled due to timing issues. The test verifies that the Tokio task can be cancelled and its result be ignored, i.e. only the user-space logic could be cancelled at any time. |
7846: To review by AI