Prevented unnecessary superblock rewrites if old version in superblock chain

Because multiple, out-of-date superblocks can exist in our superblock
chain, we need to be careful to make sure newer superblock entries
override older superblock entries.

If we see an older on-disk minor version in the superblock chain, we
were correctly overriding the on-disk minor version, but we were also
leaving the "needs superblock" bit set in our consistency state.

This isn't a hard-error, but would lead to a superblock rewrite every
mount. The rewrite would make no progress, as the out-of-date version is
effectively immutable at this point, and just waste prog cycles.

This should fix that by clearing the "needs superblock" bit if we see a
newer on-disk minor version.

.github/workflows/release.yml

@@ -254,7 +254,7 @@ jobs:
               tag_name: env.LFS_VERSION,
               name: env.LFS_VERSION | rtrimstr(".0"),
               target_commitish: "${{github.event.workflow_run.head_sha}}",
-              draft: true,
+              draft: env.LFS_VERSION | endswith(".0"),
               body: $release,
             }' | tee /dev/stderr)"
 

SPEC.md

@@ -441,9 +441,10 @@ Superblock fields:
 
 7. **Attr max (32-bits)** - Maximum size of file attributes in bytes.
 
-The superblock must always be the first entry (id 0) in a metadata pair as well
-as be the first entry written to the block. This means that the superblock
-entry can be read from a device using offsets alone.
+The superblock must always be the first entry (id 0) in the metadata pair, and
+the name tag must always be the first tag in the metadata pair. This makes it
+so that the magic string "littlefs" will always reside at offset=8 in a valid
+littlefs superblock.
 
 ---
 #### `0x2xx` LFS_TYPE_STRUCT

lfs.c

@@ -688,7 +688,7 @@ static int lfs_alloc(lfs_t *lfs, lfs_block_t *block) {
         if (lfs->lookahead.ckpoint <= 0) {
             LFS_ERROR("No more free space 0x%"PRIx32,
                     (lfs->lookahead.start + lfs->lookahead.next)
-                        % lfs->block_count);
+                        % lfs->cfg->block_count);
             return LFS_ERR_NOSPC;
         }
 
@@ -2191,7 +2191,8 @@ static int lfs_dir_splittingcompact(lfs_t *lfs, lfs_mdir_t *dir,
                 // we can do, we'll error later if we've become frozen
                 LFS_WARN("Unable to expand superblock");
             } else {
-                end = begin;
+                // duplicate the superblock entry into the new superblock
+                end = 1;
             }
         }
     }
@@ -2358,7 +2359,9 @@ fixmlist:;
 
             while (d->id >= d->m.count && d->m.split) {
                 // we split and id is on tail now
-                d->id -= d->m.count;
+                if (lfs_pair_cmp(d->m.tail, lfs->root) != 0) {
+                    d->id -= d->m.count;
+                }
                 int err = lfs_dir_fetch(lfs, &d->m, d->m.tail);
                 if (err) {
                     return err;
@@ -4466,6 +4469,7 @@ static int lfs_mount_(lfs_t *lfs, const struct lfs_config *cfg) {
             // found older minor version? set an in-device only bit in the
             // gstate so we know we need to rewrite the superblock before
             // the first write
+            bool needssuperblock = false;
             if (minor_version < lfs_fs_disk_version_minor(lfs)) {
                 LFS_DEBUG("Found older minor version "
                         "v%"PRIu16".%"PRIu16" < v%"PRIu16".%"PRIu16,
@@ -4473,10 +4477,11 @@ static int lfs_mount_(lfs_t *lfs, const struct lfs_config *cfg) {
                         minor_version,
                         lfs_fs_disk_version_major(lfs),
                         lfs_fs_disk_version_minor(lfs));
-                // note this bit is reserved on disk, so fetching more gstate
-                // will not interfere here
-                lfs_fs_prepsuperblock(lfs, true);
+                needssuperblock = true;
             }
+            // note this bit is reserved on disk, so fetching more gstate
+            // will not interfere here
+            lfs_fs_prepsuperblock(lfs, needssuperblock);
 
             // check superblock configuration
             if (superblock.name_max) {

lfs.h

@@ -59,8 +59,7 @@ typedef uint32_t lfs_block_t;
 #endif
 
 // Maximum size of custom attributes in bytes, may be redefined, but there is
-// no real benefit to using a smaller LFS_ATTR_MAX. Limited to <= 1022. Stored
-// in superblock and must be respected by other littlefs drivers.
+// no real benefit to using a smaller LFS_ATTR_MAX. Limited to <= 1022.
 #ifndef LFS_ATTR_MAX
 #define LFS_ATTR_MAX 1022
 #endif
@@ -204,8 +203,7 @@ struct lfs_config {
     // program sizes.
     lfs_size_t block_size;
 
-    // Number of erasable blocks on the device. Defaults to block_count stored
-    // on disk when zero.
+    // Number of erasable blocks on the device.
     lfs_size_t block_count;
 
     // Number of erase cycles before littlefs evicts metadata logs and moves
@@ -254,18 +252,18 @@ struct lfs_config {
 
     // Optional upper limit on length of file names in bytes. No downside for
     // larger names except the size of the info struct which is controlled by
-    // the LFS_NAME_MAX define. Defaults to LFS_NAME_MAX or name_max stored on
-    // disk when zero.
+    // the LFS_NAME_MAX define. Defaults to LFS_NAME_MAX when zero. Stored in
+    // superblock and must be respected by other littlefs drivers.
     lfs_size_t name_max;
 
     // Optional upper limit on files in bytes. No downside for larger files
-    // but must be <= LFS_FILE_MAX. Defaults to LFS_FILE_MAX or file_max stored
-    // on disk when zero.
+    // but must be <= LFS_FILE_MAX. Defaults to LFS_FILE_MAX when zero. Stored
+    // in superblock and must be respected by other littlefs drivers.
     lfs_size_t file_max;
 
     // Optional upper limit on custom attributes in bytes. No downside for
     // larger attributes size but must be <= LFS_ATTR_MAX. Defaults to
-    // LFS_ATTR_MAX or attr_max stored on disk when zero.
+    // LFS_ATTR_MAX when zero.
     lfs_size_t attr_max;
 
     // Optional upper limit on total space given to metadata pairs in bytes. On

tests/test_alloc.toml

@@ -8,22 +8,17 @@ defines.FILES = 3
 defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / FILES)'
 defines.GC = [false, true]
 defines.COMPACT_THRESH = ['-1', '0', 'BLOCK_SIZE/2']
-defines.INFER_BC = [false, true]
 code = '''
     const char *names[] = {"bacon", "eggs", "pancakes"};
     lfs_file_t files[FILES];
 
     lfs_t lfs;
     lfs_format(&lfs, cfg) => 0;
-    struct lfs_config cfg_ = *cfg;
-    if (INFER_BC) {
-        cfg_.block_count = 0;
-    }
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     lfs_mkdir(&lfs, "breakfast") => 0;
     lfs_unmount(&lfs) => 0;
 
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     for (int n = 0; n < FILES; n++) {
         char path[1024];
         sprintf(path, "breakfast/%s", names[n]);
@@ -44,7 +39,7 @@ code = '''
     }
     lfs_unmount(&lfs) => 0;
 
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     for (int n = 0; n < FILES; n++) {
         char path[1024];
         sprintf(path, "breakfast/%s", names[n]);
@@ -67,22 +62,17 @@ defines.FILES = 3
 defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / FILES)'
 defines.GC = [false, true]
 defines.COMPACT_THRESH = ['-1', '0', 'BLOCK_SIZE/2']
-defines.INFER_BC = [false, true]
 code = '''
     const char *names[] = {"bacon", "eggs", "pancakes"};
 
     lfs_t lfs;
     lfs_format(&lfs, cfg) => 0;
-    struct lfs_config cfg_ = *cfg;
-    if (INFER_BC) {
-        cfg_.block_count = 0;
-    }
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     lfs_mkdir(&lfs, "breakfast") => 0;
     lfs_unmount(&lfs) => 0;
 
     for (int n = 0; n < FILES; n++) {
-        lfs_mount(&lfs, &cfg_) => 0;
+        lfs_mount(&lfs, cfg) => 0;
         char path[1024];
         sprintf(path, "breakfast/%s", names[n]);
         lfs_file_t file;
@@ -101,7 +91,7 @@ code = '''
         lfs_unmount(&lfs) => 0;
     }
 
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     for (int n = 0; n < FILES; n++) {
         char path[1024];
         sprintf(path, "breakfast/%s", names[n]);
@@ -123,24 +113,19 @@ code = '''
 defines.FILES = 3
 defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / FILES)'
 defines.CYCLES = [1, 10]
-defines.INFER_BC = [false, true]
 code = '''
     const char *names[] = {"bacon", "eggs", "pancakes"};
     lfs_file_t files[FILES];
 
     lfs_t lfs;
     lfs_format(&lfs, cfg) => 0;
-    struct lfs_config cfg_ = *cfg;
-    if (INFER_BC) {
-        cfg_.block_count = 0;
-    }
 
     for (int c = 0; c < CYCLES; c++) {
-        lfs_mount(&lfs, &cfg_) => 0;
+        lfs_mount(&lfs, cfg) => 0;
         lfs_mkdir(&lfs, "breakfast") => 0;
         lfs_unmount(&lfs) => 0;
 
-        lfs_mount(&lfs, &cfg_) => 0;
+        lfs_mount(&lfs, cfg) => 0;
         for (int n = 0; n < FILES; n++) {
             char path[1024];
             sprintf(path, "breakfast/%s", names[n]);
@@ -158,7 +143,7 @@ code = '''
         }
         lfs_unmount(&lfs) => 0;
 
-        lfs_mount(&lfs, &cfg_) => 0;
+        lfs_mount(&lfs, cfg) => 0;
         for (int n = 0; n < FILES; n++) {
             char path[1024];
             sprintf(path, "breakfast/%s", names[n]);
@@ -174,7 +159,7 @@ code = '''
         }
         lfs_unmount(&lfs) => 0;
 
-        lfs_mount(&lfs, &cfg_) => 0;
+        lfs_mount(&lfs, cfg) => 0;
         for (int n = 0; n < FILES; n++) {
             char path[1024];
             sprintf(path, "breakfast/%s", names[n]);
@@ -190,24 +175,19 @@ code = '''
 defines.FILES = 3
 defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / FILES)'
 defines.CYCLES = [1, 10]
-defines.INFER_BC = [false, true]
 code = '''
     const char *names[] = {"bacon", "eggs", "pancakes"};
 
     lfs_t lfs;
     lfs_format(&lfs, cfg) => 0;
-    struct lfs_config cfg_ = *cfg;
-    if (INFER_BC) {
-        cfg_.block_count = 0;
-    }
 
     for (int c = 0; c < CYCLES; c++) {
-        lfs_mount(&lfs, &cfg_) => 0;
+        lfs_mount(&lfs, cfg) => 0;
         lfs_mkdir(&lfs, "breakfast") => 0;
         lfs_unmount(&lfs) => 0;
 
         for (int n = 0; n < FILES; n++) {
-            lfs_mount(&lfs, &cfg_) => 0;
+            lfs_mount(&lfs, cfg) => 0;
             char path[1024];
             sprintf(path, "breakfast/%s", names[n]);
             lfs_file_t file;
@@ -252,15 +232,10 @@ code = '''
 
 # exhaustion test
 [cases.test_alloc_exhaustion]
-defines.INFER_BC = [false, true]
 code = '''
     lfs_t lfs;
     lfs_format(&lfs, cfg) => 0;
-    struct lfs_config cfg_ = *cfg;
-    if (INFER_BC) {
-        cfg_.block_count = 0;
-    }
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     lfs_file_t file;
     lfs_file_open(&lfs, &file, "exhaustion", LFS_O_WRONLY | LFS_O_CREAT);
     size_t size = strlen("exhaustion");
@@ -288,7 +263,7 @@ code = '''
     lfs_file_close(&lfs, &file) => 0;
     lfs_unmount(&lfs) => 0;
 
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     lfs_file_open(&lfs, &file, "exhaustion", LFS_O_RDONLY);
     size = strlen("exhaustion");
     lfs_file_size(&lfs, &file) => size;
@@ -301,15 +276,10 @@ code = '''
 # exhaustion wraparound test
 [cases.test_alloc_exhaustion_wraparound]
 defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-4)) / 3)'
-defines.INFER_BC = [false, true]
 code = '''
     lfs_t lfs;
     lfs_format(&lfs, cfg) => 0;
-    struct lfs_config cfg_ = *cfg;
-    if (INFER_BC) {
-        cfg_.block_count = 0;
-    }
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
 
     lfs_file_t file;
     lfs_file_open(&lfs, &file, "padding", LFS_O_WRONLY | LFS_O_CREAT);
@@ -347,7 +317,7 @@ code = '''
     lfs_file_close(&lfs, &file) => 0;
     lfs_unmount(&lfs) => 0;
 
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
     lfs_file_open(&lfs, &file, "exhaustion", LFS_O_RDONLY);
     size = strlen("exhaustion");
     lfs_file_size(&lfs, &file) => size;
@@ -360,15 +330,10 @@ code = '''
 
 # dir exhaustion test
 [cases.test_alloc_dir_exhaustion]
-defines.INFER_BC = [false, true]
 code = '''
     lfs_t lfs;
     lfs_format(&lfs, cfg) => 0;
-    struct lfs_config cfg_ = *cfg;
-    if (INFER_BC) {
-        cfg_.block_count = 0;
-    }
-    lfs_mount(&lfs, &cfg_) => 0;
+    lfs_mount(&lfs, cfg) => 0;
 
     // find out max file size
     lfs_mkdir(&lfs, "exhaustiondir") => 0;

tests/test_superblocks.toml

@@ -14,6 +14,24 @@ code = '''
     lfs_unmount(&lfs) => 0;
 '''
 
+# make sure the magic string "littlefs" is always at offset=8
+[cases.test_superblocks_magic]
+code = '''
+    lfs_t lfs;
+    lfs_format(&lfs, cfg) => 0;
+
+    // check our magic string
+    //
+    // note if we lose power we may not have the magic string in both blocks!
+    // but we don't lose power in this test so we can assert the magic string
+    // is present in both
+    uint8_t magic[lfs_max(16, READ_SIZE)];
+    cfg->read(cfg, 0, 0, magic, lfs_max(16, READ_SIZE)) => 0;
+    assert(memcmp(&magic[8], "littlefs", 8) == 0);
+    cfg->read(cfg, 1, 0, magic, lfs_max(16, READ_SIZE)) => 0;
+    assert(memcmp(&magic[8], "littlefs", 8) == 0);
+'''
+
 # mount/unmount from interpretting a previous superblock block_count
 [cases.test_superblocks_mount_unknown_block_count]
 code = '''
@@ -28,7 +46,6 @@ code = '''
     lfs_unmount(&lfs) => 0;
 '''
 
-
 # reentrant format
 [cases.test_superblocks_reentrant_format]
 reentrant = true
@@ -135,6 +152,39 @@ code = '''
     lfs_unmount(&lfs) => 0;
 '''
 
+# make sure the magic string "littlefs" is always at offset=8
+[cases.test_superblocks_magic_expand]
+defines.BLOCK_CYCLES = [32, 33, 1]
+defines.N = [10, 100, 1000]
+code = '''
+    lfs_t lfs;
+    lfs_format(&lfs, cfg) => 0;
+    lfs_mount(&lfs, cfg) => 0;
+    for (int i = 0; i < N; i++) {
+        lfs_file_t file;
+        lfs_file_open(&lfs, &file, "dummy",
+                LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
+        lfs_file_close(&lfs, &file) => 0;
+        struct lfs_info info;
+        lfs_stat(&lfs, "dummy", &info) => 0;
+        assert(strcmp(info.name, "dummy") == 0);
+        assert(info.type == LFS_TYPE_REG);
+        lfs_remove(&lfs, "dummy") => 0;
+    }
+    lfs_unmount(&lfs) => 0;
+
+    // check our magic string
+    //
+    // note if we lose power we may not have the magic string in both blocks!
+    // but we don't lose power in this test so we can assert the magic string
+    // is present in both
+    uint8_t magic[lfs_max(16, READ_SIZE)];
+    cfg->read(cfg, 0, 0, magic, lfs_max(16, READ_SIZE)) => 0;
+    assert(memcmp(&magic[8], "littlefs", 8) == 0);
+    cfg->read(cfg, 1, 0, magic, lfs_max(16, READ_SIZE)) => 0;
+    assert(memcmp(&magic[8], "littlefs", 8) == 0);
+'''
+
 # expanding superblock with power cycle
 [cases.test_superblocks_expand_power_cycle]
 defines.BLOCK_CYCLES = [32, 33, 1]
@@ -221,6 +271,7 @@ code = '''
     lfs_unmount(&lfs) => 0;
 '''
 
+
 # mount with unknown block_count
 [cases.test_superblocks_unknown_blocks]
 code = '''