Skip to content

Commit 6cb4e86

Browse files
authored
Merge pull request #1194 from littlefs-project/fix-multi-whandle-corruption
Fixed data corruption with multiple write handles
2 parents d3375f1 + 488e84b commit 6cb4e86

3 files changed

Lines changed: 208 additions & 16 deletions

File tree

lfs.c

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3245,10 +3245,12 @@ static int lfs_file_open_(lfs_t *lfs, lfs_file_t *file,
32453245
#endif
32463246

32473247
static int lfs_file_close_(lfs_t *lfs, lfs_file_t *file) {
3248-
#ifndef LFS_READONLY
3249-
int err = lfs_file_sync_(lfs, file);
3250-
#else
32513248
int err = 0;
3249+
#ifndef LFS_READONLY
3250+
// it's not safe to do anything if our file errored
3251+
if (!(file->flags & LFS_F_ERRED)) {
3252+
err = lfs_file_sync_(lfs, file);
3253+
}
32523254
#endif
32533255

32543256
// remove from list of mdirs
@@ -3430,18 +3432,12 @@ static int lfs_file_flush(lfs_t *lfs, lfs_file_t *file) {
34303432

34313433
#ifndef LFS_READONLY
34323434
static int lfs_file_sync_(lfs_t *lfs, lfs_file_t *file) {
3433-
if (file->flags & LFS_F_ERRED) {
3434-
// it's not safe to do anything if our file errored
3435-
return 0;
3436-
}
3437-
34383435
int err = lfs_file_flush(lfs, file);
34393436
if (err) {
34403437
file->flags |= LFS_F_ERRED;
34413438
return err;
34423439
}
34433440

3444-
34453441
if ((file->flags & LFS_F_DIRTY) &&
34463442
!lfs_pair_isnull(file->m.pair)) {
34473443
// before we commit metadata, we need sync the disk to make sure
@@ -3486,6 +3482,17 @@ static int lfs_file_sync_(lfs_t *lfs, lfs_file_t *file) {
34863482
file->flags &= ~LFS_F_DIRTY;
34873483
}
34883484

3485+
// mark any other file handles as dirty + desync
3486+
for (lfs_file_t *f = (lfs_file_t*)lfs->mlist; f; f = f->next) {
3487+
if (file != f
3488+
&& f->type == LFS_TYPE_REG
3489+
&& lfs_pair_cmp(f->m.pair, file->m.pair) == 0
3490+
&& f->id == file->id) {
3491+
f->flags |= LFS_F_DUSTY;
3492+
}
3493+
}
3494+
3495+
file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY;
34893496
return 0;
34903497
}
34913498
#endif
@@ -3693,7 +3700,7 @@ static lfs_ssize_t lfs_file_write_(lfs_t *lfs, lfs_file_t *file,
36933700
return nsize;
36943701
}
36953702

3696-
file->flags &= ~LFS_F_ERRED;
3703+
file->flags &= ~LFS_F_ERRED & ~LFS_F_DUSTY;
36973704
return nsize;
36983705
}
36993706
#endif
@@ -4772,7 +4779,8 @@ int lfs_fs_traverse_(lfs_t *lfs,
47724779
continue;
47734780
}
47744781

4775-
if ((f->flags & LFS_F_DIRTY) && !(f->flags & LFS_F_INLINE)) {
4782+
if (((f->flags & LFS_F_DIRTY) || (f->flags & LFS_F_DUSTY))
4783+
&& !(f->flags & LFS_F_INLINE)) {
47764784
int err = lfs_ctz_traverse(lfs, &f->cache, &lfs->rcache,
47774785
f->ctz.head, f->ctz.size, cb, data);
47784786
if (err) {

lfs.h

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,15 @@ enum lfs_open_flags {
135135

136136
// internally used flags
137137
#ifndef LFS_READONLY
138-
LFS_F_DIRTY = 0x010000, // File does not match storage
139-
LFS_F_WRITING = 0x020000, // File has been written since last flush
138+
LFS_F_DIRTY = 0x00010000, // File does not match storage due to write
139+
LFS_F_DUSTY = 0x00020000, // File does not match storage due to desync
140+
LFS_F_WRITING = 0x00040000, // File has been written since last flush
140141
#endif
141-
LFS_F_READING = 0x040000, // File has been read since last flush
142+
LFS_F_READING = 0x00080000, // File has been read since last flush
142143
#ifndef LFS_READONLY
143-
LFS_F_ERRED = 0x080000, // An error occurred during write
144+
LFS_F_ERRED = 0x00100000, // An error occurred during write
144145
#endif
145-
LFS_F_INLINE = 0x100000, // Currently inlined in directory entry
146+
LFS_F_INLINE = 0x01000000, // Currently inlined in directory entry
146147
};
147148

148149
// File seek flags

tests/test_alloc.toml

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,189 @@ code = '''
250250
}
251251
'''
252252

253+
# multiple handle allocation test
254+
#
255+
# this tests that multiple open handles to the same file don't clobber
256+
# each other
257+
[cases.test_alloc_multihandle]
258+
defines.FILES = 2
259+
defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / FILES)'
260+
defines.GC = [false, true]
261+
defines.COMPACT_THRESH = ['-1', '0', 'BLOCK_SIZE/2']
262+
defines.INFER_BC = [false, true]
263+
defines.SYNC = [false, true]
264+
code = '''
265+
const char *names[] = {"eggs", "spinach"};
266+
lfs_file_t files[FILES];
267+
268+
lfs_t lfs;
269+
lfs_format(&lfs, cfg) => 0;
270+
struct lfs_config cfg_ = *cfg;
271+
if (INFER_BC) {
272+
cfg_.block_count = 0;
273+
}
274+
lfs_mount(&lfs, &cfg_) => 0;
275+
lfs_mkdir(&lfs, "breakfast") => 0;
276+
277+
// write one file
278+
char path[1024];
279+
sprintf(path, "breakfast/quiche");
280+
lfs_file_open(&lfs, &files[0], path,
281+
LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
282+
if (GC) {
283+
lfs_fs_gc(&lfs) => 0;
284+
}
285+
size_t size = strlen(names[0]);
286+
for (lfs_size_t i = 0; i < SIZE; i += size) {
287+
lfs_file_write(&lfs, &files[0], names[0], size) => size;
288+
}
289+
// sync?
290+
if (SYNC) {
291+
lfs_file_sync(&lfs, &files[0]) => 0;
292+
}
293+
294+
// write the other file
295+
sprintf(path, "breakfast/quiche");
296+
lfs_file_open(&lfs, &files[1], path,
297+
LFS_O_RDWR | LFS_O_CREAT | LFS_O_TRUNC) => 0;
298+
if (GC) {
299+
lfs_fs_gc(&lfs) => 0;
300+
}
301+
size = strlen(names[1]);
302+
for (lfs_size_t i = 0; i < SIZE; i += size) {
303+
lfs_file_write(&lfs, &files[1], names[1], size) => size;
304+
}
305+
// sync?
306+
if (SYNC) {
307+
lfs_file_sync(&lfs, &files[1]) => 0;
308+
}
309+
310+
// try to read from both
311+
for (int n = 0; n < FILES; n++) {
312+
lfs_file_rewind(&lfs, &files[n]) => 0;
313+
size_t size = strlen(names[n]);
314+
for (lfs_size_t i = 0; i < SIZE; i += size) {
315+
uint8_t buffer[1024];
316+
lfs_file_read(&lfs, &files[n], buffer, size) => size;
317+
assert(memcmp(buffer, names[n], size) == 0);
318+
}
319+
}
320+
for (int n = 0; n < FILES; n++) {
321+
lfs_file_close(&lfs, &files[n]) => 0;
322+
}
323+
lfs_unmount(&lfs) => 0;
324+
325+
// check after remounting
326+
lfs_mount(&lfs, &cfg_) => 0;
327+
{
328+
// last one wins
329+
int n = FILES-1;
330+
char path[1024];
331+
sprintf(path, "breakfast/quiche");
332+
lfs_file_t file;
333+
lfs_file_open(&lfs, &file, path, LFS_O_RDONLY) => 0;
334+
size_t size = strlen(names[n]);
335+
for (lfs_size_t i = 0; i < SIZE; i += size) {
336+
uint8_t buffer[1024];
337+
lfs_file_read(&lfs, &file, buffer, size) => size;
338+
assert(memcmp(buffer, names[n], size) == 0);
339+
}
340+
lfs_file_close(&lfs, &file) => 0;
341+
}
342+
lfs_unmount(&lfs) => 0;
343+
'''
344+
345+
# multiple handle allocation reuse test
346+
[cases.test_alloc_multihandle_reuse]
347+
defines.FILES = 2
348+
defines.SIZE = '(((BLOCK_SIZE-8)*(BLOCK_COUNT-6)) / (FILES+1))'
349+
defines.CYCLES = [1, 10]
350+
defines.INFER_BC = [false, true]
351+
defines.SYNC = [false, true]
352+
code = '''
353+
const char *names[] = {"eggs", "spinach"};
354+
lfs_file_t files[FILES];
355+
356+
lfs_t lfs;
357+
lfs_format(&lfs, cfg) => 0;
358+
struct lfs_config cfg_ = *cfg;
359+
if (INFER_BC) {
360+
cfg_.block_count = 0;
361+
}
362+
363+
lfs_mount(&lfs, &cfg_) => 0;
364+
lfs_mkdir(&lfs, "breakfast") => 0;
365+
366+
// write one file
367+
char path[1024];
368+
sprintf(path, "breakfast/quiche");
369+
lfs_file_open(&lfs, &files[0], path,
370+
LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
371+
if (GC) {
372+
lfs_fs_gc(&lfs) => 0;
373+
}
374+
size_t size = strlen(names[0]);
375+
for (lfs_size_t i = 0; i < SIZE; i += size) {
376+
lfs_file_write(&lfs, &files[0], names[0], size) => size;
377+
}
378+
// sync?
379+
if (SYNC) {
380+
lfs_file_sync(&lfs, &files[0]) => 0;
381+
}
382+
383+
for (int c = 0; c < CYCLES; c++) {
384+
// write the other file
385+
sprintf(path, "breakfast/quiche");
386+
lfs_file_open(&lfs, &files[1], path,
387+
LFS_O_RDWR | LFS_O_CREAT | LFS_O_TRUNC) => 0;
388+
if (GC) {
389+
lfs_fs_gc(&lfs) => 0;
390+
}
391+
size = strlen(names[1]);
392+
for (lfs_size_t i = 0; i < SIZE; i += size) {
393+
lfs_file_write(&lfs, &files[1], names[1], size) => size;
394+
}
395+
// sync?
396+
if (SYNC) {
397+
lfs_file_sync(&lfs, &files[1]) => 0;
398+
}
399+
400+
// try to read from both
401+
for (int n = 0; n < FILES; n++) {
402+
lfs_file_rewind(&lfs, &files[n]) => 0;
403+
size_t size = strlen(names[n]);
404+
for (lfs_size_t i = 0; i < SIZE; i += size) {
405+
uint8_t buffer[1024];
406+
lfs_file_read(&lfs, &files[n], buffer, size) => size;
407+
assert(memcmp(buffer, names[n], size) == 0);
408+
}
409+
}
410+
411+
lfs_file_close(&lfs, &files[1]) => 0;
412+
}
413+
lfs_file_close(&lfs, &files[0]) => 0;
414+
lfs_unmount(&lfs) => 0;
415+
416+
// check after remounting
417+
lfs_mount(&lfs, &cfg_) => 0;
418+
{
419+
// last one wins
420+
int n = (SYNC) ? FILES-1 : 0;
421+
char path[1024];
422+
sprintf(path, "breakfast/quiche");
423+
lfs_file_t file;
424+
lfs_file_open(&lfs, &file, path, LFS_O_RDONLY) => 0;
425+
size_t size = strlen(names[n]);
426+
for (int i = 0; i < SIZE; i += size) {
427+
uint8_t buffer[1024];
428+
lfs_file_read(&lfs, &file, buffer, size) => size;
429+
assert(memcmp(buffer, names[n], size) == 0);
430+
}
431+
lfs_file_close(&lfs, &file) => 0;
432+
}
433+
lfs_unmount(&lfs) => 0;
434+
'''
435+
253436
# exhaustion test
254437
[cases.test_alloc_exhaustion]
255438
defines.INFER_BC = [false, true]

0 commit comments

Comments
 (0)