Prototype for batching statx with io_uring - #1302
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe workspace adds ChangesHost filesystem scanning
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant scan_inner
participant IoUring
participant update_entry
scan_inner->>IoUring: Submit batched statx requests
IoUring-->>scan_inner: Return statx completions
scan_inner->>scan_inner: Classify files and directories
scan_inner->>update_entry: Update matching scan entries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1302 +/- ##
==========================================
+ Coverage 33.22% 35.59% +2.37%
==========================================
Files 21 22 +1
Lines 2971 3214 +243
Branches 2971 3214 +243
==========================================
+ Hits 987 1144 +157
- Misses 1981 2067 +86
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
fact/src/host_scanner.rs (1)
205-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
update_entrycall acrossis_file/is_dirbranches.Both branches call
self.update_entry(path.as_path())with an identical.with_context(...), differing only in which metric label to increment. Consider merging to avoid the duplication.Also note: once the pointer/mask bugs above are fixed, if
stx_modestill doesn't match eitherS_IFREGorS_IFDIR(e.g., symlinks, sockets, devices), the entry is silently skipped with no metric increment or log — worth at least a debug log for visibility.♻️ Proposed refactor
- let is_dir = (statxbuf.stx_mode as u32 & libc::S_IFMT) == libc::S_IFDIR; - let is_file = (statxbuf.stx_mode as u32 & libc::S_IFMT) == libc::S_IFREG; - - if is_file { - self.metrics.scan_inc(ScanLabels::FileScanned); - self.update_entry(path.as_path()).with_context(|| { - format!("Failed to update entry for {}", path.display()) - })?; - } else if is_dir { - self.metrics.scan_inc(ScanLabels::DirectoryScanned); - self.update_entry(path.as_path()).with_context(|| { - format!("Failed to update entry for {}", path.display()) - })?; - } + let file_type = statxbuf.stx_mode as u32 & libc::S_IFMT; + let label = match file_type { + libc::S_IFREG => Some(ScanLabels::FileScanned), + libc::S_IFDIR => Some(ScanLabels::DirectoryScanned), + _ => None, + }; + + if let Some(label) = label { + self.metrics.scan_inc(label); + self.update_entry(path.as_path()).with_context(|| { + format!("Failed to update entry for {}", path.display()) + })?; + } else { + debug!("Unclassified statx mode {:`#o`} for {}", statxbuf.stx_mode, path.display()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/host_scanner.rs` around lines 205 - 222, Refactor the file and directory handling in the batch loop around IoUringStatx so the metric label is selected and incremented per type, then call self.update_entry(path.as_path()) with the shared context only once. Preserve skipping unsupported file types, but add debug-level visibility for entries whose mode matches neither S_IFREG nor S_IFDIR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fact/src/host_scanner.rs`:
- Around line 192-203: Update scan_inner to propagate the submission error from
ring.submission().push(op) through its existing anyhow::Result path instead of
panicking with expect. Replace the hardcoded user_data(0x99) when building each
op with a per-entry identifier so the completion debug log can distinguish
entries.
- Line 182: Update the statx mask in the host-scanning call to use bitwise OR
when combining requested flags, or only libc::STATX_TYPE since the result checks
stx_mode file-type bits. Remove the current libc::STATX_TYPE & libc::STATX_INO
expression and retain STATX_INO only if the returned inode is used later.
- Around line 163-188: Update the statx buffer ownership in the glob mapping
that constructs IoUringStatx so the address passed to opcode::Statx::new remains
stable through submission and completion. Allocate the statxbuf with stable heap
storage, such as a Box, and retain that ownership until the io_uring operation
completes; ensure moving the operation into the collected Vec cannot relocate
the kernel target.
- Line 161: Replace the bitwise-XOR expressions used for the IoUring ring size
and submission chunk size in the host scanner with the intended batch size of
128, using either a literal or an equivalent power-of-two expression.
---
Nitpick comments:
In `@fact/src/host_scanner.rs`:
- Around line 205-222: Refactor the file and directory handling in the batch
loop around IoUringStatx so the metric label is selected and incremented per
type, then call self.update_entry(path.as_path()) with the shared context only
once. Preserve skipping unsupported file types, but add debug-level visibility
for entries whose mode matches neither S_IFREG nor S_IFDIR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 65c6548e-2187-486b-ba5c-d819832c523b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlfact/Cargo.tomlfact/src/host_scanner.rs
| unsafe { | ||
| ring.submission() | ||
| .push(op) | ||
| .expect("submission queue is full"); | ||
| } | ||
| } | ||
|
|
||
| ring.submit_and_wait(batch.len())?; | ||
|
|
||
| for cqe in ring.completion() { | ||
| debug!("IoUring CQE {:?}, {:?}", cqe.user_data(), cqe.result()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 💤 Low value
Prefer propagating submission errors instead of panicking.
scan_inner already returns anyhow::Result<()>; using .expect("submission queue is full") inside the loop will abort the scan task rather than letting the caller recover/log via the existing ? error path.
Separately, every op is built with the same hardcoded user_data(0x99) (line 184), so the debug!("IoUring CQE {:?}, {:?}", ...) log can't distinguish which entry a given completion belongs to; consider tagging with a per-entry index for useful debugging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fact/src/host_scanner.rs` around lines 192 - 203, Update scan_inner to
propagate the submission error from ring.submission().push(op) through its
existing anyhow::Result path instead of panicking with expect. Replace the
hardcoded user_data(0x99) when building each op with a per-entry identifier so
the completion debug log can distinguish entries.
abf869b to
93b4b2f
Compare
93b4b2f to
49d058a
Compare
2 participants
Description
Prototype, statx buffer is not filled.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.
Summary by CodeRabbit