To avoid the obvious conflict with lfs_attr. Unlike lfsr_rattr_t,
lfs_attr is user facing, so it gets priority.
This name may change in the future if something better comes up, but in
the meantime we need to change the name to _something_.
Is this the reason Linux/BSD/etc call these xattrs?
(Note littlefs's attrs are much more limited than xattrs. We should
_not_ call these xattrs in case we want to add true xattrs in the
future.)
After running into issues with LFS_A_CREAT/EXCL in file-attached custom
attributes, we're left in a really weird place:
- None of lfs_setattr's flags are valid in lfs_attr
- None of lfs_attr's flags are valid in lfs_setattr
I also started thinking about the actual use case for LFS_A_CREAT/EXCL,
and it's really not clear.
littlefs really doesn't care about interprocess communication the same
way POSIX/other filesystem APIs do. We can always rely on integration
layers wrapping up multiple operations in a single mutex, so offering
flexible creation semantics has diminished value. LFS_A_CREAT and
LFS_A_EXCL can both be emulated by calling lfsr_getattr first and
checking its return value.
Thinking ahead to the hypothetical lfsr_set API. The main purpose of
lfsr_set is to provide an API that's easier to use but less powerful
than lfsr_file_open. And adding a flags argument seems to run counter to
that.
For example, if you saw this code with no knowledge of littlefs:
lfsr_setattr(&lfs, "cat", 'a', "meow", 4, 0);
You would probably be surprised that it returns LFS_ERR_NOENT without
additional flags.
I realize Linux sidesteps this with XATTR_CREATE/REPLACE by making 0
default to implicitly creating, but I didn't want to introduce
inconsistent flag behavior like this unless I had to.
---
So for now dropping LFS_A_CREAT/EXCL and flags argument to lfsr_setattr.
Code savings minimal, this was mostly for API ergonomics:
code stack
before: 38104 2624
after: 38084 (-0.1%) 2624 (+0.0%)
Unlike lfsr_setattr/getattr/etc, file-attached custom attributes are
RAM-backed snapshots attached to, well, files, that can be committed
atomically along with the file's contents. Great for power-loss
resilience, but boy does it make a mess of an API.
This API was really where custom attributes needed some TLC.
The biggest change is how file-attached custom attributes interact with
file sync broadcasting.
A common complaint from users is that setting custom attributes did not
update attributes in open file handles. This behavior is _very_
inconsistent with other filesystems and created a lot of confusion.
Since we're nailing down littlefs's snapshot/broadcasting model as a
part of larger changes, it makes sense to also nail down how custom
attributes interact.
In the new model:
- Custom attributes are still in-RAM snapshots. Updates do not
immediately take effect, even across write calls.
- On lfsr_file_sync or lfsr_file_close, custom attributes are written
atomically to disk and broadcasted to all open file handles.
- lfsr_setattr/removeattr also take part in attribute broadcasting. When
called, lfsr_setattr/removeattr updates the attribute on disk and
broadcasts the attribute changes to all open file handles.
- Desynced files do _not_ recieve any attribute broadcasts in the same
way they do not recieve any data broadcasts.
This should hopefully make littlefs behave much more consistently with
other filesystems, while still maintaining a well-defined snapshot and
power-loss properties.
---
The lfs_attr struct also gained several new fields:
// Custom attribute structure, used to describe custom attributes
// committed atomically during file writes.
struct lfs_attr {
// Type of attribute
//
// Note some of this range is reserved:
// 0x00-0x7f - Free for custom attributes
// 0x80-0xff - May be assigned a standard attribute
uint8_t type;
// Flags that control how attr is read/written/removed
uint8_t flags;
// Pointer the buffer where the attr will be read/written
void *buffer;
// Size of the attr buffer in bytes, this can be set to
// LFS_ERR_NOATTR to remove the attr
lfs_ssize_t buffer_size;
// Optional pointer to a mutable attr size, updated on read/write,
// set to LFS_ERR_NOATTR if attr does not exist
//
// Defaults to buffer_size if NULL
lfs_ssize_t *size;
};
Which are useful for several new features:
- lfs_attr now supports LFS_A_RDONLY/WRONLY/RDWR modes.
One of the blockers for attribute broadcasting was in-ROM attributes,
where broadcast updates would hard-fault. But now if you mark in-ROM
attributes as WRONLY, and in-RAM attributes as RDWR, this problem goes
away.
- When opened, lfs_attr now optionally writes the attribute size to the
indirect size field.
No more hacky zero padding and not knowing an attribute's size.
Note this follows the same rules as lfsr_getattr, so it does truncate
if the buffer is too small.
The size field can also be set to NULL, in which case lfs_attr
defaults to the buffer_size. This can be quite useful for pure
ROM-backed attributes.
- Missing attributes are now represented with size=LFS_ERR_NOATTR.
No more zero-sized vs missing attribute ambiguity.
This also makes it possible to remove attributes via lfs_attr, by
setting the size to LFS_ERR_NOATTR manually.
This does lead to a bit of a quirk where buffer_size can be
LFS_ERR_NOATTR, which is a bit weird but at least consistent.
- Changes to lfs_attrs will now always trigger file syncs by default.
Previously, if you changed an attribute, you had to also change the
file's contents for it to get written to disk. As pointed out by users
this is both surprising and difficult to work around.
Solving this is quite tricky since there's no real signalling
mechanism between attribute buffers and littlefs. The best I could
come up with is to read attributes from disk during lfsr_file_sync to
see if anything changed.
At the very least, the new flag LFS_A_LAZY restores the old behavior
in case the extra reads in lfsr_file_sync are problematic.
Though I suspect _most_ calls to lfsr_file_sync immediately follow
intentional changes to a file. It would be interesting to know of
examples where this is not the case...
These new fields do increase the size of lfs_attr, which is a downside,
but thanks to flags fitting in type's padding, this is only an increase
from 3 words (12 bytes) -> 4 words (16 bytes).
---
Other implementation notes:
- I did try to implement LFS_A_CREAT/EXCL in lfs_attr but this proved
to be too messy and inconsistent, so I dropped the idea for now.
The idea was to error with NOATTR/EXIST if the lfs_attr flag in
incompatible with what's on disk, but this led to a lot of complexity
for what is a pretty niche use case.
It's also inconsistent with rdonly attrs, which do _not_ error with
NOATTR during lfsr_file_opencfg, because that would be kind of
annoying.
- Having both `struct lfs_attr` and `lfsr_attr_t` to represent different
things in the codebase is both fragile and confusing. One of these
needs to change, probably `lfsr_attr_t`.
If only I could think of a good name...
One of the nice side-effects of the now-dropped uattr/sattr split was
avoiding this conflict.
- We still need more tests related to how custom attributes interact
with other filesystem operations, but I wanted to get what is
currently working committed, see the TODOs in test_attrs.toml.
All of the new bells and whistles unfortunately do add up.
lfsr_file_sync is also the root of our current stack hot-path, so the
additional attr also adds a bit of stack:
code stack
before: 37116 2608
after: 38104 (+2.7%) 2624 (+0.6%)
Still, having a consistent and flexible API is well worth it.
Though I do think at some point we should add a compile-time option to
opt-out of custom attributes (LFS_NO_ATTR?).
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...
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%)
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%)
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.
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.
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...
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%)
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 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
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%)
These simply imply LFS_O_FLUSH/SYNC on all open writable files.
LFS_M_SYNC is equivalent to MS_SYNCHRONOUS in Linux/etc, while
LFS_M_FLUSH is just provided for consistency.
As pure conveniences, these may seem a bit out of scope for littlefs,
except they are _very_ cheap:
code stack
before: 36356 2664
after: 36356 (+0.0%) 2664 (+0.0%)
Ok, they're not _completely_ free! It just turns out they cost 8 bytes,
and a bit of simplification around flag checking in lfsr_mount saved
8 bytes:
code stack
before: 36356 2664
m_flush/sync: 36364 (+0.0%) 2664 (+0.0%)
mount-no-mask: 36356 (+0.0%) 2664 (+0.0%)
These flags just call lfsr_file_ckmeta/ckdata under the hood, but make
it very easy to check metadata/data when opening a file. As an extra
plus they implicitly close the file on failure, so might make cleanup
easier.
Of course, everything has a cost:
code stack
before: 36368 2664
after: 36424 (+0.2%) 2664 (+0.0%)
These also ruin my previous "you don't pay for what you don't call"
assertion, since runtime flags unfortunately always pull in code.
We should add a compile-time switch for these evntually.
These are basically the same as lfsr_fs_ckmeta/ckdata but limited to a
single file. They may be useful when you need to validate a file but
don't want to bother validating the entire filesystem:
// Check a file for metadata errors
int lfsr_file_ckmeta(lfs_t *lfs, lfsr_file_t *file);
// Check a file for metadata + data errors
int lfsr_file_ckdata(lfs_t *lfs, lfsr_file_t *file);
I've also added test_ck to test these and added some more
lfsr_fs_ckmeta/ckdata tests there. These currently just test simple
full-block clobbering, but we should eventually test more interesting
error patterns.
Unfortunately lfsr_file_ckmeta/ckdata can't reuse the internal
lfsr_mtree_traverse in quite the same way lfsr_fs_ckmeta/ckdata can, so
they're actually a bit more expensive. Though keep in mind with
link-time gc you won't pay the cost unless you call these functions:
code stack
before: 36024 2696
after: 36368 (+1.0%) 2664 (-1.2%)
Oh, and the multiple calls to lfsr_btree/bshrub_traverse apparently
uninlined it out of lfsr_mtree_traverse, saving the stack cost in the
stack hot-path... Yay?
It's really not that much code (36 bytes, and only if you call
lfsr_fs_gc), and implicit state is better the explicit state (less
things that can fall out of sync).
I'm keeping the fancy F/GC flag masking in lfsr_fs_gc though.
Code changes:
code stack
before: 35988 2696
after: 36024 (+0.1%) 2696 (+0.0%)
This is equivalent to the user-facing LFS_I_CANLOOKAHEAD flag, but
explicitly set in lfs_alloc/lfs_alloc_markfree, rather than being
implied.
Usually, I prefer implicit state, as this means less things that can
fall out-of-sync if there is a filesystem bug, but for
LFS_F_CANLOOKAHEAD explicit state might be warranted.
The main benefit is we can take advantage of the matching F/GC bit
patterns to simplify lfsr_fs_gc's progress checks.
This ends up saving a bit of code:
code stack
before: 36048 2696
after: 35988 (-0.2%) 2696 (+0.0%)
If we add CKMETA/CKDATA and eventually REPAIRMETA/REPAIRDATA to the file
open flags, we'll end up with 17 flags total (13 user-facing,
4 internal), which is a bit (heh) too much for a 16-bit flags field!
There are a few ways to solve this, dropping features for one, instead
I've decided to expand the fields flag to 32-bits. Fortunately this was
already the field size for all user-facing fields.
To avoid a RAM increase, I've also shoved the opened-file types and
traversal tstates into the same field.
We have various flags in quite a few places now, here's how
everything fits together:
8 8 8 8
.----++----++----++----.
.----..---..--..-------.
o_flags: |type|| f ||t || o |
|----||---|:--:'-------'
|----||---|:--:--------.
d_flags: |type|| f |: : |
|----||---|:--:--------'
|----||---|:--'--..----.
t_flags: |type|| f || t ||tstt|
'----''---'|-----|'----'
.----------|-----|-----.
gc_flags: | | t | |
'----------|-----|-----'
.-----.---.|-----|.----.
m_flags: | | m || t || m |
'-----|---|'-----'|----|
.----.|---|-------|----|
i_flags: | i || m | | m |
'----''---'-------'----'
Unfortunately, using the full 32-bit flag space highlights that C99's
enum types are kind of garbage...
In C99 enums are strictly signed ints, which means attempting to use
them for 32-bit bit fields overflows. There is no way around this so
I've switched our flag definitions to #defines.
I've kept types as enums for now but I'm keeping my eye on them...
---
The tradeoff of merging the type/btype/tstate/flags fields is that it
takes more code to extract/encode the various subfields. Since these
fields our heavily used in our codebase, this really adds up:
code stack
before: 35888 2696
after: 36048 (+0.4%) 2696 (+0.0%)
At least in theory the type fields can be optimized to a byte load, but
not btype/tstate. Also accessing bits in higher positions may be adding
cost.
Now we consider if it's still possible for the current traversal to make
progress. If it can, we continue with the relevant masked flags,
otherwise we restart. This should prevent us from traversing the
filesystem for no reason.
I also reverted the ckedmeta/ckeddata flags, these ended up just adding
code cost. We're not in the stack hot-path anyways...
Code changes:
code stack
before: 36244 2680
after: 36228 (-0.0%) 2680 (+0.0%)
Also moved ckmeta/ckdata progress into lfs->flags. We have the bits
available so we might as well use them instead of allocating bools on
the stack...
Whether or not to consider ckmeta/ckdata successful when the filesystem
has been mutated is a bit nuanced.
Initially, I thought we trigger a re-traversal, since we may have
introduced new blocks that haven't been checked. But think about it,
where did those blocks come from?
Any new blocks introduced by filesystem mutation will have just been
written. And if a write introduces corruption you probably have bigger
problems...
... Actually as I write this I realized mounting without ckprogs makes
this even more nuanced, but since ckmeta/ckdata is more intended for
data-at-rest error detection I'm going to keep the change for now.
If you want to catch write errors, you really should enable ckprogs.
This is only a problem for lfsr_fs_gc, and the use cases for
ckmeta/ckdata in lfsr_fs_gc will probably catch any write errors on the
next cycle anyways...
Code changes:
code stack
before: 36208 2680
after: 36244 (+0.1%) 2680 (+0.0%)
So instead of configuring gc_steps at mount time (or eventually compile
time), lfsr_fs_gc now takes a steps parameter that controls how much gc
work to attempt:
int lfsr_fs_gc(lfs_t *lfs, lfs_soff_t steps, uint32_t flags);
This API was needed internally to better deduplicate on-mount gc, and I
figured it might also be useful for users to be able to easily change
gc_steps per lfsr_fs_gc call.
I realize this could also be accomplished with the theoretical
lfsr_fs_gccfg, but it's a bit easier to not need a struct every call.
Most likely, depending on project/system, users will always call
lfsr_fs_gc with either 1 (minimal work) or -1 (maximal work), or, worst
case, can define a system-wide GC_STEPS somewhere.
---
Deduplicating on-mount gc work better saved some code, though it's worth
noting this could have been done internally and not exposed to users:
code stack
before: 36476 2680 (+0.0%)
after: 36316 (-0.4%) 2680 (+0.0%)
The main reason for this change is to allow keeping track of existing
known-free blocks while trying to find more free blocks. This makes it
so failed filesystem traversals don't result in negative progress, which
is nice.
This was difficult in the previous lookahead scheme, since we we'd need
to shift the lookahead buffer to keep off=0 rooted at the first bit.
Shifting bytes is relatively easily with memmove, but it gets tricky
when shifting bits:
lookahead before: ???? ???? ???? ??00 1101 0101 00?? ????
^ ^
off off+size
shift: 0011 0101 0100 ???? ???? ???? ???? ????
^ ^
off off+size
traverse: 0011 0101 0100 0000 0000 0000 1100 0000
^ ^
off off+size
Instead, we now just let the lookahead buffer wrap around. No shifting
required:
lookahead before: ???? ???? ???? ??00 1101 0101 00?? ????
^ ^
off off+size
traverse: 0000 0000 1100 0000 1101 0101 0000 0000
^
off
^
off+size
This gets a bit confusing with the lookahead window also wrapping around
disk, but the math works out with enough modulos (if modulos are too
expensive, we should eventually be able to optimize these into simple
bit masks via compile-time config).
In the future, if we move away from the const config struct, it would
also be nice to try to reducing the number of modulos by storing the
lookahead buffer size in bits instead of bytes...
Note that if the lookahead buffer is larger than disk, the lookahead
window will sort of travel around the underlying buffer. This isn't
inherently a problem, but it did cause some bugs.
To avoid similar bit-related problems with zeroing, lfs_alloc_inc now
also zeros bits as we allocate/skip them, so bits should always be zero
when we start a lookahead traversal. Though note we still need to
manually memset the buffer when discarding lookahead state in init/grow.
---
The end result is surprisingly a net savings in terms of code size. I
guess mainly due to dropping all the lfs_alloc_shift calls:
code stack
before: 36472 2680
after: 36412 (-0.2%) 2680 (+0.0%)
These functions provide an easy API for checking all metadata/data
checksums in the filesystem:
// Check the filesystem for metadata errors
int lfsr_fs_ckmeta(lfs_t *lfs);
// Check the filesystem for metadata + data errors
int lfsr_fs_ckdata(lfs_t *lfs);
These are more-or-less the same as calling lfsr_fs_gc with
LFS_GC_CKMETA/CKDATA, but don't involve the gc/traversal-invalidation
machinery, and may be a bit easier for users to pick up.
---
Unfortunately, for simple wrappers, we're again hit with a somewhat
surprising code cost:
code stack
before: 36288 2680
after: 36472 (+0.5%) 2680 (+0.0%)
But I think we can again blame the high overhead of LFS_TRAVERSAL/
lfsr_mtree_gc. We should look into reducing/deduplicating this logic...
These tell littlefs to do the relevant gc work during mount, which may
be more convenient than calling lfsr_mount and then lfsr_fs_gc.
It also implicitly tears down the filesystem on error, which you can
imagine would be quite useful for LFS_M_CKMETA/LFS_M_CKDATA.
Some flags are more useful here than other (is LFS_M_LOOKAHEAD/COMPACT
really useful?), but since we just pass these directly to our traversal
APIs, we might as well support all of them for consistency.
Also note that since these only change mount's behavior, and have no
effect on the rest of the filesystem, these LFS_M_* flags don't have
related LFS_I_* flags and are not returned by lfsr_fs_stat.
---
This added quite a chunk of code, considering that this is entirely for
convenience:
code stack
before: 35932 2680
after: 36280 (+1.0%) 2680 (+0.0%)
But I think this is mostly because our low-level traversal state is
relatively costly to manage. It may be possible to deduplicate this a
bit better...
This has been a long-time coming, mount flags are just too useful for
configuring a filesystem at runtime.
Currently this is limited to LFS_M_RDONLY and LFS_M_CKPROGS, but there
are a few more planned in the future:
LFS_M_RDWR = 0x0000, // Mount the filesystem as read and write
LFS_M_RDONLY = 0x0001, // Mount the filesystem as readonly
LFS_M_STRICT* = 0x0002, // Error if on-disk config does not match
LFS_M_FORCE* = 0x0004, // Ignore compat flags, mount readonly
LFS_M_FORCEWITHRECKLESSABANDON*
= 0x0008, // Ignore compat flags, mount read write
LFS_M_CKPROGS = 0x0010, // Check progs by reading back progged data
LFS_M_CKREADS* = 0x0020, // Check reads via checksums
* Hypothetical
As a convenience, we also return mount flags in the struct lfs_fsinfo's
flags field as their relevant LFS_I_* variants. Though only to match
statvfs, and only because it's cheap, littlefs's API is low-level and we
should expect users to know what flags they passed to lfsr_mount.
As for the new mount flags:
- LFS_M_RDONLY - For consistency with existing APIs, this just asserts
on write operations, which makes it a bit useless... But the info flag
LFS_I_RDONLY may be useful for falling back to a readonly mode if
we encounter on-disk compat issues.
At least if implement the theoretical LFS_UNTRUSTED_USER mode
LFS_M_RDONLY could become a runtime error.
- LFS_M_RDWR - This really just exists to compliment LFS_M_RDONLY and to
match LFS_O_RDONLY/LFS_O_RDWR. It's just an alias for 0, and I don't
think there will ever be a reason to make it non-0 (but I can always
be wrong!).
- LFS_M_CKPROGS - This replaces the check_progs config option and avoids
using a full byte to store a bool.
We should probably also have a compile-time option to compile this out
(LFS_NO_CKPROGS?), but that's a future thing to do.
This ended up adding a surprising bit of code, considering we're just
moving flags around, and noise in lfs_alloc added a bit of stack again:
code stack
before: 35880 2672
after: 35932 (+0.1%) 2680 (+0.3%)
Thinking again of use cases, lfsr_fs_gc provides the perfect API to call
in the background to perform any pending filesystem work. But what if
there's no work to be done? Sure we could just spin forever, but that's
a waste. Especially on devices that can turn on sleep modes to save
power.
To help with this, this commit adds a set of flags to struct lfs_fsinfo
that signals when lfsr_fs_gc can accomplish work:
LFS_I_INCONSISTENT = 0x01, // Filesystem needs mkconsistent to write
LFS_I_NEEDSUPGRADE* = 0x02, // Filesystem needs an upgrade to write
LFS_I_CANLOOKAHEAD = 0x04, // Lookahead buffer is not full
LFS_I_CANPREERASE+ = 0x08, // Pre-erase buffer is not full
LFS_I_UNCOMPACTED = 0x10, // Filesystem may have uncompacted metadata
LFS_I_NEEDSREPAIRMETA+ = 0x20, // Filesystem contains damaged metadata
LFS_I_NEEDSREPAIRDATA+ = 0x40, // Filesystem contains damaged data
*Hypothetical
+Planned
This flags field also provides a useful place internally to store other
filesystem-related flags, currently LFS_F_ORPHANS, though this may be
expanded in the future.
These flags allow users to know exactly what work can/needs to be done
for the filesystem to make progress:
- LFS_I_INCONSISTENT => LFS_GC_MKCONSISTENT or lfsr_fs_mkconsistent
- LFS_I_CANLOOKAHEAD => LFS_GC_LOOKAHEAD
- LFS_I_UNCOMPACTED => LFS_GC_COMPACT
The one is new!
If we complete a compaction-traversal without any mutation, we know
all mdirs/btree nodes have been compacted and future traversals won't
accomplish anything. Of course, we need to clear this bit on
filesystem mutation.
Right now we just pessimistically assume the filesystem is uncompacted
during mount, but in theory we can also figure this out during our
initial mount traversal.
- LFS_GC_CKMETA/CKDATA?
LFS_GC_CKMETA and LFS_GC_CKDATA are a bit trickier. In theory,
LFS_GC_CKMETA/CKDATA will always accomplish something, since time is
the only ingredient necessary to introduce bit errors.
So there isn't really a reasonable flag here. It's entirely up to the
user to decide when to do an LFS_GC_CKMETA/CKDATA traversal.
Code changes:
code stack
before: 35740 2672
after: 35880 (+0.4%) 2672 (+0.0%)
Thinking about use case a bit, most lfsr_fs_gc will be to perform
background work, and can benefit from being incremental.
We already support incremental gc and all the mess associated with
traversal invalidation via the traversal API, so we might as well expose
this through lfsr_fs_gc.
The main downside is that we need to store an lfsr_traversal_t object
somewhere, which is not exactly a cheap struct. I was originally
considering limiting incremental gc to the traversal API for this
reason, but I think the value add of an incremental lfsr_fs_gc is too
compelling... Though we really should add a compile-time option
(LFS_NO_GC? LFS_NO_INCRGC?) to allow users to opt-out of this RAM cost
if they're never going to call this function.
Oh, and lfs_t also becomes self-referential, which might become a
problem for higher-level language users...
---
The incremental behavior of lfsr_fs_gc can be controlled by the new
gc_steps config option. This allows more than one step to be performed
at a time, which may allow for more progress when intermixed with
write-heavy filesystem operations. Setting gc_steps=-1 performs a full
traversal every call, which guarantees always making some amount of
progress.
This adds a bit of code, since we now need to check for/resume existing
traversals. But the real cost is the added RAM to lfs_t, which is
unfortunately wasted if you never call lfsr_fs_gc:
code stack lfs_t
before: 35708 2672 164
after: 35756 (+0.1%) 2672 (+0.0%) 296 (+80.5%)
This just provides a simple, easy-to-call, wrapper over the new
traversal API:
int lfsr_fs_gc(lfs_t *lfs, uint32_t flags);
The main difference from its previous incarnation, is that lfsr_fs_gc
now takes a flags argument to indicate exactly what gc operations to
perform. This gives the user more control, and may also make the API
more robust towards adding new features:
LFS_GC_MTREEONLY = 0x0010, // Only traverse the mtree
LFS_GC_MKCONSISTENT = 0x0020, // Make the filesystem consistent
LFS_GC_LOOKAHEAD = 0x0040, // Populate lookahead buffer
LFS_GC_COMPACT = 0x0080, // Compact metadata logs
LFS_GC_CKMETA = 0x0100, // Check metadata checksums
LFS_GC_CKDATA = 0x0200, // Check metadata + data checksums
LFS_GC_REPAIRMETA+ = 0x0400, // Repair metadata blocks
LFS_GC_REPAIRDATA+ = 0x0800, // Repair metadata + data blocks
+ Planned
Alternatively, gc_flags could have been added as a config option. But
making gc_flags a function argument matches other flag APIs (open
mainly), and is slightly more flexible in that it allows a system to do
different gc operations in different system states (though this could
also be accomplished with the hypothetical lfsr_fs_gccfg, which would
probably be good to add anyways).
Worst case, defining a system-wide define that you always pass to
lfsr_fs_gc accomplishes roughly the same thing.
---
This adds a bit more code, mainly to check if we actually need to
traverse, and to make sure traversals accomplish all of the requested
work.
code stack
before: 35448 2680
after: 35708 (+0.7%) 2672 (-0.3%)
Curiously it also saved a bit of stack, which is a bit silly given this
commit is purely code addition. Apparently something in lfs_alloc and
lfsr_fs_gc is shared, getting uninlined, and messing with the stack
measurement. lfs_alloc is quite sensitive to stack changes after all.
- LFS_O_FLUSH 0x0040 -> 0x0040
- LFS_O_SYNC 0x00c0 -> 0x0080
- LFS_T_CKMETA 0x0100 -> 0x0100
- LFS_T_CKDATA 0x0300 -> 0x0200
This is just simpler and should avoid any surprises for both devs and
users.
This has no impact on code size:
code stack
before: 35448 2680
after: 35448 (+0.0%) 2680 (+0.0%)
After thinking about this for a bit, there are some compelling
motivations for including an incremental LFS_T_MKCONSISTENT:
- Being able to run incremental LFS_T_MKCONSISTENT traversals in
parallel with read-only operations is actually quite enticing.
The only complicated part is maintaining the invalidatable traversal
state, which already exists with lfsr_traversal_t (except the
annoying LFS_F_MUTATED bit).
- While it's not really effective to combine LFS_T_MKCONSISTENT and
LFS_T_LOOKAHEAD traversals, it _is_ possible to combine
LFS_T_MKCONSISTENT with LFS_T_COMPACT, LFS_T_CKMETA,
LFS_T_REPAIRMETA (future), etc.
Really, LFS_T_LOOKAHEAD is the odd one out.
- Making LFS_T_MKCONSISTENT incremental means all filesystem-level
traversals (except lfsr_mount) can be run incrementally. Which is a
nice feature to have when O(n = entire fs) risks being very long
running.
The main downside of LFS_T_MKCONSISTENT (and LFS_T_COMPACT, etc) is that
attempting to run it immediately after mount will likely recursively
trigger a lookahead scan to satisfy block allocation requests -- which
will block the current thread for the duration of the lookahead scan.
But this seems to be more a problem of LFS_T_LOOKAHEAD interacting with
other traversals poorly.
Fortunately, long term, the current plan is to replace the lookahead
buffer with an on-disk block map on disks where the lookahead scan is a
bottleneck. If this gets implemented the problem goes away.
So re-reverting this for now. Worst case we can always re-re-revert this
again in the future. There is already a working implementation, so might
as well see where it goes...
Supporting incremental LFS_T_MKCONSISTENT does add a bit of a code
cost, but there is still some room for deduplicating lfsr_mtree_gc +
lfsr_fs_mkconsistent, which may be interesting:
code stack
before: 35232 2680
after: 35480 (+0.7%) 2680 (+0.0%)
Checking for orphans + other traversal work turned out to mesh much
worse than originally thought:
- Adjusting mids and being able to drop mdirs mid-traversal complicates
traversal quite a bit and has potential to hide difficult to reproduce
bugs.
- Implementing incremental mkconsistent requires it's own separate state
to detect mutation correctly since LFS_T_MKCONSISTENT and
LFS_T_LOOKAHEAD are invalidated by slightly different things.
- If hasorphans=true, we're likely going to find orphans and clobber the
traversal. So it's not really worth trying to opportunistically prove
there are no orphans while doing other traversal operations.
- We don't really want to traverse the mroot/mtree during mkconsistent,
which makes deduplicating these two functions a bit tricky. Doable,
but annoying.
- grms don't involve traversals and are their own separate awkward step
already.
Combine this with the fact that needing to scan for orphans should be
relatively rare in practice -- requiring either a powerloss or a
complicated set of file operations with at minimum 3 desynced files --
and parallel orphan checking starts to look like more trouble than it's
worth...
Instead, we now only check if the hasorphan bit has been set, and if it
has been we just call lfsr_fs_mkconsistent directly. This does a full
traversal in a single step, but at least makes it so traversal +
LFS_T_MKCONSISTENT in a background thread will do any necessary
janitorial work.
This saves a bit code:
code stack
before: 35480 2680
after: 35232 (-0.7%) 2680 (+0.0%)
What seemed like a simple tweak to lfsr_fs_fixorphans, integration into
lfsr_mtree_gc, turned out to be surprisingly annoying.
- We need an additional traversal flag, LFS_F_MUTATED, in order to know
if we intentionally modified the filesystem. This is different from
LFS_F_DIRTY in that we don't invalidate orphan scans:
- LFS_F_DIRTY => invalidate lookahead + orphans
- LFS_F_MUTATED => invalidate lookahead
- We need to break up lfsr_fs_fixorphans to expose lfsr_mdir_fixorphans,
which is probably a good thing for readability.
The interactions with each mdir being associated with a given mid is
not great though, and requires a bit of awkward mid shuffling.
- Unlike LFS_T_COMPACT, LFS_T_MKCONSISTENT introduces more complicated
mid changes, and makes it so mdirs can now be dropped in the middle of
traversal.
This messes with our internal lfsr_mtree_traverse -> lfsr_mtree_gc
control flow, and means a single lfsr_traversal_read call may process
an unbounded number of blocks in rare cases with lots of orphans.
But the good news is things are working, and lfsr_traversal_read with
LFS_T_MKCONSISTENT can scan for orphans in parallel with other traversal
operations.
Adds a bit of code:
code stack
before: 35220 2680
after: 35472 (+0.7%) 2680 (+0.0%)
The tests highlighted that the LFS_I_DIRTY flag in lfsr_tinfo approach
is insufficient. Consider what happens if our filesystem is mutated
while traversing the last mdir:
1. Traversal traverses last mdir, populate blocks, return first block
2. Filesystem mutated, maybe mdir was compacted, clobbers traversal and
sets LFS_I_DIRTY
3. Traversal return LFS_ERR_NOENT immediately, last block never
returned (and out of date), LFS_I_DIRTY never returned
Not only do we miss the LFS_I_DIRTY flag, but we completely miss the
last block in the mdir pair without any warning.
This is _not_ a problem for the actual lookahead buffer, since we still
internally check the LFS_I_DIRTY flag before marking it as complete, but
it is an issue for any external logic that depends on the traversal
being complete...
---
We could revert to LFS_T_EXCL, but, to be honest, I just really don't
know a good name for this flag...
LFS_T_EXCL is a bad name because it conflicts with LFS_O_EXCL. These
flags have very different behaviors, which risks confusing users, and
risks potential name conflicts down the line if we ever want
LFS_T_EXCL-esque semantics for open dirs/files (not unreasonable, though
quite fancy).
My current best contender is LFS_T_WATCH, but while scratching my head
on this, I starting to wonder why we're even providing LFS_T_EXCL in the
first place...
We err on the side of forcing users to implement filesystem-external
features themselves when possible elsewhere, and LFS_T_EXCL technically
_can_ be implemented entirely outside of the filesystem. Though to be
fair it is quite annoying/tedious.
It's not like there's any equivalent feature for dir/file reads anyways.
And a background thread calling lfsr_traversal_read with LFS_T_LOOKAHEAD
will still _eventually_ make progress, even if it takes a bit longer.
Don't get me wrong, I understand it is significantly easier to implement
this inside the filesystem than outside. But it's also easier to
implement this later than right now. And if we implement this later,
hopefully we'll have a better idea what exactly will be useful for
users.
---
Removing LFS_T_EXCL/LFS_I_DIRTY has no real impact on code cost. We were
really just exposing internal logic that we need for lookahead
correctness anyways:
code stack
before: 35224 2680
after: 35220 (-0.0%) 2680 (+0.0%)
This just forwards the internal LFS_I_DIRTY flag to the user via the
lfsr_tinfo flags field.
Benefits of this approach:
- Gives the user more flexibility on what to do if the filesystem is
modified, maybe you want to keep traversing depending on some other
logic.
- Can eventually add other flags to tinfo.flags, such as
LFS_I_COMPACTED, LFS_I_REPAIRED, LFS_I_INCONSISTENT, etc.
- Avoids confusion around the very different behaviors of LFS_O_EXCL and
LFS_T_EXCL.
I tried to come up with a better name (maybe LFS_T_WATCH?) but it was
a bit of a struggle... Switching to a flags approach sidesteps the
issue.
- Can drop the LFS_ERR_BUSY error code for now.
Code changes were fairly insignificant:
code stack
before: 35244 2680
after: 35224 (-0.1%) 2680 (+0.0%)
The only concern is that the tests highlighted it's possible for our
flag scheme to miss mutation if it happens after/during the last set of
blocks... Not sure how to handle this yet...
LFS_O_SYNC always implies LFS_O_FLUSH, otherwise what exactly are you
syncing? Making this explicit in the bit pattern should hopefully make
this clear for curious users, though lfsr_file_flush would be called
anyways because of how lfsr_file_sync is implemented.
This also moves the LFS_O_DESYNC bit pattern around so SYNC/FLUSH are
neighbors. SYNC/DESYNC may seem related, but in lfsr_file_open they
actually are quite different:
LFS_O_FLUSH 0x0040 ---- ---- -1-- ----
LFS_O_SYNC 0x00c0 ---- ---- 11-- ----
LFS_O_DESYNC 0x0100 ---- ---1 ---- ----
Code changes, mostly just noise from moving bits around:
code stack
before: 35228 2680
after: 35244 (+0.0%) 2680 (+0.0%)
It still doesn't make sense to check data without checking metadata, but
keeping this named LFS_T_CKDATA should hopefully clarify what it does
differently from LFS_T_CKMETA.
This implication is also now encoded in the bit pattern:
LFS_T_CKMETA 0x0100 ---- ---1 ---- ----
LFS_T_CKDATA 0x0300 ---- --11 ---- ----
In theory a clever user could force only the CKDATA bit to be set, and
such a configuration would _probably_ work fine, but it won't be
supported just to cut down on possible configurations to test.
No code changes:
code stack
before: 35228 2680
after: 35228 (+0.0%) 2680 (+0.0%)
It's probably a bad reason, but this avoids wasting too much time
figuring out how to name things.
Now most traversal functions return an lfsr_tag_t + lfsr_bptr_t pair,
which is enough to describe the current relevant traversal objects:
tag=LFSR_TAG_MDIR => (lfsr_mdir_t*)bptr.data.u.buffer
tag=LFSR_TAG_BRANCH => (lfsr_rbyd_t*)bptr.data.u.buffer
tag=LFSR_TAG_DATA => bptr.data
tag=LFSR_TAG_BPTR => bptr
This would be a bit better if lfsr_data_t's buffer field was a void*,
but that would mess with byte-level arithmetic, which is more common
with lfsr_data_ts.
This also adopts the fragmented/optional out-params used elsewhere in
the codebase. I thought this would add quite a bit more stack cost,
since we need redundant tags/bptrs to make lfsr_mtree_traverse/
lfsr_mtree_gc work, but surprisingly not:
code stack
before: 35256 2680
after: 35228 (-0.1%) 2680 (+0.0%)
It seems we make up the extra stack cost of redundant tags/bptrs by
giving the compiler more stack-alloc flexibility, tighter per-function
return types, and opting-out of tags/bptrs in most low-level traversals:
lfs_alloc mainly.
But if the fragmented/optional out-params is net harmful for code/stack
size, we should reconsider the pattern system-wide. This does probably
deserve a second look in the future...
This could go either way, it's a case of the classic C strchr type
conundrum.
But unlike iteration, we're more likely to mutate things when doing a
full traversal, so requiring everything to be mutable makes a bit more
sense.
Note that even readonly operations, fetchck for example, need access to
a mutable rbyd struct.
No code changes:
code stack
before: 35256 2680
after: 35256 (+0.0%) 2680 (+0.0%)
Just in case any tags leak through. If an orphan tag ended up in an
lfsr_stat call, it could be quite confusing to users...
Current types:
// user facing
LFS_TYPE_REG 1 ---1
LFS_TYPE_DIR 2 --1-
LFS_TYPE_SYMLINK* 3 --11
// internal
LFS_TYPE_BOOKMARK 4 -1-- -.
LFS_TYPE_ORPHAN 5 -1-1 +- on-disk only
LFS_TYPE_COMPR* 6 -11- -'
LFS_TYPE_TRAVERSAL 9 1--1 <-- in-ram only
* Hypothetical
This has no impact on code size:
code stack
before: 35228 2688
after: 35228 (+0.0%) 2688 (+0.0%)
So now files and traversals contain several nested structs:
file <-- lfsr_file_t
file.o <-- lfsr_obshrub_t
file.o.o <-- lfsr_omdir_t
This gets a bit ugly, but it's really the only way to make the compiler
happy when also with C's annoying strict aliasing rules.
This also makes lfsr_traversal_t a simple alias of lfsr_mtraversal_t,
with lfsr_mtraversal_t now including all of the obshrub/omdir state.
This simplifies things internally, and allows lfsr_mtree_gc to assert on
opened-list enrollment, but risks increased stack cost for all of the
unused fields.
Fortunately this stack cost turned out to not be that significant:
code stack
before: 35264 2680 (+0.0%)
after: 35256 (-0.0%) 2688 (+0.3%)
This adds an indirect pointer to lfsr_btraversal_t, so references to the
btree/bshrub root point to the actual btree/bshrub root rbyd struct.
This means if our bshrub root is mutated due to, say, mdir compaction,
this doesn't necessarily invalidate our btraversal.
But note this is strictly limited to bshrub roots. If you modify any
other part of the bshrub/btree, expect the traversal to be broken.
This means we can do whatever we want with mdirs and not worry about
invaliding bshrub traversals, which is quite nice! It also fixes our
failing bshrub-traversal-mutation tests.
This adds a bit of stack cost, but because we are moving fewer rbyd
structs around in lfsr_btree_traverse_, actually ends up saving a bit of
code. Though we are well below the compiler noise floor:
code stack
before: 35368 2680
after: 35356 (-0.0%) 2688 (+0.3%)
Christopher Haster