From b613b65921c3fd6c3e03e0df9bb686f6478cf332 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Thu, 22 May 2025 14:51:40 -0500 Subject: [PATCH] Fixed a nasty overrecycling + shrub + ckprog bug In lfsr_mdir_compact__, we rely on shrub_.block != mdir.block to avoid compacting shrubs multiple times. This works for the most part because we set shrub_.block = shrub.block (the old mdir block) at the beginning of lfsr_mdir_commit. We don't actually reset shrub_.block on a bad prog, but in theory that was ok because we never try to compact into the same block twice. But this falls apart if we overrecycle the mdir! With overrecycling, if we encounter a bad prog during a compaction and there are no more blocks to relocate to, we try one last time to compact into the same block (this logic is mainly for recycle overflows, where it makes a bit more sense). Of course, compacting into the same block breaks the above shrub_.block != mdir.block invariant, which causes the shrub compaction to be skipped, uses the old shrub_.trunk (which now points to garbage), and breaks everything. Fortunately the solution is relatively simple: Just discard any staged shrubs that have been committed when we relocate/overrecycle. --- While fixing this I went ahead and renamed overcompaction -> overrecycling. To me, overcompaction implies something _very_ different, and I think this better describes the relationship between overrecycling and block_recycles. Also added test_ck_ckprogs_overrecycling to nail this down and prevent a regression in the future. This bug _was_ caught by test_ck_spam_fwrite_fuzz, but only after unrelated fs changes. Adds a bit of code, but a smaller + dysfunctional filesystem is not very useful: code stack ctx before: 37056 2304 (+0.0%) 636 (+0.0%) after: 37088 (+0.1%) 2304 (+0.0%) 636 (+0.0%) --- lfs.c | 25 ++++-- tests/test_ck.toml | 217 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 233 insertions(+), 9 deletions(-) diff --git a/lfs.c b/lfs.c index 386d3168..ca448d35 100644 --- a/lfs.c +++ b/lfs.c @@ -3778,6 +3778,9 @@ static int lfsr_rbyd_p_flush(lfs_t *lfs, lfsr_rbyd_t *rbyd, static inline int lfsr_rbyd_p_push(lfs_t *lfs, lfsr_rbyd_t *rbyd, lfsr_alt_t p[static 3], lfsr_tag_t alt, lfsr_rid_t weight, lfs_size_t jump) { + // jump should actually be in the rbyd + LFS_ASSERT(jump < lfsr_rbyd_eoff(rbyd)); + int err = lfsr_rbyd_p_flush(lfs, rbyd, p, 1); if (err) { return err; @@ -8419,7 +8422,7 @@ static int lfsr_mdir_commit_(lfs_t *lfs, lfsr_mdir_t *mdir, swap:; // can't commit, can we compact? bool relocated = false; - bool overcompacted = false; + bool overrecycled = false; // check if we're within our compaction threshold lfs_ssize_t estimate = lfsr_mdir_estimate__(lfs, mdir, start_rid, end_rid, @@ -8447,7 +8450,7 @@ swap:; relocate:; // needs relocation? bad prog? ok, try allocating a new mdir err = lfsr_mdir_alloc__(lfs, &mdir_, mdir->mid, relocated); - if (err && !(err == LFS_ERR_NOSPC && !overcompacted)) { + if (err && !(err == LFS_ERR_NOSPC && !overrecycled)) { return err; } relocated = true; @@ -8455,10 +8458,10 @@ relocate:; // no more blocks? wear-leveling falls apart here, but we can try // without relocating if (err == LFS_ERR_NOSPC) { - LFS_WARN("Overcompacting mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", + LFS_WARN("Overrecycling mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"}", lfsr_dbgmbid(lfs, mdir->mid), mdir->rbyd.blocks[0], mdir->rbyd.blocks[1]); - overcompacted = true; + overrecycled = true; err = lfsr_mdir_swap__(lfs, &mdir_, mdir, true); if (err) { @@ -8473,6 +8476,16 @@ relocate:; } } + // discard any committed shrubs, we need to do this explicitly + // when overrecycling + for (lfsr_omdir_t *o = lfs->omdirs; o; o = o->next) { + if (lfsr_o_isbshrub(o->flags) + && ((lfsr_bshrub_t*)o)->shrub_.blocks[0] + == mdir_.rbyd.blocks[0]) { + ((lfsr_bshrub_t*)o)->shrub_.blocks[0] = -1; + } + } + compact:; #ifdef LFS_DBGMDIRCOMMITS LFS_DEBUG("Compacting mdir %"PRId32" 0x{%"PRIx32",%"PRIx32"} " @@ -8484,7 +8497,7 @@ compact:; // don't copy over gcksum if relocating lfsr_srid_t start_rid_ = start_rid; - if (relocated && !overcompacted) { + if (relocated && !overrecycled) { start_rid_ = lfs_smax(start_rid_, -1); } @@ -8515,7 +8528,7 @@ compact:; } // consume gcksumdelta if relocated - if (relocated && !overcompacted) { + if (relocated && !overrecycled) { lfs->gcksum_d ^= mdir->gcksumdelta; } // update mdir diff --git a/tests/test_ck.toml b/tests/test_ck.toml index d187f956..5be284cb 100644 --- a/tests/test_ck.toml +++ b/tests/test_ck.toml @@ -1501,7 +1501,6 @@ code = ''' LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL); assert(!err || err == LFS_ERR_CORRUPT); if (err == LFS_ERR_CORRUPT) { - lfsr_file_close(&lfs, &file) => 0; goto corrupt_mounted; } uint32_t prng = 42; @@ -1512,9 +1511,11 @@ code = ''' lfs_ssize_t res = lfsr_file_write(&lfs, &file, wbuf, SIZE); assert(res == SIZE || res == LFS_ERR_CORRUPT); if (res == LFS_ERR_CORRUPT) { + lfsr_file_close(&lfs, &file) => 0; goto corrupt_mounted; } err = lfsr_file_close(&lfs, &file); + assert(!err || err == LFS_ERR_CORRUPT); if (err == LFS_ERR_CORRUPT) { goto corrupt_mounted; } @@ -1623,6 +1624,7 @@ code = ''' goto corrupt_mounted; } int err = lfsr_file_close(&lfs, &file); + assert(!err || err == LFS_ERR_CORRUPT); if (err == LFS_ERR_CORRUPT) { goto corrupt_mounted; } @@ -1733,6 +1735,7 @@ code = ''' goto corrupt_mounted; } int err = lfsr_file_close(&lfs, &file); + assert(!err || err == LFS_ERR_CORRUPT); if (err == LFS_ERR_CORRUPT) { goto corrupt_mounted; } @@ -1762,6 +1765,214 @@ code = ''' } ''' +# overrecycling breaks several expectations around shrub blocks, +# so let's test overrecycling triggered by ckprogs +[cases.test_ck_ckprogs_overrecycling] +# limit to two blocks so we're forced to overrecycle +defines.BLOCK_COUNT = 2 +defines.BADBLOCK = [0, 1] +defines.BADBIT = -1 +defines.BADBLOCK_ = '(BADBLOCK+1) % 2' +defines.BADBIT_ = -1 +defines.M = 4000 +defines.BADBLOCK_BEHAVIOR = 'LFS_EMUBD_BADBLOCK_PROGFLIP' +# this should stay inlined +defines.SIZE = 'BLOCK_SIZE/64' +defines.N = 4 +defines.FLUSH = [false, true] +defines.SYNC = [false, true] +defines.SEED = 42 +ifdef = 'LFS_CKPROGS' +code = ''' + // to avoid taking up too much testing time, assign bits + // pseudorandomly + uint32_t prng = SEED; + for (lfs_size_t i = 0; + i < ((BADBIT == -1) ? M : 1); + i++) { + lfs_size_t badbit = (BADBIT == -1) + ? TEST_PRNG(&prng) % (8*BLOCK_SIZE) + : BADBIT; + lfs_size_t badbit_ = (BADBIT_ == -1) + ? TEST_PRNG(&prng) % (8*BLOCK_SIZE) + : BADBIT_; + printf("--- badblocks: 0x%x.%x + 0x%x.%x, " + "badbits: 0x%x + 0x%x (0x%x+%x + 0x%x+%x) ---\n", + (lfs_off_t)BADBLOCK, badbit/8, + (lfs_off_t)BADBLOCK_, badbit_/8, + badbit, + badbit_, + badbit/8, badbit%8, + badbit_/8, badbit_%8); + + // mark our badbits as bad + lfs_emubd_markbadbit(CFG, BADBLOCK, badbit) => 0; + lfs_emubd_markbadbit(CFG, BADBLOCK_, badbit_) => 0; + + // keep track of open files so we clean up correctly + lfsr_file_t files[N]; + bool open[N]; + memset(open, 0, N*sizeof(bool)); + + // formatting the filesystem may already find the bit error + lfs_t lfs; + int err = lfsr_format(&lfs, LFS_M_RDWR | LFS_M_CKPROGS, CFG); + assert(!err || err == LFS_ERR_CORRUPT); + if (err == LFS_ERR_CORRUPT) { + goto corrupt; + } + lfsr_mount(&lfs, LFS_M_RDWR | LFS_M_CKPROGS, CFG) => 0; + + { + // create some files + // + // don't use global prng here! if we do we lose reproducibility + uint32_t wprng = 42+0; + for (lfs_size_t i = 0; i < N; i++) { + char name[256]; + sprintf(name, "physalia%03d", i); + err = lfsr_file_open(&lfs, &files[i], name, + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL); + assert(!err + || err == LFS_ERR_CORRUPT + || err == LFS_ERR_NOSPC); + if (err == LFS_ERR_CORRUPT || err == LFS_ERR_NOSPC) { + goto corrupt_open; + } + open[i] = true; + + uint8_t wbuf[SIZE]; + for (lfs_size_t j = 0; j < SIZE; j++) { + wbuf[j] = 'a' + (TEST_PRNG(&wprng) % 26); + } + lfs_ssize_t res = lfsr_file_write(&lfs, &files[i], wbuf, SIZE); + assert(res == SIZE + || res == LFS_ERR_CORRUPT + || res == LFS_ERR_NOSPC); + if (res == LFS_ERR_CORRUPT || res == LFS_ERR_NOSPC) { + goto corrupt_open; + } + + // flush? + if (FLUSH) { + err = lfsr_file_flush(&lfs, &files[i]); + assert(!err + || err == LFS_ERR_CORRUPT + || err == LFS_ERR_NOSPC); + if (err == LFS_ERR_CORRUPT || err == LFS_ERR_NOSPC) { + goto corrupt_open; + } + } + + // sync? + if (SYNC) { + err = lfsr_file_sync(&lfs, &files[i]); + assert(!err + || err == LFS_ERR_CORRUPT + || err == LFS_ERR_NOSPC); + if (err == LFS_ERR_CORRUPT || err == LFS_ERR_NOSPC) { + goto corrupt_open; + } + } + } + + // rewrite for good measure + wprng = 42+1; + for (lfs_size_t i = 0; i < N; i++) { + lfsr_file_rewind(&lfs, &files[i]) => 0; + + uint8_t wbuf[SIZE]; + for (lfs_size_t j = 0; j < SIZE; j++) { + wbuf[j] = 'a' + (TEST_PRNG(&wprng) % 26); + } + lfs_ssize_t res = lfsr_file_write(&lfs, &files[i], wbuf, SIZE); + assert(res == SIZE + || res == LFS_ERR_CORRUPT + || res == LFS_ERR_NOSPC); + if (res == LFS_ERR_CORRUPT || res == LFS_ERR_NOSPC) { + goto corrupt_open; + } + + // flush? + if (FLUSH) { + err = lfsr_file_flush(&lfs, &files[i]); + assert(!err + || err == LFS_ERR_CORRUPT + || err == LFS_ERR_NOSPC); + if (err == LFS_ERR_CORRUPT || err == LFS_ERR_NOSPC) { + goto corrupt_open; + } + } + + // sync? + if (SYNC) { + err = lfsr_file_sync(&lfs, &files[i]); + assert(!err + || err == LFS_ERR_CORRUPT + || err == LFS_ERR_NOSPC); + if (err == LFS_ERR_CORRUPT || err == LFS_ERR_NOSPC) { + goto corrupt_open; + } + } + } + + // and close + for (lfs_size_t i = 0; i < N; i++) { + err = lfsr_file_close(&lfs, &files[i]); + assert(!err + || err == LFS_ERR_CORRUPT + || err == LFS_ERR_NOSPC); + open[i] = false; + if (err == LFS_ERR_CORRUPT || err == LFS_ERR_NOSPC) { + goto corrupt_open; + } + } + + // if we made it here without erroring we should be able to + // read our files + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfsr_unmount(&lfs) => 0; + lfsr_mount(&lfs, LFS_M_RDWR | LFS_M_CKPROGS, CFG) => 0; + } + + wprng = 42+1; + for (lfs_size_t i = 0; i < N; i++) { + char name[256]; + sprintf(name, "physalia%03d", i); + lfsr_file_open(&lfs, &files[i], name, LFS_O_RDONLY) => 0; + uint8_t wbuf[SIZE]; + for (lfs_size_t j = 0; j < SIZE; j++) { + wbuf[j] = 'a' + (TEST_PRNG(&wprng) % 26); + } + uint8_t rbuf[SIZE]; + lfsr_file_read(&lfs, &files[i], rbuf, SIZE) => SIZE; + assert(memcmp(rbuf, wbuf, SIZE) == 0); + lfsr_file_close(&lfs, &files[i]) => 0; + } + } + } + + corrupt_open:; + for (lfs_size_t i = 0; i < N; i++) { + if (open[i]) { + lfsr_file_desync(&lfs, &files[i]) => 0; + lfsr_file_close(&lfs, &files[i]) => 0; + open[i] = false; + } + } + + corrupt_mounted:; + lfsr_unmount(&lfs) => 0; + + corrupt:; + // reset badbits + lfs_emubd_markgood(CFG, BADBLOCK) => 0; + lfs_emubd_markgood(CFG, BADBLOCK_) => 0; + } +''' + # Some simple ckfetches tests @@ -3757,7 +3968,7 @@ code = ''' goto corrupt_mounted; } int err = lfsr_file_sync(&lfs, &sim_files[j]->file); - assert(err == 0 + assert(!err || err == LFS_ERR_NOSPC || (err == LFS_ERR_CORRUPT && (METHOD == 2 || METHOD == 3))); @@ -4359,7 +4570,7 @@ code = ''' goto corrupt_mounted; } int err = lfsr_file_sync(&lfs, &sim_files[j]->file); - assert(err == 0 + assert(!err || err == LFS_ERR_NOSPC || (err == LFS_ERR_CORRUPT && (METHOD == 2 || METHOD == 3)));