Fix allocation-eviction issue when erase state is multiple of block_cycles+1 #490
Conversation
This file contains hidden or 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
…_cycles+1 This rather interesting corner-case arises in lfs_dir_alloc anytime the uninitialized revision count happens to be a multiple of block_cycles+1. For example, the source of the bug found by tim-nordell-nimbelink: rev = 2742492087 block_cycles = 100 2742492087 % (100+1) = 0 The reason for this weird block_cycles+1 case is due to a fix for a previous bug in fe957de. To avoid aliasing, which would cause metadata pairs to wear unevenly, block_cycles incremented to the next odd number. Normally, littlefs tweaks the revision count of blocks during lfs_dir_alloc in order to make sure evictions can't happen on the first compact. Otherwise, higher-level logic such as lfs_format would break. However, this wasn't updated with the aliasing fix in fe957de, so lfs_dir_alloc was only rounding the revision count to the nearest even number. The current fix is to change the logic in lfs_dir_alloc to explicitly check for the eviction condition and increment if eviction would occur. Found by tim-nordell-nimbelink
Previously we only bumped the revision count if an eviction would occur immediately (and possibly corrupt littlefs). This works, but does risk an unoptimal superblock size if an almost-exhausted superblock was allocated during lfs_format. As pointed out by tim-nordell-nimbelink, we can align the revision count to maximize the number of block cycles without breaking the existing requirements of increasing revision counts. As an added benefit, littlefs's wear-leveling should behave more consistently after this change.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
See #489 for more information.
This rather interesting corner-case arises in lfs_dir_alloc anytime the uninitialized revision count happens to be a multiple of block_cycles+1.
For example, the source of the bug found by tim-nordell-nimbelink:
The reason for this weird block_cycles+1 case is due to a fix for a previous bug in fe957de. To avoid aliasing, which would cause metadata pairs to wear unevenly, block_cycles incremented to the next odd number.
Normally, littlefs tweaks the revision count of blocks during lfs_dir_alloc in order to make sure evictions can't happen on the first compact. Otherwise, higher-level logic such as lfs_format would break.
However, this wasn't updated with the aliasing fix in fe957de, so lfs_dir_alloc was only rounding the revision count to the nearest even number.
The current fix is to
change the logic in lfs_dir_alloc to explicitly check for the eviction condition and increment if eviction would occur.Align to the nearest multiple of the eviction condition (see #369)Should help #489
Related #369
Found by @tim-nordell-nimbelink