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%)
There can always be more tests, but I think these give a nice set of
coverage over corner-cases in our traversal clobbering scheme.
These did find a couple bugs:
- If we clobber an inlined mroot, we need to adjust the mid by two
mdirs, but only if there is no mtree/mdirs.
To avoid this and other mid-related headaches, we just provide the new
mid in lfsr_mdir_commit, since we always know it here.
- lfsr_mdir_commit compares mdirs by mptr, which means we need to
clobber traversal's mdir's mptrs or else lfsr_mdir_commit will clobber
already-clobbered traversals.
There may be a better way to solve this, but it will probably get into
the weeds with how lfsr_mdir_commit relies on mids vs mptrs...
Code changes:
code stack
before: 34570 2624
after: 34566 (-0.0%) 2624 (+0.0%)
Now that the dust has settled and we sort of know what the traversal
implementation will look like, we can look at the before and after to
get a rough idea of how much the traversal API actually costs:
code stack
no-traversal (before): 33886 2560
yes-traversal (after): 34566 (+2.0%) 2624 (+2.5%)
Note this still includes the annoying lfsr_btree_traverse inlining stack
cost, which isn't really the traversal API's fault and may be avoidable
in the future.
This splits LFSR_TSTATE_BTREE into separate LFSR_TSTATE_MTREE/BTREE/
OBTREE states that indicate what to do next after traversing the btree.
This removes the need to point indirectly to file's o.next pointer,
since we can just point to the file struct itself.
I've also simplified opened-file clobbering to just move to the next
opened mdir, instead of searching for another unsynced file. This
simplifies things but does mean we now need to clobber traversals when
closing non-file objects. Implicitly calling lfsr_opened_clobber in
lfsr_opened_remove solves this with very little extra code cost,
deduplicated, and gives us a stronger invariant for traversal references
to closed objects. So win win?
Oh, and all the explicit open-file clobber checks are now deduplicated
into lfsr_opened_clobber again.
These tweaks save quite a bit of code:
code stack
before: 34740 2624
after: 34570 (-0.5%) 2624 (+0.0%)
Now, lfsr_mdir_commit just clobbers all traversals associated with the
current mdir, irrespective of mid.
This makes our traversal clobbering model quite a bit simpler, drops any
mess related to bshrub staging, and allows lfsr_mdir_commit to handle
most of the clobbering logic with the exception of opened file handles.
This also fits mtree/mroot clobbering a bit better, with mtree
clobbering behaving the same as a file btree in the mroot.
The downside is we will miss more blocks during clobbered traversals,
but clobbered traversals are best effort anyways. The saved code cost
and simpler/more robust clobbering model are probably worth it.
Traversal clobbering is already complicated enough...
Code/stack changes:
code stack
before: 34716 2648
after: 34740 (+0.1%) 2624 (-0.9%)
A number of traversal changes:
- Traversal now traverses the mtree's btree (the inner btree nodes)
separately from iterating over mdirs in the mtree.
This makes resuming clobbered traversals more robust as there's less
state to worry about. It also reduces all btree traversals to a single
state which simplifies the traversal logic and _in theory_ reduces
code/RAM costs.
This does add a second O(n logbn) pass through the mtree, but this
takes the fast path since we already validated btree nodes. mtree
traversal is probably dominated by mdir fetching anyways...
- lfsr_mdir_commit no longer clobbers mid-related traversals. This was a
bit too complicated with attrs potentially inserting new mids.
Instead, it's up to upper layers to explicitly clobber traversals.
Most of these already need to update dir positions, so it's not that
much extra code, but it does add cost.
lfsr_mdir_commit still clobbers mroot/mtree related traversals.
- We now stage bshrubs in traversals during mdir compaction, so we
shouldn't need to clobber traversals when the mdir compacts.
In theory as long as we clobber traversals that reference opened
files, we should never end up being the only reference to a bshrub. So
we should be able to stage bshrubs without cost.
This is _not_ working at the moment, because we aren't updating the
actual btraversal state correctly... not sure how to fix this yet...
Code/stack changes:
code stack
before: 34682 2544
after: 34716 (+0.1%) 2648 (+4.1%)
The surprise stack cost is _very_ interesting. Where is this coming
from?
It turns out when we reduce all btree traversals to a single state, and a
single function call, GCC is happy to inline lfsr_btree_traverse
directly into lfsr_fs_traverse.
This is great for code cost, but now lfs_fs_traverse contains the entire
stack frame of lfsr_btree_traverse, which is quite large. When we called
lfsr_btree_traverse twice, this stack frame was never nested with
lfsr_mtree_lookup, but now our tools think it is...
I'm not sure how to fix this. Maybe improving our tooling to understand
shrinkwrap optimizations will find this doesn't actually cost as much?
Or maybe not since this is in a complicated switch case state machine?
We could use an explicit __attribute__((noinline)), but this sort of
heavy-handed optimization guidance has been out-of-scope for littlefs up
until now...
I'm leaving this as-is for now, but it may be worth looking this again
in the future.
This adds lfsr_opened_clobber which can be called to clobber any open
traversals related to an mid, or all traversals if mid=-1. Clobbering
here means throw away any in-progress btraversals and move to the next
mid. We need to do this in several places to avoid outdated references
to btrees.
The other option would be to treat traversals like additional unsynced
file handles, add them to the lookahead buffer, copy shrubs during
compaction, etc, but I don't think we want to pay this cost since the
underlying data is otherwise inaccessible. No reason to check/repair
blocks we're not using anymore...
To make this work, LFS_BTRAVERSAL(bid) now supports resuming from a
specific bid, in lfsr_mtraversal_t we use this to resume mtree traversal
from a specific mid when clobbered.
---
Other changes:
- lfsr_mdir_commit now marks all removed mdirs with LFS_F_ZOMBIE, and
updating related dir positions is done in lfsr_remove/lfsr_rename.
I was originally planning to use LFS_F_ZOMBIE to clobber traversals as
well, but it didn't work out.
- Added lfsr_fs_weight, which returns the effective mdir/mtree weight,
including inlined-in-mroot mdirs.
- Fixed did-mask miscalculation in lfsr_mkdir where fs/mtree weight
wasn't shifted by mdir_bits. This probably just went unnoticed during
some mid refactoring.
- Changed traversals to only traverse _unsynced_ opened files. No reason
to traverse files we know match disk. This also makes is so only
unsynced files need to worry about clobbering traversals.
This has the catch that we need to point to the traversing file handle
somehow so we can clobber correctly. The (hacky?) solution is to point
to the next pointer itself, which tells us both where to go next, and
what file handle we are currently traversing.
- Moved LFS_F_UNSYNC flags to before file operations, instead of after.
This is needed for the above traverse-unsync-only logic in case we
alloc in the middle of a file operation.
Code changes:
code stack
before: 34454 2544
after: 34682 (+0.7%) 2544 (+0.0%)
Also added some specific tests over corner cases caused by traversing
and mutating the filesystem at the same time.
Unfortunately these aren't passing yet. Our mid-clobbering logic doesn't
handle mid insertion correctly, so we end up clobbering more traversals
than we need to...
The traversal logic is a bit simpler if everything can pass around/
populate the same struct, so this reverts some changes made when
implementing lfsr_traversal_t, bringing back bid as a side-channel and
making btinfo/mtinfo typedef aliases.
btinfo/mtinfo are also required arguments for lfsr_btree_traverse/
lfsr_fs_traverse now, so it's even easier to forward these to lower
layers if they alias.
What return-pointers should/shouldn't be optional is still an open
question, but at least for btinfo/mtinfo matching lfs_stat makes sense.
This saves a bit of code/stack:
code stack
before: 34474 2552
after: 34454 (-0.0%) 2544 (-0.3%)
This sort of turned into a complete refactor of lfs_alloc in order to
move/reuse the lookahead buffer filling logic into lfsr_fs_traverse.
lfs_alloc now calls lfsr_fs_traverse to fill the lookahead buffer when
no more blocks are available, but also you can too with lfsr_traversal_t
+ LFS_T_LOOKAHEAD.
The one big caveat being if any mutation happens to the filesystem, any
incomplete lookahead needs to be tossed out. To help with this,
lfsr_traversal_read now returns LFS_ERR_BUSY (-16) instead of
LFS_ERR_NOENT (-2) if the filesystem has been modified since the
traversal was opened.
Note that by default lfsr_traversal_t will still try to keep traversing
blocks, but can be told to terminate immediately with LFS_T_EXCL.
Continuing the traversal is probably desired for checking checksums,
debugging, etc, as otherwise you could end up looping over only the
first couple blocks in a write-heavy system, but if you are trying to
populate the lookahead buffer you probably want to just abort and start
over.
I considered adding a flags field to lfs_tinfo for this, but decided
against it since it would be the only place in the current API where we
don't use error codes to convey behavior-changing information. Though
this may be worth reconsidering at some point...
---
In reworking lfs_alloc, a lot of the internal logic was broken up into
specific functions:
- lfs_alloc_ckpoint - checkpoint the allocator
- lfs_alloc_discard - discard any lookahead
- lfs_alloc_shift - discard/shift lookahead if progress can be made
- lfs_alloc_markinuse - mark a block as in-use
- lfs_alloc_markfree - mark any remaining blocks as free
- lfs_alloc_findnext - find the next free block in lookahead
If anything this probably makes lfs_alloc more readable, though the
original motivation was to allow lfsr_traversal_t to only shift/zero the
lookahead buffer if there's a chance we can make progress.
This was based on upstream work by opilat and myself.
Code changes:
code stack
before: 34226 2560
after: 34474 (+0.7%) 2552 (-0.3%)
This adds the lfsr_traversal_t object, which encapsulates a traversal
over all blocks in the filesystem.
This replaces the earlier lfs_fs_traverse function, but is sort of
"inside-out" in that instead of taking a callback, an lfsr_traversal_t
object can be read from to return lfs_tinfo structs that describe the
blocks in our system:
lfsr_traversal_open(&lfs, &t) => 0;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_MDIR;
tinfo.block => 0x0;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_MDIR;
tinfo.block => 0x1;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_DATA;
tinfo.block => 0x42;
lfsr_traversal_read(&lfs, &t, &tinfo) => LFS_ERR_NOENT;
lfsr_traversal_close(&lfs, &t) => 0;
This is more flexible, allowing for aborted traversals, yielding,
rewinding, etc, but also more complicated to implement, since it
requires all traversal state to be stored explicitly.
Fortunately, since we needed to reimplement filesystem traversals
anyways, I was able to build this into the new system from the start
using a small state machine to drive the traversal internally. So all
that was really needed was a bit of window dressing, adding
LFS_TYPE_TRAVERSAL to track open traversals, logic to handle
invalidating traversals on file close, mutation, etc...
Which, uh, that last one is not implemented yet. Interactions with other
filesystem operations gets messy, so I figured I'd go ahead and commit
what is currently working.
Ugh, and tests. The biggest downside of adding lfsr_traversal_t is how
many more corner-cases it adds to the system...
lfsr_traversal_t is going to be a work-in-progress for a bit...
---
lfsr_traversal_t also adds a really interesting path towards more access
to advanced low-level operations, such as checking metadata/data
checksums, incrementally progressing the garbage collector, even
repairing bad metadata/data blocks eventually.
Currently implemented is LFS_T_CKMETADATA and LFS_T_CKDATA to check
metadata and data checksums respectively. This is the first feature that
actually allows you to validate data checksums.
Code changes so far:
code stack
before: 33886 2560
after: 34226 (+1.0%) 2560 (+0.0%)
We don't actually need these, all we need are utils defined for the
largest integer size we operate on, currently uint32_t.
Counterintuitively this should make it easier to adopt different integer
widths in the future.
Or maybe this will bite us when lfs_off_t >> lfs_size_t? Oh well, if
that's the case we can fix it then.
No code changes:
code stack
before: 33886 2560
after: 33886 (+0.0%) 2560 (+0.0%)
Sort of like SHT_NOTE in elf files, but with no defined format. Using
LFSR_TAG_NOTE for additional noise/nonces is still encouraged, but it
can also be used to add debug info.
These are basically the same, but with reverting asserts-as-hints the
unreachable statements give the compiler more info. It also avoids
unused variable warnings, and matches the if-err pattern we use for
propagating errors elsewhere.
Code changes were a bit weird, seems the compiler decided to uninline
lfsr_data_fromgeomtry and lfsr_data_fromleb128 for some reason:
code stack
before: 33882 2560
after: 33886 (+0.0%) 2560 (+0.0%)
The ability of GCC is just insufficient for asserts-as-hints and at some
point it's not worth trying to workaround this.
Code with asserts should never be worse than code without asserts, so we
might as well just disable asserts completely when not debugging:
code stack
hint-assert (before): 33918 2592
no-assert (after): 33882 (-0.1%) 2560 (-1.2%)
Clang does no better here (targeting x86):
code
clang+hint-asserts (before): 51946
clang+no-asserts (after): 51663 (-0.5%)
Maybe in the future some builtin will let us force pure expressions. It
would be interesting to revisit assert driven optimizations at some
point.
With rcompat/wcompat flags, on-disk minor version bumps will hopefully
not be needed for a long time (ever?). And if the on-disk version never
changes, why was a word to report it every lfsr_fs_stat call?
But this may be something to listen to user feedback on. Worst case we
can always readd fsinfo.disk_version if users find it useful.
Code changes:
code stack
before: 33922 2592
after: 33918 (-0.0%) 2592 (+0.0%)
All in-flight blocks should be tracked by at least an open file handle
when calling lfsr_mdir_commit. It's the internals of lfsr_mdir_commit
that are really protected by the lfsr_alloc_ckpoint system.
Making lfsr_mdir_commit implicitly call lfsr_alloc_ckpoint removes one
step that was easy to forget, and means we don't need to pepper every
function with lfsr_alloc_ckpoint willy-nilly.
This also merges lfsr_fs_prepareappend -> lfsr_fs_mkconsistent, which
were really two names for the same function. lfsr_fs_grow not calling
lfsr_fs_prepareappend made this name potentially confusing.
Code changes:
code stack
before: 33942 2592
after: 33922 (-0.1%) 2592 (+0.0%)
While it may be useful to know when/why lfsr_fs_fixgrm fails, at this
point in lfsr_rename/lfsr_remove the operation has already succeeded as
far as the filesystem is concerned.
It's counterintuitive, but ignoring these errors actually tells the user
_more_ information, specifically whether or not the operation completed
on disk.
At least we can log the error via LFS_WARN, and such errors will likely
come up again in a future operation, such as the call to lfsr_fs_fixgrm
on the next filesystem mutation.
This was noticed in test_grow, which tests error code-paths quite a bit
more than any other test.
Code changes:
code stack
before: 33934 2592
after: 33942 (+0.0%) 2592 (+0.0%)
So LFSR_TAG_MOVE is last, as it's rather complex and also needs to
handle bsprouts/bshrubs.
Compiler noise:
code stack
before: 33926 2592
after: 33934 (+0.0%) 2592 (+0.0%)
Not sure how this was missed for so long, but we completely forget about
in-flight mroot attrs if we happen to uninline the mtree.
I guess this was missed because only some late-stage fs ops need to
commit mroot attrs (lfsr_fs_grow, lfsr_setattr, upgrades, etc), but
being able to commit to the mroot is definitely an operation we need to
support.
Fixing this in a non-awkward way was a bit tricky. We need some way to
commit both the provided attr-list and our new mtree, but all of the
lower layers only accept a single attr-list. The solution here
is to add a special tail-recursive LFSR_TAG_ATTRS that can be used to
chain together multiple attr-lists. This solves the problem quite
elegantly and may actually be useful in the future?
It takes a bit of code:
code stack
before: 33850 2584
after: 33926 (+0.2%) 2592 (+0.3%)
But this solves our final lfsr_fs_grow-related bug. No more mroot-split
hacks in test_grow, and we can now grow any stuck filesystem.
Well this turned into a never-ending can of worms...
I guess the good news is our newly added lfsr_grow_incr_* tests are
_very_ good at finding post-error-resume bugs.
Implementation-wise, this was fairly straightforward thanks to prior
work by BrianPugh, kaetemi, and myself:
1. Made block_count pseudo-optional by adding lfs.block_count so we can
mutate it based on what we find on-disk.
This was done a bit different from the previous implementation,
instead of setting block_count=0 to read the block_count from disk,
we allow any block_count <= the configured block_count.
This matches how we handle name_limit/file_limit/etc, and allows
users to mount a filesystem with unknown block_count while asserting
an upper bound.
2. Added lfsr_fs_grow, which can grow the filesystem.
The is basically the same as the previous implementation except we're
a bit more careful with the lookahead buffer.
I thought the previous impl might have been broken w.r.t. lookahead
buffer, but fortunately it's only broken in a way that makes us think
newly available blocks are temporarily in-use. Which is a bit funny.
One interesting thing that came out with more aggressive tests is
that it's possible to get locked-up in lfsr_fs_preparemutation trying
to clean up grms/orphans before we change the filesystem size.
Fortunately it turns out we don't _really_ need to call
lfsr_fs_preparemutation here. This gets a bit delicate, but means we
should always be able to grow a full filesystem.
To test this I've added both the simple grow/error tests from the
previous version, as well as a set of fuzz tests (a la test_relocations
and friends) that incrementally grow the filesystem when encountering
LFS_ERR_NOSPC. These have a surprising amount coverage, testing
lfsr_fs_grow, lfsr_fs_stat, lfsr_fs_size, and resuming operations after
encountering an error.
Which also means they found bugs:
- lfs_alloc_setinuse was not broken before, because lookahead.start was
always a multiple of lookahead_size. But now with lfs_alloc_discard,
this invariant may not be true.
I've just changed all lookahead.start updates to mod block_count. This
adds a bit of code, but is much easier to reason about.
While fixing this, I also added an assert to never allocate blocks
{0,1} in lfs_alloc. This is a good assert to have, but did require
some tweaks to test_btree to avoid these blocks.
- We were incorrectly patching grms in lfsr_mdir_commit when mdelta=0.
Funnily enough we also proceed to ignore the patched grm most of the
time when mdelta=0, so this went unnoticed.
- It turns out we're completely ignoring rid=-1 attrs if we split the
mroot. Not sure how this was missed. It's a bit important.
Note this is still broken. Fixing this requires some rather invasive
changes to lfsr_mdir_commit's internal logic that should probably be
in another commit...
Note again fwrite_fuzz is omitted. Currently the state of data in opened
files is undefined after a failed write, so this wouldn't really be
testing anything interesting...
More features = more code, and all of this bug fixing meant several
things contributed to code/stack changes in this commit:
code stack
before: 33654 2592
+variable block_count: 33646 (-0.0%) 2584 (+0.0%)
+lfsr_fs_grow: 33818 (+0.5%) 2584 (-0.3%)
+lookahead-start-fix: 33842 (+0.6%) 2584 (-0.3%)
+grm-patch-fix (after): 33850 (+0.6%) 2584 (-0.3%)
Wild that variable block_count actually saves code/stack. I guess the
indirect lfs->cfg->block_count load can get costly...
We really only rely on cksum here.
Passing weight was useful for an assert, but it clutters up the function
call with a parameter that isn't used normally.
Removing ths saves a bit of code:
code stack
before: 33670 2592
after: 33654 (-0.0%) 2592 (+0.0%)
The theory is that lfsr_opened_isopen is a relatively special case, and
the main cause of LFS_ASSERT side-effects. All other LFS_ASSERTs are
either limited to simple expressions, or small static-inline functions.
Unfortunately, even without lfsr_opened_isopen asserts, it seems
asserts-as-hints still results in worse code/stack costs:
code stack
no-assert (before): 33626 2552
hint-assert (after): 33670 (+0.1%) 2592 (+1.6%)
Digging around in the low-level assembly, it seems that what is
happening is the increased number of calls to static-inline functions is
causing the compiler to prefer to not-inline functions more often. Then,
even if calls would be eliminated as dead-code, the damage is done to
the containing function.
It's not entirely clear how this could be avoided. Maybe a separate
LFS_ASSERT for only pure expressions? This may not be worth trying to
solve outside of the compiler...
I realized the reason asserting on opened/closed file handles added so
much extra code was because our LFS_ASSERT macro doesn't properly
eliminate side-effects.
Consider this assert:
LFS_ASSERT(lfsr_opened_isopen(lfs, &dir->o));
Expanded:
((lfsr_opened_isopen(lfs, &dir->o))
? (void)0
: __builtin_unreachable());
Even though the compiler knows lfsr_opened_isopen must return true
here, it doesn't know what possible side-effects calling
lfsr_opened_isopen may have, and can't eliminate the function call.
This is quite a bit more obvious if you did something like:
LFS_ASSERT(lfsr_file_sync(&lfs, &file) == 0);
But since lfsr_opened_isopen is a static inline function, it gets a
little bit less clear. Even worse, whether or not the call is eliminated
probably depends if it's actually inlined and other compiler
optimization noise.
---
This commit effectively reverts LFS_ASSERT as a compiler hint, making
LFS_ASSERT an empty string if LFS_NO_ASSERT is defined.
This may not be the optimal solution, but it at least keeps the
programmer's intuition that anything in LFS_ASSERT has zero impact when
asserts are disabled. It would be nice if there was some way to tell the
compiler that an expression should have no side-effects, but as far as
I'm aware this is not currently possible.
Measurements show that just disabling asserts wins in both code and
stack over trying to leverage asserts-as-hints:
code stack
hint-assert (before): 33904 2584
no-assert (after): 33626 (-0.8%) 2552 (-1.2%)
At least both of these win over leaving asserts enabled, so
asserts-as-hints does eliminate _most_ code (measured here
with a simple assert-loop, since I assume that would have the smallest
code footprint):
code stack
loop-assert: 36874 2616
hint-assert (before): 33904 (-8.1%) 2584 (-1.2%)
no-assert (after): 33626 (-8.8%) 2552 (-2.4%)
Unfortunately asserts-as-hints was doing quite a bit of heavy lifting
at preventing overzealous GCC warnings. It took quite a few tweaks to
get GCC to shut up, and I'm still not entirely sure the best way to tell
GCC that some functions only return negative values. Currently I just
limit certain error checks to only check for negative values, which is
not great, but at least gets the code compiling again...
This adopts upstream opened/closed assertions, which are useful for
catching user mistakes (note the bug fixes in our tests):
- Assert if already open in lfsr_*_open
- Assert if not open in lfsr_file_* and lfsr_dir_* functions
- Assert if any files/dirs are still open in lfsr_unmount
Unfortunately this had a surprising code cost for what really should
have been a noop as far as the compiler is concerned. And saved a bit of
stack? Maybe our assertion hints are causing a surprising amount of code
movement? I'm really not sure what's going on and this deserves more
investigation:
code stack
before: 33686 2592
after: 33904 (+0.6%) 2584 (-0.3%)
At the very least this didn't add a noticable amount of testing time. I
was a bit concerned because our orphan/zombie testing grows opened-list
operations ~O(n^2), but any measurable overhead is less than how much
our test runtime swings between runs (+-~20s).
Before, lfsr_mount would return LFS_ERR_INVAL if it could not mount the
filesystem for any reason. This matches POSIX's mount behavior, but is,
in my humble opinion, unhelpful... A corrupted filesystem image is an
"invalid parameter"?
This splits lfsr_mount's failed-to-mount behavior into two error codes:
- LFS_ERR_CORRUPT - Failed to mount because something was corrupted.
Unlikely disk contains a littlefs image.
- LFS_ERR_NOTSUP - Failed to mount because on-disk filesystem is
incompatible. Reconfiguring your driver may successfully mount.
This offers a bit more of a hint to users on why mount failed. Though
relevant error logs will probably have more useful information. Worst
case users can always treat CORRUPT/NOTSUP the same after calling
lfsr_mount.
Code changes:
code stack
before: 33674 2592
after: 33686 (+0.0%) 2592 (+0.0%)
I realized we really can't do anything if we find a file of unknown
type... If we don't understand a file's data structure, we can't really
do any bookkeeping. Allocating new blocks will probably corrupt unknown
files since we can't traverse any related B-trees, and mdir compaction
would be an absolute mess.
So, instead, just print an error and bail during mount.
Eventually we could at least fallback to readonly mode, but this is
currently a TODO item.
This also means the LFS_ERR_NOTSUP logic in lfsr_mtree_pathlookup is no
longer needed. Since, even with readonly fallback, we should never
mutate a filesystem with unknown file types.
Maybe in the future we could have a sort of known-but-not-supported mode
for file types? So special file types could not be support, but at least
understood enough to support traversal/remove/rename/etc?
Code changes:
code stack
before: 33694 2592
after: 33674 (-0.1%) 2592 (+0.0%)
Returning the actual on-disk file type is probably more useful for users
as this gives them more information.
I was originally concerned about collisions with future internal types,
LFS_TYPE_TRAVERSAL, etc, needed for internal opened-list tracking, but
it turns out we can avoid problems by starting internal types at 0x80,
since on-disk file types are only 7-bits.
Code changes:
code stack
before: 33710 2592
after: 33694 (-0.0%) 2592 (+0.0%)
This adds a couple things so our unknown file types don't just cause our
filesystem to fall over:
- lfsr_mount now prints a warning on any unknown file types found at
mount time. Since we're already iterating over all files to find
orphans, this is basically free.
- Added LFS_TYPE_UNKNOWN to represent files with an unknown/unsupported
type. This is now returned by lfsr_stat/lfsr_dir_read for files of any
unknow type.
- Added LFS_ERR_NOTSUP. This is now returned by functions that attempt
to modify a file of unknown type, and my have more use cases in the
future.
It's tempting to allow remove/rename on unknown file types, but since
we don't know what data structures these may be referencing, doing so
would likely leak storage. Or worse. Shrubs for example would just
explode if you only moved the metadata entry.
This also adds test_incompat_unknown to test these cases.
Code changes are minimal, though there are a number of extra conditions
to check for unknown file types. The lfsr_mount condition is
particularly fun as it should be completely optimized out when debug
statements are disabled:
code stack
before: 33670 2592
after: 33710 (+0.1%) 2592 (+0.0%)
Unlike the other test_compat tests, the test_incompat tests cover
specific corner cases and don't require any special linking. We probably
always want to run these, and keeping them merged with test_compat risks
the entire suite being omitted at some point.
The test_compat tests are a bit special and probably deserves a
dedicated test suite.
test_compat has been very useful for testing compatibility on patch and
minor releases.
Though, in porting the tests, I've realized these are actually really
flimsy w.r.t. API changes... lfsp_config notably relies on compatible
struct layouts, which is _not_ guaranteed by littlefs's compatibility
rules.
For this reason I've restricted these tests to only run if LFS_VERSION
doesn't change, though this may be worth reinvestigating in the future.
test_compat on minor API releases would be quite valuable...
lfsr_fs_stat is also not quite up to date with upstream yet. It's really
just a small shim copying over static configs at the moment (except for
name_limit/file_limit). This is because we're still missing most of what
would actually be interesting here: variable block counts, minor
versions, etc.
And of course a minimal lfsr_fs_stat means minimal code changes:
code stack
before: 33642 2592
after: 33670 (+0.1%) 2592 (+0.0%)
See comments/previous commits. lfsr_fs_mkconsistent allows running
internal consistency operations without any other filesystem changes.
Implementation-wize, this just calls lfsr_fs_preparemutation which we
already need to, uh, prepare for mutation. Though it may do some
additional work in the future, such as setting compat flags, version
numbers, etc.
Added mkconsistent permutations to what seems like the relevant tests:
- test_forphans - easy for lfsr_fs_mkconsistent to accidentally delete
orphans/zombies.
- test_powerloss - heavy fuzz tests over powerloss-related consistency
operations, though this does multiply every permutation by ~2x...
Code cost minimal. I guess this is what it costs to make an internal
function non-static:
code stack
before: 33634 2592
after: 33642 (+0.0%) 2592 (+0.0%)
Changed:
- lfsr_mkdir(&lfs, "/") => LFS_ERR_EXIST
- lfsr_file_open(&lfs, &file, "/", *) => LFS_ERR_ISDIR
Unchanged:
- lfsr_remove(&lfs, "/") => LFS_ERR_INVAL
- lfsr_rename(&lfs, "/", *) => LFS_ERR_INVAL
- lfsr_rename(&lfs, *, "/") => LFS_ERR_INVAL
This better matches what Linux, etc, does: prefering a normal
dir-related error unless the only issue is that the dir in question is
the root.
Though Linux, etc, usually return EBUSY, which seems to also be used for
special device files. We could add LFS_ERR_BUSY, but I'm not sure it's
really worth it for such a rare error. It's not like the name would help
anything...
Internally, lfsr_mtree_pathlookup always returns LFS_ERR_INVAL for root,
so this unfortunately requires a bit more code to map to the correct
errors:
code stack
before: 33598 2592
after 33634 (+0.1%) 2592 (+0.0%)
Turns out there's very _very_ small powerloss hole in our current
perturb logic.
We rely on tag valid bits to validate perturb bits, but these
intentionally don't end up in the commit checksum. This means there will
always be a powerloss hole when we write the last valid bit. If we lose
power after writing that bit, suddenly the remaining commit and any
following commits may appear as valid.
Now, this is really unlikely considering we need to lose power exactly
when we write the cksum tag's valid bit, and our nonce helps protect
against this. But a hole is a hole.
The solution here is to include the _current_ perturb bit (q) in the
commit's cksum tag, alongside the _next_ perturb bit (p). This will be
included in the commit's checksum, but _not_ in the canonical checksum,
allowing the commit's checksum validate the current perturb state
without ruining our erased-state agnostic checksums:
.---+---+---+---. . . .---+---+---+---. \ \ \ \
|v| tag | |v| tag | | | | |
+---+---+---+---+ +---+---+---+---+ | | | |
| commit | | commit | | | | |
| | | | +-. | | |
+---+---+---+---+ +---+---+---+---+ / | | | |
|v|qp-------------. |v|qp| tag | | . . .
+---+---+---+---+ | +---+---+---+---+ | . . .
| cksum | | | cksum | | . . .
+---+---+---+---+ | +---+---+---+---+ | . . .
| padding | | | padding | | . . .
| | | | | | . . .
+---+---+---+---+ | . +---+---+---+---+ | | | |
| erased | +-> |v------------------' | | |
| | | +---+---+---+---+ | | |
. . | | commit | +-. | +- rbyd
. . | |.----------------. | | | | cksum
| +| -+---+---+---+ | / | +-. /
+-> |v|qp| tag | '-----' | |
| +- ^ ---+---+---+ / |
'------' cksum ----------------'
+---+---+---+---+
| padding |
| |
+---+---+---+---+
| erased |
| |
. .
. .
(Ok maybe this diagram needs work...)
This adds another thing that needs to be checked during rbyd fetch, and
note, we _do_ need to explicitly check this, but it solves the problem.
If power is loss after v, q would be invalid, and if power is lost after
q, our cksum would be invalid.
Note this would have also been an issue for the previous cksum + parity
perturb scheme.
Code changes:
code stack
before: 33570 2592
after: 33598 (+0.1%) 2592 (+0.0%)
The previous cksum + parity scheme worked, but needing to calculate both
cksum + parity on slightly different sets of metadata felt overly
complicated. After taking a step back, I've realized the problem is that
we're trying to force perturb effects to be implicit via the parity. If we
instead actually implement perturb effects explicitly, things get quite
a bit simpler...
This does add a bit more logic to the read path, but I don't think it's
worse than the mess we needed to parse separate cksum + parity.
Now, the perturb bit has the explicit behavior of inverting all tag
valid bits in the following commit. Which is conveniently the same as
xoring the crc32c with 00000080 before parsing each tag:
.---+---+---+---. . . .---+---+---+---. \ \ \ \
|v| tag | |v| tag | | | | |
+---+---+---+---+ +---+---+---+---+ | | | |
| commit | | commit | | | | |
| | | | +-. | | |
+---+---+---+---+ +---+---+---+---+ / | | | |
|v|p--------------. |v|p| tag | | . . .
+---+---+---+---+ | +---+---+---+---+ | . . .
| cksum | | | cksum | | . . .
+---+---+---+---+ | +---+---+---+---+ | . . .
| padding | | | padding | | . . .
| | | | | | . . .
+---+---+---+---+ | . +---+---+---+---+ | | | |
| erased | +-> |v------------------' | | |
| | | +---+---+---+---+ | | |
. . | | commit | +-. | +- rbyd
. . | | | | | | | cksum
| +---+---+---+---+ / | +-. /
'-> |v----------------------' | |
+---+---+---+---+ / |
| cksum ----------------'
+---+---+---+---+
| padding |
| |
+---+---+---+---+
| erased |
| |
. .
. .
With this scheme, we don't need to calculate a separate parity, because
each valid bit effectively validates the current state of the perturb
bit.
We also don't need extra logic to omit valid bits from the cksum,
because flipping all valid bits effectively makes perturb=0 the
canonical metadata encoding and cksum.
---
I also considered only inverting the first valid bit, which would have
the additional benefit of allowing entire commits to be crc32ced at
once, but since we don't actually track when we've started a commit
this turned out to be quite a bit more complicated than I thought.
We need someway to validate the first valid bit, otherwise it could be
flipped by a failed prog and we'd never notice. This is fine, we can
store a copy of the previous perturb bit in the next cksum tag, but it
does mean we need to track the perturb bit for the duration of the
commit. So we'd end up needing to track both start-of-commit and the
perturb bit state, which starts getting difficult to fit into our rbyd
struct...
It's easier and simpler to just flip every valid bit. As a plus this
means every valid bit contributes to validating the perturb bit.
---
Also renamed LFSR_TAG_PERTURB -> LFSR_TAG_NOISE just to avoid confusion.
Though not sure if this tag should stick around...
The end result is a nice bit of code/stack savings, which is what we'd
expect with a simpler scheme:
code stack
before: 33746 2600
after: 33570 (-0.5%) 2592 (-0.3%)
Turns out we don't need SHRUBALLOC, as we can infer if we need to reset
the shrub based on if it already exists in our mdir. Not in mdir =>
needs to alloc/reset.
This saves an internal tag and a bit of code:
code stack
before: 33770 2600 (+0.0%)
after: 33746 (-0.1%) 2600 (+0.0%)
It's counter-intuitive, but no top-level API should return
LFS_ERR_CORRUPT. Instead, if we can't make progress because of a corrupt
block, we should return LFS_ERR_NOSPC. This makes it easier for users to
write code that is well behaved even when a device is end-of-life.
It's up to our mroot extension algorithm to make sure this case can't be
reached in normal operation unless the device is _actually_ at
end-of-life.
Because mroot extension is a bit of a special case, we weren't
converting these corrupt errors to nospc errors consistently. This is
fixed now, along with a couple more hopefully-useful logging statements.
Found while playing around with test_exhaustion + block_recycles=-1.
This should assert on bad wear-leveling, but LFS_ERR_CORRUPT was
unexpected. Added an explicit test because this is an easy thing to let
split through:
- test_badblocks_mrootanchor_wear
Code changes were surprisingly minimal, I wonder if constants are being
swapped out somewhere low-level?
code stack
before: 33766 2600
after: 33770 (+0.0%) 2600 (+0.0%)
The main test additions are the test_powerloss tests, intended to be
high-level tests over difficult/weird powerloss environments (such as
out-of-order writes!):
- test_powerloss_dir_many - 2242 pls
- test_powerloss_file_many - 8856 pls
- test_powerloss_file_pl_fuzz - 384508 pls
- test_powerloss_filedir_pl_fuzz - 268339 pls
But there was also a bunch of other test movement in the late-stage/
high-level tests. I'm trying to keep the core of these tests somewhat
consistent so we have a nice template to extend for future testing, in
case we want to test other environmentalish concerns, but not all of
these tests make sense in all of these contexts:
badblocks powerloss relocations exhaustion
dir_many y y y
dir_fuzz y y y
file_many y y y
file_fuzz y y y
fwrite_fuzz y y
orphanzombie_fuzz y y y
orphanzombiedir_fuzz y y y
file_pl_fuzz y y
filedir_pl_fuzz y y
Why not:
- dir/file_many+exhaustion? - Needs to be unbounded
- dir/file_fuzz+powerloss? - Takes O(n^2)
- fwrite_fuzz+powerloss? - Takes O(n^2)
- fwrite_fuzz+relocations? - Doesn't really test anything
- orphanzombie*_fuzz+powerloss? - Powerloss kills zombies
- file*_pl_fuzz+badblocks? - PL + Badblocks currently incompactible
- file*_pl_fuzz+exhaustion? - PL + Badblocks currently incompactible
---
Of course, in order to actually get out-of-order write testing working,
we need to implement out-of-order write syncing.
Fortunately this was a simple exercise in placing lfsr_bd_sync calls
before any mdir commits where we may have unsynced data:
- in lfsr_file_sync, to sync any pending file data
- in lfsr_mdir_commit, to sync any mroot/mtree changes
We also call lfsr_bd_sync _after_ mdir commits in case users expect to
sequence any filesystem-external operations such as network, UI, etc. In
theory this could be optional, but no users have really requested it
yet, so leave that for a potential future improvement:
- in lfsr_mdir_commit
- in lfsr_formatinited (really just because we don't go through
lfsr_mdir_commit)
Note that lfsr_rbyd_commit has been relaxed in the scheme. It only
flushes caches, and does _not_ call lfsr_bd_sync. This is useful for
allowing multiple B-tree nodes to be written out-of-order, also long as
the whole thing is synchronized before any mdir commit.
All of these lfsr_bd_sync calls add a bit of code, but not really an
amount to care about:
code stack
before: 33678 2600
after: 33766 (+0.3%) 2600 (+0.0%)
This sort of reverts the addition of lfsr_bd_unprog, but with a slightly
better API. lfsr_bd_unprog was too much of a hack, and isn't really
generalizable. The align flag isn't necessarily any better, but at least
it's the simplest/least-confusing solution available.
And it's net savings, code-wise:
code stack lfs_t
before: 33690 2608 164
after: 33678 (-0.0%) 2600 (-0.3%) 160 (-2.4%)
The only real use case for the bd runtime bounds checks is to abort rbyd
commits when they run off the end of the block. Since rbyd's now have
their own set of low-level append functions, we're better off doing the
bounds checks there and changing all of the lfsr_bd_* bounds checks to
asserts.
Block overflows are a particularly easy mistake to make, and one that
would be good to catch early.
One interesting thing to note: We're now using LFSR_TAG_DSIZE for range
checks instead of the actual tag encoding. This may seem suboptimal, but
if LFSR_TAG_DSIZE can't fit in the remaining space in the block, the
cksum tag wouldn't be able to fit anyways. So we're not really wasting
any space.
This saves a nice bit of code:
code stack
before: 33690 2608
after: 33610 (-0.2%) 2608 (+0.0%)
While exploring the test_badblocks ERASENOOP failure more, I realized
the problem is that we are nesting crc32cs.
To be clear, using crc32cs to validate progs in general is not an issue,
that is perfectly fine on paper. The issue is that we were using crc32cs
to validate progs _that contain crc32cs_.
Looking at the collision, we can see the fully expanded lleb128s we use
for our cksum tags:
00 00 00 ff b0 02 00 87 80 80 00 3e c0 7f 7e => bdfa9b10
ab 77 de c2 b0 03 00 87 80 80 00 3e 38 d5 22 => bdfa9b10
'-.-' ^ '----.----' '----.----'
'----|------|-----------|-- cksum tag
'------|-----------|-- cksum weight (0)
'-----------|-- cksum size + padding
'-- cksum crc32c
So we ended up perfectly aligning the cksum's crc32c with our cache
line. Lucky us.
Unfortunately funny math makes it so that whenever a crc32c contains a
crc32c, the inner crc32c sort of cancels itself out from the outer
crc32c. So these two messages end up mathematically equivalent, even
though they contain different data:
crc(m) = m(x) x^|P|-1 mod P
crc(m ++ crc(m)) = (m(x) x^|P|-1 + (m(x) x^|P|-1 mod P)) x^|P|-1 mod P
crc(m ++ crc(m)) = (m(x) x^|P|-1 + m(x) x^|P|-1) x^|P|-1 mod P
crc(m ++ crc(m)) = 0 x^|P|-1 mod P
crc(m ++ crc(m)) = 0
So using a crc32c to check progs is not fit for purpose.
This leaves us with a couple options:
1. Use a different checksum, or do something like rearranging bytes to
avoid this cancelling out issue. Unfortunately this gets tricky since
crc32cs are linear, simply using an xor mask won't work...
2. Don't check progs at such a low-level, but at a high-level using the
rbyd/data block crc32cs. Since this would mean only one crc32c, this
would avoid nesting issues. Unfortunately this would probably come
with quite a high code cost to try to keep track of both the
before+after rbyd cksums everywhere...
3. Just read back the data into the rcache to compare at the byte-level,
which would mean clobbering our rcache when prog checking is enabled.
This commit goes with option 3., which is probably the simplest. It also
removes any question of crc32c collision, which could be a real nuisance
when debugging low-level block device operations, a use case where prog
checking will hopefully be quite valuable.
Clobbering the rcache also has the advantage of reverting the prog
>= read requirement, which is nice for flexibility. Though this needs to
be tested.
---
There was a bit of a hiccup, and that was how prog checking interacts
with lfsr_bd_cpy. lfsr_bd_cpy used the rcache to hold data being copied
to/from disk, but this data needs to be checked, and prog checking would
clobber the rcache. Problems! I guess this is one footgun of the
internal lfsr_bd_readnext API...
The solution is to instead turn this around and use the pcache to hold
any copied data, since this would not be clobbered when prog checking.
This has some other knock-on effects, mainly that we can't take
advantage of read hints in lfsr_bd_cpy, but has the added advantage of
potentially not clobbering the rcache at all when no checking progs.
Code changes were fairly minimal:
code stack
before: 33718 2608
after: 33690 (-0.1%) 2608 (+0.0%)
The initial goal was the simplify these layers. Keyword being initial.
Unfortunately these layers are both complex and subtle, so the goal
shifted more to be rigorous and reliable.
This mainly meant rearranging our prog/read loops to follow a consistent
style, with higher-priority buffers being sorted out before flushing
things. This gets a bit tricky with wanting to support both cache
bypassing and buffer-lending prognext/readnext, but with some redundant
prognext/readnext calls it's doable.
We also now aggressively discard rcaches on pcache conflicts. This
change does rely on the prog >= read assumption. Discarding rcaches
means we should no longer have overlapping caches, so hopefully no more
zombie rcache issues.
Our bypassing heuristic was also tweaked a bit. Now, in addition to
alignment, >= read/prog_size, and >= hint requirements, we also require
operations to be >= r/pcache_size. This should improve cache usage when
r/pcache_size >> read/prog_size, since we were too eager to bypass
before.
Long story short, this ended up being more just things shifting around
than a significant simplification of the bd layers. At least we ended up
with a nice bit of stack savings:
code stack
before: 33682 2640
after: 33718 (+0.1%) 2608 (-1.2%)
Also, test_badblocks with LFS_EMUBD_BADBLOCK_ERASENOOP is now failing. I
was worried the amount of fuzz testing we do would eventually end up
with a naturally occuring crc32c collision, and sure enough we did! Yayy
yyyy...
00 00 00 ff b0 02 00 87 80 80 00 3e c0 7f 7e => bdfa9b10
ab 77 de c2 b0 03 00 87 80 80 00 3e 38 d5 22 => bdfa9b10
Need to think about what to do with this... For now I've just commented
out the problematic test.
This configuration option enables the previous behavior of reading back
every prog to check that the data was written correctly.
Unfortunately, this brings a bit of baggage, thanks to our cache
interactions being more complicated now:
- We really want to reuse the rcache for prog validation, despite the
cache performance implications. Unfortunately, we simply can't, thanks
to the new bd utility functions tying up the rcache. lfsr_bd_cpy, for
example, does not expect rcache to be invalidated between a read and
prog, and if it is, things break (I may or may not have found this by
experience).
These bd utilities are valuable, so we really need some other way to
validate our progs.
- Since we can't rely on the rcache, this leaves checksumming as the
only option for validating progs. Checksumming isn't perfect, as there
is a decent chance of false negatives, but to be honest it's probably
good enough for anything that's not malicious.
- This also adds the new constraint that we need to be able to read back
any prog into the pcache, which implies read_size <= prog_size. This
constraint didn't exist when we could clobber our rcache, but this is
not worth throwing away the new bd utilities. Not to mention
clobbering our rcache could hurt cache performance.
Why not make read_size <= prog_size conditional on check_progs?
The main reason is convenience. One very compelling use case for
check_progs is to help debug unknown filesystem/integration failures,
buf if you can't enable check_progs without changing the filesystem
configuration, you can't really rely on check_progs for debugging.
This helps future proof what we expect from block devices, in case
future error detection/correction mechanisms can benefit from our
prog_size always being readable.
Code changes were not that significant, however there was a surprising
stack cost. This seems to be because lfsr_bd_read__ can now be called
from multiple places, causing it to no longer be inlined in
lfsr_bd_read_, costing a bit of stack for the additional function call:
before: 33566 2624
after: 33682 (+0.3%) 2640 (+0.6%)
Christopher Haster