This reduces lfsr_ck_t to just the cksize/cksum fields, and moves all of
the compile-time ifdef LFS_CKDATACKSUMS logic up into the relevant
lfsr_data_* functions.
This doesn't solve the lfsr_data_t/lfsr_bptr_t duplication problem,
unfortunately, but does simplify the code base a bit.
No significant code changes:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckdatacksums before: 39240 3008 752
ckdatacksums after: 39232 (-0.0%) 3008 (+0.0%) 752 (+0.0%)
To clarify this only checks data reads, and to makes space for future
theoretical ck-operations:
- ckmetaredund - likely
- ckdataredund - unlikely, expensive
- ckmetacksums - unlikely, expensive
- ckdatacksums - implemented
This also tweaks the relevant mount/format/info flags a bit:
LFS_M_CKPROGS 0x00100000 Check progs by reading back progged data
LFS_M_CKFETCHES 0x00200000 Check block checksums before first use
LFS_M_CKPARITY 0x00400000 Check metadata tag parity bits
LFS_M_CKMETAREDUND+ 0x01000000 Check metadata redund blocks on reads
LFS_M_CKDATAREDUND* 0x02000000 Check data redund blocks on reads
LFS_M_CKMETACKSUMS* 0x04000000 Check metadata checksums on reads
LFS_M_CKDATACKSUMS 0x08000000 Check data checksums on reads
+Planned
*Hypothetical
No code changes.
Unfortunately ckparity has proven itself to be much less useful than
originally thought.
The use of leb128 encoding in our tags means that ckparity can't even
detect single bit-errors reliably. Which raises the question: is
ckparity really worth all of the extra baggage necessary to track parity
in our codebase?
Fortunately we don't have to toss out ckparity entirely!
If we only check parity bits in lfsr_bd_readtag_, instead of on every
read, we still have a reasonable chance of noticing parity errors during
metadata lookups.
This does weaken ckparity, but allows us to drop a lot of lfsr_data_t's
ckparity baggage, at the cost of no longer, uh, unreliably detecting
parity errors during reads?
The limited error detection of ckparity means we can't reliably detect
errors during reads anyways, so we might as well keep the code/RAM/
maintenance implications at a minimum to make ckparity remotely worth
it.
---
Note the significant savings for both LFS_CKPARITY and LFS_CKCKSUMS.
Tracking parity info in lfsr_data_t had a heavy cost:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckparity before: 39700 3048 760
ckparity after: 38476 (-3.1%) 2696 (-11.5%) 760 (+0.0%)
ckcksums before: 39396 3096 760
ckcksums after: 39240 (-0.4%) 3008 (-2.8%) 752 (-1.1%)
This also means lfsr_ck_ckprefix/cksuffix calls always have a ckoff of 0
(bptrs only), which means even more code savings, yay!
So... Long store short, checking metadata cksums is just intractably
slow.
But data cksums?
Yes checking data cksums is still O(b^2), but unlike metadata lookups,
which involve many small backwards reads, data reads are very easy to
cache. So instead of O(b^2), it's more like O(b^2/c), where c is your
rcache size.
Still O(b^2) when c << b, but I'm not sure that's avoidable without
adding more cksums.
At the very least, if you have enough RAM, c == b reduces this to O(b),
which is nice for "large" systems that want hardened reads without a
performance loss.
---
But why bother checking data cksums if we still have a read-hole with
metadata cksums?
Well, while considering the problem in the context of future features, I
noticed something _really interesting_:
- ckredund + metadata - reasonable ✓
- ckredund + data - impractical ✗, parity fanout + O(f+r) is bad
- ckcksums + metadata - impractical ✗, small reads + O(b^2) is bad
- ckcksums + data - reasonable ✓, assuming enough rcache
The current planned design for data redundancy makes it also intractably
slow to check every read, since it would require xoring all blocks that
contribute to the relevant parity block, but this isn't a problem for
metadata redundancy.
So while neither ckredund nor ckcksums can tractably close the read-hole
on their own, it looks like together they will be able to cover
everything without completely sacrificing performance. Neat!
Of course this isn't possible if ckcksums/ckredund imply checking both
metadata and data, so they need to be split apart.
And I don't really see a point in keeping the intractable variants
around in the codebase.
---
Dropping metadata ckcksums also means we can get rid of the ugly
lfsr_bd_ckrbydprefix and lfsr_bd_ckrbydsuffix functions, which were
basically duplicating all of lfsr_rbyd_fetch. That was quite a wart!
This saves a nice chunk of code when ckcksums is enabled:
code stack ctx
default before: 38128 2624 752
default after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
ckparity before: 39724 3048 764
ckparity after: 39700 (-0.1%) 3048 (+0.0%) 760 (-0.5%)
ckcksums before: 40612 3184 772
ckcksums after: 39396 (-3.0%) 3096 (-2.8%) 760 (-1.6%)
In theory this gives callers more flexibility around what file states
they actually care about.
This didn't actually end up saving any code (I guess due to const
propagation?), but is kind of neat:
code stack ctx
before: 38128 2624 752
after: 38128 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
One of the unexpected side-effects of lazy file creation is that
suddenly LFS_O_EXCL doesn't make sense.
The standard definition: "Fail if the file exists", is easy enough to
implement, but doesn't really match what the user expects.
The user expects one of these calls to fail:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
But because we create files lazily (to prevent zero-length files after
powerloss), these both succeed.
---
I considered deferring the "file exists" check until we actually would
create the file, but while this _technically_ satisfies the
exclusitivity requirement, I decided against it as I think it just makes
the API way too confusing:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_close(&lfs, &file_a) => 0;
lfsr_file_close(&lfs, &file_b) => LFS_ERR_EXIST;
---
Instead, a simpler, more pragmatic approach: Fail if the file exists
_or_ if the file is open in a mode that will create the file:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => LFS_ERR_EXIST;
This explicitly does _not_ error on zombie/desync files:
lfsr_file_open(&lfs, &file_a, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
lfsr_file_desync(&lfs, &file_a) => 0;
lfsr_file_open(&lfs, &file_b, "file.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
And it does mean we aren't necessarily guaranteeing the file will be
created, but I think this does more-or-less what the user expects:
- open(a) -> desync(a) -> open(b) -> resync(a) is roughly equivalent to
opening a after creating b, which is perfectly fine with LFS_O_EXCL.
- open(a) -> open(b) (errors) -> desync(a) is one way to not actually
create the file, but is somewhat similar to removing the file after
creation.
If you're using desync files you should probably have a good
understanding of littlefs's sync model anyways.
And of course the user can always sync immediately after open to
guarantee file creation, while opting into the possibility of
zero-length files after powerloss.
Code changes:
code stack ctx
before: 38084 2624 752
after: 38128 (+0.1%) 2624 (+0.0%) 752 (+0.0%)
Making lfsr_tag_isunknown its own function suggests this is a cheap
operation, but it's not really since we need to check against all known
types.
Dropping lfsr_tag_isunknown and changing the relevant logic to fallback
to LFS_ERR_NOTSUP saves a surprising amount of code (ok, not that much,
but still surprising):
code stack ctx
before: 38140 2624 752
after: 38084 (-0.1%) 2624 (+0.0%) 752 (+0.0%)
This is the tradeoff of not erroring on unknown filetypes during mount.
- lfsr_file_open and lfsr_mtree_pathlookup now returns LFS_ERR_NOTSUP
instead of LFS_ERR_NOTDIR/LFS_ERR_ISDIR if it encounters an unkown
filetype.
This gets a bit subtle. You might think LFS_ERR_NOTDIR is reasonable,
but it's possible for our unknown filetype to be something dir-like.
Symlinks are an excellent example.
- lfsr_remove/lfsr_rename now bail with LFS_ERR_NOTSUP if encountering
an unknown filetype.
This conflicts with the POSIX philosophy of remove always being
allowed, but I'm not sure what other option there is. Maybe allowing
removes when mounted with LFS_M_FORCE?
We can't just allow removes by default because of the risk of leaking
resources. Directories being the main example of this (need to clean
up bookmarks).
Maybe leaky filetypes should also set WCOMPAT flags?
Not doing something is cheaper than doing something, so unfortunately
this costs us more than what we saved from dropping the orphan/unknown
scan during mount:
code stack ctx
bail: 38120 2624 725
no-error-no-bail (before): 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%)
error-no-bail (after): 38140 (+0.1%) 2624 (+0.0%) 752 (+0.0%)
But this is probably worth it for the extra flexibility.
The motivation here is to simplify lfsr_mount, but there's a number of
knock-on effects.
For one, lfsr_mount should now be faster on filesystems with large
blocks:
O(nb(log b)(log_b n)) -> O(nb(log_b n))
But we now no longer check if our filesystem contains orphaned
stickynotes or unknown filetypes:
- Orphaned stickynotes turned out to not be a big deal. If we find
orphans we'd need to do a second traversal to remove them anyways (no
mutation allowed in lfsr_mount), so this actually ends up a net
improvement in the found-orphan case.
If anything, doing a traversal on first write sets user expectations
correctly, and can be offloaded with lfsr_fs_mkconsistent or
lfsr_fs_gc.
- Unknown filetypes are a bit more annoying (I actually forgot about
this check), but unknown filetypes that require special care should
probably set WCOMPAT/RCOMPAT flags.
Allowing unknown filetypes is a bit more flexible in cases where a
filesystem image is being shared between drivers with different
features (bootloader + app for example).
Though we should probably add more checks/tests that we're handling
these correctly now that we no longer just bail during mount...
Also renamed LFS_I_HASORPHANS -> LFS_I_UNTIDY.
Not doing something is cheaper than doing something, so this saves a bit
of code:
code stack ctx
before: 38120 2624 752
after: 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%)
As a part of the effort to undo the overuse of the term "orphan".
I can't really think of a better name, and uncreat gets the point
across. At least it matches LFS_O_UNSYNC/LFS_O_UNFLUSH.
Apparently the Uncreated are a race of aliens in the Marvel universe?
I've been unhappy with LFSR_TAG_ORPHAN for a while now. While it's true
these represent orphaned files, they also represent zombied files. And
as long as a reference to the file exists in-RAM, I find it hard to say
these files are truely "orphaned".
We're also just using the term "orphan" for too many things.
Really this tag just represents an mid reservation. The term stickynote
works well enough for this, and fits in with the other internal tag,
LFSR_TAG_BOOKMARK.
We already have lfsr_cat_t so...
lfsr_rattr_t is a pretty fundamental type for littlefs, unfortunately
the name "rattr" is a mouthful. Shortening this to just "rat" hopefully
makes things easier to read at the cost of it being a bit less clear
what lfsr_rat_t actually is.
Though it's possible I've been staring at the dwarf spec (DW_AT_*) for
too long...
Previously, we were using the checksum of the full path to generate
dids. This mostly works, but means that different paths can end up with
different dids for the same directory:
- crc32c("a/b") => 0xfb0b40a3
- crc32c("/a/c/../b") => 0x223404f6
Not the end of the world, but this is the stuff heisenbugs are made of.
Now we instead checksum only the file name, which doesn't have this
problem, and xor with our parent's did to prevent collisions between
same-named files in different directories:
did = parent_did xor crc32c(name)
As an extra plus, this should also trivially work for the theoretical
lfsr_mkdirat function.
Code size unchanged, which is humorous but not surprising:
code stack ctx
before: 38120 2624 752
after: 38120 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
This did not have any benefit code-size wise, and while it may be nice
for lfsr_mtree_pathlookup to take care of orphans, leaving it up to the
upper-layers is both simpler and makes orphan behavior explicit in all
functions.
We don't really have that many functions without special orphan
behavior anyways.
code stack ctx
before: 38124 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
after: 38120 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
This makes it so lfsr_mtree_pathlookup returns LFS_ERR_NOENT if it finds
an orphan, with lfsr_mtree_pathlookup_ providing the original behavior
of returning orphans as though they were normal files.
In theory the deduplication is nice, but in practice the overhead of
multiple function entry-points is just too much:
code stack ctx
before: 38124 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
after: 38124 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
Fortunately, while these two code bases have almost completely diverged
at this point, we can at least reuse the reworked test_paths tests.
Mostly involving corner-cases related to trailing-slashes, these changes
gives us better alignment with POSIX and hopefully fewer surprises for
users. The full details of what's changed is in the v2.10 release notes/
commits.
---
Implementing these changes here required a little bit of backpedaling.
Something that worked quite well upstream was the use of trailing junk
in the path to tell if a parent was not found, path must be dir, etc.
This is a bit more awkward with lfsr_mtree_pathlookup, with everything
taking an explicit name_size, but it greatly simplifies the mess that
was lfsr_mtree_pathlookup's error codes.
Now it's just:
- 0 => file found
- 0, lfsr_path_isdir(path) => dir found
- 0, mdir.mid=-1 => root found
- LFS_ERR_NOENT, lfsr_path_islast(path) => file not found
- LFS_ERR_NOENT, !lfsr_path_islast(path) => parent not found
- LFS_ERR_NOTDIR => parent not a dir
Note the special mdir.mid=-1 case for the root. This was needed since
lfsr_mtree_pathlookup can now return LFS_ERR_INVAL (for empty paths, dot
dots above root, etc).
In theory we could've gotten away with a different error code, but none
of them really make sense for this case.
---
The impact on code size is a bit funny. Modifying the path in-place _is_
a cheaper API, at the cost of being a bit more convoluted, but the extra
logic added for POSIX-alignment cancels this out:
code stack ctx
before: 38100 (-0.1%) 2624 (+0.0%) 752 (+0.0%)
after: 38120 (+0.0%) 2624 (+0.0%) 752 (+0.0%)
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.)
The above-mentioned few things:
- We weren't cleaning up orphans correctly if lfsr_file_open errored.
I think at some point we relied on having no falible operations after
the orphan creation, but various refactoring since moved buffer
allocation after orphan creation.
We could rearrange things so orphan creation is last, but I think it's
safter to just deduplicate file cleanup into the new lfsr_file_close_
function.
- LFS_O_TRUNC prevented attrs from being fetched.
It's easy to see where this went wrong. LFS_O_TRUNC prevents data from
being fetched, but we should still fetch attrs.
This is a bit annoying to fix, for now just added a trunc flag to
lfsr_file_fetch.
Also added a couple tests to catch this if it regresses in the future.
- We tried to fetch attrs on orphans.
This doesn't really hurt anything, but it's a waste of read cycles.
Moving all this stuff around added some code, but lfsr_file_fetch is a
bit easier to read now, which is a good thing:
code stack
before: 38084 2624
after: 38100 (+0.0%) 2624 (+0.0%)
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...
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.
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?
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%)
Christopher Haster