Dirty pages deduplication between diff and base layer for memory - #2590
Conversation
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit 0ecb16e. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Code Review
This pull request implements memfd-backed guest memory support for Firecracker, enabling the orchestrator to retrieve dirty pages via pread on a shared file descriptor instead of process_vm_readv. Key changes include a new 4KiB page deduplication routine in the block cache, updates to the UFFD handler to manage the received memfd, and integration into the sandbox lifecycle via a new feature flag. I have no feedback to provide as the implementation is robust and no issues were identified.
c75c871 to
32d7e1d
Compare
9ff4eab to
20e59d2
Compare
Introduce a Cacher interface which abstracts the memory cache implementation, as seen by the diff/upload layer. Currently, it's only the Cache type that implements that. This is preparation for introducing a second cache type which is backed by the memfd used to map guest memory. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
We are changing Firecracker to, optionally, back the guest memory using a memfd object. When enabled, Firecracker passes over the memfd file descriptor over the UFFD UDS, alongside the UFFD file descriptor, using SCM_RIGHTS. Change the UFFD serve logic to also parse the memfd file descriptor. When present, wrap the descriptor in a Memfd object. The object itself provides an interface that lets users access the guest memory from the memfd. UFFD logic exposes the Memfd object over a newly added method of the MemoryBackend interface, called Memfd(). The noop memory backend always returns nil for now, as Firecracker might only use memfd when resuming from a snapshot. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Change the ExportMemory() logic to export the memory via a MemfdCache when Firecracker has sent us a memfd file descriptor. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Add unit tests for memfd and MemfdCache functionality. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Add a feature flag that controls whether the orchestrator will instruct Firecracker to use memfd for backing the guest memory. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
cce6428 to
1bf6df8
Compare
Trim verbose comments and drop trivial tests so the PR is easier to review. The behavioral changes are: - copyFromMemfd uses a fixed 2 MiB chunk (memfdCopyChunkSize) matching the source hugepage size, decoupled from cache.blockSize (which remains the dirty-tracking unit). - NewCacheFromMemfd no longer logs-and-swallows the memfd close error; it returns it. The size==0 fast path drops; the loop is a no-op when ranges sum to 0. - UseMemFdFlag comment matches the mmap-based implementation.
# Conflicts: # packages/orchestrator/pkg/sandbox/snapshot_metrics.go
arkamar
left a comment
There was a problem hiding this comment.
Noticed few nit things, but wasn't able to finish it completely.
jakubno
left a comment
There was a problem hiding this comment.
shouldn't we add IsCached to localDiff returning true?
Reuse the existing build diff cache lookup path and inline a single-use zero mapping check to keep the dedup diff smaller.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9017e0cf15
ℹ️ 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".
Guard dedup source lookups, preserve fast-path read errors in logs, and avoid pooling hugepage buffers for 4 KiB UFFD faults.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a06f5f. Configure here.
Enable memfd and memfile diff dedup in the local integration orchestrator so existing pause/resume tests cover the dedup path.
Enable memfile dedup across integration shards with base, best-effort, and direct I/O variants, and address remaining review feedback.
Skip empty iovecs before pwritev so zero-length batches complete cleanly.
Remove the unused memfd test override now that integration dedup coverage runs through the default memory export path.
Classify zero source pages before comparing against the base so zero bytes always map through Empty instead of uploading dirty zero pages.
Use a single integration dedup mode setting and remove unnecessary CLI flag wiring.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76e9d6b072
ℹ️ 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".
Avoid buffering one iovec per dirty page by flushing bounded pwritev batches while walking the dirty bitmap.
Install UFFD MISSING faults by writing source bytes into the FC-shared memfd and calling UFFDIO_WAKE, instead of UFFDIO_COPY. Read faults arm WP before populate so the kernel retry installs a write-protected PTE. Behind `use-memfd-wake` (sub-flag of `use-memfd`). `resume-build -resume-bench` compares `default` / `memfd-copy` / `memfd-wake` on the same template. Stacked on #2590. --------- Signed-off-by: Babis Chalios <babis.chalios@e2b.dev> Co-authored-by: Babis Chalios <babis.chalios@e2b.dev> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>

Adds 4 KiB memfile diff dedup behind
memfile-diff-dedupto avoid storing pages unchanged from the base when Firecracker reports dirty memory at 2 MiB granularity.Also fixes related page-granular restore correctness for empty mappings and chunk reads, and runs integration pause/resume coverage across base, best-effort, and direct I/O dedup modes.
Stacked on #2522.