Add LFS_READONLY define, to allow smaller builds providing read-only mode #480

maximevince · 2020-10-28T15:17:56Z

This PR tries to solve #162 , by providing an LFS_READONLY define.

LittleFS seems to be preferred over SPIFFS in many places and projects right now.
However, for my bootloader / firmware upgrade use-case, the code size matters a lot.
A bootloader could do with a read-only version of the file system, which should be a lot smaller in size.

With SPIFFS I am able to get a read-only version without caching compiled to around 3 KB (compiled for Cortex-M0),
while the read-write version with caching weighs in at around 13 KB, which is comparable to LittleFS.

By defining out a bunch of write-related functionality, I was able to reduce the read-only version of LittleFS to around 5 KB for that same Cortex-M0 target, which seems acceptable for bootloader use.

I have separately #ifdef'ed every function that is only needed for writing functionality.
However, I'm not sure if this is the preferred way, of if you'd rather have a single #ifdef around the whole lump of code.

Let me know if I can do anything to improve this PR.

Cheers and thanks for the awesome project! 👍

… mode

geky · 2020-11-14T14:50:30Z

Hi @maximevince, thanks for creating a PR! Sorry being late in getting to this. This looks great.

I have separately #ifdef'ed every function that is only needed for writing functionality.
However, I'm not sure if this is the preferred way, of if you'd rather have a single #ifdef around the whole lump of code.

I hadn't thought about this, it has the nice side-effect that it is easy to know which functions are read-only in the source code 👍

lfs.c

lfs.h

geky · 2020-11-14T19:01:13Z

Also the CI was broken but should be fixed now. It should trigger on the next push to this branch.

dpgeorge · 2020-11-17T00:07:47Z

It's good to see support for read-only mode in littlefs. I did make a very similar patch to this (with the option called LFS_ENABLE_WRITES) but did not get time to make a PR out of it. In my use of it in a bootloader, littlefs with write support takes about 11000 bytes. In a read-only build that goes down to about 4200 bytes (on a Cortex-M7). That's very similar to what's reported above.

Will be good to see this merged!

maximevince · 2020-11-17T09:58:13Z

It's good to see support for read-only mode in littlefs. I did make a very similar patch to this (with the option called LFS_ENABLE_WRITES) but did not get time to make a PR out of it. In my use of it in a bootloader, littlefs with write support takes about 11000 bytes. In a read-only build that goes down to about 4200 bytes (on a Cortex-M7). That's very similar to what's reported above.

Will be good to see this merged!

Alright, nice to hear from you @dpgeorge.
I believe we briefly conversed back when I was working on picoTCP and we made a port in the early days of micropython.

If you see any differences between the parts of the code I have #ifdef-ed, or where it could still be made smaller, let me know!

maximevince · 2020-11-17T14:44:18Z

Just updated the PR with a new commit:

Removed write-related functions from the header altogether, and adjusted lfs.c accordingly
Replaced LFS_ERR_NOSYS by LFS_ERR_ROFS (-30)

I am not entirely sure what should happen to int lfs_file_sync(lfs_t *lfs, lfs_file_t *file):
Is this function at all needed for LittleFS in read-only mode? Right now I only #ifdefed the LFS_F_DIRTY portion out of it.
And I have the same question for lfs_file_flush(lfs, file), too.

geky · 2020-11-18T16:17:11Z

Oh that is a good question. I'm going to say no to both, I don't see how they could be used in read-only mode.

If there some strange use case we're missing, they are easy to add back into the read-only build. It would be harder to remove them later.

dpgeorge · 2020-11-19T01:08:42Z

lfs.c

    if ((flags & 3) != LFS_O_RDONLY) {
+#ifdef LFS_READONLY
+        return LFS_ERR_ROFS;
+#else


I would think that files can only ever be opened in LFS_O_RDONLY mode, so this entire if-block can just be excluded with the #if .

There's also another similar block down below in this function which could also be #if'd out.

Actually that's a good point, an LFS_ASSERT((flags & 3) == LFS_O_RDONLY) would work better here.

I am not a big fan of "crashing" with an assert, instead of returning an error when a lfs_file_opencfg() is called with the wrong arguments, but I'll implement it that way, if you want.

Let's say an OS tries to write to a file mounted using the LittleFS filesystem, but LittleFS is compiled for read-only. Wouldn't it be more elegant to return an error indicating it's not possible to write, instead of asserting ?

At the moment, it should be an assert for consistency (writing a file opened as rdonly currently asserts, for example).

Returning an error can be more graceful if there is an OS that understands that error, but for many littlefs applications this isn't the case. It's easy for the context of the error to get lost as "file open failed". Looking at POSIX errors, which littlefs follows, writing to an rdonly file only returns EINVAL. An assert takes you directly to the source of the problem, sort of like a poor-man's stack trace.

There's also a code cost for returning/handling errors, an assert can be dropped at compile time. Runtime checks can be added in the OS, but they can't be removed. This makes asserts preferred for conditions that should never happen unless there is a programmer mistake.

But you're not the first person to comment on this, maybe it would be worth adding a configuration option to change the use-error asserts to return errors. The next steps there would be to create a separate issue/PR.

Alright, thanks for the detailed explanation. I can understand the reasoning indeed.

I have just pushed a new commit implementing the LFS_ASSERT for write attempts, as well as more code commented out, in case of LFS_READONLY.

One idea would be to also #if out the write-based constants, like LFS_O_WRONLY. Then it would be impossible to compile code that opened files for writing.

Oh, that's an even better idea! @dpgeorge with the great ideas

Okay, I just pushed another commit implementing this idea.
I had to make some extra #ifdef sections in other functions checking for these flags, so please carefully review that.
What I also did, is marking each #endif section with a comment indicating which #ifdef it is corresponding to.

dpgeorge · 2020-11-19T01:14:48Z

This is now looking almost exactly the same as the patch I made.

… LFS_READONLY

lfs.c

lfs.h

lfs.c

dpgeorge · 2020-11-21T03:53:38Z

lfs.h

+#endif

    // internally used flags
    LFS_F_DIRTY   = 0x010000, // File does not match storage


Is it possible (and worth it) also #if'ing these constants like LFS_F_DIRTY which are not supposed to be used in read-only mode?

It was easy for LFS_F_DIRTY and LFS_F_WRITING.

However, I am not sure about LFS_F_ERRED.
It's documented as "An error occurred during write", but it has one non-obvious use at the end of int lfs_file_opencfg().
It seems you could end up there when e.g. lfs_malloc() fails, or when opening a file for read hits an LFS_ERR_NOENT (here) (among other possibilities).
Both of which cases do not seem to be strictly write-related.
So I conclude that this constant is needed in LFS_READONLY mode as well.

This is implemented in 35cc31e

Ah, it's not actually used in lfs_file_close but in the implicit lfs_file_sync. The purpose of the LFS_F_ERRED flag is to tell lfs_file_close to release resources without writing out to disk. lfs_file_opencfg immediately calls lfs_file_close if an error occurs.

You have the implicit lfs_file_sync removed in LFS_READONLY mode, so it should be safe to remove this line as well.

I only noticed this comment now. I have ifdefed LFS_F_ERRED out completely. (99c960b)

…ing bracket

lfs.c

geky · 2020-11-22T05:58:44Z

This looks great, thanks for this! Will bring this in on the next minor release.

I added a Travis job, so we should see a read-only build size in the CI results now.

Add LFS_READONLY define, to allow smaller builds providing read-only mode

Add LFS_READYONLY define, to allow smaller builds providing read-only…

8e6826c

… mode

geky added the v2.3 label Nov 14, 2020

geky added this to the v2.3 milestone Nov 14, 2020

geky added next patch next minor and removed next patch labels Nov 14, 2020

geky reviewed Nov 14, 2020

View reviewed changes

lfs.c Outdated Show resolved Hide resolved

geky reviewed Nov 14, 2020

View reviewed changes

lfs.h Outdated Show resolved Hide resolved

geky added the needs test all fixes need test coverage to prevent regression label Nov 14, 2020

geky mentioned this pull request Nov 14, 2020

Add thread safe wrappers #470

Merged

geky removed the needs test all fixes need test coverage to prevent regression label Nov 16, 2020

LFS_READONLY: undef unavailable function declarations altogether

46b5033

dpgeorge reviewed Nov 19, 2020

View reviewed changes

geky changed the title ~~Add LFS_READYONLY define, to allow smaller builds providing read-only mode~~ Add LFS_READONLY define, to allow smaller builds providing read-only mode Nov 19, 2020

LFS_READONLY: even less code, assert on write attempts

882aae5

geky added needs minor version new functionality only allowed in minor versions and removed next minor labels Nov 20, 2020

LFS_READONLY: remove LFS_O_WRONLY and other flags when compiling with…

69da107

… LFS_READONLY

geky reviewed Nov 20, 2020

View reviewed changes

lfs.c Outdated Show resolved Hide resolved

lfs.h Outdated Show resolved Hide resolved

lfs.c Outdated Show resolved Hide resolved

lfs.c Outdated Show resolved Hide resolved

dpgeorge reviewed Nov 21, 2020

View reviewed changes

maximevince added 3 commits November 21, 2020 08:35

LFS_READONLY: do not annotate #endif, as requested

0360629

LFS_READONLY: move ifdef before comments blocks, rework dangling open…

83fd2e8

…ing bracket

LFS_READONLY: ifdef file flags that are not needed in read-only mode

35cc31e

geky reviewed Nov 21, 2020

View reviewed changes

lfs.c Outdated Show resolved Hide resolved

geky reviewed Nov 21, 2020

View reviewed changes

lfs.c Outdated Show resolved Hide resolved

lfs.c Show resolved Hide resolved

maximevince and others added 2 commits November 21, 2020 21:12

LFS_READONLY: slight refactor

94c9c4c

Added read-only build+size reporting to CI

5c4e375

geky added next minor and removed needs minor version new functionality only allowed in minor versions labels Nov 22, 2020

geky approved these changes Nov 22, 2020

View reviewed changes

LFS_READONLY: ifdef LFS_F_ERRED out as well

99c960b

geky changed the base branch from master to devel December 4, 2020 04:47

geky merged commit 67a95d7 into littlefs-project:devel Dec 4, 2020

geky added a commit that referenced this pull request Dec 4, 2020

Merge pull request #480 from maximevince/master

b9fa33f

Add LFS_READONLY define, to allow smaller builds providing read-only mode

geky mentioned this pull request Dec 4, 2020

Minor release: v2.3 #495

Merged

ronaldtse mentioned this pull request May 20, 2021

Consider replacing libsquash with littlefs or dwarFS metanorma/ruby-packer#16

Closed

Add LFS_READONLY define, to allow smaller builds providing read-only mode #480

Add LFS_READONLY define, to allow smaller builds providing read-only mode #480

Uh oh!

Conversation

maximevince commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geky commented Nov 14, 2020

Uh oh!

Uh oh!

Uh oh!

geky commented Nov 14, 2020

Uh oh!

dpgeorge commented Nov 17, 2020

Uh oh!

maximevince commented Nov 17, 2020

Uh oh!

maximevince commented Nov 17, 2020

Uh oh!

geky commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Nov 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpgeorge commented Nov 19, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geky commented Nov 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maximevince commented Oct 28, 2020 •

edited

Loading

geky Nov 19, 2020 •

edited

Loading