File-attached custom attributes could probably use a bit more testing,
but at the very least this should cover obvious file-broadcasting/
power-loss related issues.
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...
This makes it easier to evaluate the code/stack/etc sizes and run tests
without bringing in all of the outdated code.
I guess this officially makes this branch more-or-less a full rewrite,
though the benefit of commenting vs deleting this code is that it can be
easily pulled back in when useful.
The main benefit is small test ids everywhere, though this is with the
downside of needing longer names to properly prefix and avoid
collisions. But this fits into the rest of the scripts with globally
unique names a bit better. This is a C project after all.
The other small benefit is test generators may have an easier time since
per-case symbols can expect to be unique.
This mostly required names for each test case, declarations of
previously-implicit variables since the new test framework is more
conservative with what it declares (the small extra effort to add
declarations is well worth the simplicity and improved readability),
and tweaks to work with not-really-constant defines.
Also renamed test_ -> test, replacing the old ./scripts/test.py,
unfortunately git seems to have had a hard time with this.
- Removed old tests and test scripts
- Reorganize the block devices to live under one directory
- Plugged new test framework into Makefile
renamed:
- scripts/test_.py -> scripts/test.py
- tests_ -> tests
- {file,ram,test}bd/* -> bd/*
It took a surprising amount of effort to make the Makefile behave since
it turns out the "test_%" rule could override "tests/test_%.toml.test"
which is generated as part of test.py.
Christopher Haster