1d88fa98640c5acf9a8a82fa46967983512129cb
444 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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. |
||
|
Christopher Haster
|
3a90d1046b |
Reverted insert tags appending, fixed insert issues in named btrees
Changing insert tags to append seems to have broken insertion into named
btrees in a subtle way.
Consider what happens when we insert immediately before a bid that
splits the btree:
1. namelookup returns the right rbyd, with rid=-1
2. converting this into a bid gives us the left rbyd, with rid=weight
3. the commit to insert the bid ends up inserting into the left rbyd
This doesn't initially seem like an issue, both entries are effectively
the same right? Well, not when you have names. The split name tells you
what _follows_, so this unintentional flipping causes the new name to
get placed in the wrong bucket.
It's not clear if it's possible to fix this, at least not without
inverting the split names to indicate what precedes, but that's a step
too far.
This was not detected earlier because I disabled the low-level
rbyd/btree/mtree tests temporarily due to high porting cost. Guess that
goes to show there's a cost to deferring test ports for too long.
---
This issue, along with being inconsistencies between rids/bids and mids,
and being a relatively unintuitive pattern, is the final nail in the
coffin for insert tags inserting after.
Now, insert tags insert before, like in most other systems, and insert
tags in attr-list just have an implicit +1 before them to allow splits
in attr-lists to work.
This is not a pure revert, as some of the changes with all the code
moving around revealed some better detail-level ideas.
And yes, rbyd/btree tests are up to date now. Unfortunately the mtree
tests require a bit more work.
---
One thing definitely worth noting, btree merges were broken! A mistake
in the has-parent condition meant we were never attempting to merge
btrees!
This hid some bugs in the actual btree merge code caused by mixing the
implicit swap of child rbyds to deduplicate code paths with btree commit
now needing to track bid/rid separately from the attr-list.
This should be fixed now. Interesting to note this bug has been in
lfsr_btree_commit_ for a while now! I think ever since we switched to
using trunks for the has-parent check. We just haven't been merging
btree nodes at all. But since not-merging isn't technically an error,
it's difficult to test for.
Code changes:
code stack
before: 33808 2896
after: 33964 (+0.5%) 2896 (+0.0%)
|
||
|
Christopher Haster
|
7868ec7122 |
Ported over most rbyd+btree tests to new attr-list format
Found a bug, and maybe a fundamental issue: - The lfs_btree_lookupnext_ in lfsr_btree_commit_ no longer needs the min32, since we never commit with bid pointing past the end of the btree anymore. This was mixing the unsigned min32 with our now-signed bid type, causing the wrong btree leaf to be fetched when inserting at bid=-1 in a non-empty btree. Easy fix. - lfsr_btree_commit_ with bid!=-1, rid=-1 (inserting at the beginning of not-the-first rbyd) now actually appends to the leaf to the left of the rbyd instead of inserting into the expected rbyd because of how lfs_btree_lookup_ works. Initially, this doesn't seem like it would be an issue, these should be more-or-less equivalent, but this doesn't match lfsr_btree_namelookup! This is a big problem! This wasn't noticed because it's rare for the high-level tests to trigger that many btree splits with names. Named btrees are only used for the mtree, and we need mdirs to split before the mtree even splits once. Not an easy fix. On the upside, these low-level tests continue to prove themselves valuable, if tedious to maintain... |
||
|
Christopher Haster
|
f323ea1bda |
Made mkdir tests a bit more paranoid
Unfortunately the current dir+bookmark+grm design has a high risk of
the system becoming out-of-sync and losing bookmarks. In theory this
should cause test failures, but the previous grm-mid-off-by-one bug has
left me a bit paranoid.
So when dbglfs.py starting flashing bookmark errors, I started
investigating. But just I can't reproduce these errors in a controlled
way, and they cause no test failures...
My current setup involves this script to copy the disk file
"atomically", so even though dbglfs.py is slow, we shouldn't be reading
blocks from different filesystem states. Uh, beauty is in the eye of the
beholder and all that jazz?:
./scripts/watch.py -b -Kdisk bash -c "cp disk disk_ \
&& ./scripts/dbglfs.py disk_ -B4096 \
--color=always -s -a -T -f -g 2>&1 \
| head -n32"
But some brief investigation suggests cp is not atomic. After all, how
could it be?
My guess is we occasionaly catch blocks from different filesystem states
when a write occurs during a cp operation. So a false positive.
Still, might as well keep these extra asserts for a bit of extra
confidence we're not losing bookmarks during heavy mkdir operations.
|
||
|
Christopher Haster
|
e04748dadd |
Renamed SUB/SUPWIDE -> SUB/SUPMASK
This name makes more sense to me given what these bits are doing. Though that may just be from the embedded engineer side. |
||
|
Christopher Haster
|
3c13afd5c2 |
Added explicit test over unreachable tag holes
Unreachable tag holes, null tags that _should_ be unreachable but
actually are reachable, are an unfortunate quirk to our alt tag
encoding. Because we only have an altgt, not altge, our "unreachable"
tag ends up encoded with an altgt 0, an alt, which you may notice, does
not guarantee unreachability.
Fortunately, tag 0, the null tag, should intentionally be unused. So as
long as we never lookup tag 0, nothing should break.
If you do lookup tag 0, you end up with spurious null tags, which can
complicate things.
The solution here is a tag_ = max(tag, 1) in lfsr_rbyd_lookupnext.
---
One interesting thing to note, as I was writing these tests I discovered
that setting tag=max(tag,1) in lfsr_rbyd_appendattr had no effect.
appendattr needs zip the rbyd tree to keep everything connected during
range removals, so tag=0/tag=1 both end up with the same tree.
So might as well drop the tag=max(tag,1) in lfsr_rbyd_appendattr.
A side effect of this, both before and after this commit, is that any
null tag holes created during range removals sort of stick around until
the next compaction.
---
Why altgt and not altge? altgt is the inverse of altle, requiring only
a single bit flip to flip between the two. And trust me, it would be
much more costly to make altle/altgt flips more complicated than a bit
flip.
---
Why altgt/altle and not altge/altlt? This is because our rbyds are
right-leaning, that is, lookups always find the requested rid+tag, or
the next smallest rid+tag.
Consider a simple tree:
<5
.----'|
>=2 |
|'-. |
1 2 5
What should lookup(3) return? If we are right-leaning, the answer
_should_ be 5. But we need to take the <5 branch to determine if there
is a hidden 3 or 4 in that subtree.
altgt/altle does not have that problem:
<=2
.----'|
>1 |
|'-. |
1 2 5
It might seem like you can workaround this by conservatively using the
neighbor +1 as the alt target, but this runs into tag overflow problems.
UATTR(0xff)+1 (0x057f+1) becomes UATTR(0x100) (0x0580) which is not
allowed due to reserving bit 7 for future subtype extensions.
Maybe you can workaround this workaround by using (tag+0x81)&~0x80
anywhere you need to increment (including lookupnext/iteration calls!),
but this becomes a bit of a mess. And there are still concerns about
overflows at the 0x77f boundary and 0xf7f boundary.
|
||
|
Christopher Haster
|
5f25f32ff1 |
Adopted SUPWIDE tag bit, parallel to the SUBWIDE (was WIDE) bit
Like SUBWIDE, SUPWIDE allows for "mask-like" operation during rbyd
commits, where you replace an entire subrange of tags with a single tag.
- SUBWIDE - Replace all subtypes of the given suptype - Useful for
changing the subtype of an attr, for example replacing a BTREE with a
BSHRUB.
- SUPWIDE - Replace all suptypes of the given rid - Useful for changing
the suptype of an attr, for example replacing a REG file with an
ORPHAN file.
These are effectively the same modifier, just with different ranges.
One benefit is this simplifies mid-level operations a bit, rename,
remove, etc, and decreases the stack cost of the related attr lists.
Though this isn't on the hot-path, so not measurable:
code stack
before: 33956 2912
after: 33928 (-0.1%) 2912 (+0.0%)
But the real motivation for this change is to remove cases where
lfsr_mdir_commit needs to operate on multiple mids. There may be an API
simplification here.
|
||
|
Christopher Haster
|
6fc040db1a |
Adopted paren-cond ternary operator style
So: x = (cond) ? yes : no; Where there are always parentheses around the condition, even if not required for disambiguity. Additional parentheses are always allowed, but the parenthesized condition helps signal that a ternary operator is coming earlier in the expression. This style has grown on me as I think it helps code readability. It reminds me of the required parentheses for if/while statements. Might as well adopt codebase-wide. |
||
|
Christopher Haster
|
96b62ff804 |
Dropped conditional/noop attrs, prefer incremental attr allocation
So instead of using C's ternary operator everywhere:
(condition)
? LFSR_ATTR(rid, tag, delta, data)
: LFSR_ATTR_NOOP
Use incremental attr allocation instead:
lfsr_attr_t attrs[1];
lfs_size_t attr_count = 0;
if (condition) {
attrs[attr_count++] = LFSR_ATTR(rid, tag, delta, data);
}
LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t));
Incremental attr allocation is more flexible, allowing nested conditions
and conditions that span multiple attrs without sacrificing readability,
though at a verbosity cost.
We already need this for lfsr_btree_commit and lfsr_file_carve, adopting
it everywhere we need conditional attrs allows us to drop the noop attr
and avoid messy and hard-to-read C expressions.
This also changes the lfsr_btree_commit to explicitly omit noop grows.
We were relying on lfsr_rbyd_appendattr implicitly skipping these to
avoid unnecessary attr commits, but I think it's probably better to make
these noops explicit.
This does add some code cost though, I'm guessing sequential conditional
attrs landing at different offsets complicates code generation a bit:
code stack
before: 33940 2928
after: 34052 (+0.3%) 2928 (+0.0%)
|
||
|
Christopher Haster
|
9adb22eee0 |
Enforced stat/dir_read of a dir results in size=0
The size field in lfs_info doesn't really make sense for stat/dir_read when the file is a directory. Still, we should probably set it to 0 os it's not uninitialized. Fortunately we were already setting size=0 in _most_ cases, this commit is mostly just checking for size=0 in more test cases. |
||
|
Christopher Haster
|
1db215309b |
Dropped lfsr_mdir_bid/rid convenience functions
Much like the lfsr_o_* functions, I think we should avoid too many convenience layers for what really are operations on struct fields. Otherwise you quickly end up with a lot of boilerplate that just saves a couple extra characters at invocation. Characters that also help convey what is being accessed. |
||
|
Christopher Haster
|
942427dc8c |
Reworked lfsr_mtree_pathlookup a bit to better leverage internal errors
This avoids implicit info, mid.mid=-1 implying a bad path, and mid.mid=0
implying the root directory, at a tradeoff of potentially making the
returned error codes a bit confusing (0 means the file is NOT found!).
Here are the now possible return codes, aside from lower-level errors
(IO, CORRUPT, etc):
- 0 => path is valid, file NOT found
- EXIST => path is valid, file found
- INVAL => path is valid, but points to root
- NOENT => path is NOT valid, intermediate dir missing
- NOTDIR => path is NOT valid, intermediate dir is not a dir
Since the root has no real mdir entry, I think the special INVAL return
code is warranted. It needs special behavior in relevant functions
anyways.
Note that orphaned files still need special handling.
Code changes:
code stack
before: 33944 2944
after: 34036 (+0.3%) 2944 (+0.0%)
|
||
|
Christopher Haster
|
3eaee6877c |
Fixed issue where fixorphans deleted opened mids
We were just missing a check here to make sure orphaned files aren't open in-device (these aren't really orphaned because we still have a reference). This can't happen during mount, but can happen if fixorphans is triggered because of an orphaned/zombied file. Also added a test over this case to prevent regression. Actually the test was harder to implement than the fix. |
||
|
Christopher Haster
|
15593ccc49 |
Renamed scratch files -> orphan files
I was originally avoiding naming these orphans, as they're _technically_ not orphans. They do exist in the mtree. But the name orphan just describes this types purpose too well. This does lead to some confusing terms, such as the fact that orphan files can be non-orphaned if there are any in-device references. But I think this makes sense? - LFSR_TAG_SCRATCH -> LFSR_TAG_ORPHAN - LFSR_F_UNCREAT -> LFSR_F_ORPHAN - test_fscratch.toml -> test_forphan.toml |
||
|
Christopher Haster
|
f6742eefb3 |
Fixed test failures caused by changes to open semantics
- Fixed fsync tests, which needed more lfsr_file_sync calls so multiple file handles can be opened correctly. Though this points out there's no way to open a rdonly file on an uncreated file until sync is called... But I guess you wouldn't be able to recieve broadcasts until sync anyways? at which point the file would be created? - Update mtree tests based on the new remove behavior for regular files. Before this changed the mid to -1, now it points to the next mid with the zombie flag set. Upper layers use this to migrate mdirs to a scratch file if necessary. - Removed the orphaned mdir test. We don't create orphaned mdirs anymore. Technically, orphaned mdirs are currently possible if we lose power in the middle of the mtree update, but this is a bug and should be fixed (previous revisions did not have this issue). |
||
|
Christopher Haster
|
0c6db4c9a7 |
Extended fscratch tests to cover renames + different sizes
With this I think it's safe to say file renaming is decently tested. The increased range of sizes means we should be testing a good range of sprout/shrub/btree file structs. |
||
|
Christopher Haster
|
7385d84df5 |
Fixed/implemented renaming open files
This requires two things: 1. Any opened file handles need to have their mid/mdir updated after the rename succeeds. 2. Any shrubs/sprouts need to be copied over to the new mdir, even if they aren't in-tree. The LFSR_TAG_MOVE operation is starting to look an awfully lot like lfsr_mdir_compact... Unfortunately lfsr_mdir_compact, uh, compacts, whereas LFSR_TAG_MOVE appends to the rbyd like normal, so it's not clear exactly _how_ to deduplicate. |
||
|
Christopher Haster
|
f51dc5c5af |
Implemented zombied file handles
A "zombie file" is a term I just made up to describe what happens when you remove a file that is currently open. To match POSIX, the opened file handle should still be available for reading/writing, even though the file doesn't really exist in the filesystem anymore. We don't have inodes, which makes this a bit more complicated, but this is where scratch files are handy again. By creating a scratch file when we remove an opened file, we preserve the mid slot for the file's sprout/shrub. We also mark the opened file as desync, so the existing orphan reclaimation circuitry kicks in when the last file handle is closed. Really the only difference between zombie files and desync files is what happens when you call lfsr_file_sync: - Desynced lfsr_file_sync => Become synced, broadcast file state. - Zombied lfsr_file_sync => Return ENOENT, you can't sync a zombie. This _is_ a bit different from POSIX, where sync on a removed file returns 0. I considered returning 0 in this case, but with all the extra behavior around sync/desync state, I figured returning ENOENT was clearer at indicating to the user sync is no longer possible. Worst case, ENOENT is not returned from sync for any other reason, so users can always treat ENOENT and 0 as the same in higher layers. The zombie file is already desynced, so close will never error. --- Implementation wise, zombies get a bit crazy. Fortunately they add little extra code, but they make up for it by adding extra subtlety. Zombie files introduce a ton of corner cases, now even directories can have zombied shrubs. This means more tests. - Seemingly unrelated operations need to be able to remove scratch files (mkdir, rename, etc). - UNCREAT state needs to be broadcasted in seemingly unrelated operations (mkdir, rename, etc). - Zombied files need to be copied over during seemingly unrelated rename operations. - And I'm sure more corner cases I'm already forgetting. One interesting tweak that simplifies things that's worth mentioning is the change to the implicitly file mid updates on rm in lfsr_mdir_commit. For non-reg files, an rm attr causes lfsr_mdir_commit to increment the mid to the next mid in the mtree. This is the correct behavior for dirs, traversals, etc. Previously, reg files were a special case that marks the mid as -1. But by changing this to also increment the mid, as well as set the zombie flag, upper layers can broadcast zombie changes by simply creating a new file and then deleting the old file in the same commit. This seems to Just Work^TM, and avoids needing to do additional state broadcasting in upper layers, which gets tricky since we may not know exactly what the new mid is post-mdir-commit. Downside: The order matters, we need to create the new file first. This violates the normal delete-then-insert order we use elsewhere to avoid overflow issues. This isn't that bad here, since we increment by at most 1. But it is something to be wary of... Still, this is much better than any other option I can think of right now. --- Uh, ignore the test_fscratch_rename* tests for now. I somehow forgot file renaming was not yet implemented... |
||
|
Christopher Haster
|
99156b5573 |
Implemented orphans resulting from closing desynced scratch files
This is a fun corner case. What happens when you close a desynced scratch file? The obvious answer seems to be just remove the scratch file in lfsr_file_close. But then what if the file is rdonly? desynced because of an error? We really shouldn't write to disk at all when closing a desync or rdonly file. This needs to be a hard rule. So the only option is to defer the work until later somehow. Fortunately, we already have several mechanisms that lead to a very nice solution. I'm very happy with this: 1. There's nothing that says our in-device grm queue needs to always match what's on-disk (we need a separate copy for xoring anyways because of the risk of leb128 encoding differences). So if we have <=2 orphans, we can just push these onto our grm. On the next write operation, the normal grm fixing code takes over and removes the pending orphans O(1). 2. If we have >2 orphans, the best we can do is mark the filesystem as having orphans, and trigger an orphan scan on the next write operation O(nlogn). But how often do you think littlefs's use cases will end up with >2 orphans? Note we also need to scan the opened-file list to make sure we're the _last_ reference to the scratch file. Otherwise we corrupt other opened file handles! --- This commit also includes a fix for a bug where the traversal mdir fell out of sync when dropping mdirs as a part of scratch file cleanup. Found when adding more tests, this would cause scratch files to go unreclaimed. |