May rerevert this in the future, but I'm on the fence.
It's true this only saves a small amount of code, but in theory it also
reduces stack consumption in name-related functions. Currently this
doesn't affect the stack hot-path, which is a bit surprising as this
includes lfs3_set, but it may in the future.
The arguments against this optimization are also a bit weak:
- Non-null-terminated strings - We probably shouldn't optimize for a
theoretical future feature. If anything, we want to optimize in the
opposite direction to best measure the theoretical code cost.
- Precomputing strlen early - While this is generally a good idea, our
rattrs benefit greatly from compact encodings, as rattrs sitting on
the stack are one of the bigger contributors to our stack hot-path.
So for now I'm unreverting to see how long this optimization makes
sense, but could see this being rereverted in the future.
At the very least we probably want to keep the test changes to make
future testing easier.
---
Saves a bit of code:
code stack ctx
before: 35188 2136 660
after: 35160 (-0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38048 2152 772
gbmap after: 38020 (-0.1%) 2152 (+0.0%) 772 (+0.0%)
As much as I don't want to admit it, our 3-word lfs3_data_t struct is
just too large to be treated as pass-by-value with today's compilers.
It's a real shame, because I don't think there's a great technical
reason, just that compiler's pass-by-value optimizations generally stop
after 2 words.
If we could expect 16-bit block sizes (off and size), we could fit in
2 words, but this is already challenged by today's NAND chips
(bs>=128KiB).
---
So, as a compromise, this stops treating lfs3_data_t as pass-by-value,
with the exception of the lfs3_data_from* functions that still return
lfs3_data_t directly.
So instead of:
lfs3_data_t data = lfs3_data_fromecksum(&ecksum, buffer);
data = lfs3_data_slice(data, 8, -1);
return lfs3_data_size(data);
Most operations take lfs3_data_t by pointer:
lfs3_data_t data = lfs3_data_fromecksum(&ecksum, buffer);
lfs3_data_slice(&data, 8, -1);
return lfs3_data_size(&data);
One of the main consequences is there are now several ways to slice data
(internally these all redirect to lfs3_data_slice), and LFS3_DATA_SLICE
will likely see more use since we need temporary allocations to pass the
data slice by address:
- lfs3_data_slice(data, a, b) - Slices the data in place
- lfs3_data_fromslice(data, a, b) - Returns a new data slice
- LFS3_DATA_SLICE(data, a, b) - Creates a new compound-literal slice
---
As a pragmatic compromise, this saves a nice chunk of both code and
stack:
code stack ctx
before: 35316 2176 660
after: 35188 (-0.4%) 2136 (-1.8%) 660 (+0.0%)
code stack ctx
gbmap before: 38172 2192 772
gbmap after: 38048 (-0.3%) 2152 (-1.8%) 772 (+0.0%)
I don't think there was anything inherently wrong with this idea, but:
- The code savings (28 bytes) was surprisingly small.
- Expecting lfs3_path_namelen may be a headache for future
non-null-terminated string support.
- Even if you don't care about non-null-terminated strings, precomputing
strlen as early as possible is a good idea to minimize repeated strlen
scans.
Reverting adds a bit of code:
code stack ctx
before: 35288 2176 660
after: 35316 (+0.1%) 2176 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38140 2192 772
gbmap after: 38168 (+0.1%) 2192 (+0.0%) 772 (+0.0%)
I was poking around at possibly inlining small (<=255) name lens in
lfs3_rattr_t, but realized all LFS3_FROM_NAME rattrs in our system
already use the lfs3_path_namelen pattern (terminates in either
'\0' or '/').
Well, except for our tests, but who cares about those.
Adopting lfs3_path_namelen in LFS3_FROM_NAME saves a bit of code:
code stack ctx
before: 35316 2176 660
after: 35288 (-0.1%) 2176 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38168 2192 772
gbmap after: 38140 (-0.1%) 2192 (+0.0%) 772 (+0.0%)
It's funny to see what originally started as a simple list of rbyd attrs
slowly morph into a full isa. But it makes sense. What we really want is
an abstract description of operations that can be played and replayed as
necessary to atomically update the mtree.
Using a fixed lfs3_rattr_t struct to represent this in C is easy, and
avoids strict-aliasing issues, but ultimately limited when it comes to
the wide-range of data we want to attach to attributes.
Unlike a computer's isa, we want to be able to include full 12-24 byte
branch pointers directly in the instruction!
---
So here's a full variable-length isa organized by words (max(uintptr_t,
uint32_t)).
The first 32-bit word extends the 16-bit tag with an extra 16-bits of
control information:
wwll llff ffcc cccc tttt tttt tttt tttt
^'-.-''-.-''--.--' : :
'--|----|-----|----:-----------------:-- compressed weight
:: '----|-----|----:-----------------:-- total len
:: '-----|----:-----------------:-- from encoder
:: '----:-----------------:-- optional count
:: rgmm kkkk -kkk kkkk
11 => w=-1 ^^ ^ '-.' '---.---'
00 => w=0 '|-|---|------|------ rm bit
01 => w=+1 '-|---|------|------ grow bit
10 => w=attached '---|------|------ mask bits
'------|------ tag suptype
'------ tag subtype
The 4-bit length field always encodes the full length of the
instruction, including the instruction itself and optional weight. The
4-bit from + 6-bit count fields operate independently and tell
lfs3_rbyd_appendrattr_ how to actually encode the data related to the
instruction.
To work around strict-aliasing issues, complex structs are expected to
be broken down into words and reconstructed in lfs3_rbyd_appendrattr_.
Most of our structs are organized into words anyways. For example:
// new child
*r++ = LFS3_RATTR(5, LFS3_TAG_BRANCH, -2, LFS3_FROM_BRANCH);
*r++ = LFS3_RATTR_WEIGHT(+child_->weight);
*r++ = LFS3_RATTR_ARG(child_->blocks[0]);
*r++ = LFS3_RATTR_ARG(child_->trunk);
*r++ = LFS3_RATTR_ARG(child_->cksum);
This also changes rattr-lists to be null-terminated, which makes a bit
more sense in a variable-length isa:
*r++ = LFS3_RATTR_NULL; // all zeros, including length
One concern with null-terminated rattr-lists is how easy it is to
forget the null-terminator, but an assert that all non-null rattrs have
non-zero length seemed to catch the many many mistakes during adoption.
Alternatively, separate LFS3_FROM_NULL/LFS3_FROM_NIL from fields could
be used if encoding space gets tight.
I'm also quite happy with the 2-bit weight feild, which allows omitting
the optional weight word for -1,0,+1 weights. These should cover at
least all mdir operations.
Note the exact encoding of the rattr fields is less of a concern than
the tag fields, as it doesn't reside on-disk can be changed on whim.
---
Saves a nice chunk of code and stack:
code stack ctx
before: 35920 2280 660
after: 35324 (-1.7%) 2176 (-4.6%) 660 (+0.0%)
code stack ctx
gbmap before: 38812 2296 772
gbmap after: 38156 (-1.7%) 2192 (-4.5%) 772 (+0.0%)
The stack savings are obvious, but the code savings a bit less so. A
variable length isa _is_ more complicated, but by limiting most encoding
decisions to compile-time (2-bit weights vs 32-bit weights for example),
the savings from fewer word manipulations on the stack wins.
This includes the mask/rm/grow bits:
- LFS3_tag_RM
- LFS3_tag_GROW
- LFS3_tag_MASK0/2/8/12
Our in-device only handle types:
- LFS3_tag_ORPHAN
- LFS3_tag_TRV
- LFS3_tag_UNKNOWN
And in-device only tags with special behavior:
- LFS3_tag_INTERNAL
- LFS3_tag_RATTRS
- LFS3_tag_SHRUBCOMMIT
- LFS3_tag_GRMPUSH
- LFS3_tag_MOVE
- LFS3_tag_ATTRS
Usually I'm not a big fan of case-sensitive naming patterns, but this
has been useful for self-documenting what compat flags are in-device
only. Might as well extend the idea to our tag definitions.
Brings back lfs3_btrv_t, but keeps some of the btree internal changes.
I think the biggest one is dropping the internal branch pointer, now
instead of internally pointing to the root rbyd, we just unconditionally
sync the rbyd state anytime the rbyd matches the root's weight. This is
necessary to avoid out-of-sync state when traversing bshrubs under
mutation.
Also after refactoring I think the current btree traversal logic is
easier to read.
---
This is in preparation for removing the leaf cache, or at least making
it opt-in.
It adds a chunk of stack, but in theory we can reclaim this by allowing
leaf caches to be disabled:
code stack ctx
before: 37160 2352 688
after: 37088 (-0.2%) 2384 (+1.4%) 688 (+0.0%)
Having gbmap/bmap used in different places for the same thing was
confusing. Preferring gbmap as it is consistent with other gstate (grm
queue, gcksums), even if it is a bit noisy.
It's interesting to note what didn't change:
- The BM* range tags: LFS3_TAG_BMFREE, etc. These already differs from
the GBMAP* prefix enough, and adopting GBM* would risk confusion for
actual gstate.
- The gbmap revdbg string: "bb~r". We don't have enough characters for
anything else!
- dbgbmap.py/dbgbmapsvg.py. These aren't actually related to the gbmap,
so the name difference is a good thing.
There's really no reason to immediately commit the bmap to disk, at
least no until the first mdir commit, when we need to at least discard
the previous bmap state.
We already do all the gstate handling in lfs3_mdir_commit anyways, and
piggybacking on mdir commit lets us get rid of the annoying extra mdir
param in lfs3_alloc_ckpoint.
This does mean a slightly higher risk of needing to re-rebuild the bmap
after a powerloss, but in theory only if the user does something weird
like writing to a file and never calling sync. Most on-disk operations
terminate in an mdir commit as that's how any state change becomes
atomically visibile in littlefs.
Saves a nice bit of stack:
code stack ctx
before: 36920 2368 684
after: 36920 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38552 2472 812
bmap after: 38464 (-0.2%) 2400 (-2.9%) 812 (+0.0%)
At least at a proof-of-concept level, there's still a lot of cleanup
needed.
To make things work, lfs3_alloc_ckpoint now takes an mdir, which
provides the target for gbmap gstate updates.
When the bmap is close to empty (configurable via bmap_scan_thresh), we
opportunistically rebuild it during lfs3_alloc_ckpoints. The nice thing
about lfs3_alloc_ckpoint is we know the state of all in-flight blocks,
so rebuilding the bmap just requires traversing the filesystem + in-RAM
state.
We might still fall back to the lookahead buffer, but in theory a well
tuned bmap_scan_thresh can prevent this from becoming a bottleneck (at
the cost of more frequent bmap rebuilds).
---
This is also probably a good time to resume measuring code/ram costs,
though it's worth repeating the above note about the bmap work still
needing cleanup:
code stack ctx
before: 36840 2368 684
after: 36920 (+0.2%) 2368 (+0.0%) 684 (+0.0%)
Haha, no, the bmap isn't basically free, it's just an opt-in features.
With -DLFS3_YES_BMAP=1:
code stack ctx
no bmap: 36920 2368 684
yes bmap: 38552 (+4.4%) 2472 (+4.4%) 812 (+18.7%)
This doesn't fully replace the lookahead buffer, but at least augments
it with known bmap state when available.
To be honest, this is a minimal effort hack to try to get something
benchmarkable without dealing with all the catch-22 issues that a
self-support bmap allocator would encounter (allocating blocks for the
bmap requires a bmap, oh no).
Though now that I'm writing this, maybe this is a reasonable long-term
solution? Having the lookahead buffer to fall back on solves a lot of
problems, and, realistically, it's unlikely to be a performance
bottleneck unless the user has extreme write requests (>available
storage?).
---
Also tweaked field naming to be consistent between the bmap and
lookahead buffer.
This comes from an observation that we never actually use the leaf cache
during traversals, and there is surprisingly little risk of a lookup
creating a conflict in the future.
Btree traversal fall into two categories:
1. Full traversals, where we traverse a full btree all at once. These
are unlikely to have lookup conflicts because everything is
usually self-contained in one chunk of logic.
2. Incremental traversals. These _are_ at risk, but in our current
design limited to lfs3_trv_t, which already creates a fully
bshrub/btree copy for tracking purposes.
This copy unintentionally, but conveniently, protects against lookup
conflicts.
So, why not reuse the btree leaf cache to hold the rbyd state during
traversals? In theory this makes lfs3_btree_traverse the same cost and
lfs3_btree_lookupnext, drops the need for lfs3_btrv_t, and simplifies
the internal API.
The only extra bit of state we need is the current target bid, which is
now expected as a caller-incremented argument similar to
lfs3_btree_lookupnext iteration.
There was a bit of futzing around with bid=-1 being necessary to
initialize traversal (to avoid conflicts with bid=-1 => 0 caused by
empty btrees). But the end result is a btree traversal that only needs
one extra word of state.
---
Unfortunately, in practice, the savings were not as great as expected:
code stack ctx
before: 36792 2400 684
after: 36876 (+0.2%) 2384 (-0.7%) 684 (+0.0%)
This does claw back some stack, but less than a full rbyd due to the
union with the mtortoise in lfs3_trv_t. The mtortoise now dominates. It
might be possible to union the mtortoise and the bshrub/btree state
better (both are not needed at the same time), but strict aliasing rules
in C make this tricky.
The new lfs3_btree_traverse is also a bit more complicated in terms of
code cost. In theory this would be offset by the simpler traversal setup
logic, but we only actually call lfs3_btree_traverse twice:
1. In lfs3_mtree_traverse
2. In lfs3_file_ck
Still, some stack savings + a simpler internal API makes this worthwhile
for now. lfs3_trv_t is also due for a revisit, and hopefully it's
possible to better union things with btree leaf caches somehow.
This is an indulgence to simplify the upcoming auxiliary btree work.
Brings back the previously-reverted per-btree leaf caches, where each
lfs3_btree_t keeps track of two rbyds: The root and the most recently
accessed leaf.
At the surface level, this optimizes repeated access to the same btree
leaf. A common pattern for a number of littlefs's operations that has
proven tricky to manually optimize:
- Btree iteration
- Pokes for our crystalization heuristic
- Checksum collision resolution for dids and (FUTURE) ddkeys
- Related rattrs attached to a single bid
But the real motivation is to drop lfs3_btree_*lookupleaf and simplify
the internal APIs. If repeated lfs3_btree_lookup*s are already
efficient, there's no reason for extra leaf-level APIs, and in theory
any logic that interacts with btrees will be simpler.
---
This comes at a cost (humorously about the same amount as the
tag-returning refactor, if you ignore the extra 28 bytes of ctx).
Unsurprisingly, increasing the size of lfs3_btree_t has the biggest
impact on stack and ctx:
code stack ctx
before: 36084 2336 656
after: 36784 (+1.9%) 2400 (+2.7%) 684 (+4.3%)
Also note from the previous commit messages: Btree leaf caching has
resulted in surprisingly little performance improvement for our current
benchmarks + implementation. It turns out if you're dominated by write
cost, optimizing btree lookups -- which already skip rbyd fetches, has
barely noticeable impact.
---
A note on reverting!
Eventually (after the auxiliary btree work) it will probably make sense
to revert this -- or at least provide a non-leaf-caching build for
code/RAM sensitive users.
I don't think this should be reverted as-is. Instead, I think we should
allow the option to just disable the leaf cache, while keeping the
simpler internal API. This would give us the best of all three worlds:
- A small code/RAM option
- Optimal btree iteration/nearby-lookup performance
- Simpler internal APIs
The only reason this isn't already implemented is because I want to
avoid fragmenting the codebase further while we're still in development
mode.
- test_traversal -> test_trvs
- lfs3_traversal_t -> lfs3_trv_t
- lfs3_btraversal_t -> lfs3_btrv_t
- t -> trv
- bt -> btrv
- lfs3_traversal_* -> lfs3_trv_*
- lfs3_btraversal_* -> lfs3_btrv_*
The traversal type is becoming one of the more fundamental types in
littlefs, and if DIR and REG both get shortened names, it makes sense
for TRV to have one as well.
This also removes the temptation to use t for traversals, which is
probably an even worse name.
---
Note that lfs3_btree_traverse, lfs3_mtree_traverse, etc, remain
unaffected. This may change in the future, but it's interesting to note
that verbs seem to need much less typing than nouns.
- lfs3_btree_lookupleaf
- lfs3_btree_lookupnext
- lfs3_btree_lookup
- lfs3_btree_traverse
- NOT lfs3_btree_namelookup
Looks like we're starting to claw back stack usage a bit. This makes
sense as the btree logic involves the most layers -- with out-pointers
it needs more temporary copies to inspect tags along the way:
code stack ctx
before: 36576 2376 656
after: 36520 (-0.2%) 2352 (-1.0%) 656 (+0.0%)
This is the start of a big refactor to try to move tag out-pointers into
the return position of functions, muxing with error codes via the
sign-bit when necessary.
So instead of:
lfs3_tag_t tag_;
lfs3_data_t data_;
int err = lfs3_rbyd_lookup(&lfs3, &rbyd, rid, tag,
&tag_, &data_);
if (err) {
return err;
}
We now do:
lfs3_data_t data_;
lfs3_stag_t tag_ = lfs3_rbyd_lookup(&lfs3, &rbyd, rid, tag,
&data_);
if (tag_ < 0) {
return tag_;
}
In theory, removing an out-pointer saves both code and stack, though it
will be interesting to actually see how much of an affect this has after
the dust has settled.
littlefs v2 used this technique heavily for its 32-bit tags, but we
never did a comparison with/without tags in the return position.
This is a big rewrite in the test code, so hopefully this ends up worth
it :)
Lots of regex.
Note this implicitly limits error codes to 16-bits, but supported error
codes are already a bit limited because we're using int everywhere
(instead of int32_t). If we need 32-bit error codes we can always add
another type to represent the mux in the future (lfs3_etag_t?).
---
So far the code savings look promising:
code stack ctx
before: 36828 2368 656
after: 36576 (-0.7%) 2376 (+0.3%) 656 (+0.0%)
Stack usage is a big disappointing, but hopefully that is just a
temporary cost due to the internal scaffolding between different API
types while the refactor is ongoing.
This breaks down the previously 16-bit rattr.count field into two 8-bit
rattr.from and rattr.count fields. Now, instead of using a mixture of
rattr.tag and sign(rattr.count) to determine rattr encoding, we just
jump based on rattr.from:
lfs3_rattr_t:
.---+---+---+---.
| tag |frm|cnt| -+-> 16-bit tag - on-disk encoding + rbyd flags
+---+---+---+---+ +-> 8-bit from - in-RAM encoding
| weight | '-> 8-bit count - from-specific count
+---+---+---+---+
| ptr |
'---+---+---+---'
The internal appendrattr_ ctx also saw a bit of rework, and now uses a
big union with multiple buffers instead of stacking a ridiculous number
of LFS_MAX calls. Expanding the LFS_MAX stack grows O(n^2), so this is
probably good for compile times.
And all rattr.from branches now generate an lfs3_data_t*. This was
already a side-effect of all the internal lfs3_data_from* functions, and
it simplifies the tail end of appendrattr_. No more relying on
data_count's sign bit.
Also rearranged rattr.from encoders to match source code order.
---
Unfortunately, while this did simplify the source code, it didn't really
lead to much improvement in code size:
code stack ctx
before: 37024 2416 652
after: 37016 (-0.0%) 2416 (+0.0%) 652 (+0.0%)
I guess jump tables are more a performance optimization than a code size
one. That and the benefit of cheaper appendrattr_ logic is likely
overshadowed by the extra constants needed to populate rattr.from in
every LFS3_RATTR_* macro.
Also test_attrs_fattr_resync_receive is now failing, but I think that's
just because of an unrelated bug exposed by the shrinking count field.
In theory rattr.count should be limited to internal fixed-size buffers.
Not sure why we weren't already, it doesn't really make sense to return
bid without weight, and this matches lfs3_btree/bshrub_lookupnext.
Sure we don't need weight currently, but this is useful to include in
case we need it in the future (lfs3_bptr_fetch during traversal?).
And while we're not using it, the compiler is happy to optimize it out,
so no code changes:
code stack ctx
before: 37964 2424 636
after 37964 (+0.0%) 2424 (+0.0%) 636 (+0.0%)
See the relevant commit for why. These just added surprisingly little
performance benefit for the code/stack cost.
Maybe in a future performance-preferring littlefs driver.
The idea here, is we give each lfsr_btree_t an optional leaf rbyd, in
addition to the root rbyd. This leaf rbyd acts as a cache for the most
recent leaf, allowing nearby btree lookups to skip the full btree walk.
Unfortunately, this failed on pretty much every measurable metric...
---
The motivation for this is that we often do a bunch of nearby btree
lookups:
- Btree iteration via lfsr_btree_lookupnext is a bit naive, walking from
the root every step.
- Our crystallization algorithm requires a bunch of nearby lookups to
figure out our crystallization heuristic. Currently at most 4, when
you need to lookup both crystal neighbors and then _also_ both
fragment neighbors for coalescing.
- Checksum collision resolution for dids and (FUTURE) ddkeys can require
an unbounded number of sequential lookups.
Though to be fair, this is an exceptional case if our checksum is any
good.
- Bids with multiple rattrs require nearby lookups to resolve.
Though currently this can be explicitly avoided via
lfsr_btree_lookupleaf + lfsr_rbyd_lookup.
The theory was that cases like these could explicitly keep track of the
leaf rbyd to avoid full btree walks, but in practice this never really
worked out. Tracking if we're still in the relevant leaf rbyd just adds
too much logic/code cost.
But if this leaf tracking logic was implemented once in the btree
layer...
The other theoretical benefit was being able to move more rbyds off the
stack. Sure our btrees take up more RAM, but if that results in stack
savings, that may be a win.
Oh, and this would let our btree API and rbyd API converge without
performance concerns. Internal users could in theory call
lfsr_btree_lookupnext + lfsr_btree_lookup with the same performance as
explicitly tracking the rbyd.
---
But this was a complete failure!
First the good news: There was a modest speedup of around ~2x to linear
reads.
And that's the good news.
Now the bad news:
1. There was no noticeable performance gain in any other benchmarks.
To be fair, we're at the early stages of benchmarking, so the
benchmarks may not be the most thorough, but thinking about it, there
are some explanations:
- In any benchmark that writes, fetch + erase + prog dominates. Being
able to skip fetches during lookups makes our btree lookups
surprisingly cheap!
- Any random read heavy benchmark is likely thrashing this cache,
which is to be expected.
- For small 1-block btrees, the leaf cache is useless because the
entire btree is cache in the root rbyd.
And keep in mind, our blocks are BIG. "Small" here could be on
the order of ~128KiB-1MiB for NAND flash.
- For the mtree, fetched mdirs actually already act as a sort of leaf
cache.
The extra btree leaf cache isn't doing _nothing_, but each layer of
the mtree has diminishing returns due to btree's ridiculous
branching factor.
- For file btrees, we're explicitly caching the leaf fragments/
blocks, so the extra btree leaf cache has diminishing returns for
the same reason.
2. Code cost was bad, stack cost was worse:
code stack ctx
before: 37172 2288 636
after: 38068 (+2.4%) 2416 (+5.6%) 664 (+4.4%)
Tracking the leaf required more code, that's expected. And, to be
fair, the current code has had a lot more time to congeal.
What wasn't expected was the stack cost.
Unfortunately these caches didn't really take any rbyds off the stack
hot-path:
- We _can_ get rid of the rbyd in lfsr_btree_lookup/namelookup, but
we were already hacking our way around the critical one in
lfsr_mtree_lookup/namelookup by reusing the mdir's rbyd!
- We can't even abuse the leaf rbyd in the commit logic, since the
target btree can end up iterated/traversed by lfs_alloc.
That was a fun bug.
And the addition of a second rbyd to lfsr_btree_t increases both ctx
and stack anywhere btrees are allocated.
Maybe this will make more sense when we add the auxiliary btrees, or
after more benchmarking, but for now the theoretical performance
improvements just aren't worth it.
Will probably revert this, but I wanted to commit it in case the idea is
worth resurrecting in the future, if in the future nearby btree lookups
are a bigger penalty than they are now.
This makes all rbyd_/mdir_ out-pointers required, dropping all of the
internal copies needed to make lookup/namelookup/pathlookup/etc work.
Previously, the -- rough -- rule was to make out-pointers generally
optional (lfsr_data_read and other struct initers being notable
exceptions), the idea being you can opt-out of stack allocations where
possible.
In practice this kind of backfired, with many internal functions needing
redundant stack allocations in case the relevant parameter is NULL
(lfsr_btree_lookupleaf being an excellent example).
---
As an alternative rule, I think we should only expect optional
out-pointers for things you would pass-by-value (lfsr_rid_t, lfsr_tag_t,
lfsr_data_t, etc).
I've also developed a habit of naming optional out-pointers with a
trailing underscore_, to hopefully make this subtlety a bit less subtle.
This claws back all of the stack cost of BNAMEs/MNAMEs, and most of the
code cost:
code stack ctx
before: 35888 2480 640
after: 35780 (-0.3%) 2408 (-2.9%) 640 (+0.0%)
Though we still have more function calls than we started with
(lfsr_mtree_*lookup mtree -> mdir lookups).
This is the _nth_ time I've tried to force arbitrary btree name inserts
to work, so _clearly_ I need a bigger comment.
Hopefully this will prevent me from trying to delete the LFSR_RATTR_NOOP
in test_btree_find_general_fuzz _again_.
---
The gist is that insert-before-bid+1 is fundamentally different from
insert-after-bid when named btrees are involved:
.-----f-----. insert-after-d .-------f-----.
.-b--. .--j-. => .-b---. .--j-.
| .-. .-. | | .---. .-. |
a c d h i k a c d e h i k
^
insert-before-h
=> .-----f-------.
.-b--. .---j-.
| .-. .---. |
a c d g h i k
^
The problem is that lfsr_btree_commit_ needs to find the same leaf
rbyd as lfsr_btree_namelookup, and potentially insert-before the
first rid or insert-after the last rid.
Instead of separate insert-before/after flags, we make the first tag
in a commit insert-before, and all following non-grow tags
insert-after (splits).
This info is now captured in the above mentioned comment.
Now that we don't have to worry about name tag conflicts as much, we
can add name tags for things that aren't files.
This adds LFSR_TAG_BNAME for branch names, and LFSR_TAG_MNAME for mtree
names. Note that the upper 4 bits of the subtype match LFSR_TAG_BRANCH
and LFSR_TAG_MDIR respectively:
LFSR_TAG_BNAME 0x0200 v--- --1- ---- ----
LFSR_TAG_MNAME 0x0220 v--- --1- --1- ----
LFSR_TAG_BRANCH 0x030r v--- --11 ---- --rr
LFSR_TAG_MDIR 0x0324 v--- --11 --1- -1rr
The encoding is somewhat arbitrary, but I figured reserving ~31 types
for files is probably going to be plenty for littlefs. POSIX seems to
do just fine with only ~7 all these years, and I think custom attributes
will be more enticing for "niche" file types (symlinks, compressed
files, etc), given the easy backwards compatibility.
---
In addition to the debugging benefits, the new name tags let us stop
btree lookups on the first non-bname/branch tag. Previously we always
had to fetch the first struct tag as well to check if it was a branch.
In theory this saves one rbyd lookup, but in practice it's a bit muddy.
The problem is that there's two ways to use named btrees:
1. As buckets: mtree -> mdir -> mid
2. As a table: ddtree -> ddid
The only named btree we _currently_ have is the mtree. And the mtree
operates in bucket mode, with each mdir acting more-or-less as an
extension to the btree. So we end up needing to do the second tag lookup
anyways, and all we've done is complicated up the code.
But we will _eventually_ need the table mode for the ddtree, where we
care if the ddname is an exact match.
And returning the first tag is arguably the more "correct" internal API,
vs arbitrarily the first struct tag.
But then again this change is pretty pricey...
code stack ctx
before: 35732 2440 640
after: 35888 (+0.4%) 2480 (+1.6%) 640 (+0.0%)
---
It's worth noting the new BNAME/MNAME tags don't _require_ the btree
lookup changes (which is why we can get away with not touching the dbg
scripts). The previous algorithm of always checking for branch tags
still works.
Maybe there's an argument for conditionally using the previous API when
compiling without the ddtree, but that sounds horrendously messy...
This lets us cram in one more mask for potential redund bits:
name tag mask
LFSR_TAG_MASK0 0x0000 0x0fff ---- 1111 1111 1111
LFSR_TAG_MASK2 0x1000 0x0ffc ---- 1111 1111 11--
LFSR_TAG_MASK8 0x2000 0x0f00 ---- 1111 ---- ----
LFSR_TAG_MASK12 0x3000 0x0000 ---- ---- ---- ----
'.-' '.-' '---.---'
mode bits -' | | ^
suptype ------' | |
subtype --------------' |
redund bits ------------------'
I toyed around with a bitwise alternative to the lookup table, but
couldn't come up with anything simpler than these:
- 0xfff & ~((((1<<((i>>1)*8))-1) << ((i&1)*4)) | ((1<<(i*2))-1))
- 0xfff & ~((1 << (((i>>1)*8)+((i&1)<<(1+(i>>1)))))-1)
- 0xfff & ~((1<<(2*i*i))-1) (requires multiply and 32-bit shift)
---
This also replaces the mdir/rbyd/btree/mtree lookup/sublookup/suplookup
functions with a single flexible lookup function that accepts tag masks.
This ended up adding a bit of code/stack (the extra NULL args are
surprisingly pricey), but will hopefully make the redund bits
easier/cheaper to use:
code stack ctx
before: 35548 2472 636
after: 35584 (+0.1%) 2480 (+0.3%) 636 (+0.0%)
This was a surprising side-effect the script rework: Realizing the
internal btree/rbyd lookup APIs were awkwardly inconsistent and could be
improved with a couple tweaks:
- Adopted lookupleaf name for functions that return leaf rbyds/mdirs.
There's an argument this should be called lookupnextleaf, since it
returns the next bid, unlike lookup, but I'm going to ignore that
argument because:
1. A non-next lookupleaf doesn't really make sense for trees where
you don't have to fetch the leaf (the mtree)
2. It would be a bit too verbose
- Adopted commitleaf name for functions that accept leaf rbyds.
This makes the lfsr_bshrub_commit -> lfsr_btree_commit__ mess a bit
more readable.
- Strictly limited lookup and lookupnext to return rattrs, even in
complex trees like the mtree.
Most use cases will probably stick to the lookupleaf variants, but at
least the behavior will be consistent.
- Strictly limited lookup to expect a known bid/rid.
This only really matters for lfsr_btree/bshrub_lookup, which as a
quirk of their implementation _can_ lookup both bid + rattr at the
same time. But I don't think we'll need this functionality, and
limited the behavior may allow for future optimizations.
Note there is no lfsr_file_lookup. File btrees currently only ever
have a single leaf rattr, so this API doesn't really make sense.
Internal API changes:
- lfsr_btree_lookupnext_ -> lfsr_btree_lookupleaf
- lfsr_btree_lookupnext -> lfsr_btree_lookupnext
- lfsr_btree_lookup -> lfsr_btree_lookup
- added lfsr_btree_namelookupleaf
- lfsr_btree_namelookup -> lfsr_btree_namelookup
- lfsr_btree_commit__ -> lfsr_btree_commit_
- lfsr_btree_commit_ -> lfsr_btree_commitleaf
- lfsr_btree_commit -> lfsr_btree_commit
- added lfsr_bshrub_lookupleaf
- lfsr_bshrub_lookupnext -> lfsr_bshrub_lookupnext
- lfsr_bshrub_lookup -> lfsr_bshrub_lookup
- lfsr_bshrub_commit_ -> lfsr_bshrub_commitleaf
- lfsr_bshrub_commit -> lfsr_bshrub_commit
- lfsr_mtree_lookup -> lfsr_mtree_lookupleaf
- added lfsr_mtree_lookupnext
- added lfsr_mtree_lookup
- added lfsr_mtree_namelookupleaf
- lfsr_mtree_namelookup -> lfsr_mtree_namelookup
- added lfsr_file_lookupleaf
- lfsr_file_lookupnext -> lfsr_file_lookupnext
- added lfsr_file_commitleaf
- lfsr_file_commit -> lfsr_file_commit
Also added lookupnext to Mdir/Mtree in the dbg scripts.
Unfortunately this did add both code and stack, but only because of the
optional mdir returns in the mtree lookups:
code stack ctx
before: 35520 2440 636
after: 35548 (+0.1%) 2472 (+1.3%) 636 (+0.0%)
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.
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.
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%)
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 they are a bit more annoying to call, init functions give the
compiler a chance to deduplicate common struct initialization logic. So
we should probably prefer init functions for any structs larger than a
couple words.
The cost of each init is small, but it really adds up!
code stack ctx
before: 38036 2608 752
after: 37844 (-0.5%) 2608 (+0.0%) 752 (+0.0%)
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.)
The main reason for this change is to allow keeping track of existing
known-free blocks while trying to find more free blocks. This makes it
so failed filesystem traversals don't result in negative progress, which
is nice.
This was difficult in the previous lookahead scheme, since we we'd need
to shift the lookahead buffer to keep off=0 rooted at the first bit.
Shifting bytes is relatively easily with memmove, but it gets tricky
when shifting bits:
lookahead before: ???? ???? ???? ??00 1101 0101 00?? ????
^ ^
off off+size
shift: 0011 0101 0100 ???? ???? ???? ???? ????
^ ^
off off+size
traverse: 0011 0101 0100 0000 0000 0000 1100 0000
^ ^
off off+size
Instead, we now just let the lookahead buffer wrap around. No shifting
required:
lookahead before: ???? ???? ???? ??00 1101 0101 00?? ????
^ ^
off off+size
traverse: 0000 0000 1100 0000 1101 0101 0000 0000
^
off
^
off+size
This gets a bit confusing with the lookahead window also wrapping around
disk, but the math works out with enough modulos (if modulos are too
expensive, we should eventually be able to optimize these into simple
bit masks via compile-time config).
In the future, if we move away from the const config struct, it would
also be nice to try to reducing the number of modulos by storing the
lookahead buffer size in bits instead of bytes...
Note that if the lookahead buffer is larger than disk, the lookahead
window will sort of travel around the underlying buffer. This isn't
inherently a problem, but it did cause some bugs.
To avoid similar bit-related problems with zeroing, lfs_alloc_inc now
also zeros bits as we allocate/skip them, so bits should always be zero
when we start a lookahead traversal. Though note we still need to
manually memset the buffer when discarding lookahead state in init/grow.
---
The end result is surprisingly a net savings in terms of code size. I
guess mainly due to dropping all the lfs_alloc_shift calls:
code stack
before: 36472 2680
after: 36412 (-0.2%) 2680 (+0.0%)
This has been a long-time coming, mount flags are just too useful for
configuring a filesystem at runtime.
Currently this is limited to LFS_M_RDONLY and LFS_M_CKPROGS, but there
are a few more planned in the future:
LFS_M_RDWR = 0x0000, // Mount the filesystem as read and write
LFS_M_RDONLY = 0x0001, // Mount the filesystem as readonly
LFS_M_STRICT* = 0x0002, // Error if on-disk config does not match
LFS_M_FORCE* = 0x0004, // Ignore compat flags, mount readonly
LFS_M_FORCEWITHRECKLESSABANDON*
= 0x0008, // Ignore compat flags, mount read write
LFS_M_CKPROGS = 0x0010, // Check progs by reading back progged data
LFS_M_CKREADS* = 0x0020, // Check reads via checksums
* Hypothetical
As a convenience, we also return mount flags in the struct lfs_fsinfo's
flags field as their relevant LFS_I_* variants. Though only to match
statvfs, and only because it's cheap, littlefs's API is low-level and we
should expect users to know what flags they passed to lfsr_mount.
As for the new mount flags:
- LFS_M_RDONLY - For consistency with existing APIs, this just asserts
on write operations, which makes it a bit useless... But the info flag
LFS_I_RDONLY may be useful for falling back to a readonly mode if
we encounter on-disk compat issues.
At least if implement the theoretical LFS_UNTRUSTED_USER mode
LFS_M_RDONLY could become a runtime error.
- LFS_M_RDWR - This really just exists to compliment LFS_M_RDONLY and to
match LFS_O_RDONLY/LFS_O_RDWR. It's just an alias for 0, and I don't
think there will ever be a reason to make it non-0 (but I can always
be wrong!).
- LFS_M_CKPROGS - This replaces the check_progs config option and avoids
using a full byte to store a bool.
We should probably also have a compile-time option to compile this out
(LFS_NO_CKPROGS?), but that's a future thing to do.
This ended up adding a surprising bit of code, considering we're just
moving flags around, and noise in lfs_alloc added a bit of stack again:
code stack
before: 35880 2672
after: 35932 (+0.1%) 2680 (+0.3%)
It's probably a bad reason, but this avoids wasting too much time
figuring out how to name things.
Now most traversal functions return an lfsr_tag_t + lfsr_bptr_t pair,
which is enough to describe the current relevant traversal objects:
tag=LFSR_TAG_MDIR => (lfsr_mdir_t*)bptr.data.u.buffer
tag=LFSR_TAG_BRANCH => (lfsr_rbyd_t*)bptr.data.u.buffer
tag=LFSR_TAG_DATA => bptr.data
tag=LFSR_TAG_BPTR => bptr
This would be a bit better if lfsr_data_t's buffer field was a void*,
but that would mess with byte-level arithmetic, which is more common
with lfsr_data_ts.
This also adopts the fragmented/optional out-params used elsewhere in
the codebase. I thought this would add quite a bit more stack cost,
since we need redundant tags/bptrs to make lfsr_mtree_traverse/
lfsr_mtree_gc work, but surprisingly not:
code stack
before: 35256 2680
after: 35228 (-0.1%) 2680 (+0.0%)
It seems we make up the extra stack cost of redundant tags/bptrs by
giving the compiler more stack-alloc flexibility, tighter per-function
return types, and opting-out of tags/bptrs in most low-level traversals:
lfs_alloc mainly.
But if the fragmented/optional out-params is net harmful for code/stack
size, we should reconsider the pattern system-wide. This does probably
deserve a second look in the future...
This solves the issue of multiple mdirs/rbyds in lfsr_mtree_gc, where
it's easy for traversal state to fall out of sync when mutating parts of
the filesystem.
Is it good design, with self-referential pointers making everything more
entangled? Not sure!
This saves a bit of stack, but adds a bit of code, which makes sense,
pointer chasing can be costly. But both of these changes are well below
the compiler noise floor:
code stack
before: 35228 2688
after: 35256 (+0.1%) 2680 (-0.3%)
This splits LFSR_TSTATE_BTREE into separate LFSR_TSTATE_MTREE/BTREE/
OBTREE states that indicate what to do next after traversing the btree.
This removes the need to point indirectly to file's o.next pointer,
since we can just point to the file struct itself.
I've also simplified opened-file clobbering to just move to the next
opened mdir, instead of searching for another unsynced file. This
simplifies things but does mean we now need to clobber traversals when
closing non-file objects. Implicitly calling lfsr_opened_clobber in
lfsr_opened_remove solves this with very little extra code cost,
deduplicated, and gives us a stronger invariant for traversal references
to closed objects. So win win?
Oh, and all the explicit open-file clobber checks are now deduplicated
into lfsr_opened_clobber again.
These tweaks save quite a bit of code:
code stack
before: 34740 2624
after: 34570 (-0.5%) 2624 (+0.0%)
The traversal logic is a bit simpler if everything can pass around/
populate the same struct, so this reverts some changes made when
implementing lfsr_traversal_t, bringing back bid as a side-channel and
making btinfo/mtinfo typedef aliases.
btinfo/mtinfo are also required arguments for lfsr_btree_traverse/
lfsr_fs_traverse now, so it's even easier to forward these to lower
layers if they alias.
What return-pointers should/shouldn't be optional is still an open
question, but at least for btinfo/mtinfo matching lfs_stat makes sense.
This saves a bit of code/stack:
code stack
before: 34474 2552
after: 34454 (-0.0%) 2544 (-0.3%)
This adds the lfsr_traversal_t object, which encapsulates a traversal
over all blocks in the filesystem.
This replaces the earlier lfs_fs_traverse function, but is sort of
"inside-out" in that instead of taking a callback, an lfsr_traversal_t
object can be read from to return lfs_tinfo structs that describe the
blocks in our system:
lfsr_traversal_open(&lfs, &t) => 0;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_MDIR;
tinfo.block => 0x0;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_MDIR;
tinfo.block => 0x1;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_DATA;
tinfo.block => 0x42;
lfsr_traversal_read(&lfs, &t, &tinfo) => LFS_ERR_NOENT;
lfsr_traversal_close(&lfs, &t) => 0;
This is more flexible, allowing for aborted traversals, yielding,
rewinding, etc, but also more complicated to implement, since it
requires all traversal state to be stored explicitly.
Fortunately, since we needed to reimplement filesystem traversals
anyways, I was able to build this into the new system from the start
using a small state machine to drive the traversal internally. So all
that was really needed was a bit of window dressing, adding
LFS_TYPE_TRAVERSAL to track open traversals, logic to handle
invalidating traversals on file close, mutation, etc...
Which, uh, that last one is not implemented yet. Interactions with other
filesystem operations gets messy, so I figured I'd go ahead and commit
what is currently working.
Ugh, and tests. The biggest downside of adding lfsr_traversal_t is how
many more corner-cases it adds to the system...
lfsr_traversal_t is going to be a work-in-progress for a bit...
---
lfsr_traversal_t also adds a really interesting path towards more access
to advanced low-level operations, such as checking metadata/data
checksums, incrementally progressing the garbage collector, even
repairing bad metadata/data blocks eventually.
Currently implemented is LFS_T_CKMETADATA and LFS_T_CKDATA to check
metadata and data checksums respectively. This is the first feature that
actually allows you to validate data checksums.
Code changes so far:
code stack
before: 33886 2560
after: 34226 (+1.0%) 2560 (+0.0%)
Well this turned into a never-ending can of worms...
I guess the good news is our newly added lfsr_grow_incr_* tests are
_very_ good at finding post-error-resume bugs.
Implementation-wise, this was fairly straightforward thanks to prior
work by BrianPugh, kaetemi, and myself:
1. Made block_count pseudo-optional by adding lfs.block_count so we can
mutate it based on what we find on-disk.
This was done a bit different from the previous implementation,
instead of setting block_count=0 to read the block_count from disk,
we allow any block_count <= the configured block_count.
This matches how we handle name_limit/file_limit/etc, and allows
users to mount a filesystem with unknown block_count while asserting
an upper bound.
2. Added lfsr_fs_grow, which can grow the filesystem.
The is basically the same as the previous implementation except we're
a bit more careful with the lookahead buffer.
I thought the previous impl might have been broken w.r.t. lookahead
buffer, but fortunately it's only broken in a way that makes us think
newly available blocks are temporarily in-use. Which is a bit funny.
One interesting thing that came out with more aggressive tests is
that it's possible to get locked-up in lfsr_fs_preparemutation trying
to clean up grms/orphans before we change the filesystem size.
Fortunately it turns out we don't _really_ need to call
lfsr_fs_preparemutation here. This gets a bit delicate, but means we
should always be able to grow a full filesystem.
To test this I've added both the simple grow/error tests from the
previous version, as well as a set of fuzz tests (a la test_relocations
and friends) that incrementally grow the filesystem when encountering
LFS_ERR_NOSPC. These have a surprising amount coverage, testing
lfsr_fs_grow, lfsr_fs_stat, lfsr_fs_size, and resuming operations after
encountering an error.
Which also means they found bugs:
- lfs_alloc_setinuse was not broken before, because lookahead.start was
always a multiple of lookahead_size. But now with lfs_alloc_discard,
this invariant may not be true.
I've just changed all lookahead.start updates to mod block_count. This
adds a bit of code, but is much easier to reason about.
While fixing this, I also added an assert to never allocate blocks
{0,1} in lfs_alloc. This is a good assert to have, but did require
some tweaks to test_btree to avoid these blocks.
- We were incorrectly patching grms in lfsr_mdir_commit when mdelta=0.
Funnily enough we also proceed to ignore the patched grm most of the
time when mdelta=0, so this went unnoticed.
- It turns out we're completely ignoring rid=-1 attrs if we split the
mroot. Not sure how this was missed. It's a bit important.
Note this is still broken. Fixing this requires some rather invasive
changes to lfsr_mdir_commit's internal logic that should probably be
in another commit...
Note again fwrite_fuzz is omitted. Currently the state of data in opened
files is undefined after a failed write, so this wouldn't really be
testing anything interesting...
More features = more code, and all of this bug fixing meant several
things contributed to code/stack changes in this commit:
code stack
before: 33654 2592
+variable block_count: 33646 (-0.0%) 2584 (+0.0%)
+lfsr_fs_grow: 33818 (+0.5%) 2584 (-0.3%)
+lookahead-start-fix: 33842 (+0.6%) 2584 (-0.3%)
+grm-patch-fix (after): 33850 (+0.6%) 2584 (-0.3%)
Wild that variable block_count actually saves code/stack. I guess the
indirect lfs->cfg->block_count load can get costly...
This acts as a marker to indicate a fuzz test. It should reference a
define, usually SEED, that can be randomized to get interesting test
permutations.
This is currently unused, but could lead to some interesting uses such
as time-based fuzz testing. It's also just useful for inspecting the
tests (make test-list).
Our B-trees lazily allocate their root blocks, so it makes more sense
for this to be a macro. Added/adopted a similar LFSR_SHRUB_NULL for
consistency.
Unfortunately this added a bit of code. I think because GCC struggles to
optimize compound literals, which both LFSR_BTREE_NULL and
LFSR_SHRUB_NULL expand into:
code stack
before: 33538 2624
after: 33550 (+0.0%) 2624 (+0.0%)
These don't really work because the filesystem is in an invalid state.
lfs_alloc might return LFS_ERR_NOSPC, but it also might throw a random
error because nothing was initialized correctly.
The better strategy is to just make sure these tests can't exhaust a
standard test configuration, in this case 1MiB or 256 blocks (4096x256).
If we want to test a smaller block device we can always add test case
conditions.
test_wl is intended to test wear-leveling, although right now that just
involves heavy-duty fuzz tests with extremely low block_recycles.
What may be more interesting is the addition of aggressive orphan/zombie
tests:
- test_forphans_orphanzombie_fuzz
- test_forphans_orphanzombiedir_fuzz
- test_wl_orphanzombie_fuzz
- test_wl_orphanzombiedir_fuzz
These tests mix random file/dir operations while keeping random file
handles open, creating a complex environment for hitting weird orphan/
zombie corner cases.
And they did find a bug! We were asserting on LFS_ERR_RANGE when
migrating shrubs/sprouts during lfsr_mdir_commit__. The tricky thing
about lfsr_mdir_commit__ is that we need to expect LFS_ERR_RANGE from
any append operations, since this is what trigger mdir compaction. This
is especially tricky since LFS_ERR_RANGE is a hard error in most other
functions.
Easy fix. lfsr_mdir_commit__ contains no more LFS_ERR_RANGE asserts.
With these tests hopefully that's the last time we see this mistake.
The main idea here is that diverse tests are better than many similar
tests.
Sure, if we throw fuzz tests at the system all day we'll eventually find
more bugs, but if a developer is in the loop that time is going to be
better spent writing specific tests targeting the fragile parts of the
system.
And don't worry, we can still throw fuzz tests at the system all day by
specifying explicit seeds with -DSEED=blah.
Changes:
- Limited dir-related powerloss fuzz testing to N <= 16.
These tests were the biggest culprit of excessive test runtime,
requiring O(n^2) redundant operations to recover from powerlosses
(they just replay the full sequence on powerloss).
- As a tradeoff, bumped most fuzz tests to a minimum of 20 seeds.
The big exception being the test_fwrite tests, which are heavily
parameterized and already take the most time to run. Each parameter
combination also multiplies the effective number of seeds, so
increasing the number of base seeds will probably have diminishing
returns.
- Limited test_fwrite_reversed to SIZE <= 4*1024*CHUNK.
Writing a file backwards is just about the worst way you could write a
file, since all buffering/coalescing expect writes to eventually make
forward progress. On the flip side, because it's uncommon, writing a
file backwards is also a great way to find bugs. But at some point a
compromise needs to be made.
Impacted test runtimes:
case otime ntime dtime
test_btree_push_fuzz 0.3 0.5 +0.2 (+60.2%)
test_btree_push_sparse_fuzz 0.4 3.3 +2.9 (+720.4%)
test_btree_update_fuzz 0.4 0.9 +0.6 (+141.6%)
test_btree_update_sparse_fuzz 0.5 4.5 +4.1 (+857.4%)
test_btree_pop_fuzz 0.6 2.3 +1.7 (+314.7%)
test_btree_pop_sparse_fuzz 1.2 5.7 +4.4 (+356.2%)
test_btree_split_fuzz 0.5 1.4 +0.8 (+150.2%)
test_btree_split_sparse_fuzz 0.4 5.6 +5.1 (+1163.2%)
test_btree_find_fuzz 0.5 0.7 +0.2 (+50.7%)
test_btree_find_sparse_fuzz 1.0 3.0 +2.0 (+189.8%)
test_btree_traversal_fuzz 0.6 2.3 +1.6 (+260.4%)
test_dirs_mkdir_many 3.3 2.1 -1.3 (-37.8%)
test_dirs_mkdir_many_backwards 3.5 2.1 -1.4 (-39.9%)
test_dirs_mkdir_fuzz 115.3 106.4 -8.9 (-7.7%)
test_dirs_rm_many 283.9 76.8 -207.0 (-72.9%)
test_dirs_rm_many_backwards 216.1 80.6 -135.5 (-62.7%)
test_dirs_rm_fuzz 647.0 68.5 -578.5 (-89.4%)
test_dirs_mv_many 14.2 15.4 +1.1 (+7.9%)
test_dirs_mv_many_backwards 16.5 14.5 -2.1 (-12.5%)
test_dirs_mv_fuzz 1932.5 156.7 -1775.8 (-91.9%)
test_dirs_general_fuzz 561.9 74.5 -487.4 (-86.7%)
test_dread_recursive_rm 336.6 46.2 -290.4 (-86.3%)
test_dread_recursive_mv 55.5 44.6 -11.0 (-19.8%)
test_fsync_rrrr_fuzz 0.4 0.3 -0.1 (-18.4%)
test_fsync_wrrr_fuzz 8.0 12.4 +4.5 (+56.0%)
test_fsync_wwww_fuzz 13.2 33.4 +20.2 (+152.6%)
test_fsync_wwrr_fuzz 5.4 50.9 +45.5 (+841.6%)
test_fsync_rwrw_fuzz 2.4 8.4 +6.0 (+253.9%)
test_fsync_rwrw_sparse_fuzz 3.2 7.5 +4.2 (+129.9%)
test_fsync_rwtfrwtf_sparse_fuzz 6.1 8.5 +2.4 (+39.3%)
test_fsync_drrr_fuzz 11.8 9.2 -2.6 (-21.8%)
test_fsync_wddd_fuzz 9.3 11.9 +2.6 (+28.0%)
test_fsync_rwdrwd_fuzz 1.6 33.1 +31.5 (+1963.4%)
test_fsync_rwdrwd_sparse_fuzz 0.3 1.8 +1.4 (+418.8%)
test_fsync_rwtfdrwtfd_sparse_fuzz 0.3 1.1 +0.8 (+260.2%)
test_fwrite_reversed 728.5 345.2 -383.3 (-52.6%)
TOTAL 7587.5 3792.3 -3795.2 (-50.0%)
This turned into a sort of system-wide refactor based on learned
knowledge of what we can do with lfsr_attr_t.
The big changes:
- Reverted LFSR_ATTR to mainly take lfsr_data_t again, keeping
lfsr_data_t as the default data representation in the codebase.
Now that we know
LFSR_ATTR_CAT_ still provides concatenation mechanics, and LFSR_ATTR_
provides a way to edit in-flight lfsr_attr_ts.
- Dropped lfsr_cat_t, replaced with explicit const void* + uint16_t,
tried to limit to low-level operations and prefer passing aroud
lfsr_attr_t and lfsr_data_t at a high-level.
Note this cat + cat_count pair is quite similar to the common attrs +
attr_count and buffer + size arguments.
- Adopted lfsr_attr_t more in mid-level functions, lfsr_rbyd_appendattr,
lfsr_rbyd_appendcompactattr, lfsr_file_carve, etc. This is a bit more
ergonomical, allows for use of LFSR_ATTR* macros, and in theory might
even save a bit of stack.
Unfortunately this seems to have resulted in a net hit to code cost,
though I still think it's worth it for the internal ergonomics:
code stack
before: 33652 2624
after: 33780 (+0.4%) 2640 (+0.4%)
Investigating further suggests this may just be the result of compiler
noise and changes to argument placement. lfsr_attr_t does touch a lot of
code...
It's interesting to note the adoption of lfsr_attr_t in
lfsr_rbyd_appendattr* and friends prevents their transformation into
.isra functions, though this doesn't seem to impact code cost too much:
function (5 added, 5 removed) osize nsize dsize
lfsr_cat_size - 48 +48 (+100.0%)
lfsr_file_carve - 1600 +1600 (+100.0%)
lfsr_rbyd_appendattr - 2120 +2120 (+100.0%)
lfsr_rbyd_appendattr_ - 244 +244 (+100.0%)
lfsr_rbyd_appendcompactattr - 68 +68 (+100.0%)
lfsr_rbyd_appendcompactrbyd 144 152 +8 (+5.6%)
lfsr_file_truncate 298 314 +16 (+5.4%)
lfsr_mdir_commit__ 1056 1112 +56 (+5.3%)
lfsr_mdir_compact__ 502 526 +24 (+4.8%)
lfsr_rbyd_appendattrs 132 138 +6 (+4.5%)
lfsr_file_fruncate 386 402 +16 (+4.1%)
lfsr_data_frombtree 84 86 +2 (+2.4%)
lfsr_rbyd_appendcksum 512 520 +8 (+1.6%)
lfsr_file_opencfg 572 580 +8 (+1.4%)
lfsr_rename 608 616 +8 (+1.3%)
lfsr_mkdir 500 504 +4 (+0.8%)
lfsr_bd_prog 278 280 +2 (+0.7%)
lfsr_mdir_commit 2364 2360 -4 (-0.2%)
lfsr_bshrub_commit 716 712 -4 (-0.6%)
lfsr_file_sync 526 514 -12 (-2.3%)
lfsr_file_flush_ 1868 1820 -48 (-2.6%)
lfsr_remove 456 436 -20 (-4.4%)
lfsr_fs_fixgrm 168 160 -8 (-4.8%)
lfsr_cat_size.isra.0 42 - -42 (-100.0%)
lfsr_file_carve.isra.0 1596 - -1596 (-100.0%)
lfsr_rbyd_appendattr.isra.0 2088 - -2088 (-100.0%)
lfsr_rbyd_appendattr_.isra.0 232 - -232 (-100.0%)
lfsr_rbyd_appendcompactattr.isra.0 56 - -56 (-100.0%)
TOTAL 33652 33780 +128 (+0.4%)
Christopher Haster