Now, fractions are considered equal if they have the same ratio:
- 6/6 == 12/12 => True
- 3/6 == 3/12 => False
- 1/6 == 2/12 => True
It's interesting to note this implementation is actually more
numerically stable than float comparison, though that wasn't really the
goal.
The main reason for this is to allow other fields to take over when
sorting multi-field fractional data: cov (lines + branches), testmarks
(passed + time), etc. Before, sorting would usually stop after
mismatched fraction fields, which wasn't all that useful.
Sometimes, if test_runner errors before running any tests, the last test
id can end up being None. This broke test output writing, which expected
to be able to parse an id. Instead we should just ignore the malformed
id (it's not like we can write anything relevant about any tests here),
and report it to the user at a higher level.
- Renamed build-test -> build-tests
- Renamed build-bench -> build-benches
- Added list-tests alias
- Added list-benches alias
Also made the Makefile's help text generation a bit more robust to long
rule names, which are common in Makefiles. If the name is >=21 chars, we
just indent, similar to test/bench_runner --help.
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).
Note these are different than TESTFLAGS/BENCHFLAGS:
- TEST_CFLAGS/BENCH_CFLAGS => gcc $(TEST_CFLAGS) lfs.t.a.c -o lfs.t.a.o
- TESTFLAGS/BENCHFLAGS => ./scripts/test.py $(TESTFLAGS)
Also tried to group the src/tools/flags a bit better.
We've been wasting a lot of test cycles thanks to REMOUNT. Using a test
define for this effectively duplicates the test, when we really just
want to run more post-test code without additional mutation.
The main reason for REMOUNT has been to save typing, which, well, is not
a bad reason, these tests involve a lot of typing...
But this is probably a hammer/nail situation. If we replace these with a
small post-test loop, we can save quite a bit of time:
make test -j before: 5791.9s
make test -j after: 5123.8s (-11.5%)
Some tests still use a REMOUNT define, but these should be limited to
cases where remount actually changes the test's behavior.
littlefs does this internally anyways. The original intention was to
make sure non-powers-of-2 don't break, but we don't really validate what
these end up aligned to. And the intentional mismatch risks confusion
when debugging.
If it's worth testing non-powers-of-2, it should be an explicit test.
These should already be tested elsewhere, these test cases were mostly
copied from other suites after all.
And these tests are expensive, so we really shouldn't be running
permutations that don't add anything.
These seem to fit better as a separate test suite, since they involve a
few more moving parts than just relocations (badblocks, enospc, etc).
Maybe we'll end up adding more relocation/exhaustion specific tests?
This organization can always be changed in the future.
It's worth noting that, even separated, these are still some of the
longest running test suites:
... ...
test_exhaustion 488.1s
test_dirs 593.7s
test_rbyd 675.6s
test_badblocks 975.8s
test_relocations 1013.9s
test_fwrite 1868.3s
TOTAL 6076.3s
These tests provide a litmus test for if wear-leveling is working:
- test_relocations_wl_dir_fuzz
- test_relocations_wl_file_fuzz
- test_relocations_wl_orphanzombie_fuzz
- test_relocations_wl_orphanzombiedir_fuzz
We can't test the uniformity of wear, because we only implement static
wear-leveling, but what we can test is that doubling the size of storage
results in roughly doubling the lifetime of the storage.
I did try to implement some wear-leveling tests under powerloss, this
has some promise storing the current run/state on disk, but gave up
after realizing the way our linear powerloss heuristic works would
interfere with the assumption that both runs run in identical
environments...
---
Suprisingly enough, all of this fuzz testing did find another bug! We
were returning LFS_ERR_CORRUPT instead of LFS_ERR_NOSPC if
overcompaction failed to erase/prog the revision count. This is very
hard to hit, only being reachable if a block goes bad on the same erase
cycle an mdir's recycle counter overflows, and if there are no more
blocks in our filesystem, triggering overcompaction.
Difficult to hit bug, but easy fix. Just a tiny bit of extra code:
code stack
before: 33550 2624
after: 33566 2624
I guess these wear-leveling tests are also doubling as aggressive
LFS_ERR_NOSPC exhaustion tests...
These provide useful file powerloss testing that scales linearly as long
as progress can be made. They can still struggle a bit, especially with
relocations which often fail to make progress, but they are _much_ better
than the O(n^2) simulation-based fuzz tests:
- test_files_pl_fuzz - 258734 pls
- test_relocations_pl_fuzz - 928638 pls
Our current problem with simulation-based fuzz testing is that we lose
the simulation on powerloss. We could brute force this, repeatedly
rerunning the simulation until it succeeds, but this grows O(n^2) with
our linear powerloss heuristic.
To avoid this, test_*_pl_fuzz doesn't bother with a simulation, instead
relying on internal asserts to catch bugs. This is less rigorous, but
realistically probably going to catch any powerloss related issues.
Some notes:
- We need to store some state on disk. If we don't we will still end up
with O(n^2) behavior because we simply don't know how many operations
we've accomplished so far.
- Since we rely on file operations to store our test state, this makes
this approach incompatible with the dir tests, which assume file
operations may not yet be implemented.
We still use O(n^2) powerloss testing in test_dirs, just with a small
number of directories.
- It's tempting to try to store a full simulation on disk. But you
would quickly run into atomicity issues with the simulation itself.
Powerloss resilience is tricky!
- We can at least store a checksum in the files (currently just mod 26)
to check that the file itself was not corrupted. This doesn't protect
against swapped data though.
---
Also, a bit of a tangent, but I needed to add -Wno-format-overflow to
the test flags to avoid an annoying invalid format-overlow warning:
struct lfs_info info;
char name[256];
if (strlen(info.name) < 100) { // can't overflow!?
sprintf(name, "test/%s", info.name); // <--
}
warning: '%s' directive writing up to 255 bytes into a region of size
251 [-Wformat-overflow=]
This seems like a GCC bug, because as far as I can tell there is no way
to signal or hint that the size is in bounds without just disabling the
warning completely...
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%)
This replaces any remaining calls to lfsr_rbyd_appendattrs+appendcksum
with lfsr_rbyd_commit. At one point lfsr_rbyd_commit did a bit more
related to error recover, but these are equivalent now.
Because of the added complexity of bad prog alloc loops, reducing the
number of function calls in these cases is increasingly enticing.
This saves some code, and a surprising amount of stack!
code stack
before: 33618 2648
after: 33538 (-0.2%) 2624 (-0.9%)
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.
This sort of inverts the previous logic. Tests can still define
OPS='2*N' to scale the number of ops roughly with the number of entries,
but this fits better into the test framework, allows overriding, scaling
can be more easily tweaked, can be swapped out with a constant (like in
test_wl), etc.
Also tweaked some of the related N constants/filter conditions in tests
since these are now being effectively doubled... This should leave the
resulting number of ops unchanged.
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%)
The main difference being rendering the weight with a single letter "w"
prefix:
$ ./scripts/dbglfs.py disk -b4096
littlefs v0.0 4096x256 0x{1,0}.8b w2.512, rev eb7f2a0d
...
This lets us add valuable weight info without too much noise.
Adopting this in the dbg scripts is nice for consistency.
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%)
- rename -> mv
- remove -> rm
- general -> mvrm (room for more ops)
Easier to read, fewer characters. And we're already using these in
test_files/dirs, so we should prefer these for consistency.
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.
Because of course ternary operators would cause problems.
The two problem:
LFS_ASSERT((exists) ? !err : err == LFS_ERR_NOENT);
lfsr_file_sync(&lfs, &file) => (zombie) ? 0 : LFS_ERR_NOENT;
We could work around these with parentheses, but with different assert
parsers floating around this issue is likely to crop up again in the
future.
Fortunately this just required separate "sep" vs "term" rules and a bit
more strict parsing.
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.
Originally implemented in test_files, the DENSITY param sort of squishes
the files/dirs together, so random fuzzing is more likely to end up with
mkdir/rename collisions. These can be a bit more interesting for finding
weird corner cases.
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%)
The move to lfs_memcmp/lfs_strcmp highlighted an interesting hole in
prettyasserts.py: the lack of support for custom memcmp/strcmp symbols.
Rather than just adding more flags for an increasing number of symbols,
I've added -p/--prefix and -P/--prefix-insensitive to generate relevant
symbols based on a prefix. In littlefs's case, we use -Plfs_, which
matches both lfs_memcmp and LFS_ASSERT (and LFS_MEMCMP and lfs_assert
but ignore those):
$ ./scripts/prettyasserts.py -Plfs_ lfs.t.c -o lfs.t.a.c
Don't worry, you can still provide explicit symbols, but only via
long-form flags. This gets a bit noisy:
$ ./scripts/prettyasserts.py \
--assert=LFS_ASSERT \
--unreachable=LFS_UNREACHABLE \
--memcmp=lfs_memcmp \
--strcmp=lfs_strcmp \
lfs.t.c -o lfs.t.a.c
This commit also finally gives the prettyasserts.py's symbols actual
word boundaries, instead of the big error-prone hack of sorting by size.
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%)
This better matches test_dirs/test_files.
I guess the rule is singular for filesystem building blocks (test_rbyd,
test_btree, test_mtree, etc), plural for filesystem entries (test_dirs,
test_files, test_forphans, etc)?
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?
These seem fitting here, even if the test defines aren't "real defines".
The duplicate expressions should still be side-effect free and easy to
optimize out.
This should also avoid future lfs_min32 vs intmax_t issues.
When we introduced erased-state agnostic rbyd cksums, we added a cksums
to the dbg scripts since their cksums were actually useful now.
Unfortunately this missed the explicit/hacky Rbyd constructions in
dbglfs.py used to render file structs. Fixed by falling by using
cksum=0 in these cases.
