Fixed lfsr_fs_fixorphans not fixing orphans

Turns out things get a bit tricky when mdirs are dropped while iterating
over the mtree.

This was actually broken quite a bit before traversal-related changes,
probably during some mtree refactor, but went unnoticed since no test
actually checked that lfsr_fs_fixorphans did what it said it did.

At least the new test_forphans_cleanup* tests should prevent this from
regressing again in the future.

Code changes:

           code          stack
  before: 35472           2680
  after:  35480 (+0.0%)   2680 (+0.0%)

lfs.c

@@ -12752,9 +12752,8 @@ static int lfsr_fs_fixorphans(lfs_t *lfs) {
     //
     // note this never takes longer than lfsr_mount
     //
-    for (lfsr_mid_t mid = 0;
+    lfsr_mid_t mid = 0;
-            mid < lfsr_mtree_weight(lfs);
+    while (mid < lfsr_mtree_weight(lfs)) {
-            mid += (1 << lfs->mdir_bits)) {
         lfsr_mdir_t mdir;
         int err = lfsr_mtree_lookup(lfs, mid,
                 &mdir);
@@ -12763,10 +12762,16 @@ static int lfsr_fs_fixorphans(lfs_t *lfs) {
             return err;
         }
 
+        // clean up orphans
         err = lfsr_mdir_fixorphans(lfs, &mdir);
         if (err) {
             return err;
         }
+
+        // incremend mid unless we dropped the mdir
+        if (mdir.rbyd.weight > 0) {
+            mid += 1 << lfs->mdir_bits;
+        }
     }
 
     // done, no more orphans

tests/test_forphans.toml

@@ -4610,6 +4610,438 @@ code = '''
     lfsr_unmount(&lfs) => 0;
 '''
 
+
+# test that we actually cleanup orphans correctly
+[cases.test_forphans_cleanup]
+defines.SIZE = [
+    'FILE_BUFFER_SIZE/2',
+    '2*FILE_BUFFER_SIZE',
+    'BLOCK_SIZE/2',
+    'BLOCK_SIZE',
+    '2*BLOCK_SIZE',
+    '4*BLOCK_SIZE',
+]
+# <=2 => grm-able
+#  >2 => requires orphans
+defines.N = [0, 1, 2, 3, 10, 100]
+defines.REMOUNT = [false, true]
+defines.BOOKENDS = [0x0, 0x1, 0x2, 0x3]
+if = '(SIZE*N)/BLOCK_SIZE <= 32'
+in = 'lfs.c'
+code = '''
+    lfs_t lfs;
+    lfsr_format(&lfs, CFG) => 0;
+    lfsr_mount(&lfs, CFG) => 0;
+
+    uint32_t prng = 42;
+
+    // create some unrelated files to make sure cleaning up orphans doesn't
+    // break other filesystem things
+    uint32_t bookend_prngs[2] = {0, 0};
+    if (BOOKENDS & 0x1) {
+        lfsr_file_t file;
+        lfsr_file_open(&lfs, &file, "aatman",
+                LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
+        bookend_prngs[0] = TEST_PRNG(&prng);
+        uint32_t prng_ = bookend_prngs[0];
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+        }
+        lfsr_file_write(&lfs, &file, wbuf, SIZE) => SIZE;
+        lfsr_file_close(&lfs, &file) => 0;
+    }
+
+    if (BOOKENDS & 0x2) {
+        lfsr_file_t file;
+        lfsr_file_open(&lfs, &file, "catman",
+                LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
+        bookend_prngs[1] = TEST_PRNG(&prng);
+        uint32_t prng_ = bookend_prngs[1];
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+        }
+        lfsr_file_write(&lfs, &file, wbuf, SIZE) => SIZE;
+        lfsr_file_close(&lfs, &file) => 0;
+    }
+
+    // create this many orphaned files
+    //
+    // anytime we close a not-yet-created desync file, we create an
+    // orphan, but note we need these to be different files, and we need
+    // to close them after all open calls, otherwise we just end up with
+    // one orphan (littlefs is eager to clean up orphans)
+    //
+    lfsr_file_t files[N];
+    for (lfs_size_t i = 0; i < N; i++) {
+        char name[256];
+        sprintf(name, "batman%03x", i);
+        lfsr_file_open(&lfs, &files[i], name,
+                LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL | LFS_O_DESYNC) => 0;
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng) % 26);
+        }
+        lfsr_file_write(&lfs, &files[i], wbuf, SIZE) => SIZE;
+    }
+    for (lfs_size_t i = 0; i < N; i++) {
+        lfsr_file_close(&lfs, &files[i]) => 0;
+    }
+
+    // remount? this has no effect on orphans
+    if (REMOUNT) {
+        lfsr_unmount(&lfs) => 0;
+        lfsr_mount(&lfs, CFG) => 0;
+    }
+
+    // calling lfsr_fs_mkconsistent should clean things up
+    lfsr_fs_mkconsistent(&lfs) => 0;
+
+    // we should have cleaned up all grms/orphans
+    assert(lfs.grm.mids[0] == -1);
+    assert(lfs.grm.mids[1] == -1);
+    assert(lfs.hasorphans == false);
+
+    // double check the actual disk state, it's easy for littlefs to
+    // lie here
+    assert(lfsr_mtree_weight(&lfs)
+            <= ((1+lfs_popc(BOOKENDS)) << lfs.mdir_bits));
+    lfsr_mdir_t mdir;
+    lfsr_mtree_lookup(&lfs, 0, &mdir) => 0;
+    assert(mdir.rbyd.weight <= 1+lfs_popc(BOOKENDS));
+
+    // check that other files are unaffected
+    for (int remount = 0; remount < 2; remount++) {
+        if (remount) {
+            lfsr_unmount(&lfs) => 0;
+            lfsr_mount(&lfs, CFG) => 0;
+        }
+
+        if (BOOKENDS & 0x1) {
+            lfsr_file_t file;
+            lfsr_file_open(&lfs, &file, "aatman", LFS_O_RDONLY) => 0;
+            uint32_t prng_ = bookend_prngs[0];
+            uint8_t wbuf[SIZE];
+            for (lfs_size_t j = 0; j < SIZE; j++) {
+                wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+            }
+            uint8_t rbuf[SIZE];
+            lfsr_file_read(&lfs, &file, rbuf, SIZE) => SIZE;
+            assert(memcmp(rbuf, wbuf, SIZE) == 0);
+            lfsr_file_close(&lfs, &file);
+        }
+
+        if (BOOKENDS & 0x2) {
+            lfsr_file_t file;
+            lfsr_file_open(&lfs, &file, "catman", LFS_O_RDONLY) => 0;
+            uint32_t prng_ = bookend_prngs[1];
+            uint8_t wbuf[SIZE];
+            for (lfs_size_t j = 0; j < SIZE; j++) {
+                wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+            }
+            uint8_t rbuf[SIZE];
+            lfsr_file_read(&lfs, &file, rbuf, SIZE) => SIZE;
+            assert(memcmp(rbuf, wbuf, SIZE) == 0);
+            lfsr_file_close(&lfs, &file);
+        }
+    }
+
+    lfsr_unmount(&lfs) => 0;
+'''
+
+[cases.test_forphans_cleanup_opened]
+defines.SIZE = [
+    'FILE_BUFFER_SIZE/2',
+    '2*FILE_BUFFER_SIZE',
+    'BLOCK_SIZE/2',
+    'BLOCK_SIZE',
+    '2*BLOCK_SIZE',
+    '4*BLOCK_SIZE',
+]
+# <=2 => grm-able
+#  >2 => requires orphans
+defines.N = [0, 1, 2, 3, 10, 100]
+defines.BOOKENDS = [0x0, 0x1, 0x2, 0x3]
+if = '(SIZE*N)/BLOCK_SIZE <= 32'
+in = 'lfs.c'
+code = '''
+    lfs_t lfs;
+    lfsr_format(&lfs, CFG) => 0;
+    lfsr_mount(&lfs, CFG) => 0;
+
+    uint32_t prng = 42;
+
+    // create some unrelated files to make sure cleaning up orphans doesn't
+    // break other filesystem things
+    lfsr_file_t bookend_files[2];
+    uint32_t bookend_prngs[2] = {0, 0};
+    if (BOOKENDS & 0x1) {
+        lfsr_file_open(&lfs, &bookend_files[0], "aatman",
+                LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
+        bookend_prngs[0] = TEST_PRNG(&prng);
+        uint32_t prng_ = bookend_prngs[0];
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+        }
+        lfsr_file_write(&lfs, &bookend_files[0], wbuf, SIZE) => SIZE;
+        lfsr_file_sync(&lfs, &bookend_files[0]) => 0;
+    }
+
+    if (BOOKENDS & 0x2) {
+        lfsr_file_open(&lfs, &bookend_files[1], "catman",
+                LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
+        bookend_prngs[1] = TEST_PRNG(&prng);
+        uint32_t prng_ = bookend_prngs[1];
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+        }
+        lfsr_file_write(&lfs, &bookend_files[1], wbuf, SIZE) => SIZE;
+        lfsr_file_sync(&lfs, &bookend_files[1]) => 0;
+    }
+
+    // create this many orphaned files
+    //
+    // anytime we close a not-yet-created desync file, we create an
+    // orphan, but note we need these to be different files, and we need
+    // to close them after all open calls, otherwise we just end up with
+    // one orphan (littlefs is eager to clean up orphans)
+    //
+    lfsr_file_t files[N];
+    for (lfs_size_t i = 0; i < N; i++) {
+        char name[256];
+        sprintf(name, "batman%03x", i);
+        lfsr_file_open(&lfs, &files[i], name,
+                LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL | LFS_O_DESYNC) => 0;
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng) % 26);
+        }
+        lfsr_file_write(&lfs, &files[i], wbuf, SIZE) => SIZE;
+    }
+    for (lfs_size_t i = 0; i < N; i++) {
+        lfsr_file_close(&lfs, &files[i]) => 0;
+    }
+
+    // calling lfsr_fs_mkconsistent should clean things up
+    lfsr_fs_mkconsistent(&lfs) => 0;
+
+    // we should have cleaned up all grms/orphans
+    assert(lfs.grm.mids[0] == -1);
+    assert(lfs.grm.mids[1] == -1);
+    assert(lfs.hasorphans == false);
+
+    // double check the actual disk state, it's easy for littlefs to
+    // lie here
+    assert(lfsr_mtree_weight(&lfs)
+            <= ((1+lfs_popc(BOOKENDS)) << lfs.mdir_bits));
+    lfsr_mdir_t mdir;
+    lfsr_mtree_lookup(&lfs, 0, &mdir) => 0;
+    assert(mdir.rbyd.weight <= 1+lfs_popc(BOOKENDS));
+
+    // check that other files are unaffected
+    for (int remount = 0; remount < 2; remount++) {
+        if (remount) {
+            if (BOOKENDS & 0x1) {
+                lfsr_file_close(&lfs, &bookend_files[0]) => 0;
+            }
+            if (BOOKENDS & 0x2) {
+                lfsr_file_close(&lfs, &bookend_files[1]) => 0;
+            }
+            lfsr_unmount(&lfs) => 0;
+            lfsr_mount(&lfs, CFG) => 0;
+            if (BOOKENDS & 0x1) {
+                lfsr_file_open(&lfs, &bookend_files[0], "aatman",
+                        LFS_O_RDONLY) => 0;
+            }
+            if (BOOKENDS & 0x2) {
+                lfsr_file_open(&lfs, &bookend_files[1], "catman",
+                        LFS_O_RDONLY) => 0;
+            }
+        }
+
+        if (BOOKENDS & 0x1) {
+            lfsr_file_rewind(&lfs, &bookend_files[0]) => 0;
+            uint32_t prng_ = bookend_prngs[0];
+            uint8_t wbuf[SIZE];
+            for (lfs_size_t j = 0; j < SIZE; j++) {
+                wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+            }
+            uint8_t rbuf[SIZE];
+            lfsr_file_read(&lfs, &bookend_files[0], rbuf, SIZE) => SIZE;
+            assert(memcmp(rbuf, wbuf, SIZE) == 0);
+        }
+
+        if (BOOKENDS & 0x2) {
+            lfsr_file_rewind(&lfs, &bookend_files[1]) => 0;
+            uint32_t prng_ = bookend_prngs[1];
+            uint8_t wbuf[SIZE];
+            for (lfs_size_t j = 0; j < SIZE; j++) {
+                wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+            }
+            uint8_t rbuf[SIZE];
+            lfsr_file_read(&lfs, &bookend_files[1], rbuf, SIZE) => SIZE;
+            assert(memcmp(rbuf, wbuf, SIZE) == 0);
+        }
+    }
+
+    if (BOOKENDS & 0x1) {
+        lfsr_file_close(&lfs, &bookend_files[0]);
+    }
+    if (BOOKENDS & 0x2) {
+        lfsr_file_close(&lfs, &bookend_files[1]);
+    }
+    lfsr_unmount(&lfs) => 0;
+'''
+
+[cases.test_forphans_cleanup_orphaned]
+defines.SIZE = [
+    'FILE_BUFFER_SIZE/2',
+    '2*FILE_BUFFER_SIZE',
+    'BLOCK_SIZE/2',
+    'BLOCK_SIZE',
+    '2*BLOCK_SIZE',
+    '4*BLOCK_SIZE',
+]
+# <=2 => grm-able
+#  >2 => requires orphans
+defines.N = [0, 1, 2, 3, 10, 100]
+defines.BOOKENDS = [0x0, 0x1, 0x2, 0x3]
+if = '(SIZE*N)/BLOCK_SIZE <= 32'
+in = 'lfs.c'
+code = '''
+    lfs_t lfs;
+    lfsr_format(&lfs, CFG) => 0;
+    lfsr_mount(&lfs, CFG) => 0;
+
+    uint32_t prng = 42;
+
+    // create some unrelated files to make sure cleaning up orphans doesn't
+    // break other filesystem things
+    lfsr_file_t bookend_files[2];
+    uint32_t bookend_prngs[2] = {0, 0};
+    if (BOOKENDS & 0x1) {
+        lfsr_file_open(&lfs, &bookend_files[0], "aatman",
+                LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
+        bookend_prngs[0] = TEST_PRNG(&prng);
+        uint32_t prng_ = bookend_prngs[0];
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+        }
+        lfsr_file_write(&lfs, &bookend_files[0], wbuf, SIZE) => SIZE;
+    }
+
+    if (BOOKENDS & 0x2) {
+        lfsr_file_open(&lfs, &bookend_files[1], "catman",
+                LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
+        bookend_prngs[1] = TEST_PRNG(&prng);
+        uint32_t prng_ = bookend_prngs[1];
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+        }
+        lfsr_file_write(&lfs, &bookend_files[1], wbuf, SIZE) => SIZE;
+    }
+
+    // create this many orphaned files
+    //
+    // anytime we close a not-yet-created desync file, we create an
+    // orphan, but note we need these to be different files, and we need
+    // to close them after all open calls, otherwise we just end up with
+    // one orphan (littlefs is eager to clean up orphans)
+    //
+    lfsr_file_t files[N];
+    for (lfs_size_t i = 0; i < N; i++) {
+        char name[256];
+        sprintf(name, "batman%03x", i);
+        lfsr_file_open(&lfs, &files[i], name,
+                LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL | LFS_O_DESYNC) => 0;
+        uint8_t wbuf[SIZE];
+        for (lfs_size_t j = 0; j < SIZE; j++) {
+            wbuf[j] = 'a' + (TEST_PRNG(&prng) % 26);
+        }
+        lfsr_file_write(&lfs, &files[i], wbuf, SIZE) => SIZE;
+    }
+    for (lfs_size_t i = 0; i < N; i++) {
+        lfsr_file_close(&lfs, &files[i]) => 0;
+    }
+
+    // calling lfsr_fs_mkconsistent should clean things up
+    lfsr_fs_mkconsistent(&lfs) => 0;
+
+    // we should have cleaned up all grms/orphans
+    assert(lfs.grm.mids[0] == -1);
+    assert(lfs.grm.mids[1] == -1);
+    assert(lfs.hasorphans == false);
+
+    // double check the actual disk state, it's easy for littlefs to
+    // lie here
+    assert(lfsr_mtree_weight(&lfs)
+            <= ((1+lfs_popc(BOOKENDS)) << lfs.mdir_bits));
+    lfsr_mdir_t mdir;
+    lfsr_mtree_lookup(&lfs, 0, &mdir) => 0;
+    assert(mdir.rbyd.weight <= 1+lfs_popc(BOOKENDS));
+
+    // check that other files are unaffected
+    for (int remount = 0; remount < 2; remount++) {
+        if (remount) {
+            if (BOOKENDS & 0x1) {
+                lfsr_file_close(&lfs, &bookend_files[0]) => 0;
+            }
+            if (BOOKENDS & 0x2) {
+                lfsr_file_close(&lfs, &bookend_files[1]) => 0;
+            }
+            lfsr_unmount(&lfs) => 0;
+            lfsr_mount(&lfs, CFG) => 0;
+            if (BOOKENDS & 0x1) {
+                lfsr_file_open(&lfs, &bookend_files[0], "aatman",
+                        LFS_O_RDONLY) => 0;
+            }
+            if (BOOKENDS & 0x2) {
+                lfsr_file_open(&lfs, &bookend_files[1], "catman",
+                        LFS_O_RDONLY) => 0;
+            }
+        }
+
+        if (BOOKENDS & 0x1) {
+            lfsr_file_rewind(&lfs, &bookend_files[0]) => 0;
+            uint32_t prng_ = bookend_prngs[0];
+            uint8_t wbuf[SIZE];
+            for (lfs_size_t j = 0; j < SIZE; j++) {
+                wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+            }
+            uint8_t rbuf[SIZE];
+            lfsr_file_read(&lfs, &bookend_files[0], rbuf, SIZE) => SIZE;
+            assert(memcmp(rbuf, wbuf, SIZE) == 0);
+        }
+
+        if (BOOKENDS & 0x2) {
+            lfsr_file_rewind(&lfs, &bookend_files[1]) => 0;
+            uint32_t prng_ = bookend_prngs[1];
+            uint8_t wbuf[SIZE];
+            for (lfs_size_t j = 0; j < SIZE; j++) {
+                wbuf[j] = 'a' + (TEST_PRNG(&prng_) % 26);
+            }
+            uint8_t rbuf[SIZE];
+            lfsr_file_read(&lfs, &bookend_files[1], rbuf, SIZE) => SIZE;
+            assert(memcmp(rbuf, wbuf, SIZE) == 0);
+        }
+    }
+
+    if (BOOKENDS & 0x1) {
+        lfsr_file_close(&lfs, &bookend_files[0]);
+    }
+    if (BOOKENDS & 0x2) {
+        lfsr_file_close(&lfs, &bookend_files[1]);
+    }
+    lfsr_unmount(&lfs) => 0;
+'''
+
+
+
 # these doesn't really involve scratch files, but we might as well test
 # them here
 [cases.test_forphans_mv]