And the related config options:
- cfg->file_buffer_size -> cfg->file_cache_size
- file->cfg->buffer_size -> file->cfg->cache_size
- file->cfg->buffer -> file->cfg->cache_buffer
The original motivation to rename this to file->buffer was to better
align with what other filesystems call this, but I think this is a case
where internal consistency is more important than external consistency.
file->cache better matches lfs->pcache and lfs->rcache, and makes it
easier to read code involving both file->cache and other user-provided
buffers.
Keeping the upstream name also helps with continuity.
This is the correct name for our rbyd attr type, even if it requires a
bit more typing.
lfsr_attr_t would be a better name, but that conflicts with our
user-facing attrs.
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%)
This takes advantage of another bit in lfsr_data_t's size field to
differentiate between normal lfsr_data_ts, and lfsr_data_ts in a bptr:
in-RAM buffer: on-disk data: on-disk bptr:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
|00| size | |10| size | |11| size |
+---+---+---+---+ .. +---+---+---+---+ +---+---+---+---+
| ptr -------. | block | | block |
+---+---+---+---+ | +---+---+---+---+ +---+---+---+---+
| (unused) | | | off | | off |
'---+---+---+---' | '---+---+---+---' .. +---+---+---+---+
.---+---+---+---. | | cksize |
| data |<' +---+---+---+---+
: : : | cksum |
'---+---+---+---'
Note this bit is unused even in a theoretical 16/14-bit littlefs mode.
This also leaves space for one more encoding (0b01), but I don't have
any good use for this yet. Previous ideas around an inlined
representation failed to improve anything.
This accomplishes a couple things:
1. We no longer need to return the tag in lfsr_file_lookupnext, since
these can only be blocks or fragments.
2. We no longer need to rely on cksize=0 to determine checksummed data
from non-checksummed data when running with LFS_CKDATACKSUMS.
This was supposed to be a relatively free optimization, but our
lfsr_data_fromslice implementation is being a bit... funky... It seems
we're right on the edge of some inline heuristic, where adding this flag
prevents lfsr_data_fromslice from being inlined, missing a number of
contextual optimizations and causing things to explode.
This can be worked around with __attribute__((always_inline)), but we
should probably revisit our data slicing macros to see if this can be
solved without a compiler specific hack. Relying on such a sensitive
function is not great:
code stack ctx
always_inline: 36320 2584 640
inline: 36424 (+0.3%) 2664 (+3.1%) 640 (+0.0%)
Weird inlining noise aside, this was an overall improvement. Not needing
to fetch tags in lfsr_file_lookupnext saves a bit of stack in our
hot-path, which is nice:
code stack ctx
default before: 36460 2608 640
default after: 36320 (-0.4%) 2584 (-0.9%) 640 (+0.0%)
Hmmm, though maybe not for ckdatacksums:
code stack ctx
ckdatacksums before: 37628 3048 640
ckdatacksums after: 38096 (+1.2%) 3072 (+0.8%) 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%)
D'oh, here I am trying to rely solely on our shrub.block == mdir.block
condition to tell bshrubs and btrees apart, when we already have
LFSR_RBYD_ISSHRUB as an explicit flag in the rbyd code!
Long story short, these are equivalent:
bshrub.trunk & LFSR_RBYD_ISSHRUB => bshrub is shrub
bshrub.block == mdir.block => bshrub is shrub
But in theory flag checks are cheaper and require less things being
in-sync (i.e. fewer things can go wrong).
This also means we only need to look at bshrub.trunk to determine if
it's a bshrub, btree, or neither (trunk=0):
bnull: bshrub: btree:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
| weight=0 | | weight>0 | | weight>0 |
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
| block=mdir | | block=mdir | | block!=mdir |
+---+---+---+---+ .. +---+---+---+---+ +---+---+---+---+
| (unused) | | (unused) | | (unused) |
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
|0| trunk=0 | |1| trunk | |0| trunk |
+---+---+---+---+ +---+---+---+---+ .. +---+---+---+---+
| (unused) | | estimate | |p| eoff |
+ + +---+---+---+---+ +---+---+---+---+
| | | (unused) | | cksum |
'---+---+---+---' '---+---+---+---' '---+---+---+---'
As a side-effect, lfsr_file_truncate/fruncate are back to dropping
zero-weight btrees even if they have erased-state (now handled in
lfsr_file_carve). On reflection this is the simpler approach, consistent
with LFS_O_TRUNC, uses fewer blocks, and if keeping erased-state turns
out to be more valuable we can always change this in the future.
Though we should at least add a test that we can read existing
zero-weight btrees and bshrubs...
---
This ended up highlighting that we were leaving dangling bshrub
references in lfsr_mtree_traverse_!
You may think these dangling references would've been fine with the
previous logic, but they could've created problems when the block
allocator makes a full circle. Not great!
Fortunately, relying on LFSR_RBYD_ISSHRUB is a lot safer, and lets us
catch issues like this with asserts in lfsr_mdir_commit.
---
Code savings were a bit disappointing, but any change that reduces
assumptions in the code is a good change:
code stack ctx
before: 36460 2608 640
after: 36424 (-0.1%) 2608 (+0.0%) 640 (+0.0%)
Now that we don't use bmoss or bsprouts anymore, we can drop the
LFSR_BSHRUB_ISNULLORBMOSSORBPTR flag and simplify our lfsr_bshrub_t
struct quite a bit.
However, we do still need a bnull representation, which is surprisingly
tricky... And annoying...
Current solution: Bnulls are bshrubs with weight=0. This works, but
unfortunately does mean we need to update bnull blocks on mdir
relocation/compaction, and risks bnull blocks falling out-of-sync, which
is a really weird thing to worry about:
bnull: bshrub: btree:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
| weight=0 | | weight>0 | | weight |
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
| block=mdir | | block=mdir | | block!=mdir |
+---+---+---+---+ .. +---+---+---+---+ +---+---+---+---+
| (unused) | | (unused) | | (unused) |
+ + +---+---+---+---+ +---+---+---+---+
| | | trunk | | trunk |
+ + +---+---+---+---+ .. +---+---+---+---+
| | | estimate | | eoff |
+ + +---+---+---+---+ +---+---+---+---+
| | | (unused) | | cksum |
'---+---+---+---' '---+---+---+---' '---+---+---+---'
Note we can't just assume all weight=0 files are bnulls, or else we
won't use erased-state in empty btree roots. This risks thrashing in
files oscillating around weight=0.
Technically, weight=0 bshrubs _are_ slightly different than bnulls (
bshrubs point to a null tag, while bnulls simply have no tree), but
unlike btrees, there's no reason to keep weight=0 bshrubs around. Any
bshrub erased-state can still be used by the mdir.
This change also makes LFS_O_TRUNC and lfsr_file_truncate/fruncate
behave slightly differently, with LFS_O_TRUNC unconditionally reverting
to a bnull, while lfsr_file_truncate/fruncate tries to keep the btree
root around. This may be worth revisiting...
---
Despite the awkward encoding, this simplification still ends up saving a
nice bit of code and stack:
code stack ctx
before: 36668 2616 640
after: 36460 (-0.6%) 2608 (-0.3%) 640 (+0.0%)
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%)
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.
lfs->seed was already just the rough xor of all mdir cksums, so
replacing it with our gcksum doesn't really do anything but make its
definition more rigorous.
That and save both code and ctx:
code stack ctx
before: 38560 2624 644
after: 38492 (-0.2%) 2624 (+0.0%) 640 (-0.6%)
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%)
This is mainly to free up space for flags, we're pretty close to running
out of 32-bits with future planned features:
1. Reduced file type info from 8 -> 4 bits
We don't really need more than this, but it does mean type info is
no longer a simple byte load.
2. Moved most internal file-state flags into the next 4 bits
These are mostly file-type specific (except LFS_o_ZOMBIE), so we
don't need to worry too much about overlap.
3. Compacted ck-flags into 5 bits:
LFS_M_CKPROGS 0x00000800
LFS_M_CKFETCHES 0x00001000
LFS_M_CKPARITY 0x00002000
LFS_M_CKMETAREDUND* 0x00004000
LFS_M_CKDATACKSUMS 0x00008000
*Planned
Now that ck-flags are a bit more mature, it's pretty clear we'll
probably never have CKMETACKSUMS (ckcksums + small tag reads is
crazy expensive) or CKDATAREDUND (non-trivial parity fanout makes
this crazy expensives. So reserving bits for these just wastes bits.
This also moves things around so ck-flags no longer overlap with open
flags.
It's a tight fit, and I still think file-specific ck-flags are out-of-
scope, but this at least decreases flag ambiguity.
New jenga:
8 8 8 8
.----++----++----++----.
.-..-..-.-------.------.
o_flags: |t||f||t| | o |
|-||-||-|-------:--.---'
|-||-||-'--.----.------.
t_flags: |t||f|| t | | tstt |
'-''-'|----|----'------'
.----.|----|.--.:--:.--.
m_flags: | f || t ||c ||o ||m |
|----||-.--'|--|'--''--'
|----||-|---|--|.------.
f_flags: | f ||t| |c || f |
'----''-'---'--''------'
Fortunately no major code costs:
code stack ctx
before: 37792 2608 620
after: 37788 (-0.0%) 2608 (+0.0%) 620 (+0.0%)
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%)
See previous commit for more details on why this doesn't work:
1. Losing the simple/compiler friendly lfsr_data_t costs more code/stack
than we save inlined small pieces of data (dids, leb128s, flags,
etc).
2. Inlined lfsr_data_t is fundamentally incompatible with the new
lightweight lfsr_rat_t representation for simple data.
Though I did add a comment, and marked lfsr_data_fromslice as inline.
The compiler was apparently already inlining lfsr_data_fromslice (and
it's a valuable optimization!), but making this explicit helps document/
influence future changes.
Code changes:
code stack ctx
before: 38128 2672 752
after: 38060 (-0.2%) 2608 (-2.4%) 752 (+0.0%)
The idea, which has floated up a few times, is to add a third
representation of lfsr_data_t where the data is inlined in the struct
directly. In theory saving RAM for small pieces of data such as dids,
leb128s, flags, etc:
inlined: in-RAM buffer: on-disk:
.---+---+---+---. .---+---+---+---. .---+---+---+---.
|01| size | |00| size | |1| size |
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
| inlined data | | ptr -------. | block |
+ + +---+---+---+---+ | +---+---+---+---+
| | | (unused) | | | off |
'---+---+---+---' '---+---+---+---' | '---+---+---+---'
.---+---+---+---. |
| data |<'
: : :
Unfortunately in practice this just doesn't work out.
It turns out we benefit a lot from the _simplicity_ of lfsr_data_t. When
lfsr_data_t is built out of simple words, the compiler can make some
pretty strong assumptions and basically break it down into simple
register operations.
When you stick a byte array in the middle of the struct, this sort of
breaks down.
---
We can see this in our code measurements. After adding inlined data, but
before implementing slicing (in lfsr_data_fromslice), we can see decent
stack savings. But as soon as we add the memmove to lfsr_data_fromslice,
any benefit is lost:
code stack ctx
before: 38060 2608 752
without slicing: 38056 (-0.0%) 2568 (-1.5%) 752 (+0.0%)
after: 38128 (+0.2%) 2672 (+2.5%) 752 (+0.0%)
One reason for this is the extra logic does cause lfsr_data_fromslice to
be no longer inlined, but adding __attribute__((always_inline)) only
claws back some of the code/stack savings (though it's interesting to
note the compiler heuristic failure here):
code stack ctx
before: 38060 2608 752
after+inline: 38128 (+0.2%) 2672 (+2.5%) 752 (+0.0%)
after+always_inline: 38684 (+1.6%) 2656 (+1.8%) 752 (+0.0%)
---
Oh, and inlined lfsr_data_t is no longer compatible with LFSR_RAT's
simple data conversion, since lfsr_rat_t's can only point to existing
buffers. This causes tests to fail rather quickly.
This should be reverted, but I think the hidden cost of inlined
lfsr_data_t is surprising and interesting to note.
This reduces lfsr_ck_t to just the cksize/cksum fields, and moves all of
the compile-time ifdef LFS_CKDATACKSUMS logic up into the relevant
lfsr_data_* functions.
This doesn't solve the lfsr_data_t/lfsr_bptr_t duplication problem,
unfortunately, but does simplify the code base a bit.
No significant code changes:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckdatacksums before: 39240 3008 752
ckdatacksums after: 39232 (-0.0%) 3008 (+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.
Unfortunately ckparity has proven itself to be much less useful than
originally thought.
The use of leb128 encoding in our tags means that ckparity can't even
detect single bit-errors reliably. Which raises the question: is
ckparity really worth all of the extra baggage necessary to track parity
in our codebase?
Fortunately we don't have to toss out ckparity entirely!
If we only check parity bits in lfsr_bd_readtag_, instead of on every
read, we still have a reasonable chance of noticing parity errors during
metadata lookups.
This does weaken ckparity, but allows us to drop a lot of lfsr_data_t's
ckparity baggage, at the cost of no longer, uh, unreliably detecting
parity errors during reads?
The limited error detection of ckparity means we can't reliably detect
errors during reads anyways, so we might as well keep the code/RAM/
maintenance implications at a minimum to make ckparity remotely worth
it.
---
Note the significant savings for both LFS_CKPARITY and LFS_CKCKSUMS.
Tracking parity info in lfsr_data_t had a heavy cost:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckparity before: 39700 3048 760
ckparity after: 38476 (-3.1%) 2696 (-11.5%) 760 (+0.0%)
ckcksums before: 39396 3096 760
ckcksums after: 39240 (-0.4%) 3008 (-2.8%) 752 (-1.1%)
This also means lfsr_ck_ckprefix/cksuffix calls always have a ckoff of 0
(bptrs only), which means even more code savings, yay!
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%)
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%)
As a part of the effort to undo the overuse of the term "orphan".
I can't really think of a better name, and uncreat gets the point
across. At least it matches LFS_O_UNSYNC/LFS_O_UNFLUSH.
Apparently the Uncreated are a race of aliens in the Marvel universe?
I've been unhappy with LFSR_TAG_ORPHAN for a while now. While it's true
these represent orphaned files, they also represent zombied files. And
as long as a reference to the file exists in-RAM, I find it hard to say
these files are truely "orphaned".
We're also just using the term "orphan" for too many things.
Really this tag just represents an mid reservation. The term stickynote
works well enough for this, and fits in with the other internal tag,
LFSR_TAG_BOOKMARK.
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...
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.)
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?).
These functions provide simple access to littlefs's custom attributes,
which are small pieces of user-specified metadata that can be attached
to files, dirs, root, etc:
- lfsr_getattr - Reads an attribute
- lfsr_sizeattr - Gets the size of an attribute
- lfsr_setattr - Writes an attribute
- lfsr_removeattr - Removes an attribute
You may notice these functions look quite a bit different from their
previous incarnations. This is because the custom attribute API is
getting an overhaul based on feedback provided by users
The previous API had some real design flaws that interfered with
usability, but now that things have had some time to settle (6 years!),
hopefully most of the pain points are clear.
Notable changes:
- lfsr_getattr's return value is now limited by buffer size.
The intention of the previous API, where lfsr_getattr always returns
the attr size, even if it's larger than the buffer, was to allow users
to find the attr size without an infinitely large buffer.
In defense of this design, Linux's getxattr does something somewhat
similar, returning the attr size when the buffer size equals zero.
Though getxattr does truncate when buffer size is non-zero, which is
probably safer.
But, let's be honest, this multipurpose abuse of lfsr_getattr's return
value is inconsistent with other read functions and potentially
dangerous for users.
I think one of the reasons for this API in Linux-land is the limited
syscall numbers discouraging new functions, but we have no such
limitation here! We might as well add a dedicated function for
this: lfsr_sizeattr.
- No more padding with zeros!
This was a cludge to get around the lack of returned size in custom
attributes attached to files, but is inconsistent with other read
functions, so needs to go.
In general, inconsistencies violate user assumptions, and are usually
a sign of a bad API.
- lfsr_setattr now takes flags.
This gives lfsr_setattr more flexiblity in how it operates, and may
make future extensions easier.
lfsr_setattr currently supports two flags, which may look a bit
familiar:
LFS_A_CREAT 0x04 // Create an attr if it does not exist
LFS_A_EXCL 0x08 // Fail if an attr already exists
One long-term idea is to eventually add a simple lfsr_set function to
make it easier to create small files, so this sort of design overlap
between lfsr_setattr and lfsr_file_open is hopefully a good thing.
---
Code-wise, these function are really not that bad. Adding functions adds
code, but these are just small wrappers over our internal lookup/commit
functions:
code stack
before: 36556 2608
after: 37116 (+1.5%) 2608 (+0.0%)
Of course the real cost of custom attributes is how they interact with
open files, a detail which is conveniently missing for now...
lfsr_file_resync discards the current working state of a file and
reverts it to the contents on disk. It also clears the desynced flag
from files, so provides an alternative to lfsr_file_sync for when you
don't want to write to the filesystem:
disk=A file=A disk=A file=A
| write B | write B
v v
disk=A file=B disk=A file=B
| sync | resync
v v
disk=B file=B disk=A file=A
The main motivation for this is to provide a way to mark desynced
readonly files as in-sync, without putting them into a weird state where
they are "in-sync" but don't match disk.
It's also a bit safer if the file is desynced due to an error, since
errors aren't currently guaranteed to leave file data in a defined
state. Needed to resync to recover from errors avoids accidentally
syncing partial writes.
This exact behavior can also be accomplished by closing+opening the
file, but lfsr_file_resync makes it much easier without _that_ much
extra code. It may even pay for itself if you consider what code it
saves on the user's side of things.
I considered naming this lfsr_file_discard because I think it sounds
cooler, but I figured including sync in the name provides a stronger
hint that it affects the file's desync status.
---
You may think it's not possible for a readonly file to become
out-of-sync from disk, since it's, well, readonly. But it is possible
thanks to desynced files ignoring other sync broadcasts.
Consider what happens if you open a file readonly, and write+sync the
file with another file handle at the same time:
disk=A f1=A f2=A
| desync f2
v
disk=A f1=A f2=A
| write f1=B
v
disk=A f1=B f2=A
| sync f1
v
disk=B f1=B f2=A <-- f2 is out-of-sync without any writes
---
This commit also changes lfsr_file_sync/flush to assert if the file is
readonly. Previously we allowed lfsr_file_sync to be called on readonly
files if it would be a noop, but lfsr_file_resync makes this
unnecessary.
More code means more code, but I think it is well worth it for the
additional flexibility:
code stack
before: 36412 2616
after: 36748 (+0.9%) 2616 (+0.0%)
We just have too many flags! Mount flags specifically are already close
to filling up with the currently planned features.
Fortunately the info flags, used internally to track filesystem state,
are never needed at the same time as the traversal flags which specify
one-time traversals during lfsr_mount. So we can move these to overlap
and free up quite a bit more space:
8 8 8 8
.----++----++----++----.
.----..-..-..----------.
o_flags: |type||f||t|| o |
|----||-|:-:'--.-.-----'
|----||-|:-:---:-:-----.
d_flags: |type||f|: : : : |
|----||-|:-:---:-:-----'
|----||-|:-'--..-..----.
t_flags: |type||f|| t ||f||tstt|
'----''-'|----|'-''----'
.--------|----|:-:-----.
gc_flags: | | t |: : |
'--------|----|:-:-----'
.-------.|----|.-------.
f_flags: | m || t || f |
|-------||----|'-------'
|-------||----|:-:.----.
m_flags: | m || t ||o|| m |
|-------|'----'|-||----|
|-------|.----.|-||----|
i_flags: | m || i ||o|| m |
'-------''----''-''----'
The only downside is a bit more masking and not having this info
available when debugging.
The overlap is also convenient for lfsr_fs_gc and lets us remove some
shifts, which humorously perfectly canceled out the added cost of the
masks:
code stack
before: 36416 2616
after: 36416 (+0.0%) 2616 (+0.0%)
Since we already need all the machinery to track ck info for ckparity, I
figured we might as well implement a full ckcksums option as well.
Ckcksums closes the checksum-read-hole by reading enough data to check a
relevant checksum on ever read, even if this ends up being significantly
more data than the initial request. This should always detect detectable
bit-errors, even if they occur between consecutive reads.
If this sounds naive, that's because it is. Performance will be awful.
To be clear, ckcksums should probably never be used in production. I
can't think of a use case that isn't better handled by either ECC in the
block device or the future-planned ckredund feature. Just look at the
runtime complexities:
small-reads rbyd-lookup rbyd-compaction
ckcksums: O(b^2) O(b log b) O(b^2 log b)
ckredund*: O(log_b(n) + xb) O(log b) O(b log b)
eccbd*: O(b) O(log b) O(b log b)
* theoretical
We've already seen that O(b^2) compactions turns a performance problem
into a tractability problem, so I think O(b^2 log b) compactions will be
a bit too much for most applications.
We can already seen this in our test_ck_ckcksums_* tests (which do pass
by the way!). Compare to test_ck_ckprogs_*, which is basically the same
set of tests:
test_ck_ckprogs_*: 6.08s
test_ck_ckcksums_*: 64.88s
Or consider test_rbyd with/without ckcksums:
test_rbyd: 12.21s
test_rbyd+ckcksums: 389.94s
Still, ckcksums is an interesting proof-of-concept, and does manage to
close the checksum-read-hole.
---
Like ckprogs/ckfetches/ckparity/etc, ckcksums is an opt-in feature,
requiring both 1. defining LFS_CKCKSUMS and 2. passing LFS_M_CKCKSUMS at
mount time.
Like ckparity, ckcksums requires a significant code and stack increase
to track ck info in lfsr_data_t:
code stack
before: 36416 2616
yes-ckcksums: 38872 (+6.7%) 3176 (+21.4%)
no-ckcksums: 36416 (+0.0%) 2616 (+0.0%)
It's interesting to note how this compares to all of the current
ck-modes, though each has their own set of tradeoffs:
code stack
default: 36416 2616
ckprogs: 36468 (+0.1%) 2616 (+0.0%)
ckfetches: 36666 (+0.7%) 2648 (+1.2%)
ckparity: 37996 (+4.3%) 3040 (+16.2%)
ckcksums: 38872 (+6.7%) 3176 (+21.4%)
---
Note that even though ckcksums is opt-in, it may still be worth removing
from the codebase in the future, for a couple reasons:
- Every feature, even if unused, adds developer/maintenance burden.
- Ck info is particularly messy with how it interacts with all
lfsr_data_t APIs. Though getting rid of ck info would also require
getting rid of ckparity.
- It's possible for a user to see ckcksums in the codebase,
misunderstand its tradeoffs, enable it, and get the impression that
littlefs itself is just unusably slow.
So instead of always reading the parity byte on demand, we read it once
in lfsr_bd_readtag, and store it in an unused bit in lfsr_data/ck_t.
The main reason for this is to avoid rereading that byte all the time.
Though I suppose there is also an ever-so-tiny increase in chance of
catching a bit-error after lfsr_bd_readtag. Assuming RAM is more
reliable than disk...
It also keeps the read-parity-byte mess limited to lfsr_bd_readtag, and
simplifies lfsr_bd_ckprefix/cksuffix a bit, which is nice. Though at the
cost of making lfsr_bd_readtag's API a bit most awkward with the
addition of the ckparity-specific parity_ parameter.
This adds a bit more code, but ends up saving some stack:
code stack
default before: 36412 2616
default after: 36416 (+0.0%) 2616 (+0.0%)
ckparity before: 37900 3048
ckparity after: 37948 (+0.1%) 3032 (-0.5%)
The extra 4-bytes in our non-ckparity build comes from us moving the
saving of the ecksum to after checksum calculation, since we need to
know the parity in the ckparity build. So just compiler noise.
Ckparity is pretty flawed in littlefs, for several reasons. The biggest
one being that we can't even reliably detect single-bit errors.
But! It can still provide an extra layer of safety in a system where you
don't care about the extra code/stack cost.
And, for ckreads, performance cost...
Performance isn't a big problem for parity-checking. We can assume
metadata tags are going to relatively small (and can be controlled by
fragment_size). But for data checksums, ckreads risks O(b^2) when
performing many small reads, which can be a bit of a problem.
And since ckreads doesn't really prove anything interesting about the
system anymore, it makes sense to unbundle these two checks, rename
ckreads -> ckparity, and limit it to only checking parity bits.
This way, you can enable ckparity for a bit of extra safety, with a
code/stack cost hit, but without sacrificing performance.
---
I was hoping more code/stack savings, but since we still need to track
parity context in lfsr_data_t, and still need to intercept bd_read/cmp/
cpy calls that reference metadata, we end up needing to keep most of
the ck circuitry around:
code stack
default before: 36464 2672
default after: 36464 (+0.0%) 2672 (+0.0%)
code stack
ckparity before: 38036 3080
ckparity after: 38024 (-0.0%) 3080 (+0.0%)
We even end up still tracking checksum context for bptrs! Maybe we
should just go ahead and add ckcksums as a joke...
This replaces the lfsr_mptr_t struct with simple arrays.
The main motivation for this is C99's strict aliasing. It saves a
decent amount of stack to reference the mdir's internal block array as
an mptr directly, but we were only able to accomplish this in
lfsr_mdir_mptr by violating C99's strict aliasing rules.
The main downside of this is C's wonderful array-to-pointer decay
resulting in more implicit references and chances for things to get
clobbered (the original motivation for lfsr_mptr_t was due to bugs
introduced this way).
If I know one thing about C99's strict aliasing it's that it sure loves
to make code less safe.
No significant code changes, which is probably a good thing:
code stack
default before: 36436 2672
default after: 36432 (-0.0%) 2672 (+0.0%)
ckfetches before: 36674 2704
ckfetches after: 36666 (-0.0%) 2704 (+0.0%)
Christopher Haster