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

sys/vfs: add file-system auto-mount #17341

Merged
merged 7 commits into from Feb 9, 2022
Merged

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Dec 4, 2021

Contribution description

This adds a way to let boards define automatic mount points for file systems, similar to what fstab provides.
This also bring the handling of the FAT VFS integration more in line with the other file systems.
The user no longer has to manually manage the fatfs_mtd_devs array when using the VFS layer.

Testing procedure

In e.g. examples/default add

USEMODULE += vfs
USEMODULE += vfs_auto_format
USEMODULE += littlefs2

This is now all that is needed to add file system support to an application. The vfs_auto_format module will attempt to format the file system if mounting fails (no file system present yet).

ls and the vfs commands should work on the shell.

You can add auto-mounts to an existing board by adding a single macro to the compilation unit in which the MTD is defined:

VFS_AUTO_MOUNT(littlefs2, &mtd0_dev.base, "/", 0);

Issues/PRs references

vfs_auto_format requires #14430 to work on fatfs

@github-actions github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: drivers Area: Device drivers Area: examples Area: Example Applications Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework Platform: native Platform: This PR/issue effects the native platform labels Dec 4, 2021
@benpicco benpicco added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 4, 2021
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 5, 2021
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

A couple of nitpicks.

examples/filesystem/main.c Outdated Show resolved Hide resolved
pkg/fatfs/fatfs_vfs/fatfs_vfs.c Show resolved Hide resolved
sys/include/vfs.h Outdated Show resolved Hide resolved
boards/native/board_init.c Outdated Show resolved Hide resolved
boards/native/board_init.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Area: cpu Area: CPU/MCU ports label Dec 6, 2021
@benpicco benpicco added this to Backlog / Proposals in RIOT Sprint Day via automation Dec 13, 2021
@benpicco benpicco requested a review from kfessel January 4, 2022 09:10
This avoids a cyclic include of mtd.h
@benpicco benpicco mentioned this pull request Feb 3, 2022
@benpicco
Copy link
Contributor Author

benpicco commented Feb 3, 2022

I added a vfs_auto_mount default module that gets disabled by the example.

@benpicco benpicco requested a review from chrysn February 8, 2022 14:56
@benpicco
Copy link
Contributor Author

benpicco commented Feb 8, 2022

I changed

VFS_AUTO_MOUNT(littlefs2, mtd0_dev, "/", 0);

to

VFS_AUTO_MOUNT(littlefs2, &mtd0_dev.base, "/", 0);

to reduce preprocessor usage

@chrysn
Copy link
Member

chrysn commented Feb 9, 2022

OK, please squash while I'm doing some more testing. If you like, drop a3fa01b (the .dev to .base rename) while doing so, but the consistency doesn't do any harm.

@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 9, 2022
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

My tests passed, both on native and with a modified mtd_flash on the microbit-v2, both with littlefs2. One test I'd like to add but failed so far was with fatfs (at least on native; on emulated MTD there seems to be major SNAFU when the block device doesn't have 512 byte blocks/sectors/pages/whatever and fatfs just copies a page into its 512-sized buffer...); while on master I did succeed mounting a FS created using #14430's formatting, the same test failed on this branch (even only looking at the examples/filesystem example that shouldn't have changed much); can you confirm this works for you?

(Not relevant any more, but for the record: The switch over from using .base in VSF_AUTO_MOUNT toward passing the pointer to the actual device *mtd_dev_t was done on to avoid starting off a preprocessor-based duck-typing system; see some Matrix chat from 2022-02-08).

During my tests I've wound up in some loop like this once (from a board's mount line VFS_AUTO_MOUNT(fatfs, &some_mtd, "/internal", 0);, likely from the above-mentioned SNAFU):

vfs_mount: -> "/internal" (0xb155), 0x20000200
vfs_bind: bound 1
vfs_bind: 2, 1, 0x2000048c, 0x2
vfs_bind: bound 2
vfs0: mounting as '/internal'
vfs_mount: 0x2000050c

I wonder whether a warning is warranted that anything that sends a device into a reboot loop (while running with DEVELHELP=0) might cause quick flash wear with file systems that perform writes on mount. (I can't tell quickly whether ours really do; I know that there is some writing going on with Linux file systems, judging from warning notes about data forensics I've read some time ago).

Other than that, things are looking good; the new in-code commends are just that (I'd be OK going ahead with this even without them merged, but I'd like them to be considered).


mtd_dev_t *mtd0 = &mtd0_dev.base;
#endif

Copy link
Member

@chrysn chrysn Feb 9, 2022

Choose a reason for hiding this comment

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

The automounter only depending on VFS and not on MTD (which, above, defines mtd0 in the first place) may look odd in later review, but is really OK: All mentions later to mtd0 are behind further ifdefs that (transitively) do depend on MTD because their backend is MTD.

If, at some time, someone were to write a native filesystem pass-through, its backend would not be an MTD device but a host POSIX path, thus its second argument to VFS_AUTO_MOUNT would become just that, and no dependency on the MTD module is there for that case.

[edit: highlighting that things are OK here, primarily for the benefit of people coming back to this issue, wondering]

sys/include/vfs.h Outdated Show resolved Hide resolved
sys/include/vfs.h Show resolved Hide resolved
@benpicco
Copy link
Contributor Author

benpicco commented Feb 9, 2022

I also noticed that fatfs was broken on native but would work with tests/pkg_fatfs_vfs. This made me scratch my head, but your comment about the sector size made me look closer: And indeed the Makefile of tests/pkg_fatfs_vfs does

CFLAGS += -DMTD_PAGE_SIZE=$(MTD_PAGE_SIZE)
CFLAGS += -DMTD_SECTOR_SIZE=$(MTD_SECTOR_SIZE)

and even the Readme of examples/filesystem has a Section about this.

Now I also don't like reading Readmes, should we just

#ifndef MTD_PAGE_SIZE
#ifdef MODULE_FATFS_DISKIO_MTD
#define MTD_PAGE_SIZE           (512)
#else
#define MTD_PAGE_SIZE           (256)
#endif
#endif

#ifndef MTD_SECTOR_SIZE
#ifdef MODULE_FATFS_DISKIO_MTD
#define MTD_SECTOR_SIZE         (512)
#else
#define MTD_SECTOR_SIZE         (4096)
#endif
#endif

@chrysn
Copy link
Member

chrysn commented Feb 9, 2022

Now I also don't like reading Readmes, should we just

... and I missed that too.

I have a soft preference for fatfs_diskio_mtd to produce an (unfortunately runtime) error if the MTD device's sizes don't agree with it. Tuning the defaults for native's MTD is certainly a good idea, the check could be done independently as well.

@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Feb 9, 2022
@chrysn
Copy link
Member

chrysn commented Feb 9, 2022

Thank you, please squash.

(Reading that PR I also realized that fatfs can not be expected to work on many flash-backed MTDs given that thee have 4k erasure units but FAT overwrites in 512 byte blocks; well, probably no reason to use FAT there anyway, for its main selling point is "plug the thing into a PC and it'll work with it").

This makes FAT behave more like the other file systems supported by VFS.
The `fatfs_mtd_devs` array is populated internally so the application does
not have to handle this.
This brings it in line with the other MTD implementations.
@benpicco
Copy link
Contributor Author

benpicco commented Feb 9, 2022

All green here :)

Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

And green here. Thank you!

RIOT Sprint Day automation moved this from Backlog / Proposals to Waiting For Murdock Feb 9, 2022
@benpicco benpicco merged commit be45400 into RIOT-OS:master Feb 9, 2022
RIOT Sprint Day automation moved this from Waiting For Murdock to Done Feb 9, 2022
@benpicco
Copy link
Contributor Author

benpicco commented Feb 9, 2022

Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants