Add support for littlefs #17749
Add support for littlefs #17749
Conversation
pabigot
requested review from
dbkinder,
jakub-uC,
nashif,
nordic-krch and
wentongwu
as code owners
|
All checks are passing now. Review history of this comment for details about previous failed status. |
pabigot
force-pushed
the
next/littlefs
branch
2 times, most recently
from
93c5af8 to
e41e45f
Compare
This comment has been minimized.
This comment has been minimized.
|
Addressed public reviews to this point , and non-public related to handling of |
|
Close and reopen to restart shippable now that zephyr's littlefs repo has content. |
|
I'd like to point out that, for historical reasons, the default flash page layout block size for SPI NOR devices is 64 KiBy, which is huge. For littlefs, and probably most other file systems, a more sane setting is 4 KiBy. You get this by setting |
|
I looked into cloning the NFFS tests which are comprehensive but also appear to be whitebox so non-trivial to port to a different file system. I'll revisit this next week. |
This comment has been minimized.
This comment has been minimized.
|
Latest push corrects the issue number in a commit and switches to consistently using memory pool for file cache allocations, which simplifies the code and increases flexibility for minor cache size customizations. |
| /** @brief Filesystem info structure for LittleFS mount */ | ||
| struct fs_littlefs { | ||
| /* Defaulted in driver, customizable before mount. */ | ||
| struct lfs_config cfg; |
There was a problem hiding this comment.
Would it be possible to change this to:
const struct lfs_config *cfg;and
- extend configuration macros so it will be possible to setup
lfs_configasstatic conststructure at compile time and has it whole at a flash - add function
int littlefs_config(struct fs_mount_t *mountp, struct lfs_config *cfg);which will be runtime version (i.e. need to be called before littlefs_mount) ?
There was a problem hiding this comment.
For (1), no. lfs_config.{read,prog,sync} are assigned within the driver to the functions the driver implements for a specific back-end. The structure must be in RAM.
For (2), I don't understand who would be calling that function. It's not in the FS API.
There was a problem hiding this comment.
For (1), no.
lfs_config.{read,prog,sync}are assigned within the driver to the functions the driver implements for a specific back-end. The structure must be in RAM.
I don't see any specific back-end other than defined in lfs_api_xxx functions (i.e. functions that wraps flash_area_xxx functions) and even if there would be so, it certainly does not need to be in RAM.
For (2), I don't understand who would be calling that function. It's not in the FS API.
Every concrete filesystem in Zephyr need specific setup and initialization before mount (for example in fs shell, there are mount functions for fat, nffs and littlefs. Those initialization API does not need to be FS API, it is specific to FS (so better than API is to use term SPI in meaning of 'service provider interface').
From use cases point of view there are two:
1.) static FS configuration - everything is known before build, i.e. flash parameters, sizes, offsets, buffers and other littlefs settings - this is common for most cases and target devices.
2.) runtime FS configuration that otherwise need to call second function - for example posix host utility configured by arguments from command line, or dynamic FS setup for micropython etc.
I just don't like to see wasting RAM for constant structures.
There was a problem hiding this comment.
It is not a constant structure given the way the FS API works. See https://github.com/zephyrproject-rtos/zephyr/pull/17749/files#r310552307.
The shell really does nothing more than put some generic glue code that's specific to the shell around a call to fs_mount() given a pointer to a statically-defined mount point. NFFS has to get the device name at runtime; fatfs and littlefs do not. Generally there is no FS-specific runtime configuration.
There was a problem hiding this comment.
It is not a constant structure given the way the FS API works. See https://github.com/zephyrproject-rtos/zephyr/pull/17749/files#r310552307.
The shell really does nothing more than put some generic glue code that's specific to the shell around a call to
fs_mount()given a pointer to a statically-defined mount point. NFFS has to get the device name at runtime; fatfs and littlefs do not. Generally there is no FS-specific runtime configuration.
For static configurations you wouldn't be forced to call littlefs specific config function. However there are other cases where use of such interface is legitimate: format, chkdsk, etc.) without it, it would not even be possible to create utility like described in #17719 (without recompilation everytime FS parameters is changed).
There was a problem hiding this comment.
So in those special cases you need runtime configuration, which you can do with this as it stands.
It seems that all this is about trying to move 72 bytes of required configuration data from RAM into flash. As the submitter and maintainer of this module my answer is: no. It doesn't increase functionality, and it dramatically increases complexity because of the visibility and accessibility of information that would have to be collected at compile time.
It's up to you whether you want to approve, comment, or reject this PR. If you want to figure out how to make this work later, that's on you.
There was a problem hiding this comment.
figure out how to make this work later
I'd definitely recommend any reviewer to keep this option in mind. Suggesting an improvement may be good, but an answer "it can't be all done at once" should be a well expected response.
There was a problem hiding this comment.
So in those special cases you need runtime configuration, which you can do with this as it stands.
It seems that all this is about trying to move 72 bytes of required configuration data from RAM into flash. As the submitter and maintainer of this module my answer is: no. It doesn't increase functionality, and it dramatically increases complexity because of the visibility and accessibility of information that would have to be collected at compile time.
It's up to you whether you want to approve, comment, or reject this PR. If you want to figure out how to make this work later, that's on you.
Yeah, its not my intention to stall this PR. As I said I agree with it as is.
There was a problem hiding this comment.
OK. When the review is comment-only it's hard to tell where change requests fall on the spectrum from "it would be nice if..." to "if you don't change this I'll block it".
I usually try pretty hard to accommodate requests (I spent an hour trying to write up why I couldn't make partitions independently configurable before I figured out how to make it work), but this one just isn't going to work at this time.
@pkral78 if github lets you select "Approve" in the review box, or change the review type from "Comment" to "Approve", please do. Comment-only approvals are nice, but the folks who merge won't see them. If it won't let you do that, ok. Thanks for your input; it's improved the solution significantly.
| lcp->prog = lfs_api_prog; | ||
| lcp->erase = lfs_api_erase; | ||
| lcp->sync = lfs_api_sync; | ||
| lcp->read_size = read_size; |
There was a problem hiding this comment.
@pkral78 Here is where lfs_config must be mutable. The pointers to the device-private functions get assigned, as well as information about where in the flash device the specific partition is located.
There was a problem hiding this comment.
No it does not need to, those are just immutable function pointers or constants known at compile time. I see block size is get from get_block_size call, however I don't understand why it need to be calculated at runtime, when it will be always same for same target configuration.
There was a problem hiding this comment.
prog, erase, and sync are known at compile time, but they're private to this module. How do you expect to be able to set them at compile time in const struct stored in flash and declared in a different module?
The flash characteristics are determined at runtime because Zephyr's devicetree implementation only gives you the flash area identifier. The remaining information simply isn't available readily at compile time.
There was a problem hiding this comment.
prog,erase, andsyncare known at compile time, but they're private to this module. How do you expect to be able to set them at compile time in const struct stored in flash and declared in a different module?
And does need to be private ?
The flash characteristics are determined at runtime because Zephyr's devicetree implementation only gives you the flash area identifier. The remaining information simply isn't available readily at compile time.
Yes you're right, but it's rather DT limitation (an area for improvement?).
I think that these two questions lead to decision if we could accept this PR as is and left my comments for future improvements. I am for accepting this PR as it is super useful and well written.
Remove the disabled link interface commands. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
|
Rebased on merged spi nor fixes, removed redundant target support in test. |
|
Looking good, thanks for all your work. I haven't had time to do a deep review, but I've started upgrading my local tree to a newer zephyr + your branch. One build issue: undefined |
|
@pabigot PR looks fine to me, it is tremendous job. Please answer to my comments, and I will accept it. |
|
CC @geky, in case you are interested in this integration |
littlefs is a fail-safe filesystem from ARM Mbed that has wear-leveling capabilities. Signed-off-by: Peter A. Bigot <pab@pabigot.com> Signed-off-by: Jim Paris <jim@bolt.io>
Uses a littlefs file system to maintain a boot counter. Also tests some other functions. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Add support for the littlefs file system in the fs shell. Update the sample to use the same partition configuration as the littlefs example for the SPI NOR test platform. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
The previous way of creating an absolute path relied on snprintf(), and when used with newlib gcc warned that the output could be truncated before the last format character. Rework to use code that doesn't rely on snprintf. See discussion at https://bugzilla.redhat.com/show_bug.cgi?id=1431678 Also ensure that cwd is always NUL-terminated, and use the utility function to create the absolute path in cmd_trunc. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Verify all FS API calls using the nRF52840 development kit. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Existing file system implementations do not provide the special "." (current) and ".." (parent) directory entries in the readdir results. littlefs does. Remove these entries in the abstraction layer. This simplifies code in higher level consumers that aren't prepared to see them. Consumers like FUSE that need them can put them back without having to worry about conflicts. Closes issue zephyrproject-rtos#17951 Signed-off-by: Peter A. Bigot <pab@pabigot.com>
There's desire to be able to customize parameters on a per-filesystem basis, which means we need a way to override the Kconfig defaults which are global. This also means the littlefs data structure cannot own the cache and lookahead buffers. Switch to using a macro to define the littlefs data structure. The default version uses the Kconfig constants. A custom one takes arguments providing the most likely partition-specific parameters. Finally the user is free to bypass the helper macros and set any parameters desired, though validation is limited and only present when CONFIG_DEBUG is enabled. Extend the test suite with a performance module, which confirms that these settings have an impact proportional to the log of changes to the cache or IO sizes. Signed-off-by: Peter A. Bigot <pab@pabigot.com>
Updated in the Zephyr fork and this PR. (Weird; something must have changed in zephyr, since originally we had to use newlib for this.) |
|
Oh! This is very cool. Thanks for the putting this together @pabigot. I'm going to have to add reference to this in littlefs if that's cool with you guys. |
You're welcome; and again thanks to @jimparis for the starting point.
I think that should be fine; @carlescufi is there any specific phrasing we would like from a project perspective? |
@geky @pabigot, no, since this is text to be added to an external project all that I could ask for is that Zephyr to be spelled correctly :) |
littlefs is a wear-leveling file system with power-loss resilience and bounded memory use, suitable for use on Zephyr flash partitions including SPI NOR devices and SOC flash devices.
This commit, based on an initial implementation provided by @jimparis, adds support for the file system, with an example and shell support.