Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature for adding uncommitted close to withdraw previous writes #960

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StarBTackD3v
Copy link

@StarBTackD3v StarBTackD3v commented Apr 2, 2024

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
Copy link
Collaborator

geky-bot commented Apr 2, 2024

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
Copy link
Member

geky commented Apr 11, 2024

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.

@StarBTackD3v
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants