The issue here is quite nuanced, but becomes a problem when you want to
both:
1. Filter results by a given field: -Dmeas=write
2. Output a new value for that field: -bmeas='"write+amor"'
If you didn't guess from the example, this comes up often in scripts
dealing with bench results, where we often find ourselves wanting to
append/merge modified results based on the raw measurements.
Fortunately the fix is relatively easy: We already filter by defines
in our collect function, so we don't really need to filter by defines
again when folding.
Folding occurs after expr evaluation, but collect occurs before, so this
limits filtering to the input fields _before_ expr evaluation.
This does mean we no longer filter on the output of exprs, but I don't
know if such behavior was ever intentionally desired. Worst case it can
be emulated by stacking multiple csv.py calls, which may be annoying,
but is at least well-intentioned and well-defined.
---
Note that the other result scripts, code.py, stack.py, etc, are a bit
different in that they rely on fold-time filtering for filtering
generated results. This may deserve a refactor at some point, but since
these scripts don't also evaluate exprs, it's not an immediate problem.
This may make some mathematician mad, but these are informative scripts.
Returning +-inf is much more useful than erroring when dealing with
several hundred rows of results.
And hey, if it's good enough for IEEE 754, it's good enough for us :)
Also fixed a division operator mismatch in RFrac that was causing
problems.
Not sure if this is an old habit from Python 2, or just because it looks
nicer next to __mul__, __mod__, etc, but in Python 3 this should be
__truediv__ (or __floordiv__), not __div__.
This is now inconsistent with csv.py, and I don't really want to add a
full expr parser to every script that might want to rename fields.
Field renaming (or any expr really!) can be accomplished with
intermediate calls to csv.py anyways. No reason to make these scripts
more complicated than they need to be.
The only reason RFloats reused RInt's operator definitions was to save a
few keystrokes. But this dependency is unnecessary and will get in the
way if we ever add a script that only uses RFloats.
So now the available field exprs can be queried with --help-exprs:
$ ./scripts/csv.py --help-exprs
uops:
+a Non-negation
-a Negation
!a 1 if a is zero, otherwise 0
bops:
a * b Multiplication
a / b Division
... snip ...
I was a bit torn on if this should be named --help-exprs or --list-exprs
to match test.py/bench.py, but decided on --help-exprs since it's
querying something "inside" the script, whereas test.py/bench.py's
--list-cases is querying something "outside" the script.
Internally this uses Python's docstrings, which is a nice language
feature to lean on.
Mainly for consistency with int operators, though it's unclear if either
mod is useful in the context of csv.py and related scripts.
This may be worth reverting at some point.
Now, by default, an error is raised if any branch of an expr has an
inconsistent type.
This isn't always what we want. The ternary operator, for example,
doesn't really care if the condition's type doesn't match the branch
arms. But it's a good default, and special cases can always override the
type function with their own explicit typechecking.
There's a bit of a push and pull when it comes to typechecking CSV
fields in our scripts. On one hand, we want the flexibility to accepts
scripts with various mismatched fields, on the other hand, we _really_
want to know if a typo caused a field to be quietly replaced with all
zeros...
I _think_ it's safe to say: if no fields across _all_ input files match
a requested field, we should error.
But I may end up wrong about this. Worst case we can always revert in
the future, maybe with an explicit flag to ignore missing fields.
- Updated the example in the header comment.
The previous example was way old, from back when fields were separated
by commas! Introduced in 20ec0be87 in 2022 according to git blame.
- Renamed a couple internal RExpr classes:
- Not -> NotNot
- And -> AndAnd
- Or -> OrOr
- Ife -> IfElse
This is mainly to leave room for bitwise operators in case we every
want to add them.
- Added isinf, isnan, isint, etc:
- isint(a)
- isfloat(a)
- isfrac(a)
- isinf(a)
- isnan(a)
In theory useful for conditional exprs based on the field's type.
- Accept +-nan as a float literal.
Niche, but seems necessary for completeness. Unfortunately this does
mean a field named nan (or inf) may cause problems...
I still think the 24 (23+1) char minimum is a good default for 2 column
output such as help text, especially if you don't have automatic width
detection. But our result scripts need to be a bit more flexible.
Consider:
$ make summary
code data stack structs
TOTAL 68864 0 3744 1520
Vs:
$ make summary
code data stack structs
TOTAL 68864 0 3744 1520
Up until now we were just kind of working around this with cut -c 25- in
our Makefile, but now that our result scripts automatically scale the
table widths, they should really just default to whatever is the most
useful.
- RInt/RFloat now accepts implicitly castable types (mainly
RInt(RFloat(x)) and RFloat(RInt(x))).
- RInt/RFloat/RFrac are now "truthy", implements __bool__.
- More operator support for RInt/RFloat/RFrac:
- __pos__ => +a
- __neg__ => -a
- __abs__ => abs(a)
- __div__ => a/b
- __mod__ => a%b
These work in Python, but are mainly used to implement expr eval in
csv.py.
- Allow single-arg frac:
- frac(a) => a/a
- frac(a, b) => a/b
This was already supported internally.
- Implicitly cast to frac in frac ops:
- ratio(3) => ratio(3/3) => 1.0 (100%)
- total(3) => total(3/3) => 3
This makes a bit more sense than erroring.
This now returns 1.0 if the total part of the fraction is 0.
There may be a better way to handle this, but the intention is for 0/0
to map to 100% for thing like code coverage (cov.py), test coverage
(test.py), etc.
So csv.py should now be mostly feature complete, aside from bugs.
I ended up dropping most of the bitwise operations for now. I can't
really see them being useful since csv.py and related scripts are
usually operating on purely numerical data. Worst case we can always add
them back in at some point.
I also considered dropping the logical/ternary operators, but even
though I don't see an immediate use case, the flexibility
logical/ternary operators add to a language is too much to pass on.
Another interesting thing to note is the extension of all fold functions
to operate on exprs if more than one argument is provided:
- max(1) => 1, fold=max
- max(1, 2) => 2, fold=sum
- max(1, 2, 3) => 3, fold=sum
To be honest, this is mainly just to allow a binary max/min function
without awkward naming conflicts.
Other than those changes this was pretty simple fill-out-the-definition
work.
This was more tricky than expected since Python's class scope is so
funky (I just eneded up with using lazy cached __get__ functions that
scan the RExpr class for tagged members), but these decorators help avoid
repeated boilerplate for common expr patterns.
We can even deduplicate binary expr parsing without sacrificing
precedence.
This is a work-in-progress, but the general idea is to replace the
existing rename mechanic in csv.py with a full expr parser:
$ ./scripts/csv.py input.csv -ba=x -fb=y+z
I've been putting this off for a while, as it feels like too big a jump
in complexity for what was intended to be a simple script. But
complexity is a bit funny in programming. Even if a full parser is more
difficult to implement, if it's the right grammar for the job, the
resulting script should end up both easier to understand and easier to
extend.
The original intention was that any sufficiently complicated math could
be implemented in ad-hoc Python scripts that operate directly on the CSV
files, but CSV parsing in Python is annoying enough that this never
really worked well.
But I'm probably overselling the complexity. This is classic CS stuff:
1. build a syntax tree
2. map symbols to input fields
3. typecheck, fold, eval, etc
One neat thing is that in addition to providing type and eval
information, our exprs can also provide information on how to "fold" the
field after eval. This kicks in when merging muliple rows when grouping
by -b/--by, and for finding the TOTAL results.
This can be used to merge stack results correctly with max:
$ ./scripts/csv.py stack.csv \
-fframe='sum(frame)' -flimit='max(limit)'
Or can be used to find other interesting measurements:
$ ./scripts/csv.py stack.csv \
-favg='avg(frame)' -fstddev='stddev(frame)'
These changes also make the eval order of input/output fields much
stricter which is probably a good thing.
This should replace all of the somewhat hacky fake-expr flags in csv.py:
- --int => -fa='int(b)'
- --float => -fa='float(b)'
- --frac => -fa='frac(b)'
- --sum => -fa='sum(b)'
- --prod => -fa='prod(b)'
- --min => -fa='min(b)'
- --max => -fa='max(b)'
- --avg => -fa='avg(b)'
- --stddev => -fa='stddev(b)'
- --gmean => -fa='gmean(b)'
- --gstddev => -fa='gstddev(b)'
If you squint you might be able to see a pattern.
This seems like a more fitting name now that this script has evolved
into more of a general purpose high-level CSV tool.
Unfortunately this does conflict with the standard csv module in Python,
breaking every script that imports csv (which is most of them).
Fortunately, Python is flexible enough to let us remove the current
directory before imports with a bit of an ugly hack:
# prevent local imports
__import__('sys').path.pop(0)
These scripts are intended to be standalone anyways, so this is probably
a good pattern to adopt.
This extends Rbyd.fetch to accept another rbyd, in which case we inherit
the RAM-backed block without rereading it from disk. This avoids an
issue where shrubs can become corrupted if the disk is being
simultaneously written and debugged.
Normally we can detect the checksum mismatch and toss out the rbyd
during fetch, but shrub pointers don't include a checksum since they
assume the containing rbyd has already been checksummed.
It's interesting to note this even avoids the memory copy thanks to
Python's reference counting.
If we're fetching branches anyways, we might as well check that the
checksums match. This helps protect against infinite loops in B-tree
branches.
Also fixed an issue where we weren't xoring perturb state on finding an
explicit trunk.
Note this is equivalent to LFS_M_CKFETCHES in lfs.c.
---
This doesn't mean we always need LFS_M_CKFETCHES. Our dbg scripts just
need to be a little bit tougher because 1. running tests with -j creates
wildly corrupted and entangled littlefs images, and 2. Rbyd.fetch is
almost too forgiving in choosing the nearest trunk.
These work by keeping a set of all seen mroots as we descend down the
mroot chain. Simple, but it works.
The downside of this approach is that the mroot set grows unbounded, but
it's unlikely we'll ever have enough mroots in a system for this to
really matter.
This fixes scripts like dbgbmap.py getting stuck on intentional mroot
cycles created for testing. It's not a problem for a foreground script
to get stuck in an infinite loop, since you can just kill it, but a
background script getting stuck at 100% CPU is a bit more annoying.
A typo meant we were setting all case-level flags to suite-level flags
in bench.py. And because suite-level flags are more-or-less just ored
case-level flags, all case-level flags would end up shared.
Fixed via untypo.
This matches the style used in C, which is good for consistency:
a_really_long_function_name(
double_indent_after_first_newline(
single_indent_nested_newlines))
We were already doing this for multiline control-flow statements, simply
because I'm not sure how else you could indent this without making
things really confusing:
if a_really_long_function_name(
double_indent_after_first_newline(
single_indent_nested_newlines)):
do_the_thing()
This was the only real difference style-wise between the Python code and
C code, so now both should be following roughly the same style (80 cols,
double-indent multiline exprs, prefix multiline binary ops, etc).
Mainly to avoid conflicts with match results m, this frees up the single
letter variables m for other purposes.
Choosing a two letter alias was surprisingly difficult, but mt is nice
in that it somewhat matches it (for itertools) and ft (for functools).
So now the hot path participates in sorting, folding, etc:
$ ./scripts/stack.py ./lfs.ci ./lfs_util.ci \
-Dfunction=lfsr_mount -t -sframe
function frame limit
lfsr_mount 96 2736
|-> lfsr_mdir_commit 512 2368
|-> lfsr_btree_commit__.constprop 336 1648
|-> lfs_alloc 272 1296
|-> lfsr_btree_commit 208 1856
|-> lfsr_btree_lookupnext_ 208 720
|-> lfsr_mtree_gc 192 2560
|-> lfsr_mtree_traverse 176 1024
|-> lfsr_rbyd_lookupnext 160 448
|-> lfsr_bd_readtag.constprop 128 288
|-> lfsr_mtree_lookup 128 848
|-> lfsr_bd_read 80 160
|-> lfsr_bd_read__ 80 80
|-> lfsr_fs_gc 80 2640
|-> lfsr_rbyd_sublookup 64 512
'-> lfsr_rbyd_alloc 16 1312
TOTAL 96 2736
This risks some rather unintuitive behavior now that the hot path
rendering no longer matches the call stack, but in theory the extra
sorting features are more useful?
This is a bit of an experiment, if this is more confusing than useful,
we can always revert to the strict call-order ordering.
Note that you can _usually_ get the call-order ordering by sorting by
limit, but this trick breaks if any call frames are zero sized...
This fixes an issue where mixing recursive renderers (-t/--hot or
-z/--depth) with defines (-Dfunction=lfsr_mount) would not account for
children entry widths. An unexpected side-effect of no longer filtering
the children entries.
We could continue to try to estimate the width without table rendering,
but it would basically need two full recursive pass at this point...
Instead, I've just moved the recursive stuff before table rendering,
which should remove any issues with width calculation while also
deduplicating the recursive passes.
It's invasive for a small change, but probably worthwhile long term.
The downside is this does mean our recursive scripts now build the full
table (including all recursive calls!) before they start printing. When
mixed with unbounded recursive depth (-z0 or --depth=0) this can get
quite large and cause quite a slow start.
But I guess that was the tradeoff in adopting this sort of intermediate
table rendering... At least it does make the code simpler and less bug
prone...
This makes -D/--define more useful in stack.py/perf.py/perfbd.py by no
longer hiding undfined children entries.
For example:
$ ./scripts/stack.py lfs.ci lfs_util.ci -Dfunction=lfsr_mount -t
function frame limit
lfsr_mount 96 2816
|-> lfsr_fs_gc 80 2720
|-> lfsr_mtree_gc 176 2640
|-> lfsr_mdir_commit 576 2464
... snip ...
Now shows all functions in the hot path of lfsr_mount, where before it
would only show functions in the hot path of lfsr_mount that were also
_named_ lfsr_mount.
The previous behavior was technically not wrong... but not very useful
(and confusing).
---
This was actually quite a bit annoying to get working because of the
possibility of function call cycles.
I ended up turning stack.py's result type into a fully connected graph,
which only works because Python has a cycle detector. (Actually this
script is so short-lived we probably wouldn't care if this leaked
memory.)
A nice side effect of this is now all the recursive scripts (stack.py,
perf.py, and perfbd.py) share the same internal result representation
and recursive printing logic, which is probably a good thing.
To better match -z/--depth and -t/--hot.
The fact that these short forms all don't match the first letter of the
long forms is humorous but unintentional. There's only so many letters
in the alphabet!
As a convenience, -d/--diff in our measurement scripts hides entries
that are unchanged by default.
Unfortunately this was broken during a recent refactor that ended up
filtering the line info but not the actual names.
Instead of reverting the broken part of the refactor, I've just moved the
filtering up to where we calculate the names. Hopefully this fixes the
bug while also simplifying this messy chunk of a logic a bit.
This is mainly useful for stack.py, where -t/--hot lets you quickly see
everything that contributes to the stack limit for each function.
This was (and still is) possible with -s + -z, but it was pretty
annoying to use:
- The stack trace rendered _diagonally_ as a consequence of -z, which is
probably the worst use of screen real estate.
- This trick only really worked with -s, which was the opposite order of
what you usually want on the command line: -S.
Adding a special for-purpose -t/--hot flag makes looking at the hot path
much easier, at the cost of more hacky python code (and I _mean_ hacky,
making the hot path selection useful while following exising sort rules
was annoyingly complicated).
Also added -t/--hot to perf.py and perfbd.py for consistency, though it
makes a bit less sense there.
Also also reworked related code in all three scripts: stack.py, perf.py,
perfbd.py. The logic should be a bit more equivalent, and
perf.py/perfbd.py detect cycles now.
This is a pretty classic case for memoization. We don't really need to
recalculate every stack limit at every call site.
Cuts the runtime in half:
before: 0.335s
after: 0.139s (-58.5%)
---
Unfortunately functools.cache was not fit for purpose. It's stuck using
all parameters as the key, which breaks on the "seen" parameter we use
for cycle detection that otherwise has no impact on results.
Fortunately decorators aren't too difficult in Python, so I just rolled
my own (cache1).
stack.py actually already had a simple cycle detector, since we needed
one to calculate stack limits without getting stuck.
Copying this simple cycle detector into the actual table rendering code
lets us print a nice little "cycle detected" message, instead of just
vomiting to stdout forever:
$ ./scripts/stack.py lfs.ci lfs_util.ci -z -s
function frame limit
lfsr_format 320 ∞
|-> lfsr_mountinited 304 ∞
| |-> lfsr_mountmroot 80 ∞
| | |-> lfsr_mountmroot 80 ∞ (cycle detected)
| | |-> lfsr_mdir_lookup 48 576
... snip ...
The cycle detector is a bit naive, just building a new set each step,
but it gets the job done.
As for perf.py and perfbd.py, it turns out they can't actually create
cycles, so no need for a cycle detector. This is good because I didn't
really want to test these scripts again :)
These functions provide simple access to littlefs's custom attributes,
which are small pieces of user-specified metadata that can be attached
to files, dirs, root, etc:
- lfsr_getattr - Reads an attribute
- lfsr_sizeattr - Gets the size of an attribute
- lfsr_setattr - Writes an attribute
- lfsr_removeattr - Removes an attribute
You may notice these functions look quite a bit different from their
previous incarnations. This is because the custom attribute API is
getting an overhaul based on feedback provided by users
The previous API had some real design flaws that interfered with
usability, but now that things have had some time to settle (6 years!),
hopefully most of the pain points are clear.
Notable changes:
- lfsr_getattr's return value is now limited by buffer size.
The intention of the previous API, where lfsr_getattr always returns
the attr size, even if it's larger than the buffer, was to allow users
to find the attr size without an infinitely large buffer.
In defense of this design, Linux's getxattr does something somewhat
similar, returning the attr size when the buffer size equals zero.
Though getxattr does truncate when buffer size is non-zero, which is
probably safer.
But, let's be honest, this multipurpose abuse of lfsr_getattr's return
value is inconsistent with other read functions and potentially
dangerous for users.
I think one of the reasons for this API in Linux-land is the limited
syscall numbers discouraging new functions, but we have no such
limitation here! We might as well add a dedicated function for
this: lfsr_sizeattr.
- No more padding with zeros!
This was a cludge to get around the lack of returned size in custom
attributes attached to files, but is inconsistent with other read
functions, so needs to go.
In general, inconsistencies violate user assumptions, and are usually
a sign of a bad API.
- lfsr_setattr now takes flags.
This gives lfsr_setattr more flexiblity in how it operates, and may
make future extensions easier.
lfsr_setattr currently supports two flags, which may look a bit
familiar:
LFS_A_CREAT 0x04 // Create an attr if it does not exist
LFS_A_EXCL 0x08 // Fail if an attr already exists
One long-term idea is to eventually add a simple lfsr_set function to
make it easier to create small files, so this sort of design overlap
between lfsr_setattr and lfsr_file_open is hopefully a good thing.
---
Code-wise, these function are really not that bad. Adding functions adds
code, but these are just small wrappers over our internal lookup/commit
functions:
code stack
before: 36556 2608
after: 37116 (+1.5%) 2608 (+0.0%)
Of course the real cost of custom attributes is how they interact with
open files, a detail which is conveniently missing for now...
It would be nice to have a full 8-bit range for both user attrs and
system attrs, for both backwards compatibility and maximizing the
available attr space, but I think it just doesn't make sense from an API
perspective.
Sure we could finagle the user/sys bit into a flags argument, or provide
separate lfsr_getuattr/getsattr functions, but asking users to use a
9-bit int for higher-level operations (dynamic attrs, iteration, etc) is
a bit much...
So this reduces the two attr ranges down to 7-bits, requiring 8-bits
total to store all possible attr types in the current system:
TAG_ATTR 0x0400 v--- -1-a -aaa aaaa
TAG_UATTR 0x04aa v--- -1-- -aaa aaaa
TAG_SATTR 0x05aa v--- -1-1 -aaa aaaa
This really just affects scripts, since we haven't actually implemented
attributes yet.
Worst case we still have the 9-bit encoding space carved out, so we can
always add an additional set of attrs in the future if we start running
into attr pressure.
Or, you know, just turn on the subtype leb128 encoding the 8th subtype
bit is reserved for. Then you'd only be limited by internal driver
details, probably 24-bits per attr range if we make tags 32-bits
internally. Though this would probably come with quite a code cost...
Updating the canonical checksum should only depend on if the tag is a
trunkish tag (not a checksum tag), and not if the tag is in the current
trunk. The trunk parameter to lfsr_rbyd_fetch should have no effect on
the canonical checksum.
Fixed in boath lfsr_rbyd_fetch and scripts.
Curiously no code changes:
code stack
before: 36416 2616
after: 36416 (+0.0%) 2616 (+0.0%
This moves all ckread-related logic behind the new opt-in compile-time
LFS_CKREADS flag. So in order to use ckreads you need to 1. define
LFS_CKREADS at compile time, and 2. pass LFS_M_CKREADS during
lfsr_mount.
This was always the plan since, even if ckreads worked perfectly, it
adds a significant amount of baggage (stack mostly) to track the
ck context of all reads.
---
This is the first non-trivial opt-in define in littlefs, so more test
framework features!
test.py and build.py now support the optional ifdef attribute, which
makes it easy to indicate a test suite/case should not be compiled when
a feature is missing.
Also interesting to note is the addition of LFS_IFDEF_CKREADS, which
solves several issues (and general ugliness) related to #ifdefs in
expression. For example:
// does not compile :( (can't embed ifdefs in macros)
LFS_ASSERT(flags == (
LFS_M_CKPROGS
#ifdef LFS_CKREADS
| LFS_M_CKREADS
#endif
))
// does compile :)
LFS_ASSERT(flags == (
LFS_M_CKPROGS
| LFS_IFDEF_CKREADS(LFS_M_CKREADS, 0)));
---
This brings us way back down to our pre-ckread levels of code/stack:
code stack
before-ckreads: 36352 2672
ckreads: 38060 (+4.7%) 3056 (+14.4%)
after-ckreads: 36428 (+0.2%) 2680 (+0.3%)
Unfortunately, we do end up with a bit more code cost than where we
started. Mainly due to code moving around to support the ckread
infrastructure:
code stack
lfsr_bd_readtag: +52 (+23.2%) +8 (+10.0%)
lfsr_rbyd_fetch: +36 (+5.0%) +8 (+6.2%, cold)
lfs_toleb128: -12 (-25.0%) -4 (-20.0%, cold)
total: +76 (+0.2%) +8 (+0.3%)
But oh well. Note that some of these changes are good even without
ckreads, such as only parsing the last ecksum tag.
I've been scratching my head over our rbyd perturb scheme. It's gotten
rather clunky with needing to xor valid bits and whatnot.
But it's tricky with needing erased-state to be included in parity bits,
while at the same time excluded from our canonical checksum. If only
there was some way to flip the checksums parity without changing its
value...
Enter the crc32c odd-parity zero: 0xfca42daf!
This bends the definition of zero a bit, but it is one of two numbers in
our crc32c-ring with a very interesting property:
crc32c(m) == crc32c(m xor 0xfca42daf) xor 0xfca42daf // odd-p zero
crc32c(m) == crc32c(m xor 0x00000000) xor 0x00000000 // even-p zero
Recall that crc32c's polynomial, 0x11edc6f41, is composed of two
polynomials: 0x3, the parity polynomial, and 0xf5b4253f, a maximally
sized irreducible polynomial. Because our polynomial breaks down into
two smaller polynomials, our crc32c space turns out to not be a field,
but rather a ring containing two smaller sub-fields. Because these
sub-fields are defined by their polynomials, one is the 31-bit crc
defined by the polynomial 0xf5b4253f, while the other is the current
parity.
We can move in the parity sub-field without changing our position in the
31-bit crc sub-field by xoring with a number that is one in the parity
sub-field, but zero in the 31-bit crc sub-field.
This number happens to be 0xf5b4253f (0xfca42daf bit-reversed)!
(crcs being bit-reversed will never not be annoying)
So long story short, xoring any crc32c with 0xfca42daf will change its
parity but not its value.
---
An that's basically our new perturb scheme. If we need to perturb, xor
with 0xfca42daf to change the parity, and after calculating/validating
the checksum, xor with 0xfca42daf to get our canonical checksum.
Isn't that neat!
There was one small hiccup: At first I assumed you could continue
including the valid bits in the checksum, which would have been nice for
bulk checksumming. But this doesn't work because while valid bits cancel
out so the parity doesn't change, changing valid bits _does_ change the
underlying 31-bit crc, poisoning our checksum and making everything a
mess.
So we still need to mask out valid bits, which is a bit annoying.
But then I stumbled on the funny realization that by masking our valid
bits, we accidentally end up with a fully functional parity scheme.
Because valid bits _don't_ include the previous valid bit, we can figure
out the parity for not only the entire commit, but also each individual
tag:
80 03 00 08 6c 69 74 74 6c 65 66 73 80
^'----------------.---------------' ^
| | |
v + parity = v'
Or more simply:
80 03 00 08 6c 69 74 74 6c 65 66 73 80
'----------------.----------------' ^
| |
parity = v'
Double neat!
Some other notes:
- By keeping the commit checksum perturbed, but not the canonical
checksum, the perturb state is self-validating. We no longer need to
explicitly check the previous-perturb-bit (q) to avoid the perturb
hole we ran into previously.
I'm still keeping the previous-perturb-bit (q) around, since it's
useful for debugging. We still need to know the perturb state
internally at all times in order to xor out the canonical checksum
correctly anyways.
- Thanks to all of our perturb iterations, we now know how to remove the
valid bits from the checksum easily:
cksum ^= 0x00000080 & (tag >> 8)
This makes the whole omitting-valid-bits thing less of a pain point.
- It wasn't actually worth it to perturb the checksum when building
commits, vs manually flipping each valid bit, as this would have made
our internal appendattr API really weird.
At least the perturbed checksum made fetch a bit simpler.
Not sure exactly how to draw this with our perturb scheme diagrams,
maybe something like this?
.---+---+---+---. \ \ \ \
|v| tag | | | | |
+---+---+---+---+ | | | |
| commit | | | | |
| | +-. | | |
+---+---+---+---+ / | | | |
|v|qp-------------->p>p-->p .
+---+---+---+---+ | . . .
| cksum | | . . .
+---+---+---+---+ | . . .
| padding | | . . .
| | | . . .
+---+---+---+---+ | | | |
|v------------------' | | |
+---+---+---+---+ | | |
| commit | +-. | +- rbyd
| | | | | | cksum
+---+---+---+---+ / | +-. /
|v----------------------' | |
+-------+---+---+ / |
| cksum ----------------'
+---+---+---+---+
| padding |
| |
+---+---+---+---+
| erased |
| |
. .
. .
---
Code changes were minimal, saving a tiny bit of code:
code stack
before: 36368 2664
after: 36352 (-0.0%) 2672 (+0.3%)
There was a stack bump in lfsr_bd_readtag, but as far as I can tell it's
just compiler noise? I poked around a bit but couldn't figure out why it
changed...
I think this describes the goal of the non-perturbed rbyd checksums
decently. At the very least it's less wrong that "data checksum", and
calling it the "metadata checksum" would just be confusing. (Would our
commit checksum be the "metametadata checksum" then?)
The naive implementation is simpler, less code, and more likely to be
correct, each of these are more valuable than speed in our debug
scripts.
We're in Python anyways (no offense Python!).
Plus I think it's good to show that the underlying logic of CRCs aren't
really that complex, at least until we throw optimizations into the mix.
This just forwards the internal LFS_I_DIRTY flag to the user via the
lfsr_tinfo flags field.
Benefits of this approach:
- Gives the user more flexibility on what to do if the filesystem is
modified, maybe you want to keep traversing depending on some other
logic.
- Can eventually add other flags to tinfo.flags, such as
LFS_I_COMPACTED, LFS_I_REPAIRED, LFS_I_INCONSISTENT, etc.
- Avoids confusion around the very different behaviors of LFS_O_EXCL and
LFS_T_EXCL.
I tried to come up with a better name (maybe LFS_T_WATCH?) but it was
a bit of a struggle... Switching to a flags approach sidesteps the
issue.
- Can drop the LFS_ERR_BUSY error code for now.
Code changes were fairly insignificant:
code stack
before: 35244 2680
after: 35224 (-0.1%) 2680 (+0.0%)
The only concern is that the tests highlighted it's possible for our
flag scheme to miss mutation if it happens after/during the last set of
blocks... Not sure how to handle this yet...
The internal btree node traversal here is a bit different than in
dbgbtree.py, dbgmtree.py, etc, because we want to include both inner and
leaf nodes. We could decode the branch tags multiple times, but checking
for bid changes is a bit simpler.
It's interesting to note this went missed for so long, because, well,
it's hard to actually have more than one btree node. And the tests
explicitly covering large btree structures, test_btree, aren't parsable
by dbgbmap.py since they don't create real filesystem images.
This sort of turned into a complete refactor of lfs_alloc in order to
move/reuse the lookahead buffer filling logic into lfsr_fs_traverse.
lfs_alloc now calls lfsr_fs_traverse to fill the lookahead buffer when
no more blocks are available, but also you can too with lfsr_traversal_t
+ LFS_T_LOOKAHEAD.
The one big caveat being if any mutation happens to the filesystem, any
incomplete lookahead needs to be tossed out. To help with this,
lfsr_traversal_read now returns LFS_ERR_BUSY (-16) instead of
LFS_ERR_NOENT (-2) if the filesystem has been modified since the
traversal was opened.
Note that by default lfsr_traversal_t will still try to keep traversing
blocks, but can be told to terminate immediately with LFS_T_EXCL.
Continuing the traversal is probably desired for checking checksums,
debugging, etc, as otherwise you could end up looping over only the
first couple blocks in a write-heavy system, but if you are trying to
populate the lookahead buffer you probably want to just abort and start
over.
I considered adding a flags field to lfs_tinfo for this, but decided
against it since it would be the only place in the current API where we
don't use error codes to convey behavior-changing information. Though
this may be worth reconsidering at some point...
---
In reworking lfs_alloc, a lot of the internal logic was broken up into
specific functions:
- lfs_alloc_ckpoint - checkpoint the allocator
- lfs_alloc_discard - discard any lookahead
- lfs_alloc_shift - discard/shift lookahead if progress can be made
- lfs_alloc_markinuse - mark a block as in-use
- lfs_alloc_markfree - mark any remaining blocks as free
- lfs_alloc_findnext - find the next free block in lookahead
If anything this probably makes lfs_alloc more readable, though the
original motivation was to allow lfsr_traversal_t to only shift/zero the
lookahead buffer if there's a chance we can make progress.
This was based on upstream work by opilat and myself.
Code changes:
code stack
before: 34226 2560
after: 34474 (+0.7%) 2552 (-0.3%)
Sort of like SHT_NOTE in elf files, but with no defined format. Using
LFSR_TAG_NOTE for additional noise/nonces is still encouraged, but it
can also be used to add debug info.
We redraw the output when we recieve a new line from the child process.
Which means if we never recieve a new line, nothing gets drawn.
Fixed by adding an extra case after the child process finishes.
Christopher Haster