Note: This affects the blocking lfs3_alloc_repopgbmap as well as
incremental gc/traversal repopulations. Now all repop attempts return
LFS3_ERR_NOSPC when we don't have space for the gbmap, motivation below.
This reverts the previous LFS3_t_NOSPC soft error, in which traversals
were allowed to continue some gc/traversal work when encountering
LFS3_ERR_NOSPC. This results in a simpler implementation and fewer error
cases to worry about.
Observation/motivation:
- The main motivation is noticing that when we're in low-space
conditions, we just start spamming gbmap repops even if they all fail.
That's really not great! We might as well just mark the flash as dead
if we're going to start spamming erases!
At least with an error the user can call rmgbmap to try to make
progress.
- If we're in a low-space condition, something else will probably return
LFS3_ERR_NOSPC anyways. Might as well report this early and simplify
our system.
- It's a simpler model, and littlefs3 is already much more complicated
than littlefs2. Maybe we should lean more towards a simpler system
at the cost of some niche optimizations.
---
This had the side-effect of causing more lfs3_alloc_ckpoints to return
errors during testing, which revealed a bug in our uz/uzd_fuzz tests:
- We weren't flushing after writes to the opened RDWR files, which could
cause delayed errors to occur during the later read checks in the
test.
Fortunately LFS3_O_FLUSH provides a quick and easy fix!
Note we _don't_ adopt this in all uz/uzd_fuzz tests, only those that
error. It's good to test both with and without LFS3_O_FLUSH to test
that read-flushing also works under stress.
Saves a bit of code:
code stack ctx
before: 37260 2352 688
after: 37220 (-0.1%) 2352 (+0.0%) 688 (+0.0%)
code stack ctx
gbmap before: 40220 2368 856
gbmap after: 40184 (-0.1%) 2368 (+0.0%) 856 (+0.0%)
In lfs3_fs_grow, we need to update any gbmaps to match the new disk
size. The actual patch to the gbmap is easy, but it does get a bit
delicate since we need to feed the gbmap with an allocator in the new
disk size.
Fortunately, the opportunistism of the gbmap allocator avoids any
catch-22 issues, as long as we make sure to not trigger any gbmap
rebuilds.
Adds a bit of code, but not much:
code stack ctx
before: 37168 2352 684
after: 37168 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
gbmap before: 39000 2456 800
gbmap after: 39116 (+0.3%) 2456 (+0.0%) 800 (+0.0%)
Having gbmap/bmap used in different places for the same thing was
confusing. Preferring gbmap as it is consistent with other gstate (grm
queue, gcksums), even if it is a bit noisy.
It's interesting to note what didn't change:
- The BM* range tags: LFS3_TAG_BMFREE, etc. These already differs from
the GBMAP* prefix enough, and adopting GBM* would risk confusion for
actual gstate.
- The gbmap revdbg string: "bb~r". We don't have enough characters for
anything else!
- dbgbmap.py/dbgbmapsvg.py. These aren't actually related to the gbmap,
so the name difference is a good thing.
The main change is error propagation in lfs3_alloc_ckpoint. Since
lfs3_alloc_ckpoint writes to disk during bmap rebuilds, it can now fail
in all sorts of ways. Fortunately lfs3_alloc_ckpoint should only ever be
called by write operations, where these errors are be expected.
With bmap rebuild errors now reported correctly, this unblocks most of
the remaining test TODOs:
- Passing test_badblocks
- Passing test_ck
- Passing test_trvs
With this, LFS3_YES_BMAP is now passing all but two tests, which are
still ifndef-disabled as a temporary measure:
- test_btree - We make some low-level assumptions about the lookahead
allocator when testing btrees. It's probably not worth trying to get
this passing with the bmap allocator.
- test_grow - This one does need fixing! We currently don't update
on-disk bmaps correctly when growing the filesystem.
Code changes minimal:
code stack ctx
before: 36912 2368 684
after: 36912 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38456 2400 812
bmap after: 38512 (+0.1%) 2400 (+0.0%) 812 (+0.0%)
At least at a proof-of-concept level, there's still a lot of cleanup
needed.
To make things work, lfs3_alloc_ckpoint now takes an mdir, which
provides the target for gbmap gstate updates.
When the bmap is close to empty (configurable via bmap_scan_thresh), we
opportunistically rebuild it during lfs3_alloc_ckpoints. The nice thing
about lfs3_alloc_ckpoint is we know the state of all in-flight blocks,
so rebuilding the bmap just requires traversing the filesystem + in-RAM
state.
We might still fall back to the lookahead buffer, but in theory a well
tuned bmap_scan_thresh can prevent this from becoming a bottleneck (at
the cost of more frequent bmap rebuilds).
---
This is also probably a good time to resume measuring code/ram costs,
though it's worth repeating the above note about the bmap work still
needing cleanup:
code stack ctx
before: 36840 2368 684
after: 36920 (+0.2%) 2368 (+0.0%) 684 (+0.0%)
Haha, no, the bmap isn't basically free, it's just an opt-in features.
With -DLFS3_YES_BMAP=1:
code stack ctx
no bmap: 36920 2368 684
yes bmap: 38552 (+4.4%) 2472 (+4.4%) 812 (+18.7%)
Note this includes both the lfs3_config -> lfs3_cfg structs as well as
the LFS3_CONFIG -> LFS3_CFG include define:
- LFS3_CONFIG -> LFS3_CFG
- struct lfs3_config -> struct lfs3_cfg
- struct lfs3_file_config -> struct lfs3_file_cfg
- struct lfs3_*bd_config -> struct lfs3_*bd_cfg
- cfg -> cfg
We were already using cfg as the variable name everywhere. The fact that
these names were different was an inconsistency that should be fixed
since we're committing to an API break.
LFS3_CFG is already out-of-date from upstream, and there's plans for a
config rework, but I figured I'd go ahead and change it as well to lower
the chances it gets overlooked.
---
Note this does _not_ affect LFS3_TAG_CONFIG. Having the on-disk vs
driver-level config take slightly different names is not a bad thing.
This better matches how other filesystems refer to the number of in-use
blocks.
Which makes sense when you consider that "size" could also refer to the
configured block_count. The term "usage" avoids this ambiguity.
So now calling lfsr_file_sync on zombied files is a noop:
// create a file
lfsr_file_t a;
lfsr_file_open(&lfs, &a, "a",
LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
// remove, creating a zombie
lfsr_remove(&lfs, "a") => 0;
// sync, this is now a noop (previously LFS_ERR_NOENT)
lfsr_file_sync(&lfs, &a) => 0;
// close is also a noop
lfsr_file_close(&lfs, &a) => 0;
I've been on the fence on this for a while, on one hand erroring
provides more information to the user, on the other hand a noop is less
surprising if the user comes from other systems.
Ended up making this a noop. I figured minimizing surprises is good API
design, and the user can always use lfsr_stat to check if the file still
exists.
This also matches POSIX, and, perhaps more importantly, the current
version of littlefs.
---
Note that lfsr_file_resync still errors with LFS_ERR_NOENT. It's hard to
argue the file "matches the state of disk" otherwise.
Code changes minimal:
code stack ctx
before: 35784 2440 640
after: 35780 (-0.0%) 2440 (+0.0%) 640 (+0.0%)
Seems like a better name now that LFS_TYPE_STICKYNOTE is its own file
type.
Though this does contain some tests that I think don't even use
stickynotes...
This adds the LFS_TYPE_STICKYNOTE type, allowing users to interact with
stickynotes as long as they aren't orphaned.
This hopefully solves the long-standing mess that was the LFS_O_EXCL
API.
---
As for what I mean by orphaned vs non-orphaned stickynotes:
Non-orphaned stickynotes represent files that have been "created" (via
LFS_O_CREAT), but not "committed" (via sync/close). You can still close
and convert the stickynote to a reg file, so these aren't orphans. These
are also called "uncreated" files in some parts of the codebase:
- open+O_CREAT -> non-orphaned stickynote (uncreated file)
Orphaned stickynotes are possible by either removing an open file, or
desyncing a file before sync/close. These are still invisible to the
user and will be eventually cleaned up after the last file handle is
closed:
- open+remove -> orphaned stickynote (zombied file)
- open+O_CREAT+desync+close -> orphaned stickynote (orphaned file)
Desynced files are a bit special. Even though they technically aren't
orphaned, they also behave like orphaned file handles:
- open+O_CREAT+close -> orphaned stickynote (desynced file)
The idea is this mimics the state of files post-close, and allows for
some tricks like using a desync file as a temporary file with no
observable effects on the filesystem.
---
The motivation for this comes from staring at the LFS_O_EXCL API for too
long and realizing the problem is that littlefs's API contradicts itself
when it comes to whether or not uncreated files exist.
This solution is to consistently treat uncreated files as though they
exist (the alternative would make LFS_O_EXCL pretty much useless), but I
really didn't want to do this as having what appears to be normal files
disappear after powerloss risks confusion.
The compromise here is to give these files a special type, repurposing
the internal LFS_TAG_STICKYNOTE, which hopefully hints to the user these
won't behave like normal files.
If the user is more interested in POSIX compatibility, they can always
map these to either LFS_TYPE_REG or LFS_ERR_NOENT, whichever they think
is the least confusing.
As a quirk of littlefs's API, stickynotes should never actually contain
any data, and will always have size 0.
However they can have custom attributes assigned now (which is I guess
ok? also TODO should probably test this).
---
The implementation right now is a bit naive, I mostly just wanted to get
the tests working again in this new model. It may be possible to claw
back some of this code cost:
code stack ctx
before: 35740 2440 640
after: 35952 (+0.6%) 2440 (+0.0%) 640 (+0.0%)
And the related config options:
- cfg->file_buffer_size -> cfg->file_cache_size
- file->cfg->buffer_size -> file->cfg->cache_size
- file->cfg->buffer -> file->cfg->cache_buffer
The original motivation to rename this to file->buffer was to better
align with what other filesystems call this, but I think this is a case
where internal consistency is more important than external consistency.
file->cache better matches lfs->pcache and lfs->rcache, and makes it
easier to read code involving both file->cache and other user-provided
buffers.
Keeping the upstream name also helps with continuity.
These are our current set of general-purpose high-level tests that can
be turned to when needing to test a wide range of filesystem operations.
They were getting a bit hard to keep track of without a consistent
prefix, especially since no individual test suite can actually use all
of them at the same time.
Now, finding these tests is as simple as: ./scripts/test.py -L *_spam_*
I also renamed a couple because their names were starting to get
ridiculous. I mean just look at
test_badblocks_alternating_spam_orphanzombiedir_fuzz...
- *_spam_orphanzombie_fuzz -> *_spam_oz_fuzz
- *_spam_orphanzombiedir_fuzz -> *_spam_ozd_fuzz
- *_spam_file_pl_fuzz -> *_spam_f_pl_fuzz
- *_spam_filedir_pl_fuzz -> *_spam_fd_pl_fuzz
Here are all of the current spam tests and contexts we use them in:
traversal badblocks relocations
| gc ck grow | powerloss exhaustion
dir_many y y y y y y
dir_fuzz y y y y y y y
file_many y y y y y y
file_fuzz y y y y y y y
fwrite_fuzz y y y y y
oz_fuzz y y y y y y y
ozd_fuzz y y y y y y y
f_pl_fuzz y y y
fd_pl_fuzz y y y
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%)
With rcompat/wcompat flags, on-disk minor version bumps will hopefully
not be needed for a long time (ever?). And if the on-disk version never
changes, why was a word to report it every lfsr_fs_stat call?
But this may be something to listen to user feedback on. Worst case we
can always readd fsinfo.disk_version if users find it useful.
Code changes:
code stack
before: 33922 2592
after: 33918 (-0.0%) 2592 (+0.0%)
While it may be useful to know when/why lfsr_fs_fixgrm fails, at this
point in lfsr_rename/lfsr_remove the operation has already succeeded as
far as the filesystem is concerned.
It's counterintuitive, but ignoring these errors actually tells the user
_more_ information, specifically whether or not the operation completed
on disk.
At least we can log the error via LFS_WARN, and such errors will likely
come up again in a future operation, such as the call to lfsr_fs_fixgrm
on the next filesystem mutation.
This was noticed in test_grow, which tests error code-paths quite a bit
more than any other test.
Code changes:
code stack
before: 33934 2592
after: 33942 (+0.0%) 2592 (+0.0%)
Not sure how this was missed for so long, but we completely forget about
in-flight mroot attrs if we happen to uninline the mtree.
I guess this was missed because only some late-stage fs ops need to
commit mroot attrs (lfsr_fs_grow, lfsr_setattr, upgrades, etc), but
being able to commit to the mroot is definitely an operation we need to
support.
Fixing this in a non-awkward way was a bit tricky. We need some way to
commit both the provided attr-list and our new mtree, but all of the
lower layers only accept a single attr-list. The solution here
is to add a special tail-recursive LFSR_TAG_ATTRS that can be used to
chain together multiple attr-lists. This solves the problem quite
elegantly and may actually be useful in the future?
It takes a bit of code:
code stack
before: 33850 2584
after: 33926 (+0.2%) 2592 (+0.3%)
But this solves our final lfsr_fs_grow-related bug. No more mroot-split
hacks in test_grow, and we can now grow any stuck filesystem.
Well this turned into a never-ending can of worms...
I guess the good news is our newly added lfsr_grow_incr_* tests are
_very_ good at finding post-error-resume bugs.
Implementation-wise, this was fairly straightforward thanks to prior
work by BrianPugh, kaetemi, and myself:
1. Made block_count pseudo-optional by adding lfs.block_count so we can
mutate it based on what we find on-disk.
This was done a bit different from the previous implementation,
instead of setting block_count=0 to read the block_count from disk,
we allow any block_count <= the configured block_count.
This matches how we handle name_limit/file_limit/etc, and allows
users to mount a filesystem with unknown block_count while asserting
an upper bound.
2. Added lfsr_fs_grow, which can grow the filesystem.
The is basically the same as the previous implementation except we're
a bit more careful with the lookahead buffer.
I thought the previous impl might have been broken w.r.t. lookahead
buffer, but fortunately it's only broken in a way that makes us think
newly available blocks are temporarily in-use. Which is a bit funny.
One interesting thing that came out with more aggressive tests is
that it's possible to get locked-up in lfsr_fs_preparemutation trying
to clean up grms/orphans before we change the filesystem size.
Fortunately it turns out we don't _really_ need to call
lfsr_fs_preparemutation here. This gets a bit delicate, but means we
should always be able to grow a full filesystem.
To test this I've added both the simple grow/error tests from the
previous version, as well as a set of fuzz tests (a la test_relocations
and friends) that incrementally grow the filesystem when encountering
LFS_ERR_NOSPC. These have a surprising amount coverage, testing
lfsr_fs_grow, lfsr_fs_stat, lfsr_fs_size, and resuming operations after
encountering an error.
Which also means they found bugs:
- lfs_alloc_setinuse was not broken before, because lookahead.start was
always a multiple of lookahead_size. But now with lfs_alloc_discard,
this invariant may not be true.
I've just changed all lookahead.start updates to mod block_count. This
adds a bit of code, but is much easier to reason about.
While fixing this, I also added an assert to never allocate blocks
{0,1} in lfs_alloc. This is a good assert to have, but did require
some tweaks to test_btree to avoid these blocks.
- We were incorrectly patching grms in lfsr_mdir_commit when mdelta=0.
Funnily enough we also proceed to ignore the patched grm most of the
time when mdelta=0, so this went unnoticed.
- It turns out we're completely ignoring rid=-1 attrs if we split the
mroot. Not sure how this was missed. It's a bit important.
Note this is still broken. Fixing this requires some rather invasive
changes to lfsr_mdir_commit's internal logic that should probably be
in another commit...
Note again fwrite_fuzz is omitted. Currently the state of data in opened
files is undefined after a failed write, so this wouldn't really be
testing anything interesting...
More features = more code, and all of this bug fixing meant several
things contributed to code/stack changes in this commit:
code stack
before: 33654 2592
+variable block_count: 33646 (-0.0%) 2584 (+0.0%)
+lfsr_fs_grow: 33818 (+0.5%) 2584 (-0.3%)
+lookahead-start-fix: 33842 (+0.6%) 2584 (-0.3%)
+grm-patch-fix (after): 33850 (+0.6%) 2584 (-0.3%)
Wild that variable block_count actually saves code/stack. I guess the
indirect lfs->cfg->block_count load can get costly...
Christopher Haster