Skip to content

vhdx: fix concurrent allocation and metadata publication - #5063

Draft
John Starks (jstarks) wants to merge 3 commits into
microsoft:mainfrom
jstarks:investigate/vhdx-differencing-corruption
Draft

John Starks (jstarks) wants to merge 3 commits into
microsoft:mainfrom
jstarks:investigate/vhdx-differencing-corruption

Conversation

@jstarks

Copy link
Copy Markdown
Member

Concurrent partial writes can both observe an unallocated differencing block. The allocation-lock recheck misses the transition to PartiallyPresent, so a queued writer replaces the payload and loses earlier writes and parent inheritance. Handle that state explicitly and reuse the existing allocation.

Harden the related publication and rollback paths: keep zero-fill initialization ahead of payload publication, initialize each partial block’s bitmap range through the WAL, and establish safe recycled payload contents before logging the allocation. Preserve deferred-release and soft-anchor ownership on abort, and make allocation, completion, and cache acquisition cancellation-safe. Safe partial payloads avoid recurring data-flush barriers when additional sectors become present.

Add deterministic concurrency and selective-persistence regressions, first-touch flush-count checks, and a bounded sector model covering writes, trim, abort, flush, and recovery with both logical sector sizes. Each seeded configuration runs independently to stay within the test runner’s per-test timeout.

Validated on Linux after rebasing onto current main: compilation checks for vhdx and disklayer_vhdx, all-target Clippy and documentation for vhdx, 483 passing tests across both packages with the agent nextest profile, and cargo xtask fmt --fix. Native Windows cross-validation was not run.

Recheck partially present mappings under the allocation lock so queued writes reuse their payload and preserve parent inheritance. Delay publication until zero-filled payloads are initialized, clear each partial block's bitmap through the WAL, and establish safe recycled payload contents before logging their allocation.

Preserve reclaimed space ownership on rollback and make allocation, completion, and cache acquisition cancellation-safe. Add deterministic transition and selective-persistence regressions plus a seeded sector model to exercise concurrency, recovery, and first-touch flush costs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b3027827-8feb-43a4-830a-e4bb487c1328
Create differencing fixtures with the current parent API. Run each seeded model configuration as its own test so all sixteen schedules remain covered without exceeding the runner's per-test timeout.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b3027827-8feb-43a4-830a-e4bb487c1328
Copilot AI balanced review requested due to automatic review settings November 12, 2026 19:13
@github-actions github-actions Bot added the unsafe Related to unsafe code label Nov 12, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot AI 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.

🟡 Changes recommended

Cancelling zero-fill can release a region while its initialization write is still running, allowing another writer to reuse it before the outstanding write erases the new data.

1 open issue
What changed in this PR

This PR fixes concurrent VHDX allocation so queued partial writes reuse existing payloads without losing earlier writes or parent inheritance. It also hardens metadata publication and rollback in the storage backend.

Changes:

  • Order payload initialization and sector-bitmap logging before publication.
  • Track allocation ownership during rollback and add cancellation cleanup for writes and cache acquisition.
  • Add deterministic concurrency, selective-persistence, flush-count, and seeded recovery tests.
File Description
vm/​devices/​storage/​vhdx/​src/​trim.rs Clarifies bitmap initialization responsibility in comments.
vm/​devices/​storage/​vhdx/​src/​tests/​transition_tests.rs Adds cancellation, persistence, and sector-model regressions.
vm/​devices/​storage/​vhdx/​src/​tests/​support.rs Adds controlled I/O pauses, failures, and persistence scheduling.
vm/​devices/​storage/​vhdx/​src/​tests/​mod.rs Registers transition tests.
vm/​devices/​storage/​vhdx/​src/​tests/​io_tests.rs Tests concurrent allocation, rollback ownership, and bitmap preservation.
vm/​devices/​storage/​vhdx/​src/​tests/​crash_tests.rs Updates comments describing publication and flush dependencies.
vm/​devices/​storage/​vhdx/​src/​space.rs Prevents reclaiming transitioning soft anchors and adds coverage.
vm/​devices/​storage/​vhdx/​src/​sector_bitmap.rs Ensures bitmap initialization zeros are logged.
vm/​devices/​storage/​vhdx/​src/​io.rs Fixes allocation rechecks and revises publication and rollback handling.
vm/​devices/​storage/​vhdx/​src/​cache.rs Adds cancellation cleanup and pins pending page loads.

Review effort: Balanced

Comment on lines +618 to +620
self.file
.zero_range(new_offset, self.block_size as u64)
.await
Copilot AI balanced review requested due to automatic review settings November 12, 2026 20:59

Copilot AI 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.

🔵 Needs a closer look

Cancelling resolve_write can release an extent while its zeroing I/O is still running, allowing another writer to reuse the range and have its data overwritten by the unfinished zero write.

1 open issue

Review effort: Balanced

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

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants