5dc88e3e00476dc3235dd7c19c859bf9a86c2cd5
762 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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.
|
||
|
Christopher Haster
|
d09a14f352 |
Changed DATA macros to implicitly stack allocate via compound literals
So instead of:
uint8_t mptr_buf[LFSR_MPTR_DSIZE];
int err = lfsr_btree_commit(lfs, &mtree_.u.btree, 0, LFSR_ATTRS(
LFSR_ATTR(
MDIR, +lfsr_mleafweight(lfs),
FROMMPTR(lfsr_mdir_mptr(&mdir_), &mptr_buf))));
This can be written as:
int err = lfsr_btree_commit(lfs, &mtree_.u.btree, 0, LFSR_ATTRS(
LFSR_ATTR(
MDIR, +lfsr_mleafweight(lfs),
FROMMPTR(lfsr_mdir_mptr(&mdir_)))));
Explicit stack allocation is still possible with the DATA hole, though a
bit more annoying:
attrs[attr_count++] = LFSR_ATTR(
MDIR, +lfsr_mleafweight(lfs),
DATA(lfsr_data_frommptr(
lfsr_mdir_mptr(&mdir_),
&buf[buf_size])));
buf_size += LFSR_MPTR_DSIZE;
The main motivation for this change is to be consistent with
LFSR_DATA_CAT, which was already implicitly stack allocating. The macros
that take arrays are relatively error-prone otherwise (LFSR_DATA_CAT,
LFSR_ATTRS, etc).
This does come with the benefit that the required buffer size is
implicitly provided by the macro, so no worry of it falling out-of-sync
externally. However this does come with the tradeoff of compound literal
lifetimes, which requires the result to live only as long as the current
expression.
Hopefully the fact that these are MACROs signal that they need special
care to any new developers...
Unfortunately, the use of compound literals also brings a surprising
code/stack cost:
code stack
before: 33912 2872
after: 34016 (+0.3%) 2896 (+0.8%)
Currently I can think of two reasons:
1. It's not possible to declare an uninitialized compound literal.
This probably sounds like a good thing to memory-safety fans, and
initialized is probably a good default for variable declaration, but
the reality is the required initialization does add useless code.
This specific use of compound literals is also low-risk given that we
immediately pass the literal to an lfsr_data_from* function, which
does the initialization.
2. We sometimes share on-stack buffers between branches of ternary
expressions since we know their use is exclusive. These macros sort
of get in the way of that.
What I find a bit curious is GCC doesn't seem capable of optimizating
away these overheads, which I would think would be possible given that
GCC knows all the information of how these buffers end up used.
I've noticed in general compound literals add overhead when the
underlying semantics don't really change. I wonder if this is because
compound literals are relatively new/unused, or some required
side-effects I'm missing. Maybe this will improve in the future?
Anyways, I'm keeping this change for now, since it does improve the
internal attr-list ergonomics/safety. Though these sort of changes are
always open to be revisited in the future.
Interestingly, the future-theoretical transpilation to c89 may save
code/stack because of this, which raises some questions...
|
||
|
Christopher Haster
|
b21f4b81fa |
Cleaned/reworked bd/caching layer
We really had ~2 duplicate bd layers for a bit there.
This also involved a sort of rewrite of these low-level functions to see
if there were simplifications that could be made.
A couple tweaks:
- Added small low-level lfsr_bd_read/prog/erase/sync_ functions to
only wrap the bd callbacks and apply any relevant asserts.
These should be the only place we call the bd callbacks to make it
easy to read/audit/insert hooks in the future.
- Changed pcache flush lazily, rather than eagerly flushing when full.
This isn't for any real performance reason, it just makes the code
simpler. It's not like we can shove more data into the pcache once
full.
It's _probably_ a good idea to flush eagerly, to avoid delay more work
until sync, but I couldn't figure out how to make this work cleanly
without code duplication...
- Deduplicated read pcache overwrites via lfsr_bd_read__.
This logic is a bit annoying, but we need the pcache to take priority
whenever we read from disk, which happens when we both fill our
rcache, and bypass our rcache. Since these code paths go different
places, another internal function was the only way I could think to
deduplicate this.
It may appear that our pcache/rcache prioritization loop will make
this happen naturally, as it does in lfs_file_read for example, but
this doesn't quite work as read-alignment requirements may force us to
read past the pcache... Keep in mind read_size may be > prog_size.
- Dropped LFS_BLOCK_NULL, now using cache.size=0 to indicate a cache is
unused.
This avoids a special lfs_block_t value.
- Dropped lfsr_bd_readcksum, we never used this.
We can always add it back if necessary.
In total, the caching bd prog/read functions now look quite a bit more
like our file read/write functions, so hopefully that's a good thing.
By the virtue of not have ~2 duplicate bd layers, this saves a bit of
code:
code stack
before: 33700 2800
after: 33560 (-0.4%) 2808 (+0.3%)
|
||
|
Christopher Haster
|
ddb86af059 |
Dropped lfs_cmp for manual comparisons
So instead of: lfs_cmp(cmp) <= 0 You can do: cmp <= LFS_CMP_EQ This is much simpler and still preserves the ability to use all of C's comparison operators on the results of disk comparisons. |
||
|
Christopher Haster
|
307d60299f |
Reinlined mtree/mroot commit logic into lfsr_mdir_commit
I think this is a case where separating the logic out into distinct
functions does more harm than good, by making it harder to understand
how all the different moving parts interact.
This is especially important for lfsr_mdir_commit, since this is where
all atomic operations in the filesystem get tied together. Having atomic
updates complete in different functions was particularly concerning
since it carries some implicit requirements (must not error after!).
The end result is a cumbersome function, but at least internally
relatively straightforward in how the commit propagates through the
mtree/mroot chain and internal state.
---
The other benefit of inlining is better code deduplication, since we
can treat the mroot as a normal mdir until it triggers a split or
relocation.
We can also deduplicate the grm patching, though there may be a better
way to implement this. There are still some awkward bits in the logic.
code stack
before: 33856 2888
after: 33764 (-0.3%) 2832 (-2.0%)
|
||
|
Christopher Haster
|
4b27c93f52 |
Brought back the opened namespace
- lfsr_isopened -> lfsr_opened_isopen - lfsr_addopened -> lfsr_opened_add - lfsr_removeopened -> lfsr_opened_remove - lfsr_mid_isopened -> lfsr_mid_isopen |
||
|
Christopher Haster
|
c0e9406b0b |
Reverted to mweight -> mleaf_weight and made lfs_t const
We have bleafs (bleaves?) now, so the mleaf name just makes too much sense. Even though it's used nowhere else outside of mid decoding, and may be a bit confusing. After all this time it feels weird to use a const lfs_t parameter, but that's really what the mid/mleaf functions should take. These functions are a bit of a special case as lfsr_mleafweight really wants to just be a constant. Code size did not change. |
||
|
Christopher Haster
|
204f46a131 |
Reworked internal tests remove unnecessary shim functions
These shims, originally intended to remap the tests to new internal APIs without a significant rewrite, are a long-outstanding piece of technical debt. Now that the internal API is more stable, it's time for that rewrite. Reasons for not keeping the internal shims: - They add more complexity to the test suites. - They come with (out-of-date) constraints that limit what we can test. - It's more difficult to debug test failures, with 2 layers and all. I ended up writing a small tree editor out of tree to do most of this rewrite. Did it save time? Probably not. But it was quite a bit more fun than manaully rewriting ~21K lines of code. |
||
|
Christopher Haster
|
921fe2ba1b | Tweaked documentation of implicit enums in test defines | ||
|
Christopher Haster
|
5e633aa554 |
Switched from decimal to hexidecimal for test name suffixes
This compresses a bit better, which is useful since our dbg scripts truncate into tight prefixes: - 3 decimals => 999 = <1000 - 3 hexidecimals => fff = <4096 - 4 decimals => 9999 = <10000 |
||
|
Christopher Haster
|
66a557d19d |
Dropped all alpha lookup table for 'a'+mod 26 arithmetic
I'm not really sure why I thought this required a lookup table... |
||
|
Christopher Haster
|
15cd1d29e0 |
Added explicit test over directory ordering
It turned out the previous version had a subtle ordering bug when names where the same length that went unnoticed for years. And at this point is probably baked into the on-disk format permanently. This redesign, with a named-ordered btree, relies quite a bit more on name ordering, so it's unlikely the same mistake would make it through without breaking something. And sure enough this bug was unintentionally fixed at some point. But still, better safe than sorry. Added tests over character ordering and length ordering. Open to more ordering tests in the future. Found by andriyndev |
||
|
Christopher Haster
|
40b926b947 |
Removed mid argument from lfsr_mdir_lookup*
With lfsr_mdir_t being a logical cursor pointing to a specific metadata
entry in the on-disk mdir, we don't really need the mid to be provided
on every lookup call (may have jumped the gun a bit in the attr-list
changes).
In the rare case we need to lookup unrelated mids, we call always call
lfsr_rbyd_lookup on the underlying rbyd.
This saves a little bit of code/stack:
code stack
before: 33964 2896
after: 33852 (-0.3%) 2888 (-0.3%)
|
||
|
Christopher Haster
|
d30ed42c7f |
Ported/cleaned up mtree tests
Well this was quite tedious, but these tests are valuable since our mtree has a number of hard-to-reach edge cases. This mainly ports over to the new attr-list format for mdir commits, but also cleans up a couple of lingering tedious TODO things: - mtree tests now use the new mdir commit attr-list format. - Reoriented most tests to use namelookups instead of mid lookups. Using mid lookups in testing is/was really fragile, since it depends on exactly how mids get split and moved around. namelookups are more robust, by design they don't care about the underlying mtree structure. And really, namelookups are what we care about in the mtree, mids are just a mechanism for mtree updates to work. We don't remove all mid checks though, we just compare against namelookup-derived mids when it matters (the mtree_opened tests for example). - The names we use in testing have also been updated to no longer create invalid mtrees, i.e. names are ordered correctly and always have a did. The previous mess always risked triggering asserts with false positives. - By adopting namelookup in the tests, we can actually test the on-disk state of fuzz testing. Though note we can't change names once written, without invalidating our mtree. This limits fuzz testing a little bit, but it's still a big improvement over the previous fuzz tests. - Dropped mtree tests that no longer really make sense. Mainly that the did should never be deleted, so you can never end up with an empty mtree, dropping the left-most mdir, etc. There were still a few of things lingering around. With this, all tests are working again with the attr-list changes. Wooh. |