Now -l/--list-fields includes however many results fit in 36 chars:
$ ./scripts/csv.py --list-fields test.csv
i int # 16,17,14,18,19,15,20,13,12,29,27,28,...
suite ? # bench_p26_wt
case ? # bench_p26_wt_linear,bench_p26_wt_ran...
NO_FRUNCATE int # 0
SIZE int # 2097152
SEED int # 42
The whole point of -l/--list-fields is to give a quick information dump
about what's inside a csv file, and we're already parsing everything to
try to figure out types, so why not?
Much easier to read than head:
$ head -n5 test.csv
i,suite,case,NO_FRUNCATE,SIZE,SEED,BLOCK_SIZE,FILE_SIZE,SIM_...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
16,bench_p26_wt,bench_p26_wt_linear,0,2097152,42,65536,64,36...
This extends csv.py's enumerate/accumulate exprs with optional by field
arguments. Each set of by fields gets its own state, allowing multiple
parallel enumerates/accumulates to be processed simultaneously.
This is especially useful when the number of by field sets is unknown.
In theory you could split/merge each by field set with a separate csv.py
call, but it'd be a real pain.
Consider some bench results:
case,n,simtime
bench_rbyd,1,100
bench_rbyd,2,10
bench_rbyd,3,100
bench_btree,1,200
bench_rbyd,4,10
bench_btree,2,20
bench_btree,3,2000
bench_btree,4,200
It was a bit awkward to handle these with csv.py's accumulate, as
accumulate operated strictly per-row, ignoring the case field.
But now with optional by fields:
$ ./scripts/csv.py test.csv \
-bcase -bn \
-fsimtime='accumulate(simtime, case)'
case,n simtime
bench_btree,1 200
bench_btree,2 220
bench_btree,3 2220
bench_btree,4 2420
bench_rbyd,1 100
bench_rbyd,2 110
bench_rbyd,3 210
bench_rbyd,4 220
TOTAL 5700
Note that these by fields are a bit special in csv.py's grammar. So far,
they are the only fields in field exprs that aren't typechecked. The
alternative would be string types in csv.py, but I'm not sure I want to
go that far.
---
It's tempting to try to invert this logic (accumulate(simtime, n)), but
I'm not sure how it would work internally. The duplicate by fields
("case") do get annoying, but specifying them in the expr helps make the
relevant state explicit.
Keep in mind we don't evaluate the actual by fields until much later in
csv.py. Entangling these stages risks confusion (-ba='%(b)s'
-c='enumerate(n)'? hidden by fields? overlapping by+field fields?).
I mean, what would you expect this to do?
max(a) + sum(b)
Whatever your answer is, it's wrong (the way csv.py works, we always
compute folds after expr evaluation). The best option is to error,
matching the behavior of mismatched types.
csv.py's -L/--list-computed was returning some confusing types:
$ ./scripts/csv.py /dev/null -fa='float(1)' -L
a int sum
^-- huh!?
Turns out csv.py's fold typechecking was all broken. Folds can change
the type, but only at the invocation:
$ ./scripts/csv.py /dev/null -fa='sum(float(1))' -L
a int sum
$ ./scripts/csv.py /dev/null -fa='avg(int(1))' -L
a float avg
$ ./scripts/csv.py /dev/null -fa='int(avg(1))' -L
a float avg
This is maybe defensible for explicit folds, since their evaluation is
also lifted, but not so much for things like literals/fields/etc.
---
Fixed by allowing None to indicate a generic fold, and allowing types to
be lazily figured out in csv.compile.
- Off-by-one in test_btree_find_general[_sparse]_fuzz
Because we can only create named btrees via splitting, these always
start with one entry. If all operations are randomly selected to be
splits, this can lead to an overflow of the sim buffer (sounds
unlikely, but relatively easily for small N).
The fix is to use a `for (lfs3_size_t i = 1; i < N; i++)` loop to
account for the initial entry. Note we already use this in the
test_btree_split_* tests.
An alternative is allocating space for N+1 entries, but this seems
unintuitive with N usually being associated with the upper bound on
btree size.
- Off-by-one in our sim rename pattern
When renaming, we don't bother to update sim_size, because after the
rename the sim_size size will be unchanged. But this means the
sim_size is out-of-date during the memmove that reinserts the renamed
entry. Buffer overflow!
To fix we just need to use sim_size-1 to account for the temporarily
deleted entry.
This is messy C code, so not surprised it went unnoticed, even though
this pattern ended up in quite a few tests.
Found while running with HEAP=1. This was just intended to test HEAP=1,
but I guess the injected heap hooks result in a more fragile heap? They
increase all allocations by one word, and maybe this reduces alignment
padding? Not exactly sure.
But it's a good argument for maybe adding heap canaries in the future.
Previously we ran Valgrind on all tests, but it's unclear if this will
still be reasonable with the number of tests we have now.
By "simple" I mean any non-globbing suite/case ids.
Non-globbing suite/case ids can be filtered early in the test_runner.
But globbing ids require, surprise, globbing, which is currently handled
by test/bench.py.
---
This greatly speeds up valgrind testing of specific suites/cases,
otherwise things get bogged down during the initial --list-cases due to
the sheer number of test permutations we've accumulated.
Maybe we shouldn't be running the initial --list-cases under Valgrind,
but oh well. This mostly solves the problem without too many changes.
These have been battle-tested in external benchmarks, and have proven
useful for finding a runtime estimate on stack+heap usage.
Of course, to be realistic they need to be cross-compiled and run under
QEMU (which does work!), but even on x86_64 they provide a nice insight
into RAM usage. In practice the only real difference is pointer width
anyways.
---
Enabled by default for the bench runner, these are available if
TEST/BENCH_YES_HEAP and/or TEST/BENCH_YES_STACK are defined.
(This default is provided by the Makefile. At least heap measurements
rely on linker flags, so it probably doesn't make sense to default
enable in the bench runner itself.)
Stack vs heap rely on slightly different mechanisms:
- Stack: Uses GCC's __builtin_frame_address(0) to measure the current
stack usage on entry to every bd operation.
- Heap: Relies on GCC's -Wl,--wrap flags to intercept every malloc/free
call, to track the current heap usage.
These are available via bench/test macros:
- BENCH_STACK() - Maximum stack usage of the current run
- BENCH_STACK_CURRENT() - Current stack usage
- BENCH_HEAP() - Maximum heap usage of the current run
- BENCH_HEAP_CURRENT() - Current heap usage
Note BENCH_STACK_CURRENT() can be useful for separating out the bench's
ctx from total stack usage, similarly to our static analysis.
---
One surprising outcome is that these heap hooks trivially implement a
memory leak detector. Maybe that could be useful in the test_runner as a
cheaper alternative to Valgrind?
kiwibd has been used extensively in external benchmarks, it makes sense
to make it the default bd for the bench runner:
- test_runner - defaults to emubd - more testing features
- bench_runner - defaults to kiwibd - lighter-weight disks
The benefit of kiwibd is the disk is just one big blob of RAM, so
basically no overhead. This is important when benchmarking on multi-GiB
disks.
emubd is much heavy, but as a tradeoff can do quite a bit more:
bad-block simulation, wear simulation, snapshotting, etc.
---
In theory the bd used by each runner can be controlled at compile-time
by defining -DBENCH_EMUBD, etc, but I have a feeling no one will ever
use this.
This needed a different name, and "bench probe" is sort of reminiscent
of the "debug probes" you can use to measure things in the real world.
Maybe this is just my embedded engineering background poking through,
but honestly anything is better than a single char m, especially for a
non-integer field.
These seem useful enough to have shortform flags:
- -l/--list-fields - Input fields before processing
- -L/--list-computed - Computed fields and expr dependencies
Note while -L/--list-computed has more information, it's also more
likely to trigger an assert/error due to poorly implemented field exprs.
On one hand, only inferring the used input fields is conceptually
correct because that's how csv.py works. On the other, it doesn't really
make sense for --list-computed to show _less_ information than
--list-fields.
So, showing all inferred types now:
$ ./scripts/csv.py --list-computed test.csv \
-bcase='%(case)s+%(m)s' \
-fsimtime='float(bench_simtime)/1.0e9' \
-fsimthroughput='float(n)/max(float(bench_simtime)/1.0e9,1.0e-9)'
i int .--> case ? ?
suite ? |.-> simtime int sum
case ? -+|.> simthroughput int sum
SKIP_WARMUP int |||
FILE_SIZE int |||
SEED int |||
...
m ? -'||
n int ---+
bench_reads int ||
bench_progs int ||
bench_erases int ||
bench_readed int ||
bench_progged int ||
bench_erased int ||
bench_simtime int ---'
I think this makes --list-computed a strict superset of --list-fields
now.
I was expecting -ba -Fa to sort numerically, but it was not. Turns out
hidden field fields (-F/--hidden-field) without exprs were never
typechecked.
This is not an issue for non-hidden field fields (-f/--field), because
we typecheck these explicitly in compile.
Found from some confusing behavior when by/from fields overlap. It turns
out when this happens (-bhi -Fhi, for example), the generated getattr
for the by field would trigger the __getattribute__ for the overlapping
field field, resulting in a fold on _every add operation_.
Hopefully you can see where this is a bit of a problem when summing a
large number of results (O(n^2)?).
---
Fixed by switching getattr to object.__getattribute__ and reconsidering
csv.py's entire design.
Now that we have csv.py's accumulate(), this information is strictly
redundant!
$ ./scripts/csv.py test.csv \
-bspecific_permutation_here \
-fbench_creaded='accumulate(bench_readed)'
The point of adding accumulate() was to drop these. We really shouldn't
be doubling the size of the csvs with redundant/derivable data.
This was a funny issue for external benchmarking, where we've focused
mostly on throughput benchmarking so far.
The current throughput approach is to run a benchmark for a given
simtime, and record the number of bytes written after. This is great for
allowing benchmarks to fail gracefully, but doesn't really work with the
current bench runner, which expected a known n in BENCH_START.
We can work around this by calling BENCH_START/STOP a second time
(making a mess of later scripts), but it would be nice if this was fixed
in the bench runner.
---
Humorously, BENCH_START just stores n to be printed out when BENCH_STOP
is called, so this was an easy fix.
Less useful than --list-fields, but fun.
This shows more of the internal expr eval info: input fields + types,
output fields + types + folds, and a small dependency graph showing what
goes where:
$ ./scripts/csv.py --list-computed test.csv \
-bcase='%(case)s+%(m)s' \
-fsimtime='float(bench_simtime)/1.0e9' \
-fsimthroughput='float(n)/max(float(bench_simtime)/1.0e9,1.0e-9)'
i ? .--> case ? ?
suite ? |.-> simtime int sum
case ? -+|.> simthroughput int sum
SKIP_WARMUP ? |||
FILE_SIZE ? |||
SEED ? |||
...
m ? -'||
n int ---+
bench_reads ? ||
bench_progs ? ||
bench_erases ? ||
bench_readed ? ||
bench_progged ? ||
bench_erased ? ||
bench_simtime int ---'
Maybe I was just itching to write another ascii-art renderer.
One issue I keep running into with csv.py is that it's difficult to get
started with a new/unfamiliar csv file.
csv.py itself doesn't know what to do until you start specifying fields,
but you can't start specifying fields until you know what fields there
are. Add to this the fact that our csv files have so much info shoved in
them that their "human readability" is mostly theoretical.
The --list-fields flag provides a quick solution to this:
$ ./scripts/csv.py --list-fields test.csv
i int
suite ?
case ?
SKIP_WARMUP int
FILE_SIZE int
SEED int
...
csv.py doesn't have much info at this stage, but we can at least include
the best-effort type guessing we use for field exprs.
Now that we have the enumerate expr, -i/--enumerate can be implemented
entirely during expr eval:
- -i/--enumerate => -bi -Fi=enumerate()
- -I/--hidden-enumerate => -Bi -Fi=enumerate()
Instead of internally reimplementing the same behavior.
This is what our help text implies, so might as well put our money where
our mouth is. And the less special internals we have, the better.
I considered removing -i/-I completely, but it's quite a convenient flag
when debugging csv.py expressions.
In an effort to move away from magic usage of -i/--enumerate, this adds
an explicit z field for differentiating -r/--hot results (and for normal
recursive results).
Instead of trying to think of a new flag to control this, this just
piggybacks on -Z/--children, which now accepts a tuple:
- ./scripts/csv.py -z3 -Z
- ./scripts/csv.py -z3 -Zchildren
- ./scripts/csv.py -z3 -Zz,children
The only tricky bit was needing to insert z in front of the by fields,
otherwise it was mostly a simplification from the enumerate mess.
Another positive side-effect: -r/--hot (and -z/--depth) now implies
-Zz,children, removing the annoying/confusing behavior of hotify folding
results by default.
The current... attempt at an approach was broken and becoming horribly
unmaintainable. Two issues found without even looking:
1. Field inference didn't understand prefixes, leading to duplicate
by/field fields when attempting to infer by fields with --prefix.
2. Sort wasn't working for some reason, probably because they behavior
of sort, defines, etc are really weird since they apply to both by
fields and field fields.
I considered just dropping support for --prefix completely, this really
isn't worth the time, but instead found a simple solution of moving
prefix handling to one of the first steps in collect_csv.
This has the downside of creating conflicts when a prefixed/non-prefixed
field has the same name, but I don't care. --prefix is a niche flag that
shouldn't mess with the rest of the code like this, and none of the
other scripts really handle field conflicts correctly anyways.
- Fixed the initial filter using explicit 'children'/'notes' literals
Whoops, how did this happen?
- Fixed fold using default children/notes result attributes
This one is a bit more excusable, self.children is easy to overlook.
But not actual string literals, that's silly.
This adds two new exprs to csv.py, useful for sequential data:
enumerate() A number incremented each result
accumulate(a) A running sum across results
To make these work required adding support for cross-row state, thus the
new state field in CsvExpr.Expr.eval.
Once you have that cross-row state, implementing enumerate/accumulate is
pretty straightforward. The only complication being that we need to hash
state by the unique Python id (`id(self)`), otherwise multiple exprs
would share state, which would be pretty weird.
Note that csv.py's pipeline is now quite complex, and stage order is
important!
input --> define --> expr --> folding --> sorting --> output
filtering eval
As a result, it's unfortunately not possible to organize enumerate/
accumulate by by fields. I poked around with the idea but decided it was
too complex (aren't I supposed be building a filesystem?). The guiding
principle behind csv.py is most problems can be solved with more process
substitution.
---
This is a bit clunky since we can't use the existing fold system, but
csv.py is already a pile of hacks, so what's one more?
The reason for the clunkiness is that the original idea behind csv.py
was to treat each folded row independently and order-agnostic. Not the
greatest idea in hindsight, cross-row operations are useful!
The idea here is to add some sort of accumulate operation to csv.py, so
we can stop cumulative-result clunkiness. It would also be immensely
useful as a general function, and -i/--enumerate already sets a
precedent for this sort of cross-row behavior.
But I'm starting to think using flags here is not the best way, maybe
this would be better as a field expr?
For some reason -F/--hidden-field fields weren't being parsed as a
CsvExpr, breaking any attempt to use exprs with hidden fields. Probably
just broken during a refactor.
Fortunately an easy fix.
The value of simtime isn't actually the simtime value, but the
simulated throughput, which is easy enough for our csv.py script to
calculate (with a daintily placed max to avoid divide-by-zero).
Throughput has the benefit of being somewhat size-agnostic, making
cross-benchmark comparisons easier.
I guess it's technically possible to do something similar with
readed/progged/erased numbers, but conceptually that would be really
confusing...
---
Also renamed test/bench_time -> test/bench_runtime to hopefully prevent
confusion between the two time spaces.
This is based on some work in external benchmarks. What's worked well
there is emulating a global simtime based on per-byte estimates.
This moves the emulated simtime into emubd/kiwibd, and extends the idea
with both per-byte and per-op timing estimates for hopefully more
realistic results.
---
The problem is how NAND flash reads work.
Per-byte timing estimates are surprisingly accurate for NOR flash. There
is some overhead for sending the address, but it's mostly dominated by
bus cost (~20ns/B [1]).
NAND flash, on the otherhand, technically does support byte-level reads,
but first needs to read into 2KiB buffer. Surprisingly, these are pretty
close in cost (~19ns/B bus [2] vs ~12ns/B buffer [2]).
This close-ness makes modeling NAND flash difficult. If we set
read_size=1, we risk hiding the cost of small reads, which littlefs3 is
full of (rbyd lookups). If we set read_size=2048, we unfairly penalize
littlefs3 for the same reason.
---
The solution here is to expose both per-byte and per-op timing
estimates. This lets you model NAND reads using two data points:
^
| realtime --> ...............o
| : .....'''' :
| ...............:'''' ^ :
| :....''''' | :
| ..........:::::: simtime :
| .....:'''' :
|o....:::::.....: :
|: :
|: :
+:-----------------------------------------------------------:>
min read max read
Where:
bus_timing = 19ns
buffer_timing = 25us
buffer_size = 2KiB
erase_size = 128KiB
min_read = buffer_timing
max_read = (erase_size/buffer_size)*buffer_timing - buffer_timing
read_timing = min_read
readed_timing = ((max_read - min_read)/erase_size) + bus_timing
simtime = reads*read_timing + readed*readed_timing
(per-op) (per-byte)
This should correctly penalize small reads without complicating
emubd/kiwibd too much.
That's the idea anyways! It will take some use to understand if this is
a reasonable approach.
As a plus, this is a superset of the per-byte model, so both can be used
for realistic vs idealistic simulations (and to test the bus+buffer
model itself).
1: https://www.winbond.com/resource-files/W25Q256JV%20SPI%20RevQ%2002072025%20Plus.pdf
2: https://www.winbond.com/resource-files/W25N01GV%20Rev%20R%20070323.pdf
The big TEST_IMPLICIT_DEFINES and TEST_CFG macros have been a big
pain-in-the-ass to maintain. Mostly due to C preprocessor annoyances
(bleh escaped newlines) and no-ifdef workarounds, which make a real mess
of things.
This does two things:
1. Moves all the defines out of test_runner.h and into test_defines.h
(same for benches).
2. Inverts the include logic such that test_defines.h gets included many
times with various "query macros" defined.
Currently just two, but can easily add more:
1. TEST_DEFINE(name, value) - name and default value for a define
2. TEST_CFG(name, value) - name and value for a cfg field
This seems to work surprisingly well. It solves all of the above C
preprocessor issues, and provides a flexible method for defining test
defines.
Note an important part of making this work is that test_defines.h
expands to an empty string by default.
I just noticed we weren't testing preerase with non-0xff ecksums at all!
Added to relevant tests:
# test with a number of different erase values
defines.ERASE_VALUE = [0xff, 0x00, -1]
The most important non-0xff value being -1 (noop erases), which should
usually result in fragmented ecksums.
Note this was copied from test_rbyd, where we do something similar to
test non-0xff ecksums in rbyd logs.
Based on what we implemented for the preerase gbmap known window check,
adding one block_count before mod is way simpler than a negative-friendly
mod in C.
Saves a bit of code:
code stack ctx
before: 35260 2136 660
after: 35256 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38616 2144 776
gbmap after: 38612 (-0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39280 2168 796
preerase after: 39276 (-0.0%) 2168 (+0.0%) 796 (+0.0%)
This gets gc tests working with both LFS3_GC=1 and LFS3_PREERASE=1, and
adds a few more tests that should round out the necessary preerase test
coverage:
- test_gc_preerase_progress - A simple test that checks if
LFS3_GC_PREERASE clears the LFS3_I_PREERASE flag, as well as some
checks against emubd's erase counters to see if it actually did
anything (erased >= cycles - preerased, erased < 1.25*cycles -
preerased).
- test_gc_preerase_relaxed - A test with a couple different
GC_PREERASE_COUNTs, and checks against emubd's erase counters to make
sure they demonstrate different levels of pre-erasing (erased >=
cycles - preerased, erased < 1.25*cycles - preerased).
- test_gc_preerase_decreasing - A test with increasing
GC_PREERASE_COUNTs, measuring min/max/avg emubd's erase counters, and
asserting if the avg delta is worst than ~0.75x.
This is probably the most valuable one, if only for the extra analysis
available when debugging.
And, just so we know these tests are working, they found a few more bugs:
- We were calling the implicitly ckpointing variant of lfs3_mdir_commit
in lfs3_allocclaim, when the block we just allocated is still very
much in-flight!
An easy one-character fix (lfs3_mdir_commit -> lfs3_mdir_commit_, the
non-ckpointing variant), but was a pain to track down. I guess the
good news is test_gc_nospc has proven to be a very valuable test.
Added a comment to hopefully discourage a regression.
- Found a wacky catch-22 where the block we just preerased can be
allocated during the gbmap commit that tries to save the preerased
ecksum.
This is somewhat expected during normal operation, the gbmap may need
a few allocations before the preeraser can get ahead, but we need to
make sure not to increment the preeraser's known window if the
preerased block is no longer in the gbmap's known window.
Fortunately(?), our preeraser state is pretty robust to bugs like this
due to being reset (forcing ecksum refetches) during gbmap rebuilds.
However, preeraser state falling out-of-sync risks unnecessary
erases/surprising latency during block allocation.
- Found a typo where we used lfs3->cfg->block_count instead of
lfs3->block_count again... Hopefully this becomes impossible after the
planned config rework...
---
A few other test tweaks:
- Added LFS3_F/M_REVPERTURB flags where necessary to support PREERASE.
Previously the tests only worked with LFS3_YES_REVPERTURB=1.
- Adopt lfs3_handle_isopen over lfs3.handles == lfs3.gc.t.h. With the
logic change to use the traversal handle to track its position in the
open file handles, these simplified isopen checks no longer work.
- Prefer toml lists for multiple ifdefs (hey, these were at least useful
for testing test.py's ifdef exprs).
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38616 2144 776
gbmap after: 38616 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39232 2168 796
preerase after: 39280 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
- Fixed lfs3_alloc_cansyncgbmap ignoring known window changes.
Being able to just call lfs3_btree_cmp(b, b_p) would be nice, but this
ignores known window changes!
Fixed by comparing the on-disk encoding, which is heavy-handed, but
probably the safest approach.
lfs3_alloc_cansyncgbmap will probably never be on the stack hot-path,
and the added code is roughly one function call. The main cost is
CPU-cycles, but fortunately(?) that's not something we really care
about?
- Fixed lfs3_allocclaim accidentally returning lfs3_mdir_commit's return
value instead of the allocated block!
Probably caused by a copy-paste, resulted in lfs3_allocclaim returning
block 0, which is really not good!
- Fixed assert typo in lfs3_trv_open where we assert REVPERTURB in flags
instead of lfs3->flags.
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38560 2144 776
gbmap after: 38616 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39168 2168 796
preerase after: 39232 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
- Delayed defines/permutations assignment until after generation. Just a
bit of code smell.
- Expanded all __eq__, __ne__, __lt__, __gt__, etc magic methods, just
to minimize surprises in the future.
This extends our ifdef/ifndef test attributes to support more
complicated logic expressions.
So far we haven't really needed this (ifdef/ifndef accepts an implicitly
anded list, which has covered everything so far), but I realized there's
a simple trick to make this work.
For example, in test.toml:
ifdef = 'A && !(B || C)'
Generated ifdef:
#if (defined(A) && !(defined(B) || defined(C)))
This doesn't require complex parsing or anything, just a simple regex:
s/[a-zA-Z_0-9]\+/defined(&)/g
Is using #if defined(A) everywhere instead of #ifdef A more expensive
for the compiler? Not sure. But it seems like we're heavily dominated by
the single-threaded link time, so I'm not sure we care.
Still needs testing with LFS3_GC=1, and tests that intentionally test
preerasing, but this should at least fix most fsinfo.flag related issues.
Despite no intentional preerase testing, this already found a number of
issues. Most importantly: our ckpoint-agnostic gbmap zeroing was never
going to work with preerasing!
Main fixes:
- Adopted conservative zeroing of gbmap during rebuilds
This was the biggest change. Our previous lfs3_gbmap_zero impl was
never going to work with preerasing because it unconditionally
cleared BMERASED ranges.
Not entirely wrong, but a big waste of any preerase work.
It also causes the whole system to lock up when LFS3_GC_LOOKAHEAD and
LFS3_GC_PREERASE fight to make progress. With LFS3_GC_LOOKAHEAD
clearing BMERASED ranges, and LFS3_GC_PREERASE clearing the lookahead
flag, nothing gets done!
---
The fix was to rewrite lfs3_gbmap_zero[unknown] to only zero BMERASED
(and BMINUSE, though this isn't strictly necessary) ranges in the
unknown window. This keeps any known-preerased blocks around and
avoids throwing that information away.
This is also slightly different from BMBAD ranges, which we want to
keep around forever, even if in the unknown window.
Whether or not was should limit zeroing BMINUSE ranges is an
interesting question. If we already need this logic, I think extending
it to BMINUSE is a good idea because of how it limits gbmap commits
during rebuilds:
- Unfortunately, gbmap rebuilds require quite a few commits to both
(1) zero gbmap state, and (2) set all the in-use blocks to BMINUSE.
This is especially concerning when relying on aggressive gc, such as
gc_lookgbmap=-1, which may trigger rebuilds when only a couple
blocks are allocated.
Limiting zeroing limits gbmap commits in two ways:
1. We only need to update ranges in the unknown window, which
shrinks with more aggressive gbmap rebuilds.
2. By not clearing BMINUSE ranges in the known window, populating
those blocks during the lookgbmap scan should be a noop.
Together, this hopefully makes aggressive gbmap rebuilds relatively
cheap, at least in terms of progs/erases.
- It's slightly simpler if BMINUSE and BMERASED are handled the same.
- Actually increment the preeraser known window in lfs3_alloc_inc.
Otherwise our estimated preeraser.count only ever increases! There was
some trickiness to make sure preeraser.count is only ever decremented
when allocating erased blocks, but fortunately lfs3->gbmap.ecksum's
existence can tell us that information.
- Reset preeraser state during gbmap rebuilds.
Also necessary to avoid unbounded preeraser.count. The simplest
solution is to zero the preeraser, which forces it to rescan the gbmap
for BMERASED ranges. The preeraser strictly avoids redundant erases.
This does require extra gbmap lookups during LFS3_GC_PREERASE, but
that's not the end of the world.
- Avoid erasing corrupted preerased blocks in case there's other
preerased blocks available in our gbmap.
This happens when the ecksum check fails, implying a prog was
attempted, but power was lost.
Before this change (the continue in lfs3_alloc_:11244), we were
erasing corrupt ecksums, which is not _wrong_, but sort of defeats the
purpose of prerasing. Skipping the block and trying another:
1. Is better in terms of wear-leveling (try not to double erase!)
2. Minimizes latency if we have other preerased blocks we can use
- Made lfs3_fs_gc_ preerasing actually conditional on the
LFS3_GC_PREERASE flag.
Before, lfs3_fs_gc_ was unconditionally preerasing, which is wrong!
---
Currently passing:
LFS3_YES_GBMAP=1 \
LFS3_YES_REVPERTURB=1 \
LFS3_PREERASE=1 \
make test-runner -j \
& ./scripts/test.py -j -b
Other test fixes:
- Mostly just adding the necessary LFS3_I_PREERASE flags for all
lfs3_fs_stat calls.
- LFS3_I_PREERASE and LFS3_I_LOOKAHEAD can interact in funny ways. Just
needed testing.
- lfs3_trv_t doesn't actually do anything with LFS3_T_PREERASE, so we
shouldn't try to test it.
- Adopted lfs3_fs_ck instead of explicit traversals where possible.
- test_badblocks_*_btree_many was still running with LFS3_YES_GBMAP, but
it shouldn't be. The gbmap state is undefined during internal btree
tests.
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38492 2144 776
gbmap after: 38560 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39036 2168 796
preerase after: 39168 (+0.3%) 2168 (+0.0%) 796 (+0.0%)
This started with adding -d/--diff support to dbgflags.py, which is very
useful for comparing flags during test failure.
The flag asserts in our tests generally look like this:
tests/test_mount.toml:171:assert: assert failed with 33570064,
expected eq 33570576
assert(fsinfo.flags == (
Which can now be quickly compared with dbgflags.py:
$ ./scripts/dbgflags.py +i 33570064 -d 33570576
LFS3_I_GBMAP 0x02000000 Global on-disk block-map in use
LFS3_I_REVPERTURB 0x00000010 Mounted with LFS3_M_REVPERTURB
LFS3_I_MKCONSISTENT 0x00000100 Filesystem needs mkconsistent to write
-LFS3_I_LOOKAHEAD 0x00000200 Lookahead buffer is not full
LFS3_I_PREERASE 0x00000400 Blocks can be pre-erased
LFS3_I_COMPACT 0x00000800 Filesystem may have uncompacted metadata
LFS3_I_CKMETA 0x00001000 Metadata checksums not checked recently
LFS3_I_CKDATA 0x00002000 Data checksums not checked recently
The assert print is a bit more annoying than it needs to be, as it only
prints in decimal. But, since our prettyasserts.py only works at the
syntax layer, it's not possible to make it any smarter.
---
To make this diffing work required a couple more features in our
self-parsing Flag class:
- Keep track of lineno, mainly for ordering things
- Moved find logic into a staticmethod on all classes
- Added _sentinel based defaults to find functions
- Allowed self to be non-class in line functions to deduplicate "Unknown
flag" messages
I went ahead and extended these to the other self-parsing classes (Err
and Tag) in case they're useful in the future.
This was broken. The good news is this was easily detected by our
test_mtree tests.
The problem is that mroot split + drop (resulting in an mtree with one
mdir) is indistinguishable from mroot relocation via mdelta. Both cases
have an mdelta of 0.
This also breaks the later mid-mdir update if we wanted to stay on the
current chain mroot.
The tricky part is we have several entangled cases:
- mdir=active mroot, mid>=0 - follow mdir_
- mdir=active mroot, mid<=-1 - follow mroot_, not mdir_!
- mdir=chain mroot, mid<=-1 - follow mdir_, not mroot_!
---
It's tempting to rely on mid<=-2 vs mid==-1 for chain mroots vs active
mroot, but this doesn't always work! During traversals mid is always
<=-2, in part because we don't actually know if the current mroot is the
active mroot until we try to lookup its child.
Fortunately, what _does_ work is just comparing against the mroot's
blocks, which we know.
Though the continued reliance and reliability of mptr comparisons makes
me wonder if it's possible to simplify said function...
Code changes:
code stack ctx
before: 35224 2136 660
after: 35260 (+0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38464 2144 776
gbmap after: 38492 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39008 2168 796
preerase after: 39036 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
Maybe the subject line should say "Implemented", because these never
worked in the first place. Unfortunately our tests missed this due to a
couple reasons:
- Mroot chains are difficult to create due to the required exponential
growth.
- The only thing that actually commits to chain mroots is mdir
compaction. Though this functionality will be useful for future block
eviction/error correction.
- Previous revision count issues were making relocations in our
compaction tests unlikely.
Fortunately, now that revision count behavior is more correct, our tests
are correctly highlighting that this is broken.
---
Implementing chain mroot commits was a bit intimidating, but fortunately
it just required a bit of teasing to get lfs3_mdir_commit_ to trigger
the tail-recursive mroot chain update when the mdir is a non-active
mroot.
The gcksum is also doing a great job here with identifying bugs. Without
it this bug would have been difficult to notice, since compactions
otherwise have no observable effect on the system.
Code changes:
code stack ctx
before: 35164 2136 660
after: 35224 (+0.2%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38400 2144 776
gbmap after: 38464 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38940 2168 796
preerase after: 39008 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
This is a common pitfall with mdirs that has only ended up in the
codebase ~3(?) times. Naively using a power-of-two recycle counter for
relocations ends up aliasing mdir blocks such that only one block is
actually wear-leveled.
I'm not entirely sure it was intentional, but the previous
double-increment during needsrelocation checks made mdirs relocate one
recycle early, avoiding this aliasing issue.
However, the double-increment had other issues. The most glaring is that
it would always trigger two relocations back-to-back due to the mismatch
between counter cycles and overflow checks. Sort of defeating the
purpose of wear-leveling the two mdir blocks separately...
---
What we really want is a counter that's always coprime with 2. Such as
our old friend mod (2^n)-1.
Unfortunately mod (2^n)-1 counters don't really have any great
optimization trick. They show up all the time when code relies on the
multiplicative cycle of a 2^n finite-field, but despite this, all of
the implementations I've seen rely on a simple branch to handle the
one extra state.
I'm not sure this is the best implementation, but adding 2 and
subtracting 1 on non-overflow seems to minimize resulting code cost.
Presumably because the compiler is able to deduplicate this with the
needsrelocation check. Though we're only talking about a handful of
bytes.
Code changes:
code stack ctx
before: 35152 2136 660
after: 35164 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38388 2144 776
gbmap after: 38400 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38928 2168 796
preerase after: 38940 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
---
We don't have great tests for this, as it's difficult to create
rigorous checks for our best-effort dynamic wear-leveling. But,
surprisingly enough, this _was_ caught by
test_exhaustion_spam_uzd_fuzz's doubling-disk-doubles-lifetime check!
Though only with LFS3_YES_GBMAP=1:
LFS3_YES_GBMAP=1 \
TESTS=tests/test_exhaustion.toml \
make test-runner -j \
&& ./scripts/test.py test_exhaustion_spam_uzd_fuzz -O- -j \
| grep lifetime
rbyd.weight == btree.weight does not imply rbyd is a bshrub root!
This was introduced during a btrv rework, and, unfortunately, works
_most_ of the time. It's extra deceptive because we eagerly collapse
these degenerate roots in lfs3_btree_commit_, but we can't collapse
bshrub roots!
Well, not easily anyways (I guess we could convert to a btree...), but
what's important is that single-entry btree nodes are possible, and
relying on the weight for shrubbed roots is a weak condition.
Instead, we now just check the shrub bit for shrubbed roots. We have a
whole bit for this, so might as well actually use it.
This isn't even the first reliance of the shrub bit in this function!
---
Code changes minimal:
code stack ctx
before: 35148 2136 660
after: 35152 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38388 2144 776
gbmap after: 38388 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38928 2168 796
preerase after: 38928 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
A simple but nasty typo! Quite confusing to figure out.
Valgrind was quick to highlight that mtortoise was uninitialized, but
without any sort of debugger support, it took many _many_ rereadings of
the code to figure out what was actually going wrong. I even started to
wonder if C's union aliasing rules were the culprit.
To make matters worse, I only noticed Valgrind's warning because I was
trying to find a heisenbug that turned out to be unrelated.
Code changes minimal:
code stack ctx
before: 35144 2136 660
after: 35148 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38384 2144 776
gbmap after: 38388 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38924 2168 796
preerase after: 38928 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
This was resulting in memory leak warnings from Valgrind, which were
getting in the way of debugging an unrelated uninitialized memory issue.
We normally wouldn't care about this sort of bounded memory leaks, but
in this case Valgrind can't tell if the memory leak is from the runner
or filesystem, errors, and prevents other tests from running. Just to be
more annoying, this only triggered when overriding defines, which is
something you do exactly when you are trying to debug something.
Fortunately, with a bit of typecasting we still have access to the
allocated value arrays (type-stripped due to opaque test_define_t), and
can clean up the relevant memory.
Whoops, looks like the lfs3_alloc refactoring resulted in us calling
lfs3_alloc_inc multiple times. In effect allocating multiple blocks when
triggered, wasting erased-state and lookahead scans.
The good(?) news is this was only triggered when we failed to erase a
block, which made it difficult for our tests to catch.
---
Saves a bit of code when we don't do unnecessary work:
code stack ctx
before: 35152 2136 660
after: 35144 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38392 2144 776
gbmap after: 38384 (-0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38928 2168 796
preerase after: 38924 (-0.0%) 2168 (+0.0%) 796 (+0.0%)
This was introduced with the simplified traversal clobbering logic.
Previously, traversal clobbering was a bit more aggressive, relying on
the explicit tstate state machine. This was replaced by implicit
mid-related state, which looks like it may have introduced some holes.
In this case, lfs3_mdir_commit was failing to clobber non-active mroot
chain mdirs. Non-active mroots are particularly tricky because we
(1) don't track these in-RAM, (2) only reach them during traversals,
and (3) require heavy wear-leveling writes for them to even appear in
in system.
---
The solution here is an extra check in lfs3_mdir_commit_'s post-commit
state updates to update any mid<=-1 mroots to the new active mroot.
This clobbers mroot chain traversals by skipping non-active mroots, but
this is unavoidable since lfs3_mdir_commit_ could always introduce
new/relocate mroot chain mroots. Note this should match the previous
state-machine dependent behavior.
Code changes:
code stack ctx
before: 35144 2136 660
after: 35152 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38380 2144 776
gbmap after: 38392 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38920 2168 796
preerase after: 38928 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
Previously these only implied M/F flags, which risks quite a bit of
confusion. It's entirely reasonable to expect these to affect lfs3_fs_gc
(arguably the more correct behavior?) but they did not.
Maybe these should imply the GC behavior, or maybe we should rename them
to LFS3_YES_GC_*/LFS3_YES_M_*/etc, but at the very least the current
behavior of implying M/F is probably not a good idea.
So, removing for now. This is the safest option, and better thought-out
behavior can be added in the future.
The original motivation for making LFS3_PREERASE opt-out, is that it
makes sense for LFS3_GBMAP to bring in all gbmap-related features
(PREERASE, BADBLOCKS (future)). However, after a bit of use, I think
this just complicates our ifdef logic too much.
So instead, LFS3_PREERASE is now opt-in, with the intention of making
all ifdefs relative only to the default build. I think this will make it
easier to reason about ifdefs, at least internally.
Eventually, I want to look into alternative default builds (LFS3_BIGGER,
LFS3_BIGGERR, ..., LFS3_BIGGEST), which would provide an alternative way
to enable all gbmap-related features. Though these builds have a
high-risk of bikeshedding (LFS3_GC?), so we'll see.
---
That being said, the main ergonomic improvement was probably adding
a #error, so we don't have to check ifdef GBMAP everywhere.
Maybe this should be extended to LFS3_RDONLY? Or maybe not, LFS3_RDONLY
is a bit of a special case.
No code changes:
code stack ctx
before: 35144 2136 660
after: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38380 2144 776
gbmap after: 38380 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38920 2168 796
preerase after: 38920 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
It's two words, and I've probably already spent too long fiddling around
with this.
This drops the messy NULL checks in lfs3_gbmap_set_, for a wrapper that
defaults to a global {.cksize=-1} ecksum when NULL.
---
Code changes, with both a compound literal (cl), and global
constant (gc). The current code uses a global constant:
code stack ctx
before: 35144 2136 660
after+cl: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
after+gc: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38272 2144 776
gbmap+np after+cl: 38412 (+0.4%) 2144 (+0.0%) 776 (+0.0%)
gbmap+np after+gc: 38380 (+0.3%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38940 2168 796
gbmap+yp after+cl: 38952 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
gbmap+yp after+gc: 38920 (-0.1%) 2168 (+0.0%) 796 (+0.0%)
It's interesting to note that while the global constant generally
reduces code cost, it prevents constant-expr optimizations from
eliminating the NULL checks when compiling without preerases (np).
Is that enough reason to revert this? Probably not. (1) The simpler
codebase, and reduced chance of forgetting a NULL check, is preferable,
and (2) we don't care about the code cost of niche gbmap configurations
as much as the non-gbmap modes.
This mostly reverts the previous commit, and makes non-NULL ecksums the
consistent API.
Non-NULL ecksums are what the original rbyd-level ecksum API expects,
and enforcing this avoids the ifdef mess required to minimize unused
code impact.
This unfortunately clutters up lfs3_gbmap_set_'s logic with NULL checks,
but at least keeps the mess constrained to lfs3_gbmap_set_.
lfs3_gbmap_set_ is really the only function that uses NULL ecksums, so
they should probably be lfs3_gbmap_set_'s problem to deal with.
Code changes:
code stack ctx
before: 35144 2136 660
after: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38296 2144 776
gbmap+np after: 38272 (-0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38908 2168 796
gbmap+yp after: 38940 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
Christopher Haster