Now that lfs3_alloc_ckpoint is more complicated, and can error, it makes
sense for lfs3_alloc_ckpoint to be implied by lfs3_mdir_commit.
Most lfs3_mdir_commit calls represent an atomic transaction from one
state -> another, so this saves a bit of code:
code stack ctx
before: 36912 2368 684
after: 36836 (-0.2%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38512 2400 812
bmap after: 38436 (-0.2%) 2400 (+0.0%) 812 (+0.0%)
The notable exception being bshrub-related commits in
lfs3_bshrub_commitroot_. Bshrub commits are trying to resolve an
in-flight btree, so the relevant blocks are very much _not_ at rest.
---
I've been hesitant to adopt this mostly just because it makes the
lfs3_mdir_commit* names even more of a mess:
- lfs3_mdir_commit__ -> lfs3_mdir_commit___
- lfs3_mdir_commit_ -> lfs3_mdir_commit__
- lfs3_mdir_commit -> lfs3_mdir_commit_
- added lfs3_mdir_commit
- lfs3_mdir_compact -> lfs3_mdir_compact_
- add lfs3_mdir_compact
- lfs3_mdir_alloc__ -> lfs3_mdir_alloc___
- lfs3_mdir_estimate__ -> lfs3_mdir_estimate___
- lfs3_mdir_swap__ -> lfs3_mdir_swap___
The main change is error propagation in lfs3_alloc_ckpoint. Since
lfs3_alloc_ckpoint writes to disk during bmap rebuilds, it can now fail
in all sorts of ways. Fortunately lfs3_alloc_ckpoint should only ever be
called by write operations, where these errors are be expected.
With bmap rebuild errors now reported correctly, this unblocks most of
the remaining test TODOs:
- Passing test_badblocks
- Passing test_ck
- Passing test_trvs
With this, LFS3_YES_BMAP is now passing all but two tests, which are
still ifndef-disabled as a temporary measure:
- test_btree - We make some low-level assumptions about the lookahead
allocator when testing btrees. It's probably not worth trying to get
this passing with the bmap allocator.
- test_grow - This one does need fixing! We currently don't update
on-disk bmaps correctly when growing the filesystem.
Code changes minimal:
code stack ctx
before: 36912 2368 684
after: 36912 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38456 2400 812
bmap after: 38512 (+0.1%) 2400 (+0.0%) 812 (+0.0%)
We need to adjust mids to ignore orphans during dir traversal, but we
shouldn't also adjust the dir position. In theory it shouldn't matter if
we use adjusted/non-adjusted dir positions, but it becomes a problem if
intermediate writes cause those orphans to be cleaned up. Now all your
dir positions are wrong.
Not entirely sure why this only started to fail with the bmap. I'm
guessing it's just due to the additional gstate causing the mdirs to
split differently.
Code changes minimal:
code stack ctx
before: 36920 2368 684
after: 36912 (-0.0%) 2368 (+0.0%) 684 (+0.0%)
Tangential, but toss this on the pile of problems with dir positions.
I'm increasingly convinced we should just remove the concept if we can
get away with it.
There's really no reason to immediately commit the bmap to disk, at
least no until the first mdir commit, when we need to at least discard
the previous bmap state.
We already do all the gstate handling in lfs3_mdir_commit anyways, and
piggybacking on mdir commit lets us get rid of the annoying extra mdir
param in lfs3_alloc_ckpoint.
This does mean a slightly higher risk of needing to re-rebuild the bmap
after a powerloss, but in theory only if the user does something weird
like writing to a file and never calling sync. Most on-disk operations
terminate in an mdir commit as that's how any state change becomes
atomically visibile in littlefs.
Saves a nice bit of stack:
code stack ctx
before: 36920 2368 684
after: 36920 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38552 2472 812
bmap after: 38464 (-0.2%) 2400 (-2.9%) 812 (+0.0%)
At least at a proof-of-concept level, there's still a lot of cleanup
needed.
To make things work, lfs3_alloc_ckpoint now takes an mdir, which
provides the target for gbmap gstate updates.
When the bmap is close to empty (configurable via bmap_scan_thresh), we
opportunistically rebuild it during lfs3_alloc_ckpoints. The nice thing
about lfs3_alloc_ckpoint is we know the state of all in-flight blocks,
so rebuilding the bmap just requires traversing the filesystem + in-RAM
state.
We might still fall back to the lookahead buffer, but in theory a well
tuned bmap_scan_thresh can prevent this from becoming a bottleneck (at
the cost of more frequent bmap rebuilds).
---
This is also probably a good time to resume measuring code/ram costs,
though it's worth repeating the above note about the bmap work still
needing cleanup:
code stack ctx
before: 36840 2368 684
after: 36920 (+0.2%) 2368 (+0.0%) 684 (+0.0%)
Haha, no, the bmap isn't basically free, it's just an opt-in features.
With -DLFS3_YES_BMAP=1:
code stack ctx
no bmap: 36920 2368 684
yes bmap: 38552 (+4.4%) 2472 (+4.4%) 812 (+18.7%)
The gbmap's weight is defined by the block count stored in the geometry
config field, which should always be present in valid littlefs3 images.
But our scripts routinely try to parse _invalid_ littlefs3 images when
running in parallel with benchmarks/tests (littlefs3 does _not_ support
multiple read/writers), so this was causing exceptions to be thrown.
The fix is to just assume weight=0 when the geometry field is missing.
The image isn't valid, and the gbmap is optional anyways.
This doesn't fully replace the lookahead buffer, but at least augments
it with known bmap state when available.
To be honest, this is a minimal effort hack to try to get something
benchmarkable without dealing with all the catch-22 issues that a
self-support bmap allocator would encounter (allocating blocks for the
bmap requires a bmap, oh no).
Though now that I'm writing this, maybe this is a reasonable long-term
solution? Having the lookahead buffer to fall back on solves a lot of
problems, and, realistically, it's unlikely to be a performance
bottleneck unless the user has extreme write requests (>available
storage?).
---
Also tweaked field naming to be consistent between the bmap and
lookahead buffer.
This avoids issues with the different traversal paths with an mtree vs
inline-mtree. Previously this was broken when the mtree was inlined.
This order also makes more sense if we want to check mdirs before we
consider the gstate to be trustworthy enough for gbmap traversal.
The idea behind separate ctrled+unctrled airspaces was to try to avoid
multiple interpretations of the on-disk bmap, but I'm starting to think
this adds more complexity than it solves.
The main conflict is the meaning of "in-flight" blocks. When using the
"uncontrolled" bmap algorithm, in-flight blocks need to be
double-checked by traversing the filesystem. But in the "controlled"
bmap algorithm, blocks are only marked as "in-flight" while they are
truly in-flight (in-use in RAM, but not yet in use on disk).
Representing these both with the same "in-flight" state risks
incompatible algorithms misinterpreting the bmap across different
mounts.
In theory the separate airspaces solve this, but now all the algorithms
need to know how to convert the bmap from different modes, adding
complexity and code cost.
Well, in theory at least. I'm unsure separate airspaces actually solves
this due to subtleties between what "in-flight" means in the different
algorithms (note both in-use and free blocks are "in-flight" in the
unknown airspace!). It really depends on how the "controlled" algorithm
actually works, which isn't implemented/fully designed yet.
---
Long story short, due to a time crunch, I'm ripping this out for now and
just storing the current algorithm in the wcompat flags:
LFS3_WCOMPAT_GBMAP 0x00006000 Global block-map in use
LFS3_WCOMPAT_GBMAPNONE 0x00000000 Gbmap not in use
LFS3_WCOMPAT_GBMAPCACHE 0x00002000 Gbmap in cache mode
LFS3_WCOMPAT_GBMAPVFR 0x00004000 Gbmap in VFR mode
LFS3_WCOMPAT_GBMAPIFR 0x00006000 Gbmap in IFR mode
Note GBMAPVFR/IFR != BMAPSLOW/FAST! At least BMAPSLOW/FAST can share
bmap representations:
- GBMAPVFR => Uncontrolled airspace, i.e. in-flight blocks may or may
not be in use, need to traverse open files.
- GBMAPIFR => Controlled airspace, i.e. in-flight blocks are in use,
at least until powerloss, no traversal needed, but requires more bmap
writes.
- BMAPSLOW => Treediff by checking what blocks are in B but not in A,
and what blocks are in A but not in B, O(n^2), but minimizes bmap
updates.
Can be optimized with a bloom filter.
- BMAPFAST => Treediff by clearing all blocks in A, and then setting all
blocks in B, O(n), but also writes all blocks to the bmap twice even
on small changes.
Can be optimized with a sliding bitmap window (or a block hashtable,
though a bitmap converges to the same thing in both algorithms when
>=disk_size).
It will probably be worth unifying the bmap representation later (the
more algorithm-specific flags there are, the harder interop becomes for
users, but for now this opens a path to implementing/experimenting with
bmap algorithms without dealing with this headache.
The neat thing about the on-disk bmap is that it's a range tree. We can
leverage order-statistic properties to compactly represent ranges of
similar blocks.
However, this does make updating the bmap slightly more complicated...
This only works immediately after format, and only for one pass of the
disk, but it's a good way to test bmap lookups/allocation without
worrying about more complicated filesystem-wide interactions.
Except for niche file snapshotting, most btree updates until this point
are probably linear, i.e. a successful commit replaces any internal
rbyd state that has been claimed. In this model it makes sense to mark
claimed rbyd as "invalid", since failure to replace the claimed state
indicates something went wrong during the commit.
But this isn't necessarily true when snapshotting, since we don't
replace the state of claimed snapshots.
But wait, shouldn't snapshotted rbyds become readonly? Not necessarily!
Rbyds can have multiple trunks with unrelated (or in this case, shared)
histories, so there's nothing wrong with refetching an rbyd and
continuing to commit after another snapshot commits to tbe block.
Eventually both snapshots will need to compact and diverge into two
blocks, but until then sharing an rbyd makes the most of available
erased state.
At least in theory, experience will show us how well this works.
---
Also note this is not true for mdirs. We view mdirs as atomic and always
up-to-date, so snapshotting doesn't really make sense.
Fortunately the btree traversal logic is pretty reusable, so this just
required an additional tstate (LFS3_TSTATE_BMAP).
This raises an interesting question: _when_ do we traverse the bmap? We
need to wait until at least mtree traversal completes for gstate to be
reconstructed during lfs3_mount, but I think traversing before file
btrees makes sense.
This abandons the data-backed cache idea due to concerns around
readability and maintainability. Mixing const/mutable buffers in
lfs3_data_t was not great.
Instead, we now just allocate an indirect lfs3_data_t on the stack in
lfs3_file_sync_ to avoid the previous undefined behavior.
This actually results in less stack usage total, due to lfs3_file_t
allocations in lfs3_set/read, and avoid the more long-term memory cost
in lfs3_file_t:
code stack ctx
before: 36832 2376 684
after: 36840 (+0.0%) 2368 (-0.3%) 684 (+0.0%)
Oh. And lfs3_file_sync_ isn't even on the stack hot-path, so this is a
net benefit over the previous cache -> data cast:
code stack ctx
before sa: 36844 2368 684
after sa: 36840 (-0.0%) 2368 (+0.0%) 684 (+0.0%)
Still less cool though.
This fixes a strict aliasing violation in lfs3_file_sync_, where we cast
the file cache -> lfs3_data_t to avoid an extra stack allocation, by
modifying the file's cache struct to use an lfs3_data_t directly.
- file.cache.pos -> file.cache.pos
- file.cache.buffer -> file.cache.d.u.buffer_
- file.cache.size -> file.cache.d.size
- (const lfs3_data_t*)&file->cache -> &file->cache.d
Note the underscore_ in file.cache.d.u.buffer_. This did not fit
together as well as I had hoped, due to different const expectation
between the file cache and lfs3_data_t.
Up until this point lfs3_data_t has only been used to refer to const
data (ignoring side-band pointer casting in lfs3_mtree_traverse*), while
the file cache very much contains mutable data. To work around this I
added data.u.buffer_ as a mutable variant, which works, but risks an
accidental const violation in the future.
---
Unfortunately this does come with a minor RAM cost, since we no longer
hide file.cache.pos in lfs3_data_t's buffer padding:
code stack ctx
before: 36844 2368 684
after: 36832 (-0.0%) 2376 (+0.3%) 684 (+0.0%)
lfs3_file_t before: 164
lfs3_file_t after: 168 (+2.4%)
I think it's pretty fair to call C's strict aliasing rules a real wet
blanket. It would be interesting to create a -fno-strict-aliasing
variant of littlefs in the future, to see how much code/RAM could be
saved if we were given free reign to abuse the available memory.
Probably not enough to justify the extra work, but it would be an
interesting experiment.
I think modern C simply doesn't let us do what we want to do here, so
I'm giving up, discarding the lfs3_mtortoise_t type, and just abusing
various unrelated shrub fields to implement the tortoise. This
sacrifices readability, but at least avoids undefined behavior without
a RAM penalty:
- shrub.blocks => tortoise blocks
- shrub.weight => cycle distance
- shrub.eoff => power-of-two bound
Note this keeps trunk=0, which is a nice safety net in case some code
ever tries to read from the shrub in the future.
Fortunately the mtortoise logic is fairly self-contained in
lfs3_mtree_traverse_, so with enough comments hopefully the code is not
too confusing.
---
Apparently shaves off a couple more bytes of code. I'm guessing this is
just because of the slightly different struct offsets (we're reusing the
root's rbyd instead of the leaf's rbyd now):
code stack ctx
before: 36852 2368 684
after: 36844 (-0.0%) 2368 (+0.0%) 684 (+0.0%)
This forces our cycle detection tortoise (previously trv.u.mtortoise),
into the unused shrub leaf via pointer shenanigans.
This reclaims the remaining stack (and apparently code) we theoretically
gained from the btree traversal rework, up until the compiler got in the
way:
code stack ctx
before: 36876 2384 684
after: 36852 (-0.1%) 2368 (-0.7%) 684 (+0.0%)
And it only required some _questionably_ defined behavior.
---
It's probably not well-defined behavior, but trying to understand what
the standard actually means on this is giving me a headache. I think I
have to agree C99+strict-aliasing lost the plot on this one. Note
mtortoise is only ever written/read through the same type.
What I want:
lfs3_trv_t: lfs3_bshrub_t: lfs3_handle_t:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
| handle | | handle | | handle |
| | | | | |
+---+---+---+---+ +---+---+---+---+ .. '---+---+---+---'
| root rbyd | | root rbyd |
| | | | lfs3_mtortoise_t:
+---+---+---+---+ +---+---+---+---+ .. .---+---+---+---.
| leaf rbyd | | leaf rbyd | | mtortoise |
| | | | | |
+---+---+---+---+ +---+---+---+---+ .. '---+---+---+---'
| staging rbyd | | staging rbyd |
| | | |
+---+---+---+---+ .. '---+---+---+---'
| |
: :
But I'm starting to think this is simply not possible in modern C.
At least this shows what is theoretically possible if we didn't have to
fight the compiler.
Looks like these traversal states were missed in the omdir -> handle
rename. I think HANDLES and HBTREE states make sense:
- LFS3_TSTATE_OMDIRS -> LFS3_TSTATE_HANDLES
- LFS3_TSTATE_OBTREE -> LFS3_TSTATE_HBTREE
This matches other internal rbyds: btree.r, mdir.r, etc.
The intention of the single-char names is to reduce clutter around these
severely nested structs, both btrees and mdirs _are_ rbyds, so the name
doesn't really besides C-level type info.
I was hesitant on btree.leaf.rbyd, but decided consistency probably wins
here.
This comes from an observation that we never actually use the leaf cache
during traversals, and there is surprisingly little risk of a lookup
creating a conflict in the future.
Btree traversal fall into two categories:
1. Full traversals, where we traverse a full btree all at once. These
are unlikely to have lookup conflicts because everything is
usually self-contained in one chunk of logic.
2. Incremental traversals. These _are_ at risk, but in our current
design limited to lfs3_trv_t, which already creates a fully
bshrub/btree copy for tracking purposes.
This copy unintentionally, but conveniently, protects against lookup
conflicts.
So, why not reuse the btree leaf cache to hold the rbyd state during
traversals? In theory this makes lfs3_btree_traverse the same cost and
lfs3_btree_lookupnext, drops the need for lfs3_btrv_t, and simplifies
the internal API.
The only extra bit of state we need is the current target bid, which is
now expected as a caller-incremented argument similar to
lfs3_btree_lookupnext iteration.
There was a bit of futzing around with bid=-1 being necessary to
initialize traversal (to avoid conflicts with bid=-1 => 0 caused by
empty btrees). But the end result is a btree traversal that only needs
one extra word of state.
---
Unfortunately, in practice, the savings were not as great as expected:
code stack ctx
before: 36792 2400 684
after: 36876 (+0.2%) 2384 (-0.7%) 684 (+0.0%)
This does claw back some stack, but less than a full rbyd due to the
union with the mtortoise in lfs3_trv_t. The mtortoise now dominates. It
might be possible to union the mtortoise and the bshrub/btree state
better (both are not needed at the same time), but strict aliasing rules
in C make this tricky.
The new lfs3_btree_traverse is also a bit more complicated in terms of
code cost. In theory this would be offset by the simpler traversal setup
logic, but we only actually call lfs3_btree_traverse twice:
1. In lfs3_mtree_traverse
2. In lfs3_file_ck
Still, some stack savings + a simpler internal API makes this worthwhile
for now. lfs3_trv_t is also due for a revisit, and hopefully it's
possible to better union things with btree leaf caches somehow.
I was confused, but this commit->bid update is used to limit the
commit->bid to the btree weight. Note we limit the bid after storing it
as the initial rid.
What a mouthful.
The unconditional bshrub leaf discarding in lfs3_mdir_commit was copied
from the previous btree leaf caching implementation, but discarding
_all_ bshrub leaves on _every_ mdir commit is a bit insane.
Really, the only bshrub leaves that ever need to be discarded here are
the shrub roots, which are already questionable leaf caching targets
because they're already cached as the root rbyd.
An alternative option would be to just never cache shrub roots, but
tinkering around with the idea showed it would be more costly that
conditionally discarding leaves in lfs3_mdir_commit. At least here we
can reuse some of the logic that discards file leaves.
I'm also probably overthinking what is only a small code cost:
code stack ctx
before: 36784 2400 684
after: 36792 (+0.0%) 2400 (+0.0%) 684 (+0.0%)
This doesn't take into account how much CPU time is spent creating rbyd
copies, but that is not something we are optimizing for.
This is an indulgence to simplify the upcoming auxiliary btree work.
Brings back the previously-reverted per-btree leaf caches, where each
lfs3_btree_t keeps track of two rbyds: The root and the most recently
accessed leaf.
At the surface level, this optimizes repeated access to the same btree
leaf. A common pattern for a number of littlefs's operations that has
proven tricky to manually optimize:
- Btree iteration
- Pokes for our crystalization heuristic
- Checksum collision resolution for dids and (FUTURE) ddkeys
- Related rattrs attached to a single bid
But the real motivation is to drop lfs3_btree_*lookupleaf and simplify
the internal APIs. If repeated lfs3_btree_lookup*s are already
efficient, there's no reason for extra leaf-level APIs, and in theory
any logic that interacts with btrees will be simpler.
---
This comes at a cost (humorously about the same amount as the
tag-returning refactor, if you ignore the extra 28 bytes of ctx).
Unsurprisingly, increasing the size of lfs3_btree_t has the biggest
impact on stack and ctx:
code stack ctx
before: 36084 2336 656
after: 36784 (+1.9%) 2400 (+2.7%) 684 (+4.3%)
Also note from the previous commit messages: Btree leaf caching has
resulted in surprisingly little performance improvement for our current
benchmarks + implementation. It turns out if you're dominated by write
cost, optimizing btree lookups -- which already skip rbyd fetches, has
barely noticeable impact.
---
A note on reverting!
Eventually (after the auxiliary btree work) it will probably make sense
to revert this -- or at least provide a non-leaf-caching build for
code/RAM sensitive users.
I don't think this should be reverted as-is. Instead, I think we should
allow the option to just disable the leaf cache, while keeping the
simpler internal API. This would give us the best of all three worlds:
- A small code/RAM option
- Optimal btree iteration/nearby-lookup performance
- Simpler internal APIs
The only reason this isn't already implemented is because I want to
avoid fragmenting the codebase further while we're still in development
mode.
Note --list-suite-paths was already skipping case-less suites! I think
only -Y/--summary was an outlier.
This is consistent with test.py's matching of suite ids when no cases
are found (test_runner itself doesn't really care, it just reports no
matching cases). Though we do still compile case-less suites and include
them in the test_suites array, which may be confusing in the future.
The --no-internal flag avoids building any internal tests/benches
(tests/benches with in="lfs3.c"), which can be useful for quickly
testing high-level things while refactoring. Refactors tend to break all
the internal tests, and it can be a real pain to update everything.
Note that --no-internal can be injected into the build with TESTCFLAGS:
TESTCFLAGS=--no-internal make test-runner -j \
&& ./scripts/test.py -j -b
For a curious data point, here's the current number of
internal/non-internal tests:
suites cases perms
total: 24 808 633968/776298
internal: 22 (91.7%) 532 (65.8%) 220316/310247 (34.8%)
non-internal: 2 ( 8.3%) 276 (34.2%) 413652/466051 (65.2%)
It's interesting to note that while internal tests have more test cases,
the non-internal tests generate a larger number of test permutations.
This is probably because internal tests tend to target specific corner
cases/known failure points, and don't invite much variants.
---
While --no-internal may be useful for high-level testing during a
refactor, I'm not sure it's a good idea to rely on it for _debugging_ a
refactor.
The whole point of internal testing is to catch low-level bugs early,
with as little unnecessary state as possible. Skipping these to debug
integration tests is a bit counterproductive!
- enum lfs3_scmp -> enum lfs3_cmp
- cmp -> cmp
lfs3_scmp_t is still used as the type, as the s prefix indicates the
type is signed, usually for muxing with error codes.
I think that led to the enum also being named lfs3_scmp, but that's not
quite right.
But none of this really matters because enums are so useless and broken
in C.
- enum lfs3_error -> enum lfs3_err
- err -> err
Really this just updates `enum lfs3_err` to match the prefixes used
everywhere else. And because enum types are kind of useless in C, this
has no effect on any other part of the codebase.
Note this includes both the lfs3_config -> lfs3_cfg structs as well as
the LFS3_CONFIG -> LFS3_CFG include define:
- LFS3_CONFIG -> LFS3_CFG
- struct lfs3_config -> struct lfs3_cfg
- struct lfs3_file_config -> struct lfs3_file_cfg
- struct lfs3_*bd_config -> struct lfs3_*bd_cfg
- cfg -> cfg
We were already using cfg as the variable name everywhere. The fact that
these names were different was an inconsistency that should be fixed
since we're committing to an API break.
LFS3_CFG is already out-of-date from upstream, and there's plans for a
config rework, but I figured I'd go ahead and change it as well to lower
the chances it gets overlooked.
---
Note this does _not_ affect LFS3_TAG_CONFIG. Having the on-disk vs
driver-level config take slightly different names is not a bad thing.
Not sure how this got overlooked. Now that graft traversals are
implemented directly in lfs3_alloc, there's no reason to store this
state globally.
Fortunately this was in a union, so it didn't actually show up in our
ctx measurements.
No code changes.
- test_traversal -> test_trvs
- lfs3_traversal_t -> lfs3_trv_t
- lfs3_btraversal_t -> lfs3_btrv_t
- t -> trv
- bt -> btrv
- lfs3_traversal_* -> lfs3_trv_*
- lfs3_btraversal_* -> lfs3_btrv_*
The traversal type is becoming one of the more fundamental types in
littlefs, and if DIR and REG both get shortened names, it makes sense
for TRV to have one as well.
This also removes the temptation to use t for traversals, which is
probably an even worse name.
---
Note that lfs3_btree_traverse, lfs3_mtree_traverse, etc, remain
unaffected. This may change in the future, but it's interesting to note
that verbs seem to need much less typing than nouns.
- lfs3_omdir_t -> lfs3_handle_t
- lfs3.omdirs -> lfs3.handles
- o -> h
- lfs3_omdir_* -> lfs3_handle_*
- lfs3_omdir_ismidopen -> lfs3_mid_isopen
From conversations with users, the term "handle" or "file handle" seems
to be the most common/easily understood term for the lfs3_file_t struct
itself. It makes sense to adopt this in our codebase.
I usually dislike inventing new names for things when prefixes can imply
a relationship (size -> ssize, cache -> rcache, shrub -> bshrub, etc),
but lfs3_omdirs_t was probably a bit much.
This just wraps up block allocation + struct initialization similarly to
lfs3_rbyd_alloc.
Also tweaked bptr updates in lfs3_file_crystallize__ to mutate the bptr
fields directly instead of going through lfs3_bptr_init.
Neither of these impacted code cost, everything ends up inlined in
lfs3_file_crystallize__ anyways. Hopefully it helps with readability at
least.
Also LFS3_KVONLY mode is completely broken, and fixing it is not a huge
priority. I don't think it makes sense to adopt lfs3_bptr_alloc in
lfs3_file_flushset_ anyways, it would just lead to us initializing the
lfs3_bptr_t struct twice for no real reason.
No code changes.
Currently this just has one flag the replaces the previous `erase`
argument:
LFS3_ALLOC_ERASE 0x00000001 Please erase the block
Benefits include:
- Slightly better readability at lfs3_alloc call sites.
- Possibility of more allocator flags in the future:
- LFS3_ALLOC_EMERGENCY - Use reserved blocks
- Uh, that's all I can think of right now
No code changes.
- Moved block allocator definitions into their own dedicated block
before the lfs3_bptr_t stuff:
- lfs3_alloc_discard
- lfs3_alloc_ckpoint
- lfs3_alloc
I'm looking into adding lfs3_alloc related flags, and these aren't
really predeclarable like C's function prototypes.
Predeclaring these before lfs3_bptr_t is relevant if we ever add
lfs3_bptr_alloc.
Also touched up relevant comments a bit.
- Also added inline to lfs3_alloc_ckpoint and lfs3_alloc_discard, which
was strangely missing?
The compiler figured it out anyways, so this has no impact on code
cost.
- Moved ecksum definitions below lfs3_bptr_t stuff.
I don't really know where to put these, but close to the lfs3_rbyd_t
stuff makes sense.
- And updated lazy appendrattr_ ordering to match source code order.
No code changes.
The main motivation for the `bool exists` pattern was to avoid issues
with err clobbering. But now that lfs3_mtree_pathlookup returns a
muxed tag + err, this is less of a concern.
In littlefs, the err variable is frequently used as a short lived
temporary for propagating error codes. So frequent that I really
wouldn't trust its state after a couple of lines. Quickly converting
err -> bool exists reduced the risk that some necessary err state ends
up clobbered in a refactor.
This risk is still present for the tag variable, but it's already more
common for tags to hold persistent state (they hold file types after
all), so I think this risk is manageable.
And why get rid of a variable that's arguably more self-documenting?
When you're trying to keep a program's state in your head, less state is
better than more state.
No code changes.
Last but not least, this adopts tag-returns in lfs3_mtree_pathlookup,
and indirectly in all of lfs3_mtree_pathlookup's callers (which is
almost every top-level filesystem function -- anything that needs to
look up a path).
At this level, the muxed tag/err type really shows its versatility. Take
the LFS3_ERR_NOENT and LFS3_TAG_ORPHAN tags/errs for example.
Conceptually, these take very different code paths, but after calling
lfs3_mtree_pathlookup, it's easy to switch on both as though they
represent the same file-not-found condition.
We have to be a bit more careful now to not confuse err and tag
variables in these functions, and `goto failed` is now a bit of a
landmine, but the end result is another nice chunk of code savings:
code stack ctx
before: 36216 2336 656
after: 36084 (-0.4%) 2336 (+0.0%) 656 (+0.0%)
---
I believe this finishes the tag-returning refactor, which means we can
take a step back and look at how effective tag/err muxing is as a code
size optimization:
code stack ctx
before tag-returns: 36828 2368 656
after tag-returns: 36084 (-2.0%) 2336 (-1.4%) 656 (+0.0%)
A free 744 bytes is not bad! Especially considering there's no real
downside to this.
The 32 bytes of stack savings is nice too, and suggests we had ~8
unnecessary tag out-pointers sitting on the stack hot-path.
- lfs3_mdir_namelookup
- lfs3_mtree_namelookup
These are interesting, because, unlike lfs3_rbyd_namelookup, we don't
care about how query mids compare with the found mid.
Adopting tag-returns does mean we no longer return the relevant tag
when the query mid is missing, but the fact that the tests are passing
means this is a non-issue.
Shaves off a bit more code:
code stack ctx
before: 36260 2336 656
after: 36216 (-0.1%) 2336 (+0.0%) 656 (+0.0%)
Maybe these should have been updated in lock-step with
lfs3_mtree_pathlookup, but lfs3_mtree_pathlookup is going to impact a
lot more code...
- lfs3_mtree_traverse_
- lfs3_mtree_traverse
- lfs3_mtree_gc
I like this one if only for the reduced API noise. All of these layers
need to inspect the tag to know what to do, moving the tag to the return
position means less mucking around with points in our core traversal
logic.
Shaves off a bit more code:
code stack ctx
before: 36348 2336 656
after: 36260 (-0.2%) 2336 (+0.0%) 656 (+0.0%)
- lfs3_mdir_lookupnext
- lfs3_mdir_lookup
Like btree lookups, mdir lookups are also tag-inspection heavy, so we
see some nice savings:
code stack ctx
before: 36520 2352 656
after: 36348 (-0.5%) 2336 (-0.7%) 656 (+0.0%)
lfs3_mdir_lookup also highlights how tag-returns help reduce API noise
around the tag mask bits. lfs3_mdir_lookup's tag out-pointer doesn't
really make sense with the default non-masked tags, and moving it to the
return position hides it aways a bit.
- lfs3_btree_lookupleaf
- lfs3_btree_lookupnext
- lfs3_btree_lookup
- lfs3_btree_traverse
- NOT lfs3_btree_namelookup
Looks like we're starting to claw back stack usage a bit. This makes
sense as the btree logic involves the most layers -- with out-pointers
it needs more temporary copies to inspect tags along the way:
code stack ctx
before: 36576 2376 656
after: 36520 (-0.2%) 2352 (-1.0%) 656 (+0.0%)
This is where I would've adopted tag-returns in lfs3_rbyd_namelookup,
but it turns out this isn't possible. We are already muxing error codes
with compare flags (lfs3_scmp_t)!
In theory we could merge err + lfs3_cmp_t + lfs3_tag_t into one big
16-bit ordered tag mux abomination, but I decided that was probably
overkill for now.
As a plus this avoids an awkward temporary tag copy in
lfs3_rbyd_namelookup as we search for a better tag. Turns out the
out-pointers in lfs3_rbyd_namelookup are quite useful for staging
things.
No code changes.
This is the start of a big refactor to try to move tag out-pointers into
the return position of functions, muxing with error codes via the
sign-bit when necessary.
So instead of:
lfs3_tag_t tag_;
lfs3_data_t data_;
int err = lfs3_rbyd_lookup(&lfs3, &rbyd, rid, tag,
&tag_, &data_);
if (err) {
return err;
}
We now do:
lfs3_data_t data_;
lfs3_stag_t tag_ = lfs3_rbyd_lookup(&lfs3, &rbyd, rid, tag,
&data_);
if (tag_ < 0) {
return tag_;
}
In theory, removing an out-pointer saves both code and stack, though it
will be interesting to actually see how much of an affect this has after
the dust has settled.
littlefs v2 used this technique heavily for its 32-bit tags, but we
never did a comparison with/without tags in the return position.
This is a big rewrite in the test code, so hopefully this ends up worth
it :)
Lots of regex.
Note this implicitly limits error codes to 16-bits, but supported error
codes are already a bit limited because we're using int everywhere
(instead of int32_t). If we need 32-bit error codes we can always add
another type to represent the mux in the future (lfs3_etag_t?).
---
So far the code savings look promising:
code stack ctx
before: 36828 2368 656
after: 36576 (-0.7%) 2376 (+0.3%) 656 (+0.0%)
Stack usage is a big disappointing, but hopefully that is just a
temporary cost due to the internal scaffolding between different API
types while the refactor is ongoing.
Christopher Haster