From 07cef1471cb578671aafcd96fa0d40dbd0683994 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Mon, 22 Jun 2026 20:58:35 -0700 Subject: [PATCH] refactor: extract the commit protocol (3-fsync sequence) into commit.rs --- src/transaction/commit.rs | 172 +++++++++++++++++++++++++++++++++++ src/transaction/freemap.rs | 12 --- src/transaction/lifecycle.rs | 149 ++++-------------------------- src/transaction/mod.rs | 1 + 4 files changed, 190 insertions(+), 144 deletions(-) create mode 100644 src/transaction/commit.rs diff --git a/src/transaction/commit.rs b/src/transaction/commit.rs new file mode 100644 index 0000000..46db23d --- /dev/null +++ b/src/transaction/commit.rs @@ -0,0 +1,172 @@ +//! transaction::commit — the 3-fsync commit protocol (I28 pre-drain flush -> +//! freemap persist -> data flush -> superblock write+fsync -> roots promotion). +//! The data-fsync-before-superblock-fsync ordering and the I18 allocate-before- +//! merge invariant are what make shadow paging crash-safe; this sequence is moved +//! out of lifecycle.rs VERBATIM. Stateless: operates over the manager's parts via +//! CommitCtx. The poison-on-error wrapper stays on TransactionManager::commit. + +use super::*; + +/// Borrows of the exactly-ten pieces of `TransactionManager` state the commit +/// protocol touches, bundled so `commit_inner` can stay a thin caller while the +/// load-bearing sequence lives here. All fields are distinct manager fields (plus +/// the shared `&RefCell` for the cache), so the borrow checker accepts the +/// simultaneous distinct-field borrows the caller constructs. +pub(super) struct CommitCtx<'a> { + pub cache: &'a std::cell::RefCell, + pub savepoints: &'a mut Vec, + pub txn_freed_pages: &'a mut Vec, + pub freemap: &'a mut freemap::FreemapRecycle, + pub packer: &'a mut packing::SlotPacker, + pub committed_roots: &'a mut Roots, + pub current_roots: &'a mut Roots, + pub txn_counter: &'a mut u64, + pub active_txn: &'a mut bool, + pub superblock_count: u32, +} + +pub(super) fn run_commit(ctx: &mut CommitCtx<'_>) -> Result<()> { + // I27: flatten every still-active savepoint's `freed_pages` + // back into `txn_freed_pages` before persist_freemap consumes + // it. savepoint_inner moves `txn_freed_pages` INTO the + // savepoint record (via std::mem::take), so any frees that + // happened before a still-unreleased savepoint otherwise get + // dropped on the floor when step 5 calls `savepoints.clear()` + // — a permanent freemap leak for the "commit with savepoint + // active" pattern. Mirrors `release_inner`'s merge but applied + // across the full stack. We take the lists out of the + // savepoints (rather than iterating by reference) so the + // savepoints hold no stale `freed_pages` if we ever change + // step 5 to drain instead of clear; current code is equivalent + // either way. + for sp in ctx.savepoints.iter_mut() { + ctx.txn_freed_pages.append(&mut sp.freed_pages); + } + + // I28: drain the page cache BEFORE persist_freemap runs. Without + // this, `persist_freemap`'s own freemap-page allocation + // (`structural_extend` → `new_page`) can trip + // `maybe_evict`'s spill-or-CacheFull decision (every existing entry + // dirty, nothing evictable, and either spillway disabled or full) + // and return `ChiselError::CacheFull` or `ChiselError::SpillwayFull`. + // The CacheFull variant is operational-by-design (I19 docs: "caller + // recovers by + // committing or rolling back"), but commit's poison wrapper fires + // on any error once the protocol has started — demoting an + // operational signal to fatal for a caller who has no legal + // action left (commit is precisely what failed). Pre-draining + // clears every dirty pin so the ceiling is reachable via normal + // eviction when persist_freemap itself allocates. Cost: one + // extra fsync on every commit. That is consistent with the + // project's explicit "durability over performance" posture — + // the alternative reclassifies CacheFull as fatal inside commit, + // which is both more surprising and harder to document cleanly. + // + // Ordering note: this flush is safe to do before persist_freemap. + // The shadow-paging invariant requires "new-freemap-page durable + // before superblock" (step 1's flush does that). The pre-drain + // only affects user-dirty pages, which are already part of the + // transaction's durable write set — just flushed earlier. The + // subsequent step 1 flush handles the one new freemap page + // persist_freemap adds. + ctx.cache.borrow_mut().flush()?; + + // Step 0 (ISSUES.md R2 / I11): persist the freemap tree. This marks + // `txn_freed_pages` (plus the prior commit's deferred structural frees) + // free in a COW of the committed tree and updates + // `current_roots.{freemap_page, freemap_depth}`. Runs BEFORE the main + // flush so the new freemap pages join the same durable write set as all + // other dirty data pages. + // + // Persist's own borrow lives in this scope so it drops before the held + // step-1-through-4 borrow acquired below: a second `borrow_mut` on the + // RefCell while the first is still live would panic. `current_roots` + // (mut), `freemap` (mut), and `txn_freed_pages` (read) are disjoint, so + // `txn_freed_pages` passes by `&` with no take/restore dance. + { + let mut cache = ctx.cache.borrow_mut(); + ctx.freemap + .persist(&mut cache, ctx.current_roots, ctx.txn_freed_pages)?; + } + + // Hold one RefMut for the remaining steps. Dropping and + // re-borrowing between steps would be semantically identical + // but noisier. + let mut cache = ctx.cache.borrow_mut(); + + // Step 1: Flush all dirty pages (PageCache::flush internally fsyncs). + // After this, every page the new superblock will reference is on disk. + cache.flush()?; + + // Step 2: Build the new superblock. Bumping txn_counter here both makes + // it outrank the current superblock on recovery AND (via parity) picks + // the target slot in step 3. + // + // I119 (ISSUES.md, 2026-06-21): checked, not `+= 1`. A wrapped counter + // would corrupt `Superblock::select`'s "highest counter wins" (release + // wrap to 0) — far worse than the loud, controlled panic here. Overflow + // needs 2^64 commits, so it is structurally unreachable; a dedicated + // fatal error variant for an impossible event would be speculative + // public surface, so the `expect` on the invariant is proportionate. + *ctx.txn_counter = ctx + .txn_counter + .checked_add(1) + .expect("txn_counter overflowed u64 (2^64 commits) — unreachable"); + let total_pages = cache.file_page_count()?; + let sb = Superblock { + magic: page::MAGIC, + format_version: page::FORMAT_VERSION, + txn_counter: *ctx.txn_counter, + root_handle_table_page: ctx.current_roots.handle_table_page, + root_freemap_page: ctx.current_roots.freemap_page, + total_pages, + next_handle: ctx.current_roots.next_handle, + page_size: PAGE_SIZE as u32, + named_roots: ctx.current_roots.named_roots, + // R4: every slot records the current N so open-time + // recovery can discover it from the winning slot without + // external hints. + superblock_count: ctx.superblock_count, + root_membership_index_page: ctx.current_roots.membership_index_page, + // Freemap tree depth, paired with root_freemap_page. 0 = today's + // single-leaf format; grows as the tree deepens. + freemap_depth: ctx.current_roots.freemap_depth, + }; + let buf = sb.serialize(); + // Step 3: Write to the INACTIVE slot. For N superblock slots, + // the slot is `txn_counter % N` — a round-robin that always + // targets the stalest slot. With N=2 this is the parity + // alternation from the original layout; with N>=3 it extends + // to true round-robin. The currently-active slot (and every + // other non-target slot) is never touched, so a torn write + // here can only damage the new superblock, never the N-1 + // last-known-good ones. + let inactive = *ctx.txn_counter % ctx.superblock_count as u64; + cache.io_mut().write_page(inactive, &buf)?; + // Step 4: Durability linearization point. Until this fsync returns the + // transaction is not crash-safe; after it returns the new state is + // observable on recovery. + cache.io_mut().fsync()?; + + // Step 5: Promote in-memory state. Only now is the txn officially committed. + *ctx.committed_roots = ctx.current_roots.clone(); + ctx.committed_roots.total_pages = total_pages; + // The committed freemap tree advances automatically: its {root, depth} + // ride in current_roots, promoted into committed_roots just above. No + // separate in-memory freemap copy to advance. + // R1: promote the live-slot counts. The cursor is per-transaction + // and gets reset for the next begin(). + ctx.packer.commit(); + *ctx.active_txn = false; + ctx.savepoints.clear(); + // txn_freed_pages were already marked free in the new committed freemap + // tree by persist_freemap; clear the vector now that it's done its job. + ctx.txn_freed_pages.clear(); + // Clear the freemap session set (every page COW'd this transaction is now + // committed) and promote the structural recycle for the next transaction: + // this commit's supersedes + the unconsumed reuse remainder become next + // transaction's `pending_structural_frees`. See `FreemapRecycle::commit`. + ctx.freemap.commit(); + + Ok(()) +} diff --git a/src/transaction/freemap.rs b/src/transaction/freemap.rs index ba27d55..eebbe2b 100644 --- a/src/transaction/freemap.rs +++ b/src/transaction/freemap.rs @@ -657,18 +657,6 @@ impl TransactionManager { Ok(()) } - /// Commit-path wrapper: persist this commit's data frees into a COW of the - /// committed freemap tree. `current_roots` (mut), `freemap` (mut), - /// `txn_freed_pages` (immutable read), and the cache borrow are disjoint, so - /// `txn_freed_pages` passes by `&` with no take/restore dance. `commit_inner` - /// calls this BEFORE cache.flush() so the new freemap pages join the same - /// durable write set. - pub(super) fn persist_freemap(&mut self) -> Result<()> { - let mut cache = self.cache.borrow_mut(); - self.freemap - .persist(&mut cache, &mut self.current_roots, &self.txn_freed_pages) - } - /// Commit-path wrapper: reclaim crash-orphaned freemap pages. Read the /// savepoint-active flag and superblock count into locals BEFORE borrowing the /// cache to keep the borrows clean. `pub(crate)` — defrag calls it. diff --git a/src/transaction/lifecycle.rs b/src/transaction/lifecycle.rs index 6e26056..747b28e 100644 --- a/src/transaction/lifecycle.rs +++ b/src/transaction/lifecycle.rs @@ -199,139 +199,24 @@ impl TransactionManager { } fn commit_inner(&mut self) -> Result<()> { - // I27: flatten every still-active savepoint's `freed_pages` - // back into `txn_freed_pages` before persist_freemap consumes - // it. savepoint_inner moves `txn_freed_pages` INTO the - // savepoint record (via std::mem::take), so any frees that - // happened before a still-unreleased savepoint otherwise get - // dropped on the floor when step 5 calls `savepoints.clear()` - // — a permanent freemap leak for the "commit with savepoint - // active" pattern. Mirrors `release_inner`'s merge but applied - // across the full stack. We take the lists out of the - // savepoints (rather than iterating by reference) so the - // savepoints hold no stale `freed_pages` if we ever change - // step 5 to drain instead of clear; current code is equivalent - // either way. - for sp in self.savepoints.iter_mut() { - self.txn_freed_pages.append(&mut sp.freed_pages); - } - - // I28: drain the page cache BEFORE persist_freemap runs. Without - // this, `persist_freemap`'s own freemap-page allocation - // (`structural_extend` → `new_page`) can trip - // `maybe_evict`'s spill-or-CacheFull decision (every existing entry - // dirty, nothing evictable, and either spillway disabled or full) - // and return `ChiselError::CacheFull` or `ChiselError::SpillwayFull`. - // The CacheFull variant is operational-by-design (I19 docs: "caller - // recovers by - // committing or rolling back"), but commit's poison wrapper fires - // on any error once the protocol has started — demoting an - // operational signal to fatal for a caller who has no legal - // action left (commit is precisely what failed). Pre-draining - // clears every dirty pin so the ceiling is reachable via normal - // eviction when persist_freemap itself allocates. Cost: one - // extra fsync on every commit. That is consistent with the - // project's explicit "durability over performance" posture — - // the alternative reclassifies CacheFull as fatal inside commit, - // which is both more surprising and harder to document cleanly. - // - // Ordering note: this flush is safe to do before persist_freemap. - // The shadow-paging invariant requires "new-freemap-page durable - // before superblock" (step 1's flush does that). The pre-drain - // only affects user-dirty pages, which are already part of the - // transaction's durable write set — just flushed earlier. The - // subsequent step 1 flush handles the one new freemap page - // persist_freemap adds. - self.cache.borrow_mut().flush()?; - - // Step 0 (ISSUES.md R2 / I11): persist the freemap tree. This marks - // `txn_freed_pages` (plus the prior commit's deferred structural frees) - // free in a COW of the committed tree and updates - // `current_roots.{freemap_page, freemap_depth}`. Runs BEFORE the main - // flush so the new freemap pages join the same durable write set as all - // other dirty data pages. - self.persist_freemap()?; - - // Hold one RefMut for the remaining steps. Dropping and - // re-borrowing between steps would be semantically identical - // but noisier. - let mut cache = self.cache.borrow_mut(); - - // Step 1: Flush all dirty pages (PageCache::flush internally fsyncs). - // After this, every page the new superblock will reference is on disk. - cache.flush()?; - - // Step 2: Build the new superblock. Bumping txn_counter here both makes - // it outrank the current superblock on recovery AND (via parity) picks - // the target slot in step 3. - // - // I119 (ISSUES.md, 2026-06-21): checked, not `+= 1`. A wrapped counter - // would corrupt `Superblock::select`'s "highest counter wins" (release - // wrap to 0) — far worse than the loud, controlled panic here. Overflow - // needs 2^64 commits, so it is structurally unreachable; a dedicated - // fatal error variant for an impossible event would be speculative - // public surface, so the `expect` on the invariant is proportionate. - self.txn_counter = self - .txn_counter - .checked_add(1) - .expect("txn_counter overflowed u64 (2^64 commits) — unreachable"); - let total_pages = cache.file_page_count()?; - let sb = Superblock { - magic: page::MAGIC, - format_version: page::FORMAT_VERSION, - txn_counter: self.txn_counter, - root_handle_table_page: self.current_roots.handle_table_page, - root_freemap_page: self.current_roots.freemap_page, - total_pages, - next_handle: self.current_roots.next_handle, - page_size: PAGE_SIZE as u32, - named_roots: self.current_roots.named_roots, - // R4: every slot records the current N so open-time - // recovery can discover it from the winning slot without - // external hints. + // The 3-fsync commit protocol lives in `commit::run_commit`, operating + // over the ten pieces of manager state it touches via `CommitCtx`. The + // ordering there is load-bearing (see the step-by-step rationale on + // `commit()` above and in `commit.rs`); this stays a thin caller. All + // `&mut self.` borrows are distinct fields and `&self.cache` is a + // shared borrow, so the borrow checker accepts the simultaneous borrows. + commit::run_commit(&mut commit::CommitCtx { + cache: &self.cache, + savepoints: &mut self.savepoints, + txn_freed_pages: &mut self.txn_freed_pages, + freemap: &mut self.freemap, + packer: &mut self.packer, + committed_roots: &mut self.committed_roots, + current_roots: &mut self.current_roots, + txn_counter: &mut self.txn_counter, + active_txn: &mut self.active_txn, superblock_count: self.superblock_count, - root_membership_index_page: self.current_roots.membership_index_page, - // Freemap tree depth, paired with root_freemap_page. 0 = today's - // single-leaf format; grows as the tree deepens. - freemap_depth: self.current_roots.freemap_depth, - }; - let buf = sb.serialize(); - // Step 3: Write to the INACTIVE slot. For N superblock slots, - // the slot is `txn_counter % N` — a round-robin that always - // targets the stalest slot. With N=2 this is the parity - // alternation from the original layout; with N>=3 it extends - // to true round-robin. The currently-active slot (and every - // other non-target slot) is never touched, so a torn write - // here can only damage the new superblock, never the N-1 - // last-known-good ones. - let inactive = self.txn_counter % self.superblock_count as u64; - cache.io_mut().write_page(inactive, &buf)?; - // Step 4: Durability linearization point. Until this fsync returns the - // transaction is not crash-safe; after it returns the new state is - // observable on recovery. - cache.io_mut().fsync()?; - - // Step 5: Promote in-memory state. Only now is the txn officially committed. - self.committed_roots = self.current_roots.clone(); - self.committed_roots.total_pages = total_pages; - // The committed freemap tree advances automatically: its {root, depth} - // ride in current_roots, promoted into committed_roots just above. No - // separate in-memory freemap copy to advance. - // R1: promote the live-slot counts. The cursor is per-transaction - // and gets reset for the next begin(). - self.packer.commit(); - self.active_txn = false; - self.savepoints.clear(); - // txn_freed_pages were already marked free in the new committed freemap - // tree by persist_freemap; clear the vector now that it's done its job. - self.txn_freed_pages.clear(); - // Clear the freemap session set (every page COW'd this transaction is now - // committed) and promote the structural recycle for the next transaction: - // this commit's supersedes + the unconsumed reuse remainder become next - // transaction's `pending_structural_frees`. See `FreemapRecycle::commit`. - self.freemap.commit(); - - Ok(()) + }) } /// Abort the active transaction and discard all in-memory changes. diff --git a/src/transaction/mod.rs b/src/transaction/mod.rs index 87bd9bd..f94a342 100644 --- a/src/transaction/mod.rs +++ b/src/transaction/mod.rs @@ -224,6 +224,7 @@ pub struct TransactionManager { fault: fault::FaultInjector, } +mod commit; mod config; #[cfg(test)] mod fault;