Github/littlefs
1
Fork 0
Code Actions Activity

Ripped out most of LFS_O_SYNC, restrict to writes

Browse Source 
This tears out most of the implied lfsr_file_sync calls, and restricts
LFS_O_SYNC to only imply lfsr_file_sync on _write_ operations. So only
lfsr_file_write, and maybe pwrite/writev/etc in the future.

This mainly affects lfsr_file_truncate/fruncate (and punchhole/
insertrange/collapserange in the future), while reverting the LFS_O_SYNC
related changes in lfsr_file_open:

- lfsr_file_open     + LFS_O_SYNC => does _not_ sync
- lfsr_file_close    + LFS_O_SYNC => syncs (unless desynced)
- lfsr_file_write    + LFS_O_SYNC => syncs
- lfsr_file_sync     + LFS_O_SYNC => syncs
- lfsr_file_truncate + LFS_O_SYNC => does _not_ sync
- lfsr_file_fruncate + LFS_O_SYNC => does _not_ sync

Note LFS_O_FLUSH is unaffected, it was always limited to
lfsr_file_write since that's the only function that touches file
buffers.

Also note I want this rule to apply to the future lfsr_file_punchhole/
insertrange/collapserange functions as well. Even though you can argue
these effectuate writes, they're at a level of sophistication that we
can just expect users to just call lfsr_file_sync if they want to.

---

Ok, so a number of reasons:

- This matches behavior of LFS_O_APPEND, which is intentionally
  restricted to only write operations.

  In that case I think the explicit limitation is easier to understand
  than trying to define an abstract model.

  This makes LFS_O_SYNC, LFS_O_FLUSH, and LFS_O_APPEND consistent in
  when the relevant behavior takes effect.

- This avoids the zero-sized files after powerloss. Which are just as
  likely, if not more, to trip up users vs missing syncs.

- Most truncate/fruncate operations are immediately followed by a write
  operation anyways. Which just makes the truncate/fruncate syncs wasted
  prog/erase cycles.

  Even in some of the more complicated truncate/function use cases, you
  just don't care about when fruncates/truncates hit the disk.

  Take logging via lfsr_file_fruncate for example. Yes the fruncate will
  usually happen _after_ the write operation, but this just means the
  log file will usually be one entry larger than expected. Which is a
  state you can end up with anyways after powerloss.

- This avoids confusing/conflicting LFS_O_SYNC + LFS_O_DESYNC behavior.

  Again, this simple rule is easier to reason about than a model.

You would think this would be well defined in POSIX, but it's really
not. POSIX limits O_SYNC to "write I/O operations", but doesn't really
define a "write" (it is a retroactive standard after all). ftruncate is
a bit funny in that it states "the extended area shall appear as if it
were zero-filled", but the term "write" doesn't appear in ftruncate's
documentation at all.

Searching through LKML, stack overflow, etc, it doesn't seem like anyone
else knows exactly what to do either. There was a bug report[1] in 2005
for ext3 + O_SYNC + ftruncate that was rejected, but a later bug
report[2] in 2012 for xfs + O_SYNC + fallocate that was fixed (but was
broken in almost every Linux fs?).

1: https://lore.kernel.org/lkml/1111610558.1998.193.camel@sisko.sctweedie.blueyonder.co.uk
2: https://lore.kernel.org/linux-ext4/20111116084256.GA22963@infradead.org

So, this may end up a bit controversial, but I'm going to go with the
simpler truncate/fruncate-do-not-imply-sync rule for the above reasons.

I think this is a bit more important for littlefs than other
filesystems, as it also defines the behavior of lfsr_file_open, and with
a rigorous powerloss model being core to the design.

---

This is also cheaper code/stack-wise, but if this was going to be a
deciding factor we should just put LFS_O_SYNC/LFS_O_FLUSH behind ifdefs:

                  code          stack          ctx
  before:        35816           2480          640
  after:         35740 (-0.2%)   2424 (-2.3%)  640 (+0.0%)

Compared to before the LFS_O_SYNC tweaks:

                  code          stack          ctx
  before-tweaks: 35780           2440          640
  before:        35816 (+0.1%)   2480 (+1.6%)  640 (+0.0%)
  after:         35740 (-0.1%)   2424 (-0.7%)  640 (+0.0%)
This commit is contained in:
Christopher Haster
2025-04-26 14:18:08 -05:00
parent 78f9dac162
commit 85778b2813
2 changed files with 667 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
-36
View File
@@ -11331,13 +11331,6 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
LFS_ASSERT(!lfsr_o_isrdonly(flags) || !lfsr_o_iscreat(flags));
LFS_ASSERT(!lfsr_o_isrdonly(flags) || !lfsr_o_isexcl(flags));
LFS_ASSERT(!lfsr_o_isrdonly(flags) || !lfsr_o_istrunc(flags));
// these flags are incompatible
LFS_ASSERT(lfsr_o_isrdonly(flags)
|| !lfsr_o_issync(flags)
|| !lfsr_o_isdesync(flags));
LFS_ASSERT(lfsr_o_isrdonly(flags)
|| !lfsr_o_issync(lfs->flags)
|| !lfsr_o_isdesync(flags));
for (lfs_size_t i = 0; i < cfg->attr_count; i++) {
// these flags require a writable attr
LFS_ASSERT(!lfsr_o_isrdonly(cfg->attrs[i].flags)
@@ -11466,17 +11459,6 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file,
// add to tracked mdirs
lfsr_omdir_open(lfs, &file->b.o);
// sync if requested
if (lfsr_o_issync(file->b.o.flags)
&& !lfsr_o_isrdonly(file->b.o.flags)) {
err = lfsr_file_sync(lfs, file);
if (err) {
lfsr_omdir_close(lfs, &file->b.o);
goto failed;
}
}
return 0;
failed:;
@@ -12952,8 +12934,6 @@ failed:;
int lfsr_file_desync(lfs_t *lfs, lfsr_file_t *file) {
(void)lfs;
LFS_ASSERT(lfsr_omdir_isopen(lfs, &file->b.o));
// desyncing LFS_O_SYNC files is not allowed
LFS_ASSERT(!lfsr_o_issync(file->b.o.flags));
// mark as desynced
file->b.o.flags |= LFS_O_DESYNC;
@@ -13080,14 +13060,6 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) {
file->cache.size,
size_ - lfs_min(file->cache.pos, size_));
// sync if requested
if (lfsr_o_issync(file->b.o.flags)) {
err = lfsr_file_sync(lfs, file);
if (err) {
goto failed;
}
}
return 0;
failed:;
@@ -13157,14 +13129,6 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) {
size - size_,
file->pos);
// sync if requested
if (lfsr_o_issync(file->b.o.flags)) {
err = lfsr_file_sync(lfs, file);
if (err) {
goto failed;
}
}
return 0;
failed:;