littlefs: make block size configurable at compile time

[fabian18]

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

[leandrolanzieri]

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

[fabian18]

Yes, let´s do it like that.

[fabian18]

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.

[fabian18]

special value

let´s choose < 0.

[benpicco]

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);

[fabian18]

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.

[fabian18]

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]

Thank you very much ☺️

[MrKevinWeiss]

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]

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

[maribu]

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.

[benpicco]

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)

[maribu]

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.)