Fix metadata_max==prog_size commit->end calculation #1031
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added METADATA_MAX to test_runner. - Added METADATA_MAX to bench_runner. - Added a simple metadata_max test to test_superblocks, for lack of better location. There have been several issues floating around related to metadata_max and LFS_ERR_NOSPC which makes me think there's a bug in our metadata_max logic. metadata_max was a quick patch and is relatively untested, so an undetected bug isn't too surprising. This commit adds at least some testing over metadata_max. Sure enough, the new test_superblocks_metadata_max test reveals a curious LFS_ERR_NAMETOOLONG error that shouldn't be there. More investigation needed.
The inconsistency here between the use of block_size vs metadata_max was suspicious. Turns out there's a bug when metadata_max == prog_size. We correctly use metadata_max for the block_size/2 check, but we weren't using it for the block_size-40 check. The second check seems unnecessary after the first, but it protects against running out of space in a commit for commit-related metadata (checksums, tail pointers, etc) when we can't program half-blocks. Turns out this is also needed when limiting metadata_max to a single prog, otherwise we risk erroring with LFS_ERR_NOSPC early. Found by ajheck, dpkristensen, NLLK, and likely others.
Like the read/prog/block_size checks, these are just asserts. If these invariants are broken the filesystem will break in surprising ways.
This was referenced Oct 4, 2024
Tests passed ✓, Code: 17068 B, Stack: 1440 B, Structs: 812 B
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've been trying to dig into some of the early LFS_ERR_NOSPC errors users have been seeing. Some of the problem is that littlefs returns LFS_ERR_NOSPC for both "no space" errors, and internally whenever metadata can't fit in a block/mdir. This has made tracking down the actual root cause a bit tricky.
I did at least find one bug: In mdir compaction we weren't correctly adjusting
commit->endto reserve space for commit-related metadata (checksums, tail pointers, etc) whenmetadata_max==prog_size.This went unnoticed due to a lack of testing and requiring
metadata_max==prog_size.Added a quick test to prevent regression, and also added a few more asserts to
lfs_initrelated to possiblemetadata_maxconfiguration mistakes.Found by @ajheck, @dpkristensen, @NLLK, and likely others.
Related #755, #896