From 15090e5dcfab9ccc94ae693ae4cc7fbb3c8bd284 Mon Sep 17 00:00:00 2001
From: Christopher Haster <geky@geky.net>
Date: Fri, 19 Jul 2024 02:08:54 -0500
Subject: [PATCH] gc: Also restart gc if lookahead + mutated/dirty

There is really no reason to continue lookahead traversals if our
filesystem has been mutated. Clearing the flag and restarting in this
case is more likely to make progress.

Note that it's worth continuing for all of the other current gc flags:

- LFS_GC_MKCONSISTENT - Except maybe for mkconsistent. We can't actually
  make progress, since we can't prove the filesystem is free of orphans,
  but it's beneficial to keep traversing and clearing orphans in case of
  other traversal flags that mutation would force a second traversal
  anyways.

  Continuing mkconsistent traversals also spreads out orphan cleanup a
  bit better, instead of just repeatedly cleaning up the first couple
  mdirs when under heavy contention.

  But to be honest, the chance of mutation that still leaves the
  filesystem with orphans is just so low that it's not worth doing
  anything. mkconsistent only needs to traverse the mtree anyways...

- LFS_GC_COMPACT - Like mkconsistent, compacting traversals are worth
  continuing for better mtree coverage under heavy contention.

  We will need a second pass to prove we compacted everything anyways,
  so might as well try to get as much mutation done as possible in the
  current traversal.

- LFS_GC_CKMETA/CKDATA - Continuing ckmeta/ckdata traversals provides
  better mtree coverage under heavy contention.

  This is much more important for CKMETA/CKDATA than the others, because
  _eventually_ checking every block for errors is more valuable than
  proving anything.

This adds some code, but the use of flags here is quite valuable for
expressing complex constraints like this cheaply:

           code          stack
  before: 36228           2680
  after:  36240 (+0.0%)   2680 (+0.0%)
---
 lfs.c              | 38 +++++++++++++++++++++-----------------
 tests/test_gc.toml |  6 ++++++
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/lfs.c b/lfs.c
index 70cf576a..6248cbb3 100644
--- a/lfs.c
+++ b/lfs.c
@@ -13069,29 +13069,33 @@ int lfsr_fs_gc(lfs_t *lfs, lfs_soff_t steps, uint32_t flags) {
         if (!lfsr_omdir_isopen(lfs, &lfs->gc.o.o)) {
             lfs->gc = LFSR_TRAVERSAL(pending);
             lfsr_omdir_open(lfs, &lfs->gc.o.o);
+        }
 
-        // existing traversal?
-        } else {
-            // mask flags, we can't trust existing traversals to make
-            // progress if flags change
-            lfs->gc.o.o.flags &= ~(
+        // mask flags, we can't trust existing traversals to make
+        // progress if flags change
+        lfs->gc.o.o.flags &= (
+                pending | ~(
                     LFS_GC_MKCONSISTENT
                         | LFS_GC_LOOKAHEAD
                         | LFS_GC_COMPACT
                         | LFS_GC_CKMETA
-                        | LFS_GC_CKDATA
-                    ) | pending;
+                        | LFS_GC_CKDATA));
 
-            // will this traversal still make progress? no? start over
-            if (!(lfs->gc.o.o.flags & (
-                    LFS_GC_MKCONSISTENT
-                        | LFS_GC_LOOKAHEAD
-                        | LFS_GC_COMPACT
-                        | LFS_GC_CKMETA
-                        | LFS_GC_CKDATA))) {
-                lfsr_omdir_close(lfs, &lfs->gc.o.o);
-                continue;
-            }
+        // don't bother with lookahead if we've mutated
+        if (lfsr_f_isdirty(lfs->gc.o.o.flags)
+                || lfsr_f_ismutated(lfs->gc.o.o.flags)) {
+            lfs->gc.o.o.flags &= ~LFS_GC_LOOKAHEAD;
+        }
+
+        // will this traversal still make progress? no? start over
+        if (!(lfs->gc.o.o.flags & (
+                LFS_GC_MKCONSISTENT
+                    | LFS_GC_LOOKAHEAD
+                    | LFS_GC_COMPACT
+                    | LFS_GC_CKMETA
+                    | LFS_GC_CKDATA))) {
+            lfsr_omdir_close(lfs, &lfs->gc.o.o);
+            continue;
         }
 
         // do we really need a full traversal?
diff --git a/tests/test_gc.toml b/tests/test_gc.toml
index 8e6270bf..391131c1 100644
--- a/tests/test_gc.toml
+++ b/tests/test_gc.toml
@@ -80,6 +80,8 @@ defines.SIZE = [
     '2*BLOCK_SIZE',
     '8*BLOCK_SIZE',
 ]
+# we need something to keep the traversal running
+if = 'CKMETA || CKDATA'
 code = '''
     lfs_t lfs;
     lfsr_format(&lfs, CFG) => 0;
@@ -378,6 +380,8 @@ defines.SIZE = [
 ]
 # set compact thresh to minimum
 defines.GC_COMPACT_THRESH = 'BLOCK_SIZE/2'
+# we need something to keep the traversal running
+if = 'CKMETA || CKDATA'
 code = '''
     lfs_t lfs;
     lfsr_format(&lfs, CFG) => 0;
@@ -878,6 +882,8 @@ defines.SIZE = 'FILE_BUFFER_SIZE/2'
 # <=2 => grm-able
 #  >2 => requires orphans
 defines.ORPHANS = [3, 100]
+# we need something to keep the traversal running
+if = 'CKMETA || CKDATA'
 code = '''
     lfs_t lfs;
     lfsr_format(&lfs, CFG) => 0;