Infer block_count from superblock if not provided in config.

[BrianPugh]

This pull request attempts to autodetect the block_count from the superblock if not provided in lfs_config (i.e. cfg->block_count == 0).

This PR slightly increases RAM usage by increasing the lfs_t struct by 4 bytes.

~I think this PR could also negatively impact mounting performance (I refactored it so that the superblock search and the gstate search are independent), but I think those are super fast anyways. A possible organization improvement would be to abstract the traversal to another function, but that might reduce readability. We can leave that for another PR.~

~@geky currently this is set as a draft as I want to write more unit tests, but need some advice on how to debug. Currently one of the test permutations hangs; I don't quite understand what the permutations are or how to debug.~

Related issues & PRs: #865, #753

For ease of review, the biggest commit (73285278b98e0b15a608325b3f7e4c5b8e0c6b75) just moves the identical traversal into 2 functions (superblock, gstate), and moves the superblock validation to a separate function. There's no real logic change in that commit, except that the superblock is found first, then the gstate is updated (rather than at the same time).

[BrianPugh]

@geky I think this is ready now!

[geky-bot]

Tests passed ✓, Code: 16898 B (+1.4%), Stack: 1432 B (+0.0%), Structs: 792 B (+0.5%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16898 B (+1.4%) | 1432 B (+0.0%) | 792 B (+0.5%) | Lines | 2344/2523 lines (+0.1%) | | Readonly | 6298 B (+2.8%) | 448 B (+0.0%) | 792 B (+0.5%) | Branches | 1210/1536 branches (+0.2%) | | Threadsafe | 17730 B (+1.3%) | 1432 B (+0.0%) | 800 B (+0.5%) | | **Benchmarks** | | Multiversion | 16958 B (+1.3%) | 1432 B (+0.0%) | 796 B (+0.5%) | Readed | 29370160748 B (+0.0%) | | Migrate | 18590 B (+1.3%) | 1736 B (+0.0%) | 796 B (+0.5%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17538 B (+1.4%) | 1424 B (+0.0%) | 792 B (+0.5%) | Erased | 1568888832 B (+0.0%) |

[BrianPugh]

what are our thoughts on this? It's less invasive than the other PRs. It's also a backwards compatible change. Any future changes (such as if we allow lfs_config_t to be mutable) are compatible with this PR. In that example, we'd just remove the newly introduced lfs->block_count field and have all the pointers point back to the now mutable lfs->config->block_count.

I'm looking forward to some form of block_count autodetection so that I can publish my cli tool that manages littlefs partitions to PyPI.

[geky]

Hi @BrianPugh, thanks for this. I left some feedback.

I'm not sure fetching the block_count from disk during lfs_format makes sense. I left an in-code comment about that.

I think we should also add block_count (and maybe block_size for completeness) to lfs_fs_stat, so the block_count is available to the user after mounting. This should just mean another assignment in lfs_fs_rawstat: here.

I'm also curious if the test_superblocks_fewer_blocks and test_superblocks_more_blocks tests from #753 can be ported over to this. They provide coverage for a few more corner cases, and should cover changes to lfs_fs_stat well enough.

---

I'm also thinking I'll try to get lfs_fs_grow (#753, #702) in on the same minor release as this, since they are closely related. Though that doesn't affect this PR.

[geky]

@geky currently this is set as a draft as I want to write more unit tests, but need some advice on how to debug. Currently one of the test permutations hangs; I don't quite understand what the permutations are or how to debug.

Were you able to figure out debugging with the test framework? It's unfortunately a bit dense not documented well.

If a test permutation fails in CI:

tests/test_compat.toml:1296:failure: test_compat_major_incompat:1g12gg2 (PROG_SIZE=16, BLOCK_SIZE=512) failed 

You should be able to rerun it locally like so:

# build the test runner
$ make test-runner -j
# run the failing test
$ ./scripts/test.py runners/test_runner test_compat_major_incompat:1g12gg2

Though for the Valgrind tests you need to pass the valgrind flag:

$ ./scripts/test.py runners/test_runner test_compat_major_incompat:1g12gg2 --valgrind

Note the test framework expects quite a bit more from the dev environment than littlefs itself. It's unlikely to work with a non-GCC/Clang compiler...

[BrianPugh]

I'll add the lfs_fs_stat functionality and investigate the unit tests you recommended

[geky-bot]

Tests passed ✓, Code: 16898 B (+1.4%), Stack: 1432 B (+0.0%), Structs: 792 B (+0.5%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16898 B (+1.4%) | 1432 B (+0.0%) | 792 B (+0.5%) | Lines | 2345/2524 lines (+0.1%) | | Readonly | 6298 B (+2.8%) | 448 B (+0.0%) | 792 B (+0.5%) | Branches | 1210/1536 branches (+0.2%) | | Threadsafe | 17730 B (+1.3%) | 1432 B (+0.0%) | 800 B (+0.5%) | | **Benchmarks** | | Multiversion | 16958 B (+1.3%) | 1432 B (+0.0%) | 796 B (+0.5%) | Readed | 29370160748 B (+0.0%) | | Migrate | 18574 B (+1.2%) | 1736 B (+0.0%) | 796 B (+0.5%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17538 B (+1.4%) | 1424 B (+0.0%) | 792 B (+0.5%) | Erased | 1568888832 B (+0.0%) |

[geky-bot]

Tests passed ✓, Code: 16906 B (+1.4%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16906 B (+1.4%) | 1432 B (+0.0%) | 800 B (+1.5%) | Lines | 2347/2526 lines (+0.1%) | | Readonly | 6306 B (+2.9%) | 448 B (+0.0%) | 800 B (+1.5%) | Branches | 1210/1536 branches (+0.2%) | | Threadsafe | 17738 B (+1.4%) | 1432 B (+0.0%) | 808 B (+1.5%) | | **Benchmarks** | | Multiversion | 16966 B (+1.3%) | 1432 B (+0.0%) | 804 B (+1.5%) | Readed | 29370160748 B (+0.0%) | | Migrate | 18582 B (+1.3%) | 1736 B (+0.0%) | 804 B (+1.5%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17546 B (+1.4%) | 1424 B (+0.0%) | 800 B (+1.5%) | Erased | 1568888832 B (+0.0%) |

[BrianPugh]

@geky-bot copying over the tests from #753 ALMOST works, but it would also involve copying over all the runner and related changes. I feel like that might balloon this PR and make a bunch of unnecessary merge conflicts. Waiting for your advice before performing more actions.

Since your last review I:

• Updated lfs_fs_stat along with the related struct fields.
• Fixed linting.

[geky-bot]

Tests passed ✓, Code: 16794 B (+0.8%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16794 B (+0.8%) | 1432 B (+0.0%) | 800 B (+1.5%) | Lines | 2341/2518 lines (+0.2%) | | Readonly | 6306 B (+2.9%) | 448 B (+0.0%) | 800 B (+1.5%) | Branches | 1206/1532 branches (+0.1%) | | Threadsafe | 17634 B (+0.8%) | 1432 B (+0.0%) | 808 B (+1.5%) | | **Benchmarks** | | Multiversion | 16854 B (+0.7%) | 1432 B (+0.0%) | 804 B (+1.5%) | Readed | 29370160748 B (+0.0%) | | Migrate | 18470 B (+0.7%) | 1736 B (+0.0%) | 804 B (+1.5%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17450 B (+0.9%) | 1424 B (+0.0%) | 800 B (+1.5%) | Erased | 1568888832 B (+0.0%) |

[BrianPugh]

so whats the conclusion on this? Does the PR seem good? Waiting to bundle it with other PRs for a release?

[geky]

This is looking good, thanks for the updates. I'm currently planning to bring this in on the next minor release.

I'm still wanting to look into the extra tests in https://github.com/littlefs-project/littlefs/pull/753, which as you point likely come with baggage, and lfs_fs_grow (https://github.com/littlefs-project/littlefs/pull/753, https://github.com/littlefs-project/littlefs/pull/702) since it is closely related. But I think these would be more appropriate in separate PRs.

[geky]

I've gone ahead and reverted the lfs_scan_for_superblocks and lfs_scan_for_state_updates refactor. The new code path would have led to scanning the superblock chain twice, when we only need to scan it once.

Let me know if you see anything wrong with this.

[geky-bot]

Tests passed ✓, Code: 16674 B (-0.0%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16674 B (-0.0%) | 1432 B (+0.0%) | 800 B (+1.5%) | Lines | 2319/2497 lines (+0.1%) | | Readonly | 6130 B (+0.1%) | 448 B (+0.0%) | 800 B (+1.5%) | Branches | 1193/1516 branches (+0.1%) | | Threadsafe | 17506 B (+0.0%) | 1432 B (+0.0%) | 808 B (+1.5%) | | **Benchmarks** | | Multiversion | 16734 B (-0.1%) | 1432 B (+0.0%) | 804 B (+1.5%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18350 B (-0.1%) | 1736 B (+0.0%) | 804 B (+1.5%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17330 B (+0.1%) | 1424 B (+0.0%) | 800 B (+1.5%) | Erased | 1568888832 B (+0.0%) |

[BrianPugh]

@geky that's fine by me since we no longer invoke lfs_scan_for_superblocks at multiple places 👍 . It also does make this PR a lot more terse :D

[geky]

I've also gone ahead and adopted the block-device/runner changes in https://github.com/littlefs-project/littlefs/pull/753. I realize this is probably pushing the scope of this PR, but it lets us adopt the tests in https://github.com/littlefs-project/littlefs/pull/753, and is probably a good long-term change.

I also tweaked lfs_fsinfo a bit, mainly to make the struct fields follow the order found in the superblock for consistency. I realize this is really nitpicky.

If you're ok with these changes this looks good to me for the next minor release.

[geky]

Also the now-dependent lfs_fs_grow API is here: https://github.com/littlefs-project/littlefs/pull/872

[geky-bot]

Tests passed ✓, Code: 16686 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)

| | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 16686 B (+0.0%) | 1432 B (+0.0%) | 800 B (+1.5%) | Lines | 2323/2498 lines (+0.2%) | | Readonly | 6130 B (+0.1%) | 448 B (+0.0%) | 800 B (+1.5%) | Branches | 1194/1516 branches (+0.2%) | | Threadsafe | 17518 B (+0.1%) | 1432 B (+0.0%) | 808 B (+1.5%) | | **Benchmarks** | | Multiversion | 16746 B (-0.0%) | 1432 B (+0.0%) | 804 B (+1.5%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18362 B (+0.0%) | 1736 B (+0.0%) | 804 B (+1.5%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17338 B (+0.2%) | 1424 B (+0.0%) | 800 B (+1.5%) | Erased | 1568888832 B (+0.0%) |

[BrianPugh]

all seems good to me!

[geky]

This is on master now, thanks for the PR!

[geky]

Whoops, meant to respond to this one: https://github.com/littlefs-project/littlefs/pull/886. Well this is technically on master as well so...