Skip to content

fix: lock newly allocated pages before publishing mappings - #42

Open
Konstantin V Shvachko (shvachko) wants to merge 1 commit into
microsoft:mainfrom
shvachko:fix/allocation-lock-panic
Open

Konstantin V Shvachko (shvachko) wants to merge 1 commit into
microsoft:mainfrom
shvachko:fix/allocation-lock-panic

Conversation

@shvachko

Copy link
Copy Markdown

Problem

Both PageTable::alloc_base_page_mapping and insert_mini_page_mapping publish an unlocked mapping before calling try_write().unwrap(). Snapshot sweep discovers pages directly through the page-table iterator and can take a reader lock in that interval. Allocation then panics on real reader contention. The publication window also permits reading a page before initialization is finished.

This is a demonstrated publication race, not merely a conjecture about spurious weak-CAS failure. The regression tests reproduce the original panics at src/storage.rs:150:44 and src/storage.rs:183:44 with Shuttle.

Fix

  • Add MappingTable::insert_with, which executes an initialization callback while the existing insertion mutex still prevents iterators from discovering the new ID.
  • Acquire the fresh entry's write guard in that callback for both allocation paths.
  • Return the guard to protect page initialization until the caller releases it.
  • Preserve the existing simple insertion helper and the lock primitives; no extra global lock, unsafe code, lock-primitive changes, or promotion workaround.
  • Document the publication invariant and regression requirements in doc/snapshot-recovery.md.

The callback only locks the fresh, unpublished entry and must not reenter the table. Changing to a blocking write lock after publication would not protect the initialization window; changing weak CAS to strong CAS would not remove genuine reader contention.

Regression tests

Two tests in storage::tests::allocation_publication cover base-page allocation and mini-page insertion. Each explores up to 10,000 DFS schedules using the existing Shuttle dependency. A snapshot-style iterator probes for a reader lock on the new entry; if acquired, it retains the guard while waiting for allocation to finish. Allocation must complete without panic/deadlock, and the reader must observe completed initialization.

cargo +1.93.0 test --release --features shuttle storage::tests::allocation_publication -- --test-threads=1
cargo +1.93.0 test --release storage::tests -- --test-threads=1

The nonblocking read probe avoids an unfair DFS schedule spinning indefinitely in the blocking lock. Shuttle's ordering/model limitations still apply; these tests do not claim exhaustive weak-memory verification.

Validation

  • Windows ARM64 and Linux ARM64, Rust 1.93.0: both allocation regressions pass; the existing storage round-trip test passes.
  • Negative control with the final tests and original allocation code: both regressions fail at their respective original try_write().unwrap() sites.
  • With the separate snapshot fix/tests from fix: handle cached snapshot records with recovery regressions #41 combined in an isolated evaluation branch, both native snapshot regressions also pass on both platforms.

Related downstream complete-state suite with both fixes combined:

Configuration Result
Windows ARM64 / FST 111 cases, 26,089 assertions passed
Linux ARM64 / FST 111 cases, 26,089 assertions passed
Linux ARM64 / HASH 111 cases, 26,025 assertions passed
Windows ARM64 / HASH Blocked by the separate recovery lock-upgrade panic at src/snapshot.rs:1288:83, returning Locked

The failing integration run is retained, not retried until green. This PR fixes allocation publication; it does not fix or claim full downstream acceptance of the independent recovery lock-upgrade path.

Scope

Independent PR against upstream main (ca6130715eeba872682747852b5da2cde32838aa). It does not include the cache-conversion fix from #38 / #41 and can be reviewed separately. No downstream source or dependency pin is changed.

Acquire the base/mini page write guard under the mapping insertion mutex, before snapshot iterators can discover the new ID. Reproduce both publication races with bounded Shuttle regressions and document the initialization invariant.

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

Copy link
Copy Markdown
Author

Problem definition and reproduction

The allocation panic is caused by publishing the page-table entry before acquiring its exclusive initialization lock. A valid interleaving on the original code is:

  1. Allocator: MappingTable::insert(RwLock::new(location)) inserts the entry, advances next_id, and releases the insertion mutex.
  2. Snapshot sweep: the page-table iterator reads that high-water mark and discovers the new entry; its reader acquires the entry's read lock.
  3. Allocator: value.try_write() now returns Err(()); the following unwrap() panics.

The same sequence applies to both alloc_base_page_mapping (original line 150) and insert_mini_page_mapping (original line 183). A new page not yet linked into the tree is still discoverable through this iterator. This is real contention: the regression does not need a spurious weak-CAS failure to trigger it.

The required invariant is: a new mapping is already exclusively locked when its ID becomes visible, and the allocator keeps that guard until initialization is finished. Acquiring a blocking lock after publication is insufficient because the sweep could already be reading the new, not-yet-initialized page.

The two regression tests run a real page-table allocation against a snapshot-style reader under bounded Shuttle DFS scheduling. The reader probes instead of spinning under an unfair scheduler; if it obtains a guard, it holds it while joining the allocator. On the original code, both tests reproduce:

src/storage.rs:150:44  (base allocation)
src/storage.rs:183:44  (mini allocation)
called `Result::unwrap()` on an `Err` value: ()

To run:

cargo +1.93.0 test --release --features shuttle storage::tests::allocation_publication -- --test-threads=1

For the negative control, keep the final regression tests but restore the two allocation call sites to table.insert(entry) followed by value.try_write().unwrap(). Both then fail. With this PR's lock-before-publication change, both pass on Windows ARM64 and Linux ARM64.

Separate remaining issue: combined downstream validation with the snapshot fix still reproduced the recovery ReadGuard::upgrade() failure at src/snapshot.rs:1288:83 on Windows HASH. That path is unchanged here and must not be conflated with the now-reproduced allocation publication race. The full passing/failing matrix is in the PR description.

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.

1 participant