Skip to content

Count shared snapshot extents separately#244

Merged
sjmiller609 merged 2 commits into
mainfrom
hypeship/reflink-snapshot-disk
May 27, 2026
Merged

Count shared snapshot extents separately#244
sjmiller609 merged 2 commits into
mainfrom
hypeship/reflink-snapshot-disk

Conversation

@sjmiller609
Copy link
Copy Markdown
Collaborator

@sjmiller609 sjmiller609 commented May 27, 2026

Summary

  • make snapshot disk utilization use FIEMAP on Linux so shared CoW extents are not charged to snapshot_uncompressed or snapshot_compressed
  • add a snapshot_shared disk utilization component for shared extents counted once by physical range
  • keep non-Linux and FIEMAP-unavailable paths on the existing st_blocks accounting fallback

Tests

  • go test ./lib/diskutilization ./lib/resources

Note

Medium Risk
Changes how snapshot disk usage is reported and relies on FIEMAP/ioctl on Linux, but non-Linux and fallback paths preserve prior st_blocks behavior.

Overview
Snapshot disk utilization now splits reflink / shared copy-on-write usage from per-snapshot private bytes. A new snapshot_shared component reports shared physical extents once across all guests; snapshot_uncompressed and snapshot_compressed only get the private portion.

On Linux, regular snapshot files use FIEMAP to classify extents; a cross-guest physical-range tracker deduplicates shared blocks. If FIEMAP is unavailable or no shared extents are seen, accounting falls back to existing st_blocks behavior (no double-count fix). Non-Linux builds keep the prior all-in-one block counting via a stub implementation.

Tests cover extent deduplication and an integration path with FICLONE when the filesystem supports it.

Reviewed by Cursor Bugbot for commit 2b088a2. Bugbot is set up for automated code reviews on this repo. Configure here.

@sjmiller609 sjmiller609 marked this pull request as ready for review May 27, 2026 15:48
@firetiger-agent
Copy link
Copy Markdown

Firetiger deploy monitoring skipped

This PR didn't match the auto-monitor filter configured on your GitHub connection:

Any PR that changes the kernel API. Monitor changes to API endpoints (packages/api/cmd/api/) and Temporal workflows (packages/api/lib/temporal) in the kernel repo

Reason: PR modifies disk utilization accounting logic in lib/diskutilization and lib/resources, not the kernel API endpoints (packages/api/cmd/api/) or Temporal workflows (packages/api/lib/temporal) specified in the filter.

To monitor this PR anyway, reply with @firetiger monitor this.

@sjmiller609 sjmiller609 requested a review from hiroTamada May 27, 2026 17:28
Copy link
Copy Markdown
Contributor

@hiroTamada hiroTamada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed — overall looks solid, mostly questions and nits:

Questions

  • lib/diskutilization/diskutilization_reflink_linux.go:77-80 — when FIEMAP errors or sawShared is false, the whole file falls back to stat.Blocks. For a reflink-free file with holes/preallocated-unwritten extents, FIEMAP's private-byte sum could disagree with stat.Blocks; the fallback hides that asymmetry. Intentional?
  • lib/diskutilization/diskutilization_reflink_linux.go:111-121 — counts extent.Length (logical) for shared bytes. Fine for ext4/xfs but would over-report on btrfs compressed extents. Worth a comment if compression at the fs layer isn't expected for snapshot files?

Nits

  • lib/diskutilization/diskutilization_reflink_linux.go:15 — consider using unix.FS_IOC_FIEMAP rather than hard-coding 0xC020660B.
  • lib/diskutilization/diskutilization_reflink_linux_test.go:56-64TestSharedExtentTrackerAdd covers exact-dup, partial overlaps, and disjoint. Missing zero-length, full-containment, and adjacent-merge cases — would lock down compact() behavior if you want it.
  • lib/diskutilization/diskutilization_reflink_linux.go:137-174add() + compact() is O(n log n) per call, O(n²log n) over a Collect pass. Not a concern at current guest counts; flagging in case it gets reused on a hotter path.

@sjmiller609
Copy link
Copy Markdown
Collaborator Author

the fallback is intentional: for files without FIEMAP shared extents, we keep the existing stat.Blocks behavior so sparse/preallocated edge cases don’t change as part of this patch. this only switches accounting when FIEMAP reports shared CoW extents.

we also deployed this on dev-yul-hypeman-0 and checked SigNoz: snapshot_shared appeared after deploy, snapshot_uncompressed dropped from ~1.146 TB to ~397.5 GB, and scrape health looked fine. scrape_duration_seconds did not regress around the deploy (pre avg ~0.600s/max 0.812s, post avg ~0.573s/max 0.758s), samples stayed stable, and there were no disk-utilization/FIEMAP/resource-monitoring errors.

@sjmiller609 sjmiller609 merged commit 34e1032 into main May 27, 2026
13 of 14 checks passed
@sjmiller609 sjmiller609 deleted the hypeship/reflink-snapshot-disk branch May 27, 2026 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants