Merge tag 'v2.11.3'#33
Merged
Merged
Conversation
lfs_dir_fetch relies on an impossible tag match to avoid calling a NULL callback. Add an explicit cb != NULL check in the match path so future refactors don’t risk a NULL function-pointer call.
Remove NULL check from condition and assert callback is valid.
Multiple write handles in littlefs has always been a bit confusing (hopefully improving in littlefs3), but I didn't realize it could lead to corrupted data. The problem, as noted by Ictogan1, is that syncs to related file handles ignores the LFS_F_DIRTY flag. If you open a file twice, and write to one handle, littlefs doesn't realize the other handle it out-of-date. This may not seem like a problem, but then littlefs is happy to reallocate those (still referenced) blocks, leading to data corruption: open(a, "quiche.txt") write(a) sync(a) // syncs a's contents open(b, "quiche.txt") truncate(b) sync(b) // syncs b's contents write(b) // may allocate from a rewind(a) read(a) // potentially corrupted --- What we want is to set LFS_F_DIRTY in all other file handles during lfs_file_sync, but doing so would force those file handles to sync during close. That would be even more confusing (not to mention backwards incompatible). In theory setting both LFS_F_DIRTY + LFS_F_ERRED could work, but that would prevent implicit syncs of writes that haven't actually errored: open(a, "quiche.txt") write(a) open(b, "quiche.txt") write(b) close(b) // syncs b's contents close(a) // should sync a's contents So, instead, as a somewhat clunky workaround, a new flag: LFS_F_DUSTY, which indicates a file does not match storage, but should not be synced during close. --- It's worth noting this is already fixed in littlefs3, which includes a more rigorous, and hopefully easier to use sync model. But in the meantime, this should at least prevent the loss of data. Added test_alloc_multihandle and test_alloc_multihandle_reuse to prevent a regression, test_alloc_multihandle_reuse does reproduce the bug. Found and reproduced by Ictogan1
Fix comment typo and -> an
Fixes for 'Implicit conversion loses integer precision' warnings.
…/fix/emu-block-readme Fix broken link on README.md for emu device
lfs: guard null callbacks in lfs_dir_fetchmatch
…lti-whandle-corruption Fixed data corruption with multiple write handles
WalkthroughInternal file-state synchronization is enhanced by introducing a new ChangesMulti-Handle File Synchronization State
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Collaborator
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lfs.c`:
- Line 3703: The code currently clears both LFS_F_ERRED and LFS_F_DUSTY at the
first write (file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY;), which prematurely
removes the "dusty" protection; change it to only clear the error flag here
(leave LFS_F_DUSTY set) and ensure LFS_F_DUSTY is cleared later only after the
tail has been copied and committed (i.e., in lfs_file_flush() / lfs_file_sync_()
completion paths).
- Around line 3485-3495: The code in lfs_file_sync_ currently clears LFS_F_DUSTY
and marks sibling handles dusty unconditionally; change this so the
sibling-marking loop and the clearing of this handle's LFS_F_DUSTY only happen
when the handle actually committed a new version (i.e. when file->flags has
LFS_F_DIRTY). Concretely, in lfs_file_sync_ check if (file->flags & LFS_F_DIRTY)
before running the for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; ...) loop and
before clearing the LFS_F_DUSTY bit from file->flags; leave LFS_F_DUSTY
untouched if the handle was not dirty to avoid dropping its stale-dusty state.
Ensure you still preserve other flag updates (e.g. LFS_F_ERRED) as appropriate.
In `@tests/test_alloc.toml`:
- Around line 346-351: The test case [cases.test_alloc_multihandle_reuse] is
missing the defines.GC entry which other branches (lines referencing GC) depend
on; add a defines.GC define (e.g., defines.GC = [false, true]) to the block so
the generated test includes both GC variations and can exercise the reuse path
referenced by the GC branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f854eba6-331c-47a1-9b6b-93fbf30d84d9
📒 Files selected for processing (4)
README.mdlfs.clfs.htests/test_alloc.toml
Comment on lines
+3485
to
+3495
| // mark any other file handles as dirty + desync | ||
| for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next) { | ||
| if (file != f | ||
| && f->type == LFS_TYPE_REG | ||
| && lfs_pair_cmp(f->m.pair, file->m.pair) == 0 | ||
| && f->id == file->id) { | ||
| f->flags |= LFS_F_DUSTY; | ||
| } | ||
| } | ||
|
|
||
| file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY; |
There was a problem hiding this comment.
Only transfer LFS_F_DUSTY after this handle actually commits a new version.
A stale handle can reach this block with LFS_F_DUSTY set but LFS_F_DIRTY clear. In that case, lfs_file_sync_() currently clears its dusty bit and marks siblings dusty even though this handle still points at the orphaned CTZ chain. A later alloc scan/GC can then recycle blocks that this still-open handle may read from.
Suggested fix
static int lfs_file_sync_(lfs_t *lfs, lfs_file_t *file) {
int err = lfs_file_flush(lfs, file);
+ bool committed = false;
if (err) {
file->flags |= LFS_F_ERRED;
return err;
}
if ((file->flags & LFS_F_DIRTY) &&
!lfs_pair_isnull(file->m.pair)) {
@@
}
file->flags &= ~LFS_F_DIRTY;
+ committed = true;
}
- // mark any other file handles as dirty + desync
- for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next) {
- if (file != f
- && f->type == LFS_TYPE_REG
- && lfs_pair_cmp(f->m.pair, file->m.pair) == 0
- && f->id == file->id) {
- f->flags |= LFS_F_DUSTY;
+ if (committed) {
+ // mark any other file handles as desynced
+ for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next) {
+ if (file != f
+ && f->type == LFS_TYPE_REG
+ && lfs_pair_cmp(f->m.pair, file->m.pair) == 0
+ && f->id == file->id) {
+ f->flags |= LFS_F_DUSTY;
+ }
}
+ file->flags &= ~LFS_F_DUSTY;
}
- file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY;
+ file->flags &= ~LFS_F_ERRED;
return 0;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lfs.c` around lines 3485 - 3495, The code in lfs_file_sync_ currently clears
LFS_F_DUSTY and marks sibling handles dusty unconditionally; change this so the
sibling-marking loop and the clearing of this handle's LFS_F_DUSTY only happen
when the handle actually committed a new version (i.e. when file->flags has
LFS_F_DIRTY). Concretely, in lfs_file_sync_ check if (file->flags & LFS_F_DIRTY)
before running the for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; ...) loop and
before clearing the LFS_F_DUSTY bit from file->flags; leave LFS_F_DUSTY
untouched if the handle was not dirty to avoid dropping its stale-dusty state.
Ensure you still preserve other flag updates (e.g. LFS_F_ERRED) as appropriate.
| } | ||
|
|
||
| file->flags &= ~LFS_F_ERRED; | ||
| file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY; |
There was a problem hiding this comment.
Do not clear LFS_F_DUSTY on first write.
If a stale handle starts writing, it still depends on the old CTZ chain until lfs_file_flush() has copied the untouched tail and lfs_file_sync_() has committed the replacement. Clearing LFS_F_DUSTY here lets traversal stop protecting the old chain too early, so GC/allocation can reuse blocks before the flush finishes.
Suggested fix
- file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY;
+ file->flags &= ~LFS_F_ERRED;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY; | |
| file->flags &= ~LFS_F_ERRED; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lfs.c` at line 3703, The code currently clears both LFS_F_ERRED and
LFS_F_DUSTY at the first write (file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY;),
which prematurely removes the "dusty" protection; change it to only clear the
error flag here (leave LFS_F_DUSTY set) and ensure LFS_F_DUSTY is cleared later
only after the tail has been copied and committed (i.e., in lfs_file_flush() /
lfs_file_sync_() completion paths).
Comment on lines
+346
to
+351
| [cases.test_alloc_multihandle_reuse] | ||
| defines.FILES = 2 | ||
| defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / (FILES+1))' | ||
| defines.CYCLES = [1, 10] | ||
| defines.INFER_BC = [false, true] | ||
| defines.SYNC = [false, true] |
There was a problem hiding this comment.
Add the missing GC test define.
Lines 371 and 388 branch on GC, but this case never declares defines.GC. That makes the generated test invalid before it can exercise the reuse path.
Suggested fix
[cases.test_alloc_multihandle_reuse]
defines.FILES = 2
defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / (FILES+1))'
defines.CYCLES = [1, 10]
defines.INFER_BC = [false, true]
+defines.GC = [false, true]
defines.SYNC = [false, true]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [cases.test_alloc_multihandle_reuse] | |
| defines.FILES = 2 | |
| defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / (FILES+1))' | |
| defines.CYCLES = [1, 10] | |
| defines.INFER_BC = [false, true] | |
| defines.SYNC = [false, true] | |
| [cases.test_alloc_multihandle_reuse] | |
| defines.FILES = 2 | |
| defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / (FILES+1))' | |
| defines.CYCLES = [1, 10] | |
| defines.INFER_BC = [false, true] | |
| defines.GC = [false, true] | |
| defines.SYNC = [false, true] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_alloc.toml` around lines 346 - 351, The test case
[cases.test_alloc_multihandle_reuse] is missing the defines.GC entry which other
branches (lines referencing GC) depend on; add a defines.GC define (e.g.,
defines.GC = [false, true]) to the block so the generated test includes both GC
variations and can exercise the reuse path referenced by the GC branches.
Rbb666
approved these changes
May 4, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
v2.11.3
Latest6cb4e86littlefs3 status: in-progress, unstable (update, now with plots!).
littlefs2 status: feature freeze.
Notable fix: This includes a fix for a bug found by @Ictogan1, where multiple open write handles could lead to data corruption (#1194).
| Code | Stack | Structs | | Coverage -- | -- | -- | -- | -- | -- Default | 17200 B (+0.4%) | 1448 B (+0.0%) | 812 B (+0.0%) | Lines | 2449/2610 lines (+0.0%) Readonly | 6234 B (+0.0%) | 448 B (+0.0%) | 812 B (+0.0%) | Branches | 1301/1638 branches (+0.1%) Threadsafe | 18056 B (+0.4%) | 1448 B (+0.0%) | 820 B (+0.0%) | | Benchmarks Multiversion | 17272 B (+0.4%) | 1448 B (+0.0%) | 816 B (+0.0%) | Readed | 29000746676 B (+0.0%) Migrate | 18864 B (+0.4%) | 1752 B (+0.0%) | 816 B (+0.0%) | Proged | 1482895246 B (+0.0%) Error-asserts | 18036 B (+0.5%) | 1440 B (+0.0%) | 812 B (+0.0%) | Erased | 1568921600 B (+0.0%)488e84bbFixed data corruption with multiple write handlesfd5e7f62Using LFS_ASSERT instead of an runtime check.2311cd78Guard null callbacks in lfs_dir_fetchmatch74f32c83Fix broken link on README.md for emu devicedbe96d03Fixes for 'Implicit conversion loses integer precision' warnings.b062c88aFix comment typo and -> anA special thanks to littlefs's sponsors: @micropython, @gurgalof
Notable fix: This includes a fix for a bug found by @Ictogan1, where multiple open write handles could lead to data corruption (littlefs-project#1194).
Code Stack Structs Coverage
Default 17200 B (+0.4%) 1448 B (+0.0%) 812 B (+0.0%) Lines 2449/2610 lines (+0.0%)
Readonly 6234 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1301/1638 branches (+0.1%)
Threadsafe 18056 B (+0.4%) 1448 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17272 B (+0.4%) 1448 B (+0.0%) 816 B (+0.0%) Readed 29000746676 B (+0.0%)
Migrate 18864 B (+0.4%) 1752 B (+0.0%) 816 B (+0.0%) Proged 1482895246 B (+0.0%)
Error-asserts 18036 B (+0.5%) 1440 B (+0.0%) 812 B (+0.0%) Erased 1568921600 B (+0.0%)
488e84bb Fixed data corruption with multiple write handles
fd5e7f62 Using LFS_ASSERT instead of an runtime check.
2311cd78 Guard null callbacks in lfs_dir_fetchmatch
74f32c83 Fix broken link on README.md for emu device
dbe96d03 Fixes for 'Implicit conversion loses integer precision' warnings.
b062c88a Fix comment typo and -> an
A special thanks to littlefs's sponsors: https://github.com/micropython, @gurgalof
Summary by CodeRabbit
Bug Fixes
Tests
Documentation