bb3ef46cdf81288bfd7aa536503cbed6904b0312
475 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Christopher Haster
|
1ecb346cec | Renamed fbuffer_size -> file_buffer_size | ||
|
Christopher Haster
|
50fc0ed680 |
Replaced lfsr_bd_unprog with an align flag in each prog function
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%)
|
||
|
Christopher Haster
|
8e77a5eebc |
Switched to clobbering rcache for prog checking
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%)
|
||
|
Christopher Haster
|
89f7f98fba |
Reworked bd layers with prog >= read assumption
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.
|
||
|
Christopher Haster
|
c648f96dc5 |
Added check_progs for immediate prog validation
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%) |
||
|
Christopher Haster
|
9c9a409524 |
Added fuzz test attribute
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). |
||
|
Christopher Haster
|
1c363b428a |
Replaced REMOUNT with small post-test loops where possible
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. |
||
|
Christopher Haster
|
76d3c49b5c |
Aligned block_recycles in tests to powers-of-2
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. |
||
|
Christopher Haster
|
94761c14b6 |
Dropped block_recycles=-1 from relocation tests
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. |
||
|
Christopher Haster
|
1962046ea5 |
Broke out test_relocations_wl_* -> test_exhaustion_*
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 |
||
|
Christopher Haster
|
61c03cf275 |
Added wear-leveling litmus tests, test_relocations_wl_*
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...
|
||
|
Christopher Haster
|
3b33c33339 |
Added pseudo-stateless *_pl_fuzz 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...
|
||
|
Christopher Haster
|
7e62ebe18a | Renamed test_wl -> test_relocations | ||
|
Christopher Haster
|
5d03416c82 |
Added LFSR_BTREE/SHRUB_NULL, dropped lfsr_btree_alloc
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%)
|
||
|
Christopher Haster
|
224bd8984b |
Removed all test_btree LFS_ERR_NOSPC exceptions
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. |
||
|
Christopher Haster
|
15da817af5 |
Replace fuzz DENSITY with explicit OPS in tests
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. |
||
|
Christopher Haster
|
fe11a33416 |
(Re)implemented bad prog/erase recovery
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%)
|
||
|
Christopher Haster
|
081a74cb23 |
Replaced mleafweight with explicit 1 << mdir_bits
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. |
||
|
Christopher Haster
|
99e5fb87e0 |
Prefer mv/rm in tests over files
- 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. |
||
|
Christopher Haster
|
acd41c5664 | Renamed test_forphans test name from "gello" -> "batman" | ||
|
Christopher Haster
|
25c7831417 |
Fixed clobbered shrubs after renaming over an mdir split
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...
|
||
|
Christopher Haster
|
e4fe2b5234 |
Shifted block_recycles so 0 => pure copy-on-write
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. |
||
|
Christopher Haster
|
4d76551d6b |
Fixed parse errors in prettyasserts.py caused by ternary operators
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. |
||
|
Christopher Haster
|
ba81a2bcc9 |
Added test_wl and aggressive orphan/zombie fuzz tests
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. |
||
|
Christopher Haster
|
b3feeea385 |
Adopted DENSITY param in test_dirs fuzz tests
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. |
||
|
Christopher Haster
|
56b18dfd9a |
Reworked revision count logic a bit, block_cycles -> block_recycles
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%)
|
||
|
Christopher Haster
|
786dbbf998 |
Reworked gstate/commit interactions
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%)
|
||
|
Christopher Haster
|
f307892b32 |
Renamed test_forphan -> test_forphans
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)? |
||
|
Christopher Haster
|
aa1d2f0cf9 |
Dropped lfsr_dir_t's bookmark mdir, switched to did for dir updates
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.
|
||
|
Christopher Haster
|
5c70013c11 |
Adopted compile-time LFS_MIN/LFS_MAX in test defines
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. |
||
|
Christopher Haster
|
186fd1b5f2 |
Separated cache_size out into rcache_size/pcache_size/fbuffer_size
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.
|
||
|
Christopher Haster
|
bd4a5e5ab3 |
Tried to better budget test runtime
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%) |
||
|
Christopher Haster
|
cd22c0d68b |
Aggressively cleaned up/reworked lfsr_attr_t, consumed lfsr_cat_t
This turned into a sort of system-wide refactor based on learned
knowledge of what we can do with lfsr_attr_t.
The big changes:
- Reverted LFSR_ATTR to mainly take lfsr_data_t again, keeping
lfsr_data_t as the default data representation in the codebase.
Now that we know
LFSR_ATTR_CAT_ still provides concatenation mechanics, and LFSR_ATTR_
provides a way to edit in-flight lfsr_attr_ts.
- Dropped lfsr_cat_t, replaced with explicit const void* + uint16_t,
tried to limit to low-level operations and prefer passing aroud
lfsr_attr_t and lfsr_data_t at a high-level.
Note this cat + cat_count pair is quite similar to the common attrs +
attr_count and buffer + size arguments.
- Adopted lfsr_attr_t more in mid-level functions, lfsr_rbyd_appendattr,
lfsr_rbyd_appendcompactattr, lfsr_file_carve, etc. This is a bit more
ergonomical, allows for use of LFSR_ATTR* macros, and in theory might
even save a bit of stack.
Unfortunately this seems to have resulted in a net hit to code cost,
though I still think it's worth it for the internal ergonomics:
code stack
before: 33652 2624
after: 33780 (+0.4%) 2640 (+0.4%)
Investigating further suggests this may just be the result of compiler
noise and changes to argument placement. lfsr_attr_t does touch a lot of
code...
It's interesting to note the adoption of lfsr_attr_t in
lfsr_rbyd_appendattr* and friends prevents their transformation into
.isra functions, though this doesn't seem to impact code cost too much:
function (5 added, 5 removed) osize nsize dsize
lfsr_cat_size - 48 +48 (+100.0%)
lfsr_file_carve - 1600 +1600 (+100.0%)
lfsr_rbyd_appendattr - 2120 +2120 (+100.0%)
lfsr_rbyd_appendattr_ - 244 +244 (+100.0%)
lfsr_rbyd_appendcompactattr - 68 +68 (+100.0%)
lfsr_rbyd_appendcompactrbyd 144 152 +8 (+5.6%)
lfsr_file_truncate 298 314 +16 (+5.4%)
lfsr_mdir_commit__ 1056 1112 +56 (+5.3%)
lfsr_mdir_compact__ 502 526 +24 (+4.8%)
lfsr_rbyd_appendattrs 132 138 +6 (+4.5%)
lfsr_file_fruncate 386 402 +16 (+4.1%)
lfsr_data_frombtree 84 86 +2 (+2.4%)
lfsr_rbyd_appendcksum 512 520 +8 (+1.6%)
lfsr_file_opencfg 572 580 +8 (+1.4%)
lfsr_rename 608 616 +8 (+1.3%)
lfsr_mkdir 500 504 +4 (+0.8%)
lfsr_bd_prog 278 280 +2 (+0.7%)
lfsr_mdir_commit 2364 2360 -4 (-0.2%)
lfsr_bshrub_commit 716 712 -4 (-0.6%)
lfsr_file_sync 526 514 -12 (-2.3%)
lfsr_file_flush_ 1868 1820 -48 (-2.6%)
lfsr_remove 456 436 -20 (-4.4%)
lfsr_fs_fixgrm 168 160 -8 (-4.8%)
lfsr_cat_size.isra.0 42 - -42 (-100.0%)
lfsr_file_carve.isra.0 1596 - -1596 (-100.0%)
lfsr_rbyd_appendattr.isra.0 2088 - -2088 (-100.0%)
lfsr_rbyd_appendattr_.isra.0 232 - -232 (-100.0%)
lfsr_rbyd_appendcompactattr.isra.0 56 - -56 (-100.0%)
TOTAL 33652 33780 +128 (+0.4%)
|
||
|
Christopher Haster
|
d11106a898 |
Extended LFSR_CAT_* -> LFSR_cat_*_ for implicit/explicit memory
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. |
||
|
Christopher Haster
|
88a098c616 |
Added lfsr_cat_t to represent concatenated data
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%)
|
||
|
Christopher Haster
|
ab2a1cb571 |
Enabled erase=noop in test_rbyd, changed read* to error on leb128 overflow
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.
|
||
|
Christopher Haster
|
8a75a68d8b |
Made rbyd cksums erased-state agnostic
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%)
|
||
|
Christopher Haster
|
9b4e1b4cb7 |
Replace assert(!err) with assert(err == 0) in tests
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
|
0a89d0c254 |
Fixed recoloring tail-recursion violations during range removals
I spoke too soon and made a mistake when reenabling color preservation
during range removals.
I assumed, that thanks to replacing the diverging alt with a new black
alt for stitching together diverging trunks, we would avoid the issue
where a deleted diverging alt violates our rbyd's tail-recursive
recoloring invariant.
Unfortunately, this is not the case. All the stitching alt did was make
this violation more difficult to reach, but still reachable. Arguable a
worse situation.
Now, for this violation to happen, in addition to all of the other
requirements, we need the lower-diverging trunk to become empty.
This is the only case where we have no stitching alt, because we don't
need to stitch an empty trunk. Which means if the upper-diverging trunk
has yellow nodes both before and after the diverging alt, our
tail-recursive recoloring invariant can break.
Here's an example:
.-------------r-------------.
.-o-. .---+---y----. .-o-.
.o. .o. .o. .o. .o. .-y-+-. .o. .o.
a a a a a a a a c c c e e e e e e e
'--+--'
remove
Again, this doesn't capture the alt-layout, which _is_ important, so
here's the dbgrbyd.py view:
.-> aa .-> aa
.-b-> a .-b-> a
| .-> a | .-> a
.-----------b-b-> a .-------b-b-> a
| .-> a | .-> a
| .---------b-> a | .-b-> a
| | .-> a | | .-> a
| | .-------b-> a | .-b-b-> a
r-b-y-r-b-----b-> cc -. => y-y-r-b-----> ee <- two yellows!
| | '-> c + rm | '-----b-> e different dirs!
| | .-> c -' | '-> e should not happen!
| '-y-r-b-> ee | .-> e
| | '---> e | .-b-> e
| '-----> e | | .-> e
| .-> e '-----b-b-> e
| .-b-> e
| | .-> e
'---------b-b-> e
And the steps in our appendattr algorithm that led to this state, which
is insightful:
read <r => [<r]
read >b => [<r >b]
read <r => [<r >b <r]
read <r => [<r >b <r <r]
^--^------ red + red implies yellow
ysplit => [<r >r <b]
reorder => [<r <r >b]
^--^------------- yellow-same-dir invariant held
read >b => [<r <r >b >b]
diverge => [<r <r >b]
read >r => [<r <r >b >r]
read >r => <r [<r >b >r >r]
^-----------^-- our 4-alt fifo for flips/coloring
ysplit => <r [<r >r >b]
reorder => <r [>r >r <b]
^--^---------- yellow-same-dir invariant held
^---^------------- yellow-same-dir invariant NOT held
though 2 yellows is also a problem
The previous commit fixing this bug for the one-pass algorithm may also
be useful.
This tree is now tested in test_rbyd_delete_range_rydye and
test_rbyd_delete_range_rydye_backwards, though only
test_rbyd_delete_range_rydye_backwards reveals the bug, since the bug
requires _specifically_ the lower-diverging trunk to become empty (both
rydy and rydye now have in-order and backwards tests in case of other
chirality issues).
---
Taking a step back, and looking at this bug from a higher-level, the
core of the issue is that we are somewhat arbitrarily deleting nodes
after splitting nodes. This can break our tail-recursive recoloring
invariant.
What the heck is our tail-recursive recoloring invariant?
This is a property of 2-3-4 and greater B-trees, and transitively
red-black and red-black-yellow trees, that allows for tail-recursive,
self-balancing node insertion.
Basically, if you eagerly split any 4-nodes you encounter as you descend
down the tree, you will always be guaranteed to have an open slot in
your parent, so pushing up split nodes (or recoloring) only ever
propagates up a single level:
.-----. .-------. .-------.
|.a.h.| |.a.c.h.| |.a.c.h.|
'|-|-|' '|-|-|-|' '|-|-|-|'
| .-' '-. .-' '--.
v v v v v
.-------. .---. .---. .---. .-----.
|.b.c.g.| => |.b.| |.g.| => |.b.| |.e.g.|
'|-|-|-|' '|-|' '|-|' '|-|' '|-|-|'
| | .-' '-.
v v v v
.-------. .-------. .---. .---.
|.d.e.f.| |.d.e.f.| |.d.| |.f.|
'|-|-|-|' '|-|-|-|' '|-|' '|-|'
If you lazily split, you aren't guaranteed an open slot in your parent,
so you need recursion to solve splits. This is why 2-3 trees, though
self-balancing, are not tail-recursive:
.-----. .-----.
|.a.h.| |.a.h.|
'|-|-|' '|-|-|'
| |
v v
.-------. .'''''''''.
|.b.c.g.| => >.b.c.e.g.< 5!?
'|-|-|-|' '|.|.|.|.|'
| .-' '-.
v v v
.-------. .---. .---.
|.d.e.f.| |.d.| |.f.|
'|-|-|-|' '|-|' '|-|'
But if you are eagerly splitting while also deleting nodes:
.-----. .-------. .-------. .'''''''''.
|.a.h.| |.a.c.h.| |.a.c.h.| 5!? >.a.c.e.h.<
'|-|-|' '|-|-|-|' '|-|-|-|' '|.|.|.|.|'
| .-' '-. .-' '---. .---' | '---.
v v v v v v v v
.-------. .---. .---. .---. .-------. .---. .---. .---.
|.b.c.g.| => |.b.| |.g.| => |.b.| |.d.e.f.| => |.b.| |.d.| |.g.|
'|-|-|-|' '|-|' '|-|' '|-|' '|-|-|-|' '|-|' '|-|' '|-|'
| x | x
v v
.-------. .-------.
|.d.e.f.| |.d.e.f.|
'|-|-|-|' '|-|-|-|'
Suddenly, recursion. This is a problem.
The workaround implemented here is to check during pruning if our parent
may risk recursion, and if so, recolor the last alt so nothing will
break.
This ends up equivalent to the following transformation:
.-----. .-------. .-----. .-----.
|.a.h.| |.a.c.h.| |.a.c.| |.a.c.|
'|-|-|' '|-|-|-|' '|-|-|' '|-|-|'
| .-' '-. .-' '-. .-' '--.
v v v v v v v
.-------. .---. .---. .---. .---. .---. .-----.
|.b.c.g.| => |.b.| |.g.| => |.b.| |.h.| => |.b.| |.e.h.|
'|-|-|-|' '|-|' '|-|' '|-|' '|-|' '|-|' '|-|-|'
| x | x | .-' '-.
v v v v v
.-------. .-------. .-------. .---. .---.
|.d.e.f.| |.d.e.f.| |.d.e.f.| |.d.| |.f.|
'|-|-|-|' '|-|-|-|' '|-|-|-|' '|-|' '|-|'
You may notice this isn't exactly optimal. The >h branch ends up one
level lower, making the balance of the tree off by one. But it at least
ends up with a functional tree.
I may try to find a better solution...
---
The test_rbyd_delete_range_rydy/rydye tests should cover the cases where
a diverging alt is deleted.
I also tried to write tests for the cases where an alt is pruned, the
closest I got is in test_rbyd_delete_range_dryy_backwards, but I
couldn't actually come up with a sequence that would break our rbyds.
In theory it's possible, but it would need this substructure:
.-------> c y-r-b-------> c
y-r-b-y-r-b-> c or | | '-y-r-b-> c
| | | | | | | |
Which, as far as I can tell, can't actually be created with our current
algorithm...
Note the inverse structure:
.---------> c
| .-y-r-b-> c
y-r- | |
Will be pruned before it has a chance to split. So there is no invariant
concerns there. We only have issues when it's the tail alts that get
pruned, because we decide to split before we know if we are pruning or
not. I don't think this can be avoided without additional read-ahead.
Also, even if we could create the above substructure, because we are on
a diverged trunk, and by definition all alts point the same direction,
we would never end up violating our same-dir yellow invariant/assert...
Code changes:
code stack
before: 33880 2880
after: 33912 (+0.1%) 2880 (+0.0%)
|
||
|
Christopher Haster
|
47416c1115 |
Switched to recoloring + red stitching removals due to diverged coloring bug
This was a nasty bug. I was initially concerned that this slipped
through our rbyd tests until I realized how excruciatingly rare it is.
If, during a range remove:
1. There is a pending yellow split immediately after the diverging alt
2. There is a pending yellow split immediately before the diverging alt
3. The diverging alt takes a black alt in the yellow split
4. There is a red node before the pending split before the diverging alt
5. The two alts in the red node point in different directions
We can end up violating our yellow node both-alts-point-same-direction
invariant.
The tree looks like this:
.-------------r-------------.
.-o-. .----y---+---. .-o-.
.o. .o. .-+-y-. .o. .o. .o. .o. .o.
a a a a a a a c e e e e e e e e e e
'+'
remove
Though this diagram doesn't capture the actual alt-layout, which does
matter here, so the dbgrbyd.py rendering may be more useful:
.-> aa .-> aa
.-b-> a .-b-> a
| .-> a | .-> a
.-----------b-b-> a .-----b-b-> a
| .-----> a | .-> a
| | .---> a | .-----b-> a
| .-y-r-b-> a | | .---> a
| | '-> cc <- rm | | |
r-b-y-r-b-----b-> ee => y-y-r-b-r-b-> ee <- two yellows!
| | | '-> e | | '-> e different dirs!
| | '-------b-> e | '-b-b-> e should not happen!
| | '-> e | | '-> e
| '---------b-> e | '-b-> e
| '-> e | '-> e
| .-> e | .-> e
| .-b-> e | .-b-> e
| | .-> e | | .-> e
'---------b-b-> e '-------+-b-> e
If all of these conditions are met, and we are preserving coloring, we
can end up with two yellow splits without an intermediate black alt,
implying recursion. But we're of course not recursive, so things just
break.
If we look at the trunk that is being built during our range removal:
read <r => [<r]
read >b => [<r >b]
read >r => [<r >b >r]
read >r => [<r >b >r >r]
^--^------ red+red implies yellow
ysplit => [<r >r >b]
reorder => [>r >r <b]
^--^------------- yellow-same-dir invariant held
read <b => [>r >r <b <b]
diverge => [>r >r <b]
read <r => [>r >r <b <r]
read <r => >r [>r <b <r <r]
^-----------^-- our 4-alt fifo for flips/coloring
ysplit => >r [>r <r <b]
reorder => >r [<r <r >b]
^--^---------- yellow-same-dir invariant held
^---^------------- yellow-same-dir invariant NOT held
though 2 yellows is also a problem
The important thing to note is that the diverging alt is effectively
deleted in both search paths. If the diverging alt is between two yellow
splits, that's not good.
If you think about the mapping to the underlying 2-3-4 tree, append is
only guaranteed to be tail-recursive because we eagerly split 4-nodes
into 2 2-nodes, ensuring that our parent always has a slot available for
a split (this is why 2-3 trees are not tail-recursive). But if we delete
one of the 2-nodes, and find another 4-node, the parent's slot has
already been taken. This is basically the problem we are running into
here.
A hypothetical 2-3-4-5 tree however...
Probably-isomorphic to a 2-3-4-5 tree, there are a couple of possible
solutions to this:
1. Increase the fifo to 5(?) alts and recursively propagate recolorings
up 2 nodes.
Note this would still be bounded and tail-recursive. Our current
implementation is basically an isomorphism of recursively propagating
recolorings up 1 node after all, if you want to think about it in
about the most complicated way possible...
Downsides: The increased fifo size means more RAM cost. And the
implementation would be complicated as hell. Not to mention error
prone. Imagine ~2x the current 15K lines of rbyd tests. It would be
bad.
2. Discard split recolorings after a diverged alt.
This would be quite a bit simpler, though would still require some
annoying state to know if the previous alt diverged.
If this state isn't perfect, the above checklist of conditions would
just be incremented by 1, making this bug even harder to track down.
I'm starting to think that preserving color during range removals is a
bit complicated for its own good.
Considering that color-preserving range removals aren't even rigorous
and don't guarantee a balanced tree, I think this all just needs to be
scrapped until a more rigorous solution is found.
---
So this commit drops color-preserving range removals, and moves to a
simpler paint it black + stitch together alternating red alt strategy
when encountering a diverging range removal.
Thanks to the red-stitching, the resulting search path is at least
tried to be kept as small as possible.
This results in the following, not-broken tree:
.-> aa .-> aa
.-b-> a .-b-> a
| .-> a | .-> a
.-----------b-b-> a .-----b-b-> a
| .-----> a | .-------> a
| | .---> a | | .---> a
| .-y-r-b-> a | | | .-> a
| | '-> cc <- rm | | | |
r-b-y-r-b-----b-> ee => y-r-b-r-b-r-b-> ee
| | | '-> e | | '-----> e
| | '-------b-> e | '-------b-b-> e
| | '-> e | | '-> e
| '---------b-> e | '-b-> e
| '-> e | '-> e
| .-> e | .-> e
| .-b-> e | .-b-> e
| | .-> e | | .-> e
'---------b-b-> e '---------b-b-> e
It's interesting to note that this bug is so rare that it was only
caught by test_dirs_mv_fuzz after 2180 heuristic powerlosses. But it
was caught, so that's a good sign.
But it would have been better if this was caught in the rbyd tests. I've
gone ahead and added a specialized test, test_rbyd_delete_range_rry (and
a few other), to prevent a regression, which is very likely. It's more
likely than not we'll revisit range removals in the future.
On the plus side, since recoloring is simpler than color-preservation,
this means less code:
code stack
before: 34072 2880
after: 33992 (-0.2%) 2880 (+0.0%)
|
||
|
Christopher Haster
|
34be5055b4 |
Fixed mdir drop during compaction breaking fixorphan loop
The core problem is that we weren't updating dropped mdirs with weight=0 if the mdir was compacted at the same time. This is hard to notice, because most operations that can drop don't care about the mdir afterwards, but in lfsr_fs_fixorphans this caused the fixorphan loop to think it might still have orphans it could remove. The implementation is very subtle here: - In lfsr_mdir_commit_, if an error occurs during lfsr_mdir_compact__, we need to revert to the original mdir state to allow fallback to mdir split. - In lfsr_mdir_commit_, if an error occurs during lfsr_mdir_commit__ (even after a compact), we need to update the mdir in case a drop reduced the mdir weight to zero. We also need to update the mdir for things like erased state, but this doesn't come into play in the compaction route. Fixed the bug by updating the mdir copy before lfsr_mdir_commit__. Also added asserts to all insert/delete operations in test_mtree.toml. We already had drop-during-compaction tests, but these didn't check that the mdir was updated correctly. The new asserts catch this bug and should prevent a regression. |
||
|
Christopher Haster
|
692810e18e |
Reverted lfsr_data_t lazily encoded leb128s
- It didn't save code. - An inlined buffer is potentially more useful, even if only marginally, and, uh, unproven yet. - Requiring lfs_toleb128 in a readonly implementation is a hard ask. |
||
|
Christopher Haster
|
415e148f62 |
Replaced inlined lfsr_data_t with a lazily encoded leb128
The idea is that we can save on the cost of calling lfs_toleb128
everywhere we commit leb128s, by lazily encoding during progdata.
I original thought this would have too many small problems, but:
1. We can actually implement slice surprisingly easily by just shifting
the internal word 7 bits. This emulates byte-level slicing in the
encoded leb128.
This enables read/cmp, so we can implement all of the lfsr_data_t
functions, though it does make lfs_toleb128 required for a readonly
implementation, which isn't great. Sufficient creativity with ifdefs
likely makes this a non-problem though.
2. There's really very limited use cases for non-leb128 inlined datas.
We can use it to encode the version and compatflags during
lfs_format, but that's about it. And lfs_format is definitely not on
the stack hot-path, so there's no reason to not use on-stack buffers
for these.
The original motivation for this change was noticing a surprising amount
of code savings related to lazy leb128 encoding in another lfsr_data_t
refactor. Unfortunately this savings does not seem reproducible:
code stack
before: 33864 2880
after: 33912 (+0.1%) 2888 (+0.3%)
But that's ok, this is closer to what I expected. The lfs_sizeleb128
call we need to predict the leb128 size is close to the same cost as
calling lfs_toleb128 so the savings isn't really that much.
|
||
|
Christopher Haster
|
5005db2b4e |
Moved erase into lfs_alloc, mostly
This doesn't really help us all that much right now, but will be useful
for the future-planned block map and being able to cache pre-erased
blocks.
Though the lack of erasing when allocating new mdirs raises some
questions... Oh well, future problems.
Code changes:
code stack
before: 33856 2880
after: 33864 (+0.0%) 2880 (+0.0%)
|
||
|
Christopher Haster
|
788a9d0129 |
Added lfsr_bd_unprog to replace flcksum args
Topologically, this isn't really much of a change. We just moved the
flcksum -> lfs.pcksum and made the internal API a bit better.
But hey, a better internal API at ~no cost is always a good thing:
code stack lfs_t
before: 33868 2880 212
after: 33856 (-0.0%) 2880 (+0.0%) 216 (+1.9%)
|
||
|
Christopher Haster
|
4a66816d4f |
Renamed SUP/SUBMASK -> SUP/SUB
There wasn't really a collision with this, and I think it's clear what these flags are doing. Also fixed a missed renamed of lfsr_tag_issup/subwide -> lfsr_tag_issup/sub |
||
|
Christopher Haster
|
6c9ce4e8f1 |
Reverted raw-byte comparisons for rbyd/btree namelookups
Implementing raw-byte name comparisons ended up having more negative
effects on implementation requirements than I thought it would:
1. We would never actually concatenate the did + name, as that would
require dynamic memory. Instead we need to express the concatenated
relationship using our internal lfsr_data_t representation.
I thought this wouldn't be too bad since we already have a
concatenated lfsr_data_t representation, but:
1. It was limited in scope, specifically only lfsr_data_prog was
supported. It's actually not even possible to implement
lfsr_data_read (I think) since we can't mutate the indirect
lfsr_data_ts.
2. It's not actually required. We really only use our concatenated
representation to coalesce file fragments. You could in theory
omit this representation at the cost of not being able to limit
inlined shrub overhead.
Asking all future littlefs implementations to implement a
concatenated data representation (or dynamically allocate D:) for the
basic task of file-name lookup is sort of a big ask.
2. A readonly implementation suddenly needs a toleb128 function.
Which is an unexpected implication of requiring raw-byte leb128
comparisons for file-name lookup.
3. Raw-byte comparisons require that dids are always stored in their
canonical encoding (smallest leb128), though this is probably a good
idea anyways.
And for what? A theoretical future-planned feature (content-tree)?
Let's think about the hypothetical content-tree for a second:
1. It's an advanced, opt-in feature. Which means higher code/storage-cost
should be expected.
2. Basicall all littlefs implementations need file-name lookup, so
keeping file-name lookup cheap is a much higher priority than the
opt-int content-tree.
3. Worst case, the content-tree, and any future named trees, can just
set did=0. This will cost one byte per name (and may leave room for
future extensions).
So I'm reverting this for now.
There is still time before stabilization, so if it becomes clear there
is a better way to implement name lookups, we can still change this.
(Optimistically, the content-tree may be implemented before
stabilization, since it currently looks like it's required for data
redundancy).
Code changes:
code stack
before: 34292 2896
after: 34028 (-0.8%) 2896 (+0.0%)
|
||
|
Christopher Haster
|
35a4934178 |
Switched to passing lfsr_data_t by value again
Thanks to poor compound literal optimization, it's actually cheaper to pass lfsr_data_t by value everywhere, than to make all LFSR_DATA_* macros lvalues: before: 34340 2896 after: 34292 (-0.1%) 2896 (+0.0%) Why are these two design choices linked? If lfsr_data_t is pass-by-address, the rvalue/lvalue disinction is important because we need to take the address of LFSR_DATA_* macros. If lfsr_data_t is pass-by-value, rvalue/lvalue doesn't really matter because we, well, pass by value. To be honest, this is a bit of an excuse for better lfsr_data_t ergonomics. It _is_ generally worse code-size wise to pass lfsr_data_t by value, because most ABI optimizations stop at 2 words and lfsr_data_t requires 3 words. But always passing lfsr_data_t by value even if it is suboptimal makes for more consistent internal interfaces. This also helps side-step a mistake I made earlier where I though cat/fromimm/fromleb128 were the only LFSR_DATA_* macros that needed to be lvalues to be consistent. THERE ARE MANY MORE LFSR_DATA_* macros, every LFSR_DATA_FROMBLAH macro to be specific, and the resulting code cost would be MUCH WORSE. --- This also add lfsr_sprout_t to complement lfsr_bptr_t/lfsr_shrub_t/etc. Unlike lfsr_data_t, lfsr_sprout_t _is_ pass-by-address Actually that's the only difference, haha. lfsr_sprout_t is a typedef. Though to be fair, by being pass-by-addres, lfsr_sprout_t keeps the internal sprout/shrub/bptr/btree inferfaces consistent, and saves a bit of code. |
||
|
Christopher Haster
|
94f7d2549f |
Changed rbyd/btree namelookups to only compare raw bytes
This is a simplification of the rbyd/btree layers, but implies
behavioral changes to the mtree/mdir layers.
Instead of ordering by leb128 did + name:
82 02 61 61 61 < 81 04 62 62 62
(0x102, "aaa") (0x201, "bbb")
We now order by the raw encoding, lexicographically:
82 02 61 61 61 > 81 04 62 62 62
(0x102, "aaa") (0x201, "bbb")
This may be unintuitive, but note:
1. Files _within_ a directory are still ordered, since they share a did
prefix.
2. We don't really care about the relative ordering of dids, just
that they are unique. Changing the ordering at this level does not
interfere with any of our did-related functions.
3. The only thing we may care about is that the root, did=0, is the
first mtree entry. This is still true. No leb128 encoding is < 0x00
even after encoding.
The motivation for this change is to allow for other named-btrees in the
system that may used non-did-prefixed names. At least one of these makes
sense for a sort of "content-tree" (cksum -> data block mapping).
As a plus, this change makes it possible to compare names and do btree
namelookups without needing to decode the leb128 prefix. Although I'm
struggling a bit to figure out exactly where this is useful...
One downside, this ordering only works if dids are always stored in
their canonical encoding, that is, the smallest leb128 encoding possible
for a given did. I think this is a reasonable requirement for just our
dids.
Another downside is this did add a decent chunk of code.
I did try limiting the changes to lfsr_data_namecmp, but it didn't have
much impact. I guess most of the cost comes from the reworked
lfsr_data_cmp function, which, to be fair, is quite a bit more
complicated now (it now supports limited data<=>data comparisons):
code stack
before: 34148 2896
namecmp: 34324 (+0.5%) 2896 (+0.0%)
after: 34340 (+0.6%) 2896 (+0.0%)
|
||
|
Christopher Haster
|
748bca0b61 |
Dropped LFSR_ATTR() prefix magic
Before:
LFSR_ATTR(RM(SUBMASK(REG)), 0, BUF("hi", 2))
Now:
LFSR_ATTR(
LFSR_TAG_RM | LFSR_TAG_SUBMASK | LFSR_TAG_REG, 0,
LFSR_DATA_BUF("hi", 2))
Yes, it's more verbose now.
But there were a couple reasons for dropping the idea:
- The implicit prefixing is a bit magical, and not really all that
common in C code. It would likely confuse new users on first read.
- The implicitly prefixing macros did not play will with macro expansion
rules.
In particular, because the nested not-yet-prefixed macros aren't
really macros, they aren't expanded as a part of argument prescan.
This led to surprising compile-time errors, and prevented recursive
attr-lists (which may be useful for shrubs).
- Implicit prefixes is not very C-like, and in particular it gets in the
way of sed/grep operations on source files.
- RM(SUBMASK(REG)) for combining tags is (IMO) ugly, compared to
LFSR_TAG_RM | LFSR_TAG_SUBMASK | LFSR_TAG_REG, even if the latter
requires more typing.
- Sometimes you need runtime-dependent TAG/DATA values, which implicit
prefixing gets in the way of. The LFSR_TAG_TAG(tag)/
LFSR_DATA_DATA(tag) backdoors worked around this, but they are even
more magical, and added noise to a not-actually-all-that-uncommon use
case.
And it's really not _that_ much extra effort to write out the prefixes
everywhere.
lfs.c:
lines bytes
before: 16894 537171
after: 16907 (+0.1%) 538340 (+0.2%)
tests/*.toml:
lines bytes
before: 53306 1811035
after: 54517 (+2.3%) 1851006 (+2.2%)
qadte came in quite handy again for refactoring the tests without
completely losing my sanity.
|