Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add explicit assert for minimum block size of 128 bytes #802

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

geky
Copy link
Member

@geky geky commented Apr 18, 2023

See #264 for more info.

There was already an assert for this, but because it included the underlying equation for the requirement it was too confusing for users that had no prior knowledge for why the assert could trigger.

The math works out such that 128 bytes is a reasonable minimum requirement, so I've added that number as an explicit assert. Hopefully this makes this sort of situation easier to debug.

Note that this requirement would need to be increased to 512 bytes if block addresses are ever increased to 64-bits. DESIGN.md#ctz-skip-lists goes into more detail why this is.

There was already an assert for this, but because it included the
underlying equation for the requirement it was too confusing for
users that had no prior knowledge for why the assert could trigger.

The math works out such that 128 bytes is a reasonable minimum
requirement, so I've added that number as an explicit assert.
Hopefully this makes this sort of situation easier to debug.

Note that this requirement would need to be increased to 512 bytes if
block addresses are ever increased to 64-bits. DESIGN.md goes into more
detail why this is.
@geky geky added enhancement next minor needs ci ci is probably broken labels Apr 18, 2023
@geky geky added this to the v2.6 milestone Apr 18, 2023
@geky geky mentioned this pull request Apr 18, 2023
@geky geky removed the needs ci ci is probably broken label Apr 18, 2023
@e107steved
Copy link

Seems a good idea to make the reason for the assert obvious (although would an explanatory comment suffice?).

However, a concern for the future. If you should ever make block addresses 64 bit, I hope this would be an option. The jump from 128 byte blocks to 512 byte blocks could be quite painful for those using small memories (I have just 128K bytes of FRAM, and I think other users have less!). The low overhead of littleFS is currently a key advantage, especially over FAT.
There would also be the issue of upgrading existing file systems.

@geky
Copy link
Member Author

geky commented Apr 18, 2023

Seems a good idea to make the reason for the assert obvious (although would an explanatory comment suffice?).

Well this also makes the rule-of-thumb 128 byte limit a hard requirement, which may be valuable in future changes (though unlikely).

It also asserts the math is correct, which is an intimidating piece of code someone might think is broken if they hit this assert. It might reduce the mental load when debugging if you're also calculating your block size a complicated way.

Though that may be a bit of a stretch, it could probably go either way. At least if code size is an issue these asserts can always be turned off.

However, a concern for the future. If you should ever make block addresses 64 bit, I hope this would be an option. The jump from 128 byte blocks to 512 byte blocks could be quite painful for those using small memories (I have just 128K bytes of FRAM, and I think other users have less!). The low overhead of littleFS is currently a key advantage, especially over FAT.
There would also be the issue of upgrading existing file systems.

It's just a hypothetical. As is, a 64-bit version. of littlefs would be fundamentally incompatible with the current 32-bit version. And the devices that would need 64-bits addresses would probably be very unlikely to have issues with 512 byte blocks.

Another hypothetical would be a 16-bit version of littlefs, which could in theory push the CTZ skip-list requirement down to 32-bytes (24 exactly). But I think at that point you'd really start having issues fitting file metadata into the block.

@geky geky changed the base branch from master to devel April 26, 2023 06:05
@geky geky merged commit 92298c7 into devel Apr 26, 2023
79 checks passed
@geky geky mentioned this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants