I find these little diagrams useful for visualizing the actual on-disk
encoding, which doesn't really exist in the code outside of the
lfsr_data_from* and lfsr_data_read* functions.
This should really be unsigned, rbyd weights can not be negative.
Note this is different than data.size, etc, since the signedness there
is used to differentiate the underlying encoding. Accessing data.size
directly is usually an error, though we do access it directly in several
places when assuming the underlying encoding. Signedness warnings are
actually a good thing in that case.
So unsigned instead of signed. The original intention of using int32_t
was to hint that the sign bit should be reserved, but this may just
confuse if our leb128 encoding has a sign representation, which it does
not.
Yes these offered a tiny bit of typing savings, but they are used so
infrequently (really just lfsr_mdir_commit and friends) that they aren't
really worth it.
They also sort of break the object-related function pattern, since they
operated gdeltas (uint8_t[]) instead of grms (lfsr_grm_t). It's easy
enough to pass LFSR_GRM_DSIZE where needed.
This would probably only get worse if we add more gstate types.
This had no impact on code/stack. These functions were probably already
inlined.
The original motiviation was to make the gstate-related logic a bit more
coherent, but it turns out lfsr_grm_xorgrm is quite useful for
simplifying gstate handling in lfsr_mdir_commit.
As a plus it looks like we save a surprisingly amount of stack cost, but
I think this may just be a symptom of our tooling not being able to
understand shrinkwrapped function calls:
code stack
before: 33716 2832
after 33692 (-0.1%) 2808 (-0.9%)
This code is a bit tricky since we need to reference the current mdir to
know how to update other opened mdirs, but then also update the current
mdir, which could also be in the list of opened mdirs. I think a hear a
functional language user laughing in the distance...
code stack
before: 33764 2832
after: 33716 (-0.1%) 2832 (+0.0%)
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%)
We just don't use this error since we assert. Having it in the error
enum may give the wrong impression we return it at points.
If we even end up needing it, it can be readded to the list.
Hey if it's good enough for iterators (i), it's good enough for our
other traversals/iterators:
- iterator(?) -> i
- traversal -> t
- opened -> o
Expressions involving these variable were getting quite long. At least
now our common opened-list iterator can take only one line.
This reduces lfs.c by 41 lines (16851 -> 16810).
I do wonder if the use of "o" as a variable will limit my future
employment opportunities though.
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.
Some relatively-annoying states to check for:
- lfsr_attr_isnoop
- lfsr_attr_isinsert
And some accessors for marshalled pointers used by internal tags:
- lfsr_attr_grm
- lfsr_attr_mdir
- lfsr_attr_shrubcommit
- lfsr_attr_shrubtrunk
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.
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
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%)
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.
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%)
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...
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.
This is an interesting optimization made possible by our attr-lists now
only operating on one mid. We can now discard the entire attr-list based
on if that mid is in the commit's filter range.
Unfortunately, while I had hoped this would lead to more
simplifications, we can't really push this up through many functions:
- While this eager discard works for splits, lfsr_btree_commit can also
merge, which affects two separate bids. The attrs on these bids need
to be split over the new btree inner-nodes, so we end up still needing
filtering in lfsr_rbyd_appendattrs.
We could move the filtering up into lfsr_btree_commit, but would that
really gain anything?
- lfsr_mdir_commit_ needs the filter range because we leverage this in
higher-layers to for lfsr_mdir_commit_ to omit non--1 attrs when
committing to newly hollow mroots.
In theory it might still be possible to push this up into
lfsr_mdir_commit, but we would still need to unconditionally commit
during mdir splits to append the cksum. This ends up with duplicate
function calls which ends up annoyingly expensive. Though maybe there
is a conditional count trick that could avoid this?
At least the code changes are ok:
code stack
before: 33884 2896
after: 33808 (-0.2%) 2896 (+0.0%)
This unfortunately makes inserts inconsistent between rbyd/btrees:
insert(rid=-1) => rid=0
insert(bid=-1) => bid=0
insert(mid=0) => mid=0
But seems to integrate the best throughout the rest of the codebase:
- No awkward rid=-1 encoding in the mid, mid=1.2 => bid=1, rid=2
- No need to tweak mid encoding when writing grms to disk
- Behavior of unrelated files in the mdir behave consistently
irregardless of if our tag is an insert or not:
- mid' >= mid => mid'=mid'+delta
- mid' < mid => mid'=mid'
This is convenient because only the mid updates trigger tweaks of
unrelated mids, rbyds/btrees don't really have this problem.
- We already have to do a bit of tweaking in lfsr_mdir_namelookup, since
we're converting from "buckets" in the rbyd to ids we'd insert into.
Mainly namelookup of left-most name returns rid/bid=0, but for mdirs
should return mid=-1 (now mid=0):
left-most left-most+1 left-most+2
rid/bid: 0 0 1
mid (before): -1 0 1
mid (after): 0 1 2
I think this may be a reasonable compromise between allowing splits in
rbyd/btrees, and intuitive behavior for insertions in the mdirs.
That, or I've just been staring at this code for too long...
code stack
before: 33876 2896
after: 33888 (+0.0%) 2896 (+0.0%)
The recent change to internally track mids as mid=mid+1 leaked onto disk
through the grm. This is currently the only place we actually write mids
to disk.
The mid=mid+1 encoding is a bit of a hack and probably should not be the
actual on-disk representation, since there are other ways to encode this
internally.
I did try to write some tests for this, but because the bug is on both
the encoding and decoding side it's difficult without reading the mdir
directly. I only noticed with the dbg scripts started throwing random
errors. Fortunately a regression here is unlikely.
This atypical but not unreasonable behavior (most array insert functions
I've ran into like to insert _before_ the current index) makes split
commits no longer special behavior of appendattrs/commit, and seems to
fit better into rbyd append logic (though admittedly, some of the rbyd
append logic gets really weird with the whole right-leaning business).
Though this does come with a couple downsides:
- All rbyd-based data structures need to be able to represent a -1 id
so we can insert into the first id. This is not a problems for
rids/bids, but we need to tweak mids to support mid.rid=-1.
The best solution I could come up with was to just increment rid by
one, so, assuming mbits=8:
- mid=0x100 => bid=0x100, rid=-1
- mid=0x101 => bid=0x100, rid=0
- mid=-1 => bid=-1, rid=-1
- We need to be really careful with splits over our attr-list, since
these can line up between the rid create tags reference and other
following tags intended to stick to the new rid.
This required some special handling in lfsr_rbyd_appendattrs and
lfsr_mdir_commit__.
Other than that this change is quite promising, and removed what felt
like a bunch of hacks adjusting mids in lfsr_file_carve.
code stack
before: 33992 2904
after: 33868 (-0.4%) 2896 (-0.3%)
It turns out we never really need to commit to two unrelated rids in a
single commit. And some data structures, mainly btrees/bshrubs, don't
even allow commits to unrelated rids.
Well, sort of. There are some cases that seem to require unrelated rids,
but these are easy enough to work around:
1. btree/mdir splits/merges end up with two rids - but these either
converge or diverge from one rid, so as long as we assume sequential
inserts/deletes operate on the _neighboring_ rid, things work out.
2. grms/etc commit to mid=-1 irregardless of the file mid - but these
are also very special flags that are already handled differently to
manage the global state updates, nothing new was needed here.
So, in theory, we can move the rids out of the lfsr_attr_t struct and
infer and rid changes as we play out the attr-list, saving 4 bytes
(~17%) from every attr we allocate on the stack.
As a plus, we remove the need to manually calculate the changes to the
rid in the attr-list, reducing the likelihood of bugs here and saving a
decent amount of code.
Unfortunately the code/stack savings from this change were a bit
disappointing. The extra rid parameter in every commit function added
quite a bit of overhead, and we have to do some funky memmoves in
lfsr_file_carve to account for the new strict attr-list order:
code stack lfsr_attr_t
before: 33924 2912 24
after: 33992 (+0.2%) 2904 (-0.3%) 20 (-16.7%)
Still, this decreases the amount of code that can contain bugs, and more
closely matches the actual behavior of lfsr_btree/bshrub_commit.
Someone should really get around to updating the rbyd/btree/mtree
tests... Well, at least the non-internal (dir/dread/file/fwrite/etc)
tests are working.
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.
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.
We still have an lfsr_shrub_t, it's just a simple alias of lfsr_rbyd_t.
The only difference between these two structs was that lfsr_rbyd_t had
the eoff/cksum fields, to enable incremental commits, and lfsr_shrub_t
had the estimate field, to keep track of the current shrub estimate so
we evict before overflow.
Unfortunately C makes this overlap a bit annoying. We can either add a
union, making a mess of field accesses, or use probably problematic
casting of structs with common initial sequences.
Instead of dealing with this headache, I'm just going to shove the
shrub estimate into the rbyd's eoff field and ignore the name abuse.
In normal rbyd use, eoff does effectively contain the on-disk size of
the rbyd, so it's not too far from its intended use...
This does move our estimate to overlap the eoff field instead of the
cksum field, which means we need to be a bit more careful about setting
erased state for btrees. This adds a small code cost:
code stack
before: 33928 2912
after: 33956 (+0.1%) 2912 (+0.0%)
Shrubness should have always been a property of lfsr_rbyd_t.
You know you've made a good design decision when things just sort of
fall into place and the code somehow becomes cleaner.
The downside of this change is accessing rbyd trunks requires a mask,
which is annoying, but the upside is we don't need to signal shrubness
via extra booleans in internal functions anymore.
The funny thing is, the actual motivation for this change is was just to
free up a bit in our tag encoding. Simplifying some of the internal
functions was just a nice side effect.
code stack
before: 33940 2928
after: 33928 (-0.0%) 2912 (-0.5%)
So:
switch (cond) {
case 0:;
// first case
break;
case 1:;
// second case
break;
default:;
// default case
break;
}
This basically adopts our current label style for the case statements in
switch statements. It initially looks like quite a monstrosity, but I
think it does a good job at highlighting that case statements in C are
no safer than labels and gotos.
I would not use this style in a language with better scoping in switch
statements.
I'd prefer not to use switch statements, their scoping rules in C are
just too error-prone, and the compiler usually optimizes things out
anyways, but there are some places where switch statements are clearly
the correct organization -- state machines such as lfsr_traversal_read
for example.
If you're curious about the ':;' ending, this is used in our current
style for labels to avoid "declaration is not a statement" warnings.
Which I think is just a bit of leftover from C historically not having
mixed statements/declarations.
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.
If it's convenient, _and_ saves a bit of code, I don't really see the
reasoning to drop it.
At least added an LFSR_ATTR_IF macro to try to reduce the C expression
noise.
Also I'm keeping the explicit noop grows in lfsr_btree_commit. I think
it's a good decision to prevent accidental non-nooping in the future. If
these grows didn't noop, it wouldn't be an error, but a minor parasitic
drain on performance/wear. Not great...
code stack
before: 34052 2928
after: 33940 (-0.3%) 2928 (+0.0%)
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%)
Unfortunately we can't apply the valuable overflow assertions we use
elsewhere, classic C array arguments devolving into pointers before
sizeof. It's unfortunate there's no way to get this information even
for static-sized array arguments.
code stack
before: 33976 2944
after: 33940 (-0.1%) 2928 (-0.5%)
This is just too useful a namespace to not have in the low-level file
code.
This also replaces the ftree namespace with bshrub, which is a bit of a
more concrete term?
Note that some of the bshrub functions still take lfsr_file_t instead
of lfsr_bshrub_t. In _theory_ these could take 3 pointers (mdir+bshrub+
bshrub_), but this adds a surprising amount of code cost and we really
don't gain anything.
- Rely on rbyd lookup ENOENT now that that is cheap
- Adopt internal traverse_ function with shrub bool to indicate if root
should be included, similar to btree_commit_
Code changes:
code stack
before: 33956 2944
after: 33976 (+0.1%) 2944 (+0.0%)
We really shouldn't go to disk in cases like these, it's not worth the
code tradeoff. This led to concious decisions to avoid out-of-bound
lookups in higher-layers, which sort of defeats any benefit of this
potential optimization.
code stack
before: 33948 2944
after: 33956 (+0.0%) 2944 (+0.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.
Since we need these for lfsr_dir_t (named b and p), we might as well
adopt one in lfsr_file_t (named m). This at least avoids a cast when
enrolling/unenrolling in the opened-mdir list.
This is just too enticing a simplification to avoid, even at a RAM cost.
By tracking the dir's bookmark as a separate mdir, we can trivially
deduplicate the logic to update dirs' mdirs. But this does significantly
increase the size of our lfsr_dir_t with the bookmark's type/flags/rbyd/
etc, which usually goes unused.
I guess this does optimize lfsr_dir_rewind... Is that ever a bottleneck?
Code changes:
code stack lfsr_dir_t
before: 34048 2944 48
after: 33956 (-0.3%) 2944 (+0.0%) 80 (+66.7%)
Long story short, lfsr_mkdir needs to atomically create two somewhat
arbitrary mid entries: the bookmark and the dir entry. We can't, so we
fake it with our grm:
1. Create bookmark mid, set grm to delete bookmark mid
2. Create dir mid, zeroing grm
This works well enough, with the _small_ (read not small) caveat that
creating the bookmark mid can indirectly change the dir mid in
surprising ways.
The original plan was to take advantage of our opened-mdir tracking to
track an on-stack mdir pointing to where the dir mid should go. This
gets tricky, because, by definition of not being created yet, the dir
mid doesn't actually have an mid we can track. But this shouldn't be any
issue if we track the mid _after_ where we should insert right?
Wrong.
The issue is mdir splits. Which are especially nefarious because the
vast majority of the time mdirs don't split. And the split has to happen
_exactly_ between the tracked mid and where we would have inserted. Rare,
but possible. Which is why this went undetected for so long.
Consider this example of creating dir c:
.---------------.
|bmk|reg|reg|reg|
| a | b | d | e |
'---------------'
^
'-- track reg d (to insert dir c)
First insert the did, but oh no! a split occured!
.-------.
|mdr|mdr|
| | d |
'-|---|-'
.---' '---.
v v
.-------. .-------.
|bmk|reg| |reg|reg|
| a | b | | d | e |
'-------' '-------'
^
'-- track reg d (to insert dir c)
Now insert the dir c before reg d:
.-------.
|mdr|mdr|
| | d |<--------------------.
'-|---|-' |
.---' '-----. ???????????????????
v v dir c unreachable?
.-------. .-----------. ???????????????????
|bmk|reg| |dir|reg|reg| |
| a | b | | c | d | e | |
'-------' '-----------' |
^ |
'---------------------'
Problem, dir c became unreasonable.
Some options:
1. I _think_ things work if you track the mid _before_ where we want to
insert, iff we track the dir mid.
1. Our parent directory always at least contains a bookmark mid, so
it's not possible for our bookmark to be inserted between the mid
_before_ our dir mid and our dir mid.
This is possible if we track the bookmark mid, and would break
things.
2. mdir splits use the first name in the right mdir, so even if we
split on the mid immediately after the mid _before_ our dir mid,
inserting the dir mid should not make it unreachable.
But, as experience has shown, this whole tracking thing is very
fragile.
2. Create an orphan file and track that, replacing it atomically after
we've created our bookmark.
This works, but trades read overhead for write overhead. Even though
it's O(log^2 n) reads vs O(1) writes, write overhead always takes
priority and should be minimized. Flash is destructive after all.
3. Rip our the on-stack mdir tracking and just do an additional name
lookup after the bookmark is created.
mdir tracking, while clever, has created enough difficult to find bugs
and enough headache that I think its time is up. This commit implements
option 3.
Besides, it's not like lfsr_mkdir is really a performance sensitive
function anyways...
Code changes:
code stack
before: 34036 2944
after: 34048 (+0.0%) 2944 (+0.0%)
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.
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%)
By moving this to such a low-level, this ensures consistent reporting of
grms without needed special cases in every high-level function.
The use of the orphan type is very convenient here, as it avoids a whole
category of potential bugs around mismatched mids<->mdir weights.
Also added lfsr_grm_isrm, though ironically it has less use after these
changes.
This required duplication of some rbyd/mdir lookup convenience
functions, so unfortunately it's not a win code-wise, but I think the
resilience to future bugs is worth it.
Heck, it already revealed one minor bug in the return value of
lfsr_mtree_pathlookup when an intermediary path name is grmed/orphaned:
code stack
before: 33812 2944
after: 33944 (+0.4%) 2944 (+0.0%)
Christopher Haster