Standardized FileSystem init/deinit/reset to match KVStore API #8700

geky · 2018-11-09T21:58:05Z

Description

This standardizes init/deinit/reset to match behaviour in KVStore and BlockDevice. This makes init consistent across all three APIs.

Standardized:

FileSystem::mount -> FileSystem::init
FileSystem::unmount -> FileSystem::deinit
FileSystem::reformat -> FileSystem::reset

Note that init/reset do not accept BlockDevice arguments. This paves the way to standardizing classes to be configured only at construction time.

To make this work, recursive init/deinit functions were needed similar to the BlockDevices, and these init/deinit functions are lazily called during the first filesystem operations to match the existing behavior where init/deinit are ignored completely.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Related to #8667
cc @armmbed/mbed-os-storage, @deepikabhavnani, @dannybenor, @davidsaada

Standardized: FileSystem::mount -> FileSystem::init FileSystem::unmount -> FileSystem::deinit FileSystem::reformat -> FileSystem::reset Note that init/reset no longer accept BlockDevice arguments. This paves the way to standardizing classes to be configured only at construction time. To make this work, recursive init/deinit functions were needed similar to the BlockDevices, and these init/deinit functions are lazily called during the first filesystem operations to match the existing behavior where init/deinit are ignored completely.

davidsaada

Great change, great timing. 👍

deepikabhavnani

Looks good to me, just few queries for my understanding

deepikabhavnani · 2018-11-09T22:30:26Z

features/storage/filesystem/fat/FATFileSystem.cpp

 }

 FATFileSystem::~FATFileSystem()
 {
-    // nop if unmounted
-    unmount();
+    // noop if unmounted


Is the comment still valid? Unmounted - Not initialized?

Yep! The reference counting also handles more deinits than inits:

if (_ref > 0) { _ref -= 1; if (_ref == 0) { res = f_mount(NULL, _fsid, 0); _ffs[_id] = NULL; _id = -1; } } }

deepikabhavnani · 2018-11-10T07:04:43Z

features/storage/filesystem/FileSystem.h

+     */
+    virtual int deinit();
+
+    /** Reformats the underlying filesystem


Should we specify explicitly that file system will be initialised and mounted as part of reset call?

deepikabhavnani · 2018-11-10T07:09:07Z

features/storage/filesystem/fat/FATFileSystem.cpp

 }

 /* See http://elm-chan.org/fsw/ff/en/mkfs.html for details of f_mkfs() and
 * associated arguments. */
 int FATFileSystem::format(BlockDevice *bd, bd_size_t cluster_size)
 {
-    FATFileSystem fs;
-    fs.lock();


Not sure why locking is removed in format operation..

Ah, it's because I narrowed the scope of the fs instance.

The file system only exists inside this function, so locking didn't have any benefit.

deepikabhavnani · 2018-11-10T07:18:48Z

features/storage/filesystem/littlefs/LittleFileSystem.h

+     *
+     *  The init function may be called multiple times recursively as long
+     *  as each init has a matching deinit. If not called, the file system
+     *  will be mounted on the first file system operation.


Reset will be considered as valid call before init?

dannybenor · 2018-11-10T10:35:26Z

@geky I need some review here to understand the motivation, will look at this after the weekend

offirko · 2018-11-11T06:28:58Z

Lazy init() comes with its toll .. lots of copies of "if (_ref == 0) { // make sure we're mounted ..." .. not sure if its avoidable..
Is Lazy init() supposed to handle only cases where the user didn't call init() at all , or also cover cases where the user did call init() but after he did other stuff (maybe timing issue)

I thought of these scenarios e.g. (maybe its not legit or I got it wrong..)

Scenario (A):

Init() : _ref=1
Remove("blabla") : _ref=1
Deinit() : ref=0
Final state is Deinitialized.

Scenario (B):

Remove("blabla") : _ref=1 (lazy init)
Init() : _ref=2
Deinit() : ref=1
Final state is Initialized.

dannybenor · 2018-11-11T14:30:20Z

@geky This change defines existing API as deprecated and introduces new APIs.
It lacks testing
It is issued at a very late stage of 5.11, therefore I do not think it fits 5.11

geky · 2018-11-12T19:07:41Z

@dannybenor, up to you. It would be nice to have the big storage changes in the same release.

deepikabhavnani · 2018-11-12T19:48:34Z

It lacks testing

It will be good to have test cases updated in same PR. Apart from that it looks good to me 👍

adbridge · 2018-11-14T15:02:53Z

@dannybenor @geky Could you please confirm whether you expect this to come into 5.11 or not ? As this is changing APIs including deprecation then if it does not make 5.11 it is delayed to 5.12....

dannybenor · 2018-11-14T15:33:55Z

This should be delayed to 5.12. Needs review outside github.

0xc0170 · 2018-11-19T10:07:47Z

This should be delayed to 5.12. Needs review outside github.

The version label was updated.

cmonr · 2018-12-14T00:34:15Z

@ARMmbed/mbed-os-storage Any progress? This has idled for nearly a month.

dannybenor · 2018-12-16T09:46:47Z

@cmonr This needs review outside github. Not sure how to mark it but do expect this to be solved soon

NirSonnenschein · 2018-12-16T15:29:46Z

@geky , FYI: this now needs a rebase as well.

cmonr · 2018-12-21T16:53:51Z

@dannybenor That comment is sufficient. Let us know if the review will end up taking more than two weeks.

0xc0170 · 2019-01-16T12:28:28Z

As this is being discussed offlline, we will close this PR. Please reopen with an update

cmonr requested a review from a team November 9, 2018 22:15

cmonr added needs: review release-version: 5.11.0-rc1 labels Nov 9, 2018

cmonr requested review from deepikabhavnani, dannybenor and davidsaada November 9, 2018 22:15

geky force-pushed the fs-consistent-init branch from bcef816 to 833379c Compare November 9, 2018 22:27

davidsaada approved these changes Nov 10, 2018

View reviewed changes

deepikabhavnani reviewed Nov 10, 2018

View reviewed changes

NirSonnenschein added the risk: R label Nov 18, 2018

0xc0170 added release-version: 5.12.0-rc1 and removed risk: R release-version: 5.11.0-rc1 labels Nov 19, 2018

0xc0170 closed this Jan 16, 2019

0xc0170 removed the needs: review label Jan 16, 2019

Standardized FileSystem init/deinit/reset to match KVStore API #8700

Standardized FileSystem init/deinit/reset to match KVStore API #8700

Uh oh!

Conversation

geky commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

davidsaada left a comment

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani left a comment

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

geky Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Nov 10, 2018

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Nov 10, 2018

Choose a reason for hiding this comment

Uh oh!

geky Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

deepikabhavnani Nov 10, 2018

Choose a reason for hiding this comment

Uh oh!

dannybenor commented Nov 10, 2018

Uh oh!

offirko commented Nov 11, 2018

Uh oh!

dannybenor commented Nov 11, 2018

Uh oh!

geky commented Nov 12, 2018

Uh oh!

deepikabhavnani commented Nov 12, 2018

Uh oh!

adbridge commented Nov 14, 2018

Uh oh!

dannybenor commented Nov 14, 2018

Uh oh!

0xc0170 commented Nov 19, 2018

Uh oh!

cmonr commented Dec 14, 2018

Uh oh!

dannybenor commented Dec 16, 2018

Uh oh!

NirSonnenschein commented Dec 16, 2018

Uh oh!

cmonr commented Dec 21, 2018

Uh oh!

0xc0170 commented Jan 16, 2019

Uh oh!

Uh oh!

geky commented Nov 9, 2018 •

edited

Loading