From 122864f4b6cbdda844a43854aa9a6b8d2485345e Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 10 Jan 2024 20:12:06 -0600 Subject: [PATCH] Reverted LFS_O_SYNC implicit noop sync broadcasting Reading more into POSIX, it seems that most of the write functions do have special behavior built into what would implicitly be a noop. It's difficult to find, since it usually doesn't matter, but consider the m_time field. The following operations do _not_ update m_time: - write when size=0 - truncate when size does not change - fruncate when size does not change I think it's safe to extend these to sync broadcasts in littlefs, and only guarantee sync broadcasts when the file state has changed (even though that may mean other file handles may remain out-of-date!). In this interpretation, the "write operations" described in POSIX more mean the implicit write operations effected by write/truncate/fruncate. That being said, it's not clear what the best approach is, desync files make this all a bit more muddled... This may also be reverted. --- lfs.c | 11 ++-- tests/test_fsync.toml | 118 ------------------------------------------ 2 files changed, 4 insertions(+), 125 deletions(-) diff --git a/lfs.c b/lfs.c index eca1e2f6..ec94fdc7 100644 --- a/lfs.c +++ b/lfs.c @@ -10520,9 +10520,8 @@ 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) { - goto noop; + return 0; } // checkpoint the allocator @@ -10549,6 +10548,7 @@ 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,7 +10631,6 @@ 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 @@ -10874,7 +10873,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_) { - goto noop; + return 0; } // checkpoint the allocator @@ -10943,7 +10942,6 @@ 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 @@ -10980,7 +10978,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_) { - goto noop; + return 0; } // checkpoint the allocator @@ -11077,7 +11075,6 @@ 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 8cd78346..b6e583b1 100644 --- a/tests/test_fsync.toml +++ b/tests/test_fsync.toml @@ -2085,124 +2085,6 @@ 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;