From 46488ebc7fb605d4414062ec8af1adcaa9564463 Mon Sep 17 00:00:00 2001
From: Christopher Haster <geky@geky.net>
Date: Fri, 19 Jul 2024 14:26:32 -0500
Subject: [PATCH] t: Moved lookahead population into lfsr_mtree_gc/lfs_alloc

It was a bit weird to have this in lfsr_mtree_traverse, which doesn't
change any filesystem state otherwise.

At the very least we should call lfs_alloc_markinuse and
lfs_alloc_markfree in the same function, and rerouting
lfsr_mtree_traverse eot to handle this would have added code cost
anyways.

The main cost is stack:

           code          stack
  before: 36256           2680
  after:  36288 (+0.1%)   2704 (+0.9%)

Unfortunately this reveals one of the bigger issues with our optional
return parameters: if a function with optional return parameters needs
the structs to perform work, in this case lfsr_mtree_traverse needs
lfsr_bptr_t in case ckmeta/ckdata is requested, it requires an
additional stack allocation.

In theory, these stack allocations could be elided if the return structs
are provided, but you can't really express this in standard C.

Combine this with the fact that lfs_alloc is sensitive to stack changes,
and lives at the bottom at every hot-path, and the end result is more
stack usage.

Note that the additional stack cost, 24 bytes, is exactly equal to one
tag + one bptr, 4 bytes + 20 bytes.
---
 lfs.c | 60 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/lfs.c b/lfs.c
index 2bc89056..2a3103a0 100644
--- a/lfs.c
+++ b/lfs.c
@@ -8622,11 +8622,12 @@ static int lfsr_mtree_traverse_(lfs_t *lfs, lfsr_traversal_t *t,
 }
 
 // needed in lfsr_mtree_traverse
-static void lfs_alloc_markinuse(lfs_t *lfs, lfs_block_t block);
+static void lfs_alloc_markinuse(lfs_t *lfs,
+        lfsr_tag_t tag, const lfsr_bptr_t *bptr);
 
 // high-level immutable traversal, handle extra features here,
 // but no mutation! (we're called in lfs_alloc, so things would end up
-// recursive)
+// recursive, which would be a bit bad!)
 static int lfsr_mtree_traverse(lfs_t *lfs, lfsr_traversal_t *t,
         lfsr_tag_t *tag_, lfsr_bptr_t *bptr_) {
     lfsr_tag_t tag;
@@ -8661,25 +8662,6 @@ static int lfsr_mtree_traverse(lfs_t *lfs, lfsr_traversal_t *t,
         }
     }
 
-    // track in-use blocks
-    if (lfsr_t_islookahead(t->o.o.flags)) {
-        if (tag == LFSR_TAG_MDIR) {
-            lfsr_mdir_t *mdir = (lfsr_mdir_t*)bptr.data.u.buffer;
-            lfs_alloc_markinuse(lfs, mdir->rbyd.blocks[0]);
-            lfs_alloc_markinuse(lfs, mdir->rbyd.blocks[1]);
-
-        } else if (tag == LFSR_TAG_BRANCH) {
-            lfsr_rbyd_t *rbyd = (lfsr_rbyd_t*)bptr.data.u.buffer;
-            lfs_alloc_markinuse(lfs, rbyd->blocks[0]);
-
-        } else if (tag == LFSR_TAG_BLOCK) {
-            lfs_alloc_markinuse(lfs, bptr.data.u.disk.block);
-
-        } else {
-            LFS_UNREACHABLE();
-        }
-    }
-
     if (tag_) {
         *tag_ = tag;
     }
@@ -8734,6 +8716,11 @@ dropped:;
         }
     }
 
+    // track in-use blocks?
+    if (lfsr_t_islookahead(t->o.o.flags)) {
+        lfs_alloc_markinuse(lfs, tag, &bptr);
+    }
+
     // compacting mdirs?
     if (lfsr_t_iscompact(t->o.o.flags)
             && tag == LFSR_TAG_MDIR
@@ -8913,7 +8900,7 @@ static void lfs_alloc_discard(lfs_t *lfs) {
 }
 
 // mark a block as in-use
-static void lfs_alloc_markinuse(lfs_t *lfs, lfs_block_t block) {
+static void lfs_alloc_markinuse_(lfs_t *lfs, lfs_block_t block) {
     // translate to lookahead-relative
     lfs_block_t block_ = ((
                 (lfs_sblock_t)(block
@@ -8933,6 +8920,26 @@ static void lfs_alloc_markinuse(lfs_t *lfs, lfs_block_t block) {
     }
 }
 
+// mark some filesystem object as in-use
+static void lfs_alloc_markinuse(lfs_t *lfs,
+        lfsr_tag_t tag, const lfsr_bptr_t *bptr) {
+    if (tag == LFSR_TAG_MDIR) {
+        lfsr_mdir_t *mdir = (lfsr_mdir_t*)bptr->data.u.buffer;
+        lfs_alloc_markinuse_(lfs, mdir->rbyd.blocks[0]);
+        lfs_alloc_markinuse_(lfs, mdir->rbyd.blocks[1]);
+
+    } else if (tag == LFSR_TAG_BRANCH) {
+        lfsr_rbyd_t *rbyd = (lfsr_rbyd_t*)bptr->data.u.buffer;
+        lfs_alloc_markinuse_(lfs, rbyd->blocks[0]);
+
+    } else if (tag == LFSR_TAG_BLOCK) {
+        lfs_alloc_markinuse_(lfs, bptr->data.u.disk.block);
+
+    } else {
+        LFS_UNREACHABLE();
+    }
+}
+
 // needed in lfs_alloc_markfree
 static lfs_sblock_t lfs_alloc_findfree(lfs_t *lfs);
 
@@ -9036,18 +9043,23 @@ static lfs_sblock_t lfs_alloc(lfs_t *lfs, bool erase) {
         // no blocks in our lookahead buffer?
         //
         // traverse the filesystem, building up knowledge of what blocks are
-        // in use in the next lookahead window
+        // in-use in the next lookahead window
         //
         lfsr_traversal_t t = LFSR_TRAVERSAL(LFS_T_LOOKAHEAD);
         while (true) {
+            lfsr_tag_t tag;
+            lfsr_bptr_t bptr;
             int err = lfsr_mtree_traverse(lfs, &t,
-                    NULL, NULL);
+                    &tag, &bptr);
             if (err) {
                 if (err == LFS_ERR_NOENT) {
                     break;
                 }
                 return err;
             }
+
+            // track in-use blocks
+            lfs_alloc_markinuse(lfs, tag, &bptr);
         }
 
         // mark anything not seen as free