From 1bf2a4b520ccb6deee74525a8f3c18e83d668fc4 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 30 Jun 2025 15:23:35 -0500 Subject: [PATCH] Fixed grafting allocator checkpoint hole This was quite a deep bug. We don't track the original bshrub when grafting, so it was possible to realloc those blocks even when we need their contents to finish the graft operation. This was found while experimenting with eager leaf grafting, but can also occur when grafting data fragments. --- In theory, the block allocator's checkpoint mechanism protects against this. Before we alloc, we set a checkpoint with lfs3_alloc_ckpoint. This marks the position of the block allocator before allocation, so if we loop around the entire block device we don't double alloc any in-flight blocks: ckpoint lookahead v .---'---. [mm---ddd-d---d-------|dd--d-ddd|--------d-----d-] '---.---' in-flight allocations But this only protects _new_ blocks, _old_ blocks can be anywhere on disk and are unprotected. In theory again, old blocks are always tracked via copy-on-write snapshots, but this is not the case for bshrubs while grafting! Grafting is unfortunately a multi-commit operation (we may remove multiple fragments that span different btree nodes), and each bshrub commit discards the old snapshot. This creates a window where old blocks can be double alloced _while grafting_, leading to corrupted data. You may wonder why are we discarding the old snapshot? Why not keep track of it until the grafting completes? The problem there is that we need the intermediate snapshot in order for shrubs to survive compactions. We really have 3 states: old -> mid-graft -> new And the only one we don't need to fallback to is the old state. --- A couple solutions: 1. Track all three states This would add complexity increase the cost of every lfs3_file_t. 2. Open a temporary file to track the old state This would add complexity and a big chunk of stack to what is already one of the critical functions on our stack hot-path. 3. Carefully make sure graft commits don't lose track of in-flight data until an atomic commit This doesn't work when you're trying to coalesce two data fragments in two different btree nodes. At least not without completely restructuring the btree commit logic. 4. Just explicitly track in-flight graft state out-of-band This goes with option no 4., adding lfs3->graft and lfs3->graft_count to track in-flight graft state when we're grafting. lfs3_mtree_traverse_ can include the relevant blocks during traversals, effectively masking out graft state from the lookahead buffer. This adds a bit of code/ctx, but is probably the cheapest option: code stack ctx before: 37936 2456 636 after: 38092 (+0.4%) 2456 (+0.0%) 656 (+3.1%) --- lfs3.c | 105 ++++++++++++++++++++++++++++++++++++++++++++------------- lfs3.h | 7 ++++ 2 files changed, 89 insertions(+), 23 deletions(-) diff --git a/lfs3.c b/lfs3.c index 5526e67a..eaba06d3 100644 --- a/lfs3.c +++ b/lfs3.c @@ -9722,8 +9722,9 @@ enum { LFS3_TSTATE_BTREE = 5, LFS3_TSTATE_OMDIRS = 6, LFS3_TSTATE_OBTREE = 7, + LFS3_TSTATE_GRAFT = 8, #endif - LFS3_TSTATE_DONE = 8, + LFS3_TSTATE_DONE = 9, }; static void lfs3_traversal_init(lfs3_traversal_t *t, uint32_t flags) { @@ -9743,6 +9744,9 @@ static void lfs3_traversal_init(lfs3_traversal_t *t, uint32_t flags) { t->gcksum = 0; } +// needed in lfs3_mtree_traverse_ +static inline lfs3_size_t lfs3_graft_count(lfs3_size_t graft_count); + // low-level traversal _only_ finds blocks static int lfs3_mtree_traverse_(lfs3_t *lfs3, lfs3_traversal_t *t, lfs3_tag_t *tag_, lfs3_bptr_t *bptr) { @@ -9857,9 +9861,10 @@ static int lfs3_mtree_traverse_(lfs3_t *lfs3, lfs3_traversal_t *t, err = lfs3_mtree_lookup(lfs3, t->b.o.mdir.mid, &t->b.o.mdir); if (err) { - // end of mtree? guess we're done + // end of mtree? all that's left is any graft state if (err == LFS3_ERR_NOENT) { - lfs3_t_settstate(&t->b.o.flags, LFS3_TSTATE_DONE); + t->u.gt = 0; + lfs3_t_settstate(&t->b.o.flags, LFS3_TSTATE_GRAFT); continue; } return err; @@ -10025,6 +10030,21 @@ static int lfs3_mtree_traverse_(lfs3_t *lfs3, lfs3_traversal_t *t, continue; #endif + // traverse through in-flight grafts + case LFS3_TSTATE_GRAFT:; + // done? + if (t->u.gt >= lfs3_graft_count(lfs3->graft_count)) { + lfs3_t_settstate(&t->b.o.flags, LFS3_TSTATE_DONE); + continue; + } + + if (tag_) { + *tag_ = LFS3_TAG_DATA; + } + bptr->data = lfs3->graft[t->u.gt]; + t->u.gt += 1; + return 0; + case LFS3_TSTATE_DONE:; return LFS3_ERR_NOENT; @@ -10362,7 +10382,8 @@ static void lfs3_alloc_markinuse(lfs3_t *lfs3, lfs3_rbyd_t *rbyd = (lfs3_rbyd_t*)bptr->data.u.buffer; lfs3_alloc_markinuse_(lfs3, rbyd->blocks[0]); - } else if (tag == LFS3_TAG_BLOCK) { + } else if (tag == LFS3_TAG_BLOCK + || tag == LFS3_TAG_DATA) { lfs3_alloc_markinuse_(lfs3, lfs3_bptr_block(bptr)); } else { @@ -12286,13 +12307,22 @@ static int lfs3_file_commit(lfs3_t *lfs3, lfs3_file_t *file, } #endif +// use this flag to indicate bptr vs concatenated data fragments +#define LFS3_GRAFT_ISBPTR 0x80000000 + +static inline bool lfs3_graft_isbptr(lfs3_size_t graft_count) { + return graft_count & LFS3_GRAFT_ISBPTR; +} + +static inline lfs3_size_t lfs3_graft_count(lfs3_size_t graft_count) { + return graft_count & ~LFS3_GRAFT_ISBPTR; +} + // graft bptr/fragments into our bshrub/btree #if !defined(LFS3_RDONLY) && !defined(LFS3_KVONLY) static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, lfs3_off_t pos, lfs3_off_t weight, lfs3_soff_t delta, - // data_count=-1 => single bptr - // data_count>=0 => list of concatenated fragments - const lfs3_data_t *datas, lfs3_ssize_t data_count) { + const lfs3_data_t *graft, lfs3_ssize_t graft_count) { // note! we must never allow our btree size to overflow, even // temporarily @@ -12307,12 +12337,32 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, return 0; } + // keep track of in-flight graft state + // + // normally, in-flight state would be protected by the block + // allocator's checkpoint mechanism, where checkpoints prevent double + // allocation of new blocks while the old copies remain tracked + // + // but we don't track the original bshrub copy during grafting! + // + // in theory, we could track 3 copies of the bshrub/btree: before + // after, and mid-graft (we need the mid-graft copy to survive mdir + // compactions), but that would add a lot of complexity/state to a + // critical function on the stack hot-path + // + // instead, we can just explicitly track any in-flight graft state to + // make sure we don't allocate these blocks in-between commits + // + lfs3->graft = graft; + lfs3->graft_count = graft_count; + // try to merge commits where possible lfs3_bid_t bid = file->b.shrub.weight; lfs3_rattr_t rattrs[3]; lfs3_size_t rattr_count = 0; lfs3_bptr_t l; lfs3_bptr_t r; + int err; // need a hole? if (pos > file->b.shrub.weight) { @@ -12335,11 +12385,11 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, while (pos < file->b.shrub.weight) { lfs3_bid_t weight_; lfs3_bptr_t bptr_; - int err = lfs3_file_lookupnext_(lfs3, file, pos, + err = lfs3_file_lookupnext_(lfs3, file, pos, &bid, &weight_, &bptr_); if (err) { LFS3_ASSERT(err != LFS3_ERR_NOENT); - return err; + goto failed; } // note, an entry can be both a left and right sibling @@ -12374,7 +12424,7 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, +(weight_ - lfs3->cfg->fragment_size), &bptr_))); if (err) { - return err; + goto failed; } weight_ -= lfs3->cfg->fragment_size; @@ -12405,7 +12455,7 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, &LFS3_DATA_FRUNCATE(r.data, lfs3->cfg->fragment_size)))); if (err) { - return err; + goto failed; } bid -= (weight_-lfs3_bptr_size(&bptr_)); @@ -12454,7 +12504,7 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, err = lfs3_file_commit(lfs3, file, bid, rattrs, rattr_count); if (err) { - return err; + goto failed; } delta += lfs3_min(weight, bid+1 - pos); @@ -12492,10 +12542,8 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, // append our data if (weight + delta > 0) { lfs3_size_t dsize = 0; - for (lfs3_size_t i = 0; - i < ((data_count < 0) ? 1 : (lfs3_size_t)data_count); - i++) { - dsize += lfs3_data_size(datas[i]); + for (lfs3_size_t i = 0; i < lfs3_graft_count(graft_count); i++) { + dsize += lfs3_data_size(graft[i]); } // can we coalesce a hole? @@ -12511,18 +12559,18 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, LFS3_TAG_DATA, +(weight + delta)); // append a new fragment? - } else if (data_count >= 0) { + } else if (!lfs3_graft_isbptr(graft_count)) { bid = lfs3_min(bid, file->b.shrub.weight); rattrs[rattr_count++] = LFS3_RATTR_CAT_( LFS3_TAG_DATA, +(weight + delta), - datas, data_count); + graft, graft_count); // append a new bptr? } else { bid = lfs3_min(bid, file->b.shrub.weight); rattrs[rattr_count++] = LFS3_RATTR_BPTR( LFS3_TAG_BLOCK, +(weight + delta), - (const lfs3_bptr_t*)datas); + (const lfs3_bptr_t*)graft); } } @@ -12535,14 +12583,21 @@ static int lfs3_file_graft(lfs3_t *lfs3, lfs3_file_t *file, if (rattr_count > 0) { LFS3_ASSERT(rattr_count <= sizeof(rattrs)/sizeof(lfs3_rattr_t)); - int err = lfs3_file_commit(lfs3, file, bid, + err = lfs3_file_commit(lfs3, file, bid, rattrs, rattr_count); if (err) { - return err; + goto failed; } } + lfs3->graft = NULL; + lfs3->graft_count = 0; return 0; + +failed:; + lfs3->graft = NULL; + lfs3->graft_count = 0; + return err; } #endif @@ -12800,7 +12855,7 @@ static int lfs3_file_crystallize(lfs3_t *lfs3, lfs3_file_t *file) { // and graft int err = lfs3_file_graft(lfs3, file, file->leaf.pos, file->leaf.weight, 0, - &file->leaf.bptr.data, -1); + &file->leaf.bptr.data, LFS3_GRAFT_ISBPTR | 1); if (err) { return err; } @@ -13181,7 +13236,7 @@ fragment:; lfs3_alloc_ckpoint(lfs3); int err = lfs3_file_graft(lfs3, file, file->leaf.pos, file->leaf.weight, 0, - &file->leaf.bptr.data, -1); + &file->leaf.bptr.data, LFS3_GRAFT_ISBPTR | 1); if (err) { return err; } @@ -14668,6 +14723,10 @@ static int lfs3_init(lfs3_t *lfs3, uint32_t flags, // zero linked-list of opened mdirs lfs3->omdirs = NULL; + // zero in-flight graft state + lfs3->graft = NULL; + lfs3->graft_count = 0; + // zero gstate lfs3->gcksum = 0; #ifndef LFS3_RDONLY diff --git a/lfs3.h b/lfs3.h index e1c87611..15b644c7 100644 --- a/lfs3.h +++ b/lfs3.h @@ -793,6 +793,8 @@ typedef struct lfs3_traversal { } mtortoise; // btree traversal state lfs3_btraversal_t bt; + // graft traversal state + lfs3_size_t gt; } u; // recalculate gcksum when traversing with ckmeta @@ -872,6 +874,11 @@ typedef struct lfs3 { } lookahead; #endif + #if !defined(LFS3_RDONLY) && !defined(LFS3_2BONLY) + const lfs3_data_t *graft; + lfs3_ssize_t graft_count; + #endif + // global state uint32_t gcksum; #ifndef LFS3_RDONLY