dbgerr.py and dbgtag.py have proven to be incredibly useful for quick
debugging/introspection, so I figured why not have more of that.
My favorite part is being able to quickly see all flags set on an open
file handle:
(gdb) p file.o.o.flags
$2 = 24117517
(gdb) !./scripts/dbgflags.py o 24117517
LFS_O_WRONLY 0x00000001 Open a file as write only
LFS_O_CREAT 0x00000004 Create a file if it does not exist
LFS_O_EXCL 0x00000008 Fail if a file already exists
LFS_O_DESYNC 0x00000100 Do not sync or recieve file updates
LFS_o_REG 0x01000000 Type = regular-file
LFS_o_UNFLUSH 0x00100000 File's data does not match disk
LFS_o_UNSYNC 0x00200000 File's metadata does not match disk
LFS_o_UNCREAT 0x00400000 File does not exist yet
The only concern is if dbgflags.py falls out-of-sync often, I suspect
flag encoding will have quite a bit more churn than flags/tags. But we
can always drop this script in the future if this turns into a problem.
---
While poking around this also ended up with a bunch of other small
changes:
- Added LFS_*_MODE masks for consistency with other "type<->flag
embeddings"
- Added compat flag comments
- Adopted lowercase prefix for internal flags (LFS_o_ZOMBIE), though
not sure if I'll keep this yet...
- Tweaked dbgerr.py to also match ERR_ prefixes and to ignore case
- LFS_I_INCONSISTENT -> LFS_I_MKCONSISTENT
- LFS_I_CANLOOKAHEAD -> LFS_I_LOOKAHEAD
- LFS_I_UNCOMPACTED -> LFS_I_COMPACT
- LFS_I_CANCKMETA -> LFS_I_CKMETA
- LFS_I_CANCKDATA -> LFS_I_CKDATA
This just makes everything easier to read/pattern match, even if it's
a bit inaccurate english-wise. The imperative transformations were also
wildly inconsistent...
One of the unexpected side-effects of lazy file creation is that
suddenly LFS_O_EXCL doesn't make sense.
The standard definition: "Fail if the file exists", is easy enough to
implement, but doesn't really match what the user expects.
The user expects one of these calls to fail:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
But because we create files lazily (to prevent zero-length files after
powerloss), these both succeed.
---
I considered deferring the "file exists" check until we actually would
create the file, but while this _technically_ satisfies the
exclusitivity requirement, I decided against it as I think it just makes
the API way too confusing:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_close(&lfs, &file_a) => 0;
lfsr_file_close(&lfs, &file_b) => LFS_ERR_EXIST;
---
Instead, a simpler, more pragmatic approach: Fail if the file exists
_or_ if the file is open in a mode that will create the file:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => LFS_ERR_EXIST;
This explicitly does _not_ error on zombie/desync files:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_desync(&lfs, &file_a) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
And it does mean we aren't necessarily guaranteeing the file will be
created, but I think this does more-or-less what the user expects:
- open(a) -> desync(a) -> open(b) -> resync(a) is roughly equivalent to
opening a after creating b, which is perfectly fine with LFS_O_EXCL.
- open(a) -> open(b) (errors) -> desync(a) is one way to not actually
create the file, but is somewhat similar to removing the file after
creation.
If you're using desync files you should probably have a good
understanding of littlefs's sync model anyways.
And of course the user can always sync immediately after open to
guarantee file creation, while opting into the possibility of
zero-length files after powerloss.
Code changes:
code stack ctx
before: 38084 2624 752
after: 38128 (+0.1%) 2624 (+0.0%) 752 (+0.0%)
The motivation here is to simplify lfsr_mount, but there's a number of
knock-on effects.
For one, lfsr_mount should now be faster on filesystems with large
blocks:
O(nb(log b)(log_b n)) -> O(nb(log_b n))
But we now no longer check if our filesystem contains orphaned
stickynotes or unknown filetypes:
- Orphaned stickynotes turned out to not be a big deal. If we find
orphans we'd need to do a second traversal to remove them anyways (no
mutation allowed in lfsr_mount), so this actually ends up a net
improvement in the found-orphan case.
If anything, doing a traversal on first write sets user expectations
correctly, and can be offloaded with lfsr_fs_mkconsistent or
lfsr_fs_gc.
- Unknown filetypes are a bit more annoying (I actually forgot about
this check), but unknown filetypes that require special care should
probably set WCOMPAT/RCOMPAT flags.
Allowing unknown filetypes is a bit more flexible in cases where a
filesystem image is being shared between drivers with different
features (bootloader + app for example).
Though we should probably add more checks/tests that we're handling
these correctly now that we no longer just bail during mount...
Also renamed LFS_I_HASORPHANS -> LFS_I_UNTIDY.
Not doing something is cheaper than doing something, so this saves a bit
of code:
code stack ctx
before: 38120 2624 752
after: 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%)
This is mainly to solve the weird check-hole where passing CKPROGS/
CKREADS as mount flags has no effect on lfsr_format (I mean, it'd be a
bit silly if it did somehow):
LFS_F_RDWR 0 // Format the filesystem as read and write
LFS_F_CKPROGS 0x00000010 // Check progs by reading back progged data
LFS_F_CKREADS 0x00000020 // Check reads via parity bits/checksums
This makes lfsr_format a more cumbersome interface, but I don't know if
this is necessarily a bad thing. There's always risk of data loss when
calling lfsr_format, so maybe it should be a pain to call.
At the very least, format flags may be useful in the future for
enabling/disabling format-time things such as the planned block-map,
parity-tree, etc. Though it's unclear if such significant settings
should be format flags or somehow encoded as fields in our config
struct.
---
The LFS_F_* format flags of course ended up conflicting with our
internal LFS_F_* flags, so I renamed most of the internal flags to match
the closest flag set they participate in:
- LFS_F_TYPE -> LFS_O_TYPE
- LFS_F_UNFLUSH -> LFS_O_UNFLUSH
- LFS_F_UNSYNC -> LFS_O_UNSYNC
- LFS_F_ORPHAN -> LFS_O_ORPHAN
- LFS_F_ZOMBIE -> LFS_O_ZOMBIE
- LFS_F_ORPHANS -> LFS_I_ORPHANS
- LFS_F_UNCOMPACTED -> LFS_I_UNCOMPACTED
- LFS_F_TSTATE -> LFS_T_TSTATE
- LFS_F_BTYPE -> LFS_T_BTYPE
- LFS_F_DIRTY -> LFS_T_DIRTY
- LFS_F_MUTATED -> LFS_T_MUTATED
This may make it a bit less clear which flags are a part of the public
API, vs intended only for internal use, but at the very least our asserts
in format/mount/open/etc should catch most of these mistakes.
---
Code cost ended up being pretty minimal. Actually negative. This is the
second time we're _adding_ a feature that somehow saves code, though the
reality for this one is we're really just pushing constants up into the
user's stack frame. Still, it's a good indication the cost of format
flags is small:
code stack
before: 36452 2680
after: 36448 (-0.0%) 2680 (+0.0%)
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%)
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%)
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).
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%)
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).
This sort of inverts the previous logic. Tests can still define
OPS='2*N' to scale the number of ops roughly with the number of entries,
but this fits better into the test framework, allows overriding, scaling
can be more easily tweaked, can be swapped out with a constant (like in
test_wl), etc.
Also tweaked some of the related N constants/filter conditions in tests
since these are now being effectively doubled... This should leave the
resulting number of ops unchanged.
- rename -> mv
- remove -> rm
- general -> mvrm (room for more ops)
Easier to read, fewer characters. And we're already using these in
test_files/dirs, so we should prefer these for consistency.
Good news! test_wl_orphanzombie_fuzz found a rare and difficult to reach
bug. Bad news, it found the bug only after changing littlefs's initial
revision count, which is about as unrelated a change as you can possibly
have...
Oh well, at least now we can add specialized tests targeting this (and
push them to hopefully cover anything similar):
- test_files_mv_split
- test_files_mv_split_backwards
- test_forphans_rename_split
- test_forphans_rename_split_backwards
The bug occurs when a rename of a file to/from the same mdir triggers an
mdir split, and you have that file opened, and the opened file handle
tracks a bshrub or bsprout. Oh, and if that wasn't unlikely enough, this
only breaks when the rename crosses from the new-right-sibling to the
new-left-sibling (inverse order of mdir split compacts), left-to-right
is fine.
The problem is how we stage bshrubs/bsprouts. bshrubs/bsprouts are a bit
tricky in that several unrelated operations can change their location,
sometimes multiple times in the same lfsr_mdir_commit call:
- mdir compaction - move bshrub/bsprout to new mdir
- bshrub commit - append a new shrub trunk
- rename commit - move bshrub/bsprout to a new mdir/mid
To keep track of all of this, lfsr_file_t has a dedicated field,
file.bshrub_, that holds the bshrub/bsprout's new location during
lfsr_mdir_commit. This may be changed multiple times, but the last
change wins.
This works as long as changes occur in an expected order. Importantly,
commits that change the bshrub, such as rename, need to play out after
compactions.
It turns out this is violated when splitting an mdir.
Because we have single pcache, we need to write out the entire compact +
commit of each mdir at a time. When we split, we arbitrarily do this
left-to-right, which results in left commits being played out before
right compactions.
Here's how things play out when we rename right-to-left:
1. commit rename -> bshrub = src mid, orig mdir
2. commit fails because of ERANGE
3. compact left mdir -> bshrub = src mid, left mdir
4. commit left mdir -> bshrub = dst mid, left mdir
5. compact right mdir -> bshrub = src mid, right mdir
6. commit right mdir (skips rename)
Oh no! Our staged bshrub ends up with the wrong location.
---
This is quite tricky to solve. We can't just play out the rename again
on the right mdir, because we've already lost the new bshrub trunk at
this point. Other solutions involving the grm or extra "moved" flags get
messy because, well, lfsr_mdir_commit's internals are quite messy.
The solution here, which is a bit hacky, but also obnoxiously elegant in
a way, is to reorder the split mdir compactions such that the new mdir
containing the commit mid is always compacted last. The means any
related attrs are played out after both compactions, allowing renames to
resolve correctly:
1. commit rename -> bshrub = src mid, orig mdir
2. commit fails because of ERANGE
3. right mdir contains mid
4. compact right mdir -> bshrub = src mid, right mdir
5. commit right mdir (skips rename)
6. compact left mdir -> bshrub = src mid, left mdir
7. commit left mdir -> bshrub = dst mid, left mdir
This only works as long as such commits only span a single mid, though
we already rely on mdir commits being single-mid elsewhere, so maybe
this won't be a problem?
The only real remaining concern is how much complexity this adds to
lfsr_mdir_commit. And while this feels logically messy, the resulting
code cost is surprisingly little:
code stack
before: 33458 2640
after: 33482 (+0.1%) 2640 (+0.0%)
Still, I'll have to scratch my head to see if there's a better way to
solve this...
Because of course ternary operators would cause problems.
The two problem:
LFS_ASSERT((exists) ? !err : err == LFS_ERR_NOENT);
lfsr_file_sync(&lfs, &file) => (zombie) ? 0 : LFS_ERR_NOENT;
We could work around these with parentheses, but with different assert
parsers floating around this issue is likely to crop up again in the
future.
Fortunately this just required separate "sep" vs "term" rules and a bit
more strict parsing.
test_wl is intended to test wear-leveling, although right now that just
involves heavy-duty fuzz tests with extremely low block_recycles.
What may be more interesting is the addition of aggressive orphan/zombie
tests:
- test_forphans_orphanzombie_fuzz
- test_forphans_orphanzombiedir_fuzz
- test_wl_orphanzombie_fuzz
- test_wl_orphanzombiedir_fuzz
These tests mix random file/dir operations while keeping random file
handles open, creating a complex environment for hitting weird orphan/
zombie corner cases.
And they did find a bug! We were asserting on LFS_ERR_RANGE when
migrating shrubs/sprouts during lfsr_mdir_commit__. The tricky thing
about lfsr_mdir_commit__ is that we need to expect LFS_ERR_RANGE from
any append operations, since this is what trigger mdir compaction. This
is especially tricky since LFS_ERR_RANGE is a hard error in most other
functions.
Easy fix. lfsr_mdir_commit__ contains no more LFS_ERR_RANGE asserts.
With these tests hopefully that's the last time we see this mistake.
This better matches test_dirs/test_files.
I guess the rule is singular for filesystem building blocks (test_rbyd,
test_btree, test_mtree, etc), plural for filesystem entries (test_dirs,
test_files, test_forphans, etc)?
Christopher Haster