These functions provide simple access to littlefs's custom attributes,
which are small pieces of user-specified metadata that can be attached
to files, dirs, root, etc:
- lfsr_getattr - Reads an attribute
- lfsr_sizeattr - Gets the size of an attribute
- lfsr_setattr - Writes an attribute
- lfsr_removeattr - Removes an attribute
You may notice these functions look quite a bit different from their
previous incarnations. This is because the custom attribute API is
getting an overhaul based on feedback provided by users
The previous API had some real design flaws that interfered with
usability, but now that things have had some time to settle (6 years!),
hopefully most of the pain points are clear.
Notable changes:
- lfsr_getattr's return value is now limited by buffer size.
The intention of the previous API, where lfsr_getattr always returns
the attr size, even if it's larger than the buffer, was to allow users
to find the attr size without an infinitely large buffer.
In defense of this design, Linux's getxattr does something somewhat
similar, returning the attr size when the buffer size equals zero.
Though getxattr does truncate when buffer size is non-zero, which is
probably safer.
But, let's be honest, this multipurpose abuse of lfsr_getattr's return
value is inconsistent with other read functions and potentially
dangerous for users.
I think one of the reasons for this API in Linux-land is the limited
syscall numbers discouraging new functions, but we have no such
limitation here! We might as well add a dedicated function for
this: lfsr_sizeattr.
- No more padding with zeros!
This was a cludge to get around the lack of returned size in custom
attributes attached to files, but is inconsistent with other read
functions, so needs to go.
In general, inconsistencies violate user assumptions, and are usually
a sign of a bad API.
- lfsr_setattr now takes flags.
This gives lfsr_setattr more flexiblity in how it operates, and may
make future extensions easier.
lfsr_setattr currently supports two flags, which may look a bit
familiar:
LFS_A_CREAT 0x04 // Create an attr if it does not exist
LFS_A_EXCL 0x08 // Fail if an attr already exists
One long-term idea is to eventually add a simple lfsr_set function to
make it easier to create small files, so this sort of design overlap
between lfsr_setattr and lfsr_file_open is hopefully a good thing.
---
Code-wise, these function are really not that bad. Adding functions adds
code, but these are just small wrappers over our internal lookup/commit
functions:
code stack
before: 36556 2608
after: 37116 (+1.5%) 2608 (+0.0%)
Of course the real cost of custom attributes is how they interact with
open files, a detail which is conveniently missing for now...
It would be nice to have a full 8-bit range for both user attrs and
system attrs, for both backwards compatibility and maximizing the
available attr space, but I think it just doesn't make sense from an API
perspective.
Sure we could finagle the user/sys bit into a flags argument, or provide
separate lfsr_getuattr/getsattr functions, but asking users to use a
9-bit int for higher-level operations (dynamic attrs, iteration, etc) is
a bit much...
So this reduces the two attr ranges down to 7-bits, requiring 8-bits
total to store all possible attr types in the current system:
TAG_ATTR 0x0400 v--- -1-a -aaa aaaa
TAG_UATTR 0x04aa v--- -1-- -aaa aaaa
TAG_SATTR 0x05aa v--- -1-1 -aaa aaaa
This really just affects scripts, since we haven't actually implemented
attributes yet.
Worst case we still have the 9-bit encoding space carved out, so we can
always add an additional set of attrs in the future if we start running
into attr pressure.
Or, you know, just turn on the subtype leb128 encoding the 8th subtype
bit is reserved for. Then you'd only be limited by internal driver
details, probably 24-bits per attr range if we make tags 32-bits
internally. Though this would probably come with quite a code cost...
These are just simple wrappers over their lfsr_bshrub_* cousins, with a
bit of field unpacking for convenience.
Surprisingly these didn't save any code, but saved some RAM. I guess
due to more flexibility in inlining?
code stack
before: 36552 2616
after: 36556 (+0.0%) 2608 (-0.3%)
This saves most of the cost of adding lfsr_file_resync in the first
place:
code stack
before resync: 36412 2616
before fetch: 36748 (+0.9%) 2616 (+0.0%)
after fetch: 36552 (+0.4%) 2616 (+0.0%)
lfsr_file_resync discards the current working state of a file and
reverts it to the contents on disk. It also clears the desynced flag
from files, so provides an alternative to lfsr_file_sync for when you
don't want to write to the filesystem:
disk=A file=A disk=A file=A
| write B | write B
v v
disk=A file=B disk=A file=B
| sync | resync
v v
disk=B file=B disk=A file=A
The main motivation for this is to provide a way to mark desynced
readonly files as in-sync, without putting them into a weird state where
they are "in-sync" but don't match disk.
It's also a bit safer if the file is desynced due to an error, since
errors aren't currently guaranteed to leave file data in a defined
state. Needed to resync to recover from errors avoids accidentally
syncing partial writes.
This exact behavior can also be accomplished by closing+opening the
file, but lfsr_file_resync makes it much easier without _that_ much
extra code. It may even pay for itself if you consider what code it
saves on the user's side of things.
I considered naming this lfsr_file_discard because I think it sounds
cooler, but I figured including sync in the name provides a stronger
hint that it affects the file's desync status.
---
You may think it's not possible for a readonly file to become
out-of-sync from disk, since it's, well, readonly. But it is possible
thanks to desynced files ignoring other sync broadcasts.
Consider what happens if you open a file readonly, and write+sync the
file with another file handle at the same time:
disk=A f1=A f2=A
| desync f2
v
disk=A f1=A f2=A
| write f1=B
v
disk=A f1=B f2=A
| sync f1
v
disk=B f1=B f2=A <-- f2 is out-of-sync without any writes
---
This commit also changes lfsr_file_sync/flush to assert if the file is
readonly. Previously we allowed lfsr_file_sync to be called on readonly
files if it would be a noop, but lfsr_file_resync makes this
unnecessary.
More code means more code, but I think it is well worth it for the
additional flexibility:
code stack
before: 36412 2616
after: 36748 (+0.9%) 2616 (+0.0%)
I think the assumption was that since these errors are trivially noops,
they shouldn't change any file state. But this doesn't match the
behavior of other errors, which is inconsistent and probably not what
users expect.
Also added a couple tests around FBIG that should catch this in the
future.
Curiously this actually saved a word of code, I guess because of
rerouting all errors through the same function epilogues:
code stack
before: 36416 2616
after: 36412 (-0.0%) 2616 (+0.0%)
We just have too many flags! Mount flags specifically are already close
to filling up with the currently planned features.
Fortunately the info flags, used internally to track filesystem state,
are never needed at the same time as the traversal flags which specify
one-time traversals during lfsr_mount. So we can move these to overlap
and free up quite a bit more space:
8 8 8 8
.----++----++----++----.
.----..-..-..----------.
o_flags: |type||f||t|| o |
|----||-|:-:'--.-.-----'
|----||-|:-:---:-:-----.
d_flags: |type||f|: : : : |
|----||-|:-:---:-:-----'
|----||-|:-'--..-..----.
t_flags: |type||f|| t ||f||tstt|
'----''-'|----|'-''----'
.--------|----|:-:-----.
gc_flags: | | t |: : |
'--------|----|:-:-----'
.-------.|----|.-------.
f_flags: | m || t || f |
|-------||----|'-------'
|-------||----|:-:.----.
m_flags: | m || t ||o|| m |
|-------|'----'|-||----|
|-------|.----.|-||----|
i_flags: | m || i ||o|| m |
'-------''----''-''----'
The only downside is a bit more masking and not having this info
available when debugging.
The overlap is also convenient for lfsr_fs_gc and lets us remove some
shifts, which humorously perfectly canceled out the added cost of the
masks:
code stack
before: 36416 2616
after: 36416 (+0.0%) 2616 (+0.0%)
Updating the canonical checksum should only depend on if the tag is a
trunkish tag (not a checksum tag), and not if the tag is in the current
trunk. The trunk parameter to lfsr_rbyd_fetch should have no effect on
the canonical checksum.
Fixed in boath lfsr_rbyd_fetch and scripts.
Curiously no code changes:
code stack
before: 36416 2616
after: 36416 (+0.0%) 2616 (+0.0%
Since we already need all the machinery to track ck info for ckparity, I
figured we might as well implement a full ckcksums option as well.
Ckcksums closes the checksum-read-hole by reading enough data to check a
relevant checksum on ever read, even if this ends up being significantly
more data than the initial request. This should always detect detectable
bit-errors, even if they occur between consecutive reads.
If this sounds naive, that's because it is. Performance will be awful.
To be clear, ckcksums should probably never be used in production. I
can't think of a use case that isn't better handled by either ECC in the
block device or the future-planned ckredund feature. Just look at the
runtime complexities:
small-reads rbyd-lookup rbyd-compaction
ckcksums: O(b^2) O(b log b) O(b^2 log b)
ckredund*: O(log_b(n) + xb) O(log b) O(b log b)
eccbd*: O(b) O(log b) O(b log b)
* theoretical
We've already seen that O(b^2) compactions turns a performance problem
into a tractability problem, so I think O(b^2 log b) compactions will be
a bit too much for most applications.
We can already seen this in our test_ck_ckcksums_* tests (which do pass
by the way!). Compare to test_ck_ckprogs_*, which is basically the same
set of tests:
test_ck_ckprogs_*: 6.08s
test_ck_ckcksums_*: 64.88s
Or consider test_rbyd with/without ckcksums:
test_rbyd: 12.21s
test_rbyd+ckcksums: 389.94s
Still, ckcksums is an interesting proof-of-concept, and does manage to
close the checksum-read-hole.
---
Like ckprogs/ckfetches/ckparity/etc, ckcksums is an opt-in feature,
requiring both 1. defining LFS_CKCKSUMS and 2. passing LFS_M_CKCKSUMS at
mount time.
Like ckparity, ckcksums requires a significant code and stack increase
to track ck info in lfsr_data_t:
code stack
before: 36416 2616
yes-ckcksums: 38872 (+6.7%) 3176 (+21.4%)
no-ckcksums: 36416 (+0.0%) 2616 (+0.0%)
It's interesting to note how this compares to all of the current
ck-modes, though each has their own set of tradeoffs:
code stack
default: 36416 2616
ckprogs: 36468 (+0.1%) 2616 (+0.0%)
ckfetches: 36666 (+0.7%) 2648 (+1.2%)
ckparity: 37996 (+4.3%) 3040 (+16.2%)
ckcksums: 38872 (+6.7%) 3176 (+21.4%)
---
Note that even though ckcksums is opt-in, it may still be worth removing
from the codebase in the future, for a couple reasons:
- Every feature, even if unused, adds developer/maintenance burden.
- Ck info is particularly messy with how it interacts with all
lfsr_data_t APIs. Though getting rid of ck info would also require
getting rid of ckparity.
- It's possible for a user to see ckcksums in the codebase,
misunderstand its tradeoffs, enable it, and get the impression that
littlefs itself is just unusably slow.
It's only the 7th byte (first leb128) that can fail to detect single-bit
errors. This is slightly more interesting since we actually test the
parity of a tag, and not just the revision count.
So instead of always reading the parity byte on demand, we read it once
in lfsr_bd_readtag, and store it in an unused bit in lfsr_data/ck_t.
The main reason for this is to avoid rereading that byte all the time.
Though I suppose there is also an ever-so-tiny increase in chance of
catching a bit-error after lfsr_bd_readtag. Assuming RAM is more
reliable than disk...
It also keeps the read-parity-byte mess limited to lfsr_bd_readtag, and
simplifies lfsr_bd_ckprefix/cksuffix a bit, which is nice. Though at the
cost of making lfsr_bd_readtag's API a bit most awkward with the
addition of the ckparity-specific parity_ parameter.
This adds a bit more code, but ends up saving some stack:
code stack
default before: 36412 2616
default after: 36416 (+0.0%) 2616 (+0.0%)
ckparity before: 37900 3048
ckparity after: 37948 (+0.1%) 3032 (-0.5%)
The extra 4-bytes in our non-ckparity build comes from us moving the
saving of the ecksum to after checksum calculation, since we need to
know the parity in the ckparity build. So just compiler noise.
It doesn't really matter, since any bit-errors will be thrown out during
fetch anyways, but checking the parity of revision counts is technically
the wrong thing to do if we care about compatibility with non-ckparity
builds.
Note this lfsr_bd_readck call was immediately followed by ignoring the
LFS_ERR_CORRUPT error and defaulting the revision count to zero.
Thanks allowing lfsr_bd_readck to be more aggressively inlined, this
saves both code and stack:
code stack
default before: 36412 2616
default after: 36412 (+0.0%) 2616 (+0.0%)
ckparity before: 37972 3088
ckparity after: 37900 (-0.2%) 3048 (-1.3%)
The main one being the lfsr_bd_readnext comment. lfsr_bd_readnext _can_
provide checked reads as long as we read the suffix first and use some
crc32c xor tricks:
1. Calculate c_s = crc32c(suffix)
2. Calculate c_p = crc32c(prefix)
3. Calculate c_d = crc32c(c_p, data)
4. Calculate crc = crc32c(c_d, suffix-sized zeros) xor c_s
There's probably some funny business with the init/fini xor, but you get
the idea.
Conveniently, we just never need to use a hypothetical
lfsr_bd_readnextck.
I was toying around with the idea of using lfsr_bd_readnextck to provide
better caching in the case our rcache is big (~= block_size), but it was
getting overly-complicated/problematic, so I'm dropping the idea for
now.
When lfsr_bd_readnext/prognext were introduced, lfsr_bd_read/prog were
rewired through readnext/prognext to save code size. Unfortunately this
came with a tradeoff of stack size thanks to the nested call frames.
Since lfsr_bd_read is pretty much always going to be at the bottom of
our stack hot-path, this was probably not the best tradeoff to make, so
reverting.
Funnily enough, separating prog/prognext ended up saving code size
anyways.
Some other cleanup also helped:
- In lfsr_bd_readnext we were using lfs_min(hint_, d) when d is already
strictly <= hint_.
- In lfsr_bd_prognext we were unnecessarily discarding parts of the
rcache. We prioritize the pcache anyways so this wasn't really
accomplishing anything.
So in the end, these changes saved both code and stack. Win win:
code stack
before: 36464 2672
after: 36412 (-0.1%) 2616 (-2.1%)
Ckparity is pretty flawed in littlefs, for several reasons. The biggest
one being that we can't even reliably detect single-bit errors.
But! It can still provide an extra layer of safety in a system where you
don't care about the extra code/stack cost.
And, for ckreads, performance cost...
Performance isn't a big problem for parity-checking. We can assume
metadata tags are going to relatively small (and can be controlled by
fragment_size). But for data checksums, ckreads risks O(b^2) when
performing many small reads, which can be a bit of a problem.
And since ckreads doesn't really prove anything interesting about the
system anymore, it makes sense to unbundle these two checks, rename
ckreads -> ckparity, and limit it to only checking parity bits.
This way, you can enable ckparity for a bit of extra safety, with a
code/stack cost hit, but without sacrificing performance.
---
I was hoping more code/stack savings, but since we still need to track
parity context in lfsr_data_t, and still need to intercept bd_read/cmp/
cpy calls that reference metadata, we end up needing to keep most of
the ck circuitry around:
code stack
default before: 36464 2672
default after: 36464 (+0.0%) 2672 (+0.0%)
code stack
ckparity before: 38036 3080
ckparity after: 38024 (-0.0%) 3080 (+0.0%)
We even end up still tracking checksum context for bptrs! Maybe we
should just go ahead and add ckcksums as a joke...
When you can't remember what a parameter does, it's probably a good sign
to change the name...
- all -> partial in lfsr_mdir_alloc_
- all -> relocated in relocate loops
This flips the true/false meaning in some places, but had no impact on
code size. Compilers can probably invert the representation of local
bools if it's useful anyways.
Mostly just shuffled code around. I was trying to clean this function up
a bit but didn't really get anywhere.
I did try deduplicating the two lfsr_mdir_commit__ calls, but it only
saves ~8 bytes of code, so I didn't think it was worth making this
function inconsistent with other compaction patterns in the codebase:
code stack
before: 36476 2672
dedup: 36456 (-0.1%) 2672 (+0.0%)
after: 36464 (-0.0%) 2672 (+0.0%)
---
One, uh, dangerously subtle change here is we no longer consider
corruption errors as not "overcompactable". I tried digging through the
commit messages but couldn't find the motivation for this extra check.
It may seem wrong, but trying to overcompact even on corrupt errors
matches our current "we may try a bad block again later" strategy.
Alternative strategies (which are probably more correct) are a TODO
item.
If we expect lfsr_mdir_commit__ to clobber the mdir on failure, no
reason to make a staging copy.
In theory this saves a bit of stack, but we're not on the stack hot-path
so this has no observable impact...
oframe nframe dframe
lfsr_mdir_commit_: 136 152 +16 (+11.8%)
lfsr_mdir_commit__: 176 152 -24 (-13.6%)
And the extra pointer chasing has a cost :/
code stack
before: 36432 2672
after: 36476 (+0.1%) 2672 (+0.0%)
Ckprogs does not suffer from rollback issues! I was too quick to assume
this was the case in test_ck_spam_* (I blame ckfetches), but it just
turned out that the more aggressive bit flip tests found an actual bug!
The bug in question is caused by bit-errors being introduced in multiple
blocks during mdir relocation.
When relocating, we make the false assumption that if
lfsr_mdir_compact__ returns success, the intermediary compaction has
successfully been written to disk. But this is not true until we
write the rest of the commit and flush the pcache. If the remaining
commit fails due to a bit-error, the pcache can end up corrupt and the
intermediary compaction lost.
But why do we care about the intermediary compaction at all after
corruption? Why do we keep updating the mdir every attempted relocation?
We already mark all relevant mdirs as unerased (eoff=-1) in the
top-level lfsr_mdir_commit, so as far as I can tell the only reason for
updating the mdir on error is to propagate mdir.rbyd.weight=0 when the
mdir is empty (LFS_ERR_NOENT).
But this is a bit stupid. Relying on mdir state across function
boundaries on error is incredibly fragile. If instead we consider the
mdir clobbered on any error and move all the implicit mdir.rbyd.weight=0
stuff up into lfsr_mdir_commit, this whole category of problems goes
away.
So yeah, that's what we do now:
- lfsr_mdir_commit__ failed => mdir clobbered
- lfsr_mdir_compact__ failed => mdir clobbered
- lfsr_mdir_commit_ failed => mdir preserved, marked unerased
- lfsr_mdir_commit failed => mdir preserved, marked unerased
---
Curiously, all of these changes ended up with a net-zero cost:
code stack
before: 36432 2672
after: 36432 (+0.0%) 2672 (+0.0%)
No reason to check every btree node twice!
This adds a bit of code in the ckfetches case, but it's well worth it to
avoid unnecessary checks.
It would actually have saved code if ckfetches were unconditional, but
ckfetches are currently still behind a runtime flag even when enabled:
code stack
default before: 36432 2672 (+0.0%)
default after: 36432 (+0.0%) 2672 (+0.0%)
ckfetches before: 36674 2704
ckfetches after: 36682 (+0.0%) 2704 (+0.0%)
This replaces the lfsr_mptr_t struct with simple arrays.
The main motivation for this is C99's strict aliasing. It saves a
decent amount of stack to reference the mdir's internal block array as
an mptr directly, but we were only able to accomplish this in
lfsr_mdir_mptr by violating C99's strict aliasing rules.
The main downside of this is C's wonderful array-to-pointer decay
resulting in more implicit references and chances for things to get
clobbered (the original motivation for lfsr_mptr_t was due to bugs
introduced this way).
If I know one thing about C99's strict aliasing it's that it sure loves
to make code less safe.
No significant code changes, which is probably a good thing:
code stack
default before: 36436 2672
default after: 36432 (-0.0%) 2672 (+0.0%)
ckfetches before: 36674 2704
ckfetches after: 36666 (-0.0%) 2704 (+0.0%)
Like lfsr_data_fetchbtree/branch, lfsr_data_fetchmdir merges both the
data read/decode and fetch steps into a single function that should
hopefully result in better code deduplication.
Unlike lfsr_data_fetchbtree/branch, this is actually a net positive for
code savings. And because we can abuse the mptr in the yet-uninit mdir,
we can even shave off a bit of stack:
code stack
default before: 36456 2680
default after: 36436 (-0.1%) 2672 (-0.3%)
ckfetches before: 36686 2712
ckfetches after: 36674 (-0.0%) 2704 (-0.3%)
These weren't really necessary when btree/branch fetch was nothing more
than tag decoding, but now that we have ckfetches it makes sense to
deduplicate things for a bit of code savings.
One reason I was punting on this was I wasn't really sure if btree/
branch fetch should take decoded fields or the raw lfsr_data_t. We don't
get any code savings with the former, but it's the only API that's
consistent with lfsr_mdir_fetch/lfsr_rbyd_fetch/etc. I was going to go
with lfsr_btree_fetch/fetch_, but fortunately shoving the latter into
the lfsr_data_* namespace solved this dilemma:
- lfsr_branch_fetch - fetches from decoded fields
- lfsr_data_fetchbranch - fetches from raw data + weight
- lfsr_btree_fetch - fetches from decoded fields
- lfsr_data_fetchbtree - fetches from raw data
Unfortunately, lfsr_btree_parent creates a bit of a wrinkle. We don't
want to redundantly fetch the child we're looking for, so we need to
decode and fetch in separate steps. This prevents inlining between these
small functions that could otherwise take place.
And, while they do save a bit of code, the position of these fetch
functions in the stack hot-path end up increasing our total stack
usage when ckfetches are enabled:
code stack
default before: 36428 2680
default after: 36456 (+0.1%) 2680 (+0.0%)
ckfetches before: 36848 2680
ckfetches after: 36686 (-0.4%) 2712 (+1.2%)
But maybe this is just indicative of us not accounting for
shrinkwrapping?
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
Instead of testing every block (which test_badblocks_every already
does) with a single random bit-error, the new test_ck_spam tests
continuously throw bit-errors at the filesystem until it fails.
This should reveal much more interesting failures than flipping a single
bit in the entire device, while also taking less testing time. And we
still have test_badblocks_every to make sure no specific problem blocks
(except the mrootanchor) are missed.
This makes test_ck_spam more similar to test_exhaustion than
test_badblocks_every.
All this being said, these tests are still sort of in stasis until
rollback protection gets sorted out. So we're not actually testing
anything interesting yet...
I've also reverted the test_badblocks -> test_ck dependency, since we
want to keep the longer-running tests near the end of the queue.
These are basically the same as our test_badblock tests, except we
accept LFS_ERR_CORRUPT. This lets us test more checking modes that may
not enable recovery (ckreads, ckfetches, etc).
Well, in theory, at least. The lack of rollback protection gets in the
way of both ckreads and ckfetches, so we're currently only testing
ckprogs, which isn't much of an improvement. At least this gets the
scaffolding in place...
This also inverts the test_ck -> test_badblocks dependency. Now that
these both have exhaustive tests, we might as well limit test_badblocks
to simple erroring erases/progs and let test_ck check the ck checks.
Ckfetches implements what might be your first idea on how to check
checksums in a filesystem: Check each block/mdir on first access
(fetch) to make sure the data is sound.
Unfortunately, there are two problems with this approach, both which
come from the fact that blocks are big and can't fit in RAM:
1. We still have a checksum-read hole.
We can't keep a whole block around in RAM, so reads after a fetch may
need to reread from disk, at which point new bit-errors may slip in
undetected.
This is especially problematic for traversing our rbyds, which
involves a lot of small reads in a block.
2. Ckfetches may have a surprisingly negative performance impact.
Consider the case of reading a large file with a bunch of small
reads. Because we don't cache blocks, each read may need a btree
lookup, and a full block fetch. On paper this can quickly end up
O(b^2), which is not great.
Though this is helped by the file buffer. It will be interesting to
benchmark and see if this theoretical O(b^2) translates to poor
performance in practice.
Note ckreads has this same performance issue.
Still, despite these problems, ckfetches may be useful for cases where
you just want an extra layer of safety, or don't care about the tiny
chance an error is introduced between a fetch an subsequent read.
---
Like ckprogs/ckreads, ckfetches is an opt-in feature, and requires both
1. defining LFS_CKFETCHES, and 2. passing LFS_M_CKFETCHES during mount.
This is a bit of a quick implementation to get testing in place, so the
code cost is probably higher than strictly necessary. If we can refactor
the code internally to avoid all the duplicate lfsr_rbyd_fetchck/
lfsr_bptr_ck calls, we can probably bring this down a bit:
code stack
before: 36428 2680
yes-ckfetches: 36848 (+1.2%) 2680 (+0.0%)
no-ckfetches: 36428 (+0.0%) 2680 (+0.0%)
Oh, and also added lfs_emubd_flipbit to allow tests to manually flip
bits themselves. LFS_EMUBD_BADBLOCK_PROGFLIP is quick to find the above
mentioned checksum-read hole.
This could be done manually with read+erase+prog, but no reason to make
it harder than it needs to be.
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.
Mainly to make space for more shared open/mount flags that are future
planned.
The nice thing about our flags is they don't live on-disk, so we can
always change them whenever we need to.
It gets a bit messy, but this is what the current layout looks like:
8 8 8 8
.----++----++----++----.
.----..-..-..----------.
o_flags: |type||f||t|| o |
|----||-|:-:'--.-.-----'
|----||-|:-:---:-:-----.
d_flags: |type||f|: : : : |
|----||-|:-:---:-:-----'
|----||-|:-'--..-..----.
t_flags: |type||f|| t ||f||tstt|
'----''-'|----|'-''----'
.--------|----|:-:-----.
gc_flags: | | t |: : |
'--------|----|:-:-----'
.----..-.|----|:-:.----.
f_flags: | f ||m|| t |: :| f |
'----'|-||----|:-:'----'
.----.|-||----||-|.----.
m_flags: | i ||m|| t ||o|| m |
|----||-|'----'|-||----|
|----||-|------|-||----|
i_flags: | i ||m| |o|| m |
'----''-'------'-''----'
The main downside of this layout is that some of the traversal flags,
LFS_T_DIRTY/MUTATED, now risk ambiguity with open/mount flags. But I
don't think this is really avoidable as traversals are already using
almost the entire 32-bit encoding space...
No code changes:
code stack
before: 36480 2680
after: 36480 (+0.0%) 2680 (+0.0%)
These fall out quite naturally when you consider that we call
lfsr_mountinited internally to check that our format was successful.
That being said... they don't really do anything right now since we only
write a single mdir:
- LFS_F_COMPACT - The only gc operation that _might_ actually do
something is LFS_F_COMPACT, but only if our fs config exceeds >1/2 the
block size. But I'm not sure littlefs will even be able to write file
metadata if this happens...
- LFS_F_CKMETA - We already check the only mdir by calling
lfsr_mountinited, which implicitly fetches the mrootanchor.
- LFS_F_CKDATA - We uh, don't have any data immediately after
lfsr_format. But I guess it doesn't hurt to keep this around for
consistency, it at least implies CKMETA.
Hopefully these flags will be more interesting if/when we start adding
auxiliary trees to the filesystem, otherwise they may be worth reverting
in the future...
Until then, they at least provide some consistency, and I guess a way to
triply check that format was successful.
---
This could probably be better deduplicated, but calling lfsr_fs_gc from
both lfsr_mount and lfsr_format provides a bit better code organization:
code stack
before: 36448 2680
after: 36480 (+0.1%) 2680 (+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 was only noticed when forcing btrees for other unrelated tests
(INLINED_SIZE=0, CRYSTAL_THRESH=-1), where even simple file writes would
end up with some unaligned fragments the size of our file buffer.
It was hard to notice without forcing btrees, since our crystallization
algorithm has a tendency to fix alignment issues.
The problem was that we weren't bypassing the file buffer correctly when
buffer.size == 0. We relied on the LFS_F_UNFLUSH flag to know if we
could do a bypassing write, but inlined files set the LFS_F_UNFLUSH flag
even for empty files. This led to blocked bypassing writes, attempts
to merge with empty buffers, and unaligned fragments.
To avoid this, lfsr_file_write now checks for buffer.size == 0
explicitly. There may be a better solution, but for now this gets the
job done.
---
To make sure we don't end up with unaligned fragments again in the
future, I've extend the fwrite litmus tests to check for well-aligned
fragments in addition to blocks:
- test_fwrite_simple_litmus_fragments
- test_fwrite_incr_litmus_fragments
These fixes end up adding a bit of code, as checking for both the
unflushed flag and buffer.size == 0 has a cost:
code stack
before: 36424 2680
after: 36452 (+0.1%) 2680 (+0.0%)
But hey, file aren't stuck with unaligned fragments anymore.
I'm not entirely sure what I was thinking when I thought we couldn't use
read hints in lfsr_bd_cmp. It's true read hints will just be clobbered
when ckprogs are enabled, but if ckprogs aren't enabled, and, perhaps
more rarely, rcache_size > pcache_size, we should still be able to
benefit from read hints in lfsr_bd_cmp.
This has a bigger effect on ckreads, where we likely need to read
trailing data to validate checksums/parity bits and can benefit from
earlier reads keeping more data in the rcache.
Curiously this actually saves a bit a code, not sure why that is:
code stack
before: 36428 2680
after: 36424 (-0.0%) 2680 (+0.0%)
One of these was missed during the crc -> cksum rename, so it wouldn't
have even linked correctly. Rather than fixing it I'm just going to drop
these functions.
At some point they were useful for debugging, but with emubd's disk
mirroring and dbgblock.py, it's both easier and more reliable to
find checksums with external scripts.
This moves all ckread-related logic behind the new opt-in compile-time
LFS_CKREADS flag. So in order to use ckreads you need to 1. define
LFS_CKREADS at compile time, and 2. pass LFS_M_CKREADS during
lfsr_mount.
This was always the plan since, even if ckreads worked perfectly, it
adds a significant amount of baggage (stack mostly) to track the
ck context of all reads.
---
This is the first non-trivial opt-in define in littlefs, so more test
framework features!
test.py and build.py now support the optional ifdef attribute, which
makes it easy to indicate a test suite/case should not be compiled when
a feature is missing.
Also interesting to note is the addition of LFS_IFDEF_CKREADS, which
solves several issues (and general ugliness) related to #ifdefs in
expression. For example:
// does not compile :( (can't embed ifdefs in macros)
LFS_ASSERT(flags == (
LFS_M_CKPROGS
#ifdef LFS_CKREADS
| LFS_M_CKREADS
#endif
))
// does compile :)
LFS_ASSERT(flags == (
LFS_M_CKPROGS
| LFS_IFDEF_CKREADS(LFS_M_CKREADS, 0)));
---
This brings us way back down to our pre-ckread levels of code/stack:
code stack
before-ckreads: 36352 2672
ckreads: 38060 (+4.7%) 3056 (+14.4%)
after-ckreads: 36428 (+0.2%) 2680 (+0.3%)
Unfortunately, we do end up with a bit more code cost than where we
started. Mainly due to code moving around to support the ckread
infrastructure:
code stack
lfsr_bd_readtag: +52 (+23.2%) +8 (+10.0%)
lfsr_rbyd_fetch: +36 (+5.0%) +8 (+6.2%, cold)
lfs_toleb128: -12 (-25.0%) -4 (-20.0%, cold)
total: +76 (+0.2%) +8 (+0.3%)
But oh well. Note that some of these changes are good even without
ckreads, such as only parsing the last ecksum tag.
Added some code, though we don't _really_ care:
code stack
before: 37872 3048
after: 38060 (+0.5%) 3056 (+0.3%)
Also interesting to note the difference in testing time, this highlights
_some_ of the performance cost of ckreads:
with ckreads: 1135.92s
without ckreads: 821.24s
Metastability is a rather nasty error condition where successive reads
to a memory location may return different values, either due to bus
issues or a failed prog. It's a tricky error condition to detect, and
one that ckreads was, in theory, supposed to help with.
To help test metastability (and other single-bit errors), emubd gained
several new features:
- LFS_EMUBD_BADBLOCK_PROGFLIP - Prog flips a bit
- LFS_EMUBD_BADBLOCK_READFLIP - Read flips a bit sometimes
- LFS_EMUBD_POWERLOSS_METASTABLE - Reads may flip a bit
These only affect a single bit in a given block, but by randomizing
which bit during every erase (and exhaustive bit testing in test_ck) we
should still see some fairly interesting bit-error patterns over time.
It's a bit difficult to test with more than a single bit error because
you can quickly find checksum/parity collisions when fuzz testing. But
there may be other interesting error patterns to look at in the future?
Also the erase_cycles implementation got a bit of a rework since it was
lopsided previously (progs/reads would always error before erases). And
since I was messing with emubd's internals I added lfs_emubd_markbad/
markgood and a few other convenience functions that seem useful:
- lfs_emubd_seed - Manually set the prng, needed in test_ck actually
- lfs_emubd_markbad - Mark block as bad, same as wear=-1
- lfs_emubd_markgood - Mark block as good, same as wear=0
- lfs_emubd_badbit - Get which big failed
- lfs_emubd_setbadbit - Set which bit will fail
- lfs_emubd_randomizebadbit - Randomize bad bit on erase
- lfs_emubd_markbadbit - Mark bit as bad, same as setbadbit+markbad
---
The intention of this new metastability emulation was to extend test_ck
to test ckreads/ckprogs. This went... interestingly.
The good news, the new emulation and tests worked quite well. They were
able to quite quickly show that ckreads is fundamentally not able to
detect all single-bit errors in our current design.
The problem boils down to the fact that the location of our parity bits
depends on the tag's leb128-encoded size. If a bit flip changes this
size field, we end up with a new parity bit, which 50/50 may or may not
detect the error.
For example, one bit flip:
40 0c 00 12 80 0d ff ff
'----.----' ^--------------------.
'- altble 0xc w0 -18 parity=1
40 0c 80 12 80 0d ff ff
'-------.-------' ^----------------------.
'- altble 0xc w2304 -1664 parity=1
This doesn't make ckreads _completely_ useless, just mostly useless. We
can still use it to check parity bits, but without a systematic proof.
But there's enough problems with ckreads: performance, RAM, code, etc,
that I think it may just be an interesting proof-of-concept and not
something users should actually use. Checking reads in the bd-layer
solves all of these problems...
---
At the very least ckprogs gets better testing, thanks to new tests in
test_ck and the addition of LFS_EMUBD_BADBLOCK_PROGFLIP in
test_badblocks.
The extra testing also found a ckprog/ckread hole in that we don't
ckprog/ckread during lfsr_format! I fixed this by making lfsr_format
always use ckprogs/ckreads if available, but maybe lfsr_format should
take its own set of flags?
Funnily enough this had no impact on code size since it probably just
changed the constant in a constant pool:
code stack
before: 37872 3048
after: 37872 (+0.0%) 3048 (+0.0%)
We should always know the worst-case leb128 size when calling
lfs_toleb128, so it's really a developer-error if lfs_toleb128 results
in a buffer overflow.
This is different from lfs_fromleb128, since we use lfs_fromleb128 to
parse on-disk leb128s, which may be corrupted, incomplete, malformed,
etc.
Changing this to an assert (unreachable really) saves a bit of code
since the compile can eliminate the LFS_ERR_CORRUPT that was only
reachable via developer-error:
code stack
before: 37884 3048
after: 37872 (-0.0%) 3048 (+0.0%)
These pieces of logic were common across the lfsr_bd_readck/cmpck/cpyck/
readtag functions and made sense to break out into their own functions.
It was just a bit tricky to figure out what the internal API should look
like.
This saves a bit of code at the cost of some stack. But it also makes
the code cleaner so this tradeoff is worth it to me:
code stack
before: 38100 3032
after: 37884 (-0.6%) 3048 (+0.5%)
With the adoption of the odd-parity-zero rbyd perturb scheme, it's now
possible to validate individual tag's parity with neighboring valid
bits. This sparked an idea that I previously thought was intractable.
If we:
1. Validate all metadata reads by checking their on-disk parity bits.
2. Validate all data reads by checking their in-metadata checksums.
We end up with a closed system where all reads are checked by at least
a parity bit.
Being able to check all reads is a very valuable filesystem feature, but
difficult for littlefs:
- We need to keep relevant data in RAM while validating checksums.
We can't just validate checksums and then perform a second read as
that creates a hole where new bit-errors may be introduced.
- This is solved in other filesystems by loading and checking whole
blocks in RAM. We just can't do that here.
- Without parity, we would need to check the rbyd's checksum on every
tag read. This would lead to a crazy O(n^2 log n) rbyd compaction
runtime.
Which is why I original thought ckreads was just intractable.
Now, this isn't all sunshine and rainbows. ckreads, as implemented here,
has some deeply concerning flaws:
- A parity bit is, mathematically, the minimum possible error-detection
possible. Is validating reads with only a parity bit sufficient for
real world applications?
- Validating data checksums on every read may have severe performance
implications. We need to read up to the entire block, which can lead
to O(n^2) behavior when performing a lot of small reads in a file.
- In order to validate checksums/parity-bits, we need to know where the
checksums/parity-bits actually are for each piece of data.
Our lfsr_data_t struct provides a surprisingly nice abstraction for
this, but oof is it expensive.
For the added code/stack cost alone, we probably want to eventually make
this an opt-in compile-time feature.
---
Implementation notes:
- This found an actual compiler bug! Turns out increasing lfsr_data_t
from 3-words to 5-words confuses GCC:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101854
- Mid-commit, we may have not actually written the last tag's parity
yet, which is a bit of a problem because we may read the last tag when
building the next trunk!
Fixing this required a whole separate tailck mechanism, which just
tracks in-progress commit's parity bits.
This doesn't help the code/stack cost situation...
- lfsr_bd_read/cmp/cpy all need to be extended to support calculating a
checksum on the side, which is a bit of a mess.
- bptr's cksize/cksum is redundant now, which is going to make
conditional compilation a mess.
- The extra parity byte we need to read makes hint calculation a pain.
Code cost wise... yeah, it's significant. Turns out almost doubling
lfsr_data_t has a significant impact on stack usage. Add in all the
extra code to track checksums/parity-bits and validate checksums/
parity-bits and you got yourself a pretty heavy feature:
code stack
before: 36352 2672
after: 38100 (+4.8%) 3032 (+13.5%)
I've been scratching my head over our rbyd perturb scheme. It's gotten
rather clunky with needing to xor valid bits and whatnot.
But it's tricky with needing erased-state to be included in parity bits,
while at the same time excluded from our canonical checksum. If only
there was some way to flip the checksums parity without changing its
value...
Enter the crc32c odd-parity zero: 0xfca42daf!
This bends the definition of zero a bit, but it is one of two numbers in
our crc32c-ring with a very interesting property:
crc32c(m) == crc32c(m xor 0xfca42daf) xor 0xfca42daf // odd-p zero
crc32c(m) == crc32c(m xor 0x00000000) xor 0x00000000 // even-p zero
Recall that crc32c's polynomial, 0x11edc6f41, is composed of two
polynomials: 0x3, the parity polynomial, and 0xf5b4253f, a maximally
sized irreducible polynomial. Because our polynomial breaks down into
two smaller polynomials, our crc32c space turns out to not be a field,
but rather a ring containing two smaller sub-fields. Because these
sub-fields are defined by their polynomials, one is the 31-bit crc
defined by the polynomial 0xf5b4253f, while the other is the current
parity.
We can move in the parity sub-field without changing our position in the
31-bit crc sub-field by xoring with a number that is one in the parity
sub-field, but zero in the 31-bit crc sub-field.
This number happens to be 0xf5b4253f (0xfca42daf bit-reversed)!
(crcs being bit-reversed will never not be annoying)
So long story short, xoring any crc32c with 0xfca42daf will change its
parity but not its value.
---
An that's basically our new perturb scheme. If we need to perturb, xor
with 0xfca42daf to change the parity, and after calculating/validating
the checksum, xor with 0xfca42daf to get our canonical checksum.
Isn't that neat!
There was one small hiccup: At first I assumed you could continue
including the valid bits in the checksum, which would have been nice for
bulk checksumming. But this doesn't work because while valid bits cancel
out so the parity doesn't change, changing valid bits _does_ change the
underlying 31-bit crc, poisoning our checksum and making everything a
mess.
So we still need to mask out valid bits, which is a bit annoying.
But then I stumbled on the funny realization that by masking our valid
bits, we accidentally end up with a fully functional parity scheme.
Because valid bits _don't_ include the previous valid bit, we can figure
out the parity for not only the entire commit, but also each individual
tag:
80 03 00 08 6c 69 74 74 6c 65 66 73 80
^'----------------.---------------' ^
| | |
v + parity = v'
Or more simply:
80 03 00 08 6c 69 74 74 6c 65 66 73 80
'----------------.----------------' ^
| |
parity = v'
Double neat!
Some other notes:
- By keeping the commit checksum perturbed, but not the canonical
checksum, the perturb state is self-validating. We no longer need to
explicitly check the previous-perturb-bit (q) to avoid the perturb
hole we ran into previously.
I'm still keeping the previous-perturb-bit (q) around, since it's
useful for debugging. We still need to know the perturb state
internally at all times in order to xor out the canonical checksum
correctly anyways.
- Thanks to all of our perturb iterations, we now know how to remove the
valid bits from the checksum easily:
cksum ^= 0x00000080 & (tag >> 8)
This makes the whole omitting-valid-bits thing less of a pain point.
- It wasn't actually worth it to perturb the checksum when building
commits, vs manually flipping each valid bit, as this would have made
our internal appendattr API really weird.
At least the perturbed checksum made fetch a bit simpler.
Not sure exactly how to draw this with our perturb scheme diagrams,
maybe something like this?
.---+---+---+---. \ \ \ \
|v| tag | | | | |
+---+---+---+---+ | | | |
| commit | | | | |
| | +-. | | |
+---+---+---+---+ / | | | |
|v|qp-------------->p>p-->p .
+---+---+---+---+ | . . .
| cksum | | . . .
+---+---+---+---+ | . . .
| padding | | . . .
| | | . . .
+---+---+---+---+ | | | |
|v------------------' | | |
+---+---+---+---+ | | |
| commit | +-. | +- rbyd
| | | | | | cksum
+---+---+---+---+ / | +-. /
|v----------------------' | |
+-------+---+---+ / |
| cksum ----------------'
+---+---+---+---+
| padding |
| |
+---+---+---+---+
| erased |
| |
. .
. .
---
Code changes were minimal, saving a tiny bit of code:
code stack
before: 36368 2664
after: 36352 (-0.0%) 2672 (+0.3%)
There was a stack bump in lfsr_bd_readtag, but as far as I can tell it's
just compiler noise? I poked around a bit but couldn't figure out why it
changed...
So simply calculating the ecksum over the whole prog size, instead of
manually CRCing the leading byte that we need to separately read to
check if we need to perturb.
Yes, this risks two reads instead of the one we need, but it's simpler,
less error prone, and less code. Our caching layer should prevent double
reads like this, so we might as well rely on it.
Saves some code:
code stack
before: 36396 2664
after: 36368 (-0.1%) 2664 (+0.0%)
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?)
The naive implementation is simpler, less code, and more likely to be
correct, each of these are more valuable than speed in our debug
scripts.
We're in Python anyways (no offense Python!).
Plus I think it's good to show that the underlying logic of CRCs aren't
really that complex, at least until we throw optimizations into the mix.
Christopher Haster