-
Notifications
You must be signed in to change notification settings - Fork 8.1k
fs: littlefs: add api call for lfs_fs_gc #93618
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
Conversation
|
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.
Looks like a nice clean set of changes to me. Thanks for the addition!
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.
API call requires test case.
Changes requested.
include/zephyr/fs/fs_sys.h
Outdated
* @param path Path to the file or directory. | ||
* @return 0 on success, negative errno code on fail. | ||
*/ | ||
int (*gc)(struct fs_mount_t *mountp, const char *path); |
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.
No path, only mount point. Unless you have an implementation case for it, but you do not in 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.
Cleared implementation, removed path entirely.
include/zephyr/fs/fs.h
Outdated
* @retval -ENOTSUP when not implemented by underlying file system driver; | ||
* @retval <0 an other negative errno code on error. | ||
*/ | ||
int fs_gc(const char *path); |
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.
Rename this to something like 'fs_gc_by_path' or similar; I would rather have short name, default one, to be reserved for gc by mount point object pointer.
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.
Changed logic, so now fs_gc
accepts mount point pointer.
.mkdir = littlefs_mkdir, | ||
.stat = littlefs_stat, | ||
.statvfs = littlefs_statvfs, | ||
.gc = littlefs_gc, |
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.
This should not be default. Most systems do not have this.
I would rather guard this by Kconfig option, selectable by systems that support it, and on included on demand by users that need the call.
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.
Guarded new functions by CONFIG_FILE_SYSTEM_GC
, default n
.
@mzagrabski are you able to address the change requests? |
2503b11
to
39fcb61
Compare
@de-nordic Added very simple test case for new api call, let me know if you would like to see more complex test case here. |
subsys/fs/Kconfig
Outdated
|
||
config FILE_SYSTEM_GC | ||
bool "Allow explicit garbage collector call" | ||
default n |
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.
Usually we do not set "default n", that is actually default.
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.
@mzagrabski - can you address this review comment please?
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.
@cfriedt both issues fixed
subsys/fs/littlefs_fs.c
Outdated
|
||
fs_lock(fs); | ||
|
||
ssize_t ret = lfs_fs_gc(lfs); |
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.
Try to define variables on top of the function.
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.
Looks OK.
Comments left, but can be addressed 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.
temporary nak to remove the default n
Lfs provide lfs_fs_gc function for some time now. Function allow offloading of expensive block allocation scan. Introduce lfs_fs_gc via api call fc_gc. Signed-off-by: Maciej Zagrabski <[email protected]>
|
Introduce lfs_fs_gc via api call fc_gc.