- It didn't save code.
- An inlined buffer is potentially more useful, even if only marginally,
and, uh, unproven yet.
- Requiring lfs_toleb128 in a readonly implementation is a hard ask.
The idea is that we can save on the cost of calling lfs_toleb128
everywhere we commit leb128s, by lazily encoding during progdata.
I original thought this would have too many small problems, but:
1. We can actually implement slice surprisingly easily by just shifting
the internal word 7 bits. This emulates byte-level slicing in the
encoded leb128.
This enables read/cmp, so we can implement all of the lfsr_data_t
functions, though it does make lfs_toleb128 required for a readonly
implementation, which isn't great. Sufficient creativity with ifdefs
likely makes this a non-problem though.
2. There's really very limited use cases for non-leb128 inlined datas.
We can use it to encode the version and compatflags during
lfs_format, but that's about it. And lfs_format is definitely not on
the stack hot-path, so there's no reason to not use on-stack buffers
for these.
The original motivation for this change was noticing a surprising amount
of code savings related to lazy leb128 encoding in another lfsr_data_t
refactor. Unfortunately this savings does not seem reproducible:
code stack
before: 33864 2880
after: 33912 (+0.1%) 2888 (+0.3%)
But that's ok, this is closer to what I expected. The lfs_sizeleb128
call we need to predict the leb128 size is close to the same cost as
calling lfs_toleb128 so the savings isn't really that much.
There wasn't really a collision with this, and I think it's clear what
these flags are doing.
Also fixed a missed renamed of lfsr_tag_issup/subwide ->
lfsr_tag_issup/sub
Implementing raw-byte name comparisons ended up having more negative
effects on implementation requirements than I thought it would:
1. We would never actually concatenate the did + name, as that would
require dynamic memory. Instead we need to express the concatenated
relationship using our internal lfsr_data_t representation.
I thought this wouldn't be too bad since we already have a
concatenated lfsr_data_t representation, but:
1. It was limited in scope, specifically only lfsr_data_prog was
supported. It's actually not even possible to implement
lfsr_data_read (I think) since we can't mutate the indirect
lfsr_data_ts.
2. It's not actually required. We really only use our concatenated
representation to coalesce file fragments. You could in theory
omit this representation at the cost of not being able to limit
inlined shrub overhead.
Asking all future littlefs implementations to implement a
concatenated data representation (or dynamically allocate D:) for the
basic task of file-name lookup is sort of a big ask.
2. A readonly implementation suddenly needs a toleb128 function.
Which is an unexpected implication of requiring raw-byte leb128
comparisons for file-name lookup.
3. Raw-byte comparisons require that dids are always stored in their
canonical encoding (smallest leb128), though this is probably a good
idea anyways.
And for what? A theoretical future-planned feature (content-tree)?
Let's think about the hypothetical content-tree for a second:
1. It's an advanced, opt-in feature. Which means higher code/storage-cost
should be expected.
2. Basicall all littlefs implementations need file-name lookup, so
keeping file-name lookup cheap is a much higher priority than the
opt-int content-tree.
3. Worst case, the content-tree, and any future named trees, can just
set did=0. This will cost one byte per name (and may leave room for
future extensions).
So I'm reverting this for now.
There is still time before stabilization, so if it becomes clear there
is a better way to implement name lookups, we can still change this.
(Optimistically, the content-tree may be implemented before
stabilization, since it currently looks like it's required for data
redundancy).
Code changes:
code stack
before: 34292 2896
after: 34028 (-0.8%) 2896 (+0.0%)
Thanks to poor compound literal optimization, it's actually cheaper to
pass lfsr_data_t by value everywhere, than to make all LFSR_DATA_*
macros lvalues:
before: 34340 2896
after: 34292 (-0.1%) 2896 (+0.0%)
Why are these two design choices linked? If lfsr_data_t is
pass-by-address, the rvalue/lvalue disinction is important because we
need to take the address of LFSR_DATA_* macros. If lfsr_data_t is
pass-by-value, rvalue/lvalue doesn't really matter because we, well,
pass by value.
To be honest, this is a bit of an excuse for better lfsr_data_t
ergonomics. It _is_ generally worse code-size wise to pass lfsr_data_t
by value, because most ABI optimizations stop at 2 words and
lfsr_data_t requires 3 words. But always passing lfsr_data_t by value
even if it is suboptimal makes for more consistent internal interfaces.
This also helps side-step a mistake I made earlier where I though
cat/fromimm/fromleb128 were the only LFSR_DATA_* macros that needed to
be lvalues to be consistent. THERE ARE MANY MORE LFSR_DATA_* macros,
every LFSR_DATA_FROMBLAH macro to be specific, and the resulting code
cost would be MUCH WORSE.
---
This also add lfsr_sprout_t to complement lfsr_bptr_t/lfsr_shrub_t/etc.
Unlike lfsr_data_t, lfsr_sprout_t _is_ pass-by-address
Actually that's the only difference, haha. lfsr_sprout_t is a typedef.
Though to be fair, by being pass-by-addres, lfsr_sprout_t keeps the
internal sprout/shrub/bptr/btree inferfaces consistent, and saves a bit
of code.
This is a simplification of the rbyd/btree layers, but implies
behavioral changes to the mtree/mdir layers.
Instead of ordering by leb128 did + name:
82 02 61 61 61 < 81 04 62 62 62
(0x102, "aaa") (0x201, "bbb")
We now order by the raw encoding, lexicographically:
82 02 61 61 61 > 81 04 62 62 62
(0x102, "aaa") (0x201, "bbb")
This may be unintuitive, but note:
1. Files _within_ a directory are still ordered, since they share a did
prefix.
2. We don't really care about the relative ordering of dids, just
that they are unique. Changing the ordering at this level does not
interfere with any of our did-related functions.
3. The only thing we may care about is that the root, did=0, is the
first mtree entry. This is still true. No leb128 encoding is < 0x00
even after encoding.
The motivation for this change is to allow for other named-btrees in the
system that may used non-did-prefixed names. At least one of these makes
sense for a sort of "content-tree" (cksum -> data block mapping).
As a plus, this change makes it possible to compare names and do btree
namelookups without needing to decode the leb128 prefix. Although I'm
struggling a bit to figure out exactly where this is useful...
One downside, this ordering only works if dids are always stored in
their canonical encoding, that is, the smallest leb128 encoding possible
for a given did. I think this is a reasonable requirement for just our
dids.
Another downside is this did add a decent chunk of code.
I did try limiting the changes to lfsr_data_namecmp, but it didn't have
much impact. I guess most of the cost comes from the reworked
lfsr_data_cmp function, which, to be fair, is quite a bit more
complicated now (it now supports limited data<=>data comparisons):
code stack
before: 34148 2896
namecmp: 34324 (+0.5%) 2896 (+0.0%)
after: 34340 (+0.6%) 2896 (+0.0%)
Before:
LFSR_ATTR(RM(SUBMASK(REG)), 0, BUF("hi", 2))
Now:
LFSR_ATTR(
LFSR_TAG_RM | LFSR_TAG_SUBMASK | LFSR_TAG_REG, 0,
LFSR_DATA_BUF("hi", 2))
Yes, it's more verbose now.
But there were a couple reasons for dropping the idea:
- The implicit prefixing is a bit magical, and not really all that
common in C code. It would likely confuse new users on first read.
- The implicitly prefixing macros did not play will with macro expansion
rules.
In particular, because the nested not-yet-prefixed macros aren't
really macros, they aren't expanded as a part of argument prescan.
This led to surprising compile-time errors, and prevented recursive
attr-lists (which may be useful for shrubs).
- Implicit prefixes is not very C-like, and in particular it gets in the
way of sed/grep operations on source files.
- RM(SUBMASK(REG)) for combining tags is (IMO) ugly, compared to
LFSR_TAG_RM | LFSR_TAG_SUBMASK | LFSR_TAG_REG, even if the latter
requires more typing.
- Sometimes you need runtime-dependent TAG/DATA values, which implicit
prefixing gets in the way of. The LFSR_TAG_TAG(tag)/
LFSR_DATA_DATA(tag) backdoors worked around this, but they are even
more magical, and added noise to a not-actually-all-that-uncommon use
case.
And it's really not _that_ much extra effort to write out the prefixes
everywhere.
lfs.c:
lines bytes
before: 16894 537171
after: 16907 (+0.1%) 538340 (+0.2%)
tests/*.toml:
lines bytes
before: 53306 1811035
after: 54517 (+2.3%) 1851006 (+2.2%)
qadte came in quite handy again for refactoring the tests without
completely losing my sanity.
So instead of:
lfs_cmp(cmp) <= 0
You can do:
cmp <= LFS_CMP_EQ
This is much simpler and still preserves the ability to use all of C's
comparison operators on the results of disk comparisons.
These shims, originally intended to remap the tests to new internal
APIs without a significant rewrite, are a long-outstanding piece of
technical debt. Now that the internal API is more stable, it's time for
that rewrite.
Reasons for not keeping the internal shims:
- They add more complexity to the test suites.
- They come with (out-of-date) constraints that limit what we can test.
- It's more difficult to debug test failures, with 2 layers and all.
I ended up writing a small tree editor out of tree to do most of this
rewrite.
Did it save time? Probably not. But it was quite a bit more fun than
manaully rewriting ~21K lines of code.
Changing insert tags to append seems to have broken insertion into named
btrees in a subtle way.
Consider what happens when we insert immediately before a bid that
splits the btree:
1. namelookup returns the right rbyd, with rid=-1
2. converting this into a bid gives us the left rbyd, with rid=weight
3. the commit to insert the bid ends up inserting into the left rbyd
This doesn't initially seem like an issue, both entries are effectively
the same right? Well, not when you have names. The split name tells you
what _follows_, so this unintentional flipping causes the new name to
get placed in the wrong bucket.
It's not clear if it's possible to fix this, at least not without
inverting the split names to indicate what precedes, but that's a step
too far.
This was not detected earlier because I disabled the low-level
rbyd/btree/mtree tests temporarily due to high porting cost. Guess that
goes to show there's a cost to deferring test ports for too long.
---
This issue, along with being inconsistencies between rids/bids and mids,
and being a relatively unintuitive pattern, is the final nail in the
coffin for insert tags inserting after.
Now, insert tags insert before, like in most other systems, and insert
tags in attr-list just have an implicit +1 before them to allow splits
in attr-lists to work.
This is not a pure revert, as some of the changes with all the code
moving around revealed some better detail-level ideas.
And yes, rbyd/btree tests are up to date now. Unfortunately the mtree
tests require a bit more work.
---
One thing definitely worth noting, btree merges were broken! A mistake
in the has-parent condition meant we were never attempting to merge
btrees!
This hid some bugs in the actual btree merge code caused by mixing the
implicit swap of child rbyds to deduplicate code paths with btree commit
now needing to track bid/rid separately from the attr-list.
This should be fixed now. Interesting to note this bug has been in
lfsr_btree_commit_ for a while now! I think ever since we switched to
using trunks for the has-parent check. We just haven't been merging
btree nodes at all. But since not-merging isn't technically an error,
it's difficult to test for.
Code changes:
code stack
before: 33808 2896
after: 33964 (+0.5%) 2896 (+0.0%)
Found a bug, and maybe a fundamental issue:
- The lfs_btree_lookupnext_ in lfsr_btree_commit_ no longer needs the
min32, since we never commit with bid pointing past the end of the
btree anymore.
This was mixing the unsigned min32 with our now-signed bid type,
causing the wrong btree leaf to be fetched when inserting at bid=-1 in
a non-empty btree.
Easy fix.
- lfsr_btree_commit_ with bid!=-1, rid=-1 (inserting at the beginning of
not-the-first rbyd) now actually appends to the leaf to the left of
the rbyd instead of inserting into the expected rbyd because of how
lfs_btree_lookup_ works.
Initially, this doesn't seem like it would be an issue, these should
be more-or-less equivalent, but this doesn't match
lfsr_btree_namelookup! This is a big problem!
This wasn't noticed because it's rare for the high-level tests to
trigger that many btree splits with names. Named btrees are only used
for the mtree, and we need mdirs to split before the mtree even splits
once.
Not an easy fix.
On the upside, these low-level tests continue to prove themselves
valuable, if tedious to maintain...
Like SUBWIDE, SUPWIDE allows for "mask-like" operation during rbyd
commits, where you replace an entire subrange of tags with a single tag.
- SUBWIDE - Replace all subtypes of the given suptype - Useful for
changing the subtype of an attr, for example replacing a BTREE with a
BSHRUB.
- SUPWIDE - Replace all suptypes of the given rid - Useful for changing
the suptype of an attr, for example replacing a REG file with an
ORPHAN file.
These are effectively the same modifier, just with different ranges.
One benefit is this simplifies mid-level operations a bit, rename,
remove, etc, and decreases the stack cost of the related attr lists.
Though this isn't on the hot-path, so not measurable:
code stack
before: 33956 2912
after: 33928 (-0.1%) 2912 (+0.0%)
But the real motivation for this change is to remove cases where
lfsr_mdir_commit needs to operate on multiple mids. There may be an API
simplification here.
So:
x = (cond) ? yes : no;
Where there are always parentheses around the condition, even if not
required for disambiguity. Additional parentheses are always allowed,
but the parenthesized condition helps signal that a ternary operator is
coming earlier in the expression.
This style has grown on me as I think it helps code readability. It
reminds me of the required parentheses for if/while statements.
Might as well adopt codebase-wide.
So instead of using C's ternary operator everywhere:
(condition)
? LFSR_ATTR(rid, tag, delta, data)
: LFSR_ATTR_NOOP
Use incremental attr allocation instead:
lfsr_attr_t attrs[1];
lfs_size_t attr_count = 0;
if (condition) {
attrs[attr_count++] = LFSR_ATTR(rid, tag, delta, data);
}
LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t));
Incremental attr allocation is more flexible, allowing nested conditions
and conditions that span multiple attrs without sacrificing readability,
though at a verbosity cost.
We already need this for lfsr_btree_commit and lfsr_file_carve, adopting
it everywhere we need conditional attrs allows us to drop the noop attr
and avoid messy and hard-to-read C expressions.
This also changes the lfsr_btree_commit to explicitly omit noop grows.
We were relying on lfsr_rbyd_appendattr implicitly skipping these to
avoid unnecessary attr commits, but I think it's probably better to make
these noops explicit.
This does add some code cost though, I'm guessing sequential conditional
attrs landing at different offsets complicates code generation a bit:
code stack
before: 33940 2928
after: 34052 (+0.3%) 2928 (+0.0%)
This is an attempt to simplify things a bit by moving more logic into
the ftree layer, instead of spreading things around between the
bshrub/bsprout functions.
Now, functionality is organized into high-level ftree operations and
low-level shrub/sprout operations, which only care about the inlined
portion of the shrub/sprout. No more lfsr_bshrub_commit/
lfsr_bshrub_commit__ which were mostly unrelated.
This also adds a lfsr_shrub_t type, which, by taking advantage of the
unused write-related rbyd fields to store the shrub estimate, has the
same size as lfsr_rbyd_t, but can still be casted to an rbyd/btree for
use in readonly rbyd/btree functions.
I considered merging shrub/sprout esimate and shrub/sprout compact into
some sort of ftree_estimate/compact, but it's not obvious what the
benefit would be, so leaving that on the table for now.
---
One nice change is our staging copies are now at the ftree level
(ftree.u and ftree.u_, maybe not the best names, but this is what I've
been using for unions where the name doesn't really matter, god I want
unnamed unions). This simplifies staging, and avoids staging issues
where the underlying type changes.
---
A bit unrelated, but necessary to integrate lfsr_ftree_traverse, a
generalized lfsr_tinfo_t type for all traversal functions was added
(adopted from lfsr_traversal_t really). This is a straightforward tagged
union with relevant traversal types.
The benefit of a generalized tinfo type is better chance we can just
pass the tinfo pointer through multiple layers.
Code changes:
code stack
before: 33368 2984
after: 33260 (-0.3%) 3024 (+1.3%)
We already get the leaf rbyd as a part of btree lookup, and since ids
can't be split across rbyd boundaries, we can be sure any bptr attrs
live in the same rbyd.
This can be extended to any future bptr attrs.
Aside from the small performance gain, this also means we can drop the
btree bid+tag lookups. All extra attr lookups to lookup the rbyd first.
This saves a bit of code but also avoids a set of issues with the btree
semantics where lookupnexting an extra attr can return ENOENT
prematurely when on an rbyd boundary.
As I'm typing this I realize this means we have no way to iterate over
all _tags_ in a btree, only over all _bids_. Fortunately I don't think
we will ever need the former.
code stack
before: 32136 2880
after: 31956 (-0.6%) 2880 (+0.0%)
Much like the erased-state checksums in our rbyds (ecksums), these
block-level erased-state checksums (becksums) allow us to detect failed
progs to erased parts of a block and are key to achieving efficient
incremental write performance with large blocks and frequent power
cycles/open-close cycles.
These are also key to achieving _reasonable_ write performance for
simple writes (linear, non-overwriting), since littlefs now relies
solely on becksums to efficiently append to blocks.
Though I suppose the previous block staging logic used with the CTZ
skip-list could be brought back to make becksums optional and avoid
btree lookups during simple writes (we do a _lot_ of btree
lookups)... I'll leave this open as a future optimization...
Unlike in-rbyd ecksums, becksums need to be stored out-of-band so our
data blocks only contain raw data. Since they are optional, an
additional tag in the file's btree makes sense.
Becksums are relatively simple, but they bring some challenges:
1. Adding becksums to file btrees is the first case we have for multiple
struct tags per btree id.
This isn't too complicated a problem, but requires some new internal
btree APIs.
Looking forward, which I probably shouldn't be doing this often,
multiple struct tags will also be useful for parity and content ids
as a part of data redundancy and data deduplication, though I think
it's uncontroversial to consider this both heavier-weight features...
2. Becksums only work if unfilled blocks are aligned to the prog_size.
This is the whole point of crystal_size -- to provide temporary
storage for unaligned writes -- but actually aligning the block
during writes turns out to be a bit tricky without a bunch of
unecesssary btree lookups (we already do too many btree lookups!).
The current implementation here discards the pcache to force
alignment, taking advantage of the requirement that
cache_size >= prog_size, but this is corrupting our block checksums.
Code cost:
code stack
before: 31248 2792
after: 32060 (+2.5%) 2864 (+2.5%)
Also lfsr_ftree_flush needs work. I'm usually open to gotos in C when
they improve internal logic, but even for me, the multiple goto jumps
from every left-neighbor lookup into the block writing loop is a bit
much...
The idea here is to revert moving redund blocks into lfsr_rbyd_t, and
instead just keep a redundant copy of the rbyd blocks in the redund
blocks in lfsr_mdir_t.
Surprisingly, extra overhead in lfsr_mdir_t ended up with worse stack
usage than extra overhead in lfsr_rbyd_t. I guess we end up allocated
more mdirs than rbyds, which makes a bit of sense given how complicated
lfsr_mdir_commit is:
code stack structs
redund union: 30976 2496 1072
redund in rbyd: 30948 (-0.1%) 2528 (+1.3%) 1100 (+2.6%)
redund in mdir: 31000 (+0.1%) 2536 (+1.6%) 1092 (+1.8%)
The mdir option does seem to improve struct overhead, but this hasn't
been a reliable measurement since it doesn't take into account how many
of each struct is allocated.
Given that the mdir option is inferior in both code and stack cost, and
requires more care to keep the rbyd/redund blocks in sync, I think I'm
going to revert this for now but keep the commit in the commit history
since it's an interesting comparison.
This simplifies dependent structs with redundancy, mainly lfsr_mdir_t,
at a significant RAM cost:
code stack structs
before: 30976 2496 1072
after: 30948 (-0.1%) 2528 (+1.3%) 1100 (+2.6%)
Which, to be honest, is not as bad as I thought it would be. Though it
is still pretty bad for no new features.
The motivation for this change:
1. The organization of the previous lfsr_mdir_t struct was a bit hacky
and relied on exact padding so the redund block array and rbyd block
lined up at the right offset.
2. The previous organization prevented theoretical "read-only rbyd
structs" that could omit write-related fields, e.g. eoff and cksum.
This idea is currently unused.
3. The current mdir=level-1, btree/data=level-0 redund design makes this
RAM tradeoff pretty bad, but in theory higher btree redund levels
would need the extra redund blocks in the rbyd struct anyways.
Still, the RAM impact to the current default configuration means this
should probably be reverted...
As a part of the general redesign of files, all files, not just small
files, can inline some data directly in the metadata log. Originally,
this was a single piece of inlined data or an inlined tree (shrub) that
effectively acted as an overlay over the block/btree data.
This is now changed so that when we have a block/btree, the root of the
btree is inlined. In effect making a full btree a sort of extended
shrub.
I'm currently calling this a "geoxylic btree", since that seems to be a
somewhat related botanical term. Geoxylic btrees have, at least on
paper, a number of benefits:
- There is a single lookup path instead of two, this simplifies code a
bit and decreases lookup costs.
- One data structure instead of two also means lfsr_file_t requires
less RAM, since all of the on-disk variants can go into one big union.
Though I'm not sure this is very significant vs stack/buffer costs.
- The write path is much simpler and has less duplication (it was
difficult to deduplicate the shrub/btree code because of how the
shrub goes through the mdir).
In this redesign, lfsr_btree_commit_ leaves root attrs uncommitted,
allowing lfsr_bshrub_commit to finish the job via lfsr_mdir_commit.
- We don't need to maintain a shrub estimate, we just lazily evict trees
during mdir compaction. This has a side-effect of allowing shrubs to
temporarily grow larger than shrub_size before eviction.
NOTE THIS (fundamentally?) DOESN'T WORK
- There is no awkwardly high overhead for small btrees. The btree root
for two-block files should be able to comfortably fit in the shrub
portion of the btree, for example.
- It may be possible to also make the mtree geoxylic, which should
reduce storage overhead of small mtrees and make better use of the
mroot.
All of this being said, things aren't working yet. Shrub eviction during
compaction runs into a problem with a single pcache -- how do we write
the new btrees without dropping the compaction pcache? We can't evict
btrees in a separate pass becauce their number is unbounded...
We don't strictly need this for the mtree, but its impact is pretty
minimal, and it's useful for some future plans. It also makes low-level
benchmarks a bit easier to write.
The main change involves subtleties around vestigial names in leaf
rbyds (the bottom most layer of btree inner nodes). Since the mtree
terminates in mdirs, the left-most mdir in each leaf rbyd in the mtree
never actually needs a name. But in a hypothetical strict key->value
tree, every entry in the leaf rbyds need a name, and this name needs to
be respected during btree operations (mainly merges).
As a side-effect, our named btrees now require vestigial names for every
inner btree node, with the exception of the left-most inner nodes since
those can't be merged left with anything. On the bright side, being able
to assume a vestigial name on every mergable node does simplify merge
operations a bit.
It's worth noting that despite these changes, we still update vestigial
names on inner btree nodes lazily. It isn't super clear that this should
work, but it turns out that even though a leaf nodes may diverge from
the vestigial name in it's parent, it must still following the bounds of
the parent's vestigial name because of how btree lookups work. And this
property propagates up though each layer in the btree:
.---------------.
|a: |h: |-> |
'--|---|--------'
.---' '----------.
v v
.---------------. .---------------.
|a: |c: | | |i: |m: |-> |
'--|---|--------' '--|---|--------'
...--' | | '--------...
v v
.---------------. .---------------.
|d:0|e:1|f:2|-> | |j:3|k:4|l:5|-> |
'---------------' '---------------'
The exception are the left-most inner nodes, but these can never merge
left, so it doesn't really matter. The vestigial names on the left-most
inner nodes are truly vestigial:
.---------------.
|c: |e: |-> |
'--|---|--------'
.---' '--------...
v
.---------------.
|b: |d: | |
'--|---|--------'
.---' '-------...
v
.---------------.
|a:0|b:1|c:2|-> |
'---------------'
An alternative implementation may prefer to update these names eagerly,
but this would increase the amount of data written to each inner node
during btree commits. mdir updates are lazy by necessity, so even if you
adopted eager updates, the names of deleted files would still stick
around.
This did not turn out to be useful, mainly because type-agnostic
inlining requires unnecessary encoding/decoding and risks a higher RAM
allocation than is really needed. It's better to just reserve a bit in
the weight field and allow higher-level operations to use
operation-specific unions.
code stack
before: 31580 2072
after: 31160 (-1.3%) 2072 (+0.0%)
It's probably better to have a separate names for a tag category and any
specific name, but I can't think of a better name for this tag, and I
hadn't noticed that I was already ignoring the C prefix for CCKSUM tags
in many places.
NAME/CKSUM now mean both the specific tag and tag category, which is a
bit of a hack since both happen to be the 0th-subtype of their
categories.
Note this is already showing better code reuse, which is a good sign,
though maybe that's just the benefit of reimplementing similar logic
multiple times.
Now both reading and carving end up in the same lfsr_btree_readnext and
lfsr_btree_buildcarve functions for both btrees and shrubs. Both btrees
and shrubs are fundamentally rbyds, so we can share a lot of
functionality as long as we redirect to the correct commit function at
the last minute. This surprising opportunity for deduplication was
noticed while putting together the dbg scripts.
Planned logic (not actual function names):
lfsr_file_readnext -> lfsr_shrub_readnext
| |
| v
'---------> lfsr_btree_readnext
lfsr_file_flushbuffer -> lfsr_shrub_carve ------------.
.---------------------' |
v v
lfsr_file_flushshrub -> lfsr_btree_carve -> lfsr_btree_buildcarve
Though the btree part of the above statement is only a hypothetical at
the moment. Not even the shrubs can survive compaction now.
The reason is the new SLICE tag which needs low-level support in rbyd
compact. SLICE introduces indirect refernces to data located in the same
rbyd, which removes any copying cost associated with coalescing.
Previously, a large coalesce_size risked O(n^2) runtime when
incrementally append small amounts of data, but with SLICEs we can defer
coalescing to compaction time, where the copy is effectively free.
This compaction-time-coalescing is also hypothetical, which is why our
tests are failing. But the theory is promising.
I was originally against this idea because of how it crosses abstraction
layers, requiring some very low-level code that absolutely can not be
omitted in a simpler littlefs driver. But after working on the actual
file writing code for a while I've become convinced the tradeoff is
worth it.
Note coalesce_size will likely still need to be configurable. Data in
fragmenting/sparse btrees is still susceptible to coalescing, and it's
not clear the impacts of internal fragmentation when data sizes approach
the hard block_size/2 limit.
My current thinking is that these are conceptually different types, with
BTREE tags representing the entire btree, and BRANCH tags representing
only the inner btree nodes. We already have multiple btree tags anyways:
btrees attached to files, the mtree, and in the future maybe a bmaptree.
Having separate tags also makes it possible to store a btree in a btree,
though I don't think we'll ever use this functionality.
This also removes the redundant weight field from branches. The
redundant weight field is only a minor cost relative to storage, but it
also takes up a bit of RAM when encoding. Though measurements show this
isn't really significant.
New encodings:
btree encoding: branch encoding:
.---+- -+- -+- -+- -. .---+- -+- -+- -+- -.
| weight | | blocks |
+---+- -+- -+- -+- -+ ' '
| blocks | ' '
' ' +---+- -+- -+- -+- -+
' ' | trunk |
+---+- -+- -+- -+- -+ +---+- -+- -+- -+- -'
| trunk | | cksum |
+---+- -+- -+- -+- -' '---+---+---+---'
| cksum |
'---+---+---+---'
Code/RAM changes:
code stack
before: 30836 2088
after: 30944 (+0.4%) 2080 (-0.4%)
Also reordered other on-disk structs with weight/size, so such structs
always have weight/size as the first field. This may enable some
optimizations around decoding the weight/size without needing to know
the specific type in some cases.
---
This change shouldn't have affected functionality, but it revealed a bug
in a dtree test, where a did gets caught in an mdir split and the split
name makes the did unreachable.
Marking this as a TODO for now. The fix is going to be a bit involved
(fundamental changes to the opened-mdir list), and similar work is
already planned to make removed files work.
Ended up changing the name of lfsr_mtree_traversal_t -> lfsr_traversal_t,
since this behaves more like a filesytem-wide traversal than an mtree
traversal (it returns several typed objects, not mdirs like the other
mtree functions for one).
As a part of this changeset, lfsr_btraversal_t (was lfsr_btree_traversal_t)
and lfsr_traversal_t no longer return untyped lfsr_data_ts, but instead
return specialized lfsr_{b,t}info_t structs. We weren't even using
lfsr_data_t for its original purpose in lfsr_traversal_t.
Also changed lfsr_traversal_next -> lfsr_traversal_read, you may notice
at this point the changes are intended to make lfsr_traversal_t look
more like lfsr_dir_t for consistency.
---
Internally lfsr_traversal_t now uses a full state machine with its own
enum due to the complexity of traversing the filesystem incrementally.
Because creating diagrams is fun, here's the current full state machine,
though note it will need to be extended for any
parity-trees/free-trees/etc:
mrootanchor
|
v
mrootchain
.-' |
| v
| mtree ---> openedblock
'-. | ^ | ^
v v | v |
mdirblock openedbtree
| ^
v |
mdirbtree
I'm not sure I'm happy with the current implementation, and eventually
it will need to be able to handle in-place repairs to the blocks it
sees, so this whole thing may need a rewrite.
But in the meantime, this passes the new clobber tests in test_alloc, so
it should be enough to prove the file implementation works. (which is
definitely is not fully tested yet, and some bugs had to be fixed for
the new tests in test_alloc to pass).
---
Speaking of test_alloc.
The inherent cyclic dependency between files/dirs/alloc makes it a bit
hard to know what order to test these bits of functionality in.
Originally I was testing alloc first, because it seems you need to be
confident in your block allocator before you can start testing
higher-level data structures.
But I've gone ahead and reversed this order, testing alloc after
files/dirs. This is because of an interesting observation that if alloc
is broken, you can always increase the test device's size to some absurd
number (-DDISK_SIZE=16777216, for example) to kick the can down the
road.
Testing in this order allows alloc to use more high-level APIs and
focus on corner cases where the allocator's behavior requires subtlety
to be correct (e.g. ENOSPC).
The main purpose of this change is to introduce LFSR_DATA_CAT, a
generalized way to concatenated various data references internally.
As a side-effect lfsr_data_t has been completely restructured. Now,
lfsr_data_t can be in one of 4 modes:
If the size field's sign bit=0, the lfsr_data_t points in-device. A new,
count field, determines the encoding:
sign(size)=0, count=0 => inlined:
.---+---+---+---.
| size |
|---+---+---+---|
|c=0| inlined d | note inlined data is just enough to hold
|---+ | one encoded leb128
| ata... |
'---------------'
sign(size)=1, count=1 => direct:
.---+---+---+---. .---+---+---+---.
| size | .>| data... |
|---+---+---+---| | | . |
|c=1| | | . . .
|---+---+---+---| | . . .
| direct ptr -----' . .
'---------------'
sign(size)=1, count>=2 => indirect:
.---+---+---+---. .---+---+---+---. .---+---+---+---.
| size | .>| size | .>| data... |
|---+---+---+---| | |---+---+---+---| | | . |
|c>1| | | |c=1| | | . . .
|---+---+---+---| | |---+---+---+---| | . . .
| indirect ptr ---' | direct ptr -----' . .
'---------------' '---------------' .---+---+---+---.
| size | .>| data... |
|---+---+---+---| | | . |
|c=1| | | . . .
|---+---+---+---| | . . .
| direct ptr -----' . .
'---+---+---+---'
| . |
| . |
. . .
. .
. .
note only one indirect layer is allowed due to no recursion
If the size field's sign bit=1, the lfsr_data_t points on-disk:
sign(size)=0 => on-disk:
.---+---+---+---. .....
| size | ..'' ''..
|---+---+---+---| : : :
| block ------+->| ..:|
|---+---+---+---| | |......( )::::::|
| off -------' |:::' : |
'---------------' :' : :
''.. :.''
'''''
My goal with this commit was to test the new implementation and see how
it would impact code/RAM size before adopting it in the actual file
handling code, and the results are... not great...
code stack
before: 24668 1840
after: 25552 (+3.5%) 1920 (+4.2%)
I think most of the new cost comes from the now correct handling of
read/cmp with concatentated datas, which previously would just assert.
This change gives us LFSR_DATA_CAT, so I will be working with it for
now, but this may be worth looking at again in the future. Maybe the
correct handling of read/cmp should just be reverted to an assert...
Adopted lfsr_rid/bid/mid/did_t where appropriate. This includes using
lfsr_rid_t for tag/rbyd weights. Although I am using lfsr_srid_t for
rbyd weights now, since it both captures the use of the sign bit and
reduces the number of casts a bit in the code.
I learned recently Zig has any-bit integers (e.g. uint31_t), and I'm
realizing how nice it would be to have those in this codebase.
Also tried to use lfs_size_t/lfs_off_t more correctly. In Linux/BSD,
only off_t is used for file-size-related operations and is usually much
larger than size_t. These were used interchangably in littlefs and their
original meaning kind of fell by the wayside. Getting their use right
will be important if littlefs ever supports different integer widths.
There is a bit of redundancy here, as we already know the weights of
btree's inner-branches from their parents. But in theory sharing the
same encoding for both the top level btree reference and inner-branches
should offer more chance for deduplication and hopefully less code.
This also moves some members around in the btree encoding so that the
redund blocks are at the beginning. This _might_ simplify decoding of
the variable-length redund blocks at some point.
Current btree encoding:
.----+----+----+----.
| blocks ... redund leb128s (1-20 bytes)
: :
|----+----+----+----|
| trunk ... 1 leb128 (1-5 bytes)
|----+----+----+----|
| weight ... 1 leb128 (1-5 bytes)
|----+----+----+----|
| cksum | 1 le32 (4 bytes)
'----+----+----+----'
This also partially reverts some tag name changes:
- BNAME -> BRANCH
- DMARK -> BOOKMARK
Our rbyds support changing the weight of a tag without knowing the
actual tag. This is useful for btrees, which always make weight changes
without knowing if the leading tag is a name or a branch (it depends on
the type of btree).
But to make this work, it needs the rm-bit to be set. This is because
internally the rm-bit indicates we don't want to write-out a tag. Which
we don't for grow tags, because, well, they're not real tags.
Previously this was done by putting LFSR_TAG_GROW(RM) everywhere a
generic grow as needed, but since this is so common we might as well
just set the rm-bit in LFSR_TAG_GROW.
Note that LFSR_TAG_GROW(tag) (the macro) does not set the rm-bit.
This makes the code a bit more readable at the risk of an unintuitive
relationship between LFSR_TAG_GROW and LFSR_TAG_GROW(tag).
Thanks to lazy merging, our btree nodes can drop to zero weight at
pretty much any time. Unfortunately, we can't really represent non-root
zero weight btree nodes, so things break. (Though even if we could,
those nodes would become unreachable).
Previously we relied on fuzz testing to try to catch these cases, but
that turned out to be insufficient.
This adds explicit tests covering the cases where btree drops can occur,
thanks to the realy-big-attr trick used in similar mtree tests.
Sure enough this revealed a bug that can occur when we split a btree
node at the same time one of the siblings goes to zero weight. (Remember
splits carried out before playing attr-lists).
---
Fortunately this is pretty easy to fix. We can just reroute our split
code to the normal commit/compact recursion handling if one of our
siblings drops to zero, at the cost of some spaghetti.
xkcd.com/292 seems relevant here.
With the introduction of lfsr_data_t, these stopped being useful
functions for littlefs internally.
Maybe these tests should be rewritten to use the *_lookup functions
directly? Unfortunately with the quantity of tests we have now this adds
non-trivial amount of work with questionable benefit.
These functions are no longer needed in lfs.c. They are still needed for
the tests as they are written, but that's not a reason to pollute the
littlefs source code.
Maybe these tests should be rewritten to use lfsr_btree_commit directly?
Unfortunately with the quantity of tests we have now this adds
non-trivial amount of work with questionable benefit.
This adds an extra bid parameter to lfsr_rbyd_appendall so that attrs
relative to a bid can be adjusted correctly.
This allows us to make attr-lists const again, which is generally a good
things. Passing around complex mutable state is just asking for bugs.
Though since these attr-lists are generally just passed as temporary
arguments, maybe it's not that bad?
The idea here: Instead of having unique functionality for each
individual btree operation (push/set/pop/split), we treat btrees sort of
like rbyds, with a single commit entry point that operates on attr-lists.
This adds code cost, due to needing to parse the attr-list for properties
that can affect inlined btrees (tag changes mostly), but, in theory, comes
with some advantages:
1. A single btree commit entry point with all of the inlined/uninlining
logic should offer better chances for code deduplication, vs
spreading this logic out in each btree operation.
2. Higher-levels should know what the current weight of the branch is,
so we may be able to avoid the implicit math needed to calculate
deltas.
3. Higher-levels have more knowledge about the state of the btree in
general, so there may be other shortcuts. The mtree, for example,
only operates on weight=1 entries, which greatly simplifies a lot of
the related math.
Note that btrees still have strict limits in what's possible in an
attr-list. Btree operations can't cross leaf-rbyd boundaries for
example.
---
A notable omission in this change is the loss of reinlining btrees.
This wase dropped for a couple reasons. It may be worth adding back at a
later time, maybe after we actually have files implemented, but for now
does not seem worth it:
1. Reinlining adds code cost. Reinlining is more complex than you might
expect because we only reinline on compaction. And because we compact
before playing out our attr-list, we need to know if a commit makes
the btree inlinable before committing to the btree.
This is still doable with our attr-lists. We already derive the
change in tags, since we need this to know when to uninline. But it
adds a kind of complex bailing out of btree commits.
2. The benefits of reinlining may not be that great. In most systems, a
tree that is uninlined once is likely to be uninlined again. It's
only if there is a bigger state change in a system that it makes
sense to reinline.
Though, to be fair, waiting for compaction to reinline handled this
quite well. Only reinlining when all erased storage is used up...
3. Thanks to our roots did entry, our mtree can never reinline.
It would be nice to change this, but this would require explicit
handling in lfsr_mdir_commit. Future work?
4. Files are another can of worms, with more complex interactions with
inlinability thanks to (at least on paper right now) always having
inlined data even when uninlined.
If reinlining is valuable for files this can change during that work.
5. Even if files never support reinlinability, truncating files (via
either lfsr_file_truncate or LFSR_O_TRUNC) should give the file a
blank slate, effectively reinlining the file in that case.
---
The current implementation also changes the attr-list to be mutable so
we can adjust attr-list based on the current btree node. This is a
temporary hack! We should add the appropriate functionality to our rbyd
utilities to revert this eventually.
These tests, and this feature really, is a bit tricky since our btrees
reinline "lazily". That is, our btrees only check if they can inline
during compaction, allowing potentially inlinable btrees to remain
uninlined.
This better utilizes any erased storage in the btree's rbyd, but adds
some corner cases we need to be concerned about.
Added because of some ongoing btree rewrite work, where it did catch
incorrect behavior.
Realistically, because our btree is protected by CoW checksums, the only
place we can end up with a cycle is in our mroot chain.
This is convenient, as we don't need our btree traversal state when
traversing the mroot chain, so we can put both the tortoise state and
btree traversal state into a union, theoretically saving some RAM.
Unfortunately stack measurements show no change, even though our mtree
traversal in on the hot path. I'm not sure why this is. My best guess is
that the RAM savings is beneath the compilation noise floor, since we
currently only ever create one of these structs.
Composable parsing functions always feel a bit weird to me in C. I don't
know if this is because of something C lacks, such as multiple return
values, or if composable parsers are just inherently awkward to describe
in procedural languages because of the different levels of state.
But I think the API here is pretty ok. The main idea is that data
parsers can be added as functions in the lfsr_data_* namespace that take
lfsr_data_t as a mutable reference, updating the lfsr_data_t's internal
state as data is parsed.
In practice you only need a couple of primitives, bytes, le32s, leb128s,
that touch the internals of lfsr_data_t, and the other parsers can be
built using these.
This leverages the pointer-like abstraction of lfsr_data_t, and avoids
needing to keep track of offsets. And thanks to lfsr_data_t being
relatively cheap to make copies, this API is relatively flexible.
Some other tweaks:
- Signed leb128 overflow detection is moved up into lfs_fromleb128.
littlefs now assumes _all_ leb128s are 31-bits, which is useful for
leveraging the sign bit internally.
This also fixes the an issue in overflow detection in lfs_fromleb128
which wouldn't catch overflows in the last byte of a >32-bit leb128.
- Most lfsr_data_t functions now take a pointer. This offered a small
bit of code savings and feels more natural in C. Though most functions
that accept lfsr_data_t still take a copy. Most of these functions
would need to make a copy anyways now that the parsers are consuming,
and these copies avoid concerns about shared state.
At 3-words, lfsr_data_t is right at that boundary of questionable
reasonableness for copying, but copying is a very useful feature of
this struct.
This ends up with some decent code/stack savings:
code stack
before: 22118 2048
after: 21722 (-1.8%) 1992 (-2.7%)
Generally the more creative you get with C macros, the more
unmaintainable your codebase becomes, but in this case I think a small
bit of macro sugar for the attribute lists in littlefs goes a long way
for making the internals flexible and readable.
Attribute lists generally look like this:
LFSR_ATTRS(
LFSR_ATTR(id, TAG, delta, DATA(data)),
LFSR_ATTR(id, TAG, delta, DATA(data)),
...
LFSR_ATTR(id, TAG, delta, DATA(data)))
Which more-or-less gets expanded to this:
((const lfsr_attr_t[]){
((lfsr_attr_t){id, LFSR_TAG_TAG, delta, LFSR_DATA_DATA(data)}),
((lfsr_attr_t){id, LFSR_TAG_TAG, delta, LFSR_DATA_DATA(data)}),
...
((lfsr_attr_t){id, LFSR_TAG_TAG, delta, LFSR_DATA_DATA(data)}),}),
attr_count
Note the use of preprocessor concatenation to put the TAG and DATA
identifiers in their respective namespaces. These can end up invoking
other macros, which allows attrs to be rather extensible.
Previously there were also LFSR_ATTR_ (note the trailing underscore)
macros to allow passing of variable tags/datas. This is replaced with
redundant macros which sort of "unwrap" themselves as a part of macro
expansion. This avoids a bunch of duplicate macro definitions.
#define LFSR_TAG_TAG(tag) (tag)
#define LFSR_DATA_DATA(data) (data)
So:
LFSR_ATTR(id, TAG(tag), delta, DATA(data))
Becomes:
((lfsr_attr_t){id, LFSR_TAG_TAG(tag), delta, LFSR_DATA_DATA(data)})
Becomes:
((lfsr_attr_t){id, tag, delta, data})
Originally it made sense to name the rbyd ids, well, ids, at least in
the internals of the rbyd functions. But this doesn't work well outside
of the rbyd code, where littlefs has to juggle several different id
types with different purposes:
- rid => rbyd-id, 31-bit index into an rbyd
- bid => btree-id, 31-bit index into a btree
- mid => mdir-id, 15-bit+15-bit index into the mtree
- did => directory-id, 31-bit unique identifier for directories
Even though context makes it clear which id the id refers to in the rbyd
internals, updating the name to rid makes it clearer that these are the
same type of id when looking at code both inside and outside the rbyd
functions.
Christopher Haster