feat(diskspace.plugin): smart ZFS dataset deduplication#21643
feat(diskspace.plugin): smart ZFS dataset deduplication#21643ilyam8 merged 15 commits intonetdata:masterfrom
Conversation
Replace unconditional ZFS dataset exclusion with a capacity-based heuristic that: - Excludes datasets that mirror pool capacity (no quota set) - Keeps datasets with quotas (different capacity than pool) - Keeps all datasets when pool itself is not mounted - Keeps all mounts inside LXC containers - Never excludes root filesystem New configuration options in [plugin:proc:diskspace]: - "zfs datasets heuristic" (default: yes) - enable smart heuristic - "exclude zfs datasets on paths" - manual pattern exclusion when heuristic is disabled Also exports is_lxcfs_proc_mounted() from proc.plugin for use by other collectors that need LXC container detection.
Avoid calling statvfs() for ZFS mounts on every collection cycle. Pool capacities and exclusion decisions are now cached and only rechecked every 5 minutes (ZFS_DATASET_RECHECK_SECONDS). This eliminates the performance overhead of the two-pass heuristic while still adapting to quota changes within a reasonable timeframe.
There was a problem hiding this comment.
Pull request overview
This PR implements smart ZFS dataset deduplication in diskspace.plugin to avoid duplicate metrics from datasets that mirror their parent pool's capacity. The solution uses a two-pass capacity-based heuristic to identify and exclude datasets without quotas, while keeping datasets with quotas and handling edge cases like LXC containers and unmounted pools.
Changes:
- Exported
is_lxcfs_proc_mounted()function from proc.plugin for shared use across collectors - Implemented two-pass ZFS dataset filtering: Pass 1 collects pool capacities, Pass 2 excludes datasets matching pool capacity
- Added configuration options for manual control via
zfs datasets heuristicandexclude zfs datasets on paths
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/collectors/proc.plugin/plugin_proc.h | Added function declaration for is_lxcfs_proc_mounted() to make it available to other collectors |
| src/collectors/proc.plugin/plugin_proc.c | Changed is_lxcfs_proc_mounted() from static to exported function |
| src/collectors/diskspace.plugin/plugin_diskspace.c | Core implementation: added ZFS pool tracking, capacity-based heuristic, LXC detection caching, and configuration options |
Comments suppressed due to low confidence (1)
src/collectors/proc.plugin/plugin_proc.c:125
- The condition
if (unlikely(!ff))on line 119 will always be true sinceffis initialized toNULLon line 117. This means the function opens /proc/self/mounts on every call but never closes it properly in the success path after the first open, leading to a file descriptor leak.
The logic should be restructured to avoid the unnecessary check. The variable initialization and file opening should be unconditional, or the check should be removed entirely since ff is always NULL at that point.
if (unlikely(!ff)) {
char filename[FILENAME_MAX + 1];
snprintfz(filename, FILENAME_MAX, "/proc/self/mounts");
ff = procfile_open(filename, " \t", PROCFILE_FLAG_DEFAULT);
if (unlikely(!ff))
return false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 4/5
- This PR appears safe to merge overall; the only reported issue is medium severity and limited to ZFS dataset handling.
- Most notable risk: in
src/collectors/diskspace.plugin/plugin_diskspace.c, slow-path collection bypasses the new ZFS dataset exclusion, so marked-slow datasets can still be collected and duplicates may persist. - Pay close attention to
src/collectors/diskspace.plugin/plugin_diskspace.c- ensure the ZFS exclusion heuristic applies consistently in both fast and slow paths.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/collectors/diskspace.plugin/plugin_diskspace.c">
<violation number="1" location="src/collectors/diskspace.plugin/plugin_diskspace.c:637">
P2: The new ZFS dataset exclusion is only applied in the fast path; slow-path collection bypasses it. ZFS datasets marked as slow will still be collected, so duplicates can persist despite the heuristic. Apply the same exclusion logic in the slow path (which may require carrying mount_source into basic_mountinfo) to keep behavior consistent.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Main as Diskspace Plugin
participant ZFS as NEW: ZFS Heuristic
participant State as Pool Info Dict
participant Kernel as OS / Kernel
Note over Main,Kernel: Initialization
Main->>Main: Load Config (zfs heuristic = yes)
Main->>State: Initialize Dictionary
Note over Main,Kernel: Collection Cycle (Start)
rect rgb(23, 37, 84)
Note over Main,Kernel: Pass 1: Build Pool Capacity Cache
Main->>ZFS: zfs_collect_pool_capacities()
ZFS->>Kernel: is_lxcfs_proc_mounted() (Cache result)
loop Every Mountpoint
ZFS->>ZFS: Is ZFS Pool? (No '/' in source)
opt Is Pool
ZFS->>Kernel: statvfs(mount_path)
ZFS->>State: Store max_capacity & seen_flag
end
end
end
rect rgb(30, 41, 59)
Note over Main,Kernel: Pass 2: Filter & Collect Stats
loop Every Mountpoint
Main->>Kernel: statvfs(mount_path)
Main->>ZFS: should_exclude_zfs_mount(mount, stats)
alt CHANGED: Exclusion Logic
ZFS->>ZFS: Check: Root? LXC? Not ZFS?
alt Is Dataset (Candidate for exclusion)
ZFS->>State: Get parent Pool info
alt Pool not mounted
ZFS-->>Main: Keep (Safety fallback)
else Dataset Capacity < Pool Capacity
ZFS-->>Main: Keep (Has Quota)
else Capacity == Pool Capacity
Note right of ZFS: Duplicate metric detected
ZFS-->>Main: NEW: EXCLUDE
end
else Is Pool / Root / Other
ZFS-->>Main: Keep
end
end
opt If Kept (Not Excluded)
Main->>Main: Collect metrics & update charts
end
end
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai review this PR |
@ilyam8 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Plugin as Diskspace Plugin
participant Proc as Proc Helper
participant Cache as ZFS Pool Cache
participant VFS as Kernel VFS
participant Core as Netdata Core
Note over Plugin,Cache: NEW: Phase 1 - Heuristic Prep (Every 5 mins)
Plugin->>Proc: CHANGED: is_lxcfs_proc_mounted()
Proc-->>Plugin: inside_lxc (bool)
opt Cache Expired (300s)
Plugin->>Cache: Flush Dictionary
loop Filter for ZFS Pools
Plugin->>VFS: Parse mount_source
alt Is ZFS Pool (no '/')
Plugin->>VFS: statvfs(pool_path)
VFS-->>Plugin: pool_capacity
Plugin->>Cache: Store max_capacity & mark_seen
end
end
end
Note over Plugin,Core: Phase 2 - Collection Cycle
loop Every Mount Point
Plugin->>VFS: statvfs(mount_path)
VFS-->>Plugin: mount_stats
alt NEW: ZFS Filtering Logic
Plugin->>Cache: Get pool info (max_capacity, seen)
alt Inside LXC OR Root OR Pool OR Heuristic=No
Plugin->>Plugin: Decision: KEEP
else Pool itself not mounted
Plugin->>Plugin: Decision: KEEP (Safety fallback)
else Dataset Size < Pool Max Size
Plugin->>Plugin: Decision: KEEP (Has Quota)
else Dataset Size ≈ Pool Max Size
Plugin->>Plugin: Decision: EXCLUDE (Duplicate)
end
end
opt Decision == KEEP
Plugin->>Core: Update Charts & Metrics
end
end
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/collectors/proc.plugin/plugin_proc.c:125
- The
is_lxcfs_proc_mountedfunction has a resource leak. Theprocfile *ffis initialized asNULLand checked withif (unlikely(!ff))which will always be true on first call. However, the variable is local (not static), so the file handle is never reused and is leaked on subsequent calls. The variableffshould be declared asstatic procfile *ff = NULL;to properly cache the file handle across calls, or the function should be redesigned to properly manage the resource lifecycle.
bool is_lxcfs_proc_mounted(void) {
procfile *ff = NULL;
if (unlikely(!ff)) {
char filename[FILENAME_MAX + 1];
snprintfz(filename, FILENAME_MAX, "/proc/self/mounts");
ff = procfile_open(filename, " \t", PROCFILE_FLAG_DEFAULT);
if (unlikely(!ff))
return false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix LXC detection to run unconditionally (once at startup) - Use "!*" as default pattern (explicitly exclude nothing) - Create dictionary/pattern only when needed for the active mode - Clarify tolerance comparison comments - Add mount_source to basic_mountinfo struct - Apply ZFS exclusion check to slow path (prevents duplicates)
f7f415b to
4d9bda4
Compare
Remove pointless conditional check (local variable was always NULL) and simplify control flow with single exit point for procfile_close().
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ensive checks - Handle NULL mount_source in basic_mountinfo_create_and_copy() to prevent strdupz(NULL) crash when mountinfo parsing encounters errors - Add defensive NULL check after dictionary_set() in zfs_collect_pool_capacities()
…ookup Use dictionary_get_and_acquire_item() to hold a reference while reading pool info values. This prevents a race where the main thread could flush the dictionary (freeing values) while the slow worker thread is reading from a pointer returned by dictionary_get().
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ison The previous logic tracked max_capacity across all ZFS mounts, but in ZFS statvfs returns total = used + available. This means datasets with data show larger "total" than empty datasets or the pool itself. For example: - rpool (pool): 1.7T total (empty) - rpool/ROOT/pve-1: 1.9T total (222G used + 1.7T available) Using max (1.9T) made empty datasets (1.7T) look like they had quotas. Fix: Track pool_capacity from the pool mount only, not datasets. Datasets with capacity >= pool capacity have no quota and should be excluded. Datasets with capacity < pool capacity have quotas and should be kept.
Move zfs_exclude_datasets_pattern initialization alongside other exclusion patterns in do_disk_space_stats(). Create it unconditionally so the config option always appears in netdata.conf, allowing users to customize it even when starting with heuristic enabled and later switching to pattern mode. Renamed option to "exclude space metrics on zfs datasets" for consistency with other exclusion options.
Bug: ZFS dataset exclusion worked for the first 5 minutes, then stopped. Datasets that should be excluded suddenly started being collected. Root cause: dictionary_flush() cleared all pool info entries every 5 minutes. If subsequent statvfs() calls failed (e.g., mount temporarily busy), the dictionary remained empty and should_exclude_zfs() couldn't find pool info, so it returned false (keep all datasets). Fix: Remove dictionary_flush() and update entries in place. The dictionary_set() overwrites existing entries when successful, but if statvfs fails, the previous valid pool info is preserved. This ensures stable exclusion behavior. Additional simplification: Moved statvfs() call before dictionary operations - only add/update pool info when we successfully get capacity data.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/collectors/diskspace.plugin/plugin_diskspace.c">
<violation number="1" location="src/collectors/diskspace.plugin/plugin_diskspace.c:388">
P2: Pool mount presence is never reset, so after a pool unmounts its datasets can still be excluded based on stale `pool_mount_seen`/capacity data. Clear or reset pool entries before rebuilding to ensure unmounted pools are treated as not mounted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Combine pool capacity and dataset exclusion into single cache dictionary. Previously we only cached pool capacities and checked every dataset on every collection cycle. Now we also cache the exclusion decision per dataset, avoiding statvfs() syscalls for excluded datasets. Changes: - Replace zfs_pool_info_dict with unified zfs_cache - Add zfs_dataset_cached_excluded() check before statvfs() - Per-entry timestamps instead of global zfs_last_checked - Add debug logs (to be removed after testing) Performance: For systems with hundreds of ZFS datasets, this reduces statvfs() calls from hundreds per second to just a few (only pools and datasets needing recheck).
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/collectors/diskspace.plugin/plugin_diskspace.c">
<violation number="1" location="src/collectors/diskspace.plugin/plugin_diskspace.c:509">
P2: Pool capacity cache entries are used without checking staleness. If a pool was previously mounted and then unmounted, its cached entry persists and datasets keep being excluded forever, even though the design calls for keeping datasets when the pool is not mounted. Add a `last_checked` freshness check before using `pool_entry->pool_capacity`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
If a ZFS pool was previously mounted and then unmounted, its cache entry persisted indefinitely. Datasets would continue to be excluded based on stale pool capacity, even though the design requires keeping all datasets when the pool is not mounted (so at least one mount reports pool capacity). Fix: Add freshness check before using pool_entry->pool_capacity. If the pool cache entry is older than ZFS_DATASET_RECHECK_SECONDS, treat the pool as unmounted and keep the dataset.
Using exactly ZFS_DATASET_RECHECK_SECONDS could cause race conditions where an entry is considered stale right as it's being refreshed. Using 2x interval means the entry must have missed at least one full refresh cycle before being considered stale, which reliably indicates the pool is no longer mounted.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes: #21632
Problem
Previously, diskspace.plugin excluded all ZFS datasets unconditionally, monitoring only pools. This caused issues:
Solution
tank(1TB)tank/home(no quota, shows 1TB)tank/vm(quota 200GB)Performance:
To avoid calling statvfs() on every dataset every second:
For systems with hundreds of ZFS datasets, this reduces syscalls from hundreds per second to just a few.
Visual Flow
Configuration
New options in
[plugin:proc:diskspace]:Test Plan
Tested on a system with ZFS pool
rpooland datasetsrpool/data,rpool/ROOT(no quotas)zfs datasets heuristic = yeszfs datasets heuristic = no!*excludes nothing)exclude zfs datasets on paths = rpool/dataNot directly tested (no quota datasets available):
Verified via debug logs:
Additional Information
For users: How does this change affect me?
Summary by cubic
Smart ZFS dataset deduplication in diskspace.plugin using a capacity-based heuristic that keeps datasets with quotas, excludes those mirroring their pool, and caches pool capacities and dataset exclusion decisions with a 5-minute recheck to reduce statvfs calls. Exclusion runs on both fast and slow paths, with defensive fixes to prevent a NULL mount_source crash and a dictionary race.
New Features
Refactors
Written for commit f851055. Summary will update on new commits.