btrfs: Add bd_btrfs_device_stats() to expose per-device error stats#1192
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds support for exposing Btrfs device statistics through libblockdev. It introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/plugins/btrfs.c (1)
997-1034: Bound enumeration by the reported device count.Iterating until
fs_info.max_idcan issue a large number ofBTRFS_IOC_DEV_INFOcalls on filesystems with sparse/high device IDs. Stop oncefs_info.num_devicesentries have been collected to avoid avoidable latency.♻️ Proposed loop bound
- for (guint64 devid = 1; devid <= fs_info.max_id; devid++) { + for (guint64 devid = 1; devid <= fs_info.max_id && stats_array->len < fs_info.num_devices; devid++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/btrfs.c` around lines 997 - 1034, The loop currently iterates from 1 to fs_info.max_id calling BTRFS_IOC_DEV_INFO and BTRFS_IOC_GET_DEV_STATS for every id which can be very large; change the logic in the loop that uses devid to stop once you've successfully collected fs_info.num_devices entries into stats_array (e.g., maintain a counter incremented when you g_ptr_array_add a BDBtrfsDeviceStats) so you break out early when count == fs_info.num_devices, keeping the existing ioctl/error handling for BTRFS_IOC_DEV_INFO and BTRFS_IOC_GET_DEV_STATS and using the same variables (devid, dev_info, dev_stats, stats_array, fs_info.num_devices).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/plugin_apis/btrfs.api`:
- Around line 536-546: The documentation/availability mismatch:
bd_btrfs_device_stats is an ioctl-only API but the tech category in its header
(%BD_BTRFS_TECH_MULTI_DEV-%BD_BTRFS_TECH_MODE_QUERY) is treated by
bd_btrfs_is_tech_avail() as requiring the btrfs CLI; either add a new tech
category (e.g. %BD_BTRFS_TECH_DEVICE_STATS) and document bd_btrfs_device_stats
under that category, or update bd_btrfs_is_tech_avail() to treat this specific
category as ioctl-only (not requiring btrfs-progs); update the header comment
for bd_btrfs_device_stats and the tech enum/lookup so consumers can observe
availability without the CLI present and ensure bd_btrfs_is_tech_avail(), the
tech enum, and any docstrings are kept consistent.
In `@src/plugins/btrfs.c`:
- Around line 977-978: The availability check currently routes through
bd_btrfs_is_tech_avail() which requires the btrfs CLI, but
bd_btrfs_device_stats() only needs the kernel module; update the
module-dependency check to reflect the no-CLI implementation by removing or
replacing MODULE_DEPS_BTRFS_MASK with the kernel-only dependency mask (or a new
MODULE_DEPS_BTRFS_KERNEL constant) in the check_module_deps call used by
bd_btrfs_is_tech_avail() and the checks around bd_btrfs_device_stats(); ensure
the change touches the check_module_deps(invocation using avail_module_deps,
module_deps, MODULE_DEPS_LAST, deps_check_lock, error) sites so availability no
longer requires the btrfs CLI.
---
Nitpick comments:
In `@src/plugins/btrfs.c`:
- Around line 997-1034: The loop currently iterates from 1 to fs_info.max_id
calling BTRFS_IOC_DEV_INFO and BTRFS_IOC_GET_DEV_STATS for every id which can be
very large; change the logic in the loop that uses devid to stop once you've
successfully collected fs_info.num_devices entries into stats_array (e.g.,
maintain a counter incremented when you g_ptr_array_add a BDBtrfsDeviceStats) so
you break out early when count == fs_info.num_devices, keeping the existing
ioctl/error handling for BTRFS_IOC_DEV_INFO and BTRFS_IOC_GET_DEV_STATS and
using the same variables (devid, dev_info, dev_stats, stats_array,
fs_info.num_devices).
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1ecfe5a-b256-45c0-815f-a04037295b74
📒 Files selected for processing (5)
docs/libblockdev-sections.txtsrc/lib/plugin_apis/btrfs.apisrc/plugins/btrfs.csrc/plugins/btrfs.htests/btrfs_test.py
Add a new function that retrieves btrfs device statistics (write/read/flush I/O errors, corruption errors, generation errors) using the BTRFS_IOC_GET_DEV_STATS ioctl. This avoids depending on the btrfs CLI tool and works without elevated privileges. Fixes: storaged-project#1167 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
vojtechtrefny
force-pushed
the
master_btrfs-device-stats
branch
from
cb1386e to
b821d8a
Compare
|
@jelly please take a look |
vojtechtrefny
merged commit 770148e
into
storaged-project:master
4 participants
Add a new function that retrieves btrfs device statistics (write/read/flush I/O errors, corruption errors, generation errors) using the BTRFS_IOC_GET_DEV_STATS ioctl. This avoids depending on the btrfs CLI tool and works without elevated privileges.
Fixes: #1167
Summary by CodeRabbit