pkg/fatfs: fatfs_vfs: wire up format() #14430

benpicco · 2020-07-02T17:23:33Z

Contribution description

This hooks up vfs_format to f_mkfs().
fatfs also provides a f_fdisk() function to create a partition table, but this doesn't seem to be necessary.

Since pulling in the format logic adds 3.5k .text and will not be needed on many applications, it is added as a fatfs_vfs_format pseudo-module.

Testing procedure

tests/pkg_fatfs_vfs will now format the storage before testing, so beware 😉
This is consistent with the other file-system tests though.

Issues/PRs references

maribu

Code looks good, two remarks inline

pkg/fatfs/fatfs_vfs/fatfs_vfs.c

bergzand

Tested with an sdcard on the nucleo-f401re. Works as advertised!

bergzand · 2020-12-15T10:52:58Z

pkg/fatfs/fatfs_vfs/fatfs_vfs.c

+    fatfs_desc_t *fs_desc = mountp->private_data;
+    char volume_str[TEST_FATFS_MAX_VOL_STR_LEN];
+
+    BYTE *work = malloc(FF_MAX_SS);


Do we really need malloc here?

FF_MAX_SS is 512 byte so I'd rather not put it on the stack

The general (or maybe my strict?) expectation of RIOT functions without any admonitions is that they don't use the heap, are reentrant / thread safe, and don't overexert the stack. This can't hold that, so we should put a warning around the general vfs_format function. So what'd that best be?

"Not thread safe! Do not call this concurrently on different (let alone the same) file system.": Similar to the warning already on vfs_iterate_mounts; would give license to use a static buffer.

Advantages: Consistent with other vfs functions; memory limitations are visible (with nm showing the culprit) at build time if no heap allocation is used.

"On some file systems (currently \ref FatFS), this may resort to dynamic memory allocation, and return -ENOMEM if insufficient memory is available." This would justify the current behavior.

Advantage: Works well on otherwise idle systems even with little memory.

"Beware that some file systems (currently \ref FatFS) have extensive stack requirements when running this". Very vague, and has bad error behavior.

Based on this, I'm softly leaning toward a const allocation.

How about making it configurable? Allowing a static buffer to be provided, otherwise allocating one?

I added CONFIG_FATFS_FORMAT_ALLOC_STATIC

Is this a good enough compromise @bergzand? (I would personally prefer that the default be avoiding malloc though)

After letting this brew for a year I think I'm fine with anything as long as it is documented so the user doesn't run into unexpected troubles. Having it configurable seems like a perfect solution to me

I think now that the warning I sketched above is superfluous as mountpoint-related vfs stuff is behind a mutex anyway (but please verify, I'm on mobile and can't easily read long code here)

[edit because for some reason I can't reply to this particular comment:]

The warning should still be there, for vfs_format only blocks the mount mutex briefly, and releases it for the formatting operation.

Not thread safe! Do not call this concurrently on different (let alone the same) file system. (For example, FatFS uses static buffers during formatting in some configurations.)

benpicco · 2022-02-09T15:19:14Z

So what's the verdict on this one?

chrysn · 2022-02-14T15:46:29Z

Can't do the full review right now, but seeing #17653 I noticed that (especially in combination) much of the unsightliness of the native/fatfs README can go away (because now fatfs can be formatted just like any other FS) -- to the point where after the two, the section could be gone completely (because all that's left is switching around the FS, and that should be obvious)

chrysn · 2022-02-14T17:36:21Z

pkg/fatfs/Makefile.dep

@@ -1,5 +1,9 @@
 USEMODULE += fatfs_diskio_mtd
 USEMODULE += mtd

+ifneq (,$(filter vfs_auto_format,$(USEMODULE)))
+  USEMODULE += fatfs_vfs_format


This might suffice as d default module (maybe the FAT isn't automounted)

chrysn · 2022-02-14T17:43:09Z

pkg/fatfs/fatfs_vfs/fatfs_vfs.c

+    fatfs_desc_t *fs_desc = mountp->private_data;
+    char volume_str[TEST_FATFS_MAX_VOL_STR_LEN];
+
+    BYTE *work = malloc(FF_MAX_SS);


I think now that the warning I sketched above is superfluous as mountpoint-related vfs stuff is behind a mutex anyway (but please verify, I'm on mobile and can't easily read long code here)

[edit because for some reason I can't reply to this particular comment:]

The warning should still be there, for vfs_format only blocks the mount mutex briefly, and releases it for the formatting operation.

Not thread safe! Do not call this concurrently on different (let alone the same) file system. (For example, FatFS uses static buffers during formatting in some configurations.)

chrysn · 2022-02-16T13:26:12Z

Thanks. Please squash and rebase onto current master, which will enable the changes that I think are suitable in examples/filesystem/REAMDE mergable (probably best fit in a separate commit, but if you prefer squash them right in):

file system list in lines 10/11 could now include FatFS because it can be formatted
Everything down of line 80 can IMO go away now, because FatFS needs no more special treatment than the other FSs. (The tar unpacking was just added there because formatting was not available.

FAT now behaves just like all other FSs and does not need special treatment.

benpicco · 2022-02-16T13:37:59Z

main(): This is RIOT! (Version: 2022.04-devel-398-g70d98-pkg/fatfs_format)
constfs mounted successfully
> mount
mount
Error while mounting /sda...try format
> format
format
/sda successfully formatted
> mount
mount
/sda successfully mounted
> ls /sda
ls /sda
total 0 files
> vfs df
vfs df
Mountpoint              Total         Used    Available     Capacity
/sda                     2048          102         1946       4%
/const                     27           27            0     100%

chrysn

I think this is ready to be merged. Thank you!

benpicco · 2022-02-16T14:08:12Z

Thank you for pushing this forward!

benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports Area: fs Area: File systems labels Jul 2, 2020

benpicco requested a review from chrysn July 2, 2020 17:23

benpicco requested review from aabadie, fjmolinas, leandrolanzieri, MichelRottleuthner and smlng as code owners July 2, 2020 17:23

benpicco requested a review from vincent-d July 2, 2020 17:23

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 8, 2020

benpicco force-pushed the pkg/fatfs_format branch from bc5ead1 to 82ffbea Compare August 16, 2020 17:55

benpicco added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Sep 8, 2020

benpicco force-pushed the pkg/fatfs_format branch from 82ffbea to 3aafdd9 Compare September 8, 2020 14:17

benpicco added State: waiting for CI update State: The PR requires an Update to CI to be performed first and removed State: waiting for CI update State: The PR requires an Update to CI to be performed first labels Sep 10, 2020

benpicco requested a review from maribu September 25, 2020 13:26

maribu reviewed Sep 25, 2020

View reviewed changes

pkg/fatfs/fatfs_vfs/fatfs_vfs.c Outdated Show resolved Hide resolved

pkg/fatfs/fatfs_vfs/fatfs_vfs.c Outdated Show resolved Hide resolved

benpicco force-pushed the pkg/fatfs_format branch from 0ec13cb to 0d27ad2 Compare September 28, 2020 17:51

benpicco force-pushed the pkg/fatfs_format branch from 0d27ad2 to 2c5969f Compare October 7, 2020 13:53

benpicco removed the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Oct 7, 2020

benpicco requested a review from kaspar030 October 24, 2020 19:48

bergzand reviewed Nov 17, 2020

View reviewed changes

pkg/fatfs/fatfs_vfs/fatfs_vfs.c Outdated Show resolved Hide resolved

bergzand reviewed Nov 17, 2020

View reviewed changes

bergzand added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Nov 17, 2020

bergzand self-assigned this Nov 17, 2020

bergzand reviewed Dec 15, 2020

View reviewed changes

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

benpicco mentioned this pull request Sep 2, 2021

Small fatfs usability fixes #16800

Merged

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jan 27, 2022

benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 27, 2022

github-actions bot added Area: build system Area: Build system Area: sys Area: System Area: tests Area: tests and testing framework labels Jan 27, 2022

benpicco force-pushed the pkg/fatfs_format branch from c29cb67 to 29efe9e Compare February 7, 2022 17:47

benpicco force-pushed the pkg/fatfs_format branch from 3778aae to f7e5181 Compare February 9, 2022 20:56

benpicco 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

benpicco requested review from bergzand and chrysn and removed request for chrysn February 14, 2022 14:07

chrysn reviewed Feb 14, 2022

View reviewed changes

benpicco added 3 commits February 16, 2022 14:30

pkg/fatfs: fatfs_vfs: wire up format()

5b246d0

tests/pkg_fatfs_vfs: format storage before running the test

9f4fa5a

examples/filesystem: remove dedicated FatFS section from readme

70d9856

FAT now behaves just like all other FSs and does not need special treatment.

benpicco force-pushed the pkg/fatfs_format branch from 7470ce7 to 70d9856 Compare February 16, 2022 13:36

benpicco requested a review from jia200x as a code owner February 16, 2022 13:36

github-actions bot added Area: doc Area: Documentation Area: examples Area: Example Applications labels Feb 16, 2022

chrysn approved these changes Feb 16, 2022

View reviewed changes

benpicco enabled auto-merge February 16, 2022 14:07

chrysn mentioned this pull request Feb 16, 2022

pkg/fatfs_vfs: support FF_FS_TINY=0 #17631

Merged

benpicco merged commit 22a3fc7 into RIOT-OS:master Feb 16, 2022

benpicco deleted the pkg/fatfs_format branch February 16, 2022 16:28

OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022

pkg/fatfs: fatfs_vfs: wire up format() #14430

pkg/fatfs: fatfs_vfs: wire up format() #14430

Uh oh!

Conversation

benpicco commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrysn Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

benpicco Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

bergzand Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

chrysn Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 9, 2022

Uh oh!

chrysn commented Feb 14, 2022

Uh oh!

chrysn Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

chrysn Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrysn commented Feb 16, 2022

Uh oh!

benpicco commented Feb 16, 2022

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 16, 2022

Uh oh!

Uh oh!

benpicco commented Jul 2, 2020 •

edited

Loading

benpicco Dec 15, 2020 •

edited

Loading

chrysn Feb 14, 2022 •

edited

Loading

chrysn Feb 14, 2022 •

edited

Loading