-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add a FS::check() optional method #6340
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
07af6cc
Add a FS::check() optional method
earlephilhower 54a9002
Merge branch 'master' into spiffscheck
earlephilhower a928272
Update doc w/more gc() info and link
earlephilhower fb5f0fd
Merge branch 'master' into spiffscheck
earlephilhower 7d66653
Merge branch 'master' into spiffscheck
earlephilhower File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only perform faster, but even allow for a new block allocation which could fail otherwise.
What it does:
It does search for blocks which have all occupied sectors mapped to files which are marked for deletion.
If such a block is found, it will erase that block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that sounds like a bug in SPIFFS. Shouldn't it be keeping track of used but no longer needed blocks and erase them as needed? Garbage collection is normally something handled behind the scenes (Java, SSDs, etc.) and not something users need to explicitly worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it is a bug in SPIFFS or me just trying to use SPIFFS in a way it was never intended to :)
My use case:
I try to append to a file (until some size to prevent file handling becoming too slow) and if appending fails I will throw away the oldest file of that type.
Some sort of circular buffer in files on SPIFFS.
If I do not call the GC on a highly fragmented filesystem, I may need to throw away a lot more files before I find a block to write to.
To my understanding, SPIFFS only runs a limited number of loops to call the GC and it then only will check a limited number of blocks.
A next call to the gc will check the next N blocks etc.
So it may be that a number of calls to gc yields a different result compared to only calling it once.
SPIFFS does do its administration on a bit lower priority, so if you call for a delete the space is not freed immediately.
This is why I added the call to gc from the higher level wrapper.
Calling the (SPIFFS) gc function with a number other than 0 may even try to re-allocate sectors if there are max. N sectors not marked to be deleted in a single block.
But since that's a bit tricky (and also remarkably slow on a fragmented filesystem), I did not add the integer parameter. Also the parameter is really SPIFFS specific and some other gc function on another filesystem may need a completely different integer value.
Also there is still the bug that the actual write function may return a success which later appears to be unsuccessful and you can only check it by checking the file size (if appending)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's pretty bogus, reason enough to go to LittleFS. :) What wording would you suggest here? Fixing SPIFFS is a bit larger than I'd planned with this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was by no means my intention to make this PR into a universal fix for SPIFFS :)
Just some extension to the documentation.
Like I said, my use case is probably not how SPIFFS was intended to be used.
And for the bug of
write
not returning the actual amount of data written (in some cases) is already another issue report. (which I maybe should move/copy to the SPIFFS repo instead of here)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the comment here, it seems like this would be a SPIFFS buf:
https://github.com/pellepl/spiffs/blob/ec68ba8208d7550860e4e78299d58a529b88bf85/src/spiffs.h#L640-L642 .
Have you found that doing a _gc_fast() actually fixes anything? If so, we can note it here. OTW, I'm not really sure what to say for _gc(). "It might help if SPIFFS is reporting out of space errors on a non-full filesystem." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do use it on my nodes that collect data using my "cache controller" in ESPeasy.
It is used there right after deleting a file or when writing fails. (and at boot)
Without calling the gc, the process sometimes got stuck, probably because there were no 2 erased blocks available anymore.
Also on nodes with only 128k SPIFFS it can happen that settings files cannot be changed or become corrupted (64k file, updated in chunks).
After adding the call to the garbage collection at boot, I have not seen it happen anymore.
So at least that issue was made a lot harder to reproduce :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it. I've added a high-level note to that effect and a link to this discussion for users who need to delve further. Thanks for the info!