From 1344d416d2ed86bbb9cd5e48231de2a53a95c425 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 10 Jan 2024 22:50:45 -0600 Subject: [PATCH] Relaxed asserts, allow syncing rdonly files, error on unsync This is a compromise on consistency and not breaking expected invariants. The problem: rdonly files can become unsynced: 1. file is opened rdonly + desync 2. the same file is opened and written to 3. we try to sync our original file handle What we want: 1. sync should ensure disk + files are in-sync 2. rdonly implies sync should not write to disk Without desync, and in other systems, this is not a problem, because rdonly files can never become unsynced. But with desync, a state (albiet a roundabout one) can be reached where we can't satisfy both of these invariants. I wanted to just assert on syncing a rdonly file, but this is supported on POSIX and other systems, and it makes sense that you would want to unconditionally call sync in certain circumstances (ensuring close can't write to disk for example). So adopts the approach of allowing flush and sync on rdonly files when possible, and when not possible, sync simply returns LFS_ERR_INVAL and makes it the user's problem. For the above example, this has the side effect of making the rdonly file desync again, so close can complete without touching disk. As a plus, a desynced rdonly file can now be used to test if a file has been written to. Though I'm not sure when this would be useful... Or if it's a good idea to suggest this use of the API... --- lfs.c | 32 ++++++++----- tests/test_fsync.toml | 101 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 11 deletions(-) diff --git a/lfs.c b/lfs.c index ec94fdc7..ac46d141 100644 --- a/lfs.c +++ b/lfs.c @@ -10659,11 +10659,12 @@ failed:; } int lfsr_file_flush(lfs_t *lfs, lfsr_file_t *file) { - // flushing readonly files is not supported - // - // in theory we could make this a noop, but that would be inconsistent - // with lfsr_file_sync - LFS_ASSERT(lfsr_o_iswriteable(file->flags)); + // readonly files should do nothing + LFS_ASSERT(lfsr_o_iswriteable(file->flags) + || !lfsr_f_isunflushed(file->flags) + || (lfsr_file_size_(file) <= lfs->cfg->cache_size + && lfsr_file_size_(file) <= lfs->cfg->inline_size + && lfsr_file_size_(file) <= lfs->cfg->fragment_size)); // do nothing if our file is already flushed if (!lfsr_f_isunflushed(file->flags)) { @@ -10707,12 +10708,6 @@ failed:; } int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { - // syncing readonly files is not supported - // - // in theory we could make this a noop, but then syncing desynced - // readonly files would require disk writes - LFS_ASSERT(lfsr_o_iswriteable(file->flags)); - // do nothing if our file has been removed if (file->mdir.mid == -1) { return 0; @@ -10751,6 +10746,21 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { // don't write to disk if our disk is already in-sync if (lfsr_f_isunsynced(file->flags)) { + // readonly files should do nothing + // + // but readonly files _can_ end up unsynced, in the roundabout + // case where: + // + // 1. a file is opened rdonly + desync + // 2. the same file is opened and written to + // 3. we try to sync our original file handle + // + // the best thing we can do in this case is return an error + if (!lfsr_o_iswriteable(file->flags)) { + err = LFS_ERR_INVAL; + goto failed; + } + // checkpoint the allocator again lfs_alloc_ckpoint(lfs); diff --git a/tests/test_fsync.toml b/tests/test_fsync.toml index b6e583b1..60fa2e46 100644 --- a/tests/test_fsync.toml +++ b/tests/test_fsync.toml @@ -2085,6 +2085,107 @@ code = ''' lfsr_unmount(&lfs) => 0; ''' +[cases.test_fsync_desync_wrrd_noop] +code = ''' + lfs_t lfs; + lfsr_format(&lfs, CFG) => 0; + lfsr_mount(&lfs, CFG) => 0; + + // a - writer + // b - reader kept open, recvs updates from a + // c - desynced reader + lfsr_file_t a; + lfsr_file_t b; + lfsr_file_t c; + 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_RDONLY) => 0; + lfsr_file_open(&lfs, &c, "jello", LFS_O_RDONLY | LFS_O_DESYNC) => 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 b + lfsr_file_read(&lfs, &b, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + // but not in c + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => 0; + // reopen c, should now be up to date + lfsr_file_close(&lfs, &c) => 0; + lfsr_file_open(&lfs, &c, "jello", LFS_O_RDONLY | LFS_O_DESYNC) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + + // rewrite a and sync + lfsr_file_rewind(&lfs, &a) => 0; + lfsr_file_write(&lfs, &a, "bonjour!", strlen("bonjour!")) + => strlen("bonjour!"); + lfsr_file_sync(&lfs, &a) => 0; + + // our write should show up in b + lfsr_file_rewind(&lfs, &b) => 0; + lfsr_file_read(&lfs, &b, rbuf, sizeof(rbuf)) => strlen("bonjour!"); + assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); + // but not in c + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + + // sync b, this should be a noop + lfsr_file_sync(&lfs, &b) => 0; + + // our write should show up in b + lfsr_file_rewind(&lfs, &b) => 0; + lfsr_file_read(&lfs, &b, rbuf, sizeof(rbuf)) => strlen("bonjour!"); + assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); + // but not in c + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + + // as if things couldn't get weirder + // + // what do you think should happend if we sync c? + // + // in theory, this should update a + b + disk, but that would require + // writing to disk... instead we just error + lfsr_file_sync(&lfs, &c) => LFS_ERR_INVAL; + + // our write should show up in b + lfsr_file_rewind(&lfs, &b) => 0; + lfsr_file_read(&lfs, &b, rbuf, sizeof(rbuf)) => strlen("bonjour!"); + assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); + // but not in c + lfsr_file_rewind(&lfs, &c) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("hello!"); + assert(memcmp(rbuf, "hello!", strlen("hello!")) == 0); + // reopen c, should now be up to date + lfsr_file_close(&lfs, &c) => 0; + lfsr_file_open(&lfs, &c, "jello", LFS_O_RDONLY | LFS_O_DESYNC) => 0; + lfsr_file_read(&lfs, &c, rbuf, sizeof(rbuf)) => strlen("bonjour!"); + assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); + + // we _are_ allowed to sync c if it results in a noop + lfsr_file_sync(&lfs, &c) => 0; + + // our write should show up in b + lfsr_file_rewind(&lfs, &b) => 0; + lfsr_file_read(&lfs, &b, rbuf, sizeof(rbuf)) => strlen("bonjour!"); + assert(memcmp(rbuf, "bonjour!", strlen("bonjour!")) == 0); + // and 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); + + lfsr_file_close(&lfs, &a) => 0; + lfsr_file_close(&lfs, &b) => 0; + lfsr_file_close(&lfs, &c) => 0; + lfsr_unmount(&lfs) => 0; +''' + [cases.test_fsync_desync_wdwdrr_append] code = ''' lfs_t lfs;