From 9e4bbdf0ad20d3c57e86989601defff2a438595d Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 19 Oct 2025 13:33:04 -0500 Subject: [PATCH] trv: Added test_gc_nospc, fixed pcache bug and trv-repop-conflict bug This adds test_gc_nospc with more aggressive testing of gc/traversal operations in low-space conditions. The original intention was to test the new soft-ENOSPC traversal behavior, but instead it found a couple unrelated bugs. In my defense these involve some rather subtle filesystem interactions and went unnoticed because we don't usually check data checksums: 1. lfs3_bd_flush had a rare chance where it could corrupt our prog-aligned pcksum when (1) we bypass the pcache, allowing any previous contents to stay there until flush/pcksum, and (2) some other failed prog, in this case failing repopgbmaps due to the low-space condition, leaves garbage in the pcache. When we flush we corrupt the pcksum even though the old data belongs to an unrelated block. This resulted in CKDATA failing, though the failed check is a false positive. As a workaround, lfs3_bd_prog and lfs3_bd_prognext now discard _any_ unrelated pcache, even if bypassing the pcache. This should ensure consistent behavior in all cases. Note we do something similar for with the file cache in lfs3_file_write. This means progs may not complete unless lfs3_bd_flush is called, but I think we need to call lfs3_bd_flush in all cases anyways to ensure power-loss safe behavior. The end result should be a more reliable internal bd prog API. 2. On a successful traversal with LFS3_T_REPOPLOOKAHEAD and LFS3_T_REPOPGBMAP we adopt both the new gbmap and lookahead buffer. This is wrong! The lookahead buffer is not aware of the gbmap during the traversal, and _can't_ be aware as the gbmap changes during repopulation work. This is the whole reason we have the alloc ckpoints and the in-flight window. To fix, adopting the lookahead buffer is now conditional on _not_ adopting a new gbmap. It makes the code a bit more messy, but this is the correct behavior. Populating both the gbmap and lookahead buffere requires at least two passes. Code changes minimal: code stack ctx before: 37248 2352 688 after: 37260 (+0.0%) 2352 (+0.0%) 688 (+0.0%) code stack ctx gbmap before: 40204 2368 856 gbmap after: 40220 (+0.0%) 2368 (+0.0%) 856 (+0.0%) --- lfs3.c | 55 +++++++++++++++--------- tests/test_gc.toml | 102 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 19 deletions(-) diff --git a/lfs3.c b/lfs3.c index cc30d38c..cb77761b 100644 --- a/lfs3.c +++ b/lfs3.c @@ -410,8 +410,14 @@ static int lfs3_bd_prognext(lfs3_t *lfs3, lfs3_block_t block, lfs3_size_t off, while (true) { // active pcache? - if (lfs3->pcache.block == block - && lfs3->pcache.size != 0) { + if (lfs3->pcache.size != 0) { + // wait, wrong block? this must be a leftover pcache due to + // an error, discard + if (lfs3->pcache.block != block) { + lfs3_bd_droppcache(lfs3); + continue; + } + // fits in pcache? if (off < lfs3->pcache.off + lfs3->cfg->pcache_size) { // you can't prog backwards silly @@ -465,8 +471,14 @@ static int lfs3_bd_prog(lfs3_t *lfs3, lfs3_block_t block, lfs3_size_t off, lfs3_size_t size_ = size; while (size_ > 0) { // active pcache? - if (lfs3->pcache.block == block - && lfs3->pcache.size != 0) { + if (lfs3->pcache.size != 0) { + // wait, wrong block? this must be a leftover pcache due to + // an error, discard + if (lfs3->pcache.block != block) { + lfs3_bd_droppcache(lfs3); + continue; + } + // fits in pcache? if (off_ < lfs3->pcache.off + lfs3->cfg->pcache_size) { // you can't prog backwards silly @@ -10817,26 +10829,31 @@ dropped:; eot:; #ifndef LFS3_RDONLY + // was gbmap scan successful? + // + // this is structured this way because only one repopulation + // scan can succeed at a time, if gbmap succeeds it invalidates the + // lookahead scan with the new gbmap + // + // gbmap takes priority because it actually writes to disk + if (LFS3_IFDEF_GBMAP( + lfs3_t_isrepopgbmap(mgc->t.b.h.flags) + && lfs3_f_isgbmap(lfs3->flags) + && lfs3_t_isrepopgbmap(lfs3->flags) + && !lfs3_t_ismtreeonly(mgc->t.b.h.flags) + && !lfs3_t_isckpointed(mgc->t.b.h.flags) + && !lfs3_t_isnospc(mgc->t.b.h.flags), + false)) { + #ifdef LFS3_GBMAP + lfs3_alloc_adoptgbmap(lfs3, &mgc->gbmap_, lfs3->lookahead.ckpoint); + #endif + // was lookahead scan successful? - #ifndef LFS3_2BONLY - if (lfs3_t_isrepoplookahead(mgc->t.b.h.flags) + } else if (lfs3_t_isrepoplookahead(mgc->t.b.h.flags) && !lfs3_t_ismtreeonly(mgc->t.b.h.flags) && !lfs3_t_isckpointed(mgc->t.b.h.flags)) { lfs3_alloc_adopt(lfs3, lfs3->lookahead.ckpoint); } - #endif - - // was gbmap repop successful? - #ifdef LFS3_GBMAP - if (lfs3_t_isrepopgbmap(mgc->t.b.h.flags) - && lfs3_f_isgbmap(lfs3->flags) - && lfs3_t_isrepopgbmap(lfs3->flags) - && !lfs3_t_ismtreeonly(mgc->t.b.h.flags) - && !lfs3_t_isckpointed(mgc->t.b.h.flags) - && !lfs3_t_isnospc(mgc->t.b.h.flags)) { - lfs3_alloc_adoptgbmap(lfs3, &mgc->gbmap_, lfs3->lookahead.ckpoint); - } - #endif // was mkconsistent successful? if (lfs3_t_ismkconsistent(mgc->t.b.h.flags) diff --git a/tests/test_gc.toml b/tests/test_gc.toml index 7d348a46..635ef350 100644 --- a/tests/test_gc.toml +++ b/tests/test_gc.toml @@ -2598,6 +2598,108 @@ code = ''' lfs3_unmount(&lfs3) => 0; ''' +# test that gc work doesn't break anything in low-space condiditions +[cases.test_gc_nospc] +defines.MKCONSISTENT = [false, true] +defines.REPOPLOOKAHEAD = [false, true] +defines.REPOPGBMAP = [false, true] +defines.COMPACTMETA = [false, true] +defines.CKMETA = [false, true] +defines.CKDATA = [false, true] +defines.GC_FLAGS = ''' + ((MKCONSISTENT) ? LFS3_GC_MKCONSISTENT : 0) + | ((REPOPLOOKAHEAD) ? LFS3_GC_REPOPLOOKAHEAD : 0) + | ((REPOPGBMAP) ? LFS3_IFDEF_GBMAP(LFS3_GC_REPOPGBMAP, -1) : 0) + | ((COMPACTMETA) ? LFS3_GC_COMPACTMETA : 0) + | ((CKMETA) ? LFS3_GC_CKMETA : 0) + | ((CKDATA) ? LFS3_GC_CKDATA : 0) +''' +# DON'T test with GC_STEPS=-1, it may never terminate! +defines.GC_STEPS = [1, 2, 10, 100, 1000] +# set compactmeta thresh to minimum +defines.GC_COMPACTMETA_THRESH = 'BLOCK_SIZE/2' +defines.SIZE = 'BLOCK_SIZE' +if = 'GBMAP || !REPOPGBMAP' +ifdef = 'LFS3_GC' +code = ''' + lfs3_t lfs3; + lfs3_format(&lfs3, + LFS3_F_RDWR + | ((GBMAP) ? LFS3_IFDEF_GBMAP(LFS3_F_GBMAP, -1) : 0), + CFG) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + + uint32_t prng = 42; + + for (uint32_t i = 0;; i++) { + // create a new file every gc cycle + lfs3_file_t file; + char name[256]; + sprintf(name, "purseweb%03x", i); + int err = lfs3_file_open(&lfs3, &file, name, + LFS3_O_WRONLY | LFS3_O_CREAT | LFS3_O_EXCL); + assert(!err || err == LFS3_ERR_NOSPC); + if (err == LFS3_ERR_NOSPC) { + break; + } + uint8_t wbuf[SIZE]; + for (lfs3_size_t j = 0; j < SIZE; j++) { + wbuf[j] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_ssize_t d = lfs3_file_write(&lfs3, &file, wbuf, SIZE); + assert(d == SIZE || d == LFS3_ERR_NOSPC); + if (d == LFS3_ERR_NOSPC) { + lfs3_file_close(&lfs3, &file) => 0; + break; + } + err = lfs3_file_close(&lfs3, &file); + assert(!err || err == LFS3_ERR_NOSPC); + if (err == LFS3_ERR_NOSPC) { + break; + } + + // gc! + // + // gc should not error, but may be unable to make progress + lfs3_fs_gc(&lfs3) => 0; + } + + // check the contents of the files that were written + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfs3_unmount(&lfs3) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + } + + // reset prng + uint32_t prng = 42; + + // try to read + for (uint32_t i = 0;; i++) { + lfs3_file_t file; + char name[256]; + sprintf(name, "purseweb%03x", i); + int err = lfs3_file_open(&lfs3, &file, name, LFS3_O_RDONLY); + assert(!err || err == LFS3_ERR_NOENT); + if (err == LFS3_ERR_NOENT) { + break; + } + + uint8_t wbuf[SIZE]; + for (lfs3_size_t j = 0; j < SIZE; j++) { + wbuf[j] = 'a' + (TEST_PRNG(&prng) % 26); + } + uint8_t rbuf[SIZE]; + lfs3_file_read(&lfs3, &file, rbuf, SIZE) => SIZE; + assert(memcmp(rbuf, wbuf, SIZE) == 0); + lfs3_file_close(&lfs3, &file) => 0; + } + } + + lfs3_unmount(&lfs3) => 0; +''' + # many/fuzz tests mixed with GC