This showcases the sort of high-level result printing where -c/--compare
is useful:
$ make summary-diff
code data stack structs
BEFORE 57057 0 3056 1476
AFTER 68864 (+20.7%) 0 (+0.0%) 3744 (+22.5%) 1520 (+3.0%)
There was one hiccup though: how to hide the name of the first field.
It may seem minor, but the missing field name really does help
readability when you're staring at a wall of CLI output.
It's a bit of a hack, but this can now be controlled with -Y/--summary,
which has the sole purpose of disabling the first field name if mixed
with -c/--compare.
-c/--compare is already a weird case for the summary row anyways...
Example:
$ ./scripts/csv.py lfs.code.csv \
-bfunction -fsize \
-clfsr_rbyd_appendrattr
function size
lfsr_rbyd_appendrattr 3598
lfsr_mdir_commit 5176 (+43.9%)
lfsr_btree_commit__.constprop.0 3955 (+9.9%)
lfsr_file_flush_ 2729 (-24.2%)
lfsr_file_carve 2503 (-30.4%)
lfsr_mountinited 2357 (-34.5%)
... snip ...
I don't think this is immediately useful for our code/stack/etc
measurement scripts, but it's certainly useful in csv.py for comparing
results at a high level.
And by useful I mean it replaces a 40-line long awk script that has
outgrown its original purpose...
This may be a (very javascript-esque) mistake, but implicit conversion
to strings is useful when mixing fields and strings in -b/--by field
exprs:
$ ./scripts/csv.py input.csv -bcase='"test"+n' -fn
Note that this now (mostly) matches the behavior when the n field is
unspecified:
$ ./scripts/csv.py input.csv -bcase='"test"+n'
Er... well... mostly. When we specify n as a field, csv.py does
typecheck and parse the field, which ends up sort of canonicalizing the
field, unlike omitting n which leaves n as a string... But at least if
the field was already canonicalized the behavior matches...
It may also be better to force all -b/--by expr inputs to strings first,
but this would require us to know which expr came from where. It also
wouldn't solve the canonicalization problem.
So in:
$ ./scripts/csv.py input.csv -fa='b?c:d'
c and d must have matching types or else an error is raised.
This requires an explicit definition for the ternary operator since it's
a special case in that the type of b does not matter.
Compare to a 3-arg max call:
$ ./scripts/csv.py input.csv -fa='int(b)?float(c):float(d)' # ok
$ ./scripts/csv.py input.csv -fa='max(int(b),float(c),float(d))' # error
The main benefit of this is allowing the sort order to be controlled by
fields that don't necessarily need to be printed:
./scripts/csv.py input.csv -ba -sb -fc
By default this sorts lexicographically, but this can be changed by
providing an expression:
./scripts/csv.py input.csv -ba -sb='int(b)' -fc
Note that sort fields do _not_ change inferred by fields, this allows
sort flags to be added to existing queries without changing the results
too much:
./scripts/csv.py input.csv -fc
./scripts/csv.py input.csv -sb -fc
This was the one piece needed to be able to replace amor.py with csv.py.
The missing feature in csv.py is the ability to keep track of a
running-sum, but this is a bit of a hack in amor.py considering we
otherwise view csv entries as unordered.
We could add a running-sum to csv.py, or instead, just include a running
sum as a part of our bench output. We have all the information there
anyways, and if it simplifies the mess that is our csv scripts, that's a
win.
---
This also replaces the bench "meas", "iter", and "size" fields with the
slightly simpler "m" (measurement? metric?) and "n" fields. It's up to
the specific benchmark exactly how to interpret "n", but one field is
sufficient for existing scripts.
The issue here is quite nuanced, but becomes a problem when you want to
both:
1. Filter results by a given field: -Dmeas=write
2. Output a new value for that field: -bmeas='"write+amor"'
If you didn't guess from the example, this comes up often in scripts
dealing with bench results, where we often find ourselves wanting to
append/merge modified results based on the raw measurements.
Fortunately the fix is relatively easy: We already filter by defines
in our collect function, so we don't really need to filter by defines
again when folding.
Folding occurs after expr evaluation, but collect occurs before, so this
limits filtering to the input fields _before_ expr evaluation.
This does mean we no longer filter on the output of exprs, but I don't
know if such behavior was ever intentionally desired. Worst case it can
be emulated by stacking multiple csv.py calls, which may be annoying,
but is at least well-intentioned and well-defined.
---
Note that the other result scripts, code.py, stack.py, etc, are a bit
different in that they rely on fold-time filtering for filtering
generated results. This may deserve a refactor at some point, but since
these scripts don't also evaluate exprs, it's not an immediate problem.
This may make some mathematician mad, but these are informative scripts.
Returning +-inf is much more useful than erroring when dealing with
several hundred rows of results.
And hey, if it's good enough for IEEE 754, it's good enough for us :)
Also fixed a division operator mismatch in RFrac that was causing
problems.
Not sure if this is an old habit from Python 2, or just because it looks
nicer next to __mul__, __mod__, etc, but in Python 3 this should be
__truediv__ (or __floordiv__), not __div__.
This is now inconsistent with csv.py, and I don't really want to add a
full expr parser to every script that might want to rename fields.
Field renaming (or any expr really!) can be accomplished with
intermediate calls to csv.py anyways. No reason to make these scripts
more complicated than they need to be.
The only reason RFloats reused RInt's operator definitions was to save a
few keystrokes. But this dependency is unnecessary and will get in the
way if we ever add a script that only uses RFloats.
So now the available field exprs can be queried with --help-exprs:
$ ./scripts/csv.py --help-exprs
uops:
+a Non-negation
-a Negation
!a 1 if a is zero, otherwise 0
bops:
a * b Multiplication
a / b Division
... snip ...
I was a bit torn on if this should be named --help-exprs or --list-exprs
to match test.py/bench.py, but decided on --help-exprs since it's
querying something "inside" the script, whereas test.py/bench.py's
--list-cases is querying something "outside" the script.
Internally this uses Python's docstrings, which is a nice language
feature to lean on.
Mainly for consistency with int operators, though it's unclear if either
mod is useful in the context of csv.py and related scripts.
This may be worth reverting at some point.
Now, by default, an error is raised if any branch of an expr has an
inconsistent type.
This isn't always what we want. The ternary operator, for example,
doesn't really care if the condition's type doesn't match the branch
arms. But it's a good default, and special cases can always override the
type function with their own explicit typechecking.
There's a bit of a push and pull when it comes to typechecking CSV
fields in our scripts. On one hand, we want the flexibility to accepts
scripts with various mismatched fields, on the other hand, we _really_
want to know if a typo caused a field to be quietly replaced with all
zeros...
I _think_ it's safe to say: if no fields across _all_ input files match
a requested field, we should error.
But I may end up wrong about this. Worst case we can always revert in
the future, maybe with an explicit flag to ignore missing fields.
- Updated the example in the header comment.
The previous example was way old, from back when fields were separated
by commas! Introduced in 20ec0be87 in 2022 according to git blame.
- Renamed a couple internal RExpr classes:
- Not -> NotNot
- And -> AndAnd
- Or -> OrOr
- Ife -> IfElse
This is mainly to leave room for bitwise operators in case we every
want to add them.
- Added isinf, isnan, isint, etc:
- isint(a)
- isfloat(a)
- isfrac(a)
- isinf(a)
- isnan(a)
In theory useful for conditional exprs based on the field's type.
- Accept +-nan as a float literal.
Niche, but seems necessary for completeness. Unfortunately this does
mean a field named nan (or inf) may cause problems...
I still think the 24 (23+1) char minimum is a good default for 2 column
output such as help text, especially if you don't have automatic width
detection. But our result scripts need to be a bit more flexible.
Consider:
$ make summary
code data stack structs
TOTAL 68864 0 3744 1520
Vs:
$ make summary
code data stack structs
TOTAL 68864 0 3744 1520
Up until now we were just kind of working around this with cut -c 25- in
our Makefile, but now that our result scripts automatically scale the
table widths, they should really just default to whatever is the most
useful.
- RInt/RFloat now accepts implicitly castable types (mainly
RInt(RFloat(x)) and RFloat(RInt(x))).
- RInt/RFloat/RFrac are now "truthy", implements __bool__.
- More operator support for RInt/RFloat/RFrac:
- __pos__ => +a
- __neg__ => -a
- __abs__ => abs(a)
- __div__ => a/b
- __mod__ => a%b
These work in Python, but are mainly used to implement expr eval in
csv.py.
- Allow single-arg frac:
- frac(a) => a/a
- frac(a, b) => a/b
This was already supported internally.
- Implicitly cast to frac in frac ops:
- ratio(3) => ratio(3/3) => 1.0 (100%)
- total(3) => total(3/3) => 3
This makes a bit more sense than erroring.
This now returns 1.0 if the total part of the fraction is 0.
There may be a better way to handle this, but the intention is for 0/0
to map to 100% for thing like code coverage (cov.py), test coverage
(test.py), etc.
So csv.py should now be mostly feature complete, aside from bugs.
I ended up dropping most of the bitwise operations for now. I can't
really see them being useful since csv.py and related scripts are
usually operating on purely numerical data. Worst case we can always add
them back in at some point.
I also considered dropping the logical/ternary operators, but even
though I don't see an immediate use case, the flexibility
logical/ternary operators add to a language is too much to pass on.
Another interesting thing to note is the extension of all fold functions
to operate on exprs if more than one argument is provided:
- max(1) => 1, fold=max
- max(1, 2) => 2, fold=sum
- max(1, 2, 3) => 3, fold=sum
To be honest, this is mainly just to allow a binary max/min function
without awkward naming conflicts.
Other than those changes this was pretty simple fill-out-the-definition
work.
This was more tricky than expected since Python's class scope is so
funky (I just eneded up with using lazy cached __get__ functions that
scan the RExpr class for tagged members), but these decorators help avoid
repeated boilerplate for common expr patterns.
We can even deduplicate binary expr parsing without sacrificing
precedence.
This is a work-in-progress, but the general idea is to replace the
existing rename mechanic in csv.py with a full expr parser:
$ ./scripts/csv.py input.csv -ba=x -fb=y+z
I've been putting this off for a while, as it feels like too big a jump
in complexity for what was intended to be a simple script. But
complexity is a bit funny in programming. Even if a full parser is more
difficult to implement, if it's the right grammar for the job, the
resulting script should end up both easier to understand and easier to
extend.
The original intention was that any sufficiently complicated math could
be implemented in ad-hoc Python scripts that operate directly on the CSV
files, but CSV parsing in Python is annoying enough that this never
really worked well.
But I'm probably overselling the complexity. This is classic CS stuff:
1. build a syntax tree
2. map symbols to input fields
3. typecheck, fold, eval, etc
One neat thing is that in addition to providing type and eval
information, our exprs can also provide information on how to "fold" the
field after eval. This kicks in when merging muliple rows when grouping
by -b/--by, and for finding the TOTAL results.
This can be used to merge stack results correctly with max:
$ ./scripts/csv.py stack.csv \
-fframe='sum(frame)' -flimit='max(limit)'
Or can be used to find other interesting measurements:
$ ./scripts/csv.py stack.csv \
-favg='avg(frame)' -fstddev='stddev(frame)'
These changes also make the eval order of input/output fields much
stricter which is probably a good thing.
This should replace all of the somewhat hacky fake-expr flags in csv.py:
- --int => -fa='int(b)'
- --float => -fa='float(b)'
- --frac => -fa='frac(b)'
- --sum => -fa='sum(b)'
- --prod => -fa='prod(b)'
- --min => -fa='min(b)'
- --max => -fa='max(b)'
- --avg => -fa='avg(b)'
- --stddev => -fa='stddev(b)'
- --gmean => -fa='gmean(b)'
- --gstddev => -fa='gstddev(b)'
If you squint you might be able to see a pattern.
This seems like a more fitting name now that this script has evolved
into more of a general purpose high-level CSV tool.
Unfortunately this does conflict with the standard csv module in Python,
breaking every script that imports csv (which is most of them).
Fortunately, Python is flexible enough to let us remove the current
directory before imports with a bit of an ugly hack:
# prevent local imports
__import__('sys').path.pop(0)
These scripts are intended to be standalone anyways, so this is probably
a good pattern to adopt.
This extends Rbyd.fetch to accept another rbyd, in which case we inherit
the RAM-backed block without rereading it from disk. This avoids an
issue where shrubs can become corrupted if the disk is being
simultaneously written and debugged.
Normally we can detect the checksum mismatch and toss out the rbyd
during fetch, but shrub pointers don't include a checksum since they
assume the containing rbyd has already been checksummed.
It's interesting to note this even avoids the memory copy thanks to
Python's reference counting.
If we're fetching branches anyways, we might as well check that the
checksums match. This helps protect against infinite loops in B-tree
branches.
Also fixed an issue where we weren't xoring perturb state on finding an
explicit trunk.
Note this is equivalent to LFS_M_CKFETCHES in lfs.c.
---
This doesn't mean we always need LFS_M_CKFETCHES. Our dbg scripts just
need to be a little bit tougher because 1. running tests with -j creates
wildly corrupted and entangled littlefs images, and 2. Rbyd.fetch is
almost too forgiving in choosing the nearest trunk.
These work by keeping a set of all seen mroots as we descend down the
mroot chain. Simple, but it works.
The downside of this approach is that the mroot set grows unbounded, but
it's unlikely we'll ever have enough mroots in a system for this to
really matter.
This fixes scripts like dbgbmap.py getting stuck on intentional mroot
cycles created for testing. It's not a problem for a foreground script
to get stuck in an infinite loop, since you can just kill it, but a
background script getting stuck at 100% CPU is a bit more annoying.
A typo meant we were setting all case-level flags to suite-level flags
in bench.py. And because suite-level flags are more-or-less just ored
case-level flags, all case-level flags would end up shared.
Fixed via untypo.
This matches the style used in C, which is good for consistency:
a_really_long_function_name(
double_indent_after_first_newline(
single_indent_nested_newlines))
We were already doing this for multiline control-flow statements, simply
because I'm not sure how else you could indent this without making
things really confusing:
if a_really_long_function_name(
double_indent_after_first_newline(
single_indent_nested_newlines)):
do_the_thing()
This was the only real difference style-wise between the Python code and
C code, so now both should be following roughly the same style (80 cols,
double-indent multiline exprs, prefix multiline binary ops, etc).
Mainly to avoid conflicts with match results m, this frees up the single
letter variables m for other purposes.
Choosing a two letter alias was surprisingly difficult, but mt is nice
in that it somewhat matches it (for itertools) and ft (for functools).
So now the hot path participates in sorting, folding, etc:
$ ./scripts/stack.py ./lfs.ci ./lfs_util.ci \
-Dfunction=lfsr_mount -t -sframe
function frame limit
lfsr_mount 96 2736
|-> lfsr_mdir_commit 512 2368
|-> lfsr_btree_commit__.constprop 336 1648
|-> lfs_alloc 272 1296
|-> lfsr_btree_commit 208 1856
|-> lfsr_btree_lookupnext_ 208 720
|-> lfsr_mtree_gc 192 2560
|-> lfsr_mtree_traverse 176 1024
|-> lfsr_rbyd_lookupnext 160 448
|-> lfsr_bd_readtag.constprop 128 288
|-> lfsr_mtree_lookup 128 848
|-> lfsr_bd_read 80 160
|-> lfsr_bd_read__ 80 80
|-> lfsr_fs_gc 80 2640
|-> lfsr_rbyd_sublookup 64 512
'-> lfsr_rbyd_alloc 16 1312
TOTAL 96 2736
This risks some rather unintuitive behavior now that the hot path
rendering no longer matches the call stack, but in theory the extra
sorting features are more useful?
This is a bit of an experiment, if this is more confusing than useful,
we can always revert to the strict call-order ordering.
Note that you can _usually_ get the call-order ordering by sorting by
limit, but this trick breaks if any call frames are zero sized...
This fixes an issue where mixing recursive renderers (-t/--hot or
-z/--depth) with defines (-Dfunction=lfsr_mount) would not account for
children entry widths. An unexpected side-effect of no longer filtering
the children entries.
We could continue to try to estimate the width without table rendering,
but it would basically need two full recursive pass at this point...
Instead, I've just moved the recursive stuff before table rendering,
which should remove any issues with width calculation while also
deduplicating the recursive passes.
It's invasive for a small change, but probably worthwhile long term.
The downside is this does mean our recursive scripts now build the full
table (including all recursive calls!) before they start printing. When
mixed with unbounded recursive depth (-z0 or --depth=0) this can get
quite large and cause quite a slow start.
But I guess that was the tradeoff in adopting this sort of intermediate
table rendering... At least it does make the code simpler and less bug
prone...
This makes -D/--define more useful in stack.py/perf.py/perfbd.py by no
longer hiding undfined children entries.
For example:
$ ./scripts/stack.py lfs.ci lfs_util.ci -Dfunction=lfsr_mount -t
function frame limit
lfsr_mount 96 2816
|-> lfsr_fs_gc 80 2720
|-> lfsr_mtree_gc 176 2640
|-> lfsr_mdir_commit 576 2464
... snip ...
Now shows all functions in the hot path of lfsr_mount, where before it
would only show functions in the hot path of lfsr_mount that were also
_named_ lfsr_mount.
The previous behavior was technically not wrong... but not very useful
(and confusing).
---
This was actually quite a bit annoying to get working because of the
possibility of function call cycles.
I ended up turning stack.py's result type into a fully connected graph,
which only works because Python has a cycle detector. (Actually this
script is so short-lived we probably wouldn't care if this leaked
memory.)
A nice side effect of this is now all the recursive scripts (stack.py,
perf.py, and perfbd.py) share the same internal result representation
and recursive printing logic, which is probably a good thing.
To better match -z/--depth and -t/--hot.
The fact that these short forms all don't match the first letter of the
long forms is humorous but unintentional. There's only so many letters
in the alphabet!
As a convenience, -d/--diff in our measurement scripts hides entries
that are unchanged by default.
Unfortunately this was broken during a recent refactor that ended up
filtering the line info but not the actual names.
Instead of reverting the broken part of the refactor, I've just moved the
filtering up to where we calculate the names. Hopefully this fixes the
bug while also simplifying this messy chunk of a logic a bit.
This is mainly useful for stack.py, where -t/--hot lets you quickly see
everything that contributes to the stack limit for each function.
This was (and still is) possible with -s + -z, but it was pretty
annoying to use:
- The stack trace rendered _diagonally_ as a consequence of -z, which is
probably the worst use of screen real estate.
- This trick only really worked with -s, which was the opposite order of
what you usually want on the command line: -S.
Adding a special for-purpose -t/--hot flag makes looking at the hot path
much easier, at the cost of more hacky python code (and I _mean_ hacky,
making the hot path selection useful while following exising sort rules
was annoyingly complicated).
Also added -t/--hot to perf.py and perfbd.py for consistency, though it
makes a bit less sense there.
Also also reworked related code in all three scripts: stack.py, perf.py,
perfbd.py. The logic should be a bit more equivalent, and
perf.py/perfbd.py detect cycles now.
This is a pretty classic case for memoization. We don't really need to
recalculate every stack limit at every call site.
Cuts the runtime in half:
before: 0.335s
after: 0.139s (-58.5%)
---
Unfortunately functools.cache was not fit for purpose. It's stuck using
all parameters as the key, which breaks on the "seen" parameter we use
for cycle detection that otherwise has no impact on results.
Fortunately decorators aren't too difficult in Python, so I just rolled
my own (cache1).
stack.py actually already had a simple cycle detector, since we needed
one to calculate stack limits without getting stuck.
Copying this simple cycle detector into the actual table rendering code
lets us print a nice little "cycle detected" message, instead of just
vomiting to stdout forever:
$ ./scripts/stack.py lfs.ci lfs_util.ci -z -s
function frame limit
lfsr_format 320 ∞
|-> lfsr_mountinited 304 ∞
| |-> lfsr_mountmroot 80 ∞
| | |-> lfsr_mountmroot 80 ∞ (cycle detected)
| | |-> lfsr_mdir_lookup 48 576
... snip ...
The cycle detector is a bit naive, just building a new set each step,
but it gets the job done.
As for perf.py and perfbd.py, it turns out they can't actually create
cycles, so no need for a cycle detector. This is good because I didn't
really want to test these scripts again :)
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.)
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...
Christopher Haster