Skip to content

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 5 commits into from
Jul 27, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions cores/esp8266/FS.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,13 @@ bool FS::gc() {
return _impl->gc();
}

bool FS::check() {
if (!_impl) {
return false;
}
return _impl->check();
}

bool FS::format() {
if (!_impl) {
return false;
Expand Down
2 changes: 2 additions & 0 deletions cores/esp8266/FS.h
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,9 @@ class FS
bool rmdir(const char* path);
bool rmdir(const String& path);

// Low-level FS routines, not needed by most applications
bool gc();
bool check();

friend class ::SDClass; // More of a frenemy, but SD needs internal implementation to get private FAT bits
protected:
Expand Down
1 change: 1 addition & 0 deletions cores/esp8266/FSImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class FSImpl {
virtual bool mkdir(const char* path) = 0;
virtual bool rmdir(const char* path) = 0;
virtual bool gc() { return true; } // May not be implemented in all file systems.
virtual bool check() { return true; } // May not be implemented in all file systems.
};

} // namespace fs
Expand Down
5 changes: 5 additions & 0 deletions cores/esp8266/spiffs_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,11 @@ class SPIFFSImpl : public FSImpl
return SPIFFS_gc_quick( &_fs, 0 ) == SPIFFS_OK;
}

bool check() override
{
return SPIFFS_check(&_fs) == SPIFFS_OK;
}

protected:
friend class SPIFFSFileImpl;
friend class SPIFFSDirImpl;
Expand Down
23 changes: 23 additions & 0 deletions doc/filesystem.rst
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,29 @@ block size - ``pageSize`` — filesystem logical page size - ``maxOpenFiles``
``maxPathLength`` — max file name length (including one byte for zero
termination)

gc
~~

.. code:: cpp

SPIFFS.gc()

Only implemented in SPIFFS. Performs a quick garbage collection operation on SPIFFS,
possibly making writes perform faster in the future. Not normally needed by applications
as SPIFFS will take care of things itself on writes.
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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)

Copy link
Collaborator Author

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...

Copy link
Contributor

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)

Copy link
Collaborator Author

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." ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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." ?

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 :)

Copy link
Collaborator Author

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!


check
~~~~~

.. code:: cpp

SPIFFS.begin();
SPIFFS.check();

Only implemented in SPIFFS. Performs an in-depth check of the filesystem metadata and
correct what is repairable. Not normally needed, and not guaranteed to actually fix
anything should there be corruption.

Directory object (Dir)
----------------------

Expand Down