littlefs: make block size configurable at compile time by fabian18 · Pull Request #18141 · RIOT-OS/RIOT

fabian18 · 2022-05-29T12:10:07Z

Contribution description

The reason for me to create this PR was that I was experiencing problems when I tried to run littlefs and littlefs2 on my at24c256 EEPROM.
Currently, the block size is always equal to mtd->page_size * mtd->pages_per_sector, but in practice the block size must only be a multiple of the sector size.

In my case, the resulting block size was 64 bytes, equal to the sector size, equal to the page size.
I read some littlefs documentation and found out that the minimum block size must be 104 bytes.
With this patch, resulting in a block size of 128 bytes, problems seemed to be gone.

diff --git a/drivers/at24cxxx/mtd/mtd.c b/drivers/at24cxxx/mtd/mtd.c
index 73b530a98c..76588c8efd 100644
--- a/drivers/at24cxxx/mtd/mtd.c
+++ b/drivers/at24cxxx/mtd/mtd.c
@@ -38,9 +38,9 @@ static int _mtd_at24cxxx_init(mtd_dev_t *mtd)
         return init;
     }
     mtd->page_size = DEV(mtd)->params.page_size;
-    mtd->pages_per_sector = 1;
+    mtd->pages_per_sector = 2;
     mtd->sector_count = DEV(mtd)->params.eeprom_size /
-                        DEV(mtd)->params.page_size;
+                        (DEV(mtd)->params.page_size * mtd->pages_per_sector);
     mtd->write_size = 1;
     return 0;
 }

However, this is not a proper fix to set an imaginary sector size, that just happens to work for some filesystem.
So I concluded that the proper fix would be to have the opportunity to configure a reasonable logical block size for a filesystem and keep the device properties untouched, like it is the case for most common filesystems that run on Linux.

For fatfs, I did not find a way to configure the block size at first glace. Maybe they don´t support it.

Testing procedure

A ~~freshly formatted~~ filesystem must still work. ~~Existing filesystems may need to be formatted.~~
If you are using an at24c256on stm32 like me, you must configure the block size to be 128 bytes, else

assert(dev < I2C_NUMOF && length < MAX_BYTES_PER_FRAME); /* MAX_BYTES_PER_FRAME = 256 */

will fire.

Issues/PRs references

pkg/spiffs/fs/spiffs_fs.c

leandrolanzieri · 2022-06-10T18:47:12Z

pkg/littlefs2/Kconfig

+    int "Minimum acceptable block size"
+    default 512
+    help
+        Sets the minimum acceptable block size which must be a power of two.


Isn't it safer to express this as the exponent of 2^n? Then we don't have to do any checks

Yes, let´s do it like that.

And let´s have a special value that sets the block size to the sector size and chose this value as default. So nobody will be forced to reformat an existing FS. However the tests must also pass with different block sizes of course.

special value

let´s choose < 0.

sys/include/fs/littlefs2_fs.h

pkg/littlefs2/fs/littlefs2_fs.c

benpicco · 2022-06-19T16:18:26Z

pkg/littlefs2/fs/littlefs2_fs.c

+    if (!(block_size = ((1u << CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP) / block_size) * block_size)) {
+        block_size = fs->dev->pages_per_sector * fs->dev->page_size;
    }


This needs some explaining.
I think we need the least common multiple here, so

block_size = (1 << CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP) * block_size / gcd(block_size, 1 << CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP);

size_t block_size = fs->dev->pages_per_sector * fs->dev->page_size; block_size = (1 << CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP) * block_size / gcd(block_size, 1 << CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP);

Example (desired block size is 512):

block_size = 3 * 128; block_size = (1 << 9) * 384 / gcd(384, 1 << 9); = 512 * 384 / 128 = 1536

This ensures that the actual block size is not smaller then 1 << CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP, but this is not really the the result I had expected.
I had something different in mind. Right now, the resulting block size is the next smaller multiple of the sector size (<=). But the documentation reads that CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP is the "minimum acceptable" block size, so it should be the next bigger multiple of the sector size. In the example above, it would be 768.

pkg/littlefs2/fs/littlefs2_fs.c

fabian18 · 2022-06-20T12:29:32Z

sys/include/fs/littlefs2_fs.h

     * if set to 0, the total number of sectors from the mtd is used */
    uint32_t base_addr;
-    uint8_t sectors_per_block;  /**< number of sectors per block */
+    uint16_t sectors_per_block; /**< number of sectors per block */


I made this a uint16_t because I chose the maximum of CONFIG_LITTLEFS2_MIN_BLOCK_SIZE_EXP to be 2^15 in Kconfig.

benpicco

Changes look good and existing file systems still mount fine with this change.

fabian18 · 2022-08-01T08:14:25Z

Thank you very much ☺️

MrKevinWeiss · 2022-08-18T10:05:31Z

It looks like this PR broke samr21-xpro tests for tests/pkg_littlefs and tests/pkg_littlefs2.

in the future the CI: run tests would have tested it. It has also shown up on the nightlies for a while.

MrKevinWeiss · 2022-08-18T12:59:26Z

Just to clarify it also fixes things, it just exposes other things.

maribu · 2022-08-18T16:33:37Z

sys/include/fs/littlefs_fs.h

     * total number of block is defined in @p config.
     * if set to 0, the total number of sectors from the mtd is used */
    uint32_t base_addr;
+    uint16_t sectors_per_block; /**< number of sectors per block */


This addition resulted in the subsequent buffers no longer being aligned two 4 bytes by accident, which triggered unaligned memory accesses. On Cortex M0+, such as used by the samr21-xpro, this then resulted in hard faults.

But the issue really is that alignment requirements need to be communicated to the compiler. It was just luck that this worked before.

Well not so much luck since the uint32_t will always be word-aligned.

Moving sectors_per_block to end end of the struct should be the simple fix. (The buffers are already word-multiples)

I doubt anyone placed them after a uint32_t member to align them without adding a comment to warn about breakage. To me, this looks like we got lucky here :)

(E.g. iolist_t and pkt_snip_t have a common memory layout, but that is documented to avoid anyone from breaking it. I would expect similar comments here if that was inentionally.)

fabian18 requested review from MrKevinWeiss, jia200x and leandrolanzieri as code owners May 29, 2022 12:10

github-actions bot added Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: sys Area: System labels May 29, 2022

fabian18 added Area: fs Area: File systems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 29, 2022

fabian18 commented Jun 1, 2022

View reviewed changes

pkg/spiffs/fs/spiffs_fs.c Outdated Show resolved Hide resolved

fabian18 requested a review from maribu June 1, 2022 11:21

fabian18 force-pushed the filesystems_configure_block_size branch from c858fce to d036448 Compare June 3, 2022 09:00

fabian18 changed the title ~~filesystems: make block size configurable at compile time~~ littlefs: make block size configurable at compile time Jun 3, 2022

fabian18 force-pushed the filesystems_configure_block_size branch 2 times, most recently from cc16c16 to ebc4b42 Compare June 7, 2022 14:19

fabian18 requested review from MichelRottleuthner, aabadie, fjmolinas and smlng as code owners June 7, 2022 14:19

github-actions bot added the Area: tests Area: tests and testing framework label Jun 7, 2022

fabian18 force-pushed the filesystems_configure_block_size branch from ebc4b42 to af45ba0 Compare June 10, 2022 10:32

leandrolanzieri reviewed Jun 10, 2022

View reviewed changes

fabian18 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 11, 2022

benpicco reviewed Jun 12, 2022

View reviewed changes

sys/include/fs/littlefs2_fs.h Show resolved Hide resolved

pkg/littlefs2/fs/littlefs2_fs.c Outdated Show resolved Hide resolved

maribu requested a review from benpicco June 15, 2022 10:50

fabian18 force-pushed the filesystems_configure_block_size branch from 1b2990b to ef8da60 Compare June 18, 2022 08:29

fabian18 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 18, 2022

benpicco reviewed Jun 19, 2022

View reviewed changes

fabian18 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 20, 2022

github-actions bot added the Area: drivers Area: Device drivers label Jun 20, 2022

fabian18 commented Jun 20, 2022

View reviewed changes

fabian18 force-pushed the filesystems_configure_block_size branch from 59a94eb to 81e6478 Compare June 20, 2022 13:21

github-actions bot removed the Area: drivers Area: Device drivers label Jun 20, 2022

fabian18 mentioned this pull request Jul 26, 2022

gcoap/fileserver: add file and directory creation and deletion #18133

Merged

fabian18 added 4 commits July 31, 2022 10:57

pkg/littlefs: make block size configurable

41c0b1e

pkg/littlefs2: make block size configurable

a29d5c2

pkg/littlefs2: add warning if block size is not reasonable

4dd5cb3

pkg/littlefs: add warning if block size is not reasonable

578e628

fabian18 force-pushed the filesystems_configure_block_size branch from 81e6478 to 578e628 Compare July 31, 2022 09:02

fabian18 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 31, 2022

benpicco approved these changes Jul 31, 2022

View reviewed changes

benpicco merged commit f375856 into RIOT-OS:master Jul 31, 2022

maribu reviewed Aug 18, 2022

View reviewed changes

maribu mentioned this pull request Aug 18, 2022

pkg/{littlefs,littlefs2}: fix unaligned memory accesses #18473

Merged

2 tasks

maribu added this to the Release 2022.10 milestone Oct 14, 2022

benpicco mentioned this pull request Feb 10, 2023

drivers/mtd_flashpage: implement pagewise API, don't use raw addresses #19258

Merged

fabian18 deleted the filesystems_configure_block_size branch January 11, 2025 15:30

Conversation

fabian18 commented May 29, 2022 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

fabian18 commented Aug 1, 2022

Uh oh!

MrKevinWeiss commented Aug 18, 2022

Uh oh!

MrKevinWeiss commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribu Aug 18, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fabian18 commented May 29, 2022 •

edited

Loading

maribu Aug 18, 2022 •

edited

Loading