So instead of looking for another bookmark, we check to see if the next
mid's did matches.
This is in theory more robust, and can handle neighboring
directories/files without bookmarks, but adds a bit of code cost.
Deduplicating these checks into lfsr_grm_pushdid helps reduce this a
bit:
code stack
before: 34406 2624
after-no-dedup: 34526 (+0.3%) 2624 (+0.0%)
after-dedup: 34470 (+0.2%) 2624 (+0.0%)
lfsr_mtree_seek is a bit of an odd function, a hammer for too many
nails.
Using lfsr_mtree_lookup directly with manual mdir.mid manipulation gives
the internal layers more flexibility and room for optimizations.
Code changes:
code stack
before: 34426 2624
after: 34406 (-0.1%) 2624 (+0.0%)
This makes mtree implicit in most of littlefs's core functions, which
simplifies things. It also makes lfsr_mtree_traverse naming consistent
with other mtree-esque operation.
Renames:
- Renamed lfsr_fs_weight -> lfsr_mtree_weight (implicit mtree)
- Renamed lfsr_mtree_weight -> lfsr_mtree_weight_ (explicit mtree)
- Renamed lfsr_fs_traverse* -> lfsr_mtree_traverse*
- Renamed LFSR_TSTATE_* -> LFSR_MTRAVERSAL_*
Implicit mtree functions, note these are pretty much the backbone of
littlefs:
- lfsr_mtree_weight
- lfsr_mtree_lookup
- lfsr_mtree_seek
- lfsr_mtree_namelookup
- lfsr_mtree_pathlookup
- lfsr_mtree_traverse
This makes the naming is a bit inconsistent with lfsr_btree_*,
lfsr_rbyd_*, etc, but sometimes rules needs to bend a bit.
Besides, most of these functions needed access to the mroot anyways, so
it's not like they were really ever able to operate on independent
mtrees correctly.
And you can't complain about the code savings:
code stack
before: 34562 2624
after: 34426 (-0.4%) 2624 (+0.0%)
This, in theory, deduplicates this bounds check, and matches
lfsr_btree_lookup/lfsr_rbyd_lookup in behavior.
There's was only one non-asserting bounds check that could actually be
"deduplicated" (ignoring lfsr_mtree_seek for now), so this just resulted
in compiler noise:
code stack
before: 34558 2624
after: 34562 (+0.0%) 2624 (+0.0%)
So now lfsr_traversal_read will only return LFS_ERR_BUSY if LFS_T_EXCL
was provided to lfsr_traversal_open.
This means it's no longer possible to opportunistically traverse blocks,
_and_ detect mutation in the same traversal (though I suppose you could
open multiple traversals for this?), but on the flipside this
potentially frees up the implementation a bit.
This motivation for this is that LFS_ERR_BUSY is potentially confusing
and annoying to handle if you don't care about mutation.
code stack
before: 34566 2624
after: 34558 (-0.0%) 2624 (+0.0%)
blocks={0,0} technically worked, but only because the only mdirs allowed
at block 0 are mroot blocks. In theory, an mdir at block 0 could match
blocks={0,0} and clobber incorrectly, but it's not possible for such an
mdir to be in a non-trivial mtree, since blocks={0,1} are reserved for
the mrootanchor.
But bleh, that's complicated.
Setting blocks={-1,-1} makes the mdir truely invalid/unmatchable and
provides a stronger invariant at a minor code cost.
Code changes:
code stack
before: 34554 2624
after: 34566 (+0.0%) 2624 (+0.0%)
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.
We redraw the output when we recieve a new line from the child process.
Which means if we never recieve a new line, nothing gets drawn.
Fixed by adding an extra case after the child process finishes.
code.py, specifically, was getting messed up by inconsequential GCC
objdump errors on Clang -g3 generated binaries.
Now stderr from child processes is just redirected to /dev/null when
-v/--verbose is not provided.
If we actually depended on redirecting stderr->stdout these scripts
would have been broken when -v/--verbose was provided anyways. Not
really sure what the original code was trying to do...
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...
Inspired by errno's/dbgerr.py's -l/--list, this gives a quick and easy
list of the current tag encodings, which can be very useful:
$ ./scripts/dbgtag.py -l
LFSR_TAG_NULL 0x0000 v--- ---- ---- ----
LFSR_TAG_CONFIG 0x00tt v--- ---- -ttt tttt
LFSR_TAG_MAGIC 0x0003 v--- ---- ---- --11
LFSR_TAG_VERSION 0x0004 v--- ---- ---- -1--
... snip ...
We already need to keep dbgtag.py in-sync or risk a bad debugging
experience, so we might as well let it tell us all the information it
currently knows.
Also yay for self-inspecting code, I don't know if it's bad that I'm
becoming a fan of parsing information out of comments...
This is based on moreutils's errno program, but specialized for
littlefs's error codes. Everything is hardcoded in dbgerr.py, so this
should be portable to any OS really.
Hopefully this will be useful for debugging littlefs errors:
$ ./scripts/dbgerr.py -84
LFS_ERR_CORRUPT -84 Corrupted
The idea behind LFS_ERR_UNKNOWN is to reserve -1 as a general purpose
"Idunno what error code to use just bail" error. Discouraged for
production code, but very useful for hacking things together either for
quick prototypes or when trying to bootstrap a system.
-1 was actually already carved out for this purpose, but not documented.
I've used it heavily in other project, and it would be good to actually
codify this so others know the option is available.
This should end up being one of the allowed bd error codes, along with
LFS_ERR_IO, LFS_ERR_CORRUPT, etc.
---
If you try to map this to Linux errno codes, this does unfortunately
conflict with EPERM (-1), but what is EPERM other than an arbitrary "you
can't do that" error? Outside of Linux/POSIX, I think you can probably
get away with using EACCES (-13) for permission related errors instead.
I think the tradeoff is well worth it given we're not _really_
constrained to exact POSIX semantics.
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%)
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%)
Christopher Haster