From 7b81f01db49da8e62f5637f7819b3f27f3796fd2 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sat, 26 Apr 2025 01:28:00 -0500 Subject: [PATCH] Tweaked lfsr_file_open to only sync when unsynced Do'h! I almost forgot about LFS_O_TRUNC. If LFS_O_CREAT + LFS_O_SYNC implies lfsr_file_sync, clearly LFS_O_TRUNC + LFS_O_SYNC should as well. This changes lfsr_file_open to only imply lfsr_file_sync if any open operation sets the unsync flag, which is the only case where lfsr_file_sync would do anything anyways. This does have a subtle change in behavior when LFS_O_CREAT + LFS_O_SYNC + LFS_O_DESYNC, in that the desync flag is only cleared if the file did not exist before. But I think this is more expected than unconditionally syncing. Note this matches the behavior of lfsr_file_write, which does _not_ imply lfsr_file_sync if the write is size=0. --- Also added better tests over lfsr_file_open + LFS_O_TRUNC, this flag isn't very well tested... Which found a bug! We were incorrectly setting LFS_o_UNFLUSH when opening with LFS_O_TRUNC, when we should have set LFS_o_UNSYNC. This caused littlefs to never bother updating the file's metadata unless some other write comes along (which is what usually follows LFS_O_TRUNC). To help catch bugs like this, I added an assert to lfsr_file_flush that unflushed files are always marked unsynced. A synced + unflushed file is weird and should never happen. --- Code changes minimal: code stack ctx before: 35820 2480 640 after: 35824 (+0.0%) 2480 (+0.0%) 640 (+0.0%) --- lfs.c | 23 +++++--- tests/test_fsync.toml | 128 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 129 insertions(+), 22 deletions(-) diff --git a/lfs.c b/lfs.c index 373054a8..48522e1d 100644 --- a/lfs.c +++ b/lfs.c @@ -11247,7 +11247,7 @@ static int lfsr_file_fetch(lfs_t *lfs, lfsr_file_t *file, bool trunc) { // update the bshrub file->b.shrub = file->b.shrub_; - // mark as synced + // mark as in-sync file->b.o.flags &= ~LFS_o_UNSYNC; } @@ -11353,8 +11353,8 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, // setup file state file->cfg = cfg; file->b.o.flags = lfsr_o_settype(flags, LFS_TYPE_REG) - // default to unflushed for orphans/truncated files - | LFS_o_UNFLUSH; + // default to unsynced for uncreated/truncated files + | LFS_o_UNSYNC; file->pos = 0; file->eblock = 0; file->eoff = -1; @@ -11423,10 +11423,12 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, } } - // if stickynote, mark as uncreated and unsynced, we need to convert - // to reg file on first sync - if (!exists || tag == LFSR_TAG_STICKYNOTE || tag == LFSR_TAG_ORPHAN) { - file->b.o.flags |= LFS_o_UNCREAT | LFS_o_UNSYNC; + // if stickynote, mark as uncreated, we need to convert to reg file + // on first sync + if (!exists + || tag == LFSR_TAG_STICKYNOTE + || tag == LFSR_TAG_ORPHAN) { + file->b.o.flags |= LFS_o_UNCREAT; } // allocate cache if necessary @@ -11459,7 +11461,9 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, lfsr_omdir_open(lfs, &file->b.o); // sync if requested - if (lfsr_o_iscreat(flags) && lfsr_o_issync(flags)) { + if (!lfsr_o_isrdonly(file->b.o.flags) + && lfsr_o_issync(file->b.o.flags) + && lfsr_o_isunsync(file->b.o.flags)) { err = lfsr_file_sync(lfs, file); if (err) { lfsr_omdir_close(lfs, &file->b.o); @@ -12702,6 +12706,8 @@ int lfsr_file_flush(lfs_t *lfs, lfsr_file_t *file) { if (!lfsr_o_isunflush(file->b.o.flags)) { return 0; } + // unflushed files must be unsynced + LFS_ASSERT(lfsr_o_isunsync(file->b.o.flags)); // clobber entangled traversals lfsr_omdir_mkdirty(lfs, &file->b.o); @@ -12748,6 +12754,7 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { if (err) { goto failed; } + // build a commit of any pending file metadata lfsr_rattr_t rattrs[3]; lfs_size_t rattr_count = 0; diff --git a/tests/test_fsync.toml b/tests/test_fsync.toml index 2f68017e..7669afd5 100644 --- a/tests/test_fsync.toml +++ b/tests/test_fsync.toml @@ -160,8 +160,27 @@ code = ''' assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); lfsr_file_close(&lfs, &c) => 0; - // lets rewrite a - lfsr_file_rewind(&lfs, &a) => 0; + // reopen with LFS_O_TRUNC + lfsr_file_close(&lfs, &a) => 0; + lfsr_file_open(&lfs, &a, "jello", + LFS_O_WRONLY + | LFS_O_TRUNC + | ((FLUSH == 2) ? LFS_O_FLUSH : 0) + | ((SYNC == 2) ? LFS_O_SYNC : 0)) => 0; + + // should immediately show up in b + lfsr_file_read(&lfs, &b, rbuf, sizeof(rbuf)) => 0; + // and on disk + lfsr_file_open(&lfs, &c, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => 0; + lfsr_file_close(&lfs, &c) => 0; + + // reopen LFS_O_WRONLY and rewrite a + lfsr_file_close(&lfs, &a) => 0; + lfsr_file_open(&lfs, &a, "jello", + LFS_O_WRONLY + | ((FLUSH == 2) ? LFS_O_FLUSH : 0) + | ((SYNC == 2) ? LFS_O_SYNC : 0)) => 0; lfsr_file_write(&lfs, &a, "bonjour!", strlen("bonjour!")) => strlen("bonjour!"); @@ -405,19 +424,77 @@ code = ''' assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); lfsr_file_close(&lfs, &d) => 0; - // lets rewrite a one last time - lfsr_file_rewind(&lfs, &a) => 0; - lfsr_file_write(&lfs, &a, "ohayo!", strlen("ohayo!")) + // reopen b with LFS_O_TRUNC + lfsr_file_close(&lfs, &b) => 0; + lfsr_file_open(&lfs, &b, "jello", + LFS_O_WRONLY + | LFS_O_TRUNC + | ((FLUSH == 2) ? LFS_O_FLUSH : 0) + | ((SYNC == 2) ? LFS_O_SYNC : 0)) => 0; + + // should immediately show up in c + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => 0; + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => 0; + lfsr_file_close(&lfs, &d) => 0; + + // sync a + lfsr_file_sync(&lfs, &a) => 0; + + // a's contents were clobbered, so we should see b + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => 0; + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => 0; + lfsr_file_close(&lfs, &d) => 0; + + // reopen LFS_O_WRONLY and rewrite b + lfsr_file_close(&lfs, &b) => 0; + lfsr_file_open(&lfs, &b, "jello", + LFS_O_WRONLY + | ((FLUSH == 2) ? LFS_O_FLUSH : 0) + | ((SYNC == 2) ? LFS_O_SYNC : 0)) => 0; + lfsr_file_write(&lfs, &b, "ohayo!", strlen("ohayo!")) => strlen("ohayo!"); // should immediately show up in c lfsr_file_rewind(&lfs, &c) => 0; - lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("ohayo!r!"); - assert(memcmp(rbuf, "ohayo!r!", strlen("ohayo!r!")) == 0); + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("ohayo!"); + assert(memcmp(rbuf, "ohayo!", strlen("ohayo!")) == 0); // and on disk lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; - lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("ohayo!r!"); - assert(memcmp(rbuf, "ohayo!r!", strlen("ohayo!r!")) == 0); + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("ohayo!"); + assert(memcmp(rbuf, "ohayo!", strlen("ohayo!")) == 0); + lfsr_file_close(&lfs, &d) => 0; + + // sync a + lfsr_file_sync(&lfs, &a) => 0; + + // a's contents were clobbered, so we should see b + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("ohayo!"); + assert(memcmp(rbuf, "ohayo!", strlen("ohayo!")) == 0); + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("ohayo!"); + assert(memcmp(rbuf, "ohayo!", strlen("ohayo!")) == 0); + lfsr_file_close(&lfs, &d) => 0; + + // lets rewrite a one last time + lfsr_file_rewind(&lfs, &a) => 0; + lfsr_file_write(&lfs, &a, "zdrasti!", strlen("zdrasti!")) + => strlen("zdrasti!"); + + // should immediately show up in c + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("zdrasti!"); + assert(memcmp(rbuf, "zdrasti!", strlen("zdrasti!")) == 0); + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("zdrasti!"); + assert(memcmp(rbuf, "zdrasti!", strlen("zdrasti!")) == 0); lfsr_file_close(&lfs, &d) => 0; // close a and b @@ -426,12 +503,12 @@ code = ''' // should still have a lfsr_file_rewind(&lfs, &c) => 0; - lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("ohayo!r!"); - assert(memcmp(rbuf, "ohayo!r!", strlen("ohayo!r!")) == 0); + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("zdrasti!"); + assert(memcmp(rbuf, "zdrasti!", strlen("zdrasti!")) == 0); // and on disk lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; - lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("ohayo!r!"); - assert(memcmp(rbuf, "ohayo!r!", strlen("ohayo!r!")) == 0); + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("zdrasti!"); + assert(memcmp(rbuf, "zdrasti!", strlen("zdrasti!")) == 0); lfsr_file_close(&lfs, &d) => 0; lfsr_file_close(&lfs, &c) => 0; @@ -2474,7 +2551,30 @@ code = ''' assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); lfsr_file_close(&lfs, &d) => 0; - // write to a + // reopen with LFS_O_TRUNC + lfsr_file_close(&lfs, &a) => 0; + lfsr_file_open(&lfs, &a, "jello", + LFS_O_WRONLY + | LFS_O_TRUNC + | ((FLUSH == 2) ? LFS_O_FLUSH : 0) + | ((SYNC == 2) ? LFS_O_SYNC : 0) + | LFS_O_DESYNC) => 0; + + // should immediately show up in c + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => 0; + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => 0; + lfsr_file_close(&lfs, &d) => 0; + + // reopen LFS_O_WRONLY and rewrite a + lfsr_file_close(&lfs, &a) => 0; + lfsr_file_open(&lfs, &a, "jello", + LFS_O_WRONLY + | ((FLUSH == 2) ? LFS_O_FLUSH : 0) + | ((SYNC == 2) ? LFS_O_SYNC : 0) + | LFS_O_DESYNC) => 0; lfsr_file_write(&lfs, &a, "bonjour!", strlen("bonjour!")) => strlen("bonjour!");