I think this describes the goal of the non-perturbed rbyd checksums
decently. At the very least it's less wrong that "data checksum", and
calling it the "metadata checksum" would just be confusing. (Would our
commit checksum be the "metametadata checksum" then?)
It's expected for our crystal boundary calculation to underflow, but
when checking for holes we were using the wrong signed/unsigned
comparison, so lfsr_file_carve thought there was a hole when there
wasn't:
-crs pos pos -crs
.-------| <-- this lookup ------| .--
'---. | +crs --. | +crs '--
. |---|---. ended up |---|---. .
. v v v looking --> v v v .
. .---. like this .---. .
. |dat| |dat| .
. '---' '---' .
. 0 . n 0 . n .
'---.---' '---.---'
no hole clearly a hole
This led to unoptimal block compaction and weird block alignment for
even relatively simple files.
The crystallization threshold is only a heuristic so this didn't exactly
break anything, but it was causing block-aligned files to waste a bit of
of space which wasn't great.
---
To hopefully protect against this in the future, I've added a couple
*_litmus tests to check that at least some simple block-aligned files
end up with the correct number of branches/blocks. This should at least
give us some confidence our crystallization algorithm is working as
intended.
We don't have all that many tests (any?) over the exact topology of
files, mainly because of how many heuristics are involved. Maybe we
should look into adding a couple more.
No code changes:
code stack
before: 36396 2664
after: 36396 (+0.0%) 2664 (+0.0%)
It was a bit tricky to figure out what this should look like.
Traditionally, filesystems tend to fallback to readonly if they detect
unsupported wcompat (ro_compat) flags or similar config mismatch.
We could do something similar in littlefs, but since we default to
asserting on writes to readonly objects for smaller code size, this
would be really weird and hard to use from a users perspective...
Instead, lfsr_mount returns LFS_ERR_NOTSUP on encountering wcompat-
mismatch in RDWR mode, but _not_ RDONLY mode. This allows the common
rdonly-fallback pattern to be implemented on the user's side of things,
similar to the common format-fallback pattern:
int err = lfsr_mount(&lfs, LFS_M_RDWR, &cfg);
if (err && err != LFS_ERR_NOTSUP) {
return err;
}
if (err == LFS_ERR_NOTSUP) {
err = lfsr_mount(&lfs, LFS_M_RDONLY, &cfg);
if (err) {
return err;
}
}
Note that lfsr_mount may still return LFS_ERR_NOTSUP if it encounters
rcompat-flags, even with RDONLY. Detecting this state will likely need
two lfsr_mount calls with the current API, but I don't think that will
be a big deal.
The main benefit of this scheme is that it is quite cheap thanks to
pushing the fallback logic on the user:
code stack
before: 36356 2664
after: 36396 (+0.1%) 2664 (+0.0%)
One missing puzzle piece here is how do you upgrade the filesystem? But I
think the lesson from the on-disk v2.0 -> v2.1 version bump is that this
should really be an explicit function (lfsr_fs_upgrade?). If explicit
and stand-alone, like lfsr_format, we shouldn't need a weird pseudo-
rdonly mode at all.
These simply imply LFS_O_FLUSH/SYNC on all open writable files.
LFS_M_SYNC is equivalent to MS_SYNCHRONOUS in Linux/etc, while
LFS_M_FLUSH is just provided for consistency.
As pure conveniences, these may seem a bit out of scope for littlefs,
except they are _very_ cheap:
code stack
before: 36356 2664
after: 36356 (+0.0%) 2664 (+0.0%)
Ok, they're not _completely_ free! It just turns out they cost 8 bytes,
and a bit of simplification around flag checking in lfsr_mount saved
8 bytes:
code stack
before: 36356 2664
m_flush/sync: 36364 (+0.0%) 2664 (+0.0%)
mount-no-mask: 36356 (+0.0%) 2664 (+0.0%)
These flags just call lfsr_file_ckmeta/ckdata under the hood, but make
it very easy to check metadata/data when opening a file. As an extra
plus they implicitly close the file on failure, so might make cleanup
easier.
Of course, everything has a cost:
code stack
before: 36368 2664
after: 36424 (+0.2%) 2664 (+0.0%)
These also ruin my previous "you don't pay for what you don't call"
assertion, since runtime flags unfortunately always pull in code.
We should add a compile-time switch for these evntually.
These are basically the same as lfsr_fs_ckmeta/ckdata but limited to a
single file. They may be useful when you need to validate a file but
don't want to bother validating the entire filesystem:
// Check a file for metadata errors
int lfsr_file_ckmeta(lfs_t *lfs, lfsr_file_t *file);
// Check a file for metadata + data errors
int lfsr_file_ckdata(lfs_t *lfs, lfsr_file_t *file);
I've also added test_ck to test these and added some more
lfsr_fs_ckmeta/ckdata tests there. These currently just test simple
full-block clobbering, but we should eventually test more interesting
error patterns.
Unfortunately lfsr_file_ckmeta/ckdata can't reuse the internal
lfsr_mtree_traverse in quite the same way lfsr_fs_ckmeta/ckdata can, so
they're actually a bit more expensive. Though keep in mind with
link-time gc you won't pay the cost unless you call these functions:
code stack
before: 36024 2696
after: 36368 (+1.0%) 2664 (-1.2%)
Oh, and the multiple calls to lfsr_btree/bshrub_traverse apparently
uninlined it out of lfsr_mtree_traverse, saving the stack cost in the
stack hot-path... Yay?
It's really not that much code (36 bytes, and only if you call
lfsr_fs_gc), and implicit state is better the explicit state (less
things that can fall out of sync).
I'm keeping the fancy F/GC flag masking in lfsr_fs_gc though.
Code changes:
code stack
before: 35988 2696
after: 36024 (+0.1%) 2696 (+0.0%)
This is equivalent to the user-facing LFS_I_CANLOOKAHEAD flag, but
explicitly set in lfs_alloc/lfs_alloc_markfree, rather than being
implied.
Usually, I prefer implicit state, as this means less things that can
fall out-of-sync if there is a filesystem bug, but for
LFS_F_CANLOOKAHEAD explicit state might be warranted.
The main benefit is we can take advantage of the matching F/GC bit
patterns to simplify lfsr_fs_gc's progress checks.
This ends up saving a bit of code:
code stack
before: 36048 2696
after: 35988 (-0.2%) 2696 (+0.0%)
If we add CKMETA/CKDATA and eventually REPAIRMETA/REPAIRDATA to the file
open flags, we'll end up with 17 flags total (13 user-facing,
4 internal), which is a bit (heh) too much for a 16-bit flags field!
There are a few ways to solve this, dropping features for one, instead
I've decided to expand the fields flag to 32-bits. Fortunately this was
already the field size for all user-facing fields.
To avoid a RAM increase, I've also shoved the opened-file types and
traversal tstates into the same field.
We have various flags in quite a few places now, here's how
everything fits together:
8 8 8 8
.----++----++----++----.
.----..---..--..-------.
o_flags: |type|| f ||t || o |
|----||---|:--:'-------'
|----||---|:--:--------.
d_flags: |type|| f |: : |
|----||---|:--:--------'
|----||---|:--'--..----.
t_flags: |type|| f || t ||tstt|
'----''---'|-----|'----'
.----------|-----|-----.
gc_flags: | | t | |
'----------|-----|-----'
.-----.---.|-----|.----.
m_flags: | | m || t || m |
'-----|---|'-----'|----|
.----.|---|-------|----|
i_flags: | i || m | | m |
'----''---'-------'----'
Unfortunately, using the full 32-bit flag space highlights that C99's
enum types are kind of garbage...
In C99 enums are strictly signed ints, which means attempting to use
them for 32-bit bit fields overflows. There is no way around this so
I've switched our flag definitions to #defines.
I've kept types as enums for now but I'm keeping my eye on them...
---
The tradeoff of merging the type/btype/tstate/flags fields is that it
takes more code to extract/encode the various subfields. Since these
fields our heavily used in our codebase, this really adds up:
code stack
before: 35888 2696
after: 36048 (+0.4%) 2696 (+0.0%)
At least in theory the type fields can be optimized to a byte load, but
not btype/tstate. Also accessing bits in higher positions may be adding
cost.
After thinking about this for a while, btree node compaction is
subtlety different from mdir compaction, less valuable, and adds more
risk:
- Unlike mdirs, btree node compaction will always allocate a new
block, leading to a higher chance of alloc failure.
- Btree node compaction also always requires additional writes to
propagate btree changes, whereas mdir compaction is usually
self-contained unless it triggers a relocation. If btree nodes are
mostly full this risks being counter-productive.
- Btree node compaction requires a full tree traversal, whereas mdir
compaction requires only traversing the mtree. Though you can always
force mtree-only traversal manually with LFS_GC_MTREEONLY.
- Btrees/bshrubs are also more likely to be "cold storage", that is it
probably won't be uncommon to create long-lived read-only btrees as a
part of files. Compacting these btrees can actually be counter-
productive as it can encourage splitting.
- Btrees/bshrubs are also more likely to be one use, and discarded as a
file is truncated and rewritten. Compacting btree nodes in this case
is a waste of erase cycles.
And since btree node compaction also introduces a lot of complexity/risk
of bugs, I'm going to drop this for now and limit LFS_GC_COMPACT to only
compacting mdirs. At least this tested implementation will live in the
history and can always be reintroduced in the future if it becomes a
wanted feature.
---
As is usually the case, doing less work ends up with less code:
code stack
before: 36292 2704
after: 35888 (-1.1%) 2696 (-0.3%)
Note this still keeps the rbyd-specific commit logic necessary for
committing to specific btree nodes, even though btree node compaction
was the only current use case. This should eventually be useful for
metadata repair. Hopefully const-propagation can minimize the cost, but
realistically this means we're probably leaving some code savings on the
table.
These should never need to mutate the filesystem, so calling
lfsr_mtree_gc doesn't really make sense. This mainly matters for
link-time gc in case we never need lfsr_mtree_gc (readonly mode?).
Unfortunately this adds a code cost because the optional pointers to
lfsr_mtree_traverse can no longer be const-propagated:
code stack
before: 36284 2704
after: 36292 (+0.0%) 2704 (+0.0%)
Nothing consequential.
One interesting question is if we should swap our dirty bits during the
call to lfsr_mtree_traverse. At the moment I think limiting this to just
our lfsr_mtree_gc logic will create the least surprise in the future.
Code changes minimal:
code stack
before: 36288 2704
after: 36284 (-0.0%) 2704 (+0.0%)
It was a bit weird to have this in lfsr_mtree_traverse, which doesn't
change any filesystem state otherwise.
At the very least we should call lfs_alloc_markinuse and
lfs_alloc_markfree in the same function, and rerouting
lfsr_mtree_traverse eot to handle this would have added code cost
anyways.
The main cost is stack:
code stack
before: 36256 2680
after: 36288 (+0.1%) 2704 (+0.9%)
Unfortunately this reveals one of the bigger issues with our optional
return parameters: if a function with optional return parameters needs
the structs to perform work, in this case lfsr_mtree_traverse needs
lfsr_bptr_t in case ckmeta/ckdata is requested, it requires an
additional stack allocation.
In theory, these stack allocations could be elided if the return structs
are provided, but you can't really express this in standard C.
Combine this with the fact that lfs_alloc is sensitive to stack changes,
and lives at the bottom at every hot-path, and the end result is more
stack usage.
Note that the additional stack cost, 24 bytes, is exactly equal to one
tag + one bptr, 4 bytes + 20 bytes.
Two main reasons:
1. If we mount without ckprogs, we do actually have a pretty decent hole
here where data can be written with errors and go unchecked during
lfsr_fs_gc.
2. If we're traversing a btree that gets mutated mid-traversal, we're
kicked entirely off the btree. This means we could miss large ranges
of btree nodes/data blocks that may not have themselves been mutated.
Not great.
Worst case, it doesn't hurt to check things again if the filesystem
changes. If this is too much of a bottleneck, you should probably be
running gc in incremental mode anyways, which always starts a new
traversal on ckmeta/ckdata.
Checking for dirty/mutated doesn't really add that much code:
code stack
before: 36240 2680
after: 36256 (+0.0%) 2680 (+0.0%)
There is really no reason to continue lookahead traversals if our
filesystem has been mutated. Clearing the flag and restarting in this
case is more likely to make progress.
Note that it's worth continuing for all of the other current gc flags:
- LFS_GC_MKCONSISTENT - Except maybe for mkconsistent. We can't actually
make progress, since we can't prove the filesystem is free of orphans,
but it's beneficial to keep traversing and clearing orphans in case of
other traversal flags that mutation would force a second traversal
anyways.
Continuing mkconsistent traversals also spreads out orphan cleanup a
bit better, instead of just repeatedly cleaning up the first couple
mdirs when under heavy contention.
But to be honest, the chance of mutation that still leaves the
filesystem with orphans is just so low that it's not worth doing
anything. mkconsistent only needs to traverse the mtree anyways...
- LFS_GC_COMPACT - Like mkconsistent, compacting traversals are worth
continuing for better mtree coverage under heavy contention.
We will need a second pass to prove we compacted everything anyways,
so might as well try to get as much mutation done as possible in the
current traversal.
- LFS_GC_CKMETA/CKDATA - Continuing ckmeta/ckdata traversals provides
better mtree coverage under heavy contention.
This is much more important for CKMETA/CKDATA than the others, because
_eventually_ checking every block for errors is more valuable than
proving anything.
This adds some code, but the use of flags here is quite valuable for
expressing complex constraints like this cheaply:
code stack
before: 36228 2680
after: 36240 (+0.0%) 2680 (+0.0%)
Now we consider if it's still possible for the current traversal to make
progress. If it can, we continue with the relevant masked flags,
otherwise we restart. This should prevent us from traversing the
filesystem for no reason.
I also reverted the ckedmeta/ckeddata flags, these ended up just adding
code cost. We're not in the stack hot-path anyways...
Code changes:
code stack
before: 36244 2680
after: 36228 (-0.0%) 2680 (+0.0%)
Also moved ckmeta/ckdata progress into lfs->flags. We have the bits
available so we might as well use them instead of allocating bools on
the stack...
Whether or not to consider ckmeta/ckdata successful when the filesystem
has been mutated is a bit nuanced.
Initially, I thought we trigger a re-traversal, since we may have
introduced new blocks that haven't been checked. But think about it,
where did those blocks come from?
Any new blocks introduced by filesystem mutation will have just been
written. And if a write introduces corruption you probably have bigger
problems...
... Actually as I write this I realized mounting without ckprogs makes
this even more nuanced, but since ckmeta/ckdata is more intended for
data-at-rest error detection I'm going to keep the change for now.
If you want to catch write errors, you really should enable ckprogs.
This is only a problem for lfsr_fs_gc, and the use cases for
ckmeta/ckdata in lfsr_fs_gc will probably catch any write errors on the
next cycle anyways...
Code changes:
code stack
before: 36208 2680
after: 36244 (+0.1%) 2680 (+0.0%)
It makes the the are-any-files-still-open assert a bit uglier, but we
really don't need to bother calling lfsr_omdir_close here. We're done
with this struct anyways...
Not doing something saves a bit of code:
code stack
before: 36240 2680
after: 36208 (-0.1%) 2680 (+0.0%)
Thinking about this more, we probably don't want to entangle
lfsr_fs_mkconsistent/ckmeta/etc and lfsr_fs_gc:
- lfsr_fs_ckmeta/ckdata are readonly and don't need to clobber
traversals. The system can make more progress if these use separate
states.
- We already need a bit of code to force traversals to restart for
lfsr_fs_ckmeta/ckdata, so these already aren't simple wrappers.
- lfsr_fs_mkconsistent should also probably not invalidate gc traversals
when the filesystem is already consistent. It is called by... checks
notes... every function that writes to disk.
This could be fixed in lfsr_fs_mkconsistent, but it'd be pretty close
to just calling lfsr_mtree_gc...
- We don't really benefit from reusing the gc traversal state.
lfsr_fs_mkconsistent/ckmeta/etc aren't on the stack hot-path, so the
stack usage is more-or-less free (though I realize this depends on
what functions are called in a given system).
- Calling lfsr_fs_gc can actually be a detriment for code size when
considering link-time-gc (not related to fs-gc), since it will drag in
the function when we don't need the traversal-invalidation features.
- Calling lfsr_fs_gc vs lfsr_mtree_gc shouldn't really be a significant
code size difference. We should probably look into lfsr_mtree_gc,
which is called from many places, instead of tangling everything
together...
So this commit reverts gc-restarts and brings back gc masking on flag
change.
At the very least, moving all the code around led to a bit of code
savings:
code stack
before gc-restart: 36316 2680
gc-restart: 36068 (-0.7%) 2680 (+0.0%)
after gc-restart: 36240 (-0.2%) 2680 (+0.0%)
This simplifies lfsr_fs_gc a bit, and allows lfsr_fs_mkconsistent/
ckmeta/etc to call lfsr_fs_gc directly (it would be a bit strange for
these function to finish up unrelated gc traversals).
Unfortunately, this does risk gc getting stuck constantly restarting if
there is contention between two lfsr_fs_gc calls with different flags,
but you could argue this would be a system design mistake...
The deduplication of traversal state leads to some pretty nice code
savings:
code stack
before: 36316 2680
after: 36068 (-0.7%) 2680 (+0.0%)
So instead of configuring gc_steps at mount time (or eventually compile
time), lfsr_fs_gc now takes a steps parameter that controls how much gc
work to attempt:
int lfsr_fs_gc(lfs_t *lfs, lfs_soff_t steps, uint32_t flags);
This API was needed internally to better deduplicate on-mount gc, and I
figured it might also be useful for users to be able to easily change
gc_steps per lfsr_fs_gc call.
I realize this could also be accomplished with the theoretical
lfsr_fs_gccfg, but it's a bit easier to not need a struct every call.
Most likely, depending on project/system, users will always call
lfsr_fs_gc with either 1 (minimal work) or -1 (maximal work), or, worst
case, can define a system-wide GC_STEPS somewhere.
---
Deduplicating on-mount gc work better saved some code, though it's worth
noting this could have been done internally and not exposed to users:
code stack
before: 36476 2680 (+0.0%)
after: 36316 (-0.4%) 2680 (+0.0%)
Matching lfsr_mdir_commit, it's probably safer if lfs_alloc_ckpoint
calls are always explicit.
This is doubly true for traversals since we absolutely must not call
lfs_alloc_ckpoint in lfsr_mtree_traverse, or else lfs_alloc will break
in a really comical fashion.
It's also a bit silly how little an impact on code size this had:
before: 36472 2680
after: 36476 (-0.0%) 2680 (+0.0%)
This is mainly to avoid confusion around if lfs_alloc_ckpoint needs to
be called before this function.
I guess it's not problematic to call lfs_alloc_ckpoint unnecessarily...
but unlike every other lfsr_mdir_* function we call lfs_alloc_ckpoint
internally for every orphan we fix. Otherwise we could end up with
ENOSPC too early.
lfsr_fs_fixorphans_ is really only internal glue between
lfsr_fs_fixorphans and lfsr_mtree_gc anyways...
I thought it was a bit funny we test various disk sizes in test_grow,
but no where else! test_grow actually found several bugs when reworking
the lookahead buffer related to small disks, so I figured we should have
some more intentional tests... And behold! A bug!
The issue is that we implicitly call lfs_alloc_ckpoint in
lfsr_mdir_commit. Originally the thinking was that this would be fine
since any in-flight blocks should be committed to a tracked btree/bshrub
first, but lfsr_bshrub_commit goes _through_ lfsr_mdir_commit. Bit of a
problem.
So if we call lfsr_bshrub_commit to add a recently allocated block, it
may end up calling lfsr_mdir_commit, erronously ckpointing the
allocator, and then clobbering the new block if the mdir needs to be
relocated, split, etc.
---
The fix here is to just move lfs_alloc_ckpoint out of lfsr_mdir_commit.
This adds a bit of noise, but it's probably a good thing for alloc
ckpoints to be explicit.
At least lfs_alloc_ckpoint is cheap:
code stack
before: 36412 2680
after: 36472 (+0.2%) 2680 (+0.0%)
The main reason for this change is to allow keeping track of existing
known-free blocks while trying to find more free blocks. This makes it
so failed filesystem traversals don't result in negative progress, which
is nice.
This was difficult in the previous lookahead scheme, since we we'd need
to shift the lookahead buffer to keep off=0 rooted at the first bit.
Shifting bytes is relatively easily with memmove, but it gets tricky
when shifting bits:
lookahead before: ???? ???? ???? ??00 1101 0101 00?? ????
^ ^
off off+size
shift: 0011 0101 0100 ???? ???? ???? ???? ????
^ ^
off off+size
traverse: 0011 0101 0100 0000 0000 0000 1100 0000
^ ^
off off+size
Instead, we now just let the lookahead buffer wrap around. No shifting
required:
lookahead before: ???? ???? ???? ??00 1101 0101 00?? ????
^ ^
off off+size
traverse: 0000 0000 1100 0000 1101 0101 0000 0000
^
off
^
off+size
This gets a bit confusing with the lookahead window also wrapping around
disk, but the math works out with enough modulos (if modulos are too
expensive, we should eventually be able to optimize these into simple
bit masks via compile-time config).
In the future, if we move away from the const config struct, it would
also be nice to try to reducing the number of modulos by storing the
lookahead buffer size in bits instead of bytes...
Note that if the lookahead buffer is larger than disk, the lookahead
window will sort of travel around the underlying buffer. This isn't
inherently a problem, but it did cause some bugs.
To avoid similar bit-related problems with zeroing, lfs_alloc_inc now
also zeros bits as we allocate/skip them, so bits should always be zero
when we start a lookahead traversal. Though note we still need to
manually memset the buffer when discarding lookahead state in init/grow.
---
The end result is surprisingly a net savings in terms of code size. I
guess mainly due to dropping all the lfs_alloc_shift calls:
code stack
before: 36472 2680
after: 36412 (-0.2%) 2680 (+0.0%)
These functions provide an easy API for checking all metadata/data
checksums in the filesystem:
// Check the filesystem for metadata errors
int lfsr_fs_ckmeta(lfs_t *lfs);
// Check the filesystem for metadata + data errors
int lfsr_fs_ckdata(lfs_t *lfs);
These are more-or-less the same as calling lfsr_fs_gc with
LFS_GC_CKMETA/CKDATA, but don't involve the gc/traversal-invalidation
machinery, and may be a bit easier for users to pick up.
---
Unfortunately, for simple wrappers, we're again hit with a somewhat
surprising code cost:
code stack
before: 36288 2680
after: 36472 (+0.5%) 2680 (+0.0%)
But I think we can again blame the high overhead of LFS_TRAVERSAL/
lfsr_mtree_gc. We should look into reducing/deduplicating this logic...
Whoops! Turns out it's easy to forget to checkpoint the allocator when
most standalone operations involve lfsr_mdir_commit which checkpoints
the allocator automatically...
This is also the only case where we are doing btree modifications
outside of either file operations or mtree updates.
Easy fix, code changes minimal:
code stack
before: 36280 2680
after: 36288 (+0.0%) 2680 (+0.0%)
These tell littlefs to do the relevant gc work during mount, which may
be more convenient than calling lfsr_mount and then lfsr_fs_gc.
It also implicitly tears down the filesystem on error, which you can
imagine would be quite useful for LFS_M_CKMETA/LFS_M_CKDATA.
Some flags are more useful here than other (is LFS_M_LOOKAHEAD/COMPACT
really useful?), but since we just pass these directly to our traversal
APIs, we might as well support all of them for consistency.
Also note that since these only change mount's behavior, and have no
effect on the rest of the filesystem, these LFS_M_* flags don't have
related LFS_I_* flags and are not returned by lfsr_fs_stat.
---
This added quite a chunk of code, considering that this is entirely for
convenience:
code stack
before: 35932 2680
after: 36280 (+1.0%) 2680 (+0.0%)
But I think this is mostly because our low-level traversal state is
relatively costly to manage. It may be possible to deduplicate this a
bit better...
This has been a long-time coming, mount flags are just too useful for
configuring a filesystem at runtime.
Currently this is limited to LFS_M_RDONLY and LFS_M_CKPROGS, but there
are a few more planned in the future:
LFS_M_RDWR = 0x0000, // Mount the filesystem as read and write
LFS_M_RDONLY = 0x0001, // Mount the filesystem as readonly
LFS_M_STRICT* = 0x0002, // Error if on-disk config does not match
LFS_M_FORCE* = 0x0004, // Ignore compat flags, mount readonly
LFS_M_FORCEWITHRECKLESSABANDON*
= 0x0008, // Ignore compat flags, mount read write
LFS_M_CKPROGS = 0x0010, // Check progs by reading back progged data
LFS_M_CKREADS* = 0x0020, // Check reads via checksums
* Hypothetical
As a convenience, we also return mount flags in the struct lfs_fsinfo's
flags field as their relevant LFS_I_* variants. Though only to match
statvfs, and only because it's cheap, littlefs's API is low-level and we
should expect users to know what flags they passed to lfsr_mount.
As for the new mount flags:
- LFS_M_RDONLY - For consistency with existing APIs, this just asserts
on write operations, which makes it a bit useless... But the info flag
LFS_I_RDONLY may be useful for falling back to a readonly mode if
we encounter on-disk compat issues.
At least if implement the theoretical LFS_UNTRUSTED_USER mode
LFS_M_RDONLY could become a runtime error.
- LFS_M_RDWR - This really just exists to compliment LFS_M_RDONLY and to
match LFS_O_RDONLY/LFS_O_RDWR. It's just an alias for 0, and I don't
think there will ever be a reason to make it non-0 (but I can always
be wrong!).
- LFS_M_CKPROGS - This replaces the check_progs config option and avoids
using a full byte to store a bool.
We should probably also have a compile-time option to compile this out
(LFS_NO_CKPROGS?), but that's a future thing to do.
This ended up adding a surprising bit of code, considering we're just
moving flags around, and noise in lfs_alloc added a bit of stack again:
code stack
before: 35880 2672
after: 35932 (+0.1%) 2680 (+0.3%)
Thinking again of use cases, lfsr_fs_gc provides the perfect API to call
in the background to perform any pending filesystem work. But what if
there's no work to be done? Sure we could just spin forever, but that's
a waste. Especially on devices that can turn on sleep modes to save
power.
To help with this, this commit adds a set of flags to struct lfs_fsinfo
that signals when lfsr_fs_gc can accomplish work:
LFS_I_INCONSISTENT = 0x01, // Filesystem needs mkconsistent to write
LFS_I_NEEDSUPGRADE* = 0x02, // Filesystem needs an upgrade to write
LFS_I_CANLOOKAHEAD = 0x04, // Lookahead buffer is not full
LFS_I_CANPREERASE+ = 0x08, // Pre-erase buffer is not full
LFS_I_UNCOMPACTED = 0x10, // Filesystem may have uncompacted metadata
LFS_I_NEEDSREPAIRMETA+ = 0x20, // Filesystem contains damaged metadata
LFS_I_NEEDSREPAIRDATA+ = 0x40, // Filesystem contains damaged data
*Hypothetical
+Planned
This flags field also provides a useful place internally to store other
filesystem-related flags, currently LFS_F_ORPHANS, though this may be
expanded in the future.
These flags allow users to know exactly what work can/needs to be done
for the filesystem to make progress:
- LFS_I_INCONSISTENT => LFS_GC_MKCONSISTENT or lfsr_fs_mkconsistent
- LFS_I_CANLOOKAHEAD => LFS_GC_LOOKAHEAD
- LFS_I_UNCOMPACTED => LFS_GC_COMPACT
The one is new!
If we complete a compaction-traversal without any mutation, we know
all mdirs/btree nodes have been compacted and future traversals won't
accomplish anything. Of course, we need to clear this bit on
filesystem mutation.
Right now we just pessimistically assume the filesystem is uncompacted
during mount, but in theory we can also figure this out during our
initial mount traversal.
- LFS_GC_CKMETA/CKDATA?
LFS_GC_CKMETA and LFS_GC_CKDATA are a bit trickier. In theory,
LFS_GC_CKMETA/CKDATA will always accomplish something, since time is
the only ingredient necessary to introduce bit errors.
So there isn't really a reasonable flag here. It's entirely up to the
user to decide when to do an LFS_GC_CKMETA/CKDATA traversal.
Code changes:
code stack
before: 35740 2672
after: 35880 (+0.4%) 2672 (+0.0%)
LFS_GC_MTREEONLY is a rather niche/littlefs-specific flag, and we
probably shouldn't expect users to know when to use it. So now we
automatically switch to LFS_GC_MTREEONLY mode in lfsr_fs_gc if it is
sufficient for accomplishing all pending gc work.
Though currently the only traversal that can be LFS_GC_MTREEONLY is
LFS_GC_MKCONSISTENT...
Note that LFS_GC_MTREEONLY can still be explicitly provided, as it does
change the behavior of LFS_GC_COMPACT and LFS_GC_CKMETA (and combining
LFS_GC_MTREEONLY with LFS_GC_LOOKAHEAD/LFS_GC_CKDATA still asserts).
This adds a bit of code:
code stack
before: 35728 2672
after: 35740 (+0.0%) 2672 (+0.0%)
This just deduplicates the post-traversal work (clearing orphan flags,
marking lookahead as free, etc) that every gc-esque function needs to do
on a succesful traversal, into the common lfsr_mtree_gc function.
This saves a bit of code:
code stack
before: 35756 2672
after: 35728 (-0.1%) 2672 (+0.0%)
Thinking about use case a bit, most lfsr_fs_gc will be to perform
background work, and can benefit from being incremental.
We already support incremental gc and all the mess associated with
traversal invalidation via the traversal API, so we might as well expose
this through lfsr_fs_gc.
The main downside is that we need to store an lfsr_traversal_t object
somewhere, which is not exactly a cheap struct. I was originally
considering limiting incremental gc to the traversal API for this
reason, but I think the value add of an incremental lfsr_fs_gc is too
compelling... Though we really should add a compile-time option
(LFS_NO_GC? LFS_NO_INCRGC?) to allow users to opt-out of this RAM cost
if they're never going to call this function.
Oh, and lfs_t also becomes self-referential, which might become a
problem for higher-level language users...
---
The incremental behavior of lfsr_fs_gc can be controlled by the new
gc_steps config option. This allows more than one step to be performed
at a time, which may allow for more progress when intermixed with
write-heavy filesystem operations. Setting gc_steps=-1 performs a full
traversal every call, which guarantees always making some amount of
progress.
This adds a bit of code, since we now need to check for/resume existing
traversals. But the real cost is the added RAM to lfs_t, which is
unfortunately wasted if you never call lfsr_fs_gc:
code stack lfs_t
before: 35708 2672 164
after: 35756 (+0.1%) 2672 (+0.0%) 296 (+80.5%)
This just provides a simple, easy-to-call, wrapper over the new
traversal API:
int lfsr_fs_gc(lfs_t *lfs, uint32_t flags);
The main difference from its previous incarnation, is that lfsr_fs_gc
now takes a flags argument to indicate exactly what gc operations to
perform. This gives the user more control, and may also make the API
more robust towards adding new features:
LFS_GC_MTREEONLY = 0x0010, // Only traverse the mtree
LFS_GC_MKCONSISTENT = 0x0020, // Make the filesystem consistent
LFS_GC_LOOKAHEAD = 0x0040, // Populate lookahead buffer
LFS_GC_COMPACT = 0x0080, // Compact metadata logs
LFS_GC_CKMETA = 0x0100, // Check metadata checksums
LFS_GC_CKDATA = 0x0200, // Check metadata + data checksums
LFS_GC_REPAIRMETA+ = 0x0400, // Repair metadata blocks
LFS_GC_REPAIRDATA+ = 0x0800, // Repair metadata + data blocks
+ Planned
Alternatively, gc_flags could have been added as a config option. But
making gc_flags a function argument matches other flag APIs (open
mainly), and is slightly more flexible in that it allows a system to do
different gc operations in different system states (though this could
also be accomplished with the hypothetical lfsr_fs_gccfg, which would
probably be good to add anyways).
Worst case, defining a system-wide define that you always pass to
lfsr_fs_gc accomplishes roughly the same thing.
---
This adds a bit more code, mainly to check if we actually need to
traverse, and to make sure traversals accomplish all of the requested
work.
code stack
before: 35448 2680
after: 35708 (+0.7%) 2672 (-0.3%)
Curiously it also saved a bit of stack, which is a bit silly given this
commit is purely code addition. Apparently something in lfs_alloc and
lfsr_fs_gc is shared, getting uninlined, and messing with the stack
measurement. lfs_alloc is quite sensitive to stack changes after all.
- LFS_O_FLUSH 0x0040 -> 0x0040
- LFS_O_SYNC 0x00c0 -> 0x0080
- LFS_T_CKMETA 0x0100 -> 0x0100
- LFS_T_CKDATA 0x0300 -> 0x0200
This is just simpler and should avoid any surprises for both devs and
users.
This has no impact on code size:
code stack
before: 35448 2680
after: 35448 (+0.0%) 2680 (+0.0%)
The trick is realizing the dirty/mutated bits are redundant when
mutating. So instead of saving the current dirty bit on the stack, we
can just swap the mutated/dirty bits temporarily.
The bit swapping xor trick comes from Sean Eron Anderson's infamous bit
twiddling hacks collection, unfortunately it doesn't seem possible to
avoid the hardcoded bit locations...
Saves a bit of code:
code stack
before: 35504 2680
after: 35488 (-0.0%) 2680 (+0.0%)
The hope here was that deduplicating the lfsr_fs_fixorphans/
lfsr_mtree_gc mtree traversal would result in code savings since we'd
end up with one shared code path.
Unfortunately in practice this didn't work out:
code stack
before: 35484 2680
after: 35504 (+0.1%) 2680 (+0.0%)
Still, it is nice to have one shared code path, because that means fewer
corner cases that could break.
lfsr_mdir_fixorphans always ends up setting the zombie flag, which is a
bit annoying. If we don't clear the zombie flag, lfsr_remove/lfsr_rename
may cause repeated mids during traversal, which probably won't break
anything, but isn't great...
Code changes:
code stack
before: 35480 2680
after: 35484 (+0.0%) 2680 (+0.0%)
After thinking about this for a bit, there are some compelling
motivations for including an incremental LFS_T_MKCONSISTENT:
- Being able to run incremental LFS_T_MKCONSISTENT traversals in
parallel with read-only operations is actually quite enticing.
The only complicated part is maintaining the invalidatable traversal
state, which already exists with lfsr_traversal_t (except the
annoying LFS_F_MUTATED bit).
- While it's not really effective to combine LFS_T_MKCONSISTENT and
LFS_T_LOOKAHEAD traversals, it _is_ possible to combine
LFS_T_MKCONSISTENT with LFS_T_COMPACT, LFS_T_CKMETA,
LFS_T_REPAIRMETA (future), etc.
Really, LFS_T_LOOKAHEAD is the odd one out.
- Making LFS_T_MKCONSISTENT incremental means all filesystem-level
traversals (except lfsr_mount) can be run incrementally. Which is a
nice feature to have when O(n = entire fs) risks being very long
running.
The main downside of LFS_T_MKCONSISTENT (and LFS_T_COMPACT, etc) is that
attempting to run it immediately after mount will likely recursively
trigger a lookahead scan to satisfy block allocation requests -- which
will block the current thread for the duration of the lookahead scan.
But this seems to be more a problem of LFS_T_LOOKAHEAD interacting with
other traversals poorly.
Fortunately, long term, the current plan is to replace the lookahead
buffer with an on-disk block map on disks where the lookahead scan is a
bottleneck. If this gets implemented the problem goes away.
So re-reverting this for now. Worst case we can always re-re-revert this
again in the future. There is already a working implementation, so might
as well see where it goes...
Supporting incremental LFS_T_MKCONSISTENT does add a bit of a code
cost, but there is still some room for deduplicating lfsr_mtree_gc +
lfsr_fs_mkconsistent, which may be interesting:
code stack
before: 35232 2680
after: 35480 (+0.7%) 2680 (+0.0%)
Checking for orphans + other traversal work turned out to mesh much
worse than originally thought:
- Adjusting mids and being able to drop mdirs mid-traversal complicates
traversal quite a bit and has potential to hide difficult to reproduce
bugs.
- Implementing incremental mkconsistent requires it's own separate state
to detect mutation correctly since LFS_T_MKCONSISTENT and
LFS_T_LOOKAHEAD are invalidated by slightly different things.
- If hasorphans=true, we're likely going to find orphans and clobber the
traversal. So it's not really worth trying to opportunistically prove
there are no orphans while doing other traversal operations.
- We don't really want to traverse the mroot/mtree during mkconsistent,
which makes deduplicating these two functions a bit tricky. Doable,
but annoying.
- grms don't involve traversals and are their own separate awkward step
already.
Combine this with the fact that needing to scan for orphans should be
relatively rare in practice -- requiring either a powerloss or a
complicated set of file operations with at minimum 3 desynced files --
and parallel orphan checking starts to look like more trouble than it's
worth...
Instead, we now only check if the hasorphan bit has been set, and if it
has been we just call lfsr_fs_mkconsistent directly. This does a full
traversal in a single step, but at least makes it so traversal +
LFS_T_MKCONSISTENT in a background thread will do any necessary
janitorial work.
This saves a bit code:
code stack
before: 35480 2680
after: 35232 (-0.7%) 2680 (+0.0%)
Turns out things get a bit tricky when mdirs are dropped while iterating
over the mtree.
This was actually broken quite a bit before traversal-related changes,
probably during some mtree refactor, but went unnoticed since no test
actually checked that lfsr_fs_fixorphans did what it said it did.
At least the new test_forphans_cleanup* tests should prevent this from
regressing again in the future.
Code changes:
code stack
before: 35472 2680
after: 35480 (+0.0%) 2680 (+0.0%)
What seemed like a simple tweak to lfsr_fs_fixorphans, integration into
lfsr_mtree_gc, turned out to be surprisingly annoying.
- We need an additional traversal flag, LFS_F_MUTATED, in order to know
if we intentionally modified the filesystem. This is different from
LFS_F_DIRTY in that we don't invalidate orphan scans:
- LFS_F_DIRTY => invalidate lookahead + orphans
- LFS_F_MUTATED => invalidate lookahead
- We need to break up lfsr_fs_fixorphans to expose lfsr_mdir_fixorphans,
which is probably a good thing for readability.
The interactions with each mdir being associated with a given mid is
not great though, and requires a bit of awkward mid shuffling.
- Unlike LFS_T_COMPACT, LFS_T_MKCONSISTENT introduces more complicated
mid changes, and makes it so mdirs can now be dropped in the middle of
traversal.
This messes with our internal lfsr_mtree_traverse -> lfsr_mtree_gc
control flow, and means a single lfsr_traversal_read call may process
an unbounded number of blocks in rare cases with lots of orphans.
But the good news is things are working, and lfsr_traversal_read with
LFS_T_MKCONSISTENT can scan for orphans in parallel with other traversal
operations.
Adds a bit of code:
code stack
before: 35220 2680
after: 35472 (+0.7%) 2680 (+0.0%)
The tests highlighted that the LFS_I_DIRTY flag in lfsr_tinfo approach
is insufficient. Consider what happens if our filesystem is mutated
while traversing the last mdir:
1. Traversal traverses last mdir, populate blocks, return first block
2. Filesystem mutated, maybe mdir was compacted, clobbers traversal and
sets LFS_I_DIRTY
3. Traversal return LFS_ERR_NOENT immediately, last block never
returned (and out of date), LFS_I_DIRTY never returned
Not only do we miss the LFS_I_DIRTY flag, but we completely miss the
last block in the mdir pair without any warning.
This is _not_ a problem for the actual lookahead buffer, since we still
internally check the LFS_I_DIRTY flag before marking it as complete, but
it is an issue for any external logic that depends on the traversal
being complete...
---
We could revert to LFS_T_EXCL, but, to be honest, I just really don't
know a good name for this flag...
LFS_T_EXCL is a bad name because it conflicts with LFS_O_EXCL. These
flags have very different behaviors, which risks confusing users, and
risks potential name conflicts down the line if we ever want
LFS_T_EXCL-esque semantics for open dirs/files (not unreasonable, though
quite fancy).
My current best contender is LFS_T_WATCH, but while scratching my head
on this, I starting to wonder why we're even providing LFS_T_EXCL in the
first place...
We err on the side of forcing users to implement filesystem-external
features themselves when possible elsewhere, and LFS_T_EXCL technically
_can_ be implemented entirely outside of the filesystem. Though to be
fair it is quite annoying/tedious.
It's not like there's any equivalent feature for dir/file reads anyways.
And a background thread calling lfsr_traversal_read with LFS_T_LOOKAHEAD
will still _eventually_ make progress, even if it takes a bit longer.
Don't get me wrong, I understand it is significantly easier to implement
this inside the filesystem than outside. But it's also easier to
implement this later than right now. And if we implement this later,
hopefully we'll have a better idea what exactly will be useful for
users.
---
Removing LFS_T_EXCL/LFS_I_DIRTY has no real impact on code cost. We were
really just exposing internal logic that we need for lookahead
correctness anyways:
code stack
before: 35224 2680
after: 35220 (-0.0%) 2680 (+0.0%)
This just forwards the internal LFS_I_DIRTY flag to the user via the
lfsr_tinfo flags field.
Benefits of this approach:
- Gives the user more flexibility on what to do if the filesystem is
modified, maybe you want to keep traversing depending on some other
logic.
- Can eventually add other flags to tinfo.flags, such as
LFS_I_COMPACTED, LFS_I_REPAIRED, LFS_I_INCONSISTENT, etc.
- Avoids confusion around the very different behaviors of LFS_O_EXCL and
LFS_T_EXCL.
I tried to come up with a better name (maybe LFS_T_WATCH?) but it was
a bit of a struggle... Switching to a flags approach sidesteps the
issue.
- Can drop the LFS_ERR_BUSY error code for now.
Code changes were fairly insignificant:
code stack
before: 35244 2680
after: 35224 (-0.1%) 2680 (+0.0%)
The only concern is that the tests highlighted it's possible for our
flag scheme to miss mutation if it happens after/during the last set of
blocks... Not sure how to handle this yet...
LFS_O_SYNC always implies LFS_O_FLUSH, otherwise what exactly are you
syncing? Making this explicit in the bit pattern should hopefully make
this clear for curious users, though lfsr_file_flush would be called
anyways because of how lfsr_file_sync is implemented.
This also moves the LFS_O_DESYNC bit pattern around so SYNC/FLUSH are
neighbors. SYNC/DESYNC may seem related, but in lfsr_file_open they
actually are quite different:
LFS_O_FLUSH 0x0040 ---- ---- -1-- ----
LFS_O_SYNC 0x00c0 ---- ---- 11-- ----
LFS_O_DESYNC 0x0100 ---- ---1 ---- ----
Code changes, mostly just noise from moving bits around:
code stack
before: 35228 2680
after: 35244 (+0.0%) 2680 (+0.0%)
It still doesn't make sense to check data without checking metadata, but
keeping this named LFS_T_CKDATA should hopefully clarify what it does
differently from LFS_T_CKMETA.
This implication is also now encoded in the bit pattern:
LFS_T_CKMETA 0x0100 ---- ---1 ---- ----
LFS_T_CKDATA 0x0300 ---- --11 ---- ----
In theory a clever user could force only the CKDATA bit to be set, and
such a configuration would _probably_ work fine, but it won't be
supported just to cut down on possible configurations to test.
No code changes:
code stack
before: 35228 2680
after: 35228 (+0.0%) 2680 (+0.0%)
Christopher Haster