Not sure how this was missed. The whole tradeoff of shrinking
rattr.count was that by default lfs3_rattr_t would take up less space,
but user-provided buffers would need an indirect lfs3_data_t to support
arbitrary buffer sizes.
This managed to scrape by with a 16-bit count (15-bit really), but
fortunately failed test_attrs_fattr_resync_receive with an 8-bit count.
And only barely! 256 is the smallest possible custom attr that
overflows.
I guess a point towards making internal limitation as tight as possible
to catch mistakes like these earlier.
---
Added test_attrs_setattr_big and test_attrs_fattr_big to catch this in
the future.
Note that while this added some code, stack is unaffected. This is
because custom attribute handling is off the hot-path, which is why the
lfs3_rattr_t -> lfs3_rattr_t+lfs3_data_t split is worth it:
code stack ctx
before: 37016 2416 652
after: 37052 (+0.1%) 2416 (+0.0%) 652 (+0.0%)
This breaks down the previously 16-bit rattr.count field into two 8-bit
rattr.from and rattr.count fields. Now, instead of using a mixture of
rattr.tag and sign(rattr.count) to determine rattr encoding, we just
jump based on rattr.from:
lfs3_rattr_t:
.---+---+---+---.
| tag |frm|cnt| -+-> 16-bit tag - on-disk encoding + rbyd flags
+---+---+---+---+ +-> 8-bit from - in-RAM encoding
| weight | '-> 8-bit count - from-specific count
+---+---+---+---+
| ptr |
'---+---+---+---'
The internal appendrattr_ ctx also saw a bit of rework, and now uses a
big union with multiple buffers instead of stacking a ridiculous number
of LFS_MAX calls. Expanding the LFS_MAX stack grows O(n^2), so this is
probably good for compile times.
And all rattr.from branches now generate an lfs3_data_t*. This was
already a side-effect of all the internal lfs3_data_from* functions, and
it simplifies the tail end of appendrattr_. No more relying on
data_count's sign bit.
Also rearranged rattr.from encoders to match source code order.
---
Unfortunately, while this did simplify the source code, it didn't really
lead to much improvement in code size:
code stack ctx
before: 37024 2416 652
after: 37016 (-0.0%) 2416 (+0.0%) 652 (+0.0%)
I guess jump tables are more a performance optimization than a code size
one. That and the benefit of cheaper appendrattr_ logic is likely
overshadowed by the extra constants needed to populate rattr.from in
every LFS3_RATTR_* macro.
Also test_attrs_fattr_resync_receive is now failing, but I think that's
just because of an unrelated bug exposed by the shrinking count field.
In theory rattr.count should be limited to internal fixed-size buffers.
Limited to nested struct fields where the names don't really matter:
- bptr.data -> bptr.d
- mdir.rbyd -> mdir.r
Ok it actually just ended up those two.
This is on the tail end of some optimization work that ended up
abandoned because of maintainability concerns. But it did highlight that
struct nesting gets a bit out-of-control when trying to both optimize
stack allocations and respect C99's strict aliasing.
Consider further fragmenting lfs3_rbyd_t for fine-grain stack
allocations:
typedef struct lfs3_rbyd {
struct lfs3_rtrunkcksum {
struct lfs3_rtrunk {
lfs3_rid_t weight;
struct lfs3_rtrunktrunk {
lfs3_block_t blocks[2];
lfs3_size_t trunk;
} rtrunktrunk;
} rtrunk;
uint32_t cksum;
} rtrunkcksum;
lfs3_size_t eoff;
} lfs3_rbyd_t;
Accessing fields just starts to get silly:
rbyd.rtrunkcksum.rtrunk.trunktrunk.trunk
At least single-char field names keeps a little bit of readability:
rbyd.ck.t.t.trunk
Or for some real examples:
- file->b.o.mdir.rbyd.weight -> file->b.o.mdir.r.weight
- bptr->data.u.disk.block -> bptr->d.u.disk.block
So we now keep blocks around until they can be replaced with a single
fragment. This is simpler, cheaper, and reduces the number of commits
needed to graft (though note arbitrary range removals still keep this
unbounded).
---
So, this is a delicate tradeoff.
On one hand, not fully fragmenting blocks risks keeping around bptrs
containing very little data, depending on fragment_size.
On the other hand:
- It's expensive, and disk utilization during random _deletes_ is not
the biggest of concerns.
Note our crystallization algorithm should still clean up partial
blocks _eventually_, so this doesn't really impact random writes.
The main concerns are lfs3_file_truncate/fruncate, and in the future
collapserange/punchhole.
- Fragmenting bptrs introduces more commits, which have their own
prog/erase cost, and it's unclear how this impacts logging operations.
There's no point in fragmenting blocks at the head of a log if we're
going to fruncate them eventually.
I figure lets err on minimizing complexity/code size for now, and if
this turns out to be a mistake, we can always revert or introduce
fragmenting >1 fragment blocks as an optional feature in the future.
---
Saves a big chunk of code, stack, and even some ctx (no more
fragment_thresh):
code stack ctx
before: 37504 2448 656
after: 37024 (-1.3%) 2416 (-1.3%) 652 (-0.6%)
Not sure why we weren't already, it doesn't really make sense to return
bid without weight, and this matches lfs3_btree/bshrub_lookupnext.
Sure we don't need weight currently, but this is useful to include in
case we need it in the future (lfs3_bptr_fetch during traversal?).
And while we're not using it, the compiler is happy to optimize it out,
so no code changes:
code stack ctx
before: 37964 2424 636
after 37964 (+0.0%) 2424 (+0.0%) 636 (+0.0%)
Like the bshrub/btree dedup, this add lfs3_bptr_fetch to help dedup
bptr/data fetching.
The original plan was to eliminate bptrs from lfs3_file_lookupnext and
lfs3_file_traverse, and just return tagged data like the other
lookup/traverse functions. But this didn't work out very well. We return
arbitrary attrs from lfs3_file_traverse, so all this would've
accomplished is making every lfs3_file_lookupnext call messier.
But I think I'm still going to keep lfs3_bptr_fetch around as it
provides a nice place to deduplicate some other bits of logic:
- It makes sense to limit bptrs to compressed weights here, as opposed
to the somewhat arbitrary lfs3_file_lookupnext function.
- And it would be a bit silly to not put the bptr's LFS3_CKFETCHES logic
in lfs3_bptr_fetch.
This may fetch more than previously (during crystallization pokes?),
but better safe than sorry. LFS3_CKFETCHES will likely be a relatively
niche feature anyways.
As for lfs3_file_traverse, I got rid of it completely.
We already have special logic in lfs3_mtree_traverse_ and lfs3_file_ck
for bptrs anyways, since bptrs, unlike data fragments, reference actual
blocks. And this disentangles lfs3_mtree_traverse_ from the file APIs,
which was a bit of an awkward design.
---
This adds a bit of code to the default build, but I think it's worth it
for the better code organization:
code stack ctx
before: 37896 2424 636
after: 37964 (+0.2%) 2424 (+0.0%) 636 (+0.0%)
It also saves some code in LFS3_CKFETCHES mode, thanks to deduping all
the fetch ckfetches fetch checkhes:
code stack ctx
ckfetches before: 38144 2464 636
ckfetches after: 38072 (-0.2%) 2472 (+0.3%) 636 (+0.0%)
One of the ideas behind the key-value API is that it is potentially much
cheaper than a full file API. With the key-value API, we get the
guarantee that all data must fit in RAM, and avoid headaches like
random reads/writes and needing to broadcast file state.
For an example of just how much complexity is avoided, the see the
difference between lfs3_file_flushonce_ vs the mess that is
lfs3_file_flush_ + lfs3_file_crystallize + lfs3_file_graft.
However, littlefs is designed around files, and a couple design
decisions hold back how much code saving is possible:
1. littlefs's shrubs are designed around being enrolled in the omdir
linked-list, so internally we still have most of the file open/close
code lumbering around.
2. Directories and traversals still exist, so we'd need the omdir
linked-list anyways, and we still need to broadcast _some_ changes.
3. Despite being intended for small amounts of data, lfs3_set/get can
still be used to create arbitrarily large files. So we still need all
of the bshrub/btree logic.
Which we still need for the mtree anyways, so this isn't really that
much of a downside.
It also may be possible to save more code by aggressively rewriting the
_entire_ read/write path for lfs3_set/get, to not reuse any of the
existing file logic in LFS3_KVONLY mode. But I decided against this due
to concerns around maintainability.
The duplicate lfs3_file_read + lfs3_file_readonce and lfs3_file_flush_ +
lfs3_file_flushonce_ are already enough of a concern.
Anyways, here's LFS3_KVONLY:
code stack ctx
default: 37824 2416 636
kvonly: 30936 (-18.2%) 2168 (-10.3%) 636 (+0.0%)
LFS3_RDONLY + LFS3_KVONLY is also interesting:
code stack ctx
rdonly: 10776 856 508
rdonly+kvonly: 9904 (-8.1%) 888 (+3.7%) 508 (+0.0%)
---
This also added some noise to the default build's code, mainly due to
tweaks in lfs3_file_readnext to allow better reuse in LFS3_KVONLY:
code stack ctx
before: 37824 2416 636
after: 37860 (+0.1%) 2416 (+0.0%) 636 (+0.0%)
These are high-risk corner cases for the key-value API, so we should
test them.
At one point I was relying on an optional buffer parameter in
lfs3_file_sync_, but that would have broken if lfs3_set's buffer was
NULL.
This adds a couple functions that treat files as simple key-value pairs:
- lfs3_get - Read a file
- lfs3_size - Get the size of a file
- lfs3_set - Write a file
- lfs3_remove - Remove a file (this one already exists!)
The idea is the only real difference between a filesystem and key-value
store in the microcontroller space is the API, and the key-value API
_is_ much easier to use.
It also opens the door to making the file API opt-out in the future to
trade code cost for feature set. littlefs will probably never be
competitive with other microcontroller-scale key-value stores, but it
may be interesting for systems already using littlefs for other storage.
And don't worry, these are still files, so they can always be opened
with the full file API when more advanced operations are needed.
These APIs also matches the custom attribute APIs, which makes sense
because they're both key-values. Any mismatch should be considered an
API bug, because the best user interface is a consistent one.
This new API is tested in tests/test_kv.toml.
---
At the moment the implementation is naive, just sitting on top of the
file API. This works remarkably well thanks to littlefs's cache
bypassing logic, but does have some downsides:
- lfs3_set always writes two commits: one for the stickynote and one for
the file sync.
Unfortunately this is a fundamental limitation of littlefs's file API.
One nice benefit of lfs3_set is in theory we can bypass this
limitation, but not if we just sit on top of the file API.
- There may be code savings from more tightly integrating the key-value
code.
This also highlighted an awkward corner case with per-file cache
configuration in which the buffer needs to be non-null even if zero. Not
the end of the world, but just a bit awkward. Maybe this deserves
revisiting in the config API rework?
---
Code changes were relatively minimal given that this is a whole new API,
unfortunately the stack took quite a hit:
code stack ctx
before: 37352 2280 636
after: 37644 (+0.8%) 2448 (+7.4%) 636 (+0.0%)
The stack surprised me, but in hindsight it makes sense. In sitting on
top of the reset of the codebase, the key-value API adds very little
code, but every stack allocation in these functions add to the stack
hot-path.
This isn't the end of the world, and it's actually probably a good thing
to have an lfs3_file_t allocated in the stack hot-path. lfs3_file_t's
size has been a bit difficult to track thanks to struct lfs3_info
dominating ctx measurements...
These mimic the relevant LFS_O_* flags, and allow users to assert
whether or not a traversal will mutate the filesystem:
LFS_T_MODE 0x00000001 The traversal's access mode
LFS_T_RDWR 0x00000000 Open traversal as read and write
LFS_T_RDONLY 0x00000001 Open traversal as read only
In theory, these could also change internal allocations, but littlefs
doesn't really work that way.
Note we _don't_ add related LFS_GC_RDONLY, LFS_GC_RDWR, etc flags. These
are sort of implied by the relevant LFS_M_* flags.
Adds a bit more code, probably because of the slightly more complicated
internal constants for the internal traversals. But I think the
self-documentingness is worth it:
code stack ctx
before: 37200 2288 636
after: 37220 (+0.1%) 2288 (+0.0%) 636 (+0.0%)
- LFS_CKPARITY -> LFS_CKMETAPARITY
- LFS_CKDATACKSUMS -> LFS_CKDATACKSUMREADS
The goal here is to provide hints for 1. what is being checked (META,
DATA, etc), and 2. on what operation (FETCHES, PROGS, READS, etc).
Note that LFS_CKDATACKSUMREADS is intended to eventually be a part of a
set of flags that can pull off closed fully-checked reads:
- LFS_CKMETAREDUNDREADS - Check data checksums on reads
- LFS_CKDATACKSUMREADS - Check metadata redund blocks on reads
- LFS_CKREADS - LFS_CKMETAREDUNDREADS + LFS_CKDATACKSUMREADS
Also it's probably not a bad idea for LFS_CKMETAPARITY to be harder to
use. It's really not worth enabling unless you understand its
limitations (<1 bit of error detection, yay).
No code changes.
See the relevant commit for why. These just added surprisingly little
performance benefit for the code/stack cost.
Maybe in a future performance-preferring littlefs driver.
The idea here, is we give each lfsr_btree_t an optional leaf rbyd, in
addition to the root rbyd. This leaf rbyd acts as a cache for the most
recent leaf, allowing nearby btree lookups to skip the full btree walk.
Unfortunately, this failed on pretty much every measurable metric...
---
The motivation for this is that we often do a bunch of nearby btree
lookups:
- Btree iteration via lfsr_btree_lookupnext is a bit naive, walking from
the root every step.
- Our crystallization algorithm requires a bunch of nearby lookups to
figure out our crystallization heuristic. Currently at most 4, when
you need to lookup both crystal neighbors and then _also_ both
fragment neighbors for coalescing.
- Checksum collision resolution for dids and (FUTURE) ddkeys can require
an unbounded number of sequential lookups.
Though to be fair, this is an exceptional case if our checksum is any
good.
- Bids with multiple rattrs require nearby lookups to resolve.
Though currently this can be explicitly avoided via
lfsr_btree_lookupleaf + lfsr_rbyd_lookup.
The theory was that cases like these could explicitly keep track of the
leaf rbyd to avoid full btree walks, but in practice this never really
worked out. Tracking if we're still in the relevant leaf rbyd just adds
too much logic/code cost.
But if this leaf tracking logic was implemented once in the btree
layer...
The other theoretical benefit was being able to move more rbyds off the
stack. Sure our btrees take up more RAM, but if that results in stack
savings, that may be a win.
Oh, and this would let our btree API and rbyd API converge without
performance concerns. Internal users could in theory call
lfsr_btree_lookupnext + lfsr_btree_lookup with the same performance as
explicitly tracking the rbyd.
---
But this was a complete failure!
First the good news: There was a modest speedup of around ~2x to linear
reads.
And that's the good news.
Now the bad news:
1. There was no noticeable performance gain in any other benchmarks.
To be fair, we're at the early stages of benchmarking, so the
benchmarks may not be the most thorough, but thinking about it, there
are some explanations:
- In any benchmark that writes, fetch + erase + prog dominates. Being
able to skip fetches during lookups makes our btree lookups
surprisingly cheap!
- Any random read heavy benchmark is likely thrashing this cache,
which is to be expected.
- For small 1-block btrees, the leaf cache is useless because the
entire btree is cache in the root rbyd.
And keep in mind, our blocks are BIG. "Small" here could be on
the order of ~128KiB-1MiB for NAND flash.
- For the mtree, fetched mdirs actually already act as a sort of leaf
cache.
The extra btree leaf cache isn't doing _nothing_, but each layer of
the mtree has diminishing returns due to btree's ridiculous
branching factor.
- For file btrees, we're explicitly caching the leaf fragments/
blocks, so the extra btree leaf cache has diminishing returns for
the same reason.
2. Code cost was bad, stack cost was worse:
code stack ctx
before: 37172 2288 636
after: 38068 (+2.4%) 2416 (+5.6%) 664 (+4.4%)
Tracking the leaf required more code, that's expected. And, to be
fair, the current code has had a lot more time to congeal.
What wasn't expected was the stack cost.
Unfortunately these caches didn't really take any rbyds off the stack
hot-path:
- We _can_ get rid of the rbyd in lfsr_btree_lookup/namelookup, but
we were already hacking our way around the critical one in
lfsr_mtree_lookup/namelookup by reusing the mdir's rbyd!
- We can't even abuse the leaf rbyd in the commit logic, since the
target btree can end up iterated/traversed by lfs_alloc.
That was a fun bug.
And the addition of a second rbyd to lfsr_btree_t increases both ctx
and stack anywhere btrees are allocated.
Maybe this will make more sense when we add the auxiliary btrees, or
after more benchmarking, but for now the theoretical performance
improvements just aren't worth it.
Will probably revert this, but I wanted to commit it in case the idea is
worth resurrecting in the future, if in the future nearby btree lookups
are a bigger penalty than they are now.
In lfsr_mdir_compact__, we rely on shrub_.block != mdir.block to avoid
compacting shrubs multiple times. This works for the most part because
we set shrub_.block = shrub.block (the old mdir block) at the beginning
of lfsr_mdir_commit. We don't actually reset shrub_.block on a bad prog,
but in theory that was ok because we never try to compact into the same
block twice.
But this falls apart if we overrecycle the mdir!
With overrecycling, if we encounter a bad prog during a compaction and
there are no more blocks to relocate to, we try one last time to compact
into the same block (this logic is mainly for recycle overflows, where
it makes a bit more sense).
Of course, compacting into the same block breaks the above shrub_.block
!= mdir.block invariant, which causes the shrub compaction to be
skipped, uses the old shrub_.trunk (which now points to garbage), and
breaks everything.
Fortunately the solution is relatively simple: Just discard any staged
shrubs that have been committed when we relocate/overrecycle.
---
While fixing this I went ahead and renamed overcompaction ->
overrecycling. To me, overcompaction implies something _very_ different,
and I think this better describes the relationship between overrecycling
and block_recycles.
Also added test_ck_ckprogs_overrecycling to nail this down and prevent a
regression in the future. This bug _was_ caught by
test_ck_spam_fwrite_fuzz, but only after unrelated fs changes.
Adds a bit of code, but a smaller + dysfunctional filesystem is not very
useful:
code stack ctx
before: 37056 2304 (+0.0%) 636 (+0.0%)
after: 37088 (+0.1%) 2304 (+0.0%) 636 (+0.0%)
TLDR: Added file->leaf, which can track file fragments (read only) and
blocks independently from file->b.shrub. This speeds up linear
read/write performance at a heavy code/stack cost.
The jury is still out on if this ends up reverted.
---
This is another change motivated by benchmarking, specifically the
significant regression in linear reads.
The problem is that CTZ skip-lists are actually _really_ good at
appending blocks! (but only appending blocks) The entire state of the
file is contained in the last block, so file writes can resume without
any reads. With B-trees, we need at least 1 B-tree lookup to resume
appending, and this really adds up when writing extremely blocks.
To try to mitigate this, I added file->leaf, a single in-RAM bptr for
tracking the most recent leaf we've operated on. This avoids B-tree
lookups during linear reads, and allowing the leaf to fall out-of-sync
with the B-tree avoids both B-tree lookups and commits during writes.
Unfortunately this isn't a complete win for writes. If we write
fragments, i.e. cache_size < prog_size, we still need to incrementally
commit to the B-tree. Fragments are a bit annoying for caching as any
B-tree commit can discard the block they reside on.
For reading, however, this brings read performance back to roughly the
same as CTZ skip-lists.
---
This also turned into more-or-less a full rewrite of the lfsr_file_flush
-> lfsr_file_crystallize code path, which is probably a good thing. This
code needed some TLC.
file->leaf also replaces the previous eblock/eoff mechanism for
erased-state tracking via the new LFSR_BPTR_ISERASED flag. This should
be useful when exploring more erased-state tracking mechanisms (ddtree).
Unfortunately, all of this additional in-RAM state is very costly. I
think there's some cleanup that can be done (the current impl is a bit
of a mess/proof-of-concept), but this does add a significant chunk of
both code and stack:
code stack ctx
before: 36016 2296 636
after: 37228 (+3.4%) 2328 (+1.4%) 636 (+0.0%)
file->leaf also increases the size of lfsr_file_t, but this doesn't show
up in ctx because struct lfs_info dominates:
lfsr_file_t before: 116
lfsr_file_t after: 136 (+17.2%)
Hm... Maybe ctx measurements should use a lower LFS_NAME_MAX?
This better matches how other filesystems refer to the number of in-use
blocks.
Which makes sense when you consider that "size" could also refer to the
configured block_count. The term "usage" avoids this ambiguity.
This carves out two more bits in cksum tags to store the "phase" of the
rbyd block (maybe the name is too fancy, this is just the lowest 2 bits
of the block address):
LFSR_TAG_CKSUM 0x300p v-11 ---- ---- -pqq
^ ^
| '-- phase bits
'---- perturb bit
The intention here is to catch mrootanchors that are "out-of-phase",
i.e. they've been shifted by a small number of blocks.
This can happen if we find the wrong mrootanchor (after, say, a magic
scan), and risks filesystem corruption:
formatted
.-----------------'-----------------.
mounted
.-----------------'-----------------.
.--------+--------+--------+--------+ ...
|(erased)| mroot |
| | anchor | ...
| | |
'--------+--------+--------+--------+ ...
Including the lower 2 bits of the block address in cksum tags avoids
this, for up to a 3 block shift (the maximum number of redund
mrootanchors).
---
Note that cksum tags really are the only place we could put these bits.
Anywhere else and they would interfere with the canonical cksum, which
would break error correction. By definition these need to be different
per block.
We include these phase bits in every cksum tag (because it's easier),
but these don't really say much about mdirs that are not the
mrootanchor. Non-anchor mdirs can have arbitrary block addresses,
therefore arbitrary phase bits.
You _might_ be able to do something interesting if you sort the rbyd
addresses and use the index as the phase bits, but that would add quite
a bit of code for questionable benefit...
You could argue this adds noise to our cksums, but:
1. 2 bits seems like a really small amount of noise
2. our cksums are just crc32cs
3. the phase bits humorously never change when you rewrite a block
---
As with any feature this adds code, but only a small amount. I think
it's worth the extra protection:
code stack ctx
before: 35792 2368 636
after: 35824 (+0.1%) 2368 (+0.0%) 636 (+0.0%)
Also added test_mount_incompat_out_of_phase to test this.
The dbg scripts _don't_ error (block mismatch seems likely when
debugging), but dbgrbyd.py at least adds phase mismatch notes in
-l/--log mode.
This drops the leading count/mode byte, and instead uses mid=0 to
terminate grms. This shaves off 1 bytes from grmdeltas.
Previously, we needed the count/mode byte for a couple reasons:
- We needed to know the number of grm entries somehow, and there wasn't
always an obvious sentinel value. mid=-1, for example, is
unrepresentable with our unsigned leb128 encoding.
But now that development has settled, we can use mid=0.0 to figure out
the end-of-queue. mid=0.0 should always map to the root bookmark,
which doesn't make sense to delete, so it makes for a reasonable null
terminator here.
- It provided a route for future grm extensions, which could use the >2
count/mode encodings.
But I think we can use additional grm tag encodings for this.
There's only one gdelta tag so far, but the current plan for future
gdelta tags is to carve out the bottom 2 bits for redund like we do
with the struct tags:
LFSR_TAG_GDELTA 0x01tt v--- ---1 -ttt ttrr
LFSR_TAG_GRMDELTA 0x0100 v--- ---1 ---- ----
LFSR_TAG_GBMAPDELTA 0x0104 v--- ---1 ---- -1rr
LFSR_TAG_GDDTREEDELTA 0x0108 v--- ---1 ---- 1-rr
LFSR_TAG_GPTREEDELTA 0x010c v--- ---1 ---- 11rr
...
Decoding is a bit more complicated for gstate, since we will need to
xor those bits if mutable, but this avoids needing a full byte just
for redund in every auxiliary tree.
Long story short, we can leverage the lower 2 bits of the grm tag for
future extensions using the same mechanism.
This may seem like a lot of effort for only a handful of bytes, but keep
in mind each gdelta lives in more-or-less every mdir in the filesystem.
Also saves a bit of code/ctx:
code stack ctx
before: 35772 2368 640
after: 35768 (-0.0%) 2368 (+0.0%) 636 (-0.6%)
Why?
- lfsr_mtree_lookupleaf vs lfsr_mtree_commit is inconsistent. Should
lfsr_mdir_commit be called lfsr_mtree_commitleaf? That'd be weird.
It's reasonable to call mdirs entries of the mtree, but it'd be weird
to call rbyds entries of btrees, so the inconsistency there is
expected.
- lfsr_mtree_lookup/lfsr_mtree_lookupnext (going mtree -> mdir) aren't
actually useful.
- The lfsr_mtree_namelookup/lfsr_mtree_namelookupleaf split is just more
of a headache than it's worth.
Saves a tiny bit of code:
code stack ctx
before: 35768 2392 640
after: 35764 (-0.0%) 2392 (+0.0%) 640 (+0.0%)
This makes all rbyd_/mdir_ out-pointers required, dropping all of the
internal copies needed to make lookup/namelookup/pathlookup/etc work.
Previously, the -- rough -- rule was to make out-pointers generally
optional (lfsr_data_read and other struct initers being notable
exceptions), the idea being you can opt-out of stack allocations where
possible.
In practice this kind of backfired, with many internal functions needing
redundant stack allocations in case the relevant parameter is NULL
(lfsr_btree_lookupleaf being an excellent example).
---
As an alternative rule, I think we should only expect optional
out-pointers for things you would pass-by-value (lfsr_rid_t, lfsr_tag_t,
lfsr_data_t, etc).
I've also developed a habit of naming optional out-pointers with a
trailing underscore_, to hopefully make this subtlety a bit less subtle.
This claws back all of the stack cost of BNAMEs/MNAMEs, and most of the
code cost:
code stack ctx
before: 35888 2480 640
after: 35780 (-0.3%) 2408 (-2.9%) 640 (+0.0%)
Though we still have more function calls than we started with
(lfsr_mtree_*lookup mtree -> mdir lookups).
This is the _nth_ time I've tried to force arbitrary btree name inserts
to work, so _clearly_ I need a bigger comment.
Hopefully this will prevent me from trying to delete the LFSR_RATTR_NOOP
in test_btree_find_general_fuzz _again_.
---
The gist is that insert-before-bid+1 is fundamentally different from
insert-after-bid when named btrees are involved:
.-----f-----. insert-after-d .-------f-----.
.-b--. .--j-. => .-b---. .--j-.
| .-. .-. | | .---. .-. |
a c d h i k a c d e h i k
^
insert-before-h
=> .-----f-------.
.-b--. .---j-.
| .-. .---. |
a c d g h i k
^
The problem is that lfsr_btree_commit_ needs to find the same leaf
rbyd as lfsr_btree_namelookup, and potentially insert-before the
first rid or insert-after the last rid.
Instead of separate insert-before/after flags, we make the first tag
in a commit insert-before, and all following non-grow tags
insert-after (splits).
This info is now captured in the above mentioned comment.
Now that we don't have to worry about name tag conflicts as much, we
can add name tags for things that aren't files.
This adds LFSR_TAG_BNAME for branch names, and LFSR_TAG_MNAME for mtree
names. Note that the upper 4 bits of the subtype match LFSR_TAG_BRANCH
and LFSR_TAG_MDIR respectively:
LFSR_TAG_BNAME 0x0200 v--- --1- ---- ----
LFSR_TAG_MNAME 0x0220 v--- --1- --1- ----
LFSR_TAG_BRANCH 0x030r v--- --11 ---- --rr
LFSR_TAG_MDIR 0x0324 v--- --11 --1- -1rr
The encoding is somewhat arbitrary, but I figured reserving ~31 types
for files is probably going to be plenty for littlefs. POSIX seems to
do just fine with only ~7 all these years, and I think custom attributes
will be more enticing for "niche" file types (symlinks, compressed
files, etc), given the easy backwards compatibility.
---
In addition to the debugging benefits, the new name tags let us stop
btree lookups on the first non-bname/branch tag. Previously we always
had to fetch the first struct tag as well to check if it was a branch.
In theory this saves one rbyd lookup, but in practice it's a bit muddy.
The problem is that there's two ways to use named btrees:
1. As buckets: mtree -> mdir -> mid
2. As a table: ddtree -> ddid
The only named btree we _currently_ have is the mtree. And the mtree
operates in bucket mode, with each mdir acting more-or-less as an
extension to the btree. So we end up needing to do the second tag lookup
anyways, and all we've done is complicated up the code.
But we will _eventually_ need the table mode for the ddtree, where we
care if the ddname is an exact match.
And returning the first tag is arguably the more "correct" internal API,
vs arbitrarily the first struct tag.
But then again this change is pretty pricey...
code stack ctx
before: 35732 2440 640
after: 35888 (+0.4%) 2480 (+1.6%) 640 (+0.0%)
---
It's worth noting the new BNAME/MNAME tags don't _require_ the btree
lookup changes (which is why we can get away with not touching the dbg
scripts). The previous algorithm of always checking for branch tags
still works.
Maybe there's an argument for conditionally using the previous API when
compiling without the ddtree, but that sounds horrendously messy...
Found another ckparity flaw! Only detected now due to the reworked tag
encoding, but it's just luck this wasn't detected earlier.
Consider the following bit flip:
03 04 80 04 80 04 6b ... data w512 512
43 04 80 04 80 04 6b ... altble 0x304 w512 -512
^
flip
Not only are leb128s problem for ckparity, but even the difference in
alt vs normal tag encoding presents a vulnerability.
So limiting the ckparity tests further, to just 47 of the first 48 bits.
Mainly to make room for some future planned stuff:
- Moved the mroot's redund bits from LFSR_TAG_GEOMETRY to
LFSR_TAG_MAGIC:
LFSR_TAG_MAGIC 0x003r v--- ---- --11 --rr
This has the benefit of living in a fixed location (off=0x5), which
may make mounting/debugging easier. It also makes LFSR_TAG_GEOMETRY
less of a special case (LFSR_TAG_MAGIC is already a _very_ special
case).
Unfortunately, this does get in the way of our previous magic=0x3
encoding. To compensate (and to avoid conflicts with LFSR_TAG_NULL),
I've added the 0x3_ prefix. This has the funny side-effect of
rendering redunds 0-3 as ascii 0-3 (0x30-0x33), which is a complete
accident but may actually be useful when debugging.
Currently all config tags fit in the 0x3_ prefix, which is nice for
debugging but not a hard requirement.
- Flipped LFSR_TAG_FILELIMIT/NAMELIMIT:
LFSR_TAG_FILELIMIT 0x0039 v--- ---- --11 1--1
LFSR_TAG_NAMELIMIT 0x003a v--- ---- --11 1-1-
The file limit is a _bit_ more fundamental. It's effectively the
required integer size for the filesystem.
These may also be followed by LFSR_TAG_ATTRLIMIT based on how future
attr revisits go.
- Rearranged struct tags so that LFSR_TAG_BRANCH = 0x300:
LFSR_TAG_BRANCH 0x030r v--- --11 ---- --rr
LFSR_TAG_DATA 0x0304 v--- --11 ---- -1--
LFSR_TAG_BLOCK 0x0308 v--- --11 ---- 1err
LFSR_TAG_DDKEY* 0x0310 v--- --11 ---1 ----
LFSR_TAG_DID 0x0314 v--- --11 ---1 -1--
LFSR_TAG_BSHRUB 0x0318 v--- --11 ---1 1---
LFSR_TAG_BTREE 0x031c v--- --11 ---1 11rr
LFSR_TAG_MROOT 0x032r v--- --11 --1- --rr
LFSR_TAG_MDIR 0x0324 v--- --11 --1- -1rr
LFSR_TAG_MTREE 0x032c v--- --11 --1- 11rr
*Planned
LFSR_TAG_BRANCH is a very special tag when it comes to bshrub/btree
traversal, so I think it deserves the subtype=0 slot.
This also just makes everything fit together better, and makes room
for the future planned ddkey tag.
Code changes minimal:
code stack ctx
before: 35728 2440 640
after: 35732 (+0.0%) 2440 (+0.0%) 640 (+0.0%)
This tears out most of the implied lfsr_file_sync calls, and restricts
LFS_O_SYNC to only imply lfsr_file_sync on _write_ operations. So only
lfsr_file_write, and maybe pwrite/writev/etc in the future.
This mainly affects lfsr_file_truncate/fruncate (and punchhole/
insertrange/collapserange in the future), while reverting the LFS_O_SYNC
related changes in lfsr_file_open:
- lfsr_file_open + LFS_O_SYNC => does _not_ sync
- lfsr_file_close + LFS_O_SYNC => syncs (unless desynced)
- lfsr_file_write + LFS_O_SYNC => syncs
- lfsr_file_sync + LFS_O_SYNC => syncs
- lfsr_file_truncate + LFS_O_SYNC => does _not_ sync
- lfsr_file_fruncate + LFS_O_SYNC => does _not_ sync
Note LFS_O_FLUSH is unaffected, it was always limited to
lfsr_file_write since that's the only function that touches file
buffers.
Also note I want this rule to apply to the future lfsr_file_punchhole/
insertrange/collapserange functions as well. Even though you can argue
these effectuate writes, they're at a level of sophistication that we
can just expect users to just call lfsr_file_sync if they want to.
---
Ok, so a number of reasons:
- This matches behavior of LFS_O_APPEND, which is intentionally
restricted to only write operations.
In that case I think the explicit limitation is easier to understand
than trying to define an abstract model.
This makes LFS_O_SYNC, LFS_O_FLUSH, and LFS_O_APPEND consistent in
when the relevant behavior takes effect.
- This avoids the zero-sized files after powerloss. Which are just as
likely, if not more, to trip up users vs missing syncs.
- Most truncate/fruncate operations are immediately followed by a write
operation anyways. Which just makes the truncate/fruncate syncs wasted
prog/erase cycles.
Even in some of the more complicated truncate/function use cases, you
just don't care about when fruncates/truncates hit the disk.
Take logging via lfsr_file_fruncate for example. Yes the fruncate will
usually happen _after_ the write operation, but this just means the
log file will usually be one entry larger than expected. Which is a
state you can end up with anyways after powerloss.
- This avoids confusing/conflicting LFS_O_SYNC + LFS_O_DESYNC behavior.
Again, this simple rule is easier to reason about than a model.
You would think this would be well defined in POSIX, but it's really
not. POSIX limits O_SYNC to "write I/O operations", but doesn't really
define a "write" (it is a retroactive standard after all). ftruncate is
a bit funny in that it states "the extended area shall appear as if it
were zero-filled", but the term "write" doesn't appear in ftruncate's
documentation at all.
Searching through LKML, stack overflow, etc, it doesn't seem like anyone
else knows exactly what to do either. There was a bug report[1] in 2005
for ext3 + O_SYNC + ftruncate that was rejected, but a later bug
report[2] in 2012 for xfs + O_SYNC + fallocate that was fixed (but was
broken in almost every Linux fs?).
1: https://lore.kernel.org/lkml/1111610558.1998.193.camel@sisko.sctweedie.blueyonder.co.uk
2: https://lore.kernel.org/linux-ext4/20111116084256.GA22963@infradead.org
So, this may end up a bit controversial, but I'm going to go with the
simpler truncate/fruncate-do-not-imply-sync rule for the above reasons.
I think this is a bit more important for littlefs than other
filesystems, as it also defines the behavior of lfsr_file_open, and with
a rigorous powerloss model being core to the design.
---
This is also cheaper code/stack-wise, but if this was going to be a
deciding factor we should just put LFS_O_SYNC/LFS_O_FLUSH behind ifdefs:
code stack ctx
before: 35816 2480 640
after: 35740 (-0.2%) 2424 (-2.3%) 640 (+0.0%)
Compared to before the LFS_O_SYNC tweaks:
code stack ctx
before-tweaks: 35780 2440 640
before: 35816 (+0.1%) 2480 (+1.6%) 640 (+0.0%)
after: 35740 (-0.1%) 2424 (-0.7%) 640 (+0.0%)
This is the only way I can think of resolving the weirdness that is
LFS_O_SYNC + LFS_O_DESYNC. Just don't allow it.
LFS_O_SYNC and LFS_O_DESYNC are pretty much opposite behaviors, so an
LFS_O_SYNC + LFS_O_DESYNC file seems like a contradiction.
---
This does limit a little bit what's possible with the API, but hey that
just means fewer tests/smaller API surface area for users to stub their
toes on.
Saves a tiny bit of code:
code stack ctx
before: 35824 2480 640
after: 35816 (-0.0%) 2480 (+0.0%) 640 (+0.0%)
Do'h! I almost forgot about LFS_O_TRUNC. If LFS_O_CREAT + LFS_O_SYNC
implies lfsr_file_sync, clearly LFS_O_TRUNC + LFS_O_SYNC should as well.
This changes lfsr_file_open to only imply lfsr_file_sync if any open
operation sets the unsync flag, which is the only case where
lfsr_file_sync would do anything anyways.
This does have a subtle change in behavior when LFS_O_CREAT + LFS_O_SYNC
+ LFS_O_DESYNC, in that the desync flag is only cleared if the file did
not exist before. But I think this is more expected than unconditionally
syncing.
Note this matches the behavior of lfsr_file_write, which does _not_
imply lfsr_file_sync if the write is size=0.
---
Also added better tests over lfsr_file_open + LFS_O_TRUNC, this flag
isn't very well tested...
Which found a bug!
We were incorrectly setting LFS_o_UNFLUSH when opening with LFS_O_TRUNC,
when we should have set LFS_o_UNSYNC. This caused littlefs to never
bother updating the file's metadata unless some other write comes along
(which is what usually follows LFS_O_TRUNC).
To help catch bugs like this, I added an assert to lfsr_file_flush that
unflushed files are always marked unsynced. A synced + unflushed file is
weird and should never happen.
---
Code changes minimal:
code stack ctx
before: 35820 2480 640
after: 35824 (+0.0%) 2480 (+0.0%) 640 (+0.0%)
So we keep the behavior of creating reg files with lfsr_file_open +
LFS_O_SYNC, but only clear the desync flag if lfsr_file_open would
mutate the filesystem.
This is hopefully a simpler model to reason about, and makes LFS_O_SYNC
+ LFS_O_DESYNC a bit less weird.
Saves a little bit of code:
code stack ctx
before: 35836 2488 640
after: 35820 (-0.0%) 2480 (-0.3%) 640 (+0.0%)
So now the following creates a reg file (instead of just a stickynote):
lfsr_file_open(&lfs, &file, "test.txt",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL | LFS_O_SYNC) => 0;
// powerloss!!
struct lfsr_info info;
lfsr_stat(&lfs, "test.txt", &info) => 0; // LFS_ERR_NOENT before
assert(info.type == LFS_TYPE_REG);
This hopefully results in more intuitive behavior around lfsr_file_open
with LFS_O_SYNC. Which is important as LFS_O_SYNC is often used as an
escape hatch to avoid needing to reason about syncing things when
performance is not a big concern.
Unfortunately this does come with a surprisingly big code/stack cost,
but I'm thinking of putting these flags (LFS_O_FLUSH/LFS_O_SYNC) behind
ifdefs anyways (LFS_MAYBE_SYNC?):
code stack ctx
before: 35780 2440 640
after: 35836 (+0.2%) 2488 (+2.0%) 640 (+0.0%)
Also added some more tests to make sure these open+LFS_O_SYNC cases are
explicitly covered:
- test_fsync_sync_o_wrr
- test_fsync_sync_o_wwrr
- test_fsync_desync_o_wdwrr
- test_fsync_resync_o_wdwyrr
This does make a bit of a mess when you combined LFS_O_SYNC +
LFS_O_DESYNC. What exactly should a SYNC + DESYNC file look like?
For now I've just made LFS_O_SYNC + LFS_O_DESYNC behave as if you opened
a file with LFS_O_SYNC and then immediately called lfsr_file_desync on
it. So it doesn't receive broadcasts, but _does_ create the reg file,
and _does_ sync on first write, clearing the desync flag.
But this may be worth revisiting. Maybe LFS_O_DESYNC files shouldn't
have their desync flags cleared unless lfsr_file_sync is explicitly
called? Or maybe LFS_O_SYNC + LFS_O_DESYNC should just be an error?
Unsure...
So now calling lfsr_file_sync on zombied files is a noop:
// create a file
lfsr_file_t a;
lfsr_file_open(&lfs, &a, "a",
LFS_O_RDWR | LFS_O_CREAT | LFS_O_EXCL) => 0;
// remove, creating a zombie
lfsr_remove(&lfs, "a") => 0;
// sync, this is now a noop (previously LFS_ERR_NOENT)
lfsr_file_sync(&lfs, &a) => 0;
// close is also a noop
lfsr_file_close(&lfs, &a) => 0;
I've been on the fence on this for a while, on one hand erroring
provides more information to the user, on the other hand a noop is less
surprising if the user comes from other systems.
Ended up making this a noop. I figured minimizing surprises is good API
design, and the user can always use lfsr_stat to check if the file still
exists.
This also matches POSIX, and, perhaps more importantly, the current
version of littlefs.
---
Note that lfsr_file_resync still errors with LFS_ERR_NOENT. It's hard to
argue the file "matches the state of disk" otherwise.
Code changes minimal:
code stack ctx
before: 35784 2440 640
after: 35780 (-0.0%) 2440 (+0.0%) 640 (+0.0%)
This drops the requirement that all file types are introduced with a
related wcompat flag. Instead, the wcompat flag is only required if
modification _would_ leak resources, and we treat unknown file types as
though they are regular files.
This allows modification of unknown file types without the risk of
breaking anything.
To compare with before the unknown-type rework:
Before:
> Unknown file types are allowed and may leak resources if modified,
> so attempted modification (rename/remove) will error with
> LFS_ERR_NOTSUP.
Now:
> Unknown file types are allowed but must not leak resources if
> modified. If an unknown file type would leak resources, it should set
> a related wcompat flag to only allow mounting RDONLY.
Note this includes directories, which can leak bookmarks if removed, so
filesystems using directories should set the LFSR_WCOMPAT_DIR flag.
But we no longer need the LFSR_WCOMPAT_REG/LFSR_WCOMPAT_STICKYNOTE
flags.
---
The real tricky part was getting lfsr_rename to work with unknown types,
as this broke the invariant that we only ever commit tags we know about.
Fixing this required:
- Fetching the non-unknown-mapped tag in lfsr_rename
- Mapping all name tags to LFSR_TAG_NAME in lfsr_rbyd_appendrattr_
- Adopting LFSR_RATTR_NAME for bookmark name tags
This was broken by the above lfsr_rbyd_appendrattr_ change, but it's
probably good to handle these the same as other name tags anyways.
This adds a bit of code, but not enough that I think this isn't worth
it (or worth a build-time option):
code stack ctx
before: 35924 2440 640
after: 35992 (+0.0%) 2440 (+0.0%) 640 (+0.0%)
This changes how we approach unknown file types.
Before:
> Unknown file types are allowed and may leak resources if modified,
> so attempted modification (rename/remove) will error with
> LFS_ERR_NOTSUP.
Now:
> Unknown file types are only allowed in RDONLY mode. This avoids the
> whole leaking resources headache.
Additionally, unknown types are now mapped to LFS_TYPE_UNKNOWN, instead
of just being forwarded to the user. This allows us to add internal
types/tags to the LFSR_TAG_NAME type space without worrying about
conflicts with future types:
- reg -> LFS_TYPE_REG
- dir -> LFS_TYPE_DIR
- stickynote -> LFS_TYPE_STICKYNOTE
- everything else -> LFS_TYPE_UNKNOWN
Thinking about potential future types, it seems most (symlinks,
compressed files, etc) can be better implemented via custom attributes.
Using custom attributes doesn't mean the filesystem _can't_ inject
special behavior, and custom attributes allow for perfect backwards
compatibility.
So with future types less likely, forwarding type info to users is less
important (and potentially error prone). Instead, allowing on-disk +
internal types to be represented densely is much more useful.
And it avoids setting an upper bound on future types prematurely.
---
This also includes a minor rcompat/wcompat rework. Since we're probably
going to end up with 32-bit rcompat flags anyways, might as well make
them more human-readable (nibble-aligned):
LFS_RCOMPAT_NONSTANDARD 0x00000001 Non-standard filesystem format
LFS_RCOMPAT_WRONLY 0x00000002 Reading is disallowed
LFS_RCOMPAT_BMOSS 0x00000010 Files may use inlined data
LFS_RCOMPAT_BSPROUT 0x00000020 Files may use block pointers
LFS_RCOMPAT_BSHRUB 0x00000040 Files may use inlined btrees
LFS_RCOMPAT_BTREE 0x00000080 Files may use btrees
LFS_RCOMPAT_MMOSS 0x00000100 May use an inlined mdir
LFS_RCOMPAT_MSPROUT 0x00000200 May use an mdir pointer
LFS_RCOMPAT_MSHRUB 0x00000400 May use an inlined mtree
LFS_RCOMPAT_MTREE 0x00000800 May use an mdir btree
LFS_RCOMPAT_GRM 0x00001000 Global-remove in use
LFS_WCOMPAT_NONSTANDARD 0x00000001 Non-standard filesystem format
LFS_WCOMPAT_RDONLY 0x00000002 Writing is disallowed
LFS_WCOMPAT_REG 0x00000010 Regular file types in use
LFS_WCOMPAT_DIR 0x00000020 Directory file types in use
LFS_WCOMPAT_STICKYNOTE 0x00000040 Stickynote file types in use
LFS_WCOMPAT_GCKSUM 0x00001000 Global-checksum in use
---
Code changes:
code stack ctx
before: 35928 2440 640
after: 35924 (-0.0%) 2440 (+0.0%) 640 (+0.0%)
This adds stickynotes as a target type for most of the test_attrs tests.
These were already parameterized for reg + dir + root, but they did need
some tweaks to allow us to keep files open for most of the test.
I don't really see a use case for this feature, but it keeps the API
consistent. The file name itself is also sort of an attr, and the whole
point of stickynotes is to have something to attach the file name to, so
attaching other attrs makes sense I guess?
Seems like a better name now that LFS_TYPE_STICKYNOTE is its own file
type.
Though this does contain some tests that I think don't even use
stickynotes...
This adds the LFS_TYPE_STICKYNOTE type, allowing users to interact with
stickynotes as long as they aren't orphaned.
This hopefully solves the long-standing mess that was the LFS_O_EXCL
API.
---
As for what I mean by orphaned vs non-orphaned stickynotes:
Non-orphaned stickynotes represent files that have been "created" (via
LFS_O_CREAT), but not "committed" (via sync/close). You can still close
and convert the stickynote to a reg file, so these aren't orphans. These
are also called "uncreated" files in some parts of the codebase:
- open+O_CREAT -> non-orphaned stickynote (uncreated file)
Orphaned stickynotes are possible by either removing an open file, or
desyncing a file before sync/close. These are still invisible to the
user and will be eventually cleaned up after the last file handle is
closed:
- open+remove -> orphaned stickynote (zombied file)
- open+O_CREAT+desync+close -> orphaned stickynote (orphaned file)
Desynced files are a bit special. Even though they technically aren't
orphaned, they also behave like orphaned file handles:
- open+O_CREAT+close -> orphaned stickynote (desynced file)
The idea is this mimics the state of files post-close, and allows for
some tricks like using a desync file as a temporary file with no
observable effects on the filesystem.
---
The motivation for this comes from staring at the LFS_O_EXCL API for too
long and realizing the problem is that littlefs's API contradicts itself
when it comes to whether or not uncreated files exist.
This solution is to consistently treat uncreated files as though they
exist (the alternative would make LFS_O_EXCL pretty much useless), but I
really didn't want to do this as having what appears to be normal files
disappear after powerloss risks confusion.
The compromise here is to give these files a special type, repurposing
the internal LFS_TAG_STICKYNOTE, which hopefully hints to the user these
won't behave like normal files.
If the user is more interested in POSIX compatibility, they can always
map these to either LFS_TYPE_REG or LFS_ERR_NOENT, whichever they think
is the least confusing.
As a quirk of littlefs's API, stickynotes should never actually contain
any data, and will always have size 0.
However they can have custom attributes assigned now (which is I guess
ok? also TODO should probably test this).
---
The implementation right now is a bit naive, I mostly just wanted to get
the tests working again in this new model. It may be possible to claw
back some of this code cost:
code stack ctx
before: 35740 2440 640
after: 35952 (+0.6%) 2440 (+0.0%) 640 (+0.0%)
This may be useful for compression in the future, where compression +
noise can result in blocks _larger_ than the expected weight.
Thinking about how compression might be integrated into littlefs, it
would be nice if such a topology did _not_ trigger asserts. This would
allow littlefs images to interact with compressed files at least a
little bit (rename/remove could be very useful), even if the compression
algorithm isn't supported.
Supporting this requires only a single clamp in lfsr_file_lookupleaf,
but it's a little bit more costly than you might expect:
code stack ctx
before: 35692 2440 640
after: 35740 (+0.1%) 2440 (+0.0%) 640 (+0.0%)
This is due to internal API awkwardness:
1. LFSR_DATA_TRUNCATE is surprisingly costly
2. We need to create a local weight copy in case the caller's is NULL
- test_fwrite_reversed_litmus_fragments
- test_fwrite_reversed_litmus_blocks
- test_fwrite_freversed
- test_fwrite_freversed_litmus_fragments
- test_fwrite_freversed_litmus_blocks
- test_fwrite_truncate_pos
- test_fwrite_fruncate_pos
And hey, they found some bugs:
- crystal_thresh=-1 was broken due to integer overflow in some signed
math.
Fortunately when crystal_thresh=-1 we can just skip the crystal
lookups entirely. This saves a btree lookup in fully-fragmented files.
- We were including empty fragments in our crystal size, when we should
only use them to determine crystal boundaries, like bptrs.
This is a common case for the first entry in a sparse file.
- We weren't updating pos on fruncate. fruncate's effect on pos was
actually not tested at all.
Which raises the question, what should the behavior be? Match
lfsr_file_truncate and leave the pos unaffected?
I ended up having fruncate update the file pos to keep the same pos
relative to the end, as I figured this would have the least surprise
for users. So lfsr_file_read should return the same bytes unless
clobbered.
This is almost a mirror image of lfsr_file_truncate, except we don't
allow negative positions, so fruncating more than pos forces pos to 0.
---
This behavior is now covered in a couple tests:
- test_fwrite_truncate_pos
- test_fwrite_fruncate_pos
- test_fwrite_freversed
- test_fwrite_freversed_litmus_fragments
- test_fwrite_freversed_litmus_blocks
Code changes:
code stack ctx
before: 35688 2440 640
after: 35692 (+0.0%) 2440 (+0.0%) 640 (+0.0%)
This tweaks a number of extended revision count things:
- Added LFS_REVDBG, which adds debug info to revision counts.
This initializes the bottom 12 bits of every revision count with a
hint based on rbyd type, which may be useful when debugging:
- 68 69 21 v0 (hi!.) => mroot anchor
- 6d 72 7e v0 (mr~.) => mroot
- 6d 64 7e v0 (md~.) => mdir
- 62 74 7e v0 (bt~.) => file btree node
- 62 6d 7e v0 (bm~.) => mtree node
This may be overwritten by the recycle counter if it overlaps, worst
case the recycle counter takes up the entire revision count, but these
have been chosen to at least keep some info if partially overwritten.
To make this work required the LFS_i_INMTREE hack (yay global state),
but a hack for debug info isn't the end of the world.
Note we don't have control over data blocks, so there's always a
chance they end up containing what looks like one of the above
revision counts.
- Renamed LFS_NOISY -> LFS_REVNOISE
- LFS_REVDBG and LFS_REVNOISE are incompatible, so using both asserts.
This also frees up the theoretical 0x00000030 state for an additional
rev mode in the future.
- Adopted LFS_REVNOISE (and LFS_REVDBG) in btree nodes as well.
If you need rev noise, you probably want it in all rbyds/metadata
blocks, not just mdirs.
---
This had no effect on the default code size, but did affect
LFS_REVNOISE:
code stack ctx
before: 35688 2440 640
after: 35688 (+0.0%) 2440 (+0.0%) 640 (+0.0%)
revnoise before: 35744 2440 640
revnoise after: 35880 (+0.4%) 2440 (+0.0%) 640 (+0.0%)
default: 35688 2440 640
revdbg: 35912 (+0.6%) 2448 (+0.3%) 640 (+0.0%)
revnoise: 35880 (+0.5%) 2440 (+0.0%) 640 (+0.0%)
Bit of a silly, but problematic, bug, probably introduced during the
various lfsr_bptr_t/lfsr_data_t reworks, but basically we never actually
fragmented the last fragment in a bptr.
We were fragmenting all fragments in a bptr _above_ fragment_size, but
then we'd stop at the last fragment and keep it around as a bptr,
completely wasting all of the work to fragment the block. The reason for
the different behavior being that we can combine the last fragment with
the carved data to avoid an additional commit.
Fortunately the solution is pretty non-invasive. We can just assume any
bptrs <= fragment_size should be written out as fragments.
Added test_fwrite_truncate_litmus_fragment and
test_fwrite_fruncate_litmus_fragment to catch this in the future.
Code changes:
code stack ctx
before: 35588 2448 640
after: 35600 (+0.0%) 2448 (+0.0%) 640 (+0.0%)
So now crystal_thresh only controls when fragments are compacted into
blocks, while fragment_thresh controls when blocks are broken into
fragments. Setting fragment_thresh=-1 will follow crystal_thresh and
keeps the previous behavior.
These were already two separate pieces of logic, so it makes sense to
provide two separate knobs for tuning.
Setting fragment_thresh lower than crystal_thresh has some potential to
reduce hysteresis in cases where random writes push blocks close to
crystal_thresh. It will be interesting to explore this more when
benchmarking.
---
The additional config option adds a bit of code/ctx, but hopefully that
will go away in the future config rework:
code stack ctx
before: 35584 2480 636
after: 35600 (+0.0%) 2480 (+0.0%) 640 (+0.6%)
This lets us cram in one more mask for potential redund bits:
name tag mask
LFSR_TAG_MASK0 0x0000 0x0fff ---- 1111 1111 1111
LFSR_TAG_MASK2 0x1000 0x0ffc ---- 1111 1111 11--
LFSR_TAG_MASK8 0x2000 0x0f00 ---- 1111 ---- ----
LFSR_TAG_MASK12 0x3000 0x0000 ---- ---- ---- ----
'.-' '.-' '---.---'
mode bits -' | | ^
suptype ------' | |
subtype --------------' |
redund bits ------------------'
I toyed around with a bitwise alternative to the lookup table, but
couldn't come up with anything simpler than these:
- 0xfff & ~((((1<<((i>>1)*8))-1) << ((i&1)*4)) | ((1<<(i*2))-1))
- 0xfff & ~((1 << (((i>>1)*8)+((i&1)<<(1+(i>>1)))))-1)
- 0xfff & ~((1<<(2*i*i))-1) (requires multiply and 32-bit shift)
---
This also replaces the mdir/rbyd/btree/mtree lookup/sublookup/suplookup
functions with a single flexible lookup function that accepts tag masks.
This ended up adding a bit of code/stack (the extra NULL args are
surprisingly pricey), but will hopefully make the redund bits
easier/cheaper to use:
code stack ctx
before: 35548 2472 636
after: 35584 (+0.1%) 2480 (+0.3%) 636 (+0.0%)
This was a surprising side-effect the script rework: Realizing the
internal btree/rbyd lookup APIs were awkwardly inconsistent and could be
improved with a couple tweaks:
- Adopted lookupleaf name for functions that return leaf rbyds/mdirs.
There's an argument this should be called lookupnextleaf, since it
returns the next bid, unlike lookup, but I'm going to ignore that
argument because:
1. A non-next lookupleaf doesn't really make sense for trees where
you don't have to fetch the leaf (the mtree)
2. It would be a bit too verbose
- Adopted commitleaf name for functions that accept leaf rbyds.
This makes the lfsr_bshrub_commit -> lfsr_btree_commit__ mess a bit
more readable.
- Strictly limited lookup and lookupnext to return rattrs, even in
complex trees like the mtree.
Most use cases will probably stick to the lookupleaf variants, but at
least the behavior will be consistent.
- Strictly limited lookup to expect a known bid/rid.
This only really matters for lfsr_btree/bshrub_lookup, which as a
quirk of their implementation _can_ lookup both bid + rattr at the
same time. But I don't think we'll need this functionality, and
limited the behavior may allow for future optimizations.
Note there is no lfsr_file_lookup. File btrees currently only ever
have a single leaf rattr, so this API doesn't really make sense.
Internal API changes:
- lfsr_btree_lookupnext_ -> lfsr_btree_lookupleaf
- lfsr_btree_lookupnext -> lfsr_btree_lookupnext
- lfsr_btree_lookup -> lfsr_btree_lookup
- added lfsr_btree_namelookupleaf
- lfsr_btree_namelookup -> lfsr_btree_namelookup
- lfsr_btree_commit__ -> lfsr_btree_commit_
- lfsr_btree_commit_ -> lfsr_btree_commitleaf
- lfsr_btree_commit -> lfsr_btree_commit
- added lfsr_bshrub_lookupleaf
- lfsr_bshrub_lookupnext -> lfsr_bshrub_lookupnext
- lfsr_bshrub_lookup -> lfsr_bshrub_lookup
- lfsr_bshrub_commit_ -> lfsr_bshrub_commitleaf
- lfsr_bshrub_commit -> lfsr_bshrub_commit
- lfsr_mtree_lookup -> lfsr_mtree_lookupleaf
- added lfsr_mtree_lookupnext
- added lfsr_mtree_lookup
- added lfsr_mtree_namelookupleaf
- lfsr_mtree_namelookup -> lfsr_mtree_namelookup
- added lfsr_file_lookupleaf
- lfsr_file_lookupnext -> lfsr_file_lookupnext
- added lfsr_file_commitleaf
- lfsr_file_commit -> lfsr_file_commit
Also added lookupnext to Mdir/Mtree in the dbg scripts.
Unfortunately this did add both code and stack, but only because of the
optional mdir returns in the mtree lookups:
code stack ctx
before: 35520 2440 636
after: 35548 (+0.1%) 2472 (+1.3%) 636 (+0.0%)
- mdir_bits -> mbits
- lfsr_mid_bid -> lfsr_mbid
- lfsr_mid_rid -> lfsr_mrid
These now match the naming in the dbg scripts.
I feel like this is more terse in a way that is also more readable, but
maybe that's just me.
And the related config options:
- cfg->file_buffer_size -> cfg->file_cache_size
- file->cfg->buffer_size -> file->cfg->cache_size
- file->cfg->buffer -> file->cfg->cache_buffer
The original motivation to rename this to file->buffer was to better
align with what other filesystems call this, but I think this is a case
where internal consistency is more important than external consistency.
file->cache better matches lfs->pcache and lfs->rcache, and makes it
easier to read code involving both file->cache and other user-provided
buffers.
Keeping the upstream name also helps with continuity.
This does a couple things:
- Makes attr-lists a bit more self-documenting.
- Adds a bit more type-safety. The LFSR_RATTR_* macros should be able to
reject types that don't match the expected encoding.
- Makes it easier to adjust dsize estimates at one location.
Specifically, this makes it harder to forget bptr's LFSR_BPTR_DSIZE.
---
Surprisingly this did have a small impact on code size. I'm not entirely
sure why, but considering how much of the codebase this touches I'm just
going to chalk this up to compiler noise:
code stack ctx
before: 35488 2440 636
after: 35536 (+0.1%) 2440 (+0.0%) 636 (+0.0%)
lfsr_file_carve seems the hardest hit:
function (0 added, 0 removed) osize nsize dsize
lfsr_file_open 16 20 +4 (+25.0%)
lfsr_file_carve 1316 1356 +40 (+3.0%)
lfsr_remove 408 412 +4 (+1.0%)
TOTAL 35488 35536 +48 (+0.1%)
Christopher Haster