Skip to content

Avoid synchronous fdatasync in snapshotcache writeback and eviction - #3477

Closed
Chenyi Wang (chw120) wants to merge 1 commit into
agent-substrate:mainfrom
chw120:fix-microvm-wake-latency
Closed

Avoid synchronous fdatasync in snapshotcache writeback and eviction#3477
Chenyi Wang (chw120) wants to merge 1 commit into
agent-substrate:mainfrom
chw120:fix-microvm-wake-latency

Conversation

@chw120

@chw120 Chenyi Wang (chw120) commented Oct 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Follows up on #3353.

Why: snapshotcache.StartWriteback and SyncAndEvict run fdatasync in background goroutines while holding all snapshot files open. Under #3485, both the VM memory snapshot and each fast-path tar call StartWriteback, so those fdatasync calls contend on the shared ext4 journal (#1954) and keep unlinked snapshot files ((deleted)) pinned on disk until all syncs finish. Also, writeSnapshot started writeback before mergeOnDemandDelta replaced memory-ranges, so it watched the unlinked delta file instead of the merged snapshot.

  1. StartWriteback now pins files by FileInfo and opens/closes one file at a time while polling cachestat and using data-only SYNC_FILE_RANGE_WRITE (skipping unlinked/replaced inodes without holding open fds across sleeps or sync waits), and post-restore eviction uses EvictWithRetries.
  2. snapshotVMState starts writeback after mergeOnDemandDelta.

Local ext4 benchmark (suspend with 256MiB memory-ranges + 96MiB rootfs-upper.tar, followed by immediate same-node wake):

Metric main #3485 only #3485 + #3477
tar_create 785ms 115ms 114ms
upper_join 614ms 496ms 270ms
Suspend + wake 1400ms 630ms 393ms
With bg writeback 1487ms 1077ms 639ms

AI use: AI-assisted, I reviewed and tested it.

  • Tests pass

@BenTheElder

Benjamin Elder (BenTheElder) commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

I'm not sure completely dropping the write-behind is safe, if you're close to full this may OOM? It would give even better performance but I think that may be over-shooting.

@chw120
Chenyi Wang (chw120) force-pushed the fix-microvm-wake-latency branch 2 times, most recently from c2687c6 to 4542389 Compare October 30, 2026 04:59
@chw120

Chenyi Wang (chw120) commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

I'm not sure completely dropping the write-behind is safe, if you're close to full this may OOM? It would give even better performance but I think that may be over-shooting.

Thanks for the review! Agreed on the OOM risk. I updated this PR to keep the 8 MiB SyncFileRange write-out in tarutil.writeBehind and only drop FADV_DONTNEED.

Since #3485 already covers tarutil and memborrow, the two remaining fixes here are:

  1. Drop fdatasync in snapshotcache: Replace blocking Fdatasync with cachestat polling + data-only SYNC_FILE_RANGE_WRITE to avoid shared ext4 journal contention and skip flushing snapshot files that a fast local restore has already unlinked.
  2. Track the merged memory-ranges in checkpoint.go: Start StartWriteback after mergeOnDemandDelta replaces memory-ranges, so writeback tracks the final merged image rather than the unlinked pre-merge delta and doesn't return borrowed early.

Happy to wait for #3485 to land and rebase this PR onto just those two fixes! Benjamin Elder (@BenTheElder)

@chw120 Chenyi Wang (chw120) changed the title Avoid synchronous snapshot flushes and throttling on unlimited worker pods Avoid synchronous fdatasync in snapshotcache writeback and eviction Oct 30, 2026
@chw120

Chenyi Wang (chw120) commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Updated this PR to drop the parts covered by #3485 and keep only the remaining non-overlapping changes (snapshotcache writeback/eviction without fdatasync, and starting VM snapshot writeback after mergeOnDemandDelta).

@chw120 Chenyi Wang (chw120) mentioned this pull request Oct 30, 2026
Merged
2 tasks done
@chw120
Avoid synchronous fdatasync in snapshotcache writeback and eviction
d0b7582 
Replace blocking unix.Fdatasync calls in snapshotcache.StartWriteback and post-restore eviction with cachestat polling and data-only SYNC_FILE_RANGE_WRITE, open and close one file at a time in StartWriteback so unlinked snapshot files are not held open across sleeps or sync waits, and start VM snapshot writeback after mergeOnDemandDelta so it tracks the final merged memory-ranges inode.

Signed-off-by: Chenyi Wang <[email protected]>
@chw120

Chenyi Wang (chw120) commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks Anna Pendleton (@annapendleton). Updated StartWriteback per #3485 (comment) to open and close one file per check instead of holding open fds across sleeps/waits.

@chw120

Chenyi Wang (chw120) commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator Author

The change has been included in #3485. Close the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenTheElder Benjamin Elder (BenTheElder) Awaiting requested review from BenTheElder

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chw120 @BenTheElder