7844: fs: implement tokio::fs::symlink via io_uring#49
7844: fs: implement tokio::fs::symlink via io_uring#49martin-augment wants to merge 1 commit intomasterfrom
Conversation
WalkthroughThis change implements io-uring based symlink operations in Tokio for Linux systems when 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 |
![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 (3)
tokio/tests/fs_uring_symlink.rs (2)
50-89: Test logic is correct for shutdown stress-testing.The test intentionally spawns operations in an unbounded loop (line 58) to stress-test runtime shutdown behavior with many in-flight io_uring operations. The
shutdown_timeoutensures the test completes within 300ms.Optional: Add clarifying comment about intentional loop behavior
rt.spawn(async move { - // spawning a bunch of uring operations. + // Intentionally spawn an unbounded number of operations to stress-test + // shutdown behavior with many in-flight io_uring ops. for i in 0..usize::MAX {
128-156: Consider clarifying the expected poll behavior.The test correctly validates cancellation semantics, but there's a minor inconsistency: the comment on lines 138-139 states "the first poll should return Pending," yet line 140 doesn't validate this expectation.
If the io_uring path is guaranteed to return
Pendingon first poll, consider asserting this:- // 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); + // When using io_uring (not falling back to the thread pool), + // the first poll returns Pending. + let poll_result = pin!(fut).poll_unpin(cx); + assert!(poll_result.is_pending());Alternatively, if the behavior varies based on whether io_uring is actually used, the comment could be updated to reflect that this is expected but not enforced.
tokio/src/io/uring/symlink.rs (1)
41-54: LGTM! Implementation is correct and safe.The constructor properly:
- Converts paths to null-terminated C strings
- Uses correct argument order for
SymlinkAt(dirfd, target, linkpath)- Ensures pointer validity by moving
CStrings into theSymlinkstruct, which is owned by theOpfor the operation's lifetimeThe safety invariant is maintained: the
CStringinstances (and thus their pointers) remain valid because they're owned by theSymlinkstruct, which in turn is owned by theOpthat must stay alive until the io_uring operation completes.Optional: Enhance safety documentation for clarity
Consider expanding the safety comment to be more explicit about the ownership chain:
- // SAFETY: Parameters are valid for the entire duration of the operation + // SAFETY: The CString pointers remain valid for the operation's lifetime because + // the CStrings are moved into the Symlink struct, which is owned by the Op. The Op + // must remain alive until the io_uring operation completes, ensuring pointer validity. Ok(unsafe { Op::new(symlink_op, Symlink { original, link }) })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tokio/src/fs/symlink.rstokio/src/io/uring/mod.rstokio/src/io/uring/symlink.rstokio/src/runtime/driver/op.rstokio/tests/fs_uring_symlink.rs
🧰 Additional context used
🧠 Learnings (1)
📚 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/tests/fs_uring_symlink.rs
🧬 Code graph analysis (5)
tokio/src/runtime/driver/op.rs (2)
tokio/src/fs/symlink.rs (1)
symlink(11-31)tokio/src/io/uring/symlink.rs (1)
symlink(43-53)
tokio/src/fs/symlink.rs (2)
tokio/src/runtime/handle.rs (1)
current(144-148)tokio/src/io/uring/symlink.rs (1)
symlink(43-53)
tokio/src/io/uring/mod.rs (2)
tokio/src/fs/symlink.rs (1)
symlink(11-31)tokio/src/io/uring/symlink.rs (1)
symlink(43-53)
tokio/tests/fs_uring_symlink.rs (2)
tokio/src/fs/symlink.rs (1)
symlink(11-31)tokio/src/io/uring/symlink.rs (1)
symlink(43-53)
tokio/src/io/uring/symlink.rs (3)
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/symlink.rs (1)
symlink(11-31)
⏰ 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/io/uring/mod.rs (1)
3-3: LGTM! Module declaration follows existing patterns.The module declaration is consistent with other io_uring operation modules and properly positioned.
tokio/src/fs/symlink.rs (1)
15-30: LGTM! io_uring fast path with proper fallback.The implementation correctly:
- Feature-gates the io_uring path behind appropriate flags
- Checks driver support before attempting to use io_uring
- Falls back gracefully to the thread pool-based asyncify approach
- Maintains the same public API regardless of the execution path
tokio/src/runtime/driver/op.rs (1)
3-3: LGTM! Cancellation support follows established patterns.The changes correctly integrate symlink into the cancellation infrastructure:
- Import is properly placed alongside other io_uring operations
- CancelData variant follows the same pattern as Open, Write, and Read
- No additional lifecycle handling needed due to the existing generic implementation
Also applies to: 23-23
tokio/tests/fs_uring_symlink.rs (4)
3-9: LGTM! Feature gates are correct.The test module is properly gated behind the same features as the implementation.
25-48: LGTM! Comprehensive runtime test coverage.Testing across multiple runtime configurations (current-thread and multi-thread with varying worker counts) ensures the implementation works correctly in different scenarios.
91-126: LGTM! Effective stress test for concurrent operations.The test validates both correctness and performance with 10,000 concurrent symlink operations, then verifies each symlink resolves correctly.
158-168: LGTM! Helper correctly sets up test fixtures.The helper creates a temporary directory with a target file for symlink tests, with automatic cleanup via
TempDir's Drop implementation.tokio/src/io/uring/symlink.rs (3)
11-21: LGTM! Struct correctly manages string lifetimes.The
Symlinkstruct properly owns theCStringinstances, ensuring the pointers passed to io_uring remain valid for the operation's duration. The#[allow(dead_code)]attributes are appropriate since these fields are accessed by the kernel, not Rust code.
23-33: LGTM! Completable implementation is correct.The trait implementation properly:
- Maps successful io_uring results to
()(symlink doesn't return meaningful data)- Propagates errors through
complete_with_error
35-39: LGTM! Cancellable implementation follows the established pattern.
Pull Request Review: io_uring symlink implementationSummaryThis PR implements ✅ StrengthsCode Quality
Test Coverage
Safety
🔍 Issues & Suggestions1. Critical: Infinite loop in shutdown test (tokio/tests/fs_uring_symlink.rs:57)The loop runs to Recommendation: Use a reasonable constant like other tests do (e.g., 100 or 1000) 2. Test quality: Unnecessary spawning in shutdown test (tokio/tests/fs_uring_symlink.rs:60)The shutdown test spawns individual tasks for each symlink operation, but never awaits or tracks them. This creates tasks that are abandoned, making the test behavior less predictable. Recommendation: Consider using a pattern more similar to fs_uring_read.rs:50-75 where futures are stored and managed. 3. Documentation: Missing module-level docs (tokio/src/io/uring/symlink.rs:1)The file lacks a module-level documentation comment explaining what SymlinkAt does and its semantics. 4. Missing error case testsTests are missing for error cases like invalid paths, permission denied, existing files, paths with null bytes, and symlink chains. 🔒 Security Considerations✅ No Security Issues Found
⚡ Performance Considerations✅ Good Performance Characteristics
🎯 Overall AssessmentStatus: Approve with minor fixes recommended The implementation is solid and follows tokio's patterns well. The main concern is the infinite loop in the shutdown test which must be fixed before merge. Required Changes
Recommended Changes
Code Quality Score: 8/10
Great work on this implementation! The code quality is high and it integrates well with the existing io_uring infrastructure. |
🤖 Augment PR SummarySummary: This PR adds an Changes:
Technical Notes: The io_uring path is enabled only when the runtime + fs + io_uring features are present and the kernel reports 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| multi_rt(2), | ||
| multi_rt(8), | ||
| multi_rt(64), | ||
| multi_rt(256), |
There was a problem hiding this comment.
Including multi_rt(256) can create hundreds of OS threads during CI, which may exceed resource limits and make this test flaky. Worth considering whether a smaller max thread count would still cover the intended stress scenario.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:annoying; category:bug; feedback:The Augment AI reviewer is not correct! The Tokio runtimes do not create OS threads for each task! They create num_cpus OS threads and schedule all tasks on them, so the footprint in the Linux kernel memory is kept in boundaries.
|
|
||
| rt.spawn(async move { | ||
| // spawning a bunch of uring operations. | ||
| for i in 0..usize::MAX { |
There was a problem hiding this comment.
The for i in 0..usize::MAX loop can enqueue an unbounded number of tasks before the shutdown thread fires, which can lead to high memory usage/flake on slower machines. Consider bounding the number of spawned ops or making it time-based to keep the test more deterministic.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:valid-but-wont-fix; category:bug; feedback:The Augment AI reviewer is not correct! The Tokio tasks are very lightweight, so they won't use much memory and they will be scheduled on up to N OS threads (at most 256 in this unit test), so there is no need to bounding the number of the tasks.
value:incorrect-but-reasonable; category:bug; feedback:The Claude AI reviewer is not correct! The huge amount of tasks are scheduled in the Tokio runtime but at the same time an OS thread is scheduled to execute after 300ms to shutdown the runtime, so only a few hundreds/thousands tasks will be actually ran. |
value:incorrect-but-reasonable; category:bug; feedback:The Claude AI reviewer is not correct! The purpose of the test is to give a lot of work to the Tokio runtime (to spawn many tasks) and to shut it down in a separate OS thread. It tests that the runtime could be shut down even under a high load. |
2 participants
7844: To review by AI