- Off-by-one in test_btree_find_general[_sparse]_fuzz
Because we can only create named btrees via splitting, these always
start with one entry. If all operations are randomly selected to be
splits, this can lead to an overflow of the sim buffer (sounds
unlikely, but relatively easily for small N).
The fix is to use a `for (lfs3_size_t i = 1; i < N; i++)` loop to
account for the initial entry. Note we already use this in the
test_btree_split_* tests.
An alternative is allocating space for N+1 entries, but this seems
unintuitive with N usually being associated with the upper bound on
btree size.
- Off-by-one in our sim rename pattern
When renaming, we don't bother to update sim_size, because after the
rename the sim_size size will be unchanged. But this means the
sim_size is out-of-date during the memmove that reinserts the renamed
entry. Buffer overflow!
To fix we just need to use sim_size-1 to account for the temporarily
deleted entry.
This is messy C code, so not surprised it went unnoticed, even though
this pattern ended up in quite a few tests.
Found while running with HEAP=1. This was just intended to test HEAP=1,
but I guess the injected heap hooks result in a more fragile heap? They
increase all allocations by one word, and maybe this reduces alignment
padding? Not exactly sure.
But it's a good argument for maybe adding heap canaries in the future.
Previously we ran Valgrind on all tests, but it's unclear if this will
still be reasonable with the number of tests we have now.
This walks back some of the attempt at strict object namespacing in
struct lfs3_cfg:
- cfg.file_cache_size -> cfg.fcache_size
- filecfg.cache_size -> filecfg.fcache_size
- filecfg.cache_buffer -> filecfg.fcache_buffer
- cfg.gbmap_re_thresh -> cfg.regbmap_thresh
Motivation:
- cfg.regbmap_thresh now matches cfg.gc_regbmap_thresh, instead of using
awkwardly different namespacing patterns.
- Giving fcache a more unique name is useful for discussion. Having
pcache, rcache, and then file_cache was a bit awkward.
Hopefully it's also more clear that cfg.fcache_size and
filecfg.fcache_size are related.
- Config in struct lfs3_cfg is named a bit more consistently, well, if
you ignore gc_*_* options.
- Less typing.
Though this gets into pretty subjective naming territory. May revert
this if the new terms are uncomfortable after use.
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%)
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.
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
So just like ckreads, ckprogs is now opt-in, requiring both 1. defining
LFS_CKPROGS at compile-time, and 2. passing the LFS_M_CKPROGS flag
during lfsr_mount.
_Unlike_ ckreads, ckprogs is actually a very lightweight feature. So the
difference between compiling with/without ckprogs is really quite small:
code stack
before: 36480 2680
yes-ckprogs: 36480 (+0.0%) 2680 (+0.0%)
no-ckprogs: 36428 (-0.1%) 2680 (+0.0%)
It's almost not worth putting behind an ifdef if not for consistency
with ckreads.
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%)
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%)
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).
The main test additions are the test_powerloss tests, intended to be
high-level tests over difficult/weird powerloss environments (such as
out-of-order writes!):
- test_powerloss_dir_many - 2242 pls
- test_powerloss_file_many - 8856 pls
- test_powerloss_file_pl_fuzz - 384508 pls
- test_powerloss_filedir_pl_fuzz - 268339 pls
But there was also a bunch of other test movement in the late-stage/
high-level tests. I'm trying to keep the core of these tests somewhat
consistent so we have a nice template to extend for future testing, in
case we want to test other environmentalish concerns, but not all of
these tests make sense in all of these contexts:
badblocks powerloss relocations exhaustion
dir_many y y y
dir_fuzz y y y
file_many y y y
file_fuzz y y y
fwrite_fuzz y y
orphanzombie_fuzz y y y
orphanzombiedir_fuzz y y y
file_pl_fuzz y y
filedir_pl_fuzz y y
Why not:
- dir/file_many+exhaustion? - Needs to be unbounded
- dir/file_fuzz+powerloss? - Takes O(n^2)
- fwrite_fuzz+powerloss? - Takes O(n^2)
- fwrite_fuzz+relocations? - Doesn't really test anything
- orphanzombie*_fuzz+powerloss? - Powerloss kills zombies
- file*_pl_fuzz+badblocks? - PL + Badblocks currently incompactible
- file*_pl_fuzz+exhaustion? - PL + Badblocks currently incompactible
---
Of course, in order to actually get out-of-order write testing working,
we need to implement out-of-order write syncing.
Fortunately this was a simple exercise in placing lfsr_bd_sync calls
before any mdir commits where we may have unsynced data:
- in lfsr_file_sync, to sync any pending file data
- in lfsr_mdir_commit, to sync any mroot/mtree changes
We also call lfsr_bd_sync _after_ mdir commits in case users expect to
sequence any filesystem-external operations such as network, UI, etc. In
theory this could be optional, but no users have really requested it
yet, so leave that for a potential future improvement:
- in lfsr_mdir_commit
- in lfsr_formatinited (really just because we don't go through
lfsr_mdir_commit)
Note that lfsr_rbyd_commit has been relaxed in the scheme. It only
flushes caches, and does _not_ call lfsr_bd_sync. This is useful for
allowing multiple B-tree nodes to be written out-of-order, also long as
the whole thing is synchronized before any mdir commit.
All of these lfsr_bd_sync calls add a bit of code, but not really an
amount to care about:
code stack
before: 33678 2600
after: 33766 (+0.3%) 2600 (+0.0%)
This configuration option enables the previous behavior of reading back
every prog to check that the data was written correctly.
Unfortunately, this brings a bit of baggage, thanks to our cache
interactions being more complicated now:
- We really want to reuse the rcache for prog validation, despite the
cache performance implications. Unfortunately, we simply can't, thanks
to the new bd utility functions tying up the rcache. lfsr_bd_cpy, for
example, does not expect rcache to be invalidated between a read and
prog, and if it is, things break (I may or may not have found this by
experience).
These bd utilities are valuable, so we really need some other way to
validate our progs.
- Since we can't rely on the rcache, this leaves checksumming as the
only option for validating progs. Checksumming isn't perfect, as there
is a decent chance of false negatives, but to be honest it's probably
good enough for anything that's not malicious.
- This also adds the new constraint that we need to be able to read back
any prog into the pcache, which implies read_size <= prog_size. This
constraint didn't exist when we could clobber our rcache, but this is
not worth throwing away the new bd utilities. Not to mention
clobbering our rcache could hurt cache performance.
Why not make read_size <= prog_size conditional on check_progs?
The main reason is convenience. One very compelling use case for
check_progs is to help debug unknown filesystem/integration failures,
buf if you can't enable check_progs without changing the filesystem
configuration, you can't really rely on check_progs for debugging.
This helps future proof what we expect from block devices, in case
future error detection/correction mechanisms can benefit from our
prog_size always being readable.
Code changes were not that significant, however there was a surprising
stack cost. This seems to be because lfsr_bd_read__ can now be called
from multiple places, causing it to no longer be inlined in
lfsr_bd_read_, costing a bit of stack for the additional function call:
before: 33566 2624
after: 33682 (+0.3%) 2640 (+0.6%)
This acts as a marker to indicate a fuzz test. It should reference a
define, usually SEED, that can be randomized to get interesting test
permutations.
This is currently unused, but could lead to some interesting uses such
as time-based fuzz testing. It's also just useful for inspecting the
tests (make test-list).
These seem to fit better as a separate test suite, since they involve a
few more moving parts than just relocations (badblocks, enospc, etc).
Maybe we'll end up adding more relocation/exhaustion specific tests?
This organization can always be changed in the future.
It's worth noting that, even separated, these are still some of the
longest running test suites:
... ...
test_exhaustion 488.1s
test_dirs 593.7s
test_rbyd 675.6s
test_badblocks 975.8s
test_relocations 1013.9s
test_fwrite 1868.3s
TOTAL 6076.3s
This makes it easier to evaluate the code/stack/etc sizes and run tests
without bringing in all of the outdated code.
I guess this officially makes this branch more-or-less a full rewrite,
though the benefit of commenting vs deleting this code is that it can be
easily pulled back in when useful.
When you add a function to every benchmark suite, you know if should
probably be provided by the benchmark runner itself. That being said,
randomness in tests/benchmarks is a bit tricky because it needs to be
strictly controlled and reproducible.
No global state is used, allowing tests/benches to maintain multiple
randomness stream which can be useful for checking results during a run.
There's an argument for having global prng state in that the prng could
be preserved across power-loss, but I have yet to see a use for this,
and it would add a significant requirement to any future test/bench runner.
These are really just different flavors of test.py and test_runner.c
without support for power-loss testing, but with support for measuring
the cumulative number of bytes read, programmed, and erased.
Note that the existing define parameterization should work perfectly
fine for running benchmarks across various dimensions:
./scripts/bench.py \
runners/bench_runner \
bench_file_read \
-gnor \
-DSIZE='range(0,131072,1024)'
Also added a couple basic benchmarks as a starting point.
The main benefit is small test ids everywhere, though this is with the
downside of needing longer names to properly prefix and avoid
collisions. But this fits into the rest of the scripts with globally
unique names a bit better. This is a C project after all.
The other small benefit is test generators may have an easier time since
per-case symbols can expect to be unique.
This mostly required names for each test case, declarations of
previously-implicit variables since the new test framework is more
conservative with what it declares (the small extra effort to add
declarations is well worth the simplicity and improved readability),
and tweaks to work with not-really-constant defines.
Also renamed test_ -> test, replacing the old ./scripts/test.py,
unfortunately git seems to have had a hard time with this.
- Added caching to Travis install dirs, because otherwise
pip3 install fails randomly
- Increased size of littlefs-fuse disk because test script has
a larger footprint now
- Skip a couple of reentrant tests under byte-level writes because
the tests just take too long and cause Travis to bail due to no
output for 10m
- Fixed various Valgrind errors
- Suppressed uninit checks for tests where LFS_BLOCK_ERASE_VALUE == -1.
In this case rambd goes uninitialized, which is fine for rambd's
purposes. Note I couldn't figure out how to limit this suppression
to only the malloc in rambd, this doesn't seem possible with Valgrind.
- Fixed memory leaks in exhaustion tests
- Fixed off-by-1 string null-terminator issue in paths tests
- Fixed lfs_file_sync issue caused by revealed by fixing memory leaks
in exhaustion tests. Getting ENOSPC during a file write puts the file
in a bad state where littlefs doesn't know how to write it out safely.
In this case, lfs_file_sync and lfs_file_close return 0 without
writing out state so that device-side resources can still be cleaned
up. To recover from ENOSPC, the file needs to be reopened and the
writes recreated. Not sure if there is a better way to handle this.
- Added some quality-of-life improvements to Valgrind testing
- Fit Valgrind messages into truncated output when not in verbose mode
- Turned on origin tracking
Moved .travis.yml over to use the new test framework. A part of this
involved testing all of the configurations ran on the old framework
and deciding which to carry over. The new framework duplicates some of
the cases tested by the configurations so some configurations could be
dropped.
The .travis.yml includes some extreme ones, such as no inline files,
relocations every cycle, no intrinsics, power-loss every byte, unaligned
block_count and lookahead, and odd read_sizes.
There were several configurations were some tests failed because of
limitations in the tests themselves, so many conditions were added
to make sure the configurations can run on as many tests as possible.
These should probably have been cleaned up in each commit to allow
cherry-picking, but due to time I haven't been able to.
- Went with creating an mdir copy in lfs_dir_commit. This handles a
number of related cleanup issues in lfs_dir_compact and it does so
more robustly. As a plus we can use the copy to update dependencies
in the mlist.
- Eliminated code left by the ENOSPC file outlining
- Cleaned up TODOs and lingering comments
- Changed the reentrant many directory create/rename/remove test to use
a smaller set of directories because of space issues when
READ/PROG_SIZE=512
This was an interesting issue found during a GitHub discussion with
rmollway and thrasher8390.
Blocks in the metadata-pair are relocated every "block_cycles", or, more
mathy, when rev % block_cycles == 0 as long as rev += 1 every block write.
But there's a problem, rev isn't += 1 every block write. There are two
blocks in a metadata-pair, so looking at it from each blocks
perspective, rev += 2 every block write.
This leads to a sort of aliasing issue, where, if block_cycles is
divisible by 2, one block in the metadata-pair is always relocated, and
the other block is _never_ relocated. Causing a complete failure of
block-level wear-leveling.
Fortunately, because of a previous workaround to avoid block_cycles = 1
(since this will cause the relocation algorithm to never terminate), the
actual math is rev % (block_cycles+1) == 0. This means the bug only
shows its head in the much less likely case where block_cycles is a
multiple of 2 plus 1, or, in more mathy terms, block_cycles = 2n+1 for
some n.
To workaround this we can bitwise or our block_cycles with 1 to force it
to never be a multiple of 2n.
(Maybe we should do this during initialization? But then block_cycles
would need to be mutable.)
---
There's a few unrelated changes mixed into this commit that shouldn't be
there since I added this as part of a branch of bug fixes I'm putting
together rather hastily, so unfortunately this is not easily cherry-pickable.
It's interesting how many ways block devices can show failed writes:
1. prog can error
2. erase can error
3. read can error after writing (ECC failure)
4. prog doesn't error but doesn't write the data correctly
5. erase doesn't error but doesn't erase correctly
Can read fail without an error? Yes, though this appears the same as
prog and erase failing.
These weren't all simulated by testbd since I unintentionally assumed
the block device could always error. Fixed by added additional bad-black
behaviors to testbd.
Note: This also includes a small fix where we can miss bad writes if the
underlying block device contains a valid commit with the exact same
size in the exact same offset.
Fixes:
- Fixed reproducability issue when we can't read a directory revision
- Fixed incorrect erase assumption if lfs_dir_fetch exceeds block size
- Fixed cleanup issue caused by lfs_fs_relocate failing when trying to
outline a file in lfs_file_sync
- Fixed cleanup issue if we run out of space while extending a CTZ skip-list
- Fixed missing half-orphans when allocating blocks during lfs_fs_deorphan
Also:
- Added cycle-detection to readtree.py
- Allowed pseudo-C expressions in test conditions (and it's
beautifully hacky, see line 187 of test.py)
- Better handling of ctrl-C during test runs
- Added build-only mode to test.py
- Limited stdout of test failures to 5 lines unless in verbose mode
Explanation of fixes below
1. Fixed reproducability issue when we can't read a directory revision
An interesting subtlety of the block-device layer is that the
block-device is allowed to return LFS_ERR_CORRUPT on reads to
untouched blocks. This can easily happen if a user is using ECC or
some sort of CMAC on their blocks. Normally we never run into this,
except for the optimization around directory revisions where we use
uninitialized data to start our revision count.
We correctly handle this case by ignoring whats on disk if the read
fails, but end up using unitialized RAM instead. This is not an issue
for normal use, though it can lead to a small information leak.
However it creates a big problem for reproducability, which is very
helpful for debugging.
I ended up running into a case where the RAM values for the revision
count was different, causing two identical runs to wear-level at
different times, leading to one version running out of space before a
bug occured because it expanded the superblock early.
2. Fixed incorrect erase assumption if lfs_dir_fetch exceeds block size
This could be caused if the previous tag was a valid commit and we
lost power causing a partially written tag as the start of a new
commit.
Fortunately we already have a separate condition for exceeding the
block size, so we can force that case to always treat the mdir as
unerased.
3. Fixed cleanup issue caused by lfs_fs_relocate failing when trying to
outline a file in lfs_file_sync
Most operations involving metadata-pairs treat the mdir struct as
entirely temporary and throw it out if any error occurs. Except for
lfs_file_sync since the mdir is also a part of the file struct.
This is relevant because of a cleanup issue in lfs_dir_compact that
usually doesn't have side-effects. The issue is that lfs_fs_relocate
can fail. It needs to allocate new blocks to relocate to, and as the
disk reaches its end of life, it can fail with ENOSPC quite often.
If lfs_fs_relocate fails, the containing lfs_dir_compact would return
immediately without restoring the previous state of the mdir. If a new
commit comes in on the same mdir, the old state left there could
corrupt the filesystem.
It's interesting to note this is forced to happen in lfs_file_sync,
since it always tries to outline the file if it gets ENOSPC (ENOSPC
can mean both no blocks to allocate and that the mdir is full). I'm
not actually sure this bit of code is necessary anymore, we may be
able to remove it.
4. Fixed cleanup issue if we run out of space while extending a CTZ
skip-list
The actually CTZ skip-list logic itself hasn't been touched in more
than a year at this point, so I was surprised to find a bug here. But
it turns out the CTZ skip-list could be put in an invalid state if we
run out of space while trying to extend the skip-list.
This only becomes a problem if we keep the file open, clean up some
space elsewhere, and then continue to write to the open file without
modifying it. Fortunately an easy fix.
5. Fixed missing half-orphans when allocating blocks during
lfs_fs_deorphan
This was a really interesting bug. Normally, we don't have to worry
about allocations, since we force consistency before we are allowed
to allocate blocks. But what about the deorphan operation itself?
Don't we need to allocate blocks if we relocate while deorphaning?
It turns out the deorphan operation can lead to allocating blocks
while there's still orphans and half-orphans on the threaded
linked-list. Orphans aren't an issue, but half-orphans may contain
references to blocks in the outdated half, which doesn't get scanned
during the normal allocation pass.
Fortunately we already fetch directory entries to check CTZ lists, so
we can also check half-orphans here. However this causes
lfs_fs_traverse to duplicate all metadata-pairs, not sure what to do
about this yet.
- Removed old tests and test scripts
- Reorganize the block devices to live under one directory
- Plugged new test framework into Makefile
renamed:
- scripts/test_.py -> scripts/test.py
- tests_ -> tests
- {file,ram,test}bd/* -> bd/*
It took a surprising amount of effort to make the Makefile behave since
it turns out the "test_%" rule could override "tests/test_%.toml.test"
which is generated as part of test.py.
Christopher Haster