Repository navigation
store: unlink a scripthash reorg from the tail - #1021
Conversation
|
MERGE PLZZZZZZZ :P |
|
Grok is self reviewing, will get it merged soon :) |
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Suffix unlink avoids the full-chain copy: a megakey tail is rewritten in place, an undo image sits past the shortened alloc header (224 bytes, SHUN at 256), and a non-suffix rewrite allocates the replacement before it frees. That split is right, and a second unlink of an absent suffix does not allocate. The durable protocol is not old-or-new across the windows that matter. A failed undo clear leaves SHUN armed so the next open puts the tail back after a retried disconnect has already shrunk the tip. The undo record is not fsynced before it is published, a predecessor page is overwritten while the old head still names it, and a non-suffix free can hit disk before the new head does. The new append check also rejects legitimate last pages, including a low linked tail and an inflated extent_n.
Issue counts by severity
- bugs: 5
- suggestions: 1
- nits: 0
| } | ||
| } | ||
| alloc.live_count = alloc.live_count.saturating_sub(write.removed); | ||
| clear_undo_keep_image(body, alloc)?; |
There was a problem hiding this comment.
[bug] commit_tail_page syncs the new tail and the new head, then clear_undo_keep_image is the only step that drops SHUN. If that write fails (the ENOSPC case this change is for), the error returns with the page and the head already durable and the undo record still armed. write_alloc_header is only 224 bytes and does not touch offset 256, so a later flush or another key's header write will not clear it. disconnect_tip then fails before the tip shrinks. A retry finds the creates already gone, takes SuffixPlan::Absent, and commits the tip shrink. The next open runs recover_unlink_undos and copies the saved page and the saved head word back. The disconnected block's outputs reappear on a key whose tip is already gone. A kill before that retry is fine (tip still old, recovery restores the old tail). The retry-then-restart path is not.
Suggestion: On clear_undo_keep_image failure, rollback_undo before returning so the key stays fully old and the same disconnect can run again. Do not leave an armed SHUN once the new head is durable. A short alloc-header write must not be the thing that is supposed to retire the undo.
There was a problem hiding this comment.
The new tail and the new head were already on disk before the undo record was cleared. If that clear failed, the undo stayed armed. A retry then saw the creates already gone and moved the tip. The next start copied the saved page back, so those creates showed up again on a block that was no longer the tip.
If the clear fails, we now write the saved page and the old head word back before returning the error. The key stays on the old tail, and the same disconnect can run again. 737c021
| image, | ||
| word, | ||
| }; | ||
| persist_undo(body, alloc, Some(&rec))?; |
There was a problem hiding this comment.
[bug] arm_undo writes the page image and the SHUN record, then one sync_data_only. A kill before that sync returns can persist the header block and not the image page (different file offsets; writeback is unordered). read_undo trusts magic plus aligned non-zero offsets and rollback_undo copies that image onto the live tail. The tail becomes whatever was on the unwritten page, the undo is cleared, and the key is neither the old chain nor the new one. The same disconnect cannot rebuild the lost creates. Image and record need separate barriers: sync the image, then write SHUN, then sync again. The later tail write is ordered correctly (undo is durable before write_at of the target).
Suggestion: sync_data_only after write_at of the image and before persist_undo. Keep the second sync after the record so a torn header is not trusted.
There was a problem hiding this comment.
The saved page and the undo record were written, then synced once. A kill can leave the record durable and the saved page still unwritten. Recovery would copy that blank page onto the live tail, and the creates would be gone for good.
We sync the saved page first, then write the undo record, then sync again. Recovery only runs when the record is durable, and by then the saved page is too. 737c021
| None | ||
| }; | ||
| let rec = arm_undo(self, body, alloc, write.target, word)?; | ||
| if let Err(e) = body.write_at(write.target, write.bytes) { |
There was a problem hiding this comment.
[bug] Dropping a whole tail page rewrites the predecessor in place before the head moves. prepare_boundary packs that page as a ver=2 last page (or a ver=1 page whose next now points at a new tail). The head still names the old last page, whose extent_n still covers the predecessor. collect_extent_then_tail bulk-reads that span and requires each intermediate next to be the following contiguous page. The overwritten page fails that check, so entries returns Corrupt ("extent prefix next links broken") for the whole sync_data_only plus sync_home window, which is seconds on a large body. History reads do not take the tip-commit or appender lock, so this is visible during disconnect. A crash is repaired by the undo; live readers are not. The in-place ver=2 to ver=2 rewrite of the page the head already names does not have this span break.
Suggestion: Leave every page the current head still reaches unchanged until the head word names the new tail. Put the replacement boundary on a new page (one extra page, not a chain copy) or swing the head before any byte the old span walks is overwritten.
There was a problem hiding this comment.
Dropping a whole tail page rewrote the page before it while the head still named the old last page. That earlier page was still in the middle of the published span, so a history read during the disconnect saw a broken page chain.
We first rewrite the page the head already names into a short empty tail. Readers then end the span at the previous page and follow its link onto that empty page, so the removed creates stay hidden and the chain still reads. Only then do we rewrite the previous page and move the head. The old pages are freed after the new head is synced. 737c021
| } else { | ||
| alloc.live_count = alloc.live_count.saturating_sub(old_n - new_n); | ||
| } | ||
| if let Err(e) = self.free_if_paged(body, &mut alloc, old) { |
There was a problem hiding this comment.
[bug] unlink_by_rewrite writes the replacement, write_alloc_headers the bumped allocator, then unlink_write_home, then free_if_paged with no sync_data_only and no sync_home. Those are different files. Writeback can persist the freelist pwrites that destroy the old slab or extent and drop the head pwrite. After a kill the head still names pages whose first bytes are freelist links. A failed head write does restore the in-memory allocator and tries to write the old header back, and the new slab is not allocated at the live offset before the head moves, so that narrower claim holds. The free itself is not crash-ordered. Suffix DropAll and commit_tail_page do sync the head before free; this path does not. Slab keys correctly gained no per-key fsync, but a non-suffix extent (a middle create, or a suffix the planner refused) now frees without a barrier the docs claim.
Suggestion: Sync the new body bytes, then the head, and only then free. One sync for the whole disconnect batch is enough. Do not fsync inline keys that do not free a page.
There was a problem hiding this comment.
A rewrite that replaces a slab or an extent wrote the new chain, moved the head, and freed the old pages with no sync. A kill could save the free and lose the new head, so the head still named pages that had been wiped.
For one disconnect we sync the new body bytes, then write every new head, then sync those heads, and only then free the old pages. Keys that stay inline and do not free a page are still written immediately, with no extra sync. 737c021
| }; | ||
| let contiguous_end = base_chk | ||
| .saturating_add(u64::from(n_chk.saturating_sub(1)).saturating_mul(SH_PAGE_SIZE as u64)); | ||
| if last_page < contiguous_end { |
There was a problem hiding this comment.
[bug] append_fks_to_pages rejects last_page < base + (extent_n - 1) * 4096. That is not the same as "not a real tail". A linked tail allocated from the page freelist can sit below the extent (freelist is popped before bump; suffix unlink and DropAll push page-class offsets). The append that allocates that tail still sees the old last page and succeeds; the next append sees last_page < contiguous_end and returns Corrupt, so the key can never grow. The same check rejects the real last page of an inflated extent_n. extent_span_over_cap_still_unlinks stamps extent_n past 64 MiB and still expects the key to be readable; collect_extent_then_tail treats that n as a hint and walks next. After this check, an append on that key fails before any unlink can shrink n. A linked tail past contiguous_end (the glue-slot-taken case) is allowed. new_extent_n itself does not walk off the chain or stamp a span that includes dropped pages: inside the stamped end it becomes index(boundary) + 1.
Suggestion: Reject only a last page that is strictly inside the real linked chain, not merely below the stamped geometric end. A last page below base, a linked tail, and the real tail of an over-large extent_n must still append.
There was a problem hiding this comment.
Append refused to grow a key when the last page sat below the byte offset implied by extent_n. A tail taken from the free list can sit there, and so can the real last page when extent_n is larger than the chain. The next append then failed and the key could not grow.
Append now rejects only a last page that still has a successor. A page with no next is the tail, whether or not it sits at the stamped end. 737c021
| /// A tip disconnect removes the highest creates. That must rewrite the tail | ||
| /// page, not allocate another copy of the whole extent. | ||
| #[test] | ||
| fn extent_suffix_unlink_does_not_copy_the_chain() { |
There was a problem hiding this comment.
[suggestion] The new tests call ScriptHashTable::unlink_create, unlink_creates, or test_tear_extent_tail. None call disconnect_tip. pin_megakey_block_disconnect still disconnects 257 creates of one script, which is DropAll of a one-page extent, not a multi-page suffix and not a torn head. Growth of a suffix unlink, idempotent absent unlink, and a torn page with an unchanged head word are store-local. The operator-visible holes are unpinned: a tip disconnect of a multi-page extent does not copy the body, a kill after the head word moves restores the old word on open, and a retried disconnect after a failed undo clear does not resurrect creates. test_tear_extent_tail never writes a new head, so head-word recovery is untested even in the store.
Suggestion: Extend the existing disconnect journey (or a query connect test that calls disconnect_tip) with a multi-page megakey suffix and a reopen after a torn tail whose undo carries a head word. Keep the store tests for the allocator budget.
There was a problem hiding this comment.
The chain journey now connects two blocks of one script, 5,000 creates and then 40 more, and disconnects the tip. History stays at 5,000 after that disconnect and after the node is reopened. Those 5,000 creates do not fit on one page.
Tearing the head word is still a store test: it writes an empty head after arming the undo, and reopen has to show the extent head again. A failed undo clear is covered the same way: the first unlink puts the old tail back, and a second unlink keeps the shorter key across reopen. 737c021
There was a problem hiding this comment.
The 5,000-create block was the wrong place to pay for this. On the merge-base run the scenarios binary finished in half a second. With that block it took 16 seconds, almost all of it archiving the transactions, not unlinking them.
A one-byte gap packs about 4,000 creates on a page, so a second page needs thousands of real transactions. The store tests already build that chain directly and check the suffix unlink, a failed undo clear, and a torn head word on reopen. The chain journey still calls disconnect_tip on a one-page extent (257 creates) and checks that reopen leaves those creates gone. a4aeee0
c343102 to
737c021
Compare
A one-block reorg rewrote a megakey once per output and freed the old pages before the replacement existed. Disconnect now drops that block's creates from the tail, keeps the previous pages readable until the new tail is the head, and restores a torn tail from an undo image on open.
Open rolls a saved tail back through the same path as a failed write. Ingest, sealed overflow, and a linked tail are covered.
A failed undo clear puts the old tail back. The saved page is synced before the undo record, and the head page shrinks before an earlier page is rewritten. A rewrite syncs once per disconnect, then frees. Append follows the real tail, and a multi-page disconnect survives reopen.
Archiving 5,000 transactions to force a second extent page made the scenarios binary take 16s instead of half a second. Multi-page suffix unlink, undo rollback, and torn-head reopen stay in the store tests. The journey still disconnects a one-page extent and checks reopen.
The ver=2 check already rejects a ver=1 last page, so the branch that built one could not run and kept append over the CRAP gate. Store tests walk an empty head page, drop a key whose predecessor page is empty, and append when the creates counter is zero.
a4aeee0 to
c316287
Compare
2 participants
Fixes #1018.
A one-block reorg of a megakey unlinked once per output. Each call copied the whole page chain, freed the old pages, and only then moved the head. On a full disk or a kill, the head pointed at a page whose version byte had been overwritten, and later reads failed with
scripthash extent last page not ver=2. The winning block could not connect until that disconnect finished, so shutdown sat in the loop until systemd sent SIGKILL.Disconnect now groups the block's outputs by scripthash and drops those creates from the tail. The old page is saved, the new tail is written, then the head moves. Dropped pages are freed only after that. Open restores a torn tail and the old head word, so the same disconnect can run again. A create that is not a suffix still rewrites the key once, and the old chain stays readable until the replacement is the head. Schema version is unchanged.
pin_megakey_block_disconnectstill drops a whole 257-create key.