Use memfd to track sandbox memory - #2522
Merged
Merged
Conversation
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit acc8b4b. Bugbot is set up for automated code reviews on this repo. Configure here. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
bchalios
force-pushed
the
zero-copy-pause
branch
2 times, most recently
from
April 29, 2026 09:25
6d2b804 to
314abd0
Compare
bchalios
force-pushed
the
zero-copy-pause
branch
5 times, most recently
from
April 29, 2026 15:56
1ab27f8 to
4f0b47b
Compare
❌ 4 Tests Failed:
View the full list of 6 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
bchalios
force-pushed
the
zero-copy-pause
branch
2 times, most recently
from
May 4, 2026 15:13
4ae4ebb to
9198a96
Compare
Contributor
Author
|
Update: I've removed the logic that punches holes in the memfd, progressively after copying data into the diff file. I've ran some experiments and got some signal about this causing increase in CPU utilization and slowing down PAUSE and RESUMEs. I think that we can proceed with adding support for memfd and revisiting after the deduplication work. |
bchalios
marked this pull request as ready for review
May 4, 2026 15:19
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
bchalios
force-pushed
the
zero-copy-pause
branch
2 times, most recently
from
May 19, 2026 10:12
33be29e to
8ecaf0f
Compare
Contributor
Author
Two things change here:
The amount of data the move around is the same. The only thing that changes is the source. We are just reducing the latency to our response to the In subsequent PR that we will also deduplicate the pages we're actually saving, we will be adding metrics about how much we saved depending on the code path. WDYT @jakubno? |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8ecaf0f9039830851e874ff2058d02a4c8395878. Configure here.
kalyazin
reviewed
May 19, 2026
bchalios
force-pushed
the
zero-copy-pause
branch
2 times, most recently
from
May 19, 2026 12:41
26aeaae to
2fa8bc3
Compare
Member
Thanks for clarification! Sounds good |
![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: b77746e043
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return fmt.Errorf("memfd slice [%d,%d): %w", r.Start, r.Start+r.Size, err) | ||
| } | ||
|
|
||
| copy((*cache.mmap)[cacheOff:cacheOff+r.Size], src) |
There was a problem hiding this comment.
When dirty contains one large contiguous range, BitsetRanges emits a single range and this copy can run for many GB without observing ctx. In async mode Close cancels the copy context and then waits for done, so a canceled upload or shutdown can still block until the entire contiguous memfd range is copied; copying in bounded chunks and checking ctx between chunks keeps cancellation effective.
Useful? React with 👍 / 👎.
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 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>
The gRPC handler already injects sandbox/team/template contexts via ctx, so team and template targeting for UseMemFdFlag already worked. Add a sandbox-type attribute (sandbox vs build) and pass the explicit sandboxLDContext to BoolFlag so flags can roll out to production sandboxes separately from template-builds. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
FC < 1.14 rejects the use_memfd field on snapshot load (deny_unknown_fields on MemoryBackend), so combining FCSupportsMemfd(version) with the flag avoids hard-failing resumes when the flag is flipped on across a heterogeneous fleet. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Introduce the MemfdCache wrapper (embedding *Cache) plus the NewCacheFromMemfdAsync constructor: copy runs on a goroutine so gRPC Pause can return as soon as the snapshot file and diff metadata are written. The MemfdBackgroundCopyFlag gates the dispatch in fc.ExportMemory; flag-off keeps the existing sync NewCacheFromMemfd path untouched. Introduce a DiffSource interface which abstracts the functionality of a cache, so ExportMemory() now returns a DiffSource on success. ReadAt/Slice Wait for background copies to finish before returning results in the case of the asynchronous memfd cache. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
TestRetryableClient_ActualRetryBehavior asserted the first retry delay was <200ms, but CI runners can add hundreds of ms of scheduling/network overhead on top of the jittered backoff. Bump bounds to 2s to keep the test stable while still catching order-of-magnitude regressions. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Add spans to differentiate between exporting memory from Firecracker process vs memfd. Also, differentiate between synchronous and synchronous memfd memory export. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
kalyazin
approved these changes
May 19, 2026
jakubno
approved these changes
May 19, 2026
ValentaTomas
added a commit
that referenced
this pull request
May 24, 2026
Adds 4 KiB memfile diff dedup behind `memfile-diff-dedup` to 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. --------- Signed-off-by: Babis Chalios <babis.chalios@e2b.dev> Co-authored-by: ValentaTomas <valenta.and.thomas@gmail.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
kalyazin
added a commit
that referenced
this pull request
May 28, 2026
Prefault() and Close() could race: Close() freed the uffd fd number while a prefetcher goroutine was about to acquire settleRequests.RLock and call UFFDIO_COPY. If the OS recycled the fd to a non-uffd file between the close and the ioctl, the syscall returned ENOTTY (seen in production from 2026-04-26 onward, worsening after PR #2522 added an extra fd close per session). Fix by making Close() acquire settleRequests.Lock() before closing the fd and setting a `closed` flag. Prefault() checks the flag immediately after acquiring RLock; if set, it returns nil without touching the fd. This ensures the fd is only closed after all in-flight UFFDIO_COPY callers have released the read-lock. Also add faultPhaseBeforePrefaultRLock test hook so a regression test can deterministically park Prefault before the RLock, let Close() run, and verify the closed-check path. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
kalyazin
added a commit
that referenced
this pull request
May 28, 2026
## Fix Prefault/Close race causing ENOTTY/EBADF ### Motivation Since **2026-04-26** the orchestrator has been logging `UFFD serve uffdio copy error: inappropriate ioctl for device` (ENOTTY) and `bad file descriptor` (EBADF) from the UFFD prefetch path. The error rate increased sharply on **2026-05-27** after #2522 landed. The root cause is a race between the prefetcher and sandbox teardown: ``` Prefault goroutine Close() (teardown) ──────────────────────────────── ────────────────────────────── (about to acquire RLock) syscall.Close(uffd fd) ← fd freed ← OS recycles fd number to an unrelated file acquires RLock calls UFFDIO_COPY(recycled fd) → ENOTTY (or EBADF if not yet recycled) ``` `Close()` held no lock when it freed the uffd fd. The prefetcher runs on a non-cancellable `execCtx` and has no way to observe that `Close()` has run. If the OS recycled the fd number before the prefetcher's `UFFDIO_COPY` ioctl fired, the kernel returned ENOTTY because the fd now referred to a non-uffd file. The error was benign at the sandbox level (the sandbox was already being torn down) but noisy and misleading in logs. The bug was latent from when the prefetcher was introduced in January 2026 (#1705) but only started firing after the ubuntu24 template rebuild in April populated `Prefetch.Memory` data, causing the prefetcher to actually run. ### Fix `Close()` now holds `settleRequests.Lock()` for the **entire** close sequence and is idempotent: ```go func (u *Userfaultfd) Close() error { u.settleRequests.Lock() defer u.settleRequests.Unlock() if u.closed { return nil } u.closed = true syscall.Close(u.wakeupPipe[0]) syscall.Close(u.wakeupPipe[1]) return u.fd.close() } ``` `Prefault()` checks the flag immediately after acquiring `settleRequests.RLock()`: ```go u.settleRequests.RLock() defer u.settleRequests.RUnlock() if u.closed { return nil } ``` This gives three safety guarantees: 1. Any `Prefault` caller already holding `RLock` when `Close()` runs will complete its `UFFDIO_COPY` against the still-valid fd before `Close()` can acquire the write lock. 2. Any `Prefault` call that starts after `Close()` returns will see `closed == true` and return nil without touching the fd. 3. `Close()` is idempotent: a second call returns immediately without touching already-freed fds, preventing accidental double-close of the wakeup pipe fds (and any unrelated fd the OS may have recycled those numbers to). `settleRequests` already existed for exactly this kind of serialisation (guarding the lookup→install→state-update sequence against REMOVE batches), so no new lock is introduced. In production `Serve()` drains all workers via `u.wg.Wait()` before returning, so by the time the deferred `Close()` fires the lock is always uncontended. ### Test `TestPrefaultConcurrentWithClose` deterministically reproduces the race using a `faultPhaseBeforePrefaultRLock` test hook. The goroutine is parked before it acquires `RLock`, `Close()` is called to completion, then the goroutine is released. Without the fix the test fails with `failed to fault page: failed uffdio copy: bad file descriptor`; with the fix it returns nil. --------- Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
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
5 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.
What
In Unix OSs
memfdis an anonymous file that can be used to back memory. Firecracker uses this construct when it needs to share memory with external processes (currently, when using vhost-user devices).Currently, when we take a snapshot of the sandbox (for example, during
PAUSEoperations) we need to copy its memory usingprocess_vm_readv.memfdallows us to do this in a more idiomatic way.Why
memfdallows us to have a direct view of the sandbox memory from the orchestrator without having to copy memory across processes. Moreover, if the orchestrator holds a reference tomemfd, we can post process the sandbox memory after the Firecracker process is killed. This opens up possibilities for various latency and memory utilization optimizations.What we do in this PR is that we change the cache logic to use memfd to copy Firecracker memory into the diff file if the memfd is present.