The trick is realizing the dirty/mutated bits are redundant when
mutating. So instead of saving the current dirty bit on the stack, we
can just swap the mutated/dirty bits temporarily.
The bit swapping xor trick comes from Sean Eron Anderson's infamous bit
twiddling hacks collection, unfortunately it doesn't seem possible to
avoid the hardcoded bit locations...
Saves a bit of code:
code stack
before: 35504 2680
after: 35488 (-0.0%) 2680 (+0.0%)
The hope here was that deduplicating the lfsr_fs_fixorphans/
lfsr_mtree_gc mtree traversal would result in code savings since we'd
end up with one shared code path.
Unfortunately in practice this didn't work out:
code stack
before: 35484 2680
after: 35504 (+0.1%) 2680 (+0.0%)
Still, it is nice to have one shared code path, because that means fewer
corner cases that could break.
lfsr_mdir_fixorphans always ends up setting the zombie flag, which is a
bit annoying. If we don't clear the zombie flag, lfsr_remove/lfsr_rename
may cause repeated mids during traversal, which probably won't break
anything, but isn't great...
Code changes:
code stack
before: 35480 2680
after: 35484 (+0.0%) 2680 (+0.0%)
After thinking about this for a bit, there are some compelling
motivations for including an incremental LFS_T_MKCONSISTENT:
- Being able to run incremental LFS_T_MKCONSISTENT traversals in
parallel with read-only operations is actually quite enticing.
The only complicated part is maintaining the invalidatable traversal
state, which already exists with lfsr_traversal_t (except the
annoying LFS_F_MUTATED bit).
- While it's not really effective to combine LFS_T_MKCONSISTENT and
LFS_T_LOOKAHEAD traversals, it _is_ possible to combine
LFS_T_MKCONSISTENT with LFS_T_COMPACT, LFS_T_CKMETA,
LFS_T_REPAIRMETA (future), etc.
Really, LFS_T_LOOKAHEAD is the odd one out.
- Making LFS_T_MKCONSISTENT incremental means all filesystem-level
traversals (except lfsr_mount) can be run incrementally. Which is a
nice feature to have when O(n = entire fs) risks being very long
running.
The main downside of LFS_T_MKCONSISTENT (and LFS_T_COMPACT, etc) is that
attempting to run it immediately after mount will likely recursively
trigger a lookahead scan to satisfy block allocation requests -- which
will block the current thread for the duration of the lookahead scan.
But this seems to be more a problem of LFS_T_LOOKAHEAD interacting with
other traversals poorly.
Fortunately, long term, the current plan is to replace the lookahead
buffer with an on-disk block map on disks where the lookahead scan is a
bottleneck. If this gets implemented the problem goes away.
So re-reverting this for now. Worst case we can always re-re-revert this
again in the future. There is already a working implementation, so might
as well see where it goes...
Supporting incremental LFS_T_MKCONSISTENT does add a bit of a code
cost, but there is still some room for deduplicating lfsr_mtree_gc +
lfsr_fs_mkconsistent, which may be interesting:
code stack
before: 35232 2680
after: 35480 (+0.7%) 2680 (+0.0%)
Checking for orphans + other traversal work turned out to mesh much
worse than originally thought:
- Adjusting mids and being able to drop mdirs mid-traversal complicates
traversal quite a bit and has potential to hide difficult to reproduce
bugs.
- Implementing incremental mkconsistent requires it's own separate state
to detect mutation correctly since LFS_T_MKCONSISTENT and
LFS_T_LOOKAHEAD are invalidated by slightly different things.
- If hasorphans=true, we're likely going to find orphans and clobber the
traversal. So it's not really worth trying to opportunistically prove
there are no orphans while doing other traversal operations.
- We don't really want to traverse the mroot/mtree during mkconsistent,
which makes deduplicating these two functions a bit tricky. Doable,
but annoying.
- grms don't involve traversals and are their own separate awkward step
already.
Combine this with the fact that needing to scan for orphans should be
relatively rare in practice -- requiring either a powerloss or a
complicated set of file operations with at minimum 3 desynced files --
and parallel orphan checking starts to look like more trouble than it's
worth...
Instead, we now only check if the hasorphan bit has been set, and if it
has been we just call lfsr_fs_mkconsistent directly. This does a full
traversal in a single step, but at least makes it so traversal +
LFS_T_MKCONSISTENT in a background thread will do any necessary
janitorial work.
This saves a bit code:
code stack
before: 35480 2680
after: 35232 (-0.7%) 2680 (+0.0%)
Turns out things get a bit tricky when mdirs are dropped while iterating
over the mtree.
This was actually broken quite a bit before traversal-related changes,
probably during some mtree refactor, but went unnoticed since no test
actually checked that lfsr_fs_fixorphans did what it said it did.
At least the new test_forphans_cleanup* tests should prevent this from
regressing again in the future.
Code changes:
code stack
before: 35472 2680
after: 35480 (+0.0%) 2680 (+0.0%)
What seemed like a simple tweak to lfsr_fs_fixorphans, integration into
lfsr_mtree_gc, turned out to be surprisingly annoying.
- We need an additional traversal flag, LFS_F_MUTATED, in order to know
if we intentionally modified the filesystem. This is different from
LFS_F_DIRTY in that we don't invalidate orphan scans:
- LFS_F_DIRTY => invalidate lookahead + orphans
- LFS_F_MUTATED => invalidate lookahead
- We need to break up lfsr_fs_fixorphans to expose lfsr_mdir_fixorphans,
which is probably a good thing for readability.
The interactions with each mdir being associated with a given mid is
not great though, and requires a bit of awkward mid shuffling.
- Unlike LFS_T_COMPACT, LFS_T_MKCONSISTENT introduces more complicated
mid changes, and makes it so mdirs can now be dropped in the middle of
traversal.
This messes with our internal lfsr_mtree_traverse -> lfsr_mtree_gc
control flow, and means a single lfsr_traversal_read call may process
an unbounded number of blocks in rare cases with lots of orphans.
But the good news is things are working, and lfsr_traversal_read with
LFS_T_MKCONSISTENT can scan for orphans in parallel with other traversal
operations.
Adds a bit of code:
code stack
before: 35220 2680
after: 35472 (+0.7%) 2680 (+0.0%)
The tests highlighted that the LFS_I_DIRTY flag in lfsr_tinfo approach
is insufficient. Consider what happens if our filesystem is mutated
while traversing the last mdir:
1. Traversal traverses last mdir, populate blocks, return first block
2. Filesystem mutated, maybe mdir was compacted, clobbers traversal and
sets LFS_I_DIRTY
3. Traversal return LFS_ERR_NOENT immediately, last block never
returned (and out of date), LFS_I_DIRTY never returned
Not only do we miss the LFS_I_DIRTY flag, but we completely miss the
last block in the mdir pair without any warning.
This is _not_ a problem for the actual lookahead buffer, since we still
internally check the LFS_I_DIRTY flag before marking it as complete, but
it is an issue for any external logic that depends on the traversal
being complete...
---
We could revert to LFS_T_EXCL, but, to be honest, I just really don't
know a good name for this flag...
LFS_T_EXCL is a bad name because it conflicts with LFS_O_EXCL. These
flags have very different behaviors, which risks confusing users, and
risks potential name conflicts down the line if we ever want
LFS_T_EXCL-esque semantics for open dirs/files (not unreasonable, though
quite fancy).
My current best contender is LFS_T_WATCH, but while scratching my head
on this, I starting to wonder why we're even providing LFS_T_EXCL in the
first place...
We err on the side of forcing users to implement filesystem-external
features themselves when possible elsewhere, and LFS_T_EXCL technically
_can_ be implemented entirely outside of the filesystem. Though to be
fair it is quite annoying/tedious.
It's not like there's any equivalent feature for dir/file reads anyways.
And a background thread calling lfsr_traversal_read with LFS_T_LOOKAHEAD
will still _eventually_ make progress, even if it takes a bit longer.
Don't get me wrong, I understand it is significantly easier to implement
this inside the filesystem than outside. But it's also easier to
implement this later than right now. And if we implement this later,
hopefully we'll have a better idea what exactly will be useful for
users.
---
Removing LFS_T_EXCL/LFS_I_DIRTY has no real impact on code cost. We were
really just exposing internal logic that we need for lookahead
correctness anyways:
code stack
before: 35224 2680
after: 35220 (-0.0%) 2680 (+0.0%)
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...
LFS_O_SYNC always implies LFS_O_FLUSH, otherwise what exactly are you
syncing? Making this explicit in the bit pattern should hopefully make
this clear for curious users, though lfsr_file_flush would be called
anyways because of how lfsr_file_sync is implemented.
This also moves the LFS_O_DESYNC bit pattern around so SYNC/FLUSH are
neighbors. SYNC/DESYNC may seem related, but in lfsr_file_open they
actually are quite different:
LFS_O_FLUSH 0x0040 ---- ---- -1-- ----
LFS_O_SYNC 0x00c0 ---- ---- 11-- ----
LFS_O_DESYNC 0x0100 ---- ---1 ---- ----
Code changes, mostly just noise from moving bits around:
code stack
before: 35228 2680
after: 35244 (+0.0%) 2680 (+0.0%)
It still doesn't make sense to check data without checking metadata, but
keeping this named LFS_T_CKDATA should hopefully clarify what it does
differently from LFS_T_CKMETA.
This implication is also now encoded in the bit pattern:
LFS_T_CKMETA 0x0100 ---- ---1 ---- ----
LFS_T_CKDATA 0x0300 ---- --11 ---- ----
In theory a clever user could force only the CKDATA bit to be set, and
such a configuration would _probably_ work fine, but it won't be
supported just to cut down on possible configurations to test.
No code changes:
code stack
before: 35228 2680
after: 35228 (+0.0%) 2680 (+0.0%)
It's probably a bad reason, but this avoids wasting too much time
figuring out how to name things.
Now most traversal functions return an lfsr_tag_t + lfsr_bptr_t pair,
which is enough to describe the current relevant traversal objects:
tag=LFSR_TAG_MDIR => (lfsr_mdir_t*)bptr.data.u.buffer
tag=LFSR_TAG_BRANCH => (lfsr_rbyd_t*)bptr.data.u.buffer
tag=LFSR_TAG_DATA => bptr.data
tag=LFSR_TAG_BPTR => bptr
This would be a bit better if lfsr_data_t's buffer field was a void*,
but that would mess with byte-level arithmetic, which is more common
with lfsr_data_ts.
This also adopts the fragmented/optional out-params used elsewhere in
the codebase. I thought this would add quite a bit more stack cost,
since we need redundant tags/bptrs to make lfsr_mtree_traverse/
lfsr_mtree_gc work, but surprisingly not:
code stack
before: 35256 2680
after: 35228 (-0.1%) 2680 (+0.0%)
It seems we make up the extra stack cost of redundant tags/bptrs by
giving the compiler more stack-alloc flexibility, tighter per-function
return types, and opting-out of tags/bptrs in most low-level traversals:
lfs_alloc mainly.
But if the fragmented/optional out-params is net harmful for code/stack
size, we should reconsider the pattern system-wide. This does probably
deserve a second look in the future...
This could go either way, it's a case of the classic C strchr type
conundrum.
But unlike iteration, we're more likely to mutate things when doing a
full traversal, so requiring everything to be mutable makes a bit more
sense.
Note that even readonly operations, fetchck for example, need access to
a mutable rbyd struct.
No code changes:
code stack
before: 35256 2680
after: 35256 (+0.0%) 2680 (+0.0%)
This solves the issue of multiple mdirs/rbyds in lfsr_mtree_gc, where
it's easy for traversal state to fall out of sync when mutating parts of
the filesystem.
Is it good design, with self-referential pointers making everything more
entangled? Not sure!
This saves a bit of stack, but adds a bit of code, which makes sense,
pointer chasing can be costly. But both of these changes are well below
the compiler noise floor:
code stack
before: 35228 2688
after: 35256 (+0.1%) 2680 (-0.3%)
Just in case any tags leak through. If an orphan tag ended up in an
lfsr_stat call, it could be quite confusing to users...
Current types:
// user facing
LFS_TYPE_REG 1 ---1
LFS_TYPE_DIR 2 --1-
LFS_TYPE_SYMLINK* 3 --11
// internal
LFS_TYPE_BOOKMARK 4 -1-- -.
LFS_TYPE_ORPHAN 5 -1-1 +- on-disk only
LFS_TYPE_COMPR* 6 -11- -'
LFS_TYPE_TRAVERSAL 9 1--1 <-- in-ram only
* Hypothetical
This has no impact on code size:
code stack
before: 35228 2688
after: 35228 (+0.0%) 2688 (+0.0%)
We probably want a common function to tell us if a given omdir is also
an obshrub.
Conveniently all types with bshrubs happen to share a common bit, and
this pattern will probably continue for a while:
// user facing
LFS_TYPE_REG 1 ---1 <-- bshrub
LFS_TYPE_DIR 2 --1-
LFS_TYPE_SYMLINK* 3 --11 <-- bshrub
// internal
LFS_TYPE_BOOKMARK 4 -1--
LFS_TYPE_TRAVERSAL 5 -1-1 <-- bshrub
* Hypothetical
Maybe this is overspecialized, but at least our tests will break quite
quickly if this ever turns out not to be true...
Code changes:
code stack
before: 35256 2688
after: 35228 (-0.1%) 2688 (+0.0%)
So now files and traversals contain several nested structs:
file <-- lfsr_file_t
file.o <-- lfsr_obshrub_t
file.o.o <-- lfsr_omdir_t
This gets a bit ugly, but it's really the only way to make the compiler
happy when also with C's annoying strict aliasing rules.
This also makes lfsr_traversal_t a simple alias of lfsr_mtraversal_t,
with lfsr_mtraversal_t now including all of the obshrub/omdir state.
This simplifies things internally, and allows lfsr_mtree_gc to assert on
opened-list enrollment, but risks increased stack cost for all of the
unused fields.
Fortunately this stack cost turned out to not be that significant:
code stack
before: 35264 2680 (+0.0%)
after: 35256 (-0.0%) 2688 (+0.3%)
Does what it says on the tin.
This simplifies lfsr_omdir_clobber, and can be used in more places. It's
a bit more flexible than implicitly mkdirtying in lfs_alloc_ckpoint, but
does add another function call.
But thanks to better code reuse this ends up saving a bit of code:
code stack
before: 35304 2688
after: 35264 (-0.1%) 2680 (-0.3%)
This brings back clobbering individual omdirs, so modifying an unsynced
file should leave all other traversals intact. This is the most precise
level of clobbering that I think is reasonable to implement.
This means we should be able to, say, check all currently-committed
checksums while writing to unsynced files at the same time. Which might
be useful? Maybe?
To make this work, lfsr_traversal_clobber now relies on the current
traversal state to know how to clobber correctly. This is more verbose,
but likely safer/more flexible.
Curiously, this actually ended up saving a bit of code, which is a bit
surprising:
code stack
before: 35356 2688
after: 35304 (-0.1%) 2688 (+0.0%)
Maybe manipulating the state machine directly gives the compiler more
info to work with? Not sure.
Since we're clobbering at the mid-level now, our mtraversals can only
ever point to unsynced reg file handles.
This means we can limit traversal clobbering to lfsr_file_close, and
move it out of the common/simple lfsr_omdir_close.
Look like any code changes canceled out perfectly:
code stack
before: 35356 2688
after: 35356 (-0.0%) 2688 (+0.0%)
This adds an indirect pointer to lfsr_btraversal_t, so references to the
btree/bshrub root point to the actual btree/bshrub root rbyd struct.
This means if our bshrub root is mutated due to, say, mdir compaction,
this doesn't necessarily invalidate our btraversal.
But note this is strictly limited to bshrub roots. If you modify any
other part of the bshrub/btree, expect the traversal to be broken.
This means we can do whatever we want with mdirs and not worry about
invaliding bshrub traversals, which is quite nice! It also fixes our
failing bshrub-traversal-mutation tests.
This adds a bit of stack cost, but because we are moving fewer rbyd
structs around in lfsr_btree_traverse_, actually ends up saving a bit of
code. Though we are well below the compiler noise floor:
code stack
before: 35368 2680
after: 35356 (-0.0%) 2688 (+0.3%)
Implementing gc_compact_thresh over bshrubs highlighted that it's really
not that difficult, and probably required, for traversal bshrubs to be
tracked correctly during mdir commits/compacts/splits/etc. And if we
track bshrubs across mdir commits, we might as well clobber traversals
at the mid level, allowing traversals to always reach btrees/bshrubs not
under active mutation.
One key thing to note: we should never be traversing a bshrub that is
not referenced elsewhere, either on-disk in an mdir or in-ram via an
opened file. So any compacted traversal bshrubs are not wasted prog
cycles.
This moves most of the clobbering logic back up into the high-level
functions (lfsr_remove/rename mainly), where we know which mids may be
clobbered.
This has a code cost, but it's really not all that much for more
thorough/correct filesystem traversals under mutation:
code stack
before: 35268 2680
after: 35368 (+0.3%) 2680 (+0.0%)
Unfortunately, lingering rbyd references in our btraversal structs are
still an issue, and some bshrub tests are failing... Though I do have
some ideas on how to fix this.
This does a few things:
- Deduplicates bshrub/btree rbyd lookups when rbyd is not explicitly
provided -- note this is by far the most common case.
- Moves the mid-level rbyd allocation out of the stack-hot-path.
Assuming lfsr_mtree_gc is never in the stack-hot-path, which seems
unlikely.
- Reduces pointer chasing in lfsr_btree_commit__, giving the compiler
more flexibility + assumptions and hopefully allowing it to optimize
better.
This may also be disentangling several cross-layer struct references,
which is probably a good thing.
The result is rather significant stack savings for what is a minor
refactor:
code stack
before: 35440 2800
after: 35268 (-0.5%) 2680 (-4.3%)
This doesn't quite get us back to pre-commit_-rbyd levels, but it's
pretty close:
code stack
before commit_-rbyd: 34652 2640
commit_-rbyd-explicit (before): 35440 (+2.3%) 2800 (+6.1%)
commit_-rbyd-implicit (after): 35268 (+1.8%) 2680 (+1.5%)
This gets a bit muddled now with traversals mutating inner btree nodes
directly.
Except for some asserts, we can accept any bid in the relevant rbyd
here, and accepting any bid is better than accepting only one bid
(left-leaning) inconsistent with the rest of the btree API
(right-leaning)...
Code changes minimal:
code stack
before: 35448 2800
after: 35440 (-0.0%) 2800 (+0.0%)
These aren't really different than btree nodes, except bshrubs need to
be enrolled in our opened list for commits to work.
Fortunately this is already true for explicit traversals, which are
currently the only traversals where we need to simultaneously mutate the
filesystem. This mainly just required adding additional checks for
LFS_TYPE_TRAVERSAL bshrubs, tests, and making sure traversal.bshrub is
never in an invalid state.
This continues to add code/stack cost for what is ultimately a
relatively niche feature:
code stack
before: 35268 2776
after: 35448 (+0.5%) 2800 (+0.9%)
Maybe btree/bshrub compactions should be disabled by default?
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.
Note, gc_compact_thresh over bshrubs is not yet implemented... That's
_another_ can of worms since we need to be able to commit to non-tracked
bshrubs somehow...
But at least this proves gc_compact_thresh over btrees is possible.
Now, if LFS_T_COMPACT is provided, any btree nodes > gc_compact_thresh
will be compacted during traversal/gc operations.
To make this work required a rather deep modification to the
lfsr_btree_commit/lfsr_bshrub_commit code paths to expose direct-rbyd
commit functions that can commit to arbitrary btree nodes:
- lfsr_btree_commit - bid, attrs, attr_count
- lfsr_bshrub_commit - bid, attrs, attr_count
- lfsr_btree_commit_ - bid, rbyd, rid, attrs, attr_count
- lfsr_bshrub_commit_ - bid, rbyb, rid, attrs, attr_count
- lfsr_btree_commit__ - bscratch, bid, rbyd, rid, attrs, attr_count
These are good to have, and will also be useful for implementing
metadata redundancy in the future.
Unfortunately, all of this comes at a significant code/stack cost:
code stack
before: 34652 2640
after: 35268 (+1.8%) 2776 (+5.2%)
lfs_fs_gc is still not reimplemented, but this is accessible through the
traversal API with LFS_T_COMPACT.
This is also the first traversal operation that can mutate the
filesystem, which brings its own set of problems:
- We need to set LFS_F_DIRTY in lfsr_mtree_gc now, which really
highlights how much of a mess having two flag fields is...
We do _not_ clobber in this case, since we assume lfsr_mtree_gc knows
what it's doing.
- We can now commit to an mroot in the mroot chain outside of the normal
mroot chain update logic.
This is a bit scary, but should just work.
The only issue so far is that we need to allow mdirs to follow the
mroot during mroot splits if mid=-1, even if they aren't lfs_t's mroot
mdir.
This should now be decently tested with the new
test_traversal_compact_* tests.
- It's easy for mtraversal's mdir and mtinfo's mdir to fall out of sync
when mutating... Why do we have two of these?
The actual compaction itself is pretty straightforward: just mark as
unerased, eoff=-1, and call lfsr_mdir_commit with an empty commit. This
is now wrapped up in lfsr_mdir_compact.
Code changes:
code stack
before: 34528 2640
after: 34652 (+0.4%) 2640 (+0.0%)
Though the real hard part will be implementing gc_compact_thresh over
btree nodes...
It really doesn't make sense to check data and not check metadata. We're
already traversing the metadata, so validating it adds very little
overhead, and how can we trust our data if we can't trust our metadata?
This renames LFS_T_CKDATA -> LFS_T_CK, which now also implies
LFS_T_CKMETA. This implication is done explicitly in lfsr_mtree_traverse
instead of doing anything fancy with flags.
Implying LFS_T_CKMETA also means one less configuration to support.
Code changes:
code stack
before: 34524 2640
after: 34528 (+0.0%) 2640 (+0.0%)
To hopefully hint that these are not pure-output pointers, unlike
underscore arguments elsewhere (though this isn't really an intentional
convention).
Cksums need to be initialized with zero, so that multiple prog/read
operations can chain cksum updates.
This has already tripped me up a couple times.
Separated out omdir/mdir and mtraversal. You still need to allocate an
mdir for mtraversal to work, but this avoids the extra cost of omdir's
linked-list.
To avoid _too_ many pointers, I duplicated the flags field into both
lfsr_traversal_t and lfsr_mtraversal_t. This is basically free since we
end up with a bunch of padding for mtraversal's state field, but comes
with the risk of getting confused when the two flag fields don't match
in the future.
I also merged the intermediary btype field into flags to avoid yet
another single-byte field, where it fits comfortably in 3-bits.
Note that the mdir can be uninitialized in cases where we don't need to
worry about traversal clobbering.
---
This has the same problems as separating out mdirs/bshrubs in bshrub
functions: more stack/code to move the multiple pointers around, but is
necessary to avoid strict aliasing issues. There's no way to represent
overlapping omdir/mdir/mtraversal struct in standard C99 otherwise.
The end result saves a bit of code, but adds a bit of stack:
code stack
before: 34576 2632
after: 34524 (-0.2%) 2640 (+0.3%)
Though these numbers may be close enough to the compiler noise floor to
not really care about...
This is how littlefs used to be organized, and I found it a bit easier
to navigate: low-level => go to front, high-level => go to end.
This also moves lfs_alloc immediately after the mtree logic, which is
sort of related, and before anything high-level.
lfs_init/lfs_deinit are also now immediately before lfs_mount/
lfs_unmount/lfs_format, which are closely intertwined.
This did actually affect our code cost, which is interesting. No logic
was changed, only moved:
code stack
before: 34566 2632
after: 34576 (+0.0%) 2632 (+0.0%)
There is a cyclic dependency between the bshrub and mdir logic, so
there's no obvious order, but grouping up bshrubs and btrees makes a lot
of sense since they share many low-level operations (lfsr_btree_commit_,
lfsr_btree_traverse_, etc).
Bshrubs really are just inlined btrees after all.
lfs.c is now roughly organized into three large sections:
1. Raw on-disk data structures (rbyds, btrees, bshrubs, etc)
2. The mess that is metadata (mtree, mdirs, etc)
3. High-level types/functions (files, dirs, mount, traversals, etc)
No code changes.
This gets a bit messy, since lfsr_bshrub_commit really requires the
bshrub to be enrolled in the opened mdir list to stage correctly.
To make this work, our internal SHRUBCOMMIT and SHRUBTRUNK attrs now
take a pointer to the active shrub, and assume it is followed by a
staging shrub in memory. This is a big hack/assumption that leaks
through lfsr_bshrub_commit, but it at gets the job done in our current
system.
Note some functions were renamed instead, these didn't really make sense
as pure-bshrub functions:
- lfsr_bshrub_readnext -> lfsr_file_readnext
- lfsr_bshrub_read -> lfsr_file_read_
---
The main reason for this is to comply with C99's strict aliasing rules,
which can be a real PIA sometimes.
We need to track a bshrub in lfsr_mtraversal_t, but we really don't want
to pay the RAM cost for an entire lfsr_file_t. The best option I've
found is to pass around multiple pointers to the relevant internal
structs (mdir+bshrub), but this adds a stack+code cost.
So far, strict aliasing is a net downside:
code stack
before: 34478 2624
-fno-strict-aliasing: 34502 (+0.1%) 2616 (-0.3%)
after: 34566 (+0.3%) 2632 (+0.3%)
But it's baked into the standard and we can't always rely on
-fno-strict-aliasing being available.
Been leaning towards this naming scheme. Now lfsr_omdir_* functions
match the lfsr_omdir_t type they operate on.
- Renamed lfs.opened -> lfs.omdirs
- Renamed lfsr_opened_isopen -> lfsr_omdir_isopen
- Renamed lfsr_opened_add -> lfsr_omdir_open
- Renamed lfsr_opened_remove -> lfsr_omdir_close
- Renamed lfsr_mid_isopen -> lfsr_omdir_ismidopen
Similar to the lfsr_mtree_* functions, the theory is making the grm
implicit saves some code needing to carry that extra bit of context
around.
Most of these functions don't really make sense without filesystem
context anyways.
In practice, most of these functions are inlined, so any savings the
compiler would have probably already figured out:
code stack
before: 34478 2624
after: 34478 (+0.0%) 2624 (+0.0%)
So instead of looking for another bookmark, we check to see if the next
mid's did matches.
This is in theory more robust, and can handle neighboring
directories/files without bookmarks, but adds a bit of code cost.
Deduplicating these checks into lfsr_grm_pushdid helps reduce this a
bit:
code stack
before: 34406 2624
after-no-dedup: 34526 (+0.3%) 2624 (+0.0%)
after-dedup: 34470 (+0.2%) 2624 (+0.0%)
lfsr_mtree_seek is a bit of an odd function, a hammer for too many
nails.
Using lfsr_mtree_lookup directly with manual mdir.mid manipulation gives
the internal layers more flexibility and room for optimizations.
Code changes:
code stack
before: 34426 2624
after: 34406 (-0.1%) 2624 (+0.0%)
This makes mtree implicit in most of littlefs's core functions, which
simplifies things. It also makes lfsr_mtree_traverse naming consistent
with other mtree-esque operation.
Renames:
- Renamed lfsr_fs_weight -> lfsr_mtree_weight (implicit mtree)
- Renamed lfsr_mtree_weight -> lfsr_mtree_weight_ (explicit mtree)
- Renamed lfsr_fs_traverse* -> lfsr_mtree_traverse*
- Renamed LFSR_TSTATE_* -> LFSR_MTRAVERSAL_*
Implicit mtree functions, note these are pretty much the backbone of
littlefs:
- lfsr_mtree_weight
- lfsr_mtree_lookup
- lfsr_mtree_seek
- lfsr_mtree_namelookup
- lfsr_mtree_pathlookup
- lfsr_mtree_traverse
This makes the naming is a bit inconsistent with lfsr_btree_*,
lfsr_rbyd_*, etc, but sometimes rules needs to bend a bit.
Besides, most of these functions needed access to the mroot anyways, so
it's not like they were really ever able to operate on independent
mtrees correctly.
And you can't complain about the code savings:
code stack
before: 34562 2624
after: 34426 (-0.4%) 2624 (+0.0%)
This, in theory, deduplicates this bounds check, and matches
lfsr_btree_lookup/lfsr_rbyd_lookup in behavior.
There's was only one non-asserting bounds check that could actually be
"deduplicated" (ignoring lfsr_mtree_seek for now), so this just resulted
in compiler noise:
code stack
before: 34558 2624
after: 34562 (+0.0%) 2624 (+0.0%)
So now lfsr_traversal_read will only return LFS_ERR_BUSY if LFS_T_EXCL
was provided to lfsr_traversal_open.
This means it's no longer possible to opportunistically traverse blocks,
_and_ detect mutation in the same traversal (though I suppose you could
open multiple traversals for this?), but on the flipside this
potentially frees up the implementation a bit.
This motivation for this is that LFS_ERR_BUSY is potentially confusing
and annoying to handle if you don't care about mutation.
code stack
before: 34566 2624
after: 34558 (-0.0%) 2624 (+0.0%)
blocks={0,0} technically worked, but only because the only mdirs allowed
at block 0 are mroot blocks. In theory, an mdir at block 0 could match
blocks={0,0} and clobber incorrectly, but it's not possible for such an
mdir to be in a non-trivial mtree, since blocks={0,1} are reserved for
the mrootanchor.
But bleh, that's complicated.
Setting blocks={-1,-1} makes the mdir truely invalid/unmatchable and
provides a stronger invariant at a minor code cost.
Code changes:
code stack
before: 34554 2624
after: 34566 (+0.0%) 2624 (+0.0%)
Christopher Haster