Github/littlefs
1
Fork 0
Code Actions Activity

Tweaked lfsr_file_open to only sync when unsynced

Browse Source 
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%)
This commit is contained in:
Christopher Haster
2025-04-26 01:28:00 -05:00
parent 4b87499605
commit 7b81f01db4
2 changed files with 129 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+15 -8
View File
@@ -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;
tests/test_fsync.toml
+114 -14
View File
@@ -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!");