Add support for shrinking a filesystem #1094

sosthene-nitrokey · 2025-04-16T16:06:43Z

This PR adds a new lfs_fs_shrink, which functions similarly to lfs_fs_grow, but supports reducing the block count.

This functions first checks that none of the removed block are in use. If it is the case, it will fail.

This PR adds a new `lfs_fs_shrink`, which functions similarly to `lfs_fs_grow`, but supports reducing the block count. This functions first checks that none of the removed block are in use. If it is the case, it will fail.

geky-bot · 2025-04-17T08:33:47Z

Tests passed ✓, Code: 17220 B (+0.6%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17220 B (+0.6%)	1448 B (+0.0%)	812 B (+0.0%)	Lines	2452/2613 lines (+0.0%)
Readonly	6230 B (+0.0%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1285/1614 branches (+0.2%)
Threadsafe	18088 B (+0.7%)	1448 B (+0.0%)	820 B (+0.0%)		Benchmarks
Multiversion	17292 B (+0.6%)	1448 B (+0.0%)	816 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18880 B (+0.5%)	1752 B (+0.0%)	816 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	18000 B (+0.6%)	1440 B (+0.0%)	812 B (+0.0%)	Erased	1568888832 B (+0.0%)

geky · 2025-05-02T08:10:56Z

Hi @sosthene-nitrokey, thanks for the PR.

I'm going to contradict past me a bit, but I think this would be better implemented as an extension to lfs_fs_grow.

I've come around on the idea of add lfs_fs_shrink to littlefs, since it is also valuable for allowing users to allocate fixed storage for application specific uses (XIP mainly, #692). That being said, it is still a hard problem.

But in obnoxiously POSIX fashion I think we want to avoid adding a new function and instead allow lfs_fs_grow to specify block_counts both larger and smaller than the origin block_count. This would match the behavior of lfs_file_truncate. It's unintuitive but there is a bit of reason behind the madness as it allows you to specify the block_count without caring about the current state. (It may also lead to better code reuse as you may not need the lfs_fs_rewrite_block_count function.)

To avoid unnecessary code cost, shrinking behavior would be opt-in behind ifdef LFS_SHRINK. This opens the door for using a different define, maybe ifdef LFS_SHRINKIFCHEAP (open to better names), for the erroring behavior proposed here.

So, instead of adding lfs_fs_shrink, add ifdef LFS_SHRINKIFCHEAP which enables the shrink-if-not-in-use behavior in lfs_fs_grow. I think this should be a relatively easy change to the proposed PR. But let me know if you have concerns.

Also I appreciate the relevant test cases. Usually I have to beg for those :)

geky

Added some nits to consider. But this is some pretty great code. It would just be nice to move under ifdef LFS_SHRINKIFCHEAP to fit into above future plans.

geky · 2025-05-02T08:12:02Z

lfs.c

-                {tag, &superblock}));
+        lfs_block_t threshold = block_count;
+
+        int err = lfs_fs_traverse_(lfs, lfs_shrink_check_block, &threshold, true);


nit: Please limit to 80 chars/columns (I should really add a CI job for this)

hi OverLength ctermbg=darkblue match OverLength /\%81v./

is a decent way to highlight >80 char lines if you're in vim.

geky · 2025-05-02T08:13:27Z

lfs.c

@@ -5233,38 +5233,67 @@ static int lfs_fs_gc_(lfs_t *lfs) {
 #endif

 #ifndef LFS_READONLY
+static int lfs_fs_rewrite_block_count(lfs_t *lfs, lfs_size_t block_count) {


nit: Naming is a bit weird, but useful, in littlefs. Underscores indicate namespaces/subsystems for the most part. So this should be lfs_fs_rewriteblockcount.

Makes sense. This function will be removed anyway following shrink being part of lfs_fs_grow.

geky · 2025-05-02T08:14:26Z

lfs.c

-            return tag;
-        }
-        lfs_superblock_fromle32(&superblock);
+static int lfs_shrink_check_block(void * data, lfs_block_t block) {


nit: see above, namespace quirkyness: This should be lfs_fs_shrink_checkblock.

geky · 2025-05-02T08:16:01Z

lfs.h

+// Shrinks the filesystem to a new size, updating the superblock with the new
+// block count.
+//
+// Note: This first checks that none of the blocks that are being removed are in use


nit: 80 cols

geky · 2025-05-02T08:16:55Z

lfs.c

-        }
-        lfs_superblock_fromle32(&superblock);
+static int lfs_shrink_check_block(void * data, lfs_block_t block) {
+    lfs_size_t threshold = *((lfs_size_t *) data);


nit: spacing: *((lfs_size_t*)data);

lfs.c

bmcdonnell-fb · 2025-05-02T13:11:48Z

@geky

... I think this would be better implemented as an extension to lfs_fs_grow.

Maybe rename it to lfs_fs_resize?

You could #define lfs_fs_grow lfs_fs_resize, and deprecate lfs_fs_grow.

geky · 2025-05-02T18:53:15Z

Ah, but the name abuse is part of the charm of POSIX, see lfs_file_truncate :)

Users might also think lfs_fs_resize and lfs_fs_size are somehow related (though I do want to rename lfs_fs_size -> lfs_fs_usage at some point, but it's low priority).

sosthene-nitrokey · 2025-05-05T09:48:20Z

Thank you for the review!

I made a fixup commit that applies your suggestions. I'm just not sure how to make it so that LFS_FS_SHRINKCHEAP is defined in the relevant tests (the tests pass if I add it to runners/test_runners.h so it's only about where I should put the define).

Regarding merging this functionality into lfs_fs_grow, I initially made it into its own function because I think shrinking is a more "dangerous" operation and should be explicit (as any usage would require careful review, more so than grow). I don't care much about being "POSIX-like", but if that's what you prefer no issue from me.

sosthene-nitrokey · 2025-05-05T09:49:44Z

For the name LFS_FS_SHRINKCHEAP what about LFS_FS_SHRINK_NONMOVING ?

bmcdonnell-fb · 2025-05-05T13:28:26Z

Regarding merging this functionality into lfs_fs_grow, I initially made it into its own function because I think shrinking is a more "dangerous" operation and should be explicit (as any usage would require careful review, more so than grow).

This makes sense to me.

@geky maybe consider prioritizing clarity over POSIX-likeness, in cases where POSIX-likeness is quirky, unexpected, or unclear? Just my 2¢; apologies if you've already decided otherwise for the project.

lfs.h

geky · 2025-05-05T19:43:18Z

I made a fixup commit that applies your suggestions.

Thanks! It's looking good to me codewise now.

I'm just not sure how to make it so that LFS_FS_SHRINKCHEAP is defined in the relevant tests (the tests pass if I add it to runners/test_runners.h so it's only about where I should put the define).

Ah, this is a bit messy at the moment because most ifdefs are large in scope.

You can probably copy the test-no-intrinsics job and add -DLFS_SHRINKIFCHEAP to the CFLAGS.

You could limit the test suites via:

CFLAGS="$CFLAGS -DLFS_NO_INTRINSICS" \
    TESTFLAGS="test_superblocks test_grow test_shrink" \
    make test

But this is definitely not going to be a bottleneck vs powerloss/emulated/valgrind testing, so we may as well just test everything.

There's also a minor CI rework, and an umbrella LFS_BIGGEST define, in the future, so I wouldn't worry too much about fragmentation at the moment.

geky · 2025-05-05T19:44:15Z

For the name LFS_FS_SHRINKCHEAP what about LFS_FS_SHRINK_NONMOVING ?

That is a better name, but what if I raise you LFS_SHRINKIFTRIVIAL?

geky · 2025-05-05T20:18:07Z

Regarding merging this functionality into lfs_fs_grow, I initially made it into its own function because I think shrinking is a more "dangerous" operation and should be explicit (as any usage would require careful review, more so than grow).

That's a fair argument. Though I'm not sure if "dangerous" is the right word. Shrinking would still use copy-on-write updates, so worst case lfs_fs_grow errors and you end up with an unmodified filesystem (ignoring current issues with error recovery).

However it is a much more expensive operation.

And there is also the potential benefit of compile-time gc with the separate lfs_fs_shrink, though not sure if this is useful with it already being an opt-in ifdef...

Hmm. May have to think on this. Sorry if this ends up going the other way.

The main reason for merging is to prefer fewer, more powerful (not more complicated!), functions to make it easier for users the keep the API in their head. The more fragmented your API becomes, the harder it is to know whether or not a relevant function exists as a user.

Though maybe the benefits of separating outweigh the benefits of minimizing the API surface here...

@geky maybe consider prioritizing clarity over POSIX-likeness, in cases where POSIX-likeness is quirky, unexpected, or unclear? Just my 2¢; apologies if you've already decided otherwise for the project.

At the risk of getting into controversial opinion territory, my opinion is consistency > clarity when it comes to API design. A clear name doesn't exactly help you if you can't find the function, and usually the name won't be able to capture the full idiosyncrasies of the function anyways.

geky-bot · 2025-05-06T09:24:51Z

Tests passed ✓, Code: 17116 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17116 B (+0.0%)	1448 B (+0.0%)	812 B (+0.0%)	Lines	2433/2594 lines (+0.0%)
Readonly	6230 B (+0.0%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1279/1610 branches (-0.0%)
Threadsafe	17964 B (-0.0%)	1448 B (+0.0%)	820 B (+0.0%)		Benchmarks
Multiversion	17188 B (+0.0%)	1448 B (+0.0%)	816 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18780 B (+0.0%)	1752 B (+0.0%)	816 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17896 B (+0.0%)	1440 B (+0.0%)	812 B (+0.0%)	Erased	1568888832 B (+0.0%)

sosthene-nitrokey · 2025-05-06T10:01:22Z

For the name LFS_FS_SHRINKCHEAP what about LFS_FS_SHRINK_NONMOVING ?

That is a better name, but what if I raise you LFS_SHRINKIFTRIVIAL?

I still prefer SHRINKNONMOVING. It's not that important to me.

geky · 2025-05-07T03:04:41Z

Having slept on it, I still think this should be one function (lfs_fs_grow):

It simplifies maintaining a variable sized fixed region of storage after the filesystem (for XIP, etc).
It better shares code internally.
It minimizes the API surface.
It matches lfs_fs_truncate, for better or worse.

It's subjective, but it feels like the best option to me. Thanks for the feedback both of you, and for adopting the unified function @sosthene-nitrokey.

I still prefer SHRINKNONMOVING. It's not that important to me.

We can go with LFS_SHRINKNONMOVING. Though thoughts on LFS_SHRINKNONRELOCATING or LFS_SHRINKNORELOCATE? "Relocate" is the term used elsewhere in the codebase specifically for block -> block migrations.

geky · 2025-05-07T03:14:38Z

lfs.c

+#endif
+#ifdef LFS_SHRINKIFCHEAP
+    lfs_block_t threshold = block_count;
+    err = lfs_fs_traverse_(lfs, lfs_shrink_checkblock, &threshold, true);


Ah, sorry I should have caught this earlier.

Even with LFS_SHRINKNONRELOCATING enabled, we shouldn't traverse if unless block_count < lfs->block_count. lfs_fs_traverse_ involves a lot of io, and users probably expect lfs_fs_grow to be cheap if it is a simple grow operation.

Also I think this can just be &block_count? It doesn't look like threshold is used after this.

geky · 2025-05-07T03:15:28Z

lfs.c

@@ -5233,40 +5233,63 @@ static int lfs_fs_gc_(lfs_t *lfs) {
 #endif

 #ifndef LFS_READONLY
+#ifdef LFS_SHRINKIFCHEAP
+static int lfs_shrink_checkblock(void * data, lfs_block_t block) {


nit: spacing: void *data

geky · 2025-05-07T03:15:54Z

lfs.c

@@ -5233,40 +5233,63 @@ static int lfs_fs_gc_(lfs_t *lfs) {
 #endif

 #ifndef LFS_READONLY
+#ifdef LFS_SHRINKIFCHEAP
+static int lfs_shrink_checkblock(void * data, lfs_block_t block) {
+    lfs_size_t threshold = *((lfs_size_t *) data);


nit: spacing: *((lfs_size_t*)data)

geky · 2025-05-07T03:18:34Z

tests/test_shrink.toml

+
+if = "AFTER_BLOCK_COUNT <= BLOCK_COUNT"
+code = '''
+#ifdef LFS_SHRINKIFCHEAP


Ah, yes. This is the best way to ifdef right now. I just noticed the earlier test failures.

I have a branch that adds ifdef attrs to the test case itself, but it hasn't been upstreamed yet unfortunately...

geky-bot · 2025-05-07T09:02:54Z

Tests passed ✓, Code: 17116 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17116 B (+0.0%)	1448 B (+0.0%)	812 B (+0.0%)	Lines	2433/2594 lines (+0.0%)
Readonly	6230 B (+0.0%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1279/1610 branches (-0.0%)
Threadsafe	17964 B (-0.0%)	1448 B (+0.0%)	820 B (+0.0%)		Benchmarks
Multiversion	17188 B (+0.0%)	1448 B (+0.0%)	816 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18780 B (+0.0%)	1752 B (+0.0%)	816 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17896 B (+0.0%)	1440 B (+0.0%)	812 B (+0.0%)	Erased	1568888832 B (+0.0%)

geky · 2025-05-08T03:24:15Z

Looks good to me, thanks for the tweaks 👍 Will bring this into the next minor release.

sosthene-nitrokey force-pushed the shrink-fs branch 3 times, most recently from 7b8aaa0 to a1533aa Compare April 17, 2025 08:04

Add support for shrinking a filesystem

2105e50

This PR adds a new `lfs_fs_shrink`, which functions similarly to `lfs_fs_grow`, but supports reducing the block count. This functions first checks that none of the removed block are in use. If it is the case, it will fail.

sosthene-nitrokey force-pushed the shrink-fs branch from a1533aa to 2105e50 Compare April 17, 2025 08:07

This was referenced Apr 25, 2025

Update littlefs including shrinking and block count check flag trussed-dev/littlefs2-sys#27

Merged

Block count disable and shrink trussed-dev/littlefs#1

Open

geky mentioned this pull request May 2, 2025

Ability to downsize a filesystem #1093

Open

geky reviewed May 2, 2025

View reviewed changes

lfs.c Show resolved Hide resolved

geky added enhancement needs work nothing broken but not ready yet needs minor version new functionality only allowed in minor versions labels May 2, 2025

fixup! Add support for shrinking a filesystem

9b8f802

sosthene-nitrokey force-pushed the shrink-fs branch 3 times, most recently from 9a728cd to 9b8f802 Compare May 5, 2025 16:00

geky reviewed May 5, 2025

View reviewed changes

lfs.h Outdated Show resolved Hide resolved

Mention that shrinking is unlikely to work

7782d3d

geky reviewed May 7, 2025

View reviewed changes

sosthene-nitrokey added 2 commits May 7, 2025 10:34

Rename SHRINKIFCHEAP to SHRINKNONRELOCATING

7d79423

Apply review comments

edaaaf8

geky added next minor and removed needs work nothing broken but not ready yet labels May 8, 2025

geky approved these changes May 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for shrinking a filesystem #1094

Add support for shrinking a filesystem #1094

sosthene-nitrokey commented Apr 16, 2025

geky-bot commented Apr 17, 2025

geky commented May 2, 2025

geky left a comment

geky May 2, 2025

geky May 2, 2025

geky May 2, 2025

sosthene-nitrokey May 5, 2025

geky May 2, 2025

sosthene-nitrokey May 5, 2025

geky May 2, 2025

geky May 2, 2025

bmcdonnell-fb commented May 2, 2025

geky commented May 2, 2025

sosthene-nitrokey commented May 5, 2025

sosthene-nitrokey commented May 5, 2025

bmcdonnell-fb commented May 5, 2025

geky commented May 5, 2025

geky commented May 5, 2025

geky commented May 5, 2025

geky-bot commented May 6, 2025

sosthene-nitrokey commented May 6, 2025

geky commented May 7, 2025 •

edited

Loading

geky May 7, 2025

geky May 7, 2025

geky May 7, 2025

geky May 7, 2025

geky-bot commented May 7, 2025

geky commented May 8, 2025 •

edited

Loading

Add support for shrinking a filesystem #1094

Are you sure you want to change the base?

Add support for shrinking a filesystem #1094

Conversation

sosthene-nitrokey commented Apr 16, 2025

geky-bot commented Apr 17, 2025

geky commented May 2, 2025

geky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmcdonnell-fb commented May 2, 2025

geky commented May 2, 2025

sosthene-nitrokey commented May 5, 2025

sosthene-nitrokey commented May 5, 2025

bmcdonnell-fb commented May 5, 2025

geky commented May 5, 2025

geky commented May 5, 2025

geky commented May 5, 2025

geky-bot commented May 6, 2025

sosthene-nitrokey commented May 6, 2025

geky commented May 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky-bot commented May 7, 2025

geky commented May 8, 2025 • edited Loading

geky commented May 7, 2025 •

edited

Loading

geky commented May 8, 2025 •

edited

Loading