This (re)implements the heavy-hitting tests in test_badblocks that rakes
filesystem operations over various types of prog/erase failures:
- test_badblocks_[one|region|alternating]_btree - force tall B-trees
- test_badblocks_[one|region|alternating]_dirs - large mtree
- test_badblocks_[one|region|alternating]_files - mixed mtree + files
- test_badblocks_[one|region|alternating]_fwrite_fuzz - complex files
- test_badblocks_[one|region|alternating]_orphanzombiedir_fuzz - complex
- test_badblocks_mrootanchor - uh, format fails, cheap test though
Where:
- test_badblocks_one_* - runs with every possible bad block
- test_badblocks_region_* - runs with a large region of bad blocks
- test_badblocks_alternating_* - runs with alternating bad blocks, this
one is rough for block pair allocations
This required quite a bit of rewiring of internal block allocations. I
knew this would eventually need to be (re)implemented, but the jump from
infallible to fallible progs everywhere was still quite involved:
- lfs_alloc no longer returns LFS_ERR_CORRUPT if erase fails, instead it
will keep searching for a block where an erase "sticks" or return
LFS_ERR_NOENT. This simplifies above layers.
This actually turned out to be required since the lookahead traversal
can also return LFS_ERR_CORRUPT... which needs to be treated as a hard
error and bail.
- In lfsr_btree_commit_ all inner-node compactions needed alloc loops.
This really complements B-tree's copy-on-write behavior, but does make
lfsr_btree_commit_ a bit of a goto soup...
- Same for lfsr_btree_commit/lfsr_bshrub_commit, but fortunately there
are nice and self-contained.
- lfsr_mdir_alloc__/lfsr_mdir_swap__ needed a bit of an overhaul to be
able to handle bad progs. lfsr_mdir_alloc__ now takes a bool `all`
parameter to know if it should allocate one or two of the mdir blocks.
You could argue it's simpler/cheaper to always allocate two blocks at
a time, but this could lead to premature filesystem death on
unfortunate bad block patterns. test_badblocks_alternating_*
specifically tests for this. Note we still allocate both on
relocation, but only on the first commit attempt.
This also rearranges things to move the overcompacting logic out of
lfsr_mdir_swap__ and into lfsr_mdir_commit_, since we only want to
overcompact after trying to program all possible free blocks.
- lfsr_file_flush_ now needs to rewrite the entire block of data if a
prog fails, even if appending an existing data block.
Humorously, this was really easy, since we already align everything to
any existing blocks as a part of our crystallization algorithm. Almost
too easy... (no new code! only a couple gotos! scary!)
Note some of these may be transformable into simpler while loops, but I
decided to avoid this and prefer explicit `relocate` gotos because: 1.
in some functions these end up deeply nested in existing loops and I was
already bitten by a shadowed continue, 2. the "good" path does not loop,
with a loop you need an easy to miss break and the intention is less
clear, and 3. consistency is good.
We are _not_ testing read errors yet. This is because we no longer read
back progs and the relaxed rcache/pcache alignment requirements make
this a bit difficult to (re)implement. User feedback also suggests we
may want to make this optional... So need to think on how to address
this.
Some other notes:
- Our low-level bd wrappers, lfsr_bd_*__, now log bad ops via LFS_DEBUG.
- Overcompaction is now an LFS_WARN.
- The pcache is now correctly dropped if we error during flush.
- I noticed lfsr_btree_alloc double allocated for new B-trees, it
doesn't now, maybe change this function?
- Our B-tree tests all stop on LFS_ERR_NOSPC, but this isn't guaranteed
since our filesystem isn't in a valid state. We should make sure none
of our B-tree tests actually rely on this...
Honestly, considering how much new logic was introduced, this really did
not impact code cost as much as I thought it would. Probably thanks to
the underlying data structures being built to easily discard blocks in
the first place:
code stack
before: 33474 2640
after: 33618 (+0.4%) 2648 (+0.3%)
This should allow mdir commits that would normally trigger a relocation
to continue if lfsr_mdir_alloc__ return LFS_ERR_NOSPC, though at least
with a logged warning.
This seems preferable to the alternative: locking up the filesystem.
Though this doesn't have tests yet, so take it with a grain of salt...
Code changes minimal:
code stack
before: 33470 2640
after: 33474 (+0.0%) 2640 (+0.0%)
We were unconditionally xoring our prng seed with mdir_[0]'s cksum, but
we should really use mdelta to xor in the relevant cksums.
This is a little bit more complicated, so adds a little bit of code:
code stack
before: 33442 2640
after: 33470 (+0.1%) 2640 (+0.0%)
The mleafweight naming is... not great...
Renaming mleaf_bits -> mdir_bits and replacing mleafweight with explicit
shifts of 1 << mdir_bits seems to get the job done without introducing a
new and potentially confusing name.
This was a lesson learned from recycle_bits. Sometimes more helpers just
makes code less, not more, readable.
In theory int should always be the fastest type for simple loops.
No idea why this cost 4-bytes. Looking at the dissassembly, the int
version seems to write to the stack more often? The revision count
logic doesn't change at all... Compiler noise?
code stack
before: 33438 2640
after: 33442 (+0.0%) 2640 (+0.0%)
I think this fits a bit better with the new ordering requirement for
mdir splits.
I also explored the same transformation in lfsr_btree_commit_, but
decided against it for two reasons:
1. It saves roughly the same amount of code, but increases the RAM cost,
probably due to decreased flexibility on where/when to allocate the
structs. lfsr_btree_commit_ is and likely always will be on the stack
hot-path, so this is a bit important.
2. The naming may be confusing. Unlike in lfsr_mdir_commit, sibling in
lfsr_btree_commit_ serves multiple roles, including the previous
sibling for btree merges. I imagine renaming sibling -> rbyd_[1]
would make that whole sequence quite difficult to read...
At least in lfsr_mdir_commit this saves a bit a code:
code stack
before: 33482 2640
+btree: 33398 (-0.3%) 2664 (+0.9%)
after: 33438 (-0.1%) 2640 (+0.0%)
Good news! test_wl_orphanzombie_fuzz found a rare and difficult to reach
bug. Bad news, it found the bug only after changing littlefs's initial
revision count, which is about as unrelated a change as you can possibly
have...
Oh well, at least now we can add specialized tests targeting this (and
push them to hopefully cover anything similar):
- test_files_mv_split
- test_files_mv_split_backwards
- test_forphans_rename_split
- test_forphans_rename_split_backwards
The bug occurs when a rename of a file to/from the same mdir triggers an
mdir split, and you have that file opened, and the opened file handle
tracks a bshrub or bsprout. Oh, and if that wasn't unlikely enough, this
only breaks when the rename crosses from the new-right-sibling to the
new-left-sibling (inverse order of mdir split compacts), left-to-right
is fine.
The problem is how we stage bshrubs/bsprouts. bshrubs/bsprouts are a bit
tricky in that several unrelated operations can change their location,
sometimes multiple times in the same lfsr_mdir_commit call:
- mdir compaction - move bshrub/bsprout to new mdir
- bshrub commit - append a new shrub trunk
- rename commit - move bshrub/bsprout to a new mdir/mid
To keep track of all of this, lfsr_file_t has a dedicated field,
file.bshrub_, that holds the bshrub/bsprout's new location during
lfsr_mdir_commit. This may be changed multiple times, but the last
change wins.
This works as long as changes occur in an expected order. Importantly,
commits that change the bshrub, such as rename, need to play out after
compactions.
It turns out this is violated when splitting an mdir.
Because we have single pcache, we need to write out the entire compact +
commit of each mdir at a time. When we split, we arbitrarily do this
left-to-right, which results in left commits being played out before
right compactions.
Here's how things play out when we rename right-to-left:
1. commit rename -> bshrub = src mid, orig mdir
2. commit fails because of ERANGE
3. compact left mdir -> bshrub = src mid, left mdir
4. commit left mdir -> bshrub = dst mid, left mdir
5. compact right mdir -> bshrub = src mid, right mdir
6. commit right mdir (skips rename)
Oh no! Our staged bshrub ends up with the wrong location.
---
This is quite tricky to solve. We can't just play out the rename again
on the right mdir, because we've already lost the new bshrub trunk at
this point. Other solutions involving the grm or extra "moved" flags get
messy because, well, lfsr_mdir_commit's internals are quite messy.
The solution here, which is a bit hacky, but also obnoxiously elegant in
a way, is to reorder the split mdir compactions such that the new mdir
containing the commit mid is always compacted last. The means any
related attrs are played out after both compactions, allowing renames to
resolve correctly:
1. commit rename -> bshrub = src mid, orig mdir
2. commit fails because of ERANGE
3. right mdir contains mid
4. compact right mdir -> bshrub = src mid, right mdir
5. commit right mdir (skips rename)
6. compact left mdir -> bshrub = src mid, left mdir
7. commit left mdir -> bshrub = dst mid, left mdir
This only works as long as such commits only span a single mid, though
we already rely on mdir commits being single-mid elsewhere, so maybe
this won't be a problem?
The only real remaining concern is how much complexity this adds to
lfsr_mdir_commit. And while this feels logically messy, the resulting
code cost is surprisingly little:
code stack
before: 33458 2640
after: 33482 (+0.1%) 2640 (+0.0%)
Still, I'll have to scratch my head to see if there's a better way to
solve this...
The main reason is just to avoid using 0x00000000 as the initial
revision count. The is currently the default for B-tree rbyds, and
accidentally writing a B-tree rbyd to an mroot block is both easy
(misconfigured block_count) and something we really want to notice when
debugging.
Fortunately we can still keep our sequence comparison test (0 > -1) by
only setting the top-bits. Though we still need to zero the recycle
counter to avoid premature mroot extension, so this magic number may not
stay intact depending on configuration.
This does add a bit of code, probably to load the magic number from
Thumb's constant pools, but I think it's worth it:
code stack
before: 33430 2640
after: 33458 (+0.1%) 2640 (+0.0%)
This makes a bit more sense with the new block_recycles name.
block_recycles=0 (previously block_recycles=1) requires 1 erase, but it
doesn't really "recycle" the block. With this change, block_recycles=1
"recycles" the block once (2 erases in total) before relocating, which I
think is a bit more intuitive.
Note, this sort of messes with our power-of-2 rounding, as the
block_recycles is technically rounded down to the nearest power-of-2
after adding 1:
- block_recycles=1022 -> 512 erases
- block_recycles=1023 -> 1024 erases
- block_recycles=1024 -> 1024 erases
- block_recycles=1025 -> 1024 erases
But I'm going to keep the block_recycles description more-or-less as is
for now, as I think this extra detail is more confusing than useful,
powers-of-2 stay powers-of-2, and the <=block_recycles contraint is not
violated.
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 original goal here was to restore all of the revision count/
wear-leveling features that were intentionally ignored during
refactoring, but over time a few other ideas to better leverage our
revision count bits crept in, so this is sort of the amalgamation of
that...
Note! None of these changes affect reading. mdir fetch strictly needs
only to look at the revision count as a big 32-bit counter to determine
which block is the most recent.
The interesting thing about the original definition of the revision
count, a simple 32-bit counter, is that it actually only needs 2-bits to
work. Well, three states really: 1. most recent, 2. less recent, 3.
future most recent. This means the remaining bits are sort of up for
grabs to other things.
Previously, we've used the extra revision count bits as a heuristic for
wear-leveling. Here we reintroduce that, a bit more rigorously, while
also carving out space for a nonce to help with commit collisions.
Here's the new revision count breakdown:
vvvvrrrr rrrrrrnn nnnnnnnn nnnnnnnn
'-.''----.----''---------.--------'
'------|---------------|---------- 4-bit relocation revision
'---------------|---------- recycle-bits recycle counter
'---------- pseudorandom nonce
- 4-bit relocation revision
We technically only need 2-bits to tell which block is the most
recent, but I've bumped it up to 4-bits just to be safe and to make
it a bit more readable in hex form.
- recycle-bits recycle counter
A user configurable counter, this counter tracks how many times a
metadata block has been erased. When it overflows we return the block
to the allocator to participate in block-level wear-leveling again.
This implements our copy-on-bounded-write strategy.
- pseudorandom nonce
The remaining bits we fill with a pseudorandom nonce derived from the
filesystem's prng. Note this prng isn't the greatest (it's just the
xor of all mdir cksums), but it gets the job done. It should also be
reproducible, which can be a good thing.
Suggested by ithinuel, the addition of a nonce should help with the
commit collision issue caused by noop erases. It doesn't completely
solve things, since we're only using crc32c cksums not collision
resistant cryptographic hashes, but we still have the existing
valid/perturb bit system to fall back on.
When we allocate a new mdir, we want to zero the recycle counter. This
is where our relocation revision is useful for indicating which block is
the most recent:
initial state: 10101010 10101010 10101010 10101010
'-.'
+1 zero random
v .----'----..---------'--------.
lfsr_rev_init: 10110000 00000011 01110010 11101111
When we increment, we increment recycle counter and xor in a new nonce:
initial state: 10110000 00000011 01110010 11101111
'--------.----''---------.--------'
+1 xor <-- random
v v
lfsr_rev_init: 10110000 00000111 01010100 01000000
And when the recycle counter overflows, we relocate the mdir.
If we aren't wear-leveling, we just increment the relocation revision to
maximize the nonce.
---
Some other notes:
- Renamed block_cycles -> block_recycles.
This is intended to help avoid confusing block_cycles with the actual
physical number of erase cycles supported by the device.
I've noticed this happening a few times, and it's unfortunately
equivalent to disabling wear-leveling completely. This can be improved
with better documentation, but also changing the name doesn't hurt.
- We now relocate both blocks in the mdir at the same time.
Previously we only relocated one block in the mdir per recycle. This
was necessary to keep our threaded linked-list in sync, but the
threaded linked-list is now no more!
Relocating both blocks is simpler, updates the mtree less often,
compatible with metadata redundancy, and avoids aliasing issues that
were a problem when relocating one block.
Note that block_recycles is internally multiplied by 2 so each block
sees the correct number of erase cycles.
- block_recycles is now rounded down to a power-of-2.
This makes the counter logic easier to work with and takes up less RAM
in lfs_t. This is a rough heuristic anyways.
- Moved the lfs->seed updates into lfsr_mountinited + lfsr_mdir_commit.
This avoids readonly operations affecting the seed and should help
reproducibility.
- Changed rev count in dbg scripts to render as hex, similar to cksums.
Now that we using most of the bits in the revision count, the decimal
version is, uh, not helpful...
Code changes:
code stack
before: 33342 2640
after: 33434 (+0.3%) 2640 (+0.0%)
Added:
name builtin? string.h?
lfs_memcmp y y
lfs_memcpy y y
lfs_memmove y y
lfs_memset y y
lfs_memchr y
lfs_memcchr (I wish!)
lfs_memxor
lfs_strlen y
lfs_strcmp y
lfs_strcpy y
lfs_strchr y
lfs_strcchr
lfs_strspn y
lfs_strcspn y
The intention of these is _not_ to try anything better than the stdlib,
but to allow users/integrators to override these functions if string.h
or stdlib.h is not available.
Well... The original motivation was just to add lfs_memcchr to
lfs_utils.h, which is useful for checking if a memory is all zeros, but
then things got a bit out of hand... Oh well, flexibility is good right?
Things get a bit... delicate wrapping memcmp/memcpy/memmove/memset like
this. These functions are basically primitives in C, and the compiler
can get up to all sort of tricks eliding/folding these. Unfortunately,
even just wrapping these in static inline functions seems to create
problems, so I've just defaulted to #defining the relevant lfs_*
symbols.
Even weirder, GCC's __builtin_* variants seem to be worse, code-wise,
than the stdlib symbols. Maybe because these ignore -Os hints? For this
reason I've prioritized the string.h's symbols unless LFS_NO_STRINGH is
defined:
code stack
before: 33338 2640
static-inline: 33422 (+0.3%) 2648 (+0.3%)
builtins: 33402 (+0.2%) 2640 (+0.0%)
after: 33342 (+0.0%) 2640 (+0.0%)
Comparing the LFS_NO_STRINGH and LFS_NO_INTRINSICS builds, just for
curiosity:
code stack
default: 33342 2640
no-string.h: 33486 (+0.4%) 2640 (+0.0%)
no-intrinsics: 33514 (+0.5%) 2616 (-0.9%)
no-both: 33722 (+1.1%) 2624 (-0.6%)
The extra 4 bytes introduced seem to come from the added
lfs_gdelta_xor -> lfs_memxor indirection, not really sure why, maybe
compiler/instruction alignment noise?
Why not provide __builtin_* variants for all string.h symbols? To be
honest, because we really don't care about the performance of strlen/
strcpy/strspn in littlefs. And in environments where string.h is not
available it's likely __builtin_str* won't be as well.
Note the test/bench frameworks should stick with the stdlib symbols. By
default C code should assume these are always available, and this makes
it slightly more reliable to test with -DLFS_NO_STRINGH or
-DLFS_NO_INTRINSICS.
The main change is moving away from applying gstate changes via special
attrs. Instead, gstate changes are applied implicitly, whenever the
relevant field in lfs_t differs from the gstate on-disk.
How do we recover from errors then? Well, we already need to track the
exact on-disk encoding of any gstate (grm_p) to avoid issues with minor
encoding differences, so if we encounter an error, we can revert any
changes to gstate by re-decoding the on-disk gstate. This is more
fragile: 1. all error paths in lfsr_mdir_commit need to revert gstate,
2. logic must not error between gstate updates and lfsr_mdir_commit, but
it gets the job done.
The benefit of this approach is that it's much easier to manipulate
gstate inside of lfsr_mdir_commit. No more hacky attr-list scanning to
patch grms mid-commit! It also in theory saves stack usage by dropping
an attr, but none of these attrs were on our stack hot-path.
Other gstate changes:
- Moved all grm adjustments into lfsr_mdir_commit.
This should deduplicate the messy grm adjust logic and make grms
easier to work with.
One hiccup though is the temporarily self-removing bookmark created in
lfsr_mkdir, which needs to create a grm referencing an mid that
doesn't exist yet. To work around this, lfsr_mdir_commit now
automatically creates grms for new bookmarks.
This might be a problem if we ever elide same-mdir mkdirs, but if so
we can solve that problem then.
- Dropped lfsr_data_t xoring, the added complexity wasn't really worth
it since all gstate should be small enough to buffer on the stack.
- Renamed several things:
- lfsr_grm_push/poprm -> lfsr_grm_push/pop
- lfsr_grm_isrm -> lfsr_grm_ispending
- grm_g -> grm_p
- grm.rms -> grm.mids
- Moved things around so grm/gstate logic is grouped together.
Unfortunately none of these attrs were on our stack hot-path, so no
stack savings. But thanks to the simpler logic, this does save quite a
bit of code:
code stack
before: 33514 2632
after: 33338 (+0.5%) 2640 (+0.3%)
- cat_count < 0 => single in-RAM buffer
- cat_count >= 0 => multiple concatenated datas
Note that cat_count=0 has the same effect whether or not you interpret
the cat as single or multiple datas.
Unlike, say, lfsr_data_t's size, the cat count does not mean the same
thing in both modes, so it doesn't really make sense to operate on the
count with bits masked off. This makes cat_count more like the signed
size/err union we use often.
The hope was better code generation for single/multiple cat checks. I
noticed some questionable code generation around checking the uint16_t's
sign bit and realized this might be a bit messy on 32-bit thumb. Sign
extension is in theory more common/cheaper on 32-bit ISAs, but I don't
know if the results are really conclusive:
before: 33538 2632
after: 33514 (-0.1%) 2632 (+0.0%)
So for example:
file->m.mdir.mid => file->o.mdir.mid
We already use "o" in opened-list iterations, so this is a bit more
consistent. And it doesn't increase the already obnoxious
file->o.mdir.rbyd.blocks[0] field names...
Now that lfsr_dir_t contains a single lfsr_opened_t, it makes sense for
lfsr_opened_t to always come first in lfsr_dir_t/lfsr_file_t for
consistency.
This also allows cheaper lfsr_file_t <-> lfsr_opened_t casts (noops),
which saves a bit of code:
code stack
before: 33582 2632
after: 33538 (-0.1%) 2632 (+0.0%)
This simplification comes from the observation that we don't actually
need to know the bookmark's mid to know if a given operation is in a
dir's range, just the dir's did. And since dids are immutable, we don't
need another opened-list entry or other shenanigans.
A dir's did is a bit harder to access, requiring a name lookup, but we
conveniently already fetch these in all relevant functions as a part of
path resolution.
This does mean more opened-list logic in the high-level functions:
function can zombie can create can remove
lfsr_mkdir y y n
lfsr_rename y y y
lfsr_remove y n y
lfsr_file_opencfg y y n
But I think this actually results in better code readability, since the
opened-list logic and high-level logic are closely related. I went ahead
and lifted the similar orphan/zombie opened-list logic up to this level
for this reason.
Unfortunately lifting this logic does result in a higher code cost, but
I think this is worth it for better readability and a significantly
reduced RAM cost for lfsr_dir_ts. Keep in mind these will probably
become very common for the future planned openat/*at functions:
code stack lfsr_dir_t
before: 33402 2632 80
after: 33582 (+0.5%) 2632 (+0.0%) 44 (-45.0%)
Also added a new test case, test_dread_read_rm_remkdir, to catch the
mistake of thinking the did is unique even when the dir is removed,
since that is now a concern.
We use the lfsr_attr_t struct for multiple purposes now, including some
situations where it holds the total weight, not the delta weight.
"delta" is also getting increasingly overloaded in littlefs, referring
also to offset changes ("d"), and gstate deltas...
Because of the invasive linked-lists, this was a bit more complicated
than the related move in lfs_t. But we already have similar
field-relative offsets in lfsr_dir_t for the dir + bookmark mdirs.
Added some helpers to help with this:
- lfsr_opened_dir
- lfsr_opened_constdir
- lfsr_opened_bookmark
- lfsr_opened_constbookmark
- lfsr_opened_file
- lfsr_opened_constfile
Unfortunately this resulted in less savings than in lfs_t, and actually
costs us code, likely because of how often we go from lfsr_file_t <->
lfsr_opened_t:
code stack
before: 33358 2632
after: 33402 (+0.1%) 2632 (+0.0%)
- Removed no longer used fields.
- Commented out related field asserts in lfs_init.
- Commented out pre-lfsr structs and function decls.
- Moved cfg to the first field in lfs_t.
Note that most of the code saves actually came from that last point.
Moving lfs.cfg, probably the currently most accessed field, resulted in
a surprising amount of code savings:
code stack lfs_t
before: 33702 2640 220
after+cfg last: 33592 (-0.3%) 2632 (-0.3%) 164 (-25.5%)
after+cfg first: 33358 (-1.0%) 2632 (-0.3%) 164 (-25.5%)
Maybe we should take a more rigorous/analytical approach to field
placement?
This would have been more valuable if the extra lfsr_data_t stack
allocation (12 bytes) wasn't already unioned with the btree's encoding
buffer allocation (18 bytes):
code stack
before: 33714 2640
after: 33702 (-0.0%) 2640 (+0.0%)
Oh well, this still might save some stack in the future if things shift
around.
A much requested feature, this allows much finer control of how RAM is
allocated for the system.
It was difficult to introduce this in previous versions of littlefs due
to how we steal caches during certain file operations, but now we don't
do that and treat the caches much more transparently.
Managing separate cache sizes does add a bit of code, but this is well
worth the potential for RAM savings due to increased flexibility:
code stack
before: 33656 2632
after: 33714 (+0.2%) 2640 (+0.3%)
Also interesting to note this reduces alignment requirements for the
rcache/pcache, since they don't need to share alignment, and completely
removes any alignment requirement from the file buffers.
This limits the maximum size of a file, which is also implies the
maximum integer size required to mount.
The exact name is a bit of a toss-up. I originally went with size_limit
to avoid confusion around if file_limit reflected the file size or the
number of files, but since this ends up mapping to lfs_off_t and _not_
lfs_size_t, I think size_limit may be a bit of a bad choice.
An easy mistake to make, we were incorrectly checking the non-staging
shrub to see if our shrub had been copied over. Copying over the shrub
updates the staging shrub, so this wasn't actually doing anything
useful, resulting in a bunch of duplicate shrubs.
The fix is to use the staging shrub.
This was found thanks to test_fsync_wwrr, but only after bumping our
fragment_size up from cache_size (16 bytes) -> block_size/8 (512 bytes).
I'm guessing because this allowed our shrubs to be more overcommitted.
The concern with block_size/4 is that it limits fragments to a single
fragment per-block. But while this may be inefficient, it's technically
not wrong, and may still work with other metadata (bptrs, file names,
uattrs, etc) taking up the remaining space.
This deserves benchmarking, but even if this ends up being a terrible
configuration, we should just discourage this via good defaults and
documentation.
Also added related asserts to lfs_init.
Note the fragment_size <= block_size/8 limit is to avoid wasteful corner
cases where only one fragment can fit in a block. The shrub_size <=
block_size/4 limit is looser because of how shrubs temporarily
overcommit.
As for the other limits, inline_size is bounded by shrub_size, and
crystal_thresh technically doesn't have a limit, though values >
block_size stop having an effect.
It's really frustrating that it's impossible to create an uninitialized
expression with the scope of a compound-literal...
(I'm going to ignore that this is technically possible with alloca.)
The lack of uninitialized compound-literals forces each of our attribute
lists to make a decision: 1. Use an implicit buffer and pay for
zero-initialization? or 2. Use an explicit buffer, adding code noising
and risking out-of-date buffer sizes.
As a compromise, this commit adopts explicit buffers in most of the
low/mid-level layers. Where the code is already pretty noisy, but also
heavily scrutinized and iterated over to reduce code/stack costs. This
leaves the high-level layers with the hopefully safer and more readable
implicit buffers.
You can see this zero initializing has a surprisingly high code cost,
for what is otherwise a noop:
code stack
before: 33828 2632
after: 33656 (-0.5%) 2632 (+0.0%)
Now it is fit for purpose and can replace the explicit tag comparison +
assert in lfsr_rbyd_appendattr. Previously we had to check if delta==0,
but now we just assert that delta!=0 is invalid for noops.
Unfortunately this added a couple bytes of code. The disassembly for
lfsr_rbyd_appendattr is all shuffled up, so I guess this is just
compiler noise. At least it's better than an explicit delta check:
code stack
before: 33820 2632
check delta: 33832 (+0.0%) 2632 (+0.0%)
assert delta: 33828 (+0.0%) 2632 (+0.0%)
Turns out temporary struct-literals have a slightly better code/stack
footprint than array-literals. I guess because nuances around arrays in
C can cause problems for optimization passes?
This makes forcing lvalues for macro consistency much more appealing:
code stack
sometimes rvalues: 33780 2640
array lvalues (before): 33868 (+0.3%) 2640 (+0.0%)
struct lvalues (after): 33820 (+0.1%) 2632 (-0.3%)
I think what may be going on with the unexpected stack cost related to
struct passing, is something to do with scoping and how it interacts
with function inlining + shrink wrapping.
Compound-literals have a scope limited by the current statement, and
while temporary structs _should_ have a scope limited to the current
expressions, maybe this scope is getting messed up due to function
inlining?
Still smells like a compiler bug, but if this is true, wrapping the
struct-generating function calls with compound-literals should be more
robust at preventing unexpected stack increases in the future.
As a plus, this makes all LFSR_DATA/ATTR macros lvalues, which is nice
for consistency.
---
Unfortunately, it does seem like GCC 11 is not able to elide moving
compound-literals all that well. Repeatedly nesting trivial
compound-literals results in a measurable increase in code cost, even
though it should theoretically be a noop with optimizations.
This results in an unfortunate code size increase:
code stack
before: 33780 2640
after: 33868 (+0.3%) 2640 (+0.0%)
But at some point you have to give up trying to work around
insufficiencies in the compiler. I'll take 100 bytes of code over 100
bytes of stack any day.
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%)
If we have control of the struct, such as in lfsr_data_t and lfsr_cat_t,
moving the common tag outside of the union avoids naming ambiguities.
Counter-example: This doesn't work for lfsr_bshrub_t, since the contents
of that union are also used as separate types elsewhere. Fortunately the
common initial sequence union rules kick in here.
No code changes, which is good:
code stack
before: 33652 2624
after: 33652 (+0.0%) 2624 (+0.0%)
Now that lfsr_attr_t is "small", or at least the same size as
lfsr_data_t, it makes sense to change the helper functions to take
lfsr_attr_t by value for consistency. These should all be inlined
anyways.
It's interesting to note there _are_ appendattr/progattr functions, but
these don't take lfsr_attr_t directly since we usually do some
last-minute modification to the attr's weight/tag.
Cost cost is mostly unchanged, actually shaves off a few bytes, which is
a good sign:
code stack
before: 33664 2624
after: 33652 (-0.0%) 2624 (+0.0%)
Compound-literals weren't the culprit after all! It was... RVO
interactions with inlined function arguments?
To be honest I still don't quite understand what's going on, but I
present to you this madness:
code stack
before: 33664 2624
after: 33664 (+0.0%) 2624 (+0.0%)
I think I'm understanding a bit more how RVO interacts with inline
functions. And by that I mean I'm learning that the way RVO interacts
with inline functions is unfortunately very cursed...
Just take a look at this diff. This change should be a noop. But somehow
it saves 200 bytes of RAM:
code stack
before: 33684 2824
after: 33664 (-0.1%) 2624 (-7.1%)
I think what's happening is passing the result of lfsr_data_from* into
lfsr_data_cat is somehow preventing RVO, because the parameter would
need to be copied into the right argument slot? (argument registers?)
But we really don't need a copy, because lfsr_data_cat should end up
inlined. By inserting a compound literal, we force RVO, and all of these
unnecessary copies get cleaned up after lfsr_data_cat is inlined.
Keep in mind, in a perfect world, lfsr_data_cat should be a noop.
But I could be wrong about all of this. It's not really clear what the
compiler is doing, and I haven't dived that far into the disassembly...
The name is not super important, but note lfsr_data_cat matches
lfsr_attr_cat, which is a nice bit of consistency.
The main change here is the adoption of correct field assignments
instead of a hacky cast forcing lfsr_data_t -> lfsr_cat_t. Tests were
passing even with optimizations, but I was concerned about the longevity
of this approach.
As a plus, we can actually assert on size fitting into a uint16_t thanks
to the inline function.
Unfortunately, this creates a surprising stack penalty:
code stack
before: 33756 2624
after: 33684 (-0.2%) 2824 (+7.6%)
I've also played around with instead reverting lfsr_data_from* ->
lfsr_cat_from*, and providing the inverse lfsr_cat_data, but nothing
gets us quite back to LFSR_CAT_DAT stack:
code stack
before: 33756 2624
lfsr_data_cat: 33684 (-0.2%) 2824 (+7.6%)
lfsr_cat_data: 33872 (+0.3%) 2736 (+4.3%)
This needs more investigation. Unfortunately I don't think we can revert
this, since correctness wins over code/stack costs...
Now that compound-literals have been identified as the culprit, we can
actually adopt this smaller lfsr_attr_t representation without a random
code/stack increase.
This limits lfsr_cat_t's size field to 16-bits (15-bit size + 1-bit
for concatenated datas), allowing simple small attrs (the most common)
to save a word of RAM:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---.
| tag |-----------+--->| tag |c|size |
'---+---' | +---+---+---+---+
.-|--->| delta |
lfsr_srid_t | | +---+---+---+---+
.---+---+---+---. | | .->| ptr |
| delta |-' | | '---+---+---+---'
'---+---+---+---' | |
| |
lfsr_cat_t | |
.---+---+---+---. | |
|c|size |-----------' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
The non-trivial mapping of lfsr_cat_t to lfsr_attr_t does mean a bit
more complexity on lfsr_cat_t access, but now that we figured out the
compound-literal cost it seems the compiler is able to mostly elide
these.
The end result is some nice stack savings:
code stack
before: 33812 2712
after: 33756 (-0.2%) 2624 (-3.2%)
I've been fiddling around with our LFSR_ATTR macro to try to understand
why making it an inline function costs so much, and it seems like it's
not actually the inline function, but the compound literal that is the
problem. Specifically, returning a compound literal from an inline
function results in surprisingly poor code/stack costs!
I don't really know why this happens. Compiler bug/oversight related to
lvalues/rvalues? Compound literals interfering with RVO? Unsure.
I tried a few other struct initializers just in case it was related to
constness, but it seems the problem is the compound literal:
Inlined comp-lit:
return (lfsr_attr_t){tag, delta, cat};
Inlined const comp-lit:
return (const lfsr_attr_t){tag, delta, cat};
Inlined no-init:
lfsr_attr_t attr;
attr.tag = tag;
attr.delta = delta;
attr.cat = cat;
return attr;
Inlined init:
lfsr_attr_t attr = {tag, delta, cat};
return attr;
Code/stack sizes:
code stack
macro (before): 33852 2776
inline comp-lit: 34140 (+0.9%) 2760 (-0.6%)
inline const comp-list: 34140 (+0.9%) 2760 (-0.6%)
inline no-init (after): 33812 (-0.1%) 2712 (-2.3%)
inline init: 33812 (-0.1%) 2712 (-2.3%)
The good news is this at least offers a route forward for crammed 15-bit
attrs.
I guess we should also go reasses other uses of compound literals in the
codebase...
When we're not dealing with cats/attrs, it seems useful to still have
lfsr_data_from* functions that return our general purpose lfsr_data_t
type.
Added LFSR_CAT_DAT to cheaply convert from lfsr_data_t -> lfsr_cat_t
(at least for simple buffers), and used this to keep the LFSR_CAT_*
macros, which are useful for attr-list construction.
Unfortunately, because lfsr_data_t is 3-words vs lfsr_cat_t 2-words,
this does add both code and stack cost:
code stack
before: 33672 2744
after: 33852 (+0.5%) 2776 (+1.2%)
It's interesting to note this is _not_ because of any LFSR_CAT_* usage!
I tested this explicitly and lfsr_data_from* -> LFSR_CAT_DAT adds no
cost over the previous lfsr_cat_from* functions. A win for GCC. This
cost only comes from the direct usage of the returns lfsr_data_t types
in our grm handling and branch -> btree encoding.
Still it's an annoying cost... Maybe this should be reverted? The
nuances of lfsr_cat_t vs lfsr_data_t is a bit annoying.
So, for example, these are equivalent:
lfsr_cat_t cat = LFSR_CAT_BPTR(bptr);
uint8_t buf[LFSR_BPTR_DSIZE];
lfsr_cat_t cat = LFSR_CAT_BPTR_(bptr, buf);
The first leads to more readable code, but of course sometimes you need
explicit memory allocations.
This replaces lfsr_cat_frombptr, etc, though those functions are still
available. This name change is more relevant for LFSR_CAT_DATA/DATAS,
which involve bit more complicated macros.
I think this was becoming too over-engineered. That and if you're
fighting the compiler too much, sometimes it's best to just let the
compiler win.
There is also the minor benefit of not needing to worry about uint16_t
overflows in lfsr_cat_t. One concern is it's easy for missed lfsr_data_t
indirections to go unnoticed.
This reverts lfsr_attr_t from 3 words -> 4 words, but allows easy
mapping to the lfsr_cat_t part of the lfsr_attr_t:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---. . . .---+---+---+---.
| tag |--------------->| tag | | | tag | |
'---+---' +---+---+---+---+ +---+---+---+---+
.----->| delta | | delta |
lfsr_srid_t | +---+---+---+---+ +---+---+---+---+
.---+---+---+---. | .--->|c| size | | cat |
| delta |-' | +---+---+---+---+ | |
'---+---+---+---' | .->| ptr | | |
| | '---+---+---+---' . . '---+---+---+---'
lfsr_cat_t | |
.---+---+---+---. | |
|c| size |---' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
This doesn't impact stack as much as you would expect due to compiler
overhead around inline functions operating on multi-word structs:
before 15-bit: 33672 2776
best 15-bit: 33900 (+0.7%) 2736 (-1.4%)
prev 15-bit: 34486 (+2.4%) 2912 (+4.9%)
after: 33672 (+0.0%) 2744 (-1.2%)
I think the only big thing not reverted was switching lfsr_cat_t from
storing the sum of the lfsr_data_ts sizes to just storing the number of
lfsr_data_ts. This is cheaper to initialize (because inline functions
are weirdly costly!), cheaper to iterate over, and we don't actually
need lfsr_cat_size that often, unlike lfsr_data_size.
Eventually we'll probably need to revisit this to optimize 31/15-bit,
etc, littlefs configurations. But that's a problem for another day...
The idea here was to carve out explicit space for the lfsr_attr_t tag in
lfsr_cat_t, so that when we create lfsr_attr_ts we don't need multiple
references to lfsr_cat_t and can revert back to a macro:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---. . . .---+---+---+---.
| tag |-------. .----->| delta | | delta |
'---+---' | | +---+---+---+---+ +---+---+---+---+
'-|-+--->| tag |c|size | | cat |
lfsr_srid_t | | +---+---+---+---+ | |
.---+---+---+---. | | .->| ptr | | |
| delta |-' | | '---+---+---+---' . . '---+---+---+---'
'---+---+---+---' | |
| |
lfsr_cat_t | |
.---+---+---+---. | |
| (tag) |c|size |---' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
But as a part of creating the lfsr_attr_t we need to mutate the tag in
lfsr_cat_t, which ended up still needing a static inline function, and
ended up making the code/stack size even worse!
code stack
before: 33900 2736
after: 34486 (+1.7%) 2912 (+6.4%)
I was hoping to leverage the last-update rule of C99's designated
initializers, but this weird rule didn't quite work how I expected. The
compiler is free to omit earlier struct initializers, even if a later
initializer only partially initializes the struct.
Another interesting observation about lfsr_cat_t: We rarely actually
need to represent the full range of data:
- lfsr_file_write:block - Block writes call lfsr_bd_prog directly, don't
need to be represented as attrs.
- lfsr_file_write:fragment - Fragments already need the full
concatenated representation because of, uh, potential concatenation.
- lfsr_file_sync:sprouts - Inlined sprouts may use the full range.
Fortunately we can union an lfsr_data_t with the btree buffer, so
no extra stack cost.
- lfsr_getuattr/sattr (planned) - User/sys attributes may use the full
range. But these will probably not be on the stack hot-path.
Most attrs that use the simple buffer lfsr_cat_t representation are used
to encode internal structs, such as leb128s, ecksum, bptr, mptr, etc.
The largest of these right now is our bptr encoding, at 21 bytes, so
these easily fit in a short or a byte.
The choice of 15-bits (reserving one bit for cat/buf representation), is
convenient as it allows us to fit the cat size next to our 16-bit
lfsr_tag_t for free in 32-bit aligned systems:
lfsr_attr_t
.---+---+---+---.
| tag |c|size |
+---+---+---+---+
| delta |
+---+---+---+---+
| ptr |
'---+---+---+---'
Unfortunately, it seems like C _really_ wants to fight us on this one.
Not really because of the struct packing, but because of how we want
lfsr_attr_t to interact with lfsr_cat_t:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---.
| tag |-----------+--->| tag |c|size |
'---+---' | +---+---+---+---+
.-|--->| delta |
lfsr_srid_t | | +---+---+---+---+
.---+---+---+---. | | .->| ptr |
| delta |-' | | '---+---+---+---'
'---+---+---+---' | |
| |
lfsr_cat_t | |
.---+---+---+---. | |
|c|size |-----------' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
Initializing two fields with one argument is frustratingly impossible in
C99 unless you want to duplicate the argument tree, which we
_definitely_ don't want to do because this includes the actual data
encoding steps.
The only option is to use a static inline function. You might say "oh,
but static inline costs the same as a macro". But no. Switching to a
static inline function heavily penalizes this approach (6% of stack!):
code stack
macro: 33672 2776
inline: 34512 (+2.5%) 2952 (+6.3%)
after: 33900 (+0.7%) 2736 (-1.4%)
At least this does result in net stack savings, even with the inline
function penalty.
I'm committing this temporarily mostly just to record some _very_
interesting code/stack measurements.
This explores replacing the LFSR_ATTR macro with an inline function that
does the same thing:
#define LFSR_ATTR(_tag, _delta, _cat) \
((const lfsr_attr_t){_tag, _delta, _cat})
vs:
#define LFSR_ATTR(_tag, _delta, _cat) \
lfsr_attr(_tag, _delta, _cat)
static inline lfsr_attr_t lfsr_attr(
lfsr_tag_t tag, lfsr_srid_t delta, lfsr_cat_t cat) {
return (lfsr_attr_t){tag, delta, cat};
}
The motivation for this is to eventually support more complex
lfsr_attr_t layouts. Specifically, it would be nice if we could break up
the lfsr_cat_t into separate size/ptr fields. Unfortunately we can't
declare temporaries in macros (I wish we had statement expressions), and
we really don't want to duplicate the entire cat tree, so an inline
function seems like the only way to accomplish this...
But static inline functions have the same cost as a macro you say?
No. This assumes a perfect compiler. And it's pretty unfair to compiler
developers to expect a perfect compiler.
To be fair, this is an extremely harsh test. We use LFSR_ATTR _heavily_,
which is why it's getting this much scrutiny. lfsr_attr also both takes
in a 2-word struct, and returns a _4_-word struct, which probably makes
things messy.
Still, the results are concerning:
code stack
macro: 33672 2776
inline: 34512 (+2.5%) 2952 (+6.3%)
always_inline: 34512 (+2.5%) 2952 (+6.3%)
noinline: 33920 (+0.7%) 2888 (+4.0%)
Measured with GCC 11 -mthumb -Os. I also measured with
__attribute__((always_inline/noinline)) just to see how that affected
things.
With LFSR_CAT_DATAS for explicit arrays of datas, and this biggest use
of concatenated data being a rather explicit construction in
lfsr_file_carve, I don't think we really need LFSR_CAT_CAT.
The only non-hacky use was to define LFSR_CAT_NAME. But we know names
always use exactly 2 datas, so this might as well use LFSR_CAT_DATAS.
I am going to use this soapbox to complain a bit about compound struct
literals. Why do we need an array declaration to elevate temporary
structs to automatic storage duration? I wish you could init a compound
literal with the struct itself...
✗ &f()
✓ &(uint32_t){f()}
✓ (uint32_t[]){f()}
✗ &f()
✗ &(lfsr_data_t){f()} :(
✓ (lfsr_data_t[]){f()}
Some hacky compound array literals were needed to replace the hacky
LFSR_CAT_CATs in lfsr_file_carve for this reason, but I guess it's a
hack for a hack so...
Code unchanged:
code stack
before: 33728 2776
after: 33728 (+0.0%) 2776 (+0.0%)
This attr-list allocator stuff is becoming over-engineered, these
allocations really aren't that complex...
This may have simplified after removing becksums, but if we need that
complexity again we can cross that bridge when we get to it.
Hand-allocating, dropping buf_size/data_count tracking, and refactoring
lfsr_file_carve to pre-encode the right sibling gives us a nice bit of
code/stack savings:
code stack
before: 33796 2808
after: 33728 (-0.2%) 2776 (-1.1%)
Christopher Haster