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.
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?
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%)
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...
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
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%)
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%)
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%)
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%)
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%)
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...
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%)
These don't really rely on any advanced file operations, and can run in
parallel.
This was a leftover from when test_incompat+test_compat were merged, and
test_compat should probably run after all file operations are thoroughly
tested.
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%)
Now that we are testing more powerloss behaviors, test_powerloss is the
longest running test suite by a decent margin:
Before:
$ ./scripts/summary.py test.csv -bsuite -ftime -Stime
... snip ...
test_rbyd 578.6
test_fwrite 984.5
test_badblocks 1341.5
test_exhaustion 1648.3
test_powerloss 2192.3 <--
TOTAL 7378.6
$ ./scripts/summary.py test.csv -bcase -ftime -Stime
... snip ...
test_fwrite_fuzz_aligned 247.2
test_exhaustion_file_fuzz 287.7
test_exhaustion_dir_fuzz 307.2
test_exhaustion_orphanzombie_fuzz 389.1
test_exhaustion_orphanzombiedir_fuzz 531.5
test_powerloss_file_pl_fuzz 787.7 <--
test_badblocks_single_dir_many 840.2
test_powerloss_filedir_pl_fuzz 1366.7 <--
TOTAL 7378.6
But testing more things is better than testing the same thing more.
Worst case you can always manually override OPS, -DOPS=1024, if you have
CI cycles to spare. Though note with our linear powerloss heuristic,
the tail end of long running tests also recieves fewer powerlosses,
which reduces the usefulness of running these tests longer.
These *_pl_fuzz tests also now match the default number of OPS in
test_relocations.
These emulate powerloss behavior where only some of the bits being
progged are actually progged if there is a powerloss. This behavior was
the original motivation for our ecksums/fcrcs, so it's good to have this
tested.
As a simplification, these only test the extremes:
- LFS_EMUBD_POWERLOSS_SOMEBITS => one bit progged
- LFS_EMUBD_POWERLOSS_MOSTBITS => all-but-one bit progged
Also they flips bits instead of preserving exact partial prog behavior,
but this is allowed (progs can have any intermediate value), has the
same effect as partial progs, and should encourage failed progs.
This required a number of tweaks in emubd: moved powerloss before prog,
moved mutate after powerloss, etc, but these shouldn't affect other
powerloss behaviors. Handling powerloss after prog was only to avoid
power_cycles=1 being useless, it's not strictly required.
Good news is testing so far suggests our ecksum design is sound.
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%)
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%)
This acts as a marker to indicate a fuzz test. It should reference a
define, usually SEED, that can be randomized to get interesting test
permutations.
This is currently unused, but could lead to some interesting uses such
as time-based fuzz testing. It's also just useful for inspecting the
tests (make test-list).
We've been wasting a lot of test cycles thanks to REMOUNT. Using a test
define for this effectively duplicates the test, when we really just
want to run more post-test code without additional mutation.
The main reason for REMOUNT has been to save typing, which, well, is not
a bad reason, these tests involve a lot of typing...
But this is probably a hammer/nail situation. If we replace these with a
small post-test loop, we can save quite a bit of time:
make test -j before: 5791.9s
make test -j after: 5123.8s (-11.5%)
Some tests still use a REMOUNT define, but these should be limited to
cases where remount actually changes the test's behavior.
littlefs does this internally anyways. The original intention was to
make sure non-powers-of-2 don't break, but we don't really validate what
these end up aligned to. And the intentional mismatch risks confusion
when debugging.
If it's worth testing non-powers-of-2, it should be an explicit test.
These should already be tested elsewhere, these test cases were mostly
copied from other suites after all.
And these tests are expensive, so we really shouldn't be running
permutations that don't add anything.
Christopher Haster