Skip to content

Give Firecracker forks their own mem-file instead of deferring the copy#298

Merged
sjmiller609 merged 9 commits into
mainfrom
hypeship/fc-fork-local-memfile
Jul 8, 2026
Merged

Give Firecracker forks their own mem-file instead of deferring the copy#298
sjmiller609 merged 9 commits into
mainfrom
hypeship/fc-fork-local-memfile

Conversation

@sjmiller609

@sjmiller609 sjmiller609 commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Firecracker standby forks (fork-from-standby-instance and fork-from-standby-snapshot) previously skipped copying the mem-file and stored a path to the source's memory file (FirecrackerDeferredSnapshotMemoryPath), serving UFFD faults from it while running and copying it only at the fork's next standby. Until that first standby completed, every fork silently depended on a file it didn't own:

  • DeleteSnapshot / DeleteInstance / StopInstance on the source made standby-parked forks permanently unrestorable and broke running forks' next standby. Nothing tracked or guarded these dependencies.
  • Worse, a source instance's restore + re-standby diff-writes into the same inode forks were reading (snapshot-base is promoted by rename, then Firecracker writes dirty pages in place) — silent memory corruption for dependents.

This PR removes the deferral. Standby forks now hardlink the source's snapshot mem-file into the fork's guest dir (fallback to reflink/sparse copy with a warning if linking fails), and the standby path enforces one invariant: never diff-write into a mem-file with nlink > 1 — it replaces a shared mem-file with a private reflink/sparse copy before Firecracker writes the diff.

Why hardlink (not a per-fork copy or reflink)

Benchmarked on a 25-fork burst: per-fork reflinked copies regressed fork p50 ~4x. Reflink shares disk extents but the kernel page cache is per-inode, so each fork's pager cache misses re-read the same pages from cold encrypted disk instead of hitting pages warmed by sibling forks (5.7s aggregate backing reads vs ~0s). Hardlinks keep all forks on one inode:

  • Fanout is zero-I/O — no mem-file read or write at fork time, on every filesystem.
  • Runtime matches the old shared-file profile — pager cache (keyed by the inherited FirecrackerSnapshotCacheKey) plus kernel page cache are both shared across forks.
  • Independence via inode refcount — deleting the source snapshot/instance only unlinks a name; forks keep the inode. DeleteSnapshot is safe the moment the fork API returns.
  • Correctness via the unshare guard — a fork's first standby (and a source re-entering standby while forks share its base) copies the mem-file to a private inode before the in-place diff merge. The one-time copy cost lands off the fanout burst, and is a ~free reflink on FICLONE-capable filesystems.

Firecracker mmaps the mem-file MAP_PRIVATE, so guest writes never reach the file; the standby diff merge is the only file writer and is covered by the guard.

Removed (now dead)

  • FirecrackerDeferredSnapshotMemoryPath metadata field and all wiring in fork/snapshot-fork paths
  • materializeDeferredSnapshotMemory + base/latest alternate-path resolution (both copies)
  • repointForkDeferredSnapshotMemoryToSourceBase after running-source forks
  • lockFirecrackerSnapshotSource / snapshotSourceLocks
  • hypervisor.SnapshotOptions (existed only to carry the deferred path; Snapshot() drops the param across all hypervisors)

Behavior changes / caveats

  • Upgrade: standby forks created by an older build that still carry a deferred path will not restore after this change (the field is gone and the fork has no local mem-file). Accepted intentionally; drain or restore+standby existing deferred forks before rolling out if any exist.
  • A source instance that re-enters standby while forks still share its retained base pays the unshare copy (reflink on xfs, sparse copy otherwise). Central-snapshot fanout never hits this — snapshot store files are never diff-written.
  • Bumps the UFFD pager version (CI gate on lib/instances/firecracker_uffd.go changes).

Tests

  • Unit: forks assert the mem-file shares the source's inode (instance-fork and snapshot-fork); compressed-source forks still fall back to a real copy; ensureExclusiveSnapshotMemoryOwnership unshares hardlinked mem-files (source bytes untouched) and no-ops on private/missing ones. Passing locally with lib/forkvm, lib/hypervisor/..., lib/guestmemory.
  • Integration: the fork-isolation test now asserts hardlink-at-fork + unshare-at-standby + source bytes/inode unchanged through the fork's full lifecycle; TestFCUFFDOneShotLifecycle (currently skip-gated, KERNEL-1354) additionally asserts DeleteSnapshot succeeds while a fork is running from it. KVM/UFFD integration not validated locally (sandbox has no KVM images/reflink fs) — needs the CI run.
  • TestForkCloudHypervisorFromRunningNetwork / TestStandbyAndRestore fail in my sandbox on image-pull timeouts — both reproduce identically without these changes (environmental).
  • Perf validation: re-run the 25-burst cold+warm fork bench; expected fork p50 back to main's baseline with hit-rate ~100% and ~0 backing-read time.

🤖 Generated with Claude Code


Note

High Risk
Changes core Firecracker fork/standby snapshot memory lifecycle and removes deferred-memory metadata (older standby forks may not restore after upgrade); correctness depends on unshare running under the instance write lock before any diff write.

Overview
Replaces deferred snapshot memory (forks pointing at the source mem-file until standby) with hardlinked mem-files on Firecracker standby fork fanout, plus a guard so diff snapshots never write into a shared inode.

Fork path: Standby instance/snapshot forks skip copying snapshots/snapshot-latest/memory, hardlink it from the source (reflink/sparse copy + metric on link failure), and drop FirecrackerDeferredSnapshotMemoryPath, snapshot-source locks, and repointForkDeferredSnapshotMemoryToSourceBase. UFFD one-shot restore now faults against the fork’s local hardlinked file and inherited cache key.

Standby path: Before creating a diff snapshot when reusing a retained base, ensureExclusiveSnapshotMemoryOwnership copies the mem-file to a private inode when nlink > 1 (via memory.unshare.tmp), so source or sibling forks are not mutated in place. Guest directory copy skips *.unshare.tmp files.

API cleanup: hypervisor.SnapshotOptions and Firecracker materializeDeferredSnapshotMemory are removed; Snapshot(ctx, destPath) is the same across hypervisors. UFFD pager version bumps to 0.1.6.

Docs/tests: forkvm README documents hardlink + unshare semantics; integration/unit tests assert inode sharing at fork, unshare at standby, and safe source deletion while forks run.

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

…the copy

Standby forks previously skipped the mem-file copy and stored a path to the
source's memory file, serving UFFD faults from it and copying it only at the
fork's next standby. That left forks silently dependent on the source snapshot
or instance: deleting or stopping the source stranded standby forks, and the
source's next diff snapshot mutated the shared file in place under running
forks.

Fork copies now always include the mem-file via the existing reflink-first
directory copy, so a fork owns its memory from creation and the source can be
deleted immediately. UFFD one-shot restore is unchanged except that the pager
serves from the fork-local file; the shared page cache still works because
forks inherit the source's cache key and the cloned files are byte-identical.

Removes the deferred-path machinery: FirecrackerDeferredSnapshotMemoryPath,
materialize-on-standby, base/latest alternate path resolution, snapshot source
locks, the repoint step after running-source forks, and SnapshotOptions.
Benchmarks showed per-fork reflinked mem-files regress concurrent fanout:
reflink shares disk extents but not the kernel page cache, so each fork's
pager misses re-read the same pages from cold disk instead of hitting the
cache warmed by sibling forks.

Standby forks now hardlink the source's snapshot mem-file: fanout does no
memory I/O and all forks of a snapshot fault against one inode, restoring
the shared read path. Independence is preserved by inode refcount (deleting
the source only unlinks a name) plus one invariant: the standby path never
diff-writes into a mem-file with nlink > 1 — it replaces it with a private
reflink/sparse copy first. That guard also covers a source instance
re-entering standby while forks still share its retained base, and moves the
one-time copy cost to each fork's first standby, off the fanout burst.

Falls back from hardlink to copy with a warning when linking fails.
@sjmiller609 sjmiller609 marked this pull request as ready for review July 3, 2026 14:12

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a43a9ba. Configure here.

Comment thread lib/instances/snapshot_alias_lock.go
@sjmiller609 sjmiller609 requested a review from hiroTamada July 6, 2026 19:04

@hiroTamada hiroTamada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

approved — reviewed the whole diff end to end (23 files). solid change: it replaces the fragile deferred-mem-file scheme (a fork stored a path into the source's file and copied later) with hardlink-at-fork + copy-on-write-at-standby, keyed on the inode link count. i verified the guard sits before the in-place diff-write, that the per-instance RWMutex serializes fork-hardlink (read lock) against source-standby (write lock) — so the removed per-source snapshotSourceLocks is redundant, not a lost guard — that every diff-write path routes through the guard, and that the symbol removal is complete (no dangling refs). net simplification (~‑98 lines).

no correctness bugs found. a couple of things worth confirming plus some nits, none blocking.

questions

  • lib/instances/fork.go:260 / lib/instances/snapshot.go:420shareMemFile is broader than the old shouldDeferFirecrackerSnapshotMemoryCopy it replaces: it drops both the UFFD-configured gate and the targetState != Stopped gate. so non-UFFD standby-source forks and stopped-target forks now hardlink instead of full-copy. looks intentional and safe (hardlink + guard covers them), just want to confirm it's deliberate. related: TestForkInstanceFromStandbyDoesNotArmOneShotUFFDForStoppedTarget only asserts FileExists, so the new stopped-target sharing isn't pinned by a test either way.
  • lib/instances/firecracker_memfile.go:28 — cross-filesystem forks fail os.Link with EXDEV and silently fall back to a full copy, losing the whole zero-copy / shared-page-cache win; the only signal is a per-fork warning log. are fork and source guest dirs guaranteed on the same filesystem? if a data-dir override can split them, worth a metric (or an explicit assertion) so the regression is detectable instead of silent.

nits

  • lib/instances/firecracker_memfile.go:44ensureExclusiveSnapshotMemoryOwnership is only safe because the caller (standby) holds the source's per-instance write lock; the stat → copy → rename isn't internally synchronized. worth a line in the doc comment so a future caller doesn't reintroduce the TOCTOU.
  • lib/instances/firecracker_memfile.go:59 — a hard crash between the copy and the rename leaves a full-mem-file-sized memory.unshare.tmp in snapshot-latest; the fork copy-walk (which only skips snapshots/snapshot-latest/memory) would then propagate it into new forks. consider skipping *.unshare.tmp in the walk, or sweeping stale ones on standby entry.
  • lib/instances/snapshot_alias_lock.go:136 — the os.Stat(srcMem) check disables sharing on any error, not just IsNotExist, so a real I/O or permission error is treated the same as "compressed source, no raw file." consider gating on os.IsNotExist(err) and returning other errors.
  • lib/instances/snapshot_alias_lock.go:50,73,94,115 — trace attribute renamed deferred_snapshot_memoryshare_mem_file; heads-up for any fork-latency dashboards/alerts keyed on the old name.

test coverage / rollout

  • the two integration tests that actually exercise the fix are gated: TestFirecrackerForkIsolation self-skips without /dev/kvm + /dev/userfaultfd (so a green test job doesn't prove it ran), and TestFCUFFDOneShotLifecycle — which carries the new "DeleteSnapshot while a fork is running" assertion — is unconditionally skipped (KERNEL-1354). worth confirming the fork-isolation test runs on a KVM-capable runner (and ideally the 25-fork bench) before rollout, given this is a silent-corruption-class change.
  • rollout: drain/restore any standby forks created by older builds first — they carry a deferred path and no local mem-file, so they won't restore after this ships (already called out in the description).

sjmiller609 and others added 6 commits July 8, 2026 10:08
- Count hardlink-to-copy fallbacks in a metric (labeled by errno) alongside
  the existing warning, so a host silently losing zero-copy fanout is
  visible in dashboards, not just per-fork logs.
- Propagate non-IsNotExist stat errors on the source mem-file instead of
  silently disabling sharing.
- Sweep stale memory.unshare.tmp on standby entry and skip *.unshare.tmp in
  the fork copy walk, so a crash between the unshare copy and rename cannot
  leak a mem-file-sized artifact into future forks.
- Document that ensureExclusiveSnapshotMemoryOwnership relies on the
  caller's instance write lock for the stat/copy/rename sequence.
@sjmiller609 sjmiller609 merged commit 13baa91 into main Jul 8, 2026
11 checks passed
@sjmiller609 sjmiller609 deleted the hypeship/fc-fork-local-memfile branch July 8, 2026 20:58
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