Skip to content

Commit a60a986

Browse files
committed
Duplicate the superblock entry during superblock expansion
The documentation does not match the implementation here. The intended behavior of superblock expansion was to duplicate the current superblock entry into the new superblock: .--------. .--------. .|littlefs|->|littlefs| ||bs=4096 | ||bs=4096 | ||bc=256 | ||bc=256 | ||crc32 | ||root dir| || | ||crc32 | |'--------' |'--------' '--------' '--------' The main benefit is that we can rely on the magic string "littlefs" always residing in blocks 0x{0,1}, even if the superblock chain has multiple superblocks. The downside is that earlier superblocks in the superblock chain may contain out-of-date configuration. This is a bit annoying, and risks hard-to-reach bugs, but in theory shouldn't break anything as long as the filesystem is aware of this. Unfortunately this was lost at some point during refactoring in the early v2-alpha work. A lot of code was moving around in this stage, so it's a bit hard to track down the change and if it was intentional. The result is superblock expansion creates a valid linked-list of superblocks, but only the last superblock contains a valid superblock entry: .--------. .--------. .|crc32 |->|littlefs| || | ||bs=4096 | || | ||bc=256 | || | ||root dir| || | ||crc32 | |'--------' |'--------' '--------' '--------' What's interesting is this isn't invalid as far as lfs_mount is concerned. lfs_mount is happy as long as a superblock entry exists anywhere in the superblock chain. This is good for compat flexibility, but is the main reason this has gone unnoticed for so long. --- With the benefit of more time to think about the problem, it may have been more preferable to copy only the "littlefs" magic string and NOT the superblock entry: .--------. .--------. .|littlefs|->|littlefs| ||crc32c | ||bs=4096 | || | ||bc=256 | || | ||root dir| || | ||crc32 | |'--------' |'--------' '--------' '--------' This would allow for simple "littlefs" magic string checks without the risks associated with out-of-date superblock entries. Unfortunately the current implementation errors if it finds a "littlefs" magic string without an associated superblock entry, so such a change would not be compatible with old drivers. --- This commit tweaks superblock expansion to duplicate the superblock entry instead of simply moving it to the new superblock. And adds tests over the magic string "littlefs" both before and after superblock expansion. Found by rojer and Nikola Kosturski
1 parent 4dd30c1 commit a60a986

File tree

2 files changed

+57
-3
lines changed

2 files changed

+57
-3
lines changed

lfs.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2191,7 +2191,8 @@ static int lfs_dir_splittingcompact(lfs_t *lfs, lfs_mdir_t *dir,
21912191
// we can do, we'll error later if we've become frozen
21922192
LFS_WARN("Unable to expand superblock");
21932193
} else {
2194-
end = begin;
2194+
// duplicate the superblock entry into the new superblock
2195+
end = 1;
21952196
}
21962197
}
21972198
}
@@ -2358,7 +2359,9 @@ fixmlist:;
23582359

23592360
while (d->id >= d->m.count && d->m.split) {
23602361
// we split and id is on tail now
2361-
d->id -= d->m.count;
2362+
if (lfs_pair_cmp(d->m.tail, lfs->root) != 0) {
2363+
d->id -= d->m.count;
2364+
}
23622365
int err = lfs_dir_fetch(lfs, &d->m, d->m.tail);
23632366
if (err) {
23642367
return err;

tests/test_superblocks.toml

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,24 @@ code = '''
1414
lfs_unmount(&lfs) => 0;
1515
'''
1616

17+
# make sure the magic string "littlefs" is always at offset=8
18+
[cases.test_superblocks_magic]
19+
code = '''
20+
lfs_t lfs;
21+
lfs_format(&lfs, cfg) => 0;
22+
23+
// check our magic string
24+
//
25+
// note if we lose power we may not have the magic string in both blocks!
26+
// but we don't lose power in this test so we can assert the magic string
27+
// is present in both
28+
uint8_t magic[lfs_max(16, READ_SIZE)];
29+
cfg->read(cfg, 0, 0, magic, lfs_max(16, READ_SIZE)) => 0;
30+
assert(memcmp(&magic[8], "littlefs", 8) == 0);
31+
cfg->read(cfg, 1, 0, magic, lfs_max(16, READ_SIZE)) => 0;
32+
assert(memcmp(&magic[8], "littlefs", 8) == 0);
33+
'''
34+
1735
# mount/unmount from interpretting a previous superblock block_count
1836
[cases.test_superblocks_mount_unknown_block_count]
1937
code = '''
@@ -28,7 +46,6 @@ code = '''
2846
lfs_unmount(&lfs) => 0;
2947
'''
3048

31-
3249
# reentrant format
3350
[cases.test_superblocks_reentrant_format]
3451
reentrant = true
@@ -135,6 +152,39 @@ code = '''
135152
lfs_unmount(&lfs) => 0;
136153
'''
137154

155+
# make sure the magic string "littlefs" is always at offset=8
156+
[cases.test_superblocks_magic_expand]
157+
defines.BLOCK_CYCLES = [32, 33, 1]
158+
defines.N = [10, 100, 1000]
159+
code = '''
160+
lfs_t lfs;
161+
lfs_format(&lfs, cfg) => 0;
162+
lfs_mount(&lfs, cfg) => 0;
163+
for (int i = 0; i < N; i++) {
164+
lfs_file_t file;
165+
lfs_file_open(&lfs, &file, "dummy",
166+
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
167+
lfs_file_close(&lfs, &file) => 0;
168+
struct lfs_info info;
169+
lfs_stat(&lfs, "dummy", &info) => 0;
170+
assert(strcmp(info.name, "dummy") == 0);
171+
assert(info.type == LFS_TYPE_REG);
172+
lfs_remove(&lfs, "dummy") => 0;
173+
}
174+
lfs_unmount(&lfs) => 0;
175+
176+
// check our magic string
177+
//
178+
// note if we lose power we may not have the magic string in both blocks!
179+
// but we don't lose power in this test so we can assert the magic string
180+
// is present in both
181+
uint8_t magic[lfs_max(16, READ_SIZE)];
182+
cfg->read(cfg, 0, 0, magic, lfs_max(16, READ_SIZE)) => 0;
183+
assert(memcmp(&magic[8], "littlefs", 8) == 0);
184+
cfg->read(cfg, 1, 0, magic, lfs_max(16, READ_SIZE)) => 0;
185+
assert(memcmp(&magic[8], "littlefs", 8) == 0);
186+
'''
187+
138188
# expanding superblock with power cycle
139189
[cases.test_superblocks_expand_power_cycle]
140190
defines.BLOCK_CYCLES = [32, 33, 1]
@@ -221,6 +271,7 @@ code = '''
221271
lfs_unmount(&lfs) => 0;
222272
'''
223273

274+
224275
# mount with unknown block_count
225276
[cases.test_superblocks_unknown_blocks]
226277
code = '''

0 commit comments

Comments
 (0)