From 4e7a68ff082bb792004e131d17098d4a75236a1b Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 10 Jan 2024 19:45:57 -0600 Subject: [PATCH] Fixed another subtle corner cases with noop sync broadcasting This is an extension of the noop-sync after unrelated write-sync after desync corner case: op a state b state in-sync in-sync desync(b) in-sync desync write(a) unsync desync sync(a) in-sync' desync sync(b) in-sync in-sync But instead of explicitly calling lfsr_file_sync, what if you implicitly triggered sync through something like a write on a file with the LFS_O_SYNC flag, but not a normal write, a noop write, write(0)? If the definition of LFS_O_SYNC is taken literally as "lfsr_file_write and friends implicitly call lfsr_file_sync after every call", then this should behave just as if lfsr_file_sync had been called, and unconditionally broadcast the sync. Since this is the simplest interpretation, I think this is what we should implement. Added tests, and adopted this behavior. Fortunately this just involves some small gotos (https://xkcd.com/292): code stack before: 33020 2976 after: 33026 (+0.0%) 2976 (+0.0%) --- lfs.c | 11 ++-- tests/test_fsync.toml | 118 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 4 deletions(-) diff --git a/lfs.c b/lfs.c index ec94fdc7..eca1e2f6 100644 --- a/lfs.c +++ b/lfs.c @@ -10520,8 +10520,9 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file, // underlying file, this means no updating file pos or file size // // since we need to test for this, just return early + lfs_size_t written = 0; if (size == 0) { - return 0; + goto noop; } // checkpoint the allocator @@ -10548,7 +10549,6 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file, } const uint8_t *buffer_ = buffer; - lfs_size_t written = 0; while (size > 0) { // bypass buffer? // @@ -10631,6 +10631,7 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file, // update our pos file->pos = pos; +noop:; // flush if requested // // this seems unreachable, but it's possible if we transition from @@ -10873,7 +10874,7 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { // do nothing if our size does not change lfs_off_t size = lfsr_file_size_(file); if (lfsr_file_size_(file) == size_) { - return 0; + goto noop; } // checkpoint the allocator @@ -10942,6 +10943,7 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { // mark as unsynced file->flags |= LFS_F_UNSYNCED; +noop:; // flush if requested // // this seems unreachable, but it's possible if we transition from @@ -10978,7 +10980,7 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { // do nothing if our size does not change lfs_off_t size = lfsr_file_size_(file); if (size == size_) { - return 0; + goto noop; } // checkpoint the allocator @@ -11075,6 +11077,7 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { // mark as unsynced file->flags |= LFS_F_UNSYNCED; +noop:; // flush if requested // // this seems unreachable, but it's possible if we transition from diff --git a/tests/test_fsync.toml b/tests/test_fsync.toml index b6e583b1..8cd78346 100644 --- a/tests/test_fsync.toml +++ b/tests/test_fsync.toml @@ -2085,6 +2085,124 @@ code = ''' lfsr_unmount(&lfs) => 0; ''' +[cases.test_fsync_desync_wdwdrr_noop_creative] +# there's several clever ways we can noop... +# TODO flush()? +# 0 => sync() +# 1 => write(0) +# 2 => truncate(size) +# 3 => fruncate(size) +defines.NOOP = [0, 1, 2, 3] +code = ''' + lfs_t lfs; + lfsr_format(&lfs, CFG) => 0; + lfsr_mount(&lfs, CFG) => 0; + + // a - writer + // b - syncing writer + // c - reader kept open, recvs updates from a + // d - reader kept closed, checks disk state + lfsr_file_t a; + lfsr_file_t b; + lfsr_file_t c; + lfsr_file_t d; + uint8_t rbuf[256]; + lfsr_file_open(&lfs, &a, "jello", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + lfsr_file_open(&lfs, &b, "jello", LFS_O_WRONLY | LFS_O_SYNC) => 0; + lfsr_file_open(&lfs, &c, "jello", LFS_O_RDONLY) => 0; + + // write to a and sync + lfsr_file_write(&lfs, &a, "hello!", strlen("hello!")) + => strlen("hello!"); + lfsr_file_sync(&lfs, &a) => 0; + + // our write should show up in c + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + lfsr_file_close(&lfs, &d) => 0; + + // mark b as desync, this should freeze its contents + lfsr_file_desync(&lfs, &b) => 0; + // rewrite a, sync + lfsr_file_rewind(&lfs, &a) => 0; + lfsr_file_write(&lfs, &a, "bonjour!", strlen("bonjour!")) + => strlen("bonjour!"); + lfsr_file_sync(&lfs, &a) => 0; + + // a should show up in c + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("bonjour!"); + assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("bonjour!"); + assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); + lfsr_file_close(&lfs, &d) => 0; + + // sync b, _creatively_, dragons be here + if (NOOP == 0) { + lfsr_file_sync(&lfs, &b) => 0; + } else if (NOOP == 1) { + lfsr_file_write(&lfs, &b, NULL, 0) => 0; + } else if (NOOP == 2) { + lfsr_file_size(&lfs, &b) => strlen("hello!"); + lfsr_file_truncate(&lfs, &b, strlen("hello!")) => 0; + } else if (NOOP == 3) { + lfsr_file_size(&lfs, &b) => strlen("hello!"); + lfsr_file_fruncate(&lfs, &b, strlen("hello!")) => 0; + } + + // b should show up in c, without a's changes + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + // and on disk + lfsr_file_open(&lfs, &d, "jello", LFS_O_RDONLY) => 0; + lfsr_file_read(&lfs, &d, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + lfsr_file_close(&lfs, &d) => 0; + + // rewrite b, close, desync flag should have been cleared + lfsr_file_rewind(&lfs, &b) => 0; + lfsr_file_write(&lfs, &b, "ohayo!", strlen("ohayo!")) + => strlen("ohayo!"); + lfsr_file_close(&lfs, &b) => 0; + + // our write should show up in c + 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; + + // rewrite a, close + lfsr_file_rewind(&lfs, &a) => 0; + lfsr_file_write(&lfs, &a, "zdrasti!", strlen("zdrasti!")) + => strlen("zdrasti!"); + lfsr_file_close(&lfs, &a) => 0; + + // our write should 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; + + lfsr_file_close(&lfs, &c) => 0; + lfsr_unmount(&lfs) => 0; +''' + [cases.test_fsync_desync_wdwdrr_append] code = ''' lfs_t lfs;