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
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.
Curiously, the logic from 48bd2bf was incorrect, and would allow a
commit to be tried if erased _or_ dir->count was at risk of overflow.
That is clearly wrong, we should only try to commit if both conditions
are met...
Found again by dschendt
This matches the logic originally implemented in 48bd2bf, which was lost
during the big no-recursion refactor 84da4c0.
Other notes:
- Checking >= 0xff matches the split logic during compaction (line
2158):
end - split < 0xff
- Grouping dir->erased || dir->count >= 0xff together makes it clear
these share a common code path.
- Checking for dir->count >= 0xff early avoids committing >8-bit ids to
disk.
The cat may already be out-of-the bag on this one, but opening the id
space up to the full 10-bits should probably be on a non-patch
release.
Found by dschendt
If lfs_bd_read fails, lfs_fcrc_fromle32 will read uninitialized memory, and hasfcrc will be set to true.
This may end up in a "working" state later due to crcs not matching. but it's hard to follow if that woud be the case.
Add asserts on file system reads to make sure
no positive values are returned, which would
make assumptions on error checks invalid.
This fixes clang tidy warnings on uninitialized
reads in uses of lfs_dir_get where only negative
returns are considered errors.
Thanks to yamt, GCC-specific flags should now be disabled if compiling
with clang. Dropping the explicit flags also doubles as a test that the
NO_GCC inference works.
This is the same as the implicit Clang => NO_GCC behavior introduced by
yamt, but with an explicit variable that can be assigned by users using
other, non-gcc, compilers:
$ NO_GCC=1 make
Note, stack measurements are currently GCC specific:
$ NO_GCC=1 make stack
... snip ...
FileNotFoundError: [Errno 2] No such file or directory: 'lfs.ci'
make: *** [Makefile:494: lfs.stack.csv] Error 1
Long story short: There is a specific case where removing a directory
can trigger a deorphan pass, but lfs_remove did not check for this,
would try to clean up the (already cleaned) directory orphan, and
trigger an assert:
lfs.c:4890:assert: assert failed with false, expected eq true
LFS_ASSERT(lfs_tag_size(lfs->gstate.tag) > 0x000 || orphans >= 0);
The specific case being a remove commit that triggers a relocation that
creates an orphan.
This is also possible in lfs_rename, but only if you're renaming a
directory that implies a remove, which is a pretty rare operation.
---
This was probably an oversight introduced in the non-recursive commit
logic rework.
Fortunately the fix is to just check if we even have an orphan before
trying to remove it. We can rely on this instead of the file type, so
this fix shouldn't even increase the code size.
Found and root-caused by Hugh-Baoa
These are the same as the related reentrant variants, but by opting out
of powerloss testing, we can test a much larger number of states without
having to worry about the impact on powerloss testing runtime.
Bumped CYCLES from 20 -> 2000.
This reveals an orphan remove bug found by Hugh-Baoa.
This PR adds a new `lfs_fs_shrink`, which functions similarly to
`lfs_fs_grow`, but supports reducing the block count.
This functions first checks that none of the removed block are in use.
If it is the case, it will fail.
Christopher Haster