feat(nbd): zero/discard detection to shrink snapshot diffs#2546
Merged
ValentaTomas merged 3 commits intomainfrom May 5, 2026
Merged
feat(nbd): zero/discard detection to shrink snapshot diffs#2546ValentaTomas merged 3 commits intomainfrom
ValentaTomas merged 3 commits intomainfrom
Conversation
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit e55fd51. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This was referenced May 3, 2026
ValentaTomas
added a commit
that referenced
this pull request
May 3, 2026
The test round-tripped binary.BigEndian.Put/GetUint16 on bytes the test itself wrote, asserting nothing the struct definition + 4-line parser don't already make obvious by inspection. Real flag/opcode dispatch behavior is exercised in #2546's NBD harness tests.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6c317b212
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ValentaTomas
added a commit
that referenced
this pull request
May 4, 2026
…#2547) `IsEmptyBlock` was the per-block hot path of `DiffMetadataBuilder.Process` — one call per block during the rootfs scan in `DirectProvider.exportToDiff`. It paid a full SIMD `memcmp` (4 KiB or, on hugepages, 2 MiB) before it could short-circuit, and required two static zero buffers (`EmptyBlock`, `EmptyHugePage`) to compare against. Replace it with a small `IsZero` helper that uses QEMU's `buffer_is_zero` trick: sample three bytes (first, last, middle) so most non-zero blocks reject from a single cache line. The fallback uses `bytes.Equal` on `b` shifted by one (`b[1:] == b[:n-1]`), which dispatches to the runtime's SIMD `memequal` on amd64/arm64 — same speed as the old comparison against a static buffer when the short-circuit doesn't fire. `DiffMetadataBuilder.Process` calls `IsZero` directly (the size validation `IsEmptyBlock` did was unreachable in practice). `EmptyBlock` is removed; `EmptyHugePage` stays — `build.go` uses it as the literal zero buffer for `uuid.Nil` reads in `Build.ReadAt`. Pre-factored from #2546 so it can land on its own.
ValentaTomas
force-pushed
the
feat/block-state-tracker
branch
4 times, most recently
from
37be905 to
dcb7de4
Compare
ValentaTomas
force-pushed
the
feat/nbd-zero-discard-detection
branch
from
be64f24 to
64f075d
Compare
❌ 10 Tests Failed:
View the full list of 17 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
ValentaTomas
added a commit
that referenced
this pull request
May 4, 2026
Adds `block.Tracker`, a non-generic state tracker over a fixed `block.State` enum with three universal values that cover both UFFD memory handling and NBD overlay tracking: - `NotPresent` (default) — fall through to the previous layer - `Dirty` — materialized in this layer (UFFD faulted page, NBD overlay-owned block) - `Zero` — known-zero without consulting the previous layer (UFFD page removed via `DONTNEED`/balloon, NBD explicit zero/discard) Backed by two roaring bitmaps (`dirty`, `zero`); `NotPresent` indices are implicit. Exposes `SetRange`, `Get`, and `Export` (returns clones). First consumer is #2520 (UFFD per-page state); also used by #2546 for NBD zero-page tracking.
ValentaTomas
added a commit
that referenced
this pull request
May 5, 2026
When a read range was tracked entirely as Zero, ReadAt returned len(b) unconditionally, while the non-zero path returns copy(b, slice) where slice is capped at c.size. Mirror that cap in the zero path so reads that extend past EOF report the correct number of bytes (and 0 when fully past EOF). Reported by Cursor Bugbot on #2546.
ValentaTomas
force-pushed
the
feat/nbd-zero-discard-detection
branch
from
64f075d to
645e279
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ValentaTomas
force-pushed
the
feat/nbd-zero-discard-detection
branch
2 times, most recently
from
8143945 to
43c8f20
Compare
ValentaTomas
force-pushed
the
feat/nbd-zero-discard-detection
branch
from
43c8f20 to
fa24b44
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Zero-detection loop panics on non-block-aligned writes
- Added blockEnd calculation to constrain slice access to actual buffer length, preventing index-out-of-range panic when buffer is not block-aligned.
Or push these changes by commenting:
@cursor push 3056dd5d91
Preview (3056dd5d91)
diff --git a/packages/orchestrator/pkg/sandbox/block/cache.go b/packages/orchestrator/pkg/sandbox/block/cache.go
--- a/packages/orchestrator/pkg/sandbox/block/cache.go
+++ b/packages/orchestrator/pkg/sandbox/block/cache.go
@@ -348,7 +348,8 @@
// detect-zeroes=unmap: route aligned all-zero blocks to Empty.
for i := off; i < end; i += c.blockSize {
idx := uint32(header.BlockIdx(i, c.blockSize))
- if header.IsZero(b[i-off : i-off+c.blockSize]) {
+ blockEnd := min(i-off+c.blockSize, int64(len(b)))
+ if header.IsZero(b[i-off : blockEnd]) {
c.punchHole(i, c.blockSize)
c.tracker.SetRange(idx, idx+1, Zero)
} else {You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa24b44e96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ValentaTomas
force-pushed
the
feat/nbd-zero-discard-detection
branch
from
fa24b44 to
5260bb6
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: WriteZeroesAt uses floor division for end block index
- Changed WriteZeroesAt to use BlockCeilIdx instead of BlockIdx for the end boundary to maintain consistency with setIsCached and isCached, preventing partial blocks from being zeroed without being tracked.
Or push these changes by commenting:
@cursor push 31d6f04e99
Preview (31d6f04e99)
diff --git a/packages/orchestrator/pkg/sandbox/block/cache.go b/packages/orchestrator/pkg/sandbox/block/cache.go
--- a/packages/orchestrator/pkg/sandbox/block/cache.go
+++ b/packages/orchestrator/pkg/sandbox/block/cache.go
@@ -382,7 +382,7 @@
c.punchHole(off, end-off)
c.tracker.SetRange(
uint32(header.BlockIdx(off, c.blockSize)),
- uint32(header.BlockIdx(end, c.blockSize)),
+ uint32(header.BlockCeilIdx(end, c.blockSize)),
Zero,
)You can send follow-ups to the cloud agent here.
ValentaTomas
force-pushed
the
feat/nbd-zero-discard-detection
branch
3 times, most recently
from
dc01295 to
db3c85f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Flush uses BlockIdx instead of BlockCeilIdx for end
- Changed flush closure to use BlockCeilIdx for endIdx calculation, making it consistent with setIsCached, isCached, and WriteZeroesAt to properly handle non-block-aligned end boundaries.
Or push these changes by commenting:
@cursor push bd96e07c2a
Preview (bd96e07c2a)
diff --git a/packages/orchestrator/pkg/sandbox/block/cache.go b/packages/orchestrator/pkg/sandbox/block/cache.go
--- a/packages/orchestrator/pkg/sandbox/block/cache.go
+++ b/packages/orchestrator/pkg/sandbox/block/cache.go
@@ -350,7 +350,7 @@
// same-state run as a single bulk copy / punchHole / SetRange.
flush := func(runStart, runEnd int64, runZero bool) {
startIdx := uint32(header.BlockIdx(runStart, c.blockSize))
- endIdx := uint32(header.BlockIdx(runEnd, c.blockSize))
+ endIdx := uint32(header.BlockCeilIdx(runEnd, c.blockSize))
if runZero {
c.punchHole(runStart, runEnd-runStart)
c.tracker.SetRange(startIdx, endIdx, Zero)You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 11438b0. Configure here.
NBD_CMD_TRIM, NBD_CMD_WRITE_ZEROES, and all-zero plain WRITE now collapse to Empty in the diff (mapped to uuid.Nil, already understood by the read path) and the cache file is hole-punched via MADV_REMOVE so freed pages return to tmpfs. Both NBD flags are advertised; rootfs is mounted with discard. Cache adds an `empty` bitset alongside the existing `dirty` (Empty is a strict subset of dirty); on export, dirty.AndNot(empty) splits payload from Empty. Diff format is unchanged (the Empty bitmap was always there, just never populated), so old snapshots and old readers stay compatible.
ValentaTomas
force-pushed
the
feat/nbd-zero-discard-detection
branch
from
11438b0 to
88beca9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88beca939c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Fix WriteZeroesAt docstring (no head/tail handling exists). - Collapse the multi-line WriteAtWithoutLock comment that narrated what the loop does into one line about intent. - Tighten cmdWriteZeroes one-liner.
Member
Author
|
@cla-bot check |
1 similar comment
Contributor
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
1 similar comment
|
The cla-bot has been summoned, and re-checked this pull request! |
arkamar
approved these changes
May 5, 2026
…-detection # Conflicts: # packages/orchestrator/pkg/sandbox/block/cache_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drops snapshot-diff payload for blocks the guest zeroes or trims.
NBD_CMD_TRIM,NBD_CMD_WRITE_ZEROES, and all-zero plainWRITEnow collapse toEmptyin the diff (mapped touuid.Nil, already understood by the read path) and the cache file is hole-punched viaMADV_REMOVEso freed pages return. Both NBD flags are advertised; rootfs is mounted withdiscard.Diff format unchanged (the
Emptybitmap was always there, just never populated), so old snapshots and old readers stay compatible.