fix(host_scanner): reduce syscalls during scans#1221
Conversation
Add a histogram capturing the duration of a host_scanner scan.
Instead of using `PathBuf::is_file`, `PathBuf::is_dir` and `PathBuf::metadata` individually, each of which produce independent calls to `stat` under the hood, use a single call to `PathBuf::metadata` and propagate it down to the places it is needed.
Avoid additional syscalls to insert inodes into the kernel BPF inode map if the userspace already has said inode.
📝 WalkthroughWalkthroughHost scanning now records scan durations, logs lifecycle events at info level, skips glob and metadata lookup failures, passes fetched metadata through updates, and avoids redundant inode tracking work. ChangesHost scanner observability and resilient inode tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1221 +/- ##
==========================================
- Coverage 33.22% 32.94% -0.28%
==========================================
Files 21 21
Lines 2971 2996 +25
Branches 2971 2996 +25
==========================================
Hits 987 987
- Misses 1981 2006 +25
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/host_scanner.rs (1)
205-234: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not retain a userspace inode when kernel insertion fails.
Line 217 inserts into
inode_mapbefore the BPF insertion at Line 221. On a full or failed kernel map insertion, later scans hit Lines 207-213 and never retry BPF tracking, leaving the inode falsely marked as monitored. Insert intoinode_maponly after kernel insertion succeeds, or roll back the userspace entry on every error path.🤖 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 - 234, The inode_map entry must not remain when kernel_inode_map insertion fails. Update the new-inode path around inode_map and kernel_inode_map so the userspace inode is inserted only after the kernel insertion succeeds, or remove it on every error path, while preserving the existing already-tracked path and error reporting.
🤖 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.
Outside diff comments:
In `@fact/src/host_scanner.rs`:
- Around line 205-234: The inode_map entry must not remain when kernel_inode_map
insertion fails. Update the new-inode path around inode_map and kernel_inode_map
so the userspace inode is inserted only after the kernel insertion succeeds, or
remove it on every error path, while preserving the existing already-tracked
path and error reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 5edde49b-9885-47e7-a91d-25e6873b308f
📒 Files selected for processing (2)
fact/src/host_scanner.rsfact/src/metrics/host_scanner.rs
|
/retest |
2 participants
Description
This PR makes some efforts to try and reduce the amount of syscalls that
HostScannerdoes when scanning through the filesystem. This is achieved essentially with two changes:statcalls.The first one is done by replacing individual calls to
PathBuf::is_file,PathBuf::is_dirandPathBuf::metadata, all of which callstatunder the hood, with a singlePathBuf::metadatacall and using it where needed.The second one is done by first checking if the userspace inode map has the found inode and if it is already there, we simply skip inserting it kernel side, since it should've been put there in a previous scan.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Reduced number of syscalls
Running fact with the following command:
FACT_LOGLEVEL=info RUST_BACKTRACE=1 cargo srun --bin fact --all-features -- -p '/etc/**/*:/etc/' --inodes-max=2097152 --expose-metrics --scan-interval 30, then usingperf stat -e 'syscalls:sys_enter_statx,syscalls:sys_enter_bpf' -p "$(pgrep fact)" -- sleep 30capturing a single scan. No events generated during the run, no modifications to the/etcdirectory, 25966 inodes tracked for all the scans.Before changes:
After all changes:
Average scan time
Run
factwith the following command in each commit in this PR:FACT_LOGLEVEL=info RUST_BACKTRACE=1 cargo srun --bin fact --all-features -- -p '/etc/**/*:/etc/' --inodes-max=2097152 --expose-metrics --scan-interval 1. No events generated during the run, no modifications to the/etcdirectory, 25966 inodes tracked for all the scans.Average scan time adding scan duration to log and metric: 162.48ms
Average scan time using a single metadata call: 136.44ms
Average scan time skipping re-insertion of inodes in BPF map: 114.19ms
Summary by CodeRabbit
Improvements
Monitoring