Infer block_count from superblock if not provided in config. #866
Conversation
Some block-device bound-checks are disabled during superblock search.
|
@geky I think this is ready now! |
BrianPugh
force-pushed
the
optional-block-count
branch
from
a77ed04
to
7521e0a
Compare
Tests passed ✓, Code: 16898 B (+1.4%), Stack: 1432 B (+0.0%), Structs: 792 B (+0.5%)
|
|
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 I'm looking forward to some form of |
geky
added
enhancement
needs minor version
|
Hi @BrianPugh, thanks for this. I left some feedback. I'm not sure fetching the I think we should also add I'm also curious if the I'm also thinking I'll try to get |
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: 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:1g12gg2Though for the Valgrind tests you need to pass the valgrind flag: $ ./scripts/test.py runners/test_runner test_compat_major_incompat:1g12gg2 --valgrindNote 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... |
|
I'll add the |
Tests passed ✓, Code: 16898 B (+1.4%), Stack: 1432 B (+0.0%), Structs: 792 B (+0.5%)
|
Tests passed ✓, Code: 16906 B (+1.4%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)
|
|
@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:
|
Tests passed ✓, Code: 16794 B (+0.8%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)
|
|
so whats the conclusion on this? Does the PR seem good? Waiting to bundle it with other PRs for a release? |
|
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 #753, which as you point likely come with baggage, and |
This would result in two passes through the superblock chain during mount, when we can access everything we need to in one.
|
I've gone ahead and reverted the Let me know if you see anything wrong with this. |
Tests passed ✓, Code: 16674 B (-0.0%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)
|
|
@geky that's fine by me since we no longer invoke |
In separating the configuration of littlefs from the physical geometry of the underlying device, we can no longer rely solely on lfs_config to contain all of the information necessary for the simulated block devices we use for testing. This adds a new lfs_*bd_config struct for each of the block devices, and new erase_size/erase_count fields. The erase_* name was chosen since these reflect the (simulated) physical erase size and count of erase-sized blocks, unlike the block_* variants which represent logical block sizes used for littlefs's bookkeeping. It may be worth adopting erase_size/erase_count in littlefs's config at some point in the future, but at the moment doesn't seem necessary. Changing the lfs_bd_config structs to be required is probably a good idea anyways, as it moves us more towards separating the bds from littlefs. Though we can't quite get rid of the lfs_config parameter because of the block-device API in lfs_config. Eventually it would be nice to get rid of it, but that would require API breakage.
geky
force-pushed
the
optional-block-count
branch
from
d055bfb
to
7237596
Compare
|
I've also gone ahead and adopted the block-device/runner changes in #753. I realize this is probably pushing the scope of this PR, but it lets us adopt the tests in #753, and is probably a good long-term change. I also tweaked If you're ok with these changes this looks good to me for the next minor release. |
geky
added
next minor
and removed
needs minor version
|
Also the now-dependent |
These were cherry-picked from some previous work on a related feature.
geky
force-pushed
the
optional-block-count
branch
from
7237596
to
b28d4c9
Compare
Mainly to match superblock ordering and emphasize these are logical blocks.
geky
force-pushed
the
optional-block-count
branch
from
b28d4c9
to
2c222af
Compare
Tests passed ✓, Code: 16686 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 800 B (+1.5%)
|
|
all seems good to me! |
|
This is on master now, thanks for the PR! |
|
Whoops, meant to respond to this one: #886. Well this is technically on master as well so... |
This pull request attempts to autodetect the
block_countfrom the superblock if not provided inlfs_config(i.e.cfg->block_count == 0).This PR slightly increases RAM usage by increasing the
lfs_tstruct 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 (7328527) 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).