These don't really rely on any advanced file operations, and can run in
parallel.
This was a leftover from when test_incompat+test_compat were merged, and
test_compat should probably run after all file operations are thoroughly
tested.
Returning the actual on-disk file type is probably more useful for users
as this gives them more information.
I was originally concerned about collisions with future internal types,
LFS_TYPE_TRAVERSAL, etc, needed for internal opened-list tracking, but
it turns out we can avoid problems by starting internal types at 0x80,
since on-disk file types are only 7-bits.
Code changes:
code stack
before: 33710 2592
after: 33694 (-0.0%) 2592 (+0.0%)
This adds a couple things so our unknown file types don't just cause our
filesystem to fall over:
- lfsr_mount now prints a warning on any unknown file types found at
mount time. Since we're already iterating over all files to find
orphans, this is basically free.
- Added LFS_TYPE_UNKNOWN to represent files with an unknown/unsupported
type. This is now returned by lfsr_stat/lfsr_dir_read for files of any
unknow type.
- Added LFS_ERR_NOTSUP. This is now returned by functions that attempt
to modify a file of unknown type, and my have more use cases in the
future.
It's tempting to allow remove/rename on unknown file types, but since
we don't know what data structures these may be referencing, doing so
would likely leak storage. Or worse. Shrubs for example would just
explode if you only moved the metadata entry.
This also adds test_incompat_unknown to test these cases.
Code changes are minimal, though there are a number of extra conditions
to check for unknown file types. The lfsr_mount condition is
particularly fun as it should be completely optimized out when debug
statements are disabled:
code stack
before: 33670 2592
after: 33710 (+0.1%) 2592 (+0.0%)
Unlike the other test_compat tests, the test_incompat tests cover
specific corner cases and don't require any special linking. We probably
always want to run these, and keeping them merged with test_compat risks
the entire suite being omitted at some point.
The test_compat tests are a bit special and probably deserves a
dedicated test suite.
test_compat has been very useful for testing compatibility on patch and
minor releases.
Though, in porting the tests, I've realized these are actually really
flimsy w.r.t. API changes... lfsp_config notably relies on compatible
struct layouts, which is _not_ guaranteed by littlefs's compatibility
rules.
For this reason I've restricted these tests to only run if LFS_VERSION
doesn't change, though this may be worth reinvestigating in the future.
test_compat on minor API releases would be quite valuable...
lfsr_fs_stat is also not quite up to date with upstream yet. It's really
just a small shim copying over static configs at the moment (except for
name_limit/file_limit). This is because we're still missing most of what
would actually be interesting here: variable block counts, minor
versions, etc.
And of course a minimal lfsr_fs_stat means minimal code changes:
code stack
before: 33642 2592
after: 33670 (+0.1%) 2592 (+0.0%)
See comments/previous commits. lfsr_fs_mkconsistent allows running
internal consistency operations without any other filesystem changes.
Implementation-wize, this just calls lfsr_fs_preparemutation which we
already need to, uh, prepare for mutation. Though it may do some
additional work in the future, such as setting compat flags, version
numbers, etc.
Added mkconsistent permutations to what seems like the relevant tests:
- test_forphans - easy for lfsr_fs_mkconsistent to accidentally delete
orphans/zombies.
- test_powerloss - heavy fuzz tests over powerloss-related consistency
operations, though this does multiply every permutation by ~2x...
Code cost minimal. I guess this is what it costs to make an internal
function non-static:
code stack
before: 33634 2592
after: 33642 (+0.0%) 2592 (+0.0%)
Changed:
- lfsr_mkdir(&lfs, "/") => LFS_ERR_EXIST
- lfsr_file_open(&lfs, &file, "/", *) => LFS_ERR_ISDIR
Unchanged:
- lfsr_remove(&lfs, "/") => LFS_ERR_INVAL
- lfsr_rename(&lfs, "/", *) => LFS_ERR_INVAL
- lfsr_rename(&lfs, *, "/") => LFS_ERR_INVAL
This better matches what Linux, etc, does: prefering a normal
dir-related error unless the only issue is that the dir in question is
the root.
Though Linux, etc, usually return EBUSY, which seems to also be used for
special device files. We could add LFS_ERR_BUSY, but I'm not sure it's
really worth it for such a rare error. It's not like the name would help
anything...
Internally, lfsr_mtree_pathlookup always returns LFS_ERR_INVAL for root,
so this unfortunately requires a bit more code to map to the correct
errors:
code stack
before: 33598 2592
after 33634 (+0.1%) 2592 (+0.0%)
Now that we are testing more powerloss behaviors, test_powerloss is the
longest running test suite by a decent margin:
Before:
$ ./scripts/summary.py test.csv -bsuite -ftime -Stime
... snip ...
test_rbyd 578.6
test_fwrite 984.5
test_badblocks 1341.5
test_exhaustion 1648.3
test_powerloss 2192.3 <--
TOTAL 7378.6
$ ./scripts/summary.py test.csv -bcase -ftime -Stime
... snip ...
test_fwrite_fuzz_aligned 247.2
test_exhaustion_file_fuzz 287.7
test_exhaustion_dir_fuzz 307.2
test_exhaustion_orphanzombie_fuzz 389.1
test_exhaustion_orphanzombiedir_fuzz 531.5
test_powerloss_file_pl_fuzz 787.7 <--
test_badblocks_single_dir_many 840.2
test_powerloss_filedir_pl_fuzz 1366.7 <--
TOTAL 7378.6
But testing more things is better than testing the same thing more.
Worst case you can always manually override OPS, -DOPS=1024, if you have
CI cycles to spare. Though note with our linear powerloss heuristic,
the tail end of long running tests also recieves fewer powerlosses,
which reduces the usefulness of running these tests longer.
These *_pl_fuzz tests also now match the default number of OPS in
test_relocations.
These emulate powerloss behavior where only some of the bits being
progged are actually progged if there is a powerloss. This behavior was
the original motivation for our ecksums/fcrcs, so it's good to have this
tested.
As a simplification, these only test the extremes:
- LFS_EMUBD_POWERLOSS_SOMEBITS => one bit progged
- LFS_EMUBD_POWERLOSS_MOSTBITS => all-but-one bit progged
Also they flips bits instead of preserving exact partial prog behavior,
but this is allowed (progs can have any intermediate value), has the
same effect as partial progs, and should encourage failed progs.
This required a number of tweaks in emubd: moved powerloss before prog,
moved mutate after powerloss, etc, but these shouldn't affect other
powerloss behaviors. Handling powerloss after prog was only to avoid
power_cycles=1 being useless, it's not strictly required.
Good news is testing so far suggests our ecksum design is sound.
It's counter-intuitive, but no top-level API should return
LFS_ERR_CORRUPT. Instead, if we can't make progress because of a corrupt
block, we should return LFS_ERR_NOSPC. This makes it easier for users to
write code that is well behaved even when a device is end-of-life.
It's up to our mroot extension algorithm to make sure this case can't be
reached in normal operation unless the device is _actually_ at
end-of-life.
Because mroot extension is a bit of a special case, we weren't
converting these corrupt errors to nospc errors consistently. This is
fixed now, along with a couple more hopefully-useful logging statements.
Found while playing around with test_exhaustion + block_recycles=-1.
This should assert on bad wear-leveling, but LFS_ERR_CORRUPT was
unexpected. Added an explicit test because this is an easy thing to let
split through:
- test_badblocks_mrootanchor_wear
Code changes were surprisingly minimal, I wonder if constants are being
swapped out somewhere low-level?
code stack
before: 33766 2600
after: 33770 (+0.0%) 2600 (+0.0%)
The main test additions are the test_powerloss tests, intended to be
high-level tests over difficult/weird powerloss environments (such as
out-of-order writes!):
- test_powerloss_dir_many - 2242 pls
- test_powerloss_file_many - 8856 pls
- test_powerloss_file_pl_fuzz - 384508 pls
- test_powerloss_filedir_pl_fuzz - 268339 pls
But there was also a bunch of other test movement in the late-stage/
high-level tests. I'm trying to keep the core of these tests somewhat
consistent so we have a nice template to extend for future testing, in
case we want to test other environmentalish concerns, but not all of
these tests make sense in all of these contexts:
badblocks powerloss relocations exhaustion
dir_many y y y
dir_fuzz y y y
file_many y y y
file_fuzz y y y
fwrite_fuzz y y
orphanzombie_fuzz y y y
orphanzombiedir_fuzz y y y
file_pl_fuzz y y
filedir_pl_fuzz y y
Why not:
- dir/file_many+exhaustion? - Needs to be unbounded
- dir/file_fuzz+powerloss? - Takes O(n^2)
- fwrite_fuzz+powerloss? - Takes O(n^2)
- fwrite_fuzz+relocations? - Doesn't really test anything
- orphanzombie*_fuzz+powerloss? - Powerloss kills zombies
- file*_pl_fuzz+badblocks? - PL + Badblocks currently incompactible
- file*_pl_fuzz+exhaustion? - PL + Badblocks currently incompactible
---
Of course, in order to actually get out-of-order write testing working,
we need to implement out-of-order write syncing.
Fortunately this was a simple exercise in placing lfsr_bd_sync calls
before any mdir commits where we may have unsynced data:
- in lfsr_file_sync, to sync any pending file data
- in lfsr_mdir_commit, to sync any mroot/mtree changes
We also call lfsr_bd_sync _after_ mdir commits in case users expect to
sequence any filesystem-external operations such as network, UI, etc. In
theory this could be optional, but no users have really requested it
yet, so leave that for a potential future improvement:
- in lfsr_mdir_commit
- in lfsr_formatinited (really just because we don't go through
lfsr_mdir_commit)
Note that lfsr_rbyd_commit has been relaxed in the scheme. It only
flushes caches, and does _not_ call lfsr_bd_sync. This is useful for
allowing multiple B-tree nodes to be written out-of-order, also long as
the whole thing is synchronized before any mdir commit.
All of these lfsr_bd_sync calls add a bit of code, but not really an
amount to care about:
code stack
before: 33678 2600
after: 33766 (+0.3%) 2600 (+0.0%)
This sort of reverts the addition of lfsr_bd_unprog, but with a slightly
better API. lfsr_bd_unprog was too much of a hack, and isn't really
generalizable. The align flag isn't necessarily any better, but at least
it's the simplest/least-confusing solution available.
And it's net savings, code-wise:
code stack lfs_t
before: 33690 2608 164
after: 33678 (-0.0%) 2600 (-0.3%) 160 (-2.4%)
While exploring the test_badblocks ERASENOOP failure more, I realized
the problem is that we are nesting crc32cs.
To be clear, using crc32cs to validate progs in general is not an issue,
that is perfectly fine on paper. The issue is that we were using crc32cs
to validate progs _that contain crc32cs_.
Looking at the collision, we can see the fully expanded lleb128s we use
for our cksum tags:
00 00 00 ff b0 02 00 87 80 80 00 3e c0 7f 7e => bdfa9b10
ab 77 de c2 b0 03 00 87 80 80 00 3e 38 d5 22 => bdfa9b10
'-.-' ^ '----.----' '----.----'
'----|------|-----------|-- cksum tag
'------|-----------|-- cksum weight (0)
'-----------|-- cksum size + padding
'-- cksum crc32c
So we ended up perfectly aligning the cksum's crc32c with our cache
line. Lucky us.
Unfortunately funny math makes it so that whenever a crc32c contains a
crc32c, the inner crc32c sort of cancels itself out from the outer
crc32c. So these two messages end up mathematically equivalent, even
though they contain different data:
crc(m) = m(x) x^|P|-1 mod P
crc(m ++ crc(m)) = (m(x) x^|P|-1 + (m(x) x^|P|-1 mod P)) x^|P|-1 mod P
crc(m ++ crc(m)) = (m(x) x^|P|-1 + m(x) x^|P|-1) x^|P|-1 mod P
crc(m ++ crc(m)) = 0 x^|P|-1 mod P
crc(m ++ crc(m)) = 0
So using a crc32c to check progs is not fit for purpose.
This leaves us with a couple options:
1. Use a different checksum, or do something like rearranging bytes to
avoid this cancelling out issue. Unfortunately this gets tricky since
crc32cs are linear, simply using an xor mask won't work...
2. Don't check progs at such a low-level, but at a high-level using the
rbyd/data block crc32cs. Since this would mean only one crc32c, this
would avoid nesting issues. Unfortunately this would probably come
with quite a high code cost to try to keep track of both the
before+after rbyd cksums everywhere...
3. Just read back the data into the rcache to compare at the byte-level,
which would mean clobbering our rcache when prog checking is enabled.
This commit goes with option 3., which is probably the simplest. It also
removes any question of crc32c collision, which could be a real nuisance
when debugging low-level block device operations, a use case where prog
checking will hopefully be quite valuable.
Clobbering the rcache also has the advantage of reverting the prog
>= read requirement, which is nice for flexibility. Though this needs to
be tested.
---
There was a bit of a hiccup, and that was how prog checking interacts
with lfsr_bd_cpy. lfsr_bd_cpy used the rcache to hold data being copied
to/from disk, but this data needs to be checked, and prog checking would
clobber the rcache. Problems! I guess this is one footgun of the
internal lfsr_bd_readnext API...
The solution is to instead turn this around and use the pcache to hold
any copied data, since this would not be clobbered when prog checking.
This has some other knock-on effects, mainly that we can't take
advantage of read hints in lfsr_bd_cpy, but has the added advantage of
potentially not clobbering the rcache at all when no checking progs.
Code changes were fairly minimal:
code stack
before: 33718 2608
after: 33690 (-0.1%) 2608 (+0.0%)
The initial goal was the simplify these layers. Keyword being initial.
Unfortunately these layers are both complex and subtle, so the goal
shifted more to be rigorous and reliable.
This mainly meant rearranging our prog/read loops to follow a consistent
style, with higher-priority buffers being sorted out before flushing
things. This gets a bit tricky with wanting to support both cache
bypassing and buffer-lending prognext/readnext, but with some redundant
prognext/readnext calls it's doable.
We also now aggressively discard rcaches on pcache conflicts. This
change does rely on the prog >= read assumption. Discarding rcaches
means we should no longer have overlapping caches, so hopefully no more
zombie rcache issues.
Our bypassing heuristic was also tweaked a bit. Now, in addition to
alignment, >= read/prog_size, and >= hint requirements, we also require
operations to be >= r/pcache_size. This should improve cache usage when
r/pcache_size >> read/prog_size, since we were too eager to bypass
before.
Long story short, this ended up being more just things shifting around
than a significant simplification of the bd layers. At least we ended up
with a nice bit of stack savings:
code stack
before: 33682 2640
after: 33718 (+0.1%) 2608 (-1.2%)
Also, test_badblocks with LFS_EMUBD_BADBLOCK_ERASENOOP is now failing. I
was worried the amount of fuzz testing we do would eventually end up
with a naturally occuring crc32c collision, and sure enough we did! Yayy
yyyy...
00 00 00 ff b0 02 00 87 80 80 00 3e c0 7f 7e => bdfa9b10
ab 77 de c2 b0 03 00 87 80 80 00 3e 38 d5 22 => bdfa9b10
Need to think about what to do with this... For now I've just commented
out the problematic test.
This configuration option enables the previous behavior of reading back
every prog to check that the data was written correctly.
Unfortunately, this brings a bit of baggage, thanks to our cache
interactions being more complicated now:
- We really want to reuse the rcache for prog validation, despite the
cache performance implications. Unfortunately, we simply can't, thanks
to the new bd utility functions tying up the rcache. lfsr_bd_cpy, for
example, does not expect rcache to be invalidated between a read and
prog, and if it is, things break (I may or may not have found this by
experience).
These bd utilities are valuable, so we really need some other way to
validate our progs.
- Since we can't rely on the rcache, this leaves checksumming as the
only option for validating progs. Checksumming isn't perfect, as there
is a decent chance of false negatives, but to be honest it's probably
good enough for anything that's not malicious.
- This also adds the new constraint that we need to be able to read back
any prog into the pcache, which implies read_size <= prog_size. This
constraint didn't exist when we could clobber our rcache, but this is
not worth throwing away the new bd utilities. Not to mention
clobbering our rcache could hurt cache performance.
Why not make read_size <= prog_size conditional on check_progs?
The main reason is convenience. One very compelling use case for
check_progs is to help debug unknown filesystem/integration failures,
buf if you can't enable check_progs without changing the filesystem
configuration, you can't really rely on check_progs for debugging.
This helps future proof what we expect from block devices, in case
future error detection/correction mechanisms can benefit from our
prog_size always being readable.
Code changes were not that significant, however there was a surprising
stack cost. This seems to be because lfsr_bd_read__ can now be called
from multiple places, causing it to no longer be inlined in
lfsr_bd_read_, costing a bit of stack for the additional function call:
before: 33566 2624
after: 33682 (+0.3%) 2640 (+0.6%)
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).
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%)
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 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.
- 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.
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...
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 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%)
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)?
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.
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.
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.
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%)
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.
So now, instead of one data type trying to do everything, we have two:
1. lfsr_data_t - Readable data, either in-RAM or on-disk
2. lfsr_cat_t - Concatenated data for progging, may be either a simple
in-RAM buffer or an indirect list of lfsr_data_ts
This comes from an observation that most lfsr_attr_t datas were either
simple buffers, NULL, or required the indirect concatenated datas
anyways (concatendated file fragments). By separating lfsr_cat_t and
lfsr_data_t, maybe we can save RAM in lfsr_attr_t by not needing the
three words necessary for the less-common disk references.
Note the interesting tradeoff:
Simple in-RAM buffers/NULL decrease by 1 word (4 bytes):
lfsr_data_t lfsr_cat_t
.---+---+---+---. .---+---+---+---.
|0| size | => |0| size |
+---+---+---+---+ +---+---+---+---+
| ptr | | ptr |
+---+---+---+---+ '---+---+---+---'
| (unused) |
'---+---+---+---'
'-------.-------' '-------.-------'
12 bytes 8 bytes
While on-disk references increase by 2 words (8 bytes):
lfsr_data_t lfsr_cat_t lfsr_data_t
.---+---+---+---. .---+---+---+---. .---+---+---+---.
|1| size | => |1| size | .>|1| size |
+---+---+---+---+ +---+---+---+---+ | +---+---+---+---+
| block | | ptr -------' | block |
+---+---+---+---+ '---+---+---+---' +---+---+---+---+
| off | | off |
'---+---+---+---' '---+---+---+---'
'-------.-------' '-----------------.-----------------'
12 bytes 20 bytes
Unless the on-disk references also need concatenation, in which case
this still saves 1 word (4 bytes).
Note I'm not sure this type split is generalizable to other systems. In
littlefs we can't use recursion, so progging concatenated datas already
required two nested functions, and we happen to never need to read
concatenated data, allowing us to completely omit that functionality. In
other systems, where maybe disk-reference attrs are more common, this
tradeoff may not make sense.
Some other things to note:
- We're also losing the inlined-data representation in this change.
Unfortunately earlier lfsr_data_t measurements showed that this didn't
really contribute much. It saved RAM in name attrs but added quite a
bit of complexity to lfsr_data_t operations.
- By separating simple/cat and RAM/disk, we reduce the abused size bits
from 2-bits down to 1-bit. This doesn't really matter for our current
31/28-bit littlefs impl, but is nice in that it reenables the
theoretical 31/31-bit littlefs impl without in-RAM data-structure
changes.
There are a few temporary hacks that need to be figured out, but this is
already showing code/stack savings. Which is fascinating considering the
new lfsr_cat_* functions and increased temporary allocations:
code stack
before: 33856 2824
after: 33812 (-0.1%) 2800 (-0.8%)
Now that reproducibility issues with erase_value=-1 (erase=noop) are
fixed, this much more useful to test than erase_value=0x1b. Especially
since erase=noop is filled with so many sharp corners.
These tests already found that we were being too confident with our
leb128/lleb128/tag parsing. Since we need to partially parse unfinished/
old commits, lfsr_dir_read* can easily encounter invalid leb128s during
normal operation. If this happens we should not assert.
Doing things correctly has a bit of a cost:
code stack
before: 33928 2824
after: 33976 (+0.1%) 2824 (+0.0%)
At least we haven't seen any issues with our valid bit invalidating
logic yet.
Long story short, rbyd checksums are now fully reproducible. If you
write the same set of tags to any block, you will end up with the same
checksum.
This is actually a bit tricky with littlefs's constraints.
---
The main problem boils down to erased-state. littlefs has a fairly
flexible model for erased-state, and this brings some challenges. In
littlefs, storage goes through 2 states:
1. Erase - Prepare storage for progging. Reads after an erase may return
arbitrary, but consistent, values.
2. Prog - Program storage with data. Storage must be erased and no progs
attempted. Reads after a prog must return the new data.
Note in this model erased-state may not be all 0xffs, though it likely
will be for flash. This allows littlefs to support a wide range of
other storage devices: SD, RAM, NVRAM, encryption, ECC, etc.
But this model also means erased-state may be different from block to
block, and even different on later erases of the same block.
And if that wasn't enough of a challenge, _erased-state can contain
perfectly valid commits_. Usually you can expect arbitrary valid cksums
to be rare, but thanks to SD, RAM, etc, modeling erase as a noop, valid
cksums in erased-state is actually very common.
So how do we manage erased-state in our rbyds?
First we need some way to detect it, since we can't prog if we're not
erased. This is accomplished by the forward-looking erased-state cksum
(ecksum):
.---+---+---+---. \
| commit | |
| | |
| | |
+---+---+---+---+ +-.
| ecksum -------. | | <-- ecksum - cksum of erased state
+---+---+---+---+ | / |
| cksum --------|---' <-- cksum - cksum of commit,
+---+---+---+---+ | including ecksum
| padding | |
| | |
+---+---+---+---+ \ |
| erased | +-'
| | /
. .
. .
You may have already noticed the start of our problems. The ecksum
contains the erased-state, which is different per-block, and our rbyd
cksum contains the ecksum. We need to include the ecksum so we know if
it's valid, but this means our rbyd cksum changes block to block.
Solving this is simple enough: Stop the rbyd's canonical cksum before
the ecksum, but include the ecksum in the actual cksum we write to disk.
Future commits will need to start from the canonical cksum, so the old
ecksum won't be included in new commits, but this shouldn't be a
problem:
.---+---+---+---. . . \ . \ . . . . .---+---+---+---. \ \
| commit | | | | commit | | |
| | | +- rbyd | | | |
| | | | cksum | | | |
+---+---+---+---+ +-. / +---+---+---+---+ | |
| ecksum -------. | | | ecksum | . .
+---+---+---+---+ | / | +---+---+---+---+ . .
| cksum --------|---' | cksum | . .
+---+---+---+---+ | +---+---+---+---+ . .
| padding | | | padding | . .
| | | | | . .
+---+---+---+---+ \ | . . . . . . . +---+---+---+---+ | |
| erased | +-' | commit | | |
| | / | | | +- rbyd
. . | | | | cksum
. . +---+---+---+---+ +-. /
| ecksum -------. | |
+---+---+---+---+ | / |
| cksum ------------'
+---+---+---+---+ |
| padding | |
| | |
+---+---+---+---+ \ |
| erased | +-'
| | /
. .
. .
The second challenge is the pesky possibility of existing valid commits.
We need some way to ensure that erased-state following a commit does not
accidentally contain a valid old commit.
This is where are tag's valid bits come into play: The valid bit of each
tag must match the parity of all preceding tags (equivalent to the
parity of the crc32c), and we can use some perturb bits in the cksum tag
to make sure any tags in our erased-state do _not_ match:
.---+---+---+---. \ . . . . . .---+---+---+---. \ \ \
|v| tag | | |v| tag | | | |
+---+---+---+---+ | +---+---+---+---+ | | |
| commit | | | commit | | | |
| | | | | | | |
+---+---+---+---+ +-----. +---+---+---+---+ +-. | |
|v|p| tag | | | |v|p| tag | | | | |
+---+---+---+---+ / | +---+---+---+---+ / | | |
| cksum | | | cksum | | . .
+---+---+---+---+ | +---+---+---+---+ | . .
| padding | | | padding | | . .
| | | | | | . .
+---+---+---+---+ . . . | . . +---+---+---+---+ | | |
|v---------------- != --' |v------------------' | |
| erased | +---+---+---+---+ | |
. . | commit | | |
. . | | | |
+---+---+---+---+ +-. +-.
|v|p| tag | | | | |
+---+---+---+---+ / | / |
| cksum ----------------'
+---+---+---+---+ |
| padding | |
| | |
+---+---+---+---+ |
|v---------------- != --'
| erased |
. .
. .
New problem! The rbyd cksum contains the valid bits, which contain the
perturb bits, which depends on the erased-state!
And you can't just derive the valid bits from the rbyd's canonical
cksum. This avoids erased-state poisoning, sure, but then nothing in the
new commit depends on the perturb bits! The catch-22 here is that we
need the valid bits to both depend on, and ignore, the erased-state
poisoned perturb bits.
As far as I can tell, the only way around this is to make the rybd's
canonical cksum not include the parity bits. Which is annoying, masking
out bits is not great for bulk cksum calculation...
But this does solve our problem:
.---+---+---+---. \ . . . . . .---+---+---+---. \ \ \ \
|v| tag | | |v| tag | | | o o
+---+---+---+---+ | +---+---+---+---+ | | | |
| commit | | | commit | | | | |
| | | | | | | | |
+---+---+---+---+ +-----. +---+---+---+---+ +-. | | |
|v|p| tag | | | |v|p| tag | | | | . .
+---+---+---+---+ / | +---+---+---+---+ / | | . .
| cksum | | | cksum | | . . .
+---+---+---+---+ | +---+---+---+---+ | . . .
| padding | | | padding | | . . .
| | | | | | . . .
+---+---+---+---+ . . . | . . +---+---+---+---+ | | | |
|v---------------- != --' |v------------------' | o o
| erased | +---+---+---+---+ | | |
. . | commit | | | +- rbyd
. . | | | | | cksum
+---+---+---+---+ +-. +-. /
|v|p| tag | | | o |
+---+---+---+---+ / | / |
| cksum ----------------'
+---+---+---+---+ |
| padding | |
| | |
+---+---+---+---+ |
|v---------------- != --'
| erased |
. .
. .
Note that because each commit's cksum derives from the canonical cksum,
the valid bits and commit cksums no longer contain the same data, so our
parity(m) = parity(crc32c(m)) trick no longer works.
However our crc32c still does tell us a bit about each tag's parity, so
with a couple well-placed xors we can at least avoid needing two
parallel calculations:
cksum' = crc32c(cksum, m)
valid' = parity(cksum' xor cksum) xor valid
This also means our commit cksums don't include any information about
the valid bits, since we mask these out before cksum calculation. Which
is a bit concerning, but as far as I can tell not a real problem.
---
An alternative design would be to just keep track of two cksums: A
commit cksum and a canonical cksum.
This would be much simpler, but would also require storing two cksums in
RAM in our lfsr_rbyd_t struct. A bit annoying for our 4-byte crc32cs,
and a bit more than a bit annoying for hypothetical 32-byte sha256s.
It's also not entirely clear how you would update both crc32cs
efficiently. There is a way to xor out the initial state before each
tag, but I think it would still require O(n) cycles of crc32c
calculation...
As it is, the extra bit needed to keep track of commit parity is easy
enough to sneak into some unused sign bits in our lfsr_rbyd_t struct.
---
I've also gone ahead and mixed in the current commit parity into our
cksum's perturb bits, so the commit cksum at least contains _some_
information about the previous parity.
But it's not entirely clear this actually adds anything. Our perturb
bits aren't _required_ to reflect the commit parity, so a very unlucky
power-loss could in theory still make a cksum valid for the wrong
parity.
At least this situation will be caught by later valid bits...
I've also carved out a tag encoding, LFSR_TAG_PERTURB, solely for adding
more perturb bits to commit cksums:
LFSR_TAG_CKSUM 0x3cpp v-11 cccc -ppp pppp
LFSR_TAG_CKSUM 0x30pp v-11 ---- -ppp pppp
LFSR_TAG_PERTURB 0x3100 v-11 ---1 ---- ----
LFSR_TAG_ECKSUM 0x3200 v-11 --1- ---- ----
LFSR_TAG_GCKSUMDELTA+ 0x3300 v-11 --11 ---- ----
+ Planned
This allows for more than 7 perturb bits, and could even mix in the
entire previous commit cksum, if we ever think that is worth the RAM
tradeoff.
LFSR_TAG_PERTURB also has the advantage that it is validated by the
cksum tag's valid bit before being included in the commit cksum, which
indirectly includes the current commit parity. We may eventually want to
use this instead of the cksum tag's perturb bits for this reason, but
right now I'm not sure this tiny bit of extra safety is worth the
minimum 5-byte per commit overhead...
Note if you want perturb bits that are also included in the rbyd's
canonical cksum, you can just use an LFSR_TAG_SHRUBDATA tag. Or any
unreferenced shrub tag really.
---
All of these changes required a decent amount of code, I think mostly
just to keep track of the parity bit. But the isolation of rbyd cksums
from erased-state is necessary for several future-planned features:
code stack
before: 33564 2816
after: 33916 (+1.0%) 2824 (+0.3%)
This plays better with prettyasserts.py, which prints the err value on
failure.
We _could_ extend prettyasserts.py to print the contents of !err
patterns, but this risks making the error message more confusing when
the target is an actual boolean expression. Keep in mind
prettyasserts.py is purely syntactical and doesn't really know the
expression's type.
Christopher Haster