Fixed data corruption with multiple write handles
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
This commit is contained in:
@@ -3244,10 +3244,12 @@ static int lfs_file_open_(lfs_t *lfs, lfs_file_t *file,
|
||||
#endif
|
||||
|
||||
static int lfs_file_close_(lfs_t *lfs, lfs_file_t *file) {
|
||||
#ifndef LFS_READONLY
|
||||
int err = lfs_file_sync_(lfs, file);
|
||||
#else
|
||||
int err = 0;
|
||||
#ifndef LFS_READONLY
|
||||
// it's not safe to do anything if our file errored
|
||||
if (!(file->flags & LFS_F_ERRED)) {
|
||||
err = lfs_file_sync_(lfs, file);
|
||||
}
|
||||
#endif
|
||||
|
||||
// remove from list of mdirs
|
||||
@@ -3429,18 +3431,12 @@ relocate:
|
||||
|
||||
#ifndef LFS_READONLY
|
||||
static int lfs_file_sync_(lfs_t *lfs, lfs_file_t *file) {
|
||||
if (file->flags & LFS_F_ERRED) {
|
||||
// it's not safe to do anything if our file errored
|
||||
return 0;
|
||||
}
|
||||
|
||||
int err = lfs_file_flush(lfs, file);
|
||||
if (err) {
|
||||
file->flags |= LFS_F_ERRED;
|
||||
return err;
|
||||
}
|
||||
|
||||
|
||||
if ((file->flags & LFS_F_DIRTY) &&
|
||||
!lfs_pair_isnull(file->m.pair)) {
|
||||
// before we commit metadata, we need sync the disk to make sure
|
||||
@@ -3485,6 +3481,17 @@ static int lfs_file_sync_(lfs_t *lfs, lfs_file_t *file) {
|
||||
file->flags &= ~LFS_F_DIRTY;
|
||||
}
|
||||
|
||||
// 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;
|
||||
return 0;
|
||||
}
|
||||
#endif
|
||||
@@ -3692,7 +3699,7 @@ static lfs_ssize_t lfs_file_write_(lfs_t *lfs, lfs_file_t *file,
|
||||
return nsize;
|
||||
}
|
||||
|
||||
file->flags &= ~LFS_F_ERRED;
|
||||
file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY;
|
||||
return nsize;
|
||||
}
|
||||
#endif
|
||||
@@ -4771,7 +4778,8 @@ int lfs_fs_traverse_(lfs_t *lfs,
|
||||
continue;
|
||||
}
|
||||
|
||||
if ((f->flags & LFS_F_DIRTY) && !(f->flags & LFS_F_INLINE)) {
|
||||
if (((f->flags & LFS_F_DIRTY) || (f->flags & LFS_F_DUSTY))
|
||||
&& !(f->flags & LFS_F_INLINE)) {
|
||||
int err = lfs_ctz_traverse(lfs, &f->cache, &lfs->rcache,
|
||||
f->ctz.head, f->ctz.size, cb, data);
|
||||
if (err) {
|
||||
|
||||
Reference in New Issue
Block a user