While I think shrub_size is probably the more correct name at a
technical level, inline_size is probably more what users expect and
doesn't require a deeper understanding of filesystem details.
The only risk is that users may think inline_size has no effect on large
files, when in fact it still controls how much of the btree root can be
inlined.
There's also the point that sticking with inline_size maintains
compatibility with both the upstream version and any future version that
has other file representations.
May revisit this, but renaming to lfs->cfg->inline_size for now.
Now that we no longer have bmoss files, inline_size and shrub_size are
effectively the same thing.
We weren't using this, so no code change, but it does save a word of
ctx:
code stack ctx
before: 36280 2576 640
after: 36280 (+0.0%) 2576 (+0.0%) 636 (-0.6%)
Bptrs really are a file concept, despite the name (bptr =>
block-pointer). Other bshrubs/btrees do not have bptrs.
Returning decoded bptrs from lfsr_bshrub_lookupnext and friends was a
bit of a hack to make bsprouts (single bptrs) work, but now that we
don't support bsprouts, we don't need this hack anymore.
To avoid code duplication, this does reroute mtree traversal through
lfsr_file_traverse_. Which is a bit weird, but not the worst thing this
codebase has ever done.
Code changes:
code stack ctx
before: 36492 2608 640
after: 36460 (-0.1%) 2608 (+0.0%) 640 (+0.0%)
This makes a bit less sense than adopting lfsr_bshrub_t in lfsr_bshrub_*
functions, but it gives LFSR_TAG_SHRUBCOMMIT/SHRUBTRUNK direct access to
the staging shrub without needing the shrub + 1 hack.
The whole shrub vs bshrub distinction is already a bit broken anyways,
with us relying on the opened-mdir list to correctly stage all shrubs in
the filesystem.
---
Curiously, this again ends up with net negative impact on code cost:
code stack ctx
before: 36484 2608 640
after: 36492 (+0.0%) 2608 (+0.0%) 640 (+0.0%)
This moves all of the shrub tracking logic from lfsr_obshrub_t into
lfsr_bshrub_t, completely drops the lfsr_obshrub_t type, and changes all
lfsr_bshrub_* functions to take lfsr_bshrub_t instead of the mdir+shrub
pair.
This makes the lfsr_bshrub_* functions <-> lfsr_bshrub_t relationship
more consistent with other APIs, such as lfsr_btree_t:
- lfsr_bshrub_lookupnext(lfs, &file->o.o.mdir, &file->o.bshrub, ...)
+ lfsr_bshrub_lookupnext(lfs, &file->b, ...)
I think the reason why this design wasn't obvious before is because, at
least conceptually, having the lfsr_mdir_t live inside the lfsr_bshrub_t
is a bit weird. It's only thanks to lfsr_file_t invasively using the
internal lfsr_mdir_t that we can avoid duplicate lfsr_mdir_t objects.
This also reorganizes the structs in lfs.h a bit, and renames the
related file.o -> file.b fields (much needed because lfs->gc.t.o.o.mdir.
rbyd.blocks was starting to get _real_ confusing).
---
Unfortunately, reducing the number of arguments to lfsr_bshrub_*
functions did not save nearly as much code as I thought it would. It
even ended up with a net _increase_ of code, apparently due to needing
to recalculate the bshrub->shrub offset more often:
code stack ctx
before: 36476 2608 640
after: 36484 (+0.0%) 2608 (+0.0%) 640 (+0.0%)
Strange, but this rework is still worthwhile if only for the code
readability.
The lfsr_shrub_t/lfsr_btree_t union was _technically_ not undefined
behavior, because the relevant fields were all a part of the "common
initial sequence", but collapsing these to the same type certainly does
simplify things.
The only weirdness is that we now store shrub.estimate in shrub.eoff.
We could add a union here, but the extra noise is just not worth the
slighty better name. The shrub.estimate is a sort of "simulated
shrub.eoff" anyways.
---
This makes it so all of these types alias to the same core lfsr_rbyd_t
type, which I suppose actually reflects the on-disk format quite well:
lfsr_shrub_t => lfsr_rbyd_t
lfsr_bshrub_t
lfsr_btree_t
Code cost more-or-less unaffected:
code stack ctx
before: 36432 2608 640
after: 36436 (+0.0%) 2608 (+0.0%) 640 (+0.0%)
This is a pretty suspicious looking test failure, considering the recent
changes to the mroot/mtree and related splitting logic, but it just
turned out to be a bug in the test logic.
Sort of. This loop is trying to create an mroot that will both compact
and split, but it doesn't check if the mdir was split prematurely, so it
just keeps adding files until we hit a true LFS_ERR_NOSPC condition:
if ((file1.o.o.mdir.rbyd.eoff & 0x7fffffff) > GC_COMPACT_THRESH
&& estimate > BLOCK_SIZE/2) {
break;
}
The solution is to make the filename size a bit smaller so we don't
split too early.
I also added some asserts to catch premature splits in case this happens
again. These tests are a bit delicate.
We had to be a bit clever with our lfsr_mtree_t representation to
support msprouts. Now that we don't support msprouts, we can simplify
this and drop the lfsr_mtree_t type completely! which is nice for both
code cost and readability.
Saves a bit more code:
code stack ctx
before: 38344 2624 640
after: 38284 (-0.2%) 2624 (+0.0%) 640 (+0.0%)
Which increases the total savings of dropping msprouts:
code stack ctx
yes msprouts: 38508 2624 640
no msprouts: 38284 (-0.6%) 2624 (+0.0%) 640 (+0.0%)
While we do check for out-of-bound tags in lfsr_bd_readtag, we were
ignoring the returned size in lfsr_rbyd_fetch when reading cksum tags.
This meant it was possible for lfsr_rbyd_fetch to try to read past the
end-of-block if:
1. The cksum tag was malformed with size < 4.
2. The malformed cksum tag was < 4 bytes from the end-of-block.
A pretty rare case! Considering we don't even bother writing cksum tags
when we're that close to the end-of-block. This can only happen in our
tests if existing garbage happens to look like a cksum tag.
While every cksum tag _should_ have at least 4 bytes for the cksum, we
can't guarantee that if we're parsing garbage.
Found by our test_ck_spam_dir_fuzz test.
---
I've also added a couple test_mtree_truncated_* tests to catch similar
truncation issues and prevent a regression in the future. We can't
really rely on test_ck_spam_* to always find nuanced errors like this,
but it's neat it found this one.
Code changes:
code stack ctx
before: 38340 2624 640
after: 38344 (+0.0%) 2624 (+0.0%) 640 (+0.0%)
littlefs is intentionally designed to not rely on noise, even with cksum
collisions (hello, perturb bit!). So it makes sense for this to be an
optional feature, even if it's a small one.
Disabling revision count noise by default also helps with testing. The
whole point of revision count noise is to make cksum collisions less
likely, which is a bit counterproductive when that's something we want
to test!
This doesn't really change the revision count encoding:
vvvvrrrr rrrrrrnn nnnnnnnn nnnnnnnn
'-.''----.----''---------.--------'
'------|---------------|---------- 4-bit relocation revision
'---------------|---------- recycle-bits recycle counter
'---------- pseudorandom noise (optional)
I considered moving the recycle-bits down when we're not adding noise,
but the extra logic just isn't worth making the revision count a bit
more human-readable.
---
This saves a small bit of code in the default build, at the cost of some
code for the runtime checks in the LFS_NOISY build. Though I'm hoping
future config work will let users opt-out of these runtime checks:
code stack ctx
before: 38548 2624 640
default after: 38508 (-0.1%) 2624 (+0.0%) 640 (+0.0%)
LFS_NOISY after: 38568 (+0.1%) 2624 (+0.0%) 640 (+0.0%)
Honestly the thing I'm more worried about is using one of our precious
mount flags for this... There's not that many bits left!
This just exposes the gcksum to the user, but exposing the gcksum allows
the user to store it externally for an extra layer of protection against
filesystem corruption.
As far as I'm aware this is the only real way to protect against global
rollback issues, which is a problem for any filesystem with logs (aka
any powerloss-resilient filesystem).
This required a comically small amount of code:
code stack ctx
before: 38492 2624 640
after: 38500 (+0.0%) 2624 (+0.0%) 640 (+0.0%)
We really shouldn't have two names for the same thing, it just makes
things more confusing, even if the public name doesn't quite match the
internal usage. Especially now that we internally rely on these being
the same flag.
This renames LFS_i_UNTIDY -> LFS_I_MKCONSISTENT and drops the untidy/
mktidy naming internally.
No code changes.
- In lfsr_mtree_traverse, we traverse open file bshrubs/btrees before
we validate the gcksum, which means bugs/asserts can slip through
before we have a chance to detect something is wrong.
To work around this, I've added an explicit mdir cksum check right
before we start traversing an open mdir's bshrubs/btrees. If an open
mdir doesn't match the on-disk state, the on-disk state must contain
an error (or the RAM, but that's a different story and wayyy out of
scope).
It might be better to rearrange lfsr_mtree_traverse to check gcksums
first, but this will require another look at our traversal clobbering
logic.
- For a similar reason, ckfetches can't detect open bshrub/btree
corruption as is. As its name suggests, ckfetches only checks fetches,
so any corruption after we've fetched bshrubs/btrees in lfsr_file_open
will go undetected.
Fortunately this just means we need a full ckmeta-scan in
test_ck_spam* tests that keep open files.
In real use, full ckmeta-scans should be preferred anyways. Limiting
these scans to mtreeonly was just an attempt to better stress btree
ckfetches.
At least we're still testing ckmeta+mtreeonly+ckfetches in
test_ck_spam_dir_fuzz and test_ck_spam_file_fuzz.
This gets the test_ck_spam* tests running under all of the current
interesting ck-modes.
Code changes:
code stack ctx
before: 38560 2640 644
after 38572 (+0.0%) 2640 (+0.0%) 644 (+0.0%)
Now that gcksums are working and we can detect rollback issues, it's
worth revisiting our most aggressive bit-error tests.
Unfortunately, I think due to focusing on ckprogs, these were a bit less
ready-to-go than I had hoped. We still have the read-hole, so the sort
of errors we can expect to detect is a bit limited.
Still, managed to come up with some schemes that I think are
interesting:
- ckprogs - Limited to catching bit-errors during progs, but these tests
work great.
- ckdata - Limited to manual bit-errors, but can detect both metdata +
data errors.
- ckmeta+ckfetches - Limited to manual bit-errors, ckmeta detects
mtree errors, while ckfetches detects btree + data errors.
- ckmeta+ckdatacksums - Limited to manual bit-errors, ckmeta detects
metadata errors, while ckdatacksums detects data errors.
To make testing manual bit-errors a bit easier, and to avoid
reimplementing the bit randomizer in emubd, I added
LFS_EMUBD_BADBLOCK_MANUAL and lfs_emubd_flip to let the tests manually
control when bits flip.
---
Unfortunately open files are proving to be an issue for these tests,
since we don't really expect corrupted metadata after lfsr_file_open (
assuming no read-hole).
For now I've limited these new ck-modes to the tests without open files,
but we should probably revisit this.
This adds a check that the on-disk gcksum matches the in-RAM gcksum
in lfsr_mtree_traverse, so ckmeta/ckdata scans should now be able to
at least detect global-rollback issues that occur while mounted.
This also moves the LFS_I_CKMETA/CKDATA flag clearing logic from
lfsr_mtree_gc -> lfsr_mtree_traverse. There's no reason to not clear
these flags if we've made a successful traversal. We weren't actually
calling lfsr_mtree_traverse with the right flags for this to matter, but
it does let us drop an explicit flag clear in lfsr_fs_ck.
---
These changes were a part of adding the harder versions of our ckmeta/
ckdata tests, where we flip individual bits instead of clobbering the
entire block. These are more realistic errors and stress our gcksum
system.
Recalculating the gcksum required another gcksum copy in
lfsr_traversal_t, which adds a bit of code and ctx to our incremental-gc
build:
code stack ctx
default before: 38428 2640 644
default after: 38560 (+0.3%) 2640 (+0.0%) 644 (+0.0%)
gc before: 38484 2640 788
gc after: 38616 (+0.3%) 2640 (+0.0%) 792 (+0.5%)
Unfortunately we can't easily abuse the copies in lfs_t since
multiple traversals may be open at once.
This was quite a puzzle.
The problem: How do we detect corrupt mdirs?
Seems like a simple question, but we can't just rely on mdir cksums. Our
mdirs are independently updateable logs, and logs have this annoying
tendency to "rollback" to previously valid states when corrupted.
Rollback issues aren't littlefs-specific, but what _is_ littlefs-
specific is that when one mdir rolls back, it can disagree with other
mdirs, resulting in wildly incorrect filesystem state.
To solve this, or at least protect against disagreeable mdirs, we need
to somehow include the state of all other mdirs in each mdir commit.
---
The first thought: Why not use gstate?
We already have a system for storing distributed state. If we add the
xor of all of our mdir cksums, we can rebuild it during mount and verify
that nothing changed:
.--------. .--------. .--------. .--------.
.| mdir 0 | .| mdir 1 | .| mdir 2 | .| mdir 3 |
|| | || | || | || |
|| gdelta | || gdelta | || gdelta | || gdelta |
|'-----|--' |'-----|--' |'-----|--' |'-----|--'
'------|-' '------|-' '------|-' '------|-'
'--.------' '--.------' '--.------' '--.------'
cksum | cksum | cksum | cksum |
| | v | v | v |
'---------> xor -------> xor -------> xor -------> gcksum
| v v v =?
'---------> xor -------> xor -------> xor ---> gcksum
Unfortunately it's not that easy. Consider what this looks like
mathematically (g is our gcksum, c_i is an mdir cksum, d_i is a
gcksumdelta, and +/-/sum is xor):
g = sum(c_i) = sum(d_i)
If we solve for a new gcksumdelta, d_i:
d_i = g' - g
d_i = g + c_i - g
d_i = c_i
The gcksum cancels itself out! We're left with an equation that depends
only on the current mdir, which doesn't help us at all.
Next thought: What if we permute the gcksum with a function t before
distributing it over our gcksumdeltas?
.--------. .--------. .--------. .--------.
.| mdir 0 | .| mdir 1 | .| mdir 2 | .| mdir 3 |
|| | || | || | || |
|| gdelta | || gdelta | || gdelta | || gdelta |
|'-----|--' |'-----|--' |'-----|--' |'-----|--'
'------|-' '------|-' '------|-' '------|-'
'--.------' '--.------' '--.------' '--.------'
cksum | cksum | cksum | cksum |
| | v | v | v |
'---------> xor -------> xor -------> xor -------> gcksum
| | | | .--t--'
| | | | '-> t(gcksum)
| v v v =?
'---------> xor -------> xor -------> xor ---> t(gcksum)
In math terms:
t(g) = t(sum(c_i)) = sum(d_i)
In order for this to work, t needs to be non-linear. If t is linear, the
same thing happens:
d_i = t(g') - t(g)
d_i = t(g + c_i) - t(g)
d_i = t(g) + t(c_i) - t(g)
d_i = t(c_i)
This was quite funny/frustrating (funnistrating?) during development,
because it means a lot of seemingly obvious functions don't work!
- t(g) = g - Doesn't work
- t(g) = crc32c(g) - Doesn't work because crc32cs are linear
- t(g) = g^2 in GF(2^n) - g^2 is linear in GF(2^n)!?
Fortunately, powers coprime with 2 finally give us a non-linear function
in GF(2^n), so t(g) = g^3 works:
d_i = g'^3 - g^3
d_i = (g + c_i)^3 - g^3
d_i = (g^2 + gc_i + gc_i + c_i^2)(g + c_i) - g^3
d_i = (g^2 + c_i^2)(g + c_i) - g^3
d_i = g^3 + gc_i^2 + g^2c_i + c_i^3 - g^3
d_i = gc_i^2 + g^2c_i + c_i^3
---
Bleh, now we need to implement finite-field operations? Well, not
entirely!
Note that our algorithm never uses division. This means we don't need a
full finite-field (+, -, *, /), but can get away with a finite-ring (+,
-, *). And conveniently for us, our crc32c polynomial defines a ring
epimorphic to a 31-bit finite-field.
All we need to do is define crc32c multiplication as polynomial
multiplication mod our crc32c polynomial:
crc32cmul(a, b) = pmod(pmul(a, b), P)
And since crc32c is more-or-less just pmod(x, P), this lets us take
advantage of any crc32c hardware/tables that may be available.
---
Bunch of notes:
- Our 2^n-bit crc-ring maps to a 2^n-1-bit finite-field because our crc
polynomial is defined as P(x) = Q(x)(x + 1), where Q(x) is a 2^n-1-bit
irreducible polynomial.
This is a common crc construction as it provides optimal odd-bit/2-bit
error detection, so it shouldn't be too difficult to adapt to other
crc sizes.
- t(g) = g^3 is not the only function that works, but it turns out to be
a pretty good one:
- 3 and 2^(2^n-1)-1 are coprime, which means our function t(g) = g^3
provides a one-to-one mapping in the underlying fields of all crc
rings of size 2^(2^n).
We know 3 and 2^(2^n-1)-1 are coprime because 2^(2^n-1)-1 =
2^(2^n)-1 (a Fermat number) - 2^(2^n-1) (a power-of-2), and 3
divides Fermat numbers >=3 (A023394) and is not 2.
- Our delta, when viewed as a polynomial in g: d(g) = gc^2 + g^2c +
c^3, has degree 2, which implies there are at most 2 solutions or
1-bit of information loss in the underlying field.
This is optimal since the original definition already had 2
solutions before we even chose a function:
d(g) = t(g + c) - t(g)
d(g) = t(g + c) - t((g + c) - c)
d(g) = t((g + c) + c) - t(g + c)
d(g) = d(g + c)
Though note the mapping of our crc-ring to the underlying field
already represents 1-bit of information loss.
- If you're using a cryptographic hash or other non-crc, you should
probably just use an equal sized finite-field.
Though note changing from a 2^n-1-bit field to a 2^n-bit field does
change the math a bit, with t(g) = g^7 being a better non-linear
function:
- 7 is the smallest odd-number coprime with 2^n-1, a Fermat number,
which makes t(g) = g^7 a one-to-one mapping.
3 humorously divides all 2^n-1 Fermat numbers.
- Expanding delta with t(g) = g^7 gives us a 6 degree polynomial,
which implies at most 6 solutions or ~3-bits of information loss.
This isn't actually the best you can do, some exhaustive searching
over small fields (<=2^16) suggests t(g) = g^(2^(n-1)-1) _might_ be
optimal, but that's a heck of a lot more multiplications.
- Because our crc32cs preserve parity/are epimorphic to parity bits,
addition (xor) and multiplication (crc32cmul) also preserve parity,
which can be used to show our entire gcksum system preserves parity.
This is quite neat, and means we are guaranteed to detect any odd
number of bit-errors across the entire filesystem.
- Another idea was to use two different addition operations: xor and
overflowing addition (or mod a prime).
This probably would have worked, but lacks the rigor of the above
solution.
- You might think an RS-like construction would help here, where g =
sum(c_ia^i), but this suffers from the same problem:
d_i = g' - g
d_i = g + c_ia^i - g
d_i = c_ia^i
Nothing here depends on anything outside of the current mdir.
- Another question is should we be using an RS-like construction anyways
to include location information in our gcksum?
Maybe in another system, but I don't think it's necessary in littlefs.
While our mdir are independently updateable, they aren't _entirely_
independent. The location of each mdir is stored in either the mtree
or a parent mdir, so it always gets mixed into the gcksum somewhere.
The only exception being the mrootanchor which is always at the fixed
blocks 0x{0,1}.
- This does _not_ catch "global-rollback" issues, where the most recent
commit in the entire filesystem is corrupted, revealing an older, but
still valid, filesystem state.
But as far as I am aware this is just a fundamental limitation of
powerloss-resilient filesystems, short of doing destructive
operations.
At the very least, exposing the gcksum would allow the user to store
it externally and prevent this issue.
---
Implementation details:
- Our gcksumdelta depends on the rbyd's cksum, so there's a catch-22 if
we include it in the rbyd itself.
We can avoid this by including it in the commit tags (actually the
separate canonical cksum makes this easier than it would have been
earlier), but this does mean LFSR_TAG_GCKSUMDELTA is not an
LFSR_TAG_GDELTA subtype. Unfortunate but not a dealbreaker.
- Reading/writing the gcksumdelta gets a bit annoying with it not being
in the rbyd. For now I've extended the low-level lfsr_rbyd_fetch_/
lfsr_rbyd_appendcksum_ to accept an optional gcksumdelta pointer,
which is a bit awkward, but I don't know of a better solution.
- Unlike the grm, _every_ mdir commit involves the gcksum, which means
we either need to propagate the gcksumdelta up the mroot chain
correctly, or somehow keep track of partially flushed gcksumdeltas.
To make this work I modified the low-level lfsr_mdir_commit__
functions to accept start_rid=-2 to indicate when gcksumdeltas should
be flushed.
It's a bit of a hack, but I think it might make sense to extend this
to all gdeltas eventually.
The gcksum cost both code and RAM, but I think it's well worth it for
removing an entire category of filesystem corruption:
code stack ctx
before: 37796 2608 620
after: 38428 (+1.7%) 2640 (+1.2%) 644 (+3.9%)
Most of littlefs's metadata is encoded in leb128s now, with the
exception of tags (be16, sort of), revision counts (le32), cksums
(le32), and flags.
It makes sense for tags to be a special case, these are written and
rewritten _everywhere_, but less so for flags, which are only written to
the mroot and updated infrequently.
We might as well save a bit of code by reusing our le32 machinery.
---
This changes lfsr_format to just write out compat flags as le32s, saving
a tiny bit of code at the cost of a tiny bit of disk usage (the real
benefit being a tiny bit of code simplification):
code stack ctx
before: 37792 2608 620
after: 37772 (-0.1%) 2608 (+0.0%) 620 (+0.0%)
Compat already need to handle trailing zeros gracefully, so this doesn't
change anything at mount time.
Also had to switch from enums to #defines thanks to C's broken enums.
Wooh. We already use #defines for the other flags for this reason.
LFS_WCOMPAT_RDONLY seems generally useful for tools that just want to
mark a filesystem is read-only. This is a common flag that exists in
other filesystems (RO_COMPAT_READONLY in ext4 for example).
LFS_RCOMPAT_WRONLY, on the other hand, is a bit more of a joke, but
there could be some niche use cases for it (preventing double mounts?).
Fortunately, these flags require no extra code, and fall out naturally
from our wcompat/rcompat handling.
---
Originally, the idea was to also add LFS_F_RDONLY, to match LFS_M_RDONLY
and set the LFS_WCOMPAT_RDONLY flag during format.
But this doesn't really work with the current API, since lfsr_format
would just give you an empty filesystem you can't write to. Which is a
bit silly.
Maybe we should add something like lfsr_fs_mkrdonly in the future? This
is probably low-priority.
dbgerr.py and dbgtag.py have proven to be incredibly useful for quick
debugging/introspection, so I figured why not have more of that.
My favorite part is being able to quickly see all flags set on an open
file handle:
(gdb) p file.o.o.flags
$2 = 24117517
(gdb) !./scripts/dbgflags.py o 24117517
LFS_O_WRONLY 0x00000001 Open a file as write only
LFS_O_CREAT 0x00000004 Create a file if it does not exist
LFS_O_EXCL 0x00000008 Fail if a file already exists
LFS_O_DESYNC 0x00000100 Do not sync or recieve file updates
LFS_o_REG 0x01000000 Type = regular-file
LFS_o_UNFLUSH 0x00100000 File's data does not match disk
LFS_o_UNSYNC 0x00200000 File's metadata does not match disk
LFS_o_UNCREAT 0x00400000 File does not exist yet
The only concern is if dbgflags.py falls out-of-sync often, I suspect
flag encoding will have quite a bit more churn than flags/tags. But we
can always drop this script in the future if this turns into a problem.
---
While poking around this also ended up with a bunch of other small
changes:
- Added LFS_*_MODE masks for consistency with other "type<->flag
embeddings"
- Added compat flag comments
- Adopted lowercase prefix for internal flags (LFS_o_ZOMBIE), though
not sure if I'll keep this yet...
- Tweaked dbgerr.py to also match ERR_ prefixes and to ignore case
Since we dropped lfsr_gc_setflags/setsteps, it was no longer possible to
set gc_flags to zero (perfectly valid and useful for system bringup/
testing things). Supporting gc_flags=0 means it's not possible to
provide a default, but this is probably ok as users need to opt-in to
LFS_GC anyways.
Note that at least gc_steps=0 doesn't make sense, so the default there
is reasonable.
Fixing this also highlighted that gc_flags/steps are no longer mutable,
making the comment in lfs_init out-of-date. Dropping these saves a bit
of lfs_t size, so that's nice.
And then testing also revealed that LFS_GC_CKDATA implying LFS_GC_CKDATA
means it should probably clear the LFS_I_CKMETA flag as well.
---
And here I thought this was going to be just a simple test-writing
exercise!
Code changes:
code stack ctx
default before: 37792 2608 620
default after: 37792 (-0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37896 2608 768
gc after: 37848 (-0.1%) 2608 (+0.0%) 760 (-1.0%)
The argument for this flag is pretty brittle. Yes it's _technically_
possible to end up with a compactable filesystem during lfsr_format, but
it's pretty unlikely. And keeping LFS_F_COMPACT around means we'd always
need the lfsr_mtree_gc circuitry in lfsr_format, for such a niche
situation, that can be easily cleaned up in lfsr_mount.
So dropping for now.
No code changes, but this does mean one less feature to support:
code stack ctx
before: 37804 2608 620
after: 37804 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
Looking at future planned features, we're running into some real issues
fitting all these flags into 32 bits.
I think the only real use case for LFS_T_MTREEONLY is in
lfsr_traversal_t, where the depth of traversal can't be infered. So no
reason to keep this flag around in the other APIs.
No code changes:
code stack ctx
default before: 37804 2608 620
default after: 37804 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37940 2608 768
gc after: 37940 (+0.0%) 2608 (+0.0%) 768 (+0.0%)
- LFS_I_INCONSISTENT -> LFS_I_MKCONSISTENT
- LFS_I_CANLOOKAHEAD -> LFS_I_LOOKAHEAD
- LFS_I_UNCOMPACTED -> LFS_I_COMPACT
- LFS_I_CANCKMETA -> LFS_I_CKMETA
- LFS_I_CANCKDATA -> LFS_I_CKDATA
This just makes everything easier to read/pattern match, even if it's
a bit inaccurate english-wise. The imperative transformations were also
wildly inconsistent...
- lfsr_gc -> lfsr_fs_gc
- lfsr_gc_unck -> lfsr_fs_unck
lfsr_fs_unck is surprisingly still useful in non-gc builds, since we
still have ckmeta/ckdata state. These flags can still be queried with
lfsr_fs_stat and cleared with lfsr_fs_ckmeta/ckdata/lfsr_traversal_t, so
it seems useful to keep this function around.
It's also a relatively cheap function.
Though this does mean it deserves a rename. Dropping the gc prefix
hopefully makes it clearer this function is not entirely gc-specific.
And since we no longer have lfsr_gc_setflags/setsteps, it makes sense to
rename lfsr_gc back to lfsr_fs_gc, to be consistent with the other
filesystem-wide utilities.
Code changes, apparently lfsr_fs_unck costs 12 bytes:
code stack ctx
default before: 37792 2608 620
default after: 37804 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37938 2608 768
gc after: 37940 (+0.0%) 2608 (+0.0%) 768 (+0.0%)
LFS_GC_CKMETA and LFS_GC_CKDATA are a bit unique in that their work is
never really done.
Where LFS_GC_MKCONSISTENT/COMPACT can prove things about the system,
LFS_GC_CKMETA/CKDATA can't, because it's always possible for new
bit-errors to develop. Even _during_ an LFS_GC_CKMETA/CKDATA traversal.
But while this is technically true, it's not a very useful state of
things for our lfsr_gc API...
---
What we really want is some way to know if ckmeta/ckdata has completed
"recently" (for some definition of recently), and to let users indicate
when they need another ckmeta/ckdata scan.
To try to solve this:
1. Added LFS_I_CANCKMETA and LFS_I_CANCKDATA to indicate when lfsr_gc
has not checked metadata/data.
These are set during mount (unless mounting with
LFS_M_CKMETA/CKDATA), and cleared when either lfsr_gc completes or
lfsr_fs_ckmeta/data is called. Once cleared, littlefs will not reset
them on its own.
2. Added lfsr_gc_unck to allow users to explicitly reset LFS_I_CKMETA
and/or LFS_I_CKDATA, which will tell lfsr_gc to check metadata/data
again on the next call.
There is some subtlety around clobbering ongoing traversals, but a
mask and some tests should prevent this from being a problem.
Currently, lfsr_gc_unck also allows clearing of other gc flags, but
I'm not sure there's any real use-case for this...
Note that you can still get the previous behavior if you just call
lfsr_gc_unck after every lfsr_gc call.
This also changes info flag behavior slightly in default mode, with
LFS_I_CANCKMETA/CANCKDATA telling you if metadata/data has been checked
since mount. Which does seem useful? Maybe these flags deserve a better
name?
Code changes:
code stack ctx
default before: 37796 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
default after: 37792 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37896 2608 768
gc after: 37938 (+0.1%) 2608 (+0.0%) 768 (+0.0.%)
Now that you can provide gc_flags/gc_steps in lfs_config, I think it's a
bit more clear that _mutating_ the flags/steps is a niche feature, and
not worth implementing/testing.
It raises the question why not have a similar lfsr_setflags or
lfsr_file_setflags, and the answer there is it would be a pain-in-the-
ass to make sure all possible corner cases are covered.
It actually already was a pain-in-the-ass to test lfsr_gcsetflags/
setsteps... but just because we already did the work is not a good
reason for keeping complexity around.
---
Note that most of the use cases for lfsr_gc_setflags/setsteps can be
covered by either remounting the filesystem or through the
lfsr_traversal_t APIs directly.
The end result is a bit of code savings when incremental gc is enabled:
code stack ctx
default before: 37796 2608 620
default after: 37796 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37944 2608 768
gc after 37896 (-0.1%) 2608 (+0.0%) 768 (+0.0%)
Incremental gc, being stateful and not gc-able (ironic), was always
going to need to be conditionally compilable.
This moves incremental gc behind the LFS_GC define, so that we can focus
on the "default" costs. This cuts lfs_t in nearly half!
lfs_t with LFS_GC: 308
lfs_t without LFS_C: 168 (-45.5%)
This does save less code than one might expect though. We still need
most of the internal traversal/gc logic for things like block allocation
and orphan cleanup, so most of the savings is limited to the RAM storing
the incremental state:
code stack ctx
before: 37916 2608 768
after with LFS_CFG: 37944 (+0.1%) 2608 (+0.0%) 768 (+0.0%)
after without LFS_CFG: 37796 (-0.3%) 2608 (+0.0%) 620 (-19.3%)
On the flip side, this does mean most of the incremental gc
functionality is still availables in the lfsr_traversal_t APIs.
Applications with more advanced gc use-cases may actually benefit from
_not_ enabling the incremental gc APIs, and instead use the
lfsr_traversal_t APIs directly.
Before:
int lfsr_fs_gc(lfs_t *lfs, lfs_soff_t steps, uint32_t flags);
After:
int lfsr_gc(lfs_t *lfs);
int lfsr_gc_setflags(lfs_t *lfs, uint32_t flags);
int lfsr_gc_setsteps(lfs_t *lfs, lfs_soff_t steps);
---
The interesting thing about the lfsr_gc API is that the caller will
often be very different from whoever configures the system. One example
being an OS calling lfsr_gc in a background loop, while leaving
configuration up to the user.
The idea here, is instead of forcing the OS to come up with its own
stateful system to pass flags to lfsr_gc, we just embed this state in
littlefs directly. The whole point of lfsr_gc is that it's a stateful
system anyways.
Unfortunately this state does require a bit more logic to maintain,
which adds code/ctx cost:
code stack ctx
before: 37812 2608 752
after: 37916 (+0.3%) 2608 (+0.0%) 768 (+2.1%)
While they are a bit more annoying to call, init functions give the
compiler a chance to deduplicate common struct initialization logic. So
we should probably prefer init functions for any structs larger than a
couple words.
The cost of each init is small, but it really adds up!
code stack ctx
before: 38036 2608 752
after: 37844 (-0.5%) 2608 (+0.0%) 752 (+0.0%)
These should be the last implicit buffers in LFSR_DATA_* macros, leaving
only LFSR_RAT_* macros with implicit stack-allocations (which are wayyy
too useful to give up).
There's an argument to keep these macros implicit, since they represent
relatively small things, but a stack allocation is a stack allocation.
It's safer to make stack allocations explicit, though it does risk
buffer overflow if these fall out-of-sync...
I guess we're forced to choose our poison...
In the end consistency with other LFSR_DATA_* macros wins.
---
And, again, compound-literals are so poorly optimized this minor cleanup
somehow saves code:
code stack ctx
before: 38060 2608 752
after: 38000 (-0.2%) 2608 (+0.0%) 752 (+0.0%)
This was a disappointing failure of compount-literals.
These macros protect against mismatched buffer sizes, which is great for
preventing bugs caused by simple typos, but the overhead of compound-
literals requiring initialization make them simply unusable.
This commit leaves only a couple macros with implicit buffers:
LFSR_DATA_LEB128, and the LFSR_RAT_CAT/LFSR_RATS macros.
Even the tiny cleanup of the one remaining implicit-buffer macro still
in use, LFSR_DATA_GEOMETRY, saved some code:
code stack ctx
before: 38084 2608 752
after: 38060 (-0.1%) 2608 (+0.0%) 752 (+0.0%)
- Fixed issue where some overflowed compat flags could end up ignored.
A simple typo: incrementing by the unrelated d variable, meant we
were skipping overflowed compat flags whenever the previous logic sets
d > 1.
- Fixed issue where any zero padding was treated as overflowed compat
flags.
Note this hid the previous issue from our tests.
Added more tests to prevent a regression here. Letting bad compat flag
parsing through would be _very_ annoying in the future.
Code changes:
code stack ctx
before: 38148 2608 752
after: 38084 (-0.2%) 2608 (+0.0%) 752 (+0.0%)
To clarify this only checks data reads, and to makes space for future
theoretical ck-operations:
- ckmetaredund - likely
- ckdataredund - unlikely, expensive
- ckmetacksums - unlikely, expensive
- ckdatacksums - implemented
This also tweaks the relevant mount/format/info flags a bit:
LFS_M_CKPROGS 0x00100000 Check progs by reading back progged data
LFS_M_CKFETCHES 0x00200000 Check block checksums before first use
LFS_M_CKPARITY 0x00400000 Check metadata tag parity bits
LFS_M_CKMETAREDUND+ 0x01000000 Check metadata redund blocks on reads
LFS_M_CKDATAREDUND* 0x02000000 Check data redund blocks on reads
LFS_M_CKMETACKSUMS* 0x04000000 Check metadata checksums on reads
LFS_M_CKDATACKSUMS 0x08000000 Check data checksums on reads
+Planned
*Hypothetical
No code changes.
So... Long store short, checking metadata cksums is just intractably
slow.
But data cksums?
Yes checking data cksums is still O(b^2), but unlike metadata lookups,
which involve many small backwards reads, data reads are very easy to
cache. So instead of O(b^2), it's more like O(b^2/c), where c is your
rcache size.
Still O(b^2) when c << b, but I'm not sure that's avoidable without
adding more cksums.
At the very least, if you have enough RAM, c == b reduces this to O(b),
which is nice for "large" systems that want hardened reads without a
performance loss.
---
But why bother checking data cksums if we still have a read-hole with
metadata cksums?
Well, while considering the problem in the context of future features, I
noticed something _really interesting_:
- ckredund + metadata - reasonable ✓
- ckredund + data - impractical ✗, parity fanout + O(f+r) is bad
- ckcksums + metadata - impractical ✗, small reads + O(b^2) is bad
- ckcksums + data - reasonable ✓, assuming enough rcache
The current planned design for data redundancy makes it also intractably
slow to check every read, since it would require xoring all blocks that
contribute to the relevant parity block, but this isn't a problem for
metadata redundancy.
So while neither ckredund nor ckcksums can tractably close the read-hole
on their own, it looks like together they will be able to cover
everything without completely sacrificing performance. Neat!
Of course this isn't possible if ckcksums/ckredund imply checking both
metadata and data, so they need to be split apart.
And I don't really see a point in keeping the intractable variants
around in the codebase.
---
Dropping metadata ckcksums also means we can get rid of the ugly
lfsr_bd_ckrbydprefix and lfsr_bd_ckrbydsuffix functions, which were
basically duplicating all of lfsr_rbyd_fetch. That was quite a wart!
This saves a nice chunk of code when ckcksums is enabled:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckparity before: 39724 3048 764
ckparity after: 39700 (-0.1%) 3048 (+0.0%) 760 (-0.5%)
ckcksums before: 40612 3184 772
ckcksums after: 39396 (-3.0%) 3096 (-2.8%) 760 (-1.6%)
One of the unexpected side-effects of lazy file creation is that
suddenly LFS_O_EXCL doesn't make sense.
The standard definition: "Fail if the file exists", is easy enough to
implement, but doesn't really match what the user expects.
The user expects one of these calls to fail:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
But because we create files lazily (to prevent zero-length files after
powerloss), these both succeed.
---
I considered deferring the "file exists" check until we actually would
create the file, but while this _technically_ satisfies the
exclusitivity requirement, I decided against it as I think it just makes
the API way too confusing:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_close(&lfs, &file_a) => 0;
lfsr_file_close(&lfs, &file_b) => LFS_ERR_EXIST;
---
Instead, a simpler, more pragmatic approach: Fail if the file exists
_or_ if the file is open in a mode that will create the file:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => LFS_ERR_EXIST;
This explicitly does _not_ error on zombie/desync files:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_desync(&lfs, &file_a) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
And it does mean we aren't necessarily guaranteeing the file will be
created, but I think this does more-or-less what the user expects:
- open(a) -> desync(a) -> open(b) -> resync(a) is roughly equivalent to
opening a after creating b, which is perfectly fine with LFS_O_EXCL.
- open(a) -> open(b) (errors) -> desync(a) is one way to not actually
create the file, but is somewhat similar to removing the file after
creation.
If you're using desync files you should probably have a good
understanding of littlefs's sync model anyways.
And of course the user can always sync immediately after open to
guarantee file creation, while opting into the possibility of
zero-length files after powerloss.
Code changes:
code stack ctx
before: 38084 2624 752
after: 38128 (+0.1%) 2624 (+0.0%) 752 (+0.0%)
This is the tradeoff of not erroring on unknown filetypes during mount.
- lfsr_file_open and lfsr_mtree_pathlookup now returns LFS_ERR_NOTSUP
instead of LFS_ERR_NOTDIR/LFS_ERR_ISDIR if it encounters an unkown
filetype.
This gets a bit subtle. You might think LFS_ERR_NOTDIR is reasonable,
but it's possible for our unknown filetype to be something dir-like.
Symlinks are an excellent example.
- lfsr_remove/lfsr_rename now bail with LFS_ERR_NOTSUP if encountering
an unknown filetype.
This conflicts with the POSIX philosophy of remove always being
allowed, but I'm not sure what other option there is. Maybe allowing
removes when mounted with LFS_M_FORCE?
We can't just allow removes by default because of the risk of leaking
resources. Directories being the main example of this (need to clean
up bookmarks).
Maybe leaky filetypes should also set WCOMPAT flags?
Not doing something is cheaper than doing something, so unfortunately
this costs us more than what we saved from dropping the orphan/unknown
scan during mount:
code stack ctx
bail: 38120 2624 725
no-error-no-bail (before): 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%)
error-no-bail (after): 38140 (+0.1%) 2624 (+0.0%) 752 (+0.0%)
But this is probably worth it for the extra flexibility.
The motivation here is to simplify lfsr_mount, but there's a number of
knock-on effects.
For one, lfsr_mount should now be faster on filesystems with large
blocks:
O(nb(log b)(log_b n)) -> O(nb(log_b n))
But we now no longer check if our filesystem contains orphaned
stickynotes or unknown filetypes:
- Orphaned stickynotes turned out to not be a big deal. If we find
orphans we'd need to do a second traversal to remove them anyways (no
mutation allowed in lfsr_mount), so this actually ends up a net
improvement in the found-orphan case.
If anything, doing a traversal on first write sets user expectations
correctly, and can be offloaded with lfsr_fs_mkconsistent or
lfsr_fs_gc.
- Unknown filetypes are a bit more annoying (I actually forgot about
this check), but unknown filetypes that require special care should
probably set WCOMPAT/RCOMPAT flags.
Allowing unknown filetypes is a bit more flexible in cases where a
filesystem image is being shared between drivers with different
features (bootloader + app for example).
Though we should probably add more checks/tests that we're handling
these correctly now that we no longer just bail during mount...
Also renamed LFS_I_HASORPHANS -> LFS_I_UNTIDY.
Not doing something is cheaper than doing something, so this saves a bit
of code:
code stack ctx
before: 38120 2624 752
after: 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%)
- test_attrs_fattr_zombie_no_receive
- test_attrs_fattr_mvrm_fuzz_fuzz
And renamed a number of broadcast tests to try to make it clear exactly
what we're testing:
- test_attrs_fattr_wronly_broadcast -> *_wronly_no_receive
- test_attrs_fattr_rdonly_broadcast -> *_rdonly_no_broadcast
- test_attrs_fattr_desync_broadcast -> *_desync_no_receive
- test_attrs_fattr_resync_broadcast -> *_resync_receive
- test_attrs_fattr_zombie_broadcast -> *_zombie_no_broadcast
We already have lfsr_cat_t so...
lfsr_rattr_t is a pretty fundamental type for littlefs, unfortunately
the name "rattr" is a mouthful. Shortening this to just "rat" hopefully
makes things easier to read at the cost of it being a bit less clear
what lfsr_rat_t actually is.
Though it's possible I've been staring at the dwarf spec (DW_AT_*) for
too long...
Fortunately, while these two code bases have almost completely diverged
at this point, we can at least reuse the reworked test_paths tests.
Mostly involving corner-cases related to trailing-slashes, these changes
gives us better alignment with POSIX and hopefully fewer surprises for
users. The full details of what's changed is in the v2.10 release notes/
commits.
---
Implementing these changes here required a little bit of backpedaling.
Something that worked quite well upstream was the use of trailing junk
in the path to tell if a parent was not found, path must be dir, etc.
This is a bit more awkward with lfsr_mtree_pathlookup, with everything
taking an explicit name_size, but it greatly simplifies the mess that
was lfsr_mtree_pathlookup's error codes.
Now it's just:
- 0 => file found
- 0, lfsr_path_isdir(path) => dir found
- 0, mdir.mid=-1 => root found
- LFS_ERR_NOENT, lfsr_path_islast(path) => file not found
- LFS_ERR_NOENT, !lfsr_path_islast(path) => parent not found
- LFS_ERR_NOTDIR => parent not a dir
Note the special mdir.mid=-1 case for the root. This was needed since
lfsr_mtree_pathlookup can now return LFS_ERR_INVAL (for empty paths, dot
dots above root, etc).
In theory we could've gotten away with a different error code, but none
of them really make sense for this case.
---
The impact on code size is a bit funny. Modifying the path in-place _is_
a cheaper API, at the cost of being a bit more convoluted, but the extra
logic added for POSIX-alignment cancels this out:
code stack ctx
before: 38100 (-0.1%) 2624 (+0.0%) 752 (+0.0%)
after: 38120 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
To avoid the obvious conflict with lfs_attr. Unlike lfsr_rattr_t,
lfs_attr is user facing, so it gets priority.
This name may change in the future if something better comes up, but in
the meantime we need to change the name to _something_.
Is this the reason Linux/BSD/etc call these xattrs?
(Note littlefs's attrs are much more limited than xattrs. We should
_not_ call these xattrs in case we want to add true xattrs in the
future.)
File-attached custom attributes could probably use a bit more testing,
but at the very least this should cover obvious file-broadcasting/
power-loss related issues.
The above-mentioned few things:
- We weren't cleaning up orphans correctly if lfsr_file_open errored.
I think at some point we relied on having no falible operations after
the orphan creation, but various refactoring since moved buffer
allocation after orphan creation.
We could rearrange things so orphan creation is last, but I think it's
safter to just deduplicate file cleanup into the new lfsr_file_close_
function.
- LFS_O_TRUNC prevented attrs from being fetched.
It's easy to see where this went wrong. LFS_O_TRUNC prevents data from
being fetched, but we should still fetch attrs.
This is a bit annoying to fix, for now just added a trunc flag to
lfsr_file_fetch.
Also added a couple tests to catch this if it regresses in the future.
- We tried to fetch attrs on orphans.
This doesn't really hurt anything, but it's a waste of read cycles.
Moving all this stuff around added some code, but lfsr_file_fetch is a
bit easier to read now, which is a good thing:
code stack
before: 38084 2624
after: 38100 (+0.0%) 2624 (+0.0%)
After running into issues with LFS_A_CREAT/EXCL in file-attached custom
attributes, we're left in a really weird place:
- None of lfs_setattr's flags are valid in lfs_attr
- None of lfs_attr's flags are valid in lfs_setattr
I also started thinking about the actual use case for LFS_A_CREAT/EXCL,
and it's really not clear.
littlefs really doesn't care about interprocess communication the same
way POSIX/other filesystem APIs do. We can always rely on integration
layers wrapping up multiple operations in a single mutex, so offering
flexible creation semantics has diminished value. LFS_A_CREAT and
LFS_A_EXCL can both be emulated by calling lfsr_getattr first and
checking its return value.
Thinking ahead to the hypothetical lfsr_set API. The main purpose of
lfsr_set is to provide an API that's easier to use but less powerful
than lfsr_file_open. And adding a flags argument seems to run counter to
that.
For example, if you saw this code with no knowledge of littlefs:
lfsr_setattr(&lfs, "cat", 'a', "meow", 4, 0);
You would probably be surprised that it returns LFS_ERR_NOENT without
additional flags.
I realize Linux sidesteps this with XATTR_CREATE/REPLACE by making 0
default to implicitly creating, but I didn't want to introduce
inconsistent flag behavior like this unless I had to.
---
So for now dropping LFS_A_CREAT/EXCL and flags argument to lfsr_setattr.
Code savings minimal, this was mostly for API ergonomics:
code stack
before: 38104 2624
after: 38084 (-0.1%) 2624 (+0.0%)
Unlike lfsr_setattr/getattr/etc, file-attached custom attributes are
RAM-backed snapshots attached to, well, files, that can be committed
atomically along with the file's contents. Great for power-loss
resilience, but boy does it make a mess of an API.
This API was really where custom attributes needed some TLC.
The biggest change is how file-attached custom attributes interact with
file sync broadcasting.
A common complaint from users is that setting custom attributes did not
update attributes in open file handles. This behavior is _very_
inconsistent with other filesystems and created a lot of confusion.
Since we're nailing down littlefs's snapshot/broadcasting model as a
part of larger changes, it makes sense to also nail down how custom
attributes interact.
In the new model:
- Custom attributes are still in-RAM snapshots. Updates do not
immediately take effect, even across write calls.
- On lfsr_file_sync or lfsr_file_close, custom attributes are written
atomically to disk and broadcasted to all open file handles.
- lfsr_setattr/removeattr also take part in attribute broadcasting. When
called, lfsr_setattr/removeattr updates the attribute on disk and
broadcasts the attribute changes to all open file handles.
- Desynced files do _not_ recieve any attribute broadcasts in the same
way they do not recieve any data broadcasts.
This should hopefully make littlefs behave much more consistently with
other filesystems, while still maintaining a well-defined snapshot and
power-loss properties.
---
The lfs_attr struct also gained several new fields:
// Custom attribute structure, used to describe custom attributes
// committed atomically during file writes.
struct lfs_attr {
// Type of attribute
//
// Note some of this range is reserved:
// 0x00-0x7f - Free for custom attributes
// 0x80-0xff - May be assigned a standard attribute
uint8_t type;
// Flags that control how attr is read/written/removed
uint8_t flags;
// Pointer the buffer where the attr will be read/written
void *buffer;
// Size of the attr buffer in bytes, this can be set to
// LFS_ERR_NOATTR to remove the attr
lfs_ssize_t buffer_size;
// Optional pointer to a mutable attr size, updated on read/write,
// set to LFS_ERR_NOATTR if attr does not exist
//
// Defaults to buffer_size if NULL
lfs_ssize_t *size;
};
Which are useful for several new features:
- lfs_attr now supports LFS_A_RDONLY/WRONLY/RDWR modes.
One of the blockers for attribute broadcasting was in-ROM attributes,
where broadcast updates would hard-fault. But now if you mark in-ROM
attributes as WRONLY, and in-RAM attributes as RDWR, this problem goes
away.
- When opened, lfs_attr now optionally writes the attribute size to the
indirect size field.
No more hacky zero padding and not knowing an attribute's size.
Note this follows the same rules as lfsr_getattr, so it does truncate
if the buffer is too small.
The size field can also be set to NULL, in which case lfs_attr
defaults to the buffer_size. This can be quite useful for pure
ROM-backed attributes.
- Missing attributes are now represented with size=LFS_ERR_NOATTR.
No more zero-sized vs missing attribute ambiguity.
This also makes it possible to remove attributes via lfs_attr, by
setting the size to LFS_ERR_NOATTR manually.
This does lead to a bit of a quirk where buffer_size can be
LFS_ERR_NOATTR, which is a bit weird but at least consistent.
- Changes to lfs_attrs will now always trigger file syncs by default.
Previously, if you changed an attribute, you had to also change the
file's contents for it to get written to disk. As pointed out by users
this is both surprising and difficult to work around.
Solving this is quite tricky since there's no real signalling
mechanism between attribute buffers and littlefs. The best I could
come up with is to read attributes from disk during lfsr_file_sync to
see if anything changed.
At the very least, the new flag LFS_A_LAZY restores the old behavior
in case the extra reads in lfsr_file_sync are problematic.
Though I suspect _most_ calls to lfsr_file_sync immediately follow
intentional changes to a file. It would be interesting to know of
examples where this is not the case...
These new fields do increase the size of lfs_attr, which is a downside,
but thanks to flags fitting in type's padding, this is only an increase
from 3 words (12 bytes) -> 4 words (16 bytes).
---
Other implementation notes:
- I did try to implement LFS_A_CREAT/EXCL in lfs_attr but this proved
to be too messy and inconsistent, so I dropped the idea for now.
The idea was to error with NOATTR/EXIST if the lfs_attr flag in
incompatible with what's on disk, but this led to a lot of complexity
for what is a pretty niche use case.
It's also inconsistent with rdonly attrs, which do _not_ error with
NOATTR during lfsr_file_opencfg, because that would be kind of
annoying.
- Having both `struct lfs_attr` and `lfsr_attr_t` to represent different
things in the codebase is both fragile and confusing. One of these
needs to change, probably `lfsr_attr_t`.
If only I could think of a good name...
One of the nice side-effects of the now-dropped uattr/sattr split was
avoiding this conflict.
- We still need more tests related to how custom attributes interact
with other filesystem operations, but I wanted to get what is
currently working committed, see the TODOs in test_attrs.toml.
All of the new bells and whistles unfortunately do add up.
lfsr_file_sync is also the root of our current stack hot-path, so the
additional attr also adds a bit of stack:
code stack
before: 37116 2608
after: 38104 (+2.7%) 2624 (+0.6%)
Still, having a consistent and flexible API is well worth it.
Though I do think at some point we should add a compile-time option to
opt-out of custom attributes (LFS_NO_ATTR?).
Christopher Haster