This does a couple things:
- Makes attr-lists a bit more self-documenting.
- Adds a bit more type-safety. The LFSR_RATTR_* macros should be able to
reject types that don't match the expected encoding.
- Makes it easier to adjust dsize estimates at one location.
Specifically, this makes it harder to forget bptr's LFSR_BPTR_DSIZE.
---
Surprisingly this did have a small impact on code size. I'm not entirely
sure why, but considering how much of the codebase this touches I'm just
going to chalk this up to compiler noise:
code stack ctx
before: 35488 2440 636
after: 35536 (+0.1%) 2440 (+0.0%) 636 (+0.0%)
lfsr_file_carve seems the hardest hit:
function (0 added, 0 removed) osize nsize dsize
lfsr_file_open 16 20 +4 (+25.0%)
lfsr_file_carve 1316 1356 +40 (+3.0%)
lfsr_remove 408 412 +4 (+1.0%)
TOTAL 35488 35536 +48 (+0.1%)
Mainly just for self-documentation reasons.
This may also make it easier to add LFSR_RATTR_BUF-specific asserts/
tweaks/etc, and helps future refactoring.
But functionally LFSR_RATTR_BUF is equivalent to LFSR_RATTR for now.
No code changes.
Mainly for self-documentation reasons. This is identical to
LFSR_RATTR_LEB128 except for an additional assert in
lfsr_rbyd_appendrattr_.
I was considering removing the little-leb128 concept, but it is still
helping reduce the worst-case size of the various structs we write to
disk. LFSR_BRANCH_DSIZE, for example, contributes heavily to our stack
hot-path.
No code changes.
The only requirement we have for dsize estimates is to help calculate
shrub estimates. So why bother with dsize for attrs that are never
committed to shrubs?
At the moment, the only tags we commit to shrubs are:
- LFSR_TAG_DATA
- LFSR_TAG_BLOCK
- LFSR_TAG_BRANCH
This saves a decent chunk of code:
code stack
before: 35556 2440 636
after: 35488 (-0.2%) 2440 (+0.0%) 636 (+0.0%)
No idea how long this return has been missing, but we should never
ignore a returned err without at least an assert. And this one should
definitely not be an assert.
Code changes minimal:
code stack ctx
before: 35552 2440 636
after: 35556 (+0.0%) 2440 (+0.0%) 636 (+0.0%)
This was the one lazy attr that did _not_ save us code or stack, and in
fact hurt us a bit. Fortunately our lazy attr scheme still allows for
eager attr encoding, so we can revert just this one attr.
The reason is because we need to keep the children branches around as we
recursively commit up the btree, and, even assuming worst-case, the
branch encoding takes up way less RAM than an active rbyd.
Compare for yourself:
in-RAM rbyd: on-disk branch:
.---+---+---+---. .---+- -+- -+- -+- -.
| weight | | block |
+---+---+---+---+ +---+- -+- -+- -+- -'
| blocks | | trunk |
+ + +---+- -+- -+- -+
| | | cksum |
+---+---+---+---+ '---+---+---+---'
|s| trunk |
+---+---+---+---+
|p| eoff |
+---+---+---+---+
| cksum |
'---+---+---+---'
'-------.-------' '---------.---------'
24 bytes 13 bytes
Note we also don't have to care about alignment issues when passing
around the raw encoding.
There may also be something going on with the compiler assuming all
lfsr_rbyd_t pointers may alias, but it's a bit hard to tell.
Saves both code and stack:
code stack ctx
before: 35592 2472 636
after: 35552 (-0.1%) 2440 (-1.3%) 636 (+0.0%)
With the new internal LFSR_RATTR API, there's really no reason to keep
these around.
At one point these were useful for both the implicit lvalues and
automatic buffer size, but GCC's problems with compound-literals and
code size made them almost always backfire.
Now, they're mostly obsolete thanks to the new LFSR_RATTR_* macros.
We do still have a couple LFSR_DATA_* macros (LFSR_DATA_BUF,
LFSR_DATA_SLICE, etc), but these are a bit more fundamental to the
lfsr_data_t type.
This finishes the eager -> lazy attr encoding rework.
Which makes it a good time to look at the total savings from adopting
lazy attr encoding, though there's still a bit of tinkering to do (eager
branches, cksum tags, etc):
code stack ctx
before lazy-attrs: 36280 2576 636
after lazy-attrs: 35592 (-1.9%) 2472 (-4.0%) 636 (+0.0%)
A ~free 688 byte savings in code and 104 bytes in stack is not bad.
Now that rattr.u.cat is no longer in use, the mysterious code size
increase is gone...
Still no idea why this as _any_ impact on code size though:
code stack ctx
before: 35596 2472 636
after: 35592 (-0.0%) 2472 (+0.0%) 636 (+0.0%)
This fully adopts LFSR_RATTR__ and friends:
- LFSR_RATTR -> LFSR_RATTR__ or LFSR_RATTR_DATA__
- LFSR_RATTR_BUF -> LFSR_RATTR__
- LFSR_RATTR_CAT -> LFSR_RATTR_CAT__
- LFSR_RATTR_NOOP -> LFSR_RATTR_NOOP__
- LFSR_RATTR_NAME -> LFSR_RATTR_NAME__
Note the new LFSR_RATTR__ macro also lets us a drop the special rattr
macros, at the cost of a bit less type safety:
- LFSR_RATTR_RATTRS -> LFSR_RATTR__
- LFSR_RATTR_MOVE -> LFSR_RATTR__
- LFSR_RATTR_GRM -> LFSR_RATTR__ (we weren't using this?)
- LFSR_RATTR_SHRUBCOMMIT -> LFSR_RATTR__
Curiously, this ended up adding ~88 bytes to lfsr_file_carve:
function (0 added, 0 removed) osize nsize dsize
lfsr_file_carve 1228 1316 +88 (+7.2%)
lfsr_mdir_commit 2144 2152 +8 (+0.4%)
lfsr_mdir_commit__ 1192 1188 -4 (-0.3%)
lfsr_file_truncate 184 182 -2 (-1.1%)
lfsr_mount 98 96 -2 (-2.0%)
TOTAL 35508 35596 +88 (+0.2%)
I'm really not sure why, all I can think of is maybe the change from a
forced-inline function to a macro added a bunch of compiler noise?
Still, 80 bytes is not worth two competing LFSR_RATTR APIs. Though
it may be worth looking into this in the future.
Total code changes:
code stack ctx
before: 35508 2472 636
after: 35596 (+0.2%) 2472 (+0.0%) 636 (+0.0%)
- LFSR_TAG_BRANCH -+-> lfsr_data_frombranch
- LFSR_TAG_SHRUBBRANCH -'
This was a bit more involved than the others, since it requires changing
what we store in lfsr_bscratch_t.
I did poke around a bit with trying to reduce the total number of rbyd
allocations in lfsr_btree_commit__, but unfortunately we need both the
sibling rbyds and attr rbyds allocated at the same time while we
recursively figure out how to split/merge/commit.
I also almost forgot to include the dsize estimate, but fortunately this
was caught by our tests. We _do_ need branch dsizes, since they
contribute to any non-inlined bshrub's shrub estimate!
---
This was unfortunately a net-negative. We were actually getting a lot of
value from the smaller encoded branch size:
code stack ctx
before: 35472 2440 636
after: 35508 (+0.1%) 2472 (+1.3%) 636 (+0.0%)
It may be worth reverting this, but I want to see how it interacts when
all of the existing eager rattr encoding logic has been removed.
This one is interesting in that we don't just encode to a buffer, but
need to express the concatenation of did + name somehow. Fortunately we
can still leverage the cat circuitry by setting data_count=-2:
- LFSR_TAG_NAME -+-> cat(fromleb128(did), name)
- LFSR_TAG_REG -+
- LFSR_TAG_DIR -+
- LFSR_TAG_STICKYNOTE -'
This does break our shrub estimate for name attrs (currently names have
no technical limit), which would be an issue, but we just happen to never
commit names to shrubs.
In theory you _could_ accurately estimate name attrs if you limited
names to <=(2^15)-5, but I figured this wouldn't be worth the extra code
cost in LFSR_RATTR_NAME__... Especially since it would just go unused...
Saves a nice bit of code, though no stack since we currently don't
allocate any names on the hot-path (lfsr_file_truncate):
code stack ctx
before: 35580 2440 636
after: 35472 (-0.3%) 2440 (+0.0%) 636 (+0.0%)
Note this does _not_ include LFSR_TAG_BOOKMARK, which only contains the
did and can avoid a stack allocation if encoded as a single leb128 attr.
Though breaking up the LFSR_TAG_NAME types does risk a more complicated
switch-case-table...
- LFSR_TAG_GEOMETRY ---> lfsr_data_fromgeometry
Not much to say about this one, LFSR_TAG_GEOMETRY is a bit of an
outlier.
I did consider deduplicating with the mptr encoder, but decided that
would be too hacky, and create problems for future metadata redundancy
things.
Still saves code though, which is nice:
code stack ctx
before: 35632 2440 636
after: 35580 (-0.1%) 2440 (+0.0%) 636 (+0.0%)
- LFSR_TAG_RCOMPAT -+-> lfsr_data_fromle32
- LFSR_TAG_WCOMPAT -+
- LFSR_TAG_OCOMPAT -+
- LFSR_TAG_GCKSUMDELTA -'
- LFSR_TAG_NAMELIMIT -+-> lfsr_data_fromleb128
- LFSR_TAG_FILELIMIT -+
- LFSR_TAG_BOOKMARK -+
- LFSR_TAG_DID -'
This is nice mainly from an internal API standpoint. Single le32/leb128
attrs should be pretty lightweight, and it's nice for the API to reflect
that.
With a bit of tinkering with the internal lfsr_rattr_t type, we can even
pass these directly in the lfsr_rattr_t struct itself, so no need to
keep single le32/leb128 attrs on the stack:
buffer rattr: cat attr: le32/leb128 attr:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
| tag |0|size | | tag |1|count| | tag |0|dsize|
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
| weight | | weight | | weight |
+---+---+---+---+ .. +---+---+---+---+ .. +---+---+---+---+
| ptr -------. | ptr -------. | le32/leb128 |
'---+---+---+---' | '---+---+---+---' | '---+---+---+---'
.---+---+---+---. | .---+---+---+---. |
| data |<' |mm| size |<'
: : : +---+---+---+---+
| data |
+ +
| |
+---+---+---+---+
|mm| size |
: : :
While tinkering I also ended up renaming a couple things:
- rattr.cat -> rattr.u.datas, rattr.u.buffer, rattr.u.etc
- rattr.count -> rattr.data_count
- added lfsr_rattr_dtag for ignoring on-disk/explicit-data tags
- lfsr_rattr_size -> lfsr_rattr_dsize
Surprisingly very little code savings though. I guess we don't use
single le32/leb128 attrs enough to overcome the added complexity to
lfsr_rbyd_appendrattr_'s switch-case-table?
code stack ctx
before: 35636 2440 636
after: 35632 (-0.0%) 2440 (+0.0%) 636 (+0.0%)
That or there's something else weird going on with this union and
compiler assumptions. Attempting to adopt .u.etc in LFSR_RATTR__ alone
adds ~100 bytes of code, even though both .u.etc and .u.cat are the same
type (const void *)...
Not entirely sure what's going on...
- LFSR_TAG_BLOCK -+-> lfsr_data_frombptr
- LFSR_TAG_SHRUBBLOCK -'
This is where lazy attr encoding causes some problems for our shrub
estimate calculation. As a workaround, lfsr_rattr_t includes the
worst-case encoding size (LFSR_BPTR_DSIZE) in the otherwise-unused count
field, which avoids needing a second tag lookup.
This makes our shrub estimate a little bit worse, but is unavoidable
without reencoding bptr attrs.
On the bright side, this led to a bit of simplication in
lfsr_file_carve.
Saves more code and stack:
code stack ctx
before: 35848 2504 636
after: 35636 (-0.6%) 2440 (-2.6%) 636 (+0.0%)
The idea here is to move as much attr encoding logic as possible into
lfsr_rbyd_appendrattr_, so we don't encode most attrs until the last
minute, right before we write the tag+data to disk.
This has some pretty big theoretical benefits:
- Deduplicates encoding logic, so most attrs will only have a single
lfsr_data_from* call in the entire system.
This saves code size used for function calls, stack allocations, etc.
- In theory, _significantly_ better stack usage.
The main downside with eager encoding is that we need a buffer to
hold the encoding, and this buffer needs to stay allocated while all
of the commit machinery does its work.
This ends up stacking when any low-level attr buffers in
lfsr_btree_commit/lfsr_mdir_commit/etc, even though we don't _really_
need all of these attrs encoded at the same time.
Heck, we don't even need all of the attrs in the same _commit_ to be
encoded at the same time.
Lazily encoding avoids all of this.
- It's actually a nicer internal API, and means less risk we lose/
misallocate one of the encoding buffers.
The main downside is this makes attr encodings less gc-able. However, so
far it seems like you need most tags the moment you try to write to the
filesystem, and unwanted code costs can be worked around by allowing
more code to be conditionally compiled-out (at a testing cost).
This also means we don't know the actual on-disk attr size until we're
writing attrs out to disk. Fortunately, we've ended up relying on attr
size less than I thought we would. We still need it for shrub estimates,
but we can use the worst-case encoding size (LFSR_BPTR_DSIZE) there.
---
To start, this adopts lazy attr encoding for most of the obvious/
less-involved attrs:
- LFSR_TAG_BSHRUB ---> lfsr_data_fromshrub
- LFSR_TAG_BTREE -+-> lfsr_data_frombtree
- LFSR_TAG_MTREE -'
- LFSR_TAG_MROOT -+-> lfsr_data_frommptr
- LFSR_TAG_MDIR -'
- LFSR_TAG_ECKSUM ---> lfsr_data_fromecksum
Of interesting note is LFSR_TAG_BSHRUB. These changes actually make
shrub trunk encoding less of a special case, which _must_ be lazily
encoded due to last minute shrub changes caused by mdir compactions,
relocations, etc. This lets us drop the unique LFSR_TAG_SHRUBTRUNK
handling.
Though it does risk bugs if a future refactor ever reverts to eager
encoding... I've tried to highlight this with comments around
LFSR_TAG_BSHRUB's encoding.
These changes also required moving a significant number of the
LFSR_*_DSIZE macros around so they are declared before
lfsr_rbyd_appendrattr_. This is unfortunate as it moves them farther
away from from the related lfsr_data_from* implementations, but as far
as I'm aware there's no way around this.
We also need to _not_ lazily encode when an attr is in the concatenated-
data form (count < 0), or else this breaks mdir compaction. This has the
interesting side-effect of still allowing eager encoding with
LFSR_DATA_BUF, which, while less efficient, is very useful for our
tests.
---
So far, code/stack changes look promising:
code stack ctx
before: 36280 2576 636
after: 35848 (-1.2%) 2504 (-2.8%) 636 (+0.0%)
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%)
At the cost of a bit more frequent estimate scans.
Saves a tiny bit of code:
code stack ctx
before: 36300 2576 640
after: 36280 (-0.1%) 2576 (+0.0%) 640 (+0.0%)
May revisit this in the future, but this is the best solution I can
think of right now, that doesn't run into duplicate macro-argument
side-effect issues...
Statement expressions would be another solution, but that's even less
portable!
At the moment this is only used for functions that implement
LFSR_DATA_* and LFSR_RAT_* macros. These _need_ to be inlined to avoid a
large code-size explosion, and GCC seems to have issues with this.
For most of the other inlinable functions, relying on C99's inline +
compiler heuristics seems to be fine.
Code changes:
before: 36304 2576 640
no-forceinline: 36460 (+0.4%) 2664 (+3.4%) 640 (+0.0%)
yes-forceinline: 36300 (-0.0%) 2576 (+0.0%) 640 (+0.0%)
This breaks down the in-progress lfsr_bptr_t in lfsr_file_flush_ into
its components (block, off, eoff, cksum), until right before we write it
to disk.
This may seem counterproductive. Some paths, such as block appends,
start with an lfsr_bptr_t, but breaking it down this way avoids the
lfsr_bptr_t encoding noise and gives the compiler a better chance to
optimize things.
The end result is a nice bit of code/stack savings, and a slightly more
readable function:
code stack ctx
before: 36320 2584 640
after: 36304 (-0.0%) 2576 (-0.3%) 640 (+0.0%)
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%)
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 was missed in the previous fix for ckfetches+ckmeta (ab26437e).
lfsr_file_ck should be more-or-less the same as lfsr_mtree_traverse,
just limited to the current file.
Code changes:
code stack ctx
default before: 36492 2608 640
default after: 36492 (+0.0%) 2608 (+0.0%) 640 (+0.0%)
ckfetches before: 36724 2648 640
ckfetches after: 36716 (-0.0%) 2648 (+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.
We don't strictly need to initialize all of the shrub fields, but the
path of bnulls -> bshrubs -> btrees in the file logic is complex. It's
highly likely leaving these uninitialized will cause bugs in the future.
This does come with a small code cost, but better safe than sorry:
code stack ctx
before: 36436 2608 640
after: 36476 (+0.1%) 2608 (+0.0%) 640 (+0.0%)
We no longer use this, initialization is now implicit in
lfsr_bshrub_init. Keeping this around just risks the untested estimate
logic falling out-of-date.
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%)
Unfortunately, even with the mdir update order fixed, this assert still
ends up violated by lfsr_rename.
But only if lfsr_rename brings along open + unsynced shrubs.
The problem is we copy these shrubs over with LFSR_TAG_MOVE, but don't
update the related mdir until the epilogue in lfsr_rename. This avoids
needing to parse LFSR_TAG_MOVE tags in the epilogue in lfsr_mdir_commit,
but does mean we can't rely on this assert.
Oh well, we at least have the bshrub-in-mdir assert in
lfsr_mdir_commit's prologue. This at least gives us confidence that this
constraint is usually held, even if it makes debugging a pain-in-the-
ass.
Found by our test_forphans_mv test, which is working as intended.
No code changes.
Mainly to make our bshrub-in-mdir assert stricter (we need to update
mdirs first to assert bshrubs end up in the right mdirs), but also to
make lfsr_mdir_commit epilogue mirror the prologue a bit better. In this
function especially, readability is important.
And hey, code changes no worse for the wear:
code stack ctx
before: 36424 2608 640
after: 36432 (+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%)
Similar to msprouts/mshrubs, this drops all of the logic necessary for
reading and maintaining bmoss/bshrubs, while clearing/reserving the
LFSR_RCOMPAT_BMOSS/BSPROUT flags in case we want to re-explore these in
the future.
---
Wait, wait, dropping bmosses? Inlined files? Aren't inlined files pretty
fundamental to littlefs?
bmoss: bsprout: bshrub: btree:
.--------. .--------. .--------. .--------.
.| mdir | .| mdir | .| mdir | .| mdir |
|| name | || name | || name | || config |
|| data | || bptr | || bshrub -. || btree |
|| | || | | || data <' || | |
|'--------' |'---|----' |'--------' |'---|----'
'--------' '----|---' '--------' '----|---'
v v
.--------. .--------.
| data | | btree |
| | | data |
| | | |
| | | |
'--------' '--------'
Yep! And that's why they stuck around for so long. I never really
expected something to replace the simplicity of inlined files.
But bmoss/inlined files' simplicity is deceptive. They're actually a big
pain-in-the-ass when you realize:
1. littlefs's file snapshot semantics means you somehow need to keep
track of bmoss/inlined files that are no longer in the mdir tree.
2. You actually can't rely on bmoss/inlined files always fitting in RAM,
in cases where the filesystem is shared between different drivers
with different configurations.
This has been a pretty big pain point upstream, and results in tricky
edge cases that are difficult to test.
Bshrubs just sort of side-step these problems...
Well, we _do_ still need quite a bit of logic to keep track of detached
bshrubs across mdir compactions, but we'd need that logic for bshrubs
anyways. And why pay for two piles of logic if we can get away with one?
The only difference between inlined bmoss and inlined bshrubs is the
trunk pointer (<=9 bytes) and extra data tag (<=10 bytes), adding at
most 19 extra bytes per file.
But these extra 19 bytes per file save us a big chunk of code:
code stack ctx
before: 37652 2616 640
after: 36668 (-2.6%) 2616 (+0.0%) 640 (+0.0%)
---
There _is_ an argument for keeping bmoss around: In cases where you have
a shitton of tiny files, these extra 19 bytes may add up.
But I think this is a micro-optimization for a very specific use case
that is out-of-scope for littlefs. We should always trade disk usage for
code size when possible.
This is the first step towards dropping bmoss/bsprout support
completely: Changing our write strategy to no longer emit bmosses.
Now, small files are converted directly to inlined bshrubs, which are
not _that_ much more overhead.
The biggest savings are in lfsr_file_truncate/fruncate, where we no
longer have to worry about the edge cases around converting from
bshrub/btree -> bmoss. We can just rely on btrees naturally folding into
bshrubs when they get small enough.
This saves a nice chunk of code, but keeping in mind we're still lugging
most of the bmoss circuitry around in order to support reading bmosses:
code stack ctx
before: 38284 2624 640
after: 37652 (-1.7%) 2616 (-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%)
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%)
I mean, what does the single mptr representation get us anyways?
A slightly smaller filesystem when metadata doesn't _quite_ fit in the
mroot, but fits in a single mdir? The only difference between the two is
the filesystem config, which really shouldn't be _that_ large:
mmoss: msprout: mtree:
.--------. .--------. .--------.
.| mroot | .| mroot | .| mroot |
|| config | || config | || config |
|| dir / | || mptr | || mptr |
|'--------' |'---|----' |'---|----'
'--------' '----|---' '----|---'
v v
.--------. .--------.
.| mdir | | mtree |
|| mptr | | mptr |
|| | | | |
|'--------' '---|----'
'--------' v
.--------.
.| mdir |
|| mptr |
|| |
|'--------'
'--------'
Unlike the theoretical mshrub representation, which we've already given
up on, msprouts _don't_ require a significant code cost to implement.
But if they offer little value, why keep them around? Every code cost is
code cost.
---
This drops msprout support, saving a bit of code:
code stack ctx
before: 38508 2624 640
after: 38340 (-0.4%) 2624 (+0.0%) 640 (+0.0%)
This also clears the LFSR_RCOMPAT_MSPROUT flag, allowing for msprouts to
be possibly reintroduced in the future.
But I don't really see any motivation for msprouts, even if you don't
care about code size, so I suspect this will just be a dead bit from now
on...
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!
To help with debugging. These all seem useful, though the exact output
will probably be worth messing around with:
- LFS_DEBUGRBYDFETCHES - Debug every rbyd fetch
- LFS_DEBUGRBYDCOMMITS - Debug every rbyd commit
- LFS_DEBUGBTREEFETCHES - Debug every btree/bshrub fetch (though we
currently don't fetch bshrubs...)
- LFS_DEBUGBTREECOMMITS - Debug every btree/bshrub commit
- LFS_DEBUGMDIRFETCHES - Debug every mdir fetch
- LFS_DEBUGMDIRCOMMITS - Debug every mdir commit
- LFS_DEBUGALLOCS - Debug every block allocation
Let's see if you can match these to each debug output:
lfs.c:2942:debug: Fetched rbyd 0xe.d80 w77, eoff 3536, cksum 862283c6
lfs.c:4233:debug: Committed rbyd 0xe.dd0 w78, eoff 3616, cksum 38ae1347
lfs.c:4950:debug: Fetched btree 0x9f.806 w2048, cksum 7fb89b1b
lfs.c:6609:debug: Committed btree 0x9f.806 w2048, cksum 7fb89b1b
lfs.c:6603:debug: Committed bshrub 0x{0,1}.b06 w1747
lfs.c:7290:debug: Fetched mdir -1 0x{1,0}.8f w0, cksum 7846be7a
lfs.c:9022:debug: Committed mdir 0 0x{0,1}.a10 w2, cksum 4d2ccb29
lfs.c:10083:debug: Allocated block 0x8f, lookahead 125/253/256
Also tweaked LFSR_DEBUGRBYDBALANCE to be a bit more readable when
LFS_DEBUGRBYDFETCHES is enabled, and tweaked the out-of-space error
message to show the same lookahead info as LFS_DEBUGALLOCS:
lfs.c:10101:error: No more free space (lookahead 0/0/256)
^ ^ ^
lookahead remaining --' | |
ckpoint remaining ------' |
block count ---------------'
No code changes.
This adds LFS_INFO and limits LFS_DEBUG to opt-in debug output.
Note that while LFS_DEBUG seems to be only used in LFS_DEBUGRBYDBALANCE,
it can also be useful for adding additional logging while debugging.
Having separate levels here is useful for filtering. Users should be
able to expect LFS_INFO output to not bog down a system, while LFS_DEBUG
can be a free-for-all dumping ground of debug info.
This also converges to the common 4 levels of logging found in other
systems, which was an intentional non-goal, but it's interesting to see
how each level serves a purpose.
Now with both height (alt-height) and bheight (black-height):
lfs.c:2970:debug: rbyd 0x9.c05: height 6-11, bheight 6-6
It's interesting to note this highlighted a minor mistake in our assumed
rbyd bounds.
Originally, our rbyd algorithm generated trees with height bounded by
2*bheight+1, but now that we have range removals, our height can get up
to 2*bheight+2 due to how diverged paths are stitched together.
This name helps avoid the mistake the that LFS_ASSERTRBYDBALANCE is
cheap/free like other asserts, which is very much not true.
LFS_DEBUGRBYDBALANCE is expensive and should only be used for testing.
Aside from cleaning up the mess of debug statements/commented code, this
also includes a bit of fiddling with the append logic to try to make
things a bit more readable and minimize code cost:
code stack ctx
before: 38784 2624 640
after: 38548 (-0.6%) 2624 (+0.0%) 640 (+0.0%)
Now we can better compare before and after the balance rework:
code stack ctx
before rbyd-balance-rework: 38440 2624 640
after rbyd-balance-rework: 38548 (+0.3%) 2624 (+0.0%) 640 (+0.0%)
Though it's worth emphasizing that maintaining strictly balanced rbyds
is well worth the extra code cost, since it's sort of what the rest of
the filesystem is built on.
The issue with relying solely on the jump > branch hack to disambiguate
recolorable prunes is that suddenly we're back in ambiguous territory
when we encounter post-split yellow nodes. We need to prioritize yellow
prunes or else post-split weights can become ambiguous.
This was the whole reason we added a 3rd color!
Fortunately we can still prioritize yellow nodes by trying both
recolorable prunes first, and only falling back to non-recolorable
prunes if we still have unreachable alts.
Ends up with a bit of code duplication, but gets things working again.
---
Turns out this was all we needed to get our rbyd operations perfectly
balanced! Now all test_rbyd tests are passing:
test_rbyd+balance before: 1671/385878 failed
test_rbyd+balance after: 0/385878 failed (-100.0%)
And after running the full test suite, can confirm _all_ tests are
passing with LFS_ASSERTRBYDBALANCE. So I think we have some pretty
decent confidence our rbyd algorithm maintains balance, even with range
removals:
test+balance before: 20614/631541 failed
test+balance after: 0/631541 failed (-100.0%)
It was an open question if this was even possible, so it's nice to see
some evidence balanced range removals are not a problem.
---
The current implementation is a bit hacky, so we do take a hit to code
size. Though we may be able to claw this back after cleaning things up:
code stack ctx
before: 38580 2624 640
after: 38784 (+0.5%) 2624 (+0.0%) 640 (+0.0%)
Christopher Haster