Github/littlefs
1
Fork 0
Code Actions Activity

Fixed noop sync broadcasting, added more specific sync tests

Browse Source 
There are a number of nuanced cases to watch out for when mixing sync,
desync, and "noop syncs" (sync when no write operation has occured):

1. Noop-sync after unrelated write:

     op                 a state         b state
                        in-sync         in-sync
     write(a)           unsync          in-sync
     sync(b)            in-sync         in-sync

   In this case, a should be clobbered by b when b syncs. But this
   gets tricky since b is still up to date with the disk, so b's
   unsynced flag is not set.

   The solution here is to just unconditionally broadcast all sync
   operations irregardless of on-disk state. This is all in-device
   anyways, so it shouldn't really add any overhead.

2. Noop-sync after unrelated write-sync after desync:

     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

   In this case, a should again be clobbered by b, even though a is
   in-sync with the disk. This is not tricky because of a's state, but
   because b doesn't know it is no longer in-sync with the disk.

   The solution here is to set the unsynced flag on all desynced files
   when an unrelated file is synced. This way, b knows it needs to
   update disk if sync is called. We already scan all opened files to
   update in-sync files, so this has very little cost.

3. Readonly-sync after unrelated write-sync after desync?

   This is basically the same as 2., but involves a readonly file:

     op                 a state         b state (rdonly)
                        in-sync         in-sync
     desync(b)          in-sync         desync
     write(a)           unsync          desync
     sync(a)            in-sync'        desync
     sync(b)            ???             in-sync

   In this case, I have no idea what should happen.

   I would guess the least surprising result would be for b to write
   its contents to a/disk? Bringing everything in-sync?

   But this implies that b, a readonly file, should write to disk.

   This isn't the only place a read operation would result in a write.
   RDWR files, for example, can flush buffers during a file read. But at
   least there, the file is open RDWR, not strictly RDONLY.

   It seems like writing during sync on a readonly file breaks some sort
   of invariant users expect.

   But the alternative: Dropping the current state of b in favor of a's
   state, is inconsistent with sync on WRONLY/RDWR files, and seems like
   it breaks some sort of invariant about sync modifying the current
   file's state...

   Given this situation, I think the best course of action is to just
   disallow sync on readonly files. It is now an assert.

   There is some precedent for this, upstream we already omit sync when
   compiled in LFS_READONLY mode. Though this does deviate from POSIX
   behavior...

   Worst case, by asserting, this leaves us free to introduce different
   readonly-sync behavior in the future without breaking backwards
   compatibility.

   ---

   Maybe there should be some sort of lfsr_file_resync function to
   discard current changes? Though this can be done with a close+open
   cycle, so I think the value would be low.

Added tests over these cases and fixed where they broke, except for 3.,
lfsr_file_sync and lfsr_file_flush get asserts now to prevent their use
on readonly files.

Also added a couple more specific tests to cover cases I was concerned
about.
This commit is contained in:
Christopher Haster
2024-01-10 16:47:42 -06:00
parent 0891f6264f
commit fdc8c8caf1
2 changed files with 1042 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+52 -48
View File
@@ -5118,20 +5118,6 @@ static int lfsr_mdir_lookupwide(lfs_t *lfs, const lfsr_mdir_t *mdir,
}
// track opened mdirs to keep state in-sync
static void lfsr_addopened(lfs_t *lfs, lfsr_opened_t *opened) {
opened->next = lfs->opened;
lfs->opened = opened;
}
static void lfsr_removeopened(lfs_t *lfs, lfsr_opened_t *opened) {
for (lfsr_opened_t **p = &lfs->opened; *p; p = &(*p)->next) {
if (*p == opened) {
*p = (*p)->next;
break;
}
}
}
static bool lfsr_isopened(lfs_t *lfs, const lfsr_opened_t *opened) {
for (lfsr_opened_t *p = lfs->opened; p; p = p->next) {
if (p == opened) {
@@ -5142,6 +5128,22 @@ static bool lfsr_isopened(lfs_t *lfs, const lfsr_opened_t *opened) {
return false;
}
static void lfsr_addopened(lfs_t *lfs, lfsr_opened_t *opened) {
LFS_ASSERT(!lfsr_isopened(lfs, opened));
opened->next = lfs->opened;
lfs->opened = opened;
}
static void lfsr_removeopened(lfs_t *lfs, lfsr_opened_t *opened) {
LFS_ASSERT(lfsr_isopened(lfs, opened));
for (lfsr_opened_t **p = &lfs->opened; *p; p = &(*p)->next) {
if (*p == opened) {
*p = (*p)->next;
break;
}
}
}
/// Metadata-tree things ///
@@ -10415,6 +10417,7 @@ static int lfsr_file_flush_(lfs_t *lfs, lfsr_file_t *file,
lfs_ssize_t lfsr_file_read(lfs_t *lfs, lfsr_file_t *file,
void *buffer, lfs_size_t size) {
// can't read from writeonly files
LFS_ASSERT(lfsr_o_isreadable(file->flags));
LFS_ASSERT(file->pos + size <= 0x7fffffff);
@@ -10505,6 +10508,7 @@ lfs_ssize_t lfsr_file_read(lfs_t *lfs, lfsr_file_t *file,
lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file,
const void *buffer, lfs_size_t size) {
// can't write to readonly files
LFS_ASSERT(lfsr_o_iswriteable(file->flags));
// would this write make our file larger than our size limit?
@@ -10655,14 +10659,11 @@ failed:;
}
int lfsr_file_flush(lfs_t *lfs, lfsr_file_t *file) {
// do nothing if our file is readonly
if (!lfsr_o_iswriteable(file->flags)) {
LFS_ASSERT(!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));
return 0;
}
// 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));
// do nothing if our file is already flushed
if (!lfsr_f_isunflushed(file->flags)) {
@@ -10706,19 +10707,17 @@ 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;
}
// do nothing if our file is readonly
if (!lfsr_o_iswriteable(file->flags)) {
LFS_ASSERT(!lfsr_f_isunsynced(file->flags));
// but do clear desync flag
file->flags &= ~LFS_O_DESYNC;
return 0;
}
// first flush any data in our buffer, this is a noop if already
// flushed
//
@@ -10750,14 +10749,14 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) {
&& file->buffer_size <= lfs->cfg->inline_size
&& file->buffer_size <= lfs->cfg->fragment_size));
// checkpoint the allocator again
lfs_alloc_ckpoint(lfs);
// don't write to disk if disk is already in-sync
// don't write to disk if our disk is already in-sync
if (lfsr_f_isunsynced(file->flags)) {
// checkpoint the allocator again
lfs_alloc_ckpoint(lfs);
// commit our file's metadata
uint8_t buf[LFSR_BTREE_DSIZE];
int err = lfsr_mdir_commit(lfs, &file->mdir, LFSR_ATTRS(
err = lfsr_mdir_commit(lfs, &file->mdir, LFSR_ATTRS(
(lfsr_f_isunflushed(file->flags) && file->buffer_size == 0)
? LFSR_ATTR(file->mdir.mid,
WIDE(RM(STRUCT)), 0,
@@ -10774,7 +10773,7 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) {
WIDE(BTREE), 0,
FROMBTREE(&file->ftree.u.btree, buf))));
if (err) {
return err;
goto failed;
}
}
@@ -10786,20 +10785,25 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) {
if (file_->type == LFS_TYPE_REG
&& file_->mdir.mid == file->mdir.mid
// don't double update
&& file_ != file
// don't update desynced file handles
&& !lfsr_o_isdesync(file_->flags)) {
file_->flags &= ~LFS_F_UNSYNCED;
if (lfsr_f_isunflushed(file->flags)) {
file_->flags |= LFS_F_UNFLUSHED;
&& file_ != file) {
// mark desynced files an unsynced
if (lfsr_o_isdesync(file_->flags)) {
file_->flags |= LFS_F_UNSYNCED;
// update synced files
} else {
file_->flags &= ~LFS_F_UNFLUSHED;
file_->flags &= ~LFS_F_UNSYNCED;
if (lfsr_f_isunflushed(file->flags)) {
file_->flags |= LFS_F_UNFLUSHED;
} else {
file_->flags &= ~LFS_F_UNFLUSHED;
}
file_->ftree = file->ftree;
file_->buffer_pos = file->buffer_pos;
LFS_ASSERT(file->buffer_size <= lfs->cfg->cache_size);
memcpy(file_->buffer, file->buffer, file->buffer_size);
file_->buffer_size = file->buffer_size;
}
file_->ftree = file->ftree;
file_->buffer_pos = file->buffer_pos;
LFS_ASSERT(file->buffer_size <= lfs->cfg->cache_size);
memcpy(file_->buffer, file->buffer, file->buffer_size);
file_->buffer_size = file->buffer_size;
}
}
tests/test_fsync.toml
+990
View File
File diff suppressed because it is too large Load Diff