From ffe8c1e82089075f63a8dfc60eff359148d4933e Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 8 Jul 2024 13:12:29 -0500 Subject: [PATCH] t: Reverted most of LFS_T_MKCONSISTENT, just check for new grms/orphans Checking for orphans + other traversal work turned out to mesh much worse than originally thought: - Adjusting mids and being able to drop mdirs mid-traversal complicates traversal quite a bit and has potential to hide difficult to reproduce bugs. - Implementing incremental mkconsistent requires it's own separate state to detect mutation correctly since LFS_T_MKCONSISTENT and LFS_T_LOOKAHEAD are invalidated by slightly different things. - If hasorphans=true, we're likely going to find orphans and clobber the traversal. So it's not really worth trying to opportunistically prove there are no orphans while doing other traversal operations. - We don't really want to traverse the mroot/mtree during mkconsistent, which makes deduplicating these two functions a bit tricky. Doable, but annoying. - grms don't involve traversals and are their own separate awkward step already. Combine this with the fact that needing to scan for orphans should be relatively rare in practice -- requiring either a powerloss or a complicated set of file operations with at minimum 3 desynced files -- and parallel orphan checking starts to look like more trouble than it's worth... Instead, we now only check if the hasorphan bit has been set, and if it has been we just call lfsr_fs_mkconsistent directly. This does a full traversal in a single step, but at least makes it so traversal + LFS_T_MKCONSISTENT in a background thread will do any necessary janitorial work. This saves a bit code: code stack before: 35480 2680 after: 35232 (-0.7%) 2680 (+0.0%) --- lfs.c | 173 ++++++++++---------------------------- lfs.h | 3 +- tests/test_traversal.toml | 14 +-- 3 files changed, 50 insertions(+), 140 deletions(-) diff --git a/lfs.c b/lfs.c index a1af9d15..3ac52d03 100644 --- a/lfs.c +++ b/lfs.c @@ -8265,10 +8265,6 @@ static inline bool lfsr_f_isdirty(uint32_t flags) { return flags & LFS_F_DIRTY; } -static inline bool lfsr_f_ismutated(uint32_t flags) { - return flags & LFS_F_MUTATED; -} - // needed in lfsr_mtree_traverse_ @@ -8652,7 +8648,6 @@ static int lfsr_mtree_gc(lfs_t *lfs, lfsr_traversal_t *t, // lfsr_mtree_gc to work correctly LFS_ASSERT(lfsr_omdir_isopen(lfs, &t->o.o)); -again:; lfsr_tag_t tag; lfsr_bptr_t bptr; int err = lfsr_mtree_traverse(lfs, t, @@ -8661,32 +8656,6 @@ again:; return err; } - // keep track of dirty flag before mutation - bool dirty = t->o.o.flags & LFS_F_DIRTY; - - // mkconsistencing mdirs? - if (lfsr_t_ismkconsistent(t->o.o.flags) - && tag == LFSR_TAG_MDIR - && lfs->hasorphans) { - lfsr_mdir_t *mdir = (lfsr_mdir_t*)bptr.data.u.buffer; - int err = lfsr_mdir_fixorphans(lfs, mdir); - if (err) { - return err; - } - - // did this drop our mdir? - if (mdir->mid != -1 && mdir->rbyd.weight == 0) { - t->o.o.flags &= ~LFS_F_ZOMBIE; - t->o.o.state = LFSR_TSTATE_MDIRS; - - // downgrade any new dirty flags - t->o.o.flags |= (t->o.o.flags & LFS_F_DIRTY) ? LFS_F_MUTATED : 0; - t->o.o.flags &= ~LFS_F_DIRTY; - t->o.o.flags |= (dirty) ? LFS_F_DIRTY : 0; - goto again; - } - } - // compacting mdirs? if (lfsr_t_iscompact(t->o.o.flags) && tag == LFSR_TAG_MDIR @@ -8788,11 +8757,6 @@ again:; t->u.bt.rid = t->u.bt.bid; } - // downgrade any new dirty flags - t->o.o.flags |= (t->o.o.flags & LFS_F_DIRTY) ? LFS_F_MUTATED : 0; - t->o.o.flags &= ~LFS_F_DIRTY; - t->o.o.flags |= (dirty) ? LFS_F_DIRTY : 0; - if (tag_) { *tag_ = tag; } @@ -12700,77 +12664,54 @@ static int lfsr_fs_fixgrm(lfs_t *lfs) { return 0; } -static int lfsr_mdir_fixorphans(lfs_t *lfs, lfsr_mdir_t *mdir) { - // save the current mid - lfsr_mid_t mid = mdir->mid; - - // iterate through mids looking for orphans - mdir->mid = LFSR_MID(lfs, mdir->mid, 0); - int err; - while (lfsr_mid_rid(lfs, mdir->mid) < (lfsr_srid_t)mdir->rbyd.weight) { - // is this mid open? well we're not an orphan then, skip - if (lfsr_omdir_ismidopen(lfs, mdir->mid)) { - mdir->mid += 1; - continue; - } - - // is this mid marked as an orphan? - err = lfsr_mdir_lookup(lfs, mdir, LFSR_TAG_ORPHAN, - NULL); - if (err) { - if (err == LFS_ERR_NOENT) { - mdir->mid += 1; - continue; - } - goto failed; - } - - // we found an orphaned file, remove - LFS_DEBUG("Fixing orphan %"PRId32".%"PRId32, - lfsr_mid_bid(lfs, mdir->mid) >> lfs->mdir_bits, - lfsr_mid_rid(lfs, mdir->mid)); - - err = lfsr_mdir_commit(lfs, mdir, LFSR_ATTRS( - LFSR_ATTR(LFSR_TAG_RM, -1, LFSR_DATA_NULL()))); - if (err) { - goto failed; - } - } - - // restore the current mid - mdir->mid = mid; - return 0; - -failed:; - // restore the current mid - mdir->mid = mid; - return err; -} - static int lfsr_fs_fixorphans(lfs_t *lfs) { // iterate through the filesystem and remove any orphaned files // // note this never takes longer than lfsr_mount // - lfsr_mid_t mid = 0; - while (mid < lfsr_mtree_weight(lfs)) { - lfsr_mdir_t mdir; - int err = lfsr_mtree_lookup(lfs, mid, + lfsr_mdir_t mdir = {.mid=0}; + while (mdir.mid < (lfsr_srid_t)lfsr_mtree_weight(lfs)) { + int err = lfsr_mtree_lookup(lfs, LFSR_MID(lfs, mdir.mid, 0), &mdir); if (err) { LFS_ASSERT(err != LFS_ERR_NOENT); return err; } - // clean up orphans - err = lfsr_mdir_fixorphans(lfs, &mdir); - if (err) { - return err; + while (lfsr_mid_rid(lfs, mdir.mid) + < (lfsr_srid_t)mdir.rbyd.weight) { + // is this mid open? well we're not an orphan then, skip + if (lfsr_omdir_ismidopen(lfs, mdir.mid)) { + mdir.mid += 1; + continue; + } + + // is this mid marked as an orphan? + err = lfsr_mdir_lookup(lfs, &mdir, LFSR_TAG_ORPHAN, + NULL); + if (err) { + if (err == LFS_ERR_NOENT) { + mdir.mid += 1; + continue; + } + return err; + } + + // we found an orphaned file, remove + LFS_DEBUG("Fixing orphan %"PRId32".%"PRId32, + lfsr_mid_bid(lfs, mdir.mid) >> lfs->mdir_bits, + lfsr_mid_rid(lfs, mdir.mid)); + + err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS( + LFSR_ATTR(LFSR_TAG_RM, -1, LFSR_DATA_NULL()))); + if (err) { + return err; + } } - // incremend mid unless we dropped the mdir + // increment mid unless we dropped the mdir if (mdir.rbyd.weight > 0) { - mid += 1 << lfs->mdir_bits; + mdir.mid = lfsr_mid_bid(lfs, mdir.mid) + 1; } } @@ -12915,34 +12856,14 @@ int lfsr_traversal_read(lfs_t *lfs, lfsr_traversal_t *t, struct lfs_tinfo *tinfo) { LFS_ASSERT(lfsr_omdir_isopen(lfs, &t->o.o)); - // check for pending grms every step, just in case some other - // operation introduced new grms - if (lfsr_t_ismkconsistent(t->o.o.flags) - && lfsr_grm_count(lfs) > 0) { - if (lfsr_grm_count(lfs) == 2) { - LFS_DEBUG("Fixing grm %"PRId32".%"PRId32" %"PRId32".%"PRId32, - lfsr_mid_bid(lfs, lfs->grm.mids[0]) >> lfs->mdir_bits, - lfsr_mid_rid(lfs, lfs->grm.mids[0]), - lfsr_mid_bid(lfs, lfs->grm.mids[1]) >> lfs->mdir_bits, - lfsr_mid_rid(lfs, lfs->grm.mids[1])); - } else if (lfsr_grm_count(lfs) == 1) { - LFS_DEBUG("Fixing grm %"PRId32".%"PRId32, - lfsr_mid_bid(lfs, lfs->grm.mids[0]) >> lfs->mdir_bits, - lfsr_mid_rid(lfs, lfs->grm.mids[0])); - } - - // keep track of dirty flag before mutation - bool dirty = t->o.o.flags & LFS_F_DIRTY; - - int err = lfsr_fs_fixgrm(lfs); + // mkconsistencing? + // + // check every step in case some other operation introduced a grm/orphan + if (lfsr_t_ismkconsistent(t->o.o.flags)) { + int err = lfsr_fs_mkconsistent(lfs); if (err) { return err; } - - // downgrade any new dirty flags - t->o.o.flags |= (t->o.o.flags & LFS_F_DIRTY) ? LFS_F_MUTATED : 0; - t->o.o.flags &= ~LFS_F_DIRTY; - t->o.o.flags |= (dirty) ? LFS_F_DIRTY : 0; } while (true) { @@ -12994,16 +12915,9 @@ int lfsr_traversal_read(lfs_t *lfs, lfsr_traversal_t *t, } done:; - // was mkconsistent successful? - if (lfsr_t_ismkconsistent(t->o.o.flags) - && !lfsr_f_isdirty(t->o.o.flags)) { - lfs->hasorphans = false; - } - // was a lookahead scan successful? if (lfsr_t_islookahead(t->o.o.flags) - && !lfsr_f_isdirty(t->o.o.flags) - && !lfsr_f_ismutated(t->o.o.flags)) { + && !lfsr_f_isdirty(t->o.o.flags)) { lfs_alloc_markfree(lfs); } @@ -13012,8 +12926,11 @@ done:; static void lfsr_traversal_clobber(lfs_t *lfs, lfsr_traversal_t *t) { (void)lfs; + // not yet started? + if (t->o.o.state < LFSR_TSTATE_MROOTCHAIN) { + // do nothing // mroot/mtree? transition to mdir iteration - if (t->o.o.state < LFSR_TSTATE_MDIRS) { + } else if (t->o.o.state < LFSR_TSTATE_MDIRS) { t->o.o.state = LFSR_TSTATE_MDIRS; t->o.o.mdir.mid = 0; t->o.bshrub.u.bshrub.weight = 0; @@ -13046,7 +12963,7 @@ static void lfsr_traversal_clobber(lfs_t *lfs, lfsr_traversal_t *t) { static int lfsr_traversal_rewind_(lfs_t *lfs, lfsr_traversal_t *t) { (void)lfs; // reset traversal - t->o.o.flags &= ~LFS_F_DIRTY & ~LFS_F_MUTATED; + t->o.o.flags &= ~LFS_F_DIRTY; t->o.o.state = LFSR_TSTATE_MROOTANCHOR; t->o.o.mdir.mid = -1; t->o.o.mdir.rbyd.weight = 0; diff --git a/lfs.h b/lfs.h index ff0eab9e..7a75496d 100644 --- a/lfs.h +++ b/lfs.h @@ -179,8 +179,7 @@ enum lfs_traversal_flags { // LFS_T_REPAIRDATA = 0x0c00, // Repair metadata + data blocks // internally used flags - LFS_F_DIRTY = 0x1000, // Filesystem modified during traversal - LFS_F_MUTATED = 0x4000, // Filesystem modified by traversal + LFS_F_DIRTY = 0x1000, // Filesystem has been modified }; diff --git a/tests/test_traversal.toml b/tests/test_traversal.toml index e2390f80..5199871d 100644 --- a/tests/test_traversal.toml +++ b/tests/test_traversal.toml @@ -6964,13 +6964,10 @@ code = ''' lfsr_traversal_read(&lfs, &t, &tinfo) => LFS_ERR_NOENT; lfsr_traversal_close(&lfs, &t) => 0; - // we should be able to clean up grms + // we should be able to clean up grms and orphans assert(lfs.grm.mids[0] == -1); assert(lfs.grm.mids[1] == -1); - // if we introduce actual orphans, me _must not_ clear the orphan flag - if (ORPHANS > 3) { - assert(lfs.hasorphans == true); - } + assert(lfs.hasorphans == false); // check we can still read the files for (int remount = 0; remount < 2; remount++) { @@ -7839,13 +7836,10 @@ code = ''' lfsr_traversal_read(&lfs, &t, &tinfo) => LFS_ERR_NOENT; lfsr_traversal_close(&lfs, &t) => 0; - // we should be able to clean up grms + // we should be able to clean up grms and orphans assert(lfs.grm.mids[0] == -1); assert(lfs.grm.mids[1] == -1); - // if we introduce actual orphans, me _must not_ clear the orphan flag - if (ORPHANS > 3) { - assert(lfs.hasorphans == true); - } + assert(lfs.hasorphans == false); // mdirs should have been compacted assert((file1.o.o.mdir.rbyd.eoff & 0x7fffffff) <= GC_COMPACT_THRESH);