littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
4.9k stars 770 forks source link

Feature for adding uncommitted close to withdraw previous writes #960

Open ps0nw38 opened 3 months ago

ps0nw38 commented 3 months ago

Currently, changes made by writing to a file only become valid by a commit when the file is closed in accordance to atomic operations. Scenarios are conceivable in which the intended writing to a file is not committed, but the underlying housekeeping leaves no memory leaks by closing the file properly e.g. when reading back, it is determined that the temporarily written data is logically corrupt because, for example, a higher-level integrity check failed. It would then be desirable for the previous state of the file to be retained (without having to write to a new file and move it afterwards).

To support the withdrawal of an ACID transaction (https://en.wikipedia.org/wiki/ACID), I would like to introduce the following proposal and introduce a new close function without committing the written changes.

geky-bot commented 3 months ago
Tests passed ✓, Code: 17048 B (+0.1%), Stack: 1432 B (+0.0%), Structs: 812 B (+0.0%) | | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 17048 B (+0.1%) | 1432 B (+0.0%) | 812 B (+0.0%) | Lines | 2391/2585 lines (-0.5%) | | Readonly | 6210 B (+0.3%) | 448 B (+0.0%) | 812 B (+0.0%) | Branches | 1243/1586 branches (-0.2%) | | Threadsafe | 17940 B (+0.2%) | 1432 B (+0.0%) | 820 B (+0.0%) | | **Benchmarks** | | Multiversion | 17112 B (+0.1%) | 1432 B (+0.0%) | 816 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18744 B (+0.1%) | 1736 B (+0.0%) | 816 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17752 B (+0.2%) | 1424 B (+0.0%) | 812 B (+0.0%) | Erased | 1568888832 B (+0.0%) |
geky commented 2 months ago

Hi @ps0nw38, thanks for creating a PR and proposing this.

I actually already have some plans to provide this functionality, albeit in a slightly different form, but it has been low-priority as I haven't heard all that much interest from users (though this may just be because it's uncommon in other filesystems).

It's interesting to note littlefs does have a limited form of this, but only if a file operation itself errors. If a file operation errors (LFS_ERR_CORRUPT, LFS_ERR_NOSPC, etc), the file sets the internal LFS_F_ERRED flag which tells lfs_file_close to only clean up resources. The intention is for common/naive usage (open->write->close) to still fail gracefully.

I've extended this to be user-facing, renaming LFS_F_ERRED -> LFS_O_DESYNC, on this branch, however this branch is a large body of ongoing work. A part of these changes include how sync interacts with multiple open files, which gives LFS_O_DESYNC a bit more utility.


I'll see if I can summarize the plan with LFS_O_DESYNC, it'd be nice to know your thoughts:

  1. Add LFS_O_DESYNC, a flag which indicates the file should not be saved to disk.

    This can be provided during open:

    lfs_file_open(&lfs, &file, "path", LFS_O_RDWR | LFS_O_DESYNC) => 0;

    Or set after open:

    lfs_file_desync(&lfs, &file) => 0;
  2. Desynced files are not written to disk on close.

  3. Desynced files do not receive or broadcast updates to other open files, though this isn't really relevant without other sync changes.

  4. LFS_O_DESYNC is implicitly set on any error during file operation.

  5. LFS_O_DESYNC is cleared on a successful lfs_file_sync call (but not lfs_file_close).

With LFS_O_DESYNC, lfs_file_uncommitted_close would be equivalent to the following:

lfs_file_desync(&lfs, &file) => 0;
lfs_file_close(&lfs, &file) => 0;

Which would be guaranteed to not write to disk and to not error, allowing safe resource cleanup.

ps0nw38 commented 2 months ago

Hi @geky and thanks for your reply. I like your summarized sketch with the LFS_O_DESYNC flag and it fits perfectly into the use-case. I'll take another deeper look at the branch.