- Off-by-one in test_btree_find_general[_sparse]_fuzz
Because we can only create named btrees via splitting, these always
start with one entry. If all operations are randomly selected to be
splits, this can lead to an overflow of the sim buffer (sounds
unlikely, but relatively easily for small N).
The fix is to use a `for (lfs3_size_t i = 1; i < N; i++)` loop to
account for the initial entry. Note we already use this in the
test_btree_split_* tests.
An alternative is allocating space for N+1 entries, but this seems
unintuitive with N usually being associated with the upper bound on
btree size.
- Off-by-one in our sim rename pattern
When renaming, we don't bother to update sim_size, because after the
rename the sim_size size will be unchanged. But this means the
sim_size is out-of-date during the memmove that reinserts the renamed
entry. Buffer overflow!
To fix we just need to use sim_size-1 to account for the temporarily
deleted entry.
This is messy C code, so not surprised it went unnoticed, even though
this pattern ended up in quite a few tests.
Found while running with HEAP=1. This was just intended to test HEAP=1,
but I guess the injected heap hooks result in a more fragile heap? They
increase all allocations by one word, and maybe this reduces alignment
padding? Not exactly sure.
But it's a good argument for maybe adding heap canaries in the future.
Previously we ran Valgrind on all tests, but it's unclear if this will
still be reasonable with the number of tests we have now.
This is based on some work in external benchmarks. What's worked well
there is emulating a global simtime based on per-byte estimates.
This moves the emulated simtime into emubd/kiwibd, and extends the idea
with both per-byte and per-op timing estimates for hopefully more
realistic results.
---
The problem is how NAND flash reads work.
Per-byte timing estimates are surprisingly accurate for NOR flash. There
is some overhead for sending the address, but it's mostly dominated by
bus cost (~20ns/B [1]).
NAND flash, on the otherhand, technically does support byte-level reads,
but first needs to read into 2KiB buffer. Surprisingly, these are pretty
close in cost (~19ns/B bus [2] vs ~12ns/B buffer [2]).
This close-ness makes modeling NAND flash difficult. If we set
read_size=1, we risk hiding the cost of small reads, which littlefs3 is
full of (rbyd lookups). If we set read_size=2048, we unfairly penalize
littlefs3 for the same reason.
---
The solution here is to expose both per-byte and per-op timing
estimates. This lets you model NAND reads using two data points:
^
| realtime --> ...............o
| : .....'''' :
| ...............:'''' ^ :
| :....''''' | :
| ..........:::::: simtime :
| .....:'''' :
|o....:::::.....: :
|: :
|: :
+:-----------------------------------------------------------:>
min read max read
Where:
bus_timing = 19ns
buffer_timing = 25us
buffer_size = 2KiB
erase_size = 128KiB
min_read = buffer_timing
max_read = (erase_size/buffer_size)*buffer_timing - buffer_timing
read_timing = min_read
readed_timing = ((max_read - min_read)/erase_size) + bus_timing
simtime = reads*read_timing + readed*readed_timing
(per-op) (per-byte)
This should correctly penalize small reads without complicating
emubd/kiwibd too much.
That's the idea anyways! It will take some use to understand if this is
a reasonable approach.
As a plus, this is a superset of the per-byte model, so both can be used
for realistic vs idealistic simulations (and to test the bus+buffer
model itself).
1: https://www.winbond.com/resource-files/W25Q256JV%20SPI%20RevQ%2002072025%20Plus.pdf
2: https://www.winbond.com/resource-files/W25N01GV%20Rev%20R%20070323.pdf
I just noticed we weren't testing preerase with non-0xff ecksums at all!
Added to relevant tests:
# test with a number of different erase values
defines.ERASE_VALUE = [0xff, 0x00, -1]
The most important non-0xff value being -1 (noop erases), which should
usually result in fragmented ecksums.
Note this was copied from test_rbyd, where we do something similar to
test non-0xff ecksums in rbyd logs.
This gets gc tests working with both LFS3_GC=1 and LFS3_PREERASE=1, and
adds a few more tests that should round out the necessary preerase test
coverage:
- test_gc_preerase_progress - A simple test that checks if
LFS3_GC_PREERASE clears the LFS3_I_PREERASE flag, as well as some
checks against emubd's erase counters to see if it actually did
anything (erased >= cycles - preerased, erased < 1.25*cycles -
preerased).
- test_gc_preerase_relaxed - A test with a couple different
GC_PREERASE_COUNTs, and checks against emubd's erase counters to make
sure they demonstrate different levels of pre-erasing (erased >=
cycles - preerased, erased < 1.25*cycles - preerased).
- test_gc_preerase_decreasing - A test with increasing
GC_PREERASE_COUNTs, measuring min/max/avg emubd's erase counters, and
asserting if the avg delta is worst than ~0.75x.
This is probably the most valuable one, if only for the extra analysis
available when debugging.
And, just so we know these tests are working, they found a few more bugs:
- We were calling the implicitly ckpointing variant of lfs3_mdir_commit
in lfs3_allocclaim, when the block we just allocated is still very
much in-flight!
An easy one-character fix (lfs3_mdir_commit -> lfs3_mdir_commit_, the
non-ckpointing variant), but was a pain to track down. I guess the
good news is test_gc_nospc has proven to be a very valuable test.
Added a comment to hopefully discourage a regression.
- Found a wacky catch-22 where the block we just preerased can be
allocated during the gbmap commit that tries to save the preerased
ecksum.
This is somewhat expected during normal operation, the gbmap may need
a few allocations before the preeraser can get ahead, but we need to
make sure not to increment the preeraser's known window if the
preerased block is no longer in the gbmap's known window.
Fortunately(?), our preeraser state is pretty robust to bugs like this
due to being reset (forcing ecksum refetches) during gbmap rebuilds.
However, preeraser state falling out-of-sync risks unnecessary
erases/surprising latency during block allocation.
- Found a typo where we used lfs3->cfg->block_count instead of
lfs3->block_count again... Hopefully this becomes impossible after the
planned config rework...
---
A few other test tweaks:
- Added LFS3_F/M_REVPERTURB flags where necessary to support PREERASE.
Previously the tests only worked with LFS3_YES_REVPERTURB=1.
- Adopt lfs3_handle_isopen over lfs3.handles == lfs3.gc.t.h. With the
logic change to use the traversal handle to track its position in the
open file handles, these simplified isopen checks no longer work.
- Prefer toml lists for multiple ifdefs (hey, these were at least useful
for testing test.py's ifdef exprs).
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38616 2144 776
gbmap after: 38616 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39232 2168 796
preerase after: 39280 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
- Fixed lfs3_alloc_cansyncgbmap ignoring known window changes.
Being able to just call lfs3_btree_cmp(b, b_p) would be nice, but this
ignores known window changes!
Fixed by comparing the on-disk encoding, which is heavy-handed, but
probably the safest approach.
lfs3_alloc_cansyncgbmap will probably never be on the stack hot-path,
and the added code is roughly one function call. The main cost is
CPU-cycles, but fortunately(?) that's not something we really care
about?
- Fixed lfs3_allocclaim accidentally returning lfs3_mdir_commit's return
value instead of the allocated block!
Probably caused by a copy-paste, resulted in lfs3_allocclaim returning
block 0, which is really not good!
- Fixed assert typo in lfs3_trv_open where we assert REVPERTURB in flags
instead of lfs3->flags.
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38560 2144 776
gbmap after: 38616 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39168 2168 796
preerase after: 39232 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
Still needs testing with LFS3_GC=1, and tests that intentionally test
preerasing, but this should at least fix most fsinfo.flag related issues.
Despite no intentional preerase testing, this already found a number of
issues. Most importantly: our ckpoint-agnostic gbmap zeroing was never
going to work with preerasing!
Main fixes:
- Adopted conservative zeroing of gbmap during rebuilds
This was the biggest change. Our previous lfs3_gbmap_zero impl was
never going to work with preerasing because it unconditionally
cleared BMERASED ranges.
Not entirely wrong, but a big waste of any preerase work.
It also causes the whole system to lock up when LFS3_GC_LOOKAHEAD and
LFS3_GC_PREERASE fight to make progress. With LFS3_GC_LOOKAHEAD
clearing BMERASED ranges, and LFS3_GC_PREERASE clearing the lookahead
flag, nothing gets done!
---
The fix was to rewrite lfs3_gbmap_zero[unknown] to only zero BMERASED
(and BMINUSE, though this isn't strictly necessary) ranges in the
unknown window. This keeps any known-preerased blocks around and
avoids throwing that information away.
This is also slightly different from BMBAD ranges, which we want to
keep around forever, even if in the unknown window.
Whether or not was should limit zeroing BMINUSE ranges is an
interesting question. If we already need this logic, I think extending
it to BMINUSE is a good idea because of how it limits gbmap commits
during rebuilds:
- Unfortunately, gbmap rebuilds require quite a few commits to both
(1) zero gbmap state, and (2) set all the in-use blocks to BMINUSE.
This is especially concerning when relying on aggressive gc, such as
gc_lookgbmap=-1, which may trigger rebuilds when only a couple
blocks are allocated.
Limiting zeroing limits gbmap commits in two ways:
1. We only need to update ranges in the unknown window, which
shrinks with more aggressive gbmap rebuilds.
2. By not clearing BMINUSE ranges in the known window, populating
those blocks during the lookgbmap scan should be a noop.
Together, this hopefully makes aggressive gbmap rebuilds relatively
cheap, at least in terms of progs/erases.
- It's slightly simpler if BMINUSE and BMERASED are handled the same.
- Actually increment the preeraser known window in lfs3_alloc_inc.
Otherwise our estimated preeraser.count only ever increases! There was
some trickiness to make sure preeraser.count is only ever decremented
when allocating erased blocks, but fortunately lfs3->gbmap.ecksum's
existence can tell us that information.
- Reset preeraser state during gbmap rebuilds.
Also necessary to avoid unbounded preeraser.count. The simplest
solution is to zero the preeraser, which forces it to rescan the gbmap
for BMERASED ranges. The preeraser strictly avoids redundant erases.
This does require extra gbmap lookups during LFS3_GC_PREERASE, but
that's not the end of the world.
- Avoid erasing corrupted preerased blocks in case there's other
preerased blocks available in our gbmap.
This happens when the ecksum check fails, implying a prog was
attempted, but power was lost.
Before this change (the continue in lfs3_alloc_:11244), we were
erasing corrupt ecksums, which is not _wrong_, but sort of defeats the
purpose of prerasing. Skipping the block and trying another:
1. Is better in terms of wear-leveling (try not to double erase!)
2. Minimizes latency if we have other preerased blocks we can use
- Made lfs3_fs_gc_ preerasing actually conditional on the
LFS3_GC_PREERASE flag.
Before, lfs3_fs_gc_ was unconditionally preerasing, which is wrong!
---
Currently passing:
LFS3_YES_GBMAP=1 \
LFS3_YES_REVPERTURB=1 \
LFS3_PREERASE=1 \
make test-runner -j \
& ./scripts/test.py -j -b
Other test fixes:
- Mostly just adding the necessary LFS3_I_PREERASE flags for all
lfs3_fs_stat calls.
- LFS3_I_PREERASE and LFS3_I_LOOKAHEAD can interact in funny ways. Just
needed testing.
- lfs3_trv_t doesn't actually do anything with LFS3_T_PREERASE, so we
shouldn't try to test it.
- Adopted lfs3_fs_ck instead of explicit traversals where possible.
- test_badblocks_*_btree_many was still running with LFS3_YES_GBMAP, but
it shouldn't be. The gbmap state is undefined during internal btree
tests.
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38492 2144 776
gbmap after: 38560 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39036 2168 796
preerase after: 39168 (+0.3%) 2168 (+0.0%) 796 (+0.0%)
Maybe the subject line should say "Implemented", because these never
worked in the first place. Unfortunately our tests missed this due to a
couple reasons:
- Mroot chains are difficult to create due to the required exponential
growth.
- The only thing that actually commits to chain mroots is mdir
compaction. Though this functionality will be useful for future block
eviction/error correction.
- Previous revision count issues were making relocations in our
compaction tests unlikely.
Fortunately, now that revision count behavior is more correct, our tests
are correctly highlighting that this is broken.
---
Implementing chain mroot commits was a bit intimidating, but fortunately
it just required a bit of teasing to get lfs3_mdir_commit_ to trigger
the tail-recursive mroot chain update when the mdir is a non-active
mroot.
The gcksum is also doing a great job here with identifying bugs. Without
it this bug would have been difficult to notice, since compactions
otherwise have no observable effect on the system.
Code changes:
code stack ctx
before: 35164 2136 660
after: 35224 (+0.2%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38400 2144 776
gbmap after: 38464 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38940 2168 796
preerase after: 39008 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
This was introduced with the simplified traversal clobbering logic.
Previously, traversal clobbering was a bit more aggressive, relying on
the explicit tstate state machine. This was replaced by implicit
mid-related state, which looks like it may have introduced some holes.
In this case, lfs3_mdir_commit was failing to clobber non-active mroot
chain mdirs. Non-active mroots are particularly tricky because we
(1) don't track these in-RAM, (2) only reach them during traversals,
and (3) require heavy wear-leveling writes for them to even appear in
in system.
---
The solution here is an extra check in lfs3_mdir_commit_'s post-commit
state updates to update any mid<=-1 mroots to the new active mroot.
This clobbers mroot chain traversals by skipping non-active mroots, but
this is unavoidable since lfs3_mdir_commit_ could always introduce
new/relocate mroot chain mroots. Note this should match the previous
state-machine dependent behavior.
Code changes:
code stack ctx
before: 35144 2136 660
after: 35152 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38380 2144 776
gbmap after: 38392 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38920 2168 796
preerase after: 38928 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
This mostly reverts the previous commit, and makes non-NULL ecksums the
consistent API.
Non-NULL ecksums are what the original rbyd-level ecksum API expects,
and enforcing this avoids the ifdef mess required to minimize unused
code impact.
This unfortunately clutters up lfs3_gbmap_set_'s logic with NULL checks,
but at least keeps the mess constrained to lfs3_gbmap_set_.
lfs3_gbmap_set_ is really the only function that uses NULL ecksums, so
they should probably be lfs3_gbmap_set_'s problem to deal with.
Code changes:
code stack ctx
before: 35144 2136 660
after: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38296 2144 776
gbmap+np after: 38272 (-0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38908 2168 796
gbmap+yp after: 38940 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
So:
- lfs3_alloc_markinuse -> lfs3_alloc_setinuse
- lfs3_alloc_markinusebptr -> lfs3_alloc_setinusebptr
- lfs3_gbmap_mark_ -> lfs3_gbmap_set_
- lfs3_gbmap_mark -> lfs3_gbmap_set
- lfs3_gbmap_markbptr -> lfs3_gbmap_setbptr
IMO a bit of a better name now that lfs3_gbmap_set* also stores optional
ecksums.
The main change is adding LFS3_M_REVPERTURB, which will be necessary for
preerase allocations, but I got distracted and ended up giving the
revision count subsystem a bit of a refactor.
Main changes:
- Added LFS3_M_REVPERTURB, which ensures the leading bit in the
revision count changes after each allocation/relocation/compaction.
This is generally optional, but will be required for preerase
allocations. Our ecksum system is only reliable if we ensure at least
one bit changes, otherwise the chance of ecksum collision is very
high.
The downside of LFS3_M_REVPERTURB is that we need to read the contents
of the new block to figure out what the bit should change to. Probably
a minimal cost in the system, but still a good reason to make the
behavior optional.
Does LFS3_M_REVPERTURB have any use outside of preerased allocation?
I'm not sure. Maybe it has some niche use reducing the chance of bd
ECC collisions?
- Dropped LFS3_M_REVDBG, but adding low-effort debug bits that are
always enabled.
Making LFS3_M_REVDBG conditional was probably overkill. The flag
checks probably cost more than the actual debug bits when enabled.
Instead, replaced with a simpler, low-effort debug bit system, where
we only set the debug bits during mdir allocation/relocation. These
bits shouldn't change during normal compaction, but we _don't_
introduce debug bits if mounting a filesystem from a driver without
these debug bits.
- Restricted recycle counter to at most 20-bits to make space for
things. This ensures perturb/debug bits don't get overwritten (though
we really only care about perturb bits).
2^20 (~1M) recycles is probably enough for any device littlefs will
run on, especially considering the recycle_count should probably be
several orders of magnitude smaller than the device's expected erase
cycles.
Worst case this can always be increased in the future without
backwards incompatible changes. The only hard requirement for revision
counts is that the full 32-bits are comparable.
- Simplified lfs3_rev_inc and friends, and moved most of the
disk-dependent revision count stuff down into lfs3_rbyd appendrev.
This deduplicates the messy revision count handling in
lfs3_btree_commit_.
Though note the implicit lfs3_rbyd_appendrev now defaults to writing
the btree debug bits ('b'). A bit of a hack, but works for littlefs.
Here's the resulting encoding:
vvvv---- -------- -------- -ddddddd
vvvvrrrr rrrrrr-- -------- -ddddddd
vvvvrrrr rrrrrrnn nnnnnnnn pddddddd
'-.''----.----''----.----' ^'--.--'
'------|----------|------|---|---- 4-bit relocation revision
'----------|------|---|---- recycle-bits recycle counter
'------|---|---- pseudorandom noise (if revnoise)
'---|---- perturb bit (if revperturb)
'---- low-effort debug bits
11-1--- - h = mroot anchor
11-11-1 - m = mdir
11---1- - b = btree node
Note we store revision counts as le32s, so the perturb bit should end up
as the leading bit in the first byte.
Costs a bit more code (mostly because the debug bits are now
unconditional, even if low-effort), but simplifies the codebase:
code stack ctx
before: 35124 2136 660
after: 35144 (+0.1%) 2136 (+0.0%) 660 (+0.0%)
after+yesrevperturb: 35192 (+0.2%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38252 2144 776
gbmap+np after: 38272 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
gbmap+np after+yrp: 38328 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38832 2168 796
gbmap+yp after: 38852 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
gbmap+yp after+yrp: 38908 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
This is hopefully a better alternative to LFS3_IFDEF_YES_* macros.
If we need special behavior for LFS3_IFDEF_YES_*, we almost always need
special behavior for LFS3_IFDEF_NO_* and LFS3_IFDEF_MAYBE_* as well.
So merging all three states into a single macro saves typing and
hopefully encourages correct handling of all cases.
No code changes.
Allocating pre-erased blocks gets quite complicated due to our
restricted flash model, but at least the actual pre-erasing is
relatively straightforward:
- We keep track of known preerased state in lfs3->gbmap.preeraser.
- If LFS3_GC_PREERASE is provided during gc work, we increment the
preeraser's known window by scanning the gbmap.
- Any BMFREE ranges we find, we erase a block at a time, and store the
resulting ecksum in a BMERASED range in the gbmap.
- We keep track of how many blocks we erased, and stop early if this
exceeds cfg.gc_preerase_count. This just lets users tune how many
blocks to preerase in case something (?) prevents preerased blocks
from being used.
Some notes:
- We don't really do anything with ranges in lfs3_alloc_preerase. In
theory we could bulk in erase to minimize the number of commits to the
gbmap, but we expect erase to dominate, so this probably isn't worth
it.
And if erase doesn't dominate, why would you bother pre-erasing
blocks?
- Preerasing isn't really a traversal operation, and is managed by a
sort of secondary state machine in lfs3_fs_gc_.
This also means lfs3_trv_read with LFS3_T_PREERASE does nothing, but I
guess that is ok? It's tempting to try to make lfs3_trv_read also
preerase, but it's unclear what block it should return -- it's
probably the wrong API.
- Introducing ecksums actually went quite a bit smoother than I
expected. Though it helps ecksums are the only optional payload, no
type punning or anything.
Ecksums do muddy the gbmap's design a bit, unfortunately. The main
issue being that we can only merge BMERASED ranges with equal ecksums.
This makes BMERASED ranges less compressable than the others, and may
be one reason to limit cfg.gc_preerase_count.
However:
1. This is where I think it's useful to emphasize that the gbmap's
responsibility is to track _free_ blocks, in-use blocks are
secondary.
When allocating, we're going to stop at the first BMFREE/BMERASED,
but may need to skip over an unbounded number of BMINUSE/BMBAD
blocks. So the compressability of BMFREE/BMERASED ranges should
have less of an impact on block allocation.
2. In practice, most flash uses consistent erase values, so the
resulting ecksums will probably be compressable. The exceptions are
noop-erases (SD/eMMC, RAM, NVRAM, etc), and encryption with block
address permutation?
Though noop-erases are a pretty big exception.
Code changes:
code stack ctx
before: 35116 2136 660
after: 35116 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38040 2136 776
gbmap+np after: 38188 (+0.4%) 2144 (+0.4%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38040 2136 776
gbmap+yp after: 38608 (+1.5%) 2144 (+0.4%) 796 (+2.6%)
Will revert.
The idea here is that fixgrm isn't really a traversal operation. It's
convenient, but in an effort to simplify things, dropping fixgrm from
lfs3_trv_read makes sense.
But dropping fixgrm seems to cause more problems than it's worth.
---
Note test_trvs is currently failing because attempting to remove an
orphaned stickynote in the grm queue without calling fixgrm breaks
things.
It's probably fixable, but why? If we keep the implied fixgrm it's not
possible to trigger a remove without a clean grm queue. And we want to
keep our grm queue clean anyways to prevent a full fixorphan scan.
Code changes:
code stack ctx
before: 35164 2136 660
after: 35112 (-0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38088 2136 776
gbmap after: 38040 (-0.1%) 2136 (+0.0%) 776 (+0.0%)
This effectively reverts 1f824a0:
- LFS3_T_COMPACTMETA -> LFS3_T_COMPACT
- gc_compactmeta_thresh -> gc_compact_thresh
And friends.
After using LFS3_T_COMPACTMETA for a bit, I think it just adds noise
without much value. Especially when next to LFS3_T_LOOKAHEAD,
LFS3_GC_PREERASE, LFS3_M_SYNC, etc.
It's interesting that we already have some very distinct verbs for this
sort of thing based on data type (compact => metadata, garbage-collect
=> disk, compress => data).
Our flag space is already really packed, and I'm not sure having these
as separate flags is meaningful or useful for users. They both indicate
to repopulate allocators, and most users probably won't care that there
are two subtly different allocators operating under the hood.
There's an argument that LOOKAHEAD not touching disk is a useful
distinction, but in practice you really only need LOOKAHEAD work when
mounted RDWR.
So, merged the behaviors of LOOKAHEAD + LOOKGBMAP such that
LFS3_*_LOOKAHEAD requests repopulation of all allocators based on
gc_lookahead_thresh and gc_lookgbmap_thresh.
In priority order (some notes below):
1. If max(lookahead, gbmap) < gc_lookahead_thresh => repop lookahead
2. If gbmap < gc_lookgbmap_thresh => repop gbmap
As a plus, this makes it easier to avoid LFS3_IFDEF_GBMAP mess.
---
It's interesting to note LFS3_*_LOOKAHEAD will still repopulate the
lookahead buffer when the gbmap is present, but only if this would gain
more knowledge than was is currently in the gbmap.
I considered disabling lookahead scans completely when we have a gbmap,
but repopulating the lookahead buffer is still useful if the gbmap is at
risk of exhaustion. This is what gc_lookahead_thresh is for anyways, and
users can set gc_lookahead_thresh=0 if they want to disable this
behavior.
Relatedly, lookahead scans are actually prioritized over gbmap scans
(when they would gain knowledge). In theory this minimizes gc latency,
as gbmap scans risk triggering a full lookahead scan when building the
new gbmap.
---
Code changes minimal:
code stack ctx
before: 35152 2136 660
after: 35152 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38076 2136 776
gbmap after: 38080 (+0.0%) 2136 (+0.0%) 776 (+0.0%)
May rerevert this in the future, but I'm on the fence.
It's true this only saves a small amount of code, but in theory it also
reduces stack consumption in name-related functions. Currently this
doesn't affect the stack hot-path, which is a bit surprising as this
includes lfs3_set, but it may in the future.
The arguments against this optimization are also a bit weak:
- Non-null-terminated strings - We probably shouldn't optimize for a
theoretical future feature. If anything, we want to optimize in the
opposite direction to best measure the theoretical code cost.
- Precomputing strlen early - While this is generally a good idea, our
rattrs benefit greatly from compact encodings, as rattrs sitting on
the stack are one of the bigger contributors to our stack hot-path.
So for now I'm unreverting to see how long this optimization makes
sense, but could see this being rereverted in the future.
At the very least we probably want to keep the test changes to make
future testing easier.
---
Saves a bit of code:
code stack ctx
before: 35188 2136 660
after: 35160 (-0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38048 2152 772
gbmap after: 38020 (-0.1%) 2152 (+0.0%) 772 (+0.0%)
As much as I don't want to admit it, our 3-word lfs3_data_t struct is
just too large to be treated as pass-by-value with today's compilers.
It's a real shame, because I don't think there's a great technical
reason, just that compiler's pass-by-value optimizations generally stop
after 2 words.
If we could expect 16-bit block sizes (off and size), we could fit in
2 words, but this is already challenged by today's NAND chips
(bs>=128KiB).
---
So, as a compromise, this stops treating lfs3_data_t as pass-by-value,
with the exception of the lfs3_data_from* functions that still return
lfs3_data_t directly.
So instead of:
lfs3_data_t data = lfs3_data_fromecksum(&ecksum, buffer);
data = lfs3_data_slice(data, 8, -1);
return lfs3_data_size(data);
Most operations take lfs3_data_t by pointer:
lfs3_data_t data = lfs3_data_fromecksum(&ecksum, buffer);
lfs3_data_slice(&data, 8, -1);
return lfs3_data_size(&data);
One of the main consequences is there are now several ways to slice data
(internally these all redirect to lfs3_data_slice), and LFS3_DATA_SLICE
will likely see more use since we need temporary allocations to pass the
data slice by address:
- lfs3_data_slice(data, a, b) - Slices the data in place
- lfs3_data_fromslice(data, a, b) - Returns a new data slice
- LFS3_DATA_SLICE(data, a, b) - Creates a new compound-literal slice
---
As a pragmatic compromise, this saves a nice chunk of both code and
stack:
code stack ctx
before: 35316 2176 660
after: 35188 (-0.4%) 2136 (-1.8%) 660 (+0.0%)
code stack ctx
gbmap before: 38172 2192 772
gbmap after: 38048 (-0.3%) 2152 (-1.8%) 772 (+0.0%)
I don't think there was anything inherently wrong with this idea, but:
- The code savings (28 bytes) was surprisingly small.
- Expecting lfs3_path_namelen may be a headache for future
non-null-terminated string support.
- Even if you don't care about non-null-terminated strings, precomputing
strlen as early as possible is a good idea to minimize repeated strlen
scans.
Reverting adds a bit of code:
code stack ctx
before: 35288 2176 660
after: 35316 (+0.1%) 2176 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38140 2192 772
gbmap after: 38168 (+0.1%) 2192 (+0.0%) 772 (+0.0%)
I was poking around at possibly inlining small (<=255) name lens in
lfs3_rattr_t, but realized all LFS3_FROM_NAME rattrs in our system
already use the lfs3_path_namelen pattern (terminates in either
'\0' or '/').
Well, except for our tests, but who cares about those.
Adopting lfs3_path_namelen in LFS3_FROM_NAME saves a bit of code:
code stack ctx
before: 35316 2176 660
after: 35288 (-0.1%) 2176 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38168 2192 772
gbmap after: 38140 (-0.1%) 2192 (+0.0%) 772 (+0.0%)
It's funny to see what originally started as a simple list of rbyd attrs
slowly morph into a full isa. But it makes sense. What we really want is
an abstract description of operations that can be played and replayed as
necessary to atomically update the mtree.
Using a fixed lfs3_rattr_t struct to represent this in C is easy, and
avoids strict-aliasing issues, but ultimately limited when it comes to
the wide-range of data we want to attach to attributes.
Unlike a computer's isa, we want to be able to include full 12-24 byte
branch pointers directly in the instruction!
---
So here's a full variable-length isa organized by words (max(uintptr_t,
uint32_t)).
The first 32-bit word extends the 16-bit tag with an extra 16-bits of
control information:
wwll llff ffcc cccc tttt tttt tttt tttt
^'-.-''-.-''--.--' : :
'--|----|-----|----:-----------------:-- compressed weight
:: '----|-----|----:-----------------:-- total len
:: '-----|----:-----------------:-- from encoder
:: '----:-----------------:-- optional count
:: rgmm kkkk -kkk kkkk
11 => w=-1 ^^ ^ '-.' '---.---'
00 => w=0 '|-|---|------|------ rm bit
01 => w=+1 '-|---|------|------ grow bit
10 => w=attached '---|------|------ mask bits
'------|------ tag suptype
'------ tag subtype
The 4-bit length field always encodes the full length of the
instruction, including the instruction itself and optional weight. The
4-bit from + 6-bit count fields operate independently and tell
lfs3_rbyd_appendrattr_ how to actually encode the data related to the
instruction.
To work around strict-aliasing issues, complex structs are expected to
be broken down into words and reconstructed in lfs3_rbyd_appendrattr_.
Most of our structs are organized into words anyways. For example:
// new child
*r++ = LFS3_RATTR(5, LFS3_TAG_BRANCH, -2, LFS3_FROM_BRANCH);
*r++ = LFS3_RATTR_WEIGHT(+child_->weight);
*r++ = LFS3_RATTR_ARG(child_->blocks[0]);
*r++ = LFS3_RATTR_ARG(child_->trunk);
*r++ = LFS3_RATTR_ARG(child_->cksum);
This also changes rattr-lists to be null-terminated, which makes a bit
more sense in a variable-length isa:
*r++ = LFS3_RATTR_NULL; // all zeros, including length
One concern with null-terminated rattr-lists is how easy it is to
forget the null-terminator, but an assert that all non-null rattrs have
non-zero length seemed to catch the many many mistakes during adoption.
Alternatively, separate LFS3_FROM_NULL/LFS3_FROM_NIL from fields could
be used if encoding space gets tight.
I'm also quite happy with the 2-bit weight feild, which allows omitting
the optional weight word for -1,0,+1 weights. These should cover at
least all mdir operations.
Note the exact encoding of the rattr fields is less of a concern than
the tag fields, as it doesn't reside on-disk can be changed on whim.
---
Saves a nice chunk of code and stack:
code stack ctx
before: 35920 2280 660
after: 35324 (-1.7%) 2176 (-4.6%) 660 (+0.0%)
code stack ctx
gbmap before: 38812 2296 772
gbmap after: 38156 (-1.7%) 2192 (-4.5%) 772 (+0.0%)
The stack savings are obvious, but the code savings a bit less so. A
variable length isa _is_ more complicated, but by limiting most encoding
decisions to compile-time (2-bit weights vs 32-bit weights for example),
the savings from fewer word manipulations on the stack wins.
Unintentionally arriving at the infamous "fsck" name is a bit funny.
But it's probably something we don't want to conflict with if we can
help it, on the off chance we want a sort of lfs3_fsck function in the
future. (This is all hypothetical, but lfs3_fsck may expect an unmounted
filesystem, and have a much larger scope than lfs3_fs_ck. Though typing
this out now I'm realizing how confusing that might be...)
Since lfs3_file_ck and lfs3_fs_ck share a subset of flags, it's not
_entirely_ unreasonable for lfs3_file_ck and lfs3_fs_ck to share the
same namespace.
There's a risk of confusing users around what flags lfs3_file_ck
accepts, but we have asserts, and said flags (LFS3_CK_MKCONSISTENT,
LFS3_CK_LOOKAHEAD, etc) just don't really make sense in lfs3_file_ck:
fs file
y LFS3_CK_MKCONSISTENT 0x00000800 Make the filesystem consistent
y LFS3_CK_LOOKAHEAD 0x00001000 Repopulate lookahead buffer
y LFS3_CK_LOOKGBMAP 0x00002000 Repopulate the gbmap
y LFS3_CK_PREERASE* 0x00004000 Pre-erase unused blocks
y LFS3_CK_COMPACTMETA 0x00008000 Compact metadata logs
y y LFS3_CK_CKMETA 0x00010000 Check metadata checksums
y y LFS3_CK_CKDATA 0x00020000 Check metadata + data checksums
y y LFS3_CK_REPAIRMETA* 0x00040000 Repair data blocks
y y LFS3_CK_REPAIRDATA* 0x00080000 Repair metadata + data blocks
* Planned
Another option would be to document that lfs3_fs_ck accepts both
LFS3_CK_* _and_ LFS3_GC_* flags, but I worry that would be more
confusing. It would also lock us into supporting all LFs3_GC_* flags in
lfs3_fs_ck, which may not always be the case.
Though this is an argument for doing away with the whole
LFS3_M/F/CK/GC/I_* duplication... (tbh another reason for this is to
reduce the number of namespaces by at least one).
No code changes.
TLDR: Replaced lfs3_file_ckmeta/ckdata and lfs3_fs_ckmeta/ckdata with
flag based ck functions:
- lfs3_file_ckmeta -> lfs3_file_ck + LFS3_CK_CKMETA
- lfs3_file_ckdata -> lfs3_file_ck + LFS3_CK_CKDATA
- lfs3_fs_ckmeta -> lfs3_fs_ck + LFS3_FSCK_CKMETA
- lfs3_fs_ckdata -> lfs3_fs_ck + LFS3_FSCK_CKDATA
Note lfs3_fs_ck is equivalent to lfs3_fs_gc, but:
1. Performs the work in one call (equivalent to littlefs2's lfs2_fs_gc)
2. Takes flags at call time (like lfs3_mount) instead of cfg time (like
lfs3_fs_gc)
3. Avoids the constant RAM necessary to track incremental GC state
---
Motivation:
I've been thinking: It's a bit weird that users are able to one-shot
janitorial work in lfs3_mount, but there's no equivalent function after
the filesystem is mounted.
Originally this is what lfs3_fs_gc was for, but after adding support for
incremental GC, it made sense to hide lfs3_fs_gc behind the opt-in
LFS3_GC ifdef due to the extra (ironically non-gc-able) state.
In theory lfs3_trv_t fills a bit of the gap, but, without the internal
i_flag handling and traversal restarts, it's a bit hard to use. And
basically requires duplicating said log, which we need anyways for
lfs3_mount!
So ideally we'd add an explicit one-shot GC function, but now lfs3_fs_gc
is taken.
While thinking about alternative names, I realized we can just call this
lfs3_fs_ck and completely replace lfs3_fs_ckmeta/ckdata.
This has some extra benefits:
- Avoids an explosion of ckmeta/ckdata/repairmeta/repairdata functions
- Discourages redundant traversals that could accomplish more work
- Makes it less confusing that ckdata implies ckmeta
---
I also tweaked lfs3_file_ck to match, but note that lfs3_file_ck is
internally very different from lfs3_fs_ck. For one, lfs3_file_ck only
supports "actual" check flags (LFS3_CK_*) vs all gc flags (LFS3_FSCK_*):
lfs3_file_ck:
LFS3_CK_CKMETA 0x00010000 Check metadata checksums
LFS3_CK_CKDATA 0x00020000 Check metadata + data checksums
LFS3_CK_REPAIRMETA* 0x00040000 Repair metadata blocks
LFS3_CK_REPAIRDATA* 0x00080000 Repair metadata + data blocks
* Planned
lfs3_fs_ck:
LFS3_FSCK_MKCONSISTENT 0x00000800 Make the filesystem consistent
LFS3_FSCK_LOOKAHEAD 0x00001000 Repopulate lookahead buffer
LFS3_FSCK_LOOKGBMAP 0x00002000 Repopulate the gbmap
LFS3_FSCK_PREERASE* 0x00004000 Pre-erase unused blocks
LFS3_FSCK_COMPACTMETA 0x00008000 Compact metadata logs
LFS3_FSCK_CKMETA 0x00010000 Check metadata checksums
LFS3_FSCK_CKDATA 0x00020000 Check metadata + data checksums
LFS3_FSCK_REPAIRMETA* 0x00040000 Repair metadata blocks
LFS3_FSCK_REPAIRDATA* 0x00080000 Repair metadata + data blocks
* Planned
As a plus, this also saves a bit of code:
code stack ctx
before: 35968 2280 660
after: 35924 (-0.1%) 2280 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38828 2296 772
gbmap after: 38812 (-0.0%) 2296 (+0.0%) 772 (+0.0%)
This more closely matches behavior of functions like mkdir and remove,
even though mkgbmap/rmgbmap operate on a special object and not files.
Besides, returning an error is more useful as users are always free to
ignore said error.
Adds what appears to be one literal to mkgbmap (curiously not rmgbmap?
snuck into alignment?):
code stack ctx
before: 35968 2280 660
after: 35968 (+0.0%) 2280 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38824 2296 772
gbmap after: 38828 (+0.0%) 2296 (+0.0%) 772 (+0.0%)
This includes the mask/rm/grow bits:
- LFS3_tag_RM
- LFS3_tag_GROW
- LFS3_tag_MASK0/2/8/12
Our in-device only handle types:
- LFS3_tag_ORPHAN
- LFS3_tag_TRV
- LFS3_tag_UNKNOWN
And in-device only tags with special behavior:
- LFS3_tag_INTERNAL
- LFS3_tag_RATTRS
- LFS3_tag_SHRUBCOMMIT
- LFS3_tag_GRMPUSH
- LFS3_tag_MOVE
- LFS3_tag_ATTRS
Usually I'm not a big fan of case-sensitive naming patterns, but this
has been useful for self-documenting what compat flags are in-device
only. Might as well extend the idea to our tag definitions.
Yeah, after using these for a bit, the RE* names were not great.
Trying LOOK* now, as an alternative that hopefully still implies the
similar behavior without needing an additional prefix for LOOKAHEAD:
- LFS3_*_RELOOKAHEAD -> LFS3_*_LOOKAHEAD
- LFS3_*_REGBMAP -> LFS3_*_LOOKGBMAP
- cfg.regbmap_thresh -> cfg.lookgbmap_thresh
- cfg.gc_relookahead_thresh -> cfg.gc_lookahead_thresh
- cfg.gc_regbmap_thresh -> cfg.gc_lookgbmap_thresh
I mean, why not? These redirect to the same internal lfs3_fs_gc_
function anyways. Might as well keep things consistent.
Added:
LFS3_F_MKCONSISTENT 0x00000800 Make the filesystem consistent
LFS3_F_RELOOKAHEAD 0x00001000 Repopulate lookahead buffer
LFS3_F_MKCONSISTENT is guaranteed to be a noop, but LFS3_F_RELOOKAHEAD
forces a filesystem traversal, which may have some niche use case.
No code changes.
These are unlikely to make much progress, but that doesn't seem like a
great reason to disallow these flags in lfs3_format:
LFS3_F_REGBMAP 0x00002000 Repopulate the gbmap
LFS3_F_COMPACTMETA 0x00008000 Compact metadata logs
These are actually guaranteed to do _no_ work when formatting _without_
the gbmap, but with the gbmap it's less clear. Looking forward to the
planned ckfactory feature, these may be useful for cleaning up any rbyd
commits created as a part of building the initial gbmap.
---
Also tweaked the formatting for LFS3_F_* flags a bit, including making
all ifdefs explicit (mainly ifdef LFS3_RDONLY). Mixed ifdefs are a real
pain to read.
No code changes.
LFS3_REVDBG introduced a lot of overhead for something I'm not sure
anyone will actually use (I have enough tooling that the state of an
rbyd is rarely a mystery, see dbgbmap.py). That, and we're running out
of flags!
So this reduces LFS3_REVDBG to just store one of "himb" in the first
(lowest) byte of the revision count; information that is easily
available:
vvvv---- -------- -------- --------
vvvvrrrr rrrrrr-- -------- --------
vvvvrrrr rrrrrrnn nnnnnnnn nnnnnnnn
vvvvrrrr rrrrrrnn nnnnnnnn dddddddd
'-.''----.----''----.- - - '---.--'
'------|----------|----------|---- 4-bit relocation revision
'----------|----------|---- recycle-bits recycle counter
'----------|---- pseudorandom noise (if revnoise)
'---- h, i, m, or b (if revdbg)
-11-1--- - h = mroot anchor
-11-1--1 - i = mroot
-11-11-1 - m = mdir
-11---1- - b = btree node
Some other notes:
- Enabled LFS3_REVDBG and LFS3_REVNOISE to work together, now that
LFS3_REVDBG doesn't consume all unused rev bits.
Note that LFS3_REVDBG has priority over LFS3_REVNOISE, but _not_
recycle-bits, etc. Otherwise problems would happen for recycle-bits
>2^20 (though do we care?).
- Fixed an issue where using the gcksum as a noise source results in
noise=0 when there is only an mroot. This is due to how we xor out
the current mdir cksum during an mdir commit.
Fixed by using gcksum_p instead of gcksum.
- Added missing LFS3_I_REVDBG/REVNOISE flags in the tests, so now you
can actually run the tests with LFS3_REVDBG/REVNOISE (this probably
just fell out-of-date at some point).
---
Curiously, despite LFS3_REVDBG/REVNOISE being disabled by default, this
did save some code. I'm guessing the non-tail-call mtree/gbmap commit
functions prevented some level of inlining?:
code stack ctx
before: 35964 2280 660
after: 35964 (+0.0%) 2280 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38940 2296 772
gbmap after: 38828 (-0.3%) 2296 (+0.0%) 772 (+0.0%)
Now that we use LFS3_ERR_BUSY for traversals, we no longer have an
excuse for not returning LFS3_ERR_BUSY on root-related errors:
- lfs3_remove(&lfs3, "/") => LFS3_ERR_BUSY
- lfs3_rename(&lfs3, "/", *) => LFS3_ERR_BUSY
- lfs3_rename(&lfs3, *, "/") => LFS3_ERR_BUSY
This better aligns with POSIX. Arguably we should have defined
LFS3_ERR_BUSY for this case anyways, it's not like additional error
codes cost much.
No code changes.
With the relaxation of traversal behavior under mutation, I think it
makes sense to bring back LFS3_T_EXCL. If only to allow traversals to
gaurantee termination under mutation. Now that traversals no longer
guarantee forward progress, it's possible to get stuck looping
indefinitely if the filesystem is constantly being mutated.
Non-excl traversals are probably still useful for GC work and debugging
threads, but LFS3_T_EXCL now allows traversals to terminate immediately
with LFS3_ERR_BUSY at the first sign of unrelated filesystem mutation:
LFS3_T_EXCL 0x00000008 Error if filesystem modified
Internally, we already track unrelated mutation to avoid corrupt state
(LFS3_t_DIRTY), so this is a very low-cost feature:
code stack ctx
before: 35944 2280 660
after: 35964 (+0.1%) 2280 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38916 2296 772
gbmap after: 38940 (+0.1%) 2296 (+0.0%) 772 (+0.0%)
code stack ctx
gc before: 36016 2280 768
gc after: 36036 (+0.1%) 2280 (+0.0%) 768 (+0.0%)
Mostly for consistency with mtrv.b and gbmap.b, but also (1) this
hopefully reduces confusion around the fact that these can refer to both
bshrubs and btrees, and (2) saves a bit of typing with the messy struct
namespaces forced by C's strict aliasing.
This solves the previous gc-needs-block-queue-so-we-can-clobber-block-
queue issue by adding an additional LFS3_t_STALE flag to indicate when
any block queues would be invalid.
So instead of clearing block queues in lfs3_alloc_ckpoint, we just set
LFS3_t_STALE, and any lfs3_trv_ts can clear their block queues in
lfs3_trv_read. This allows lfs3_mgc_ts to be allocated without a block
queue when doing any LFS3_M_*/LFS3_F_*/LFS3_GC_* work.
LFS3_t_STALE is set at the same time as LFS3_t_CKPOINT and LFS3_t_DIRTY,
but we need a separate bit so lfs3_trv_read can clear the flag after
flushing without losing ckpoint/dirty information.
---
Unfortunately, none of the stack-allocated lfs3_mgc_ts are on the stack
hot-path, so we don't immediate savings. But note the 2-words saved in
ctx when compiling in LFS3_GC mode:
code stack ctx
before: 35940 2280 660
after: 35944 (+0.0%) 2280 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38916 2296 772
gbmap after: 38916 (+0.0%) 2296 (+0.0%) 772 (+0.0%)
code stack ctx
gc before: 36012 2280 776
gc after: 36016 (+0.0%) 2280 (+0.0%) 768 (-1.0%)
The main idea here is to drop the flag-encoded tstate state machine, and
replace it with a matrix controlled by special mid + bid values:
-- mid ->
-5 -4 -3 -2 >=-1
bid -2 x x x --> mdir
v >=-1 x gbm gbm x --> bshrub/btree
'----|----|----|----|----> mroot anchor
'----|----|----|----> mroot chain + mtree
'----|----|----> gbmap (in-ram gbmap)
'----|----> gbmap_p (on-disk gbmap)
'----> file bshrubs/btrees
This was motivated by the observation that everything in our filesystem
can be modeled as mdir + bshrub/btree tuples, as long as some states are
noops. And we can cleanly encode these tuples in the unused negative
mid + bid ranges without needing an explicit state machine.
Well, that and the previous tstate state machine approach being an ugly
pile of switch cases and messy logic.
Note though that some mids may need to traverse multiple mdirs/bshrub/
btrees:
- The mroot chain + mtree (mid=-4) needs to traverse all mroots in the
mroot chain, and detect any cycles.
- File mdirs (mid>=-1) need to traverse both the on-disk bshrub/btree
and any opened file handles' bshrubs/btrees before moving onto the
next mid.
This grows O(n^2) because all file handles are in one big unsorted
linked-list, but as usual we don't care.
In addition to the greatly simplified traversal logic, the new state
matrix simplifies traversal clobbering: Setting bid=-2 always forces a
bshrub/btree refetch.
This comes at the cost of traversal _precision_, i.e. we can now revisit
previously visited bshrub/btree nodes. But I think this is well worth it
for more robust traversal clobbering. Traversal clobbering is delicate
and difficult to get right.
Besides, we can already revisit blocks due to CoW references, so what's
the harm in revisiting blocks when under mutation?
---
The simpler traversal logic leads to a nice amount of code savings
across the board:
code stack ctx
before: 36476 2304 660
after: 35940 (-1.5%) 2280 (-1.0%) 660 (+0.0%)
code stack ctx
gbmap before: 39524 2320 772
gbmap after: 38916 (-1.5%) 2296 (-1.0%) 772 (+0.0%)
code stack ctx
gc before: 36548 2304 804
gc after: 36012 (-1.5%) 2280 (-1.0%) 776 (-3.5%)
Note the ctx savings in LFS3_GC mode. Most of the stack/ctx savings
comes from the smaller lfs3_mtrv_t struct, which no longer needs to
stage bshrubs (we no longer care about bshrubs across mdir commit as a
part of the above clobbering simplifications):
before after
lfs3_mtrv_t: 128 100 (-21.9%)
lfs3_mgc_t: 128 100 (-21.9%)
lfs3_trv_t: 136 108 (-20.6%)
Unfortunately, the simpler clobbering means now any gc work needs the
block queue (i.e. lfs3_trv_t), solely so clobbering the block queue
doesn't clobber unallocated memory. Not great but hopefully fixable.
---
Some other notes:
- As a part of simplifying traversal clobbering, everything is triggered
by lfs3_alloc_ckpoint (via lfs3_trv_ckpoint_).
This may clobber traversals more than is strictly necessary, but
that's kinda the idea. Better safe than sorry.
And no more need to explicit lfs3_handle_clobber calls is nice.
- Opened file handle iteration is now tracked by the traversal handle's
position in the handle linked-list, instead of a separate handle
pointer. This means one less thing to disentangle and makes traversals
no longer a special case for things like lfs3_handle_close.
You may think this bumps traversals up to O(n^3) in-ram, but because
we only ever visit each unique handle + mid once, we can keep the
total O(n^2) if we're smart about linked-list updates!
- lfs3_mdir_commit needed to be tweaked to accept mids<=-1, instead of
just mid=-1 for the mroot. Unfortunately I don't know how much this
costs on its own.
- The reorganization of lfs3_mtrv_t means lfs3_mtortoise_t gets its own
struct again!
- No more tstate state machine also frees up a big chunk of the
traversal flag space, which was getting pretty cramped.
This is in preparation for some traversal simplification ideas, which
rely on all auxiliary/non-file btrees being visitable before file
btrees.
In theory the order of file vs auxiliary btrees doesn't really matter,
other than the number of different routes from mtree/mroot -> gbmap/file
btrees being a bit of a pain.
Note this is not true for the mtree, which must come first for
lfs3_mount to work.
---
Adds a bit of code when building with the gbmap:
code stack ctx
before: 36480 2304 660
after: 36476 (-0.0%) 2304 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 39464 2320 772
gbmap after: 39524 (+0.2%) 2320 (+0.0%) 772 (+0.0%)
code stack ctx
gc before: 36552 2304 804
gc after: 36548 (-0.0%) 2304 (+0.0%) 804 (+0.0%)
Brings back lfs3_btrv_t, but keeps some of the btree internal changes.
I think the biggest one is dropping the internal branch pointer, now
instead of internally pointing to the root rbyd, we just unconditionally
sync the rbyd state anytime the rbyd matches the root's weight. This is
necessary to avoid out-of-sync state when traversing bshrubs under
mutation.
Also after refactoring I think the current btree traversal logic is
easier to read.
---
This is in preparation for removing the leaf cache, or at least making
it opt-in.
It adds a chunk of stack, but in theory we can reclaim this by allowing
leaf caches to be disabled:
code stack ctx
before: 37160 2352 688
after: 37088 (-0.2%) 2384 (+1.4%) 688 (+0.0%)
I think these are good ideas to bring back when littlefs3 is more
mature, but at the moment the number of different builds is creating too
much friction.
LFS3_KVONLY and LFS3_2BONLY in particular _add_ significant chunks of
code (lfs3_file_readget_, lfs3_file_flushset_, and various extra logic
sprinkled throughout the codebase), and the current state of testing
means I have no idea if any of it still works.
These are also low-risk for introducing any disk related changes.
So, ripping out for now to keep the current experimental development
tractable. May reintroduce in the future (probably after littlefs3 is
stabilized) if there is sufficient user interest. But doing so will
probably also need to come with actual testing in CI.
This walks back some of the attempt at strict object namespacing in
struct lfs3_cfg:
- cfg.file_cache_size -> cfg.fcache_size
- filecfg.cache_size -> filecfg.fcache_size
- filecfg.cache_buffer -> filecfg.fcache_buffer
- cfg.gbmap_re_thresh -> cfg.regbmap_thresh
Motivation:
- cfg.regbmap_thresh now matches cfg.gc_regbmap_thresh, instead of using
awkwardly different namespacing patterns.
- Giving fcache a more unique name is useful for discussion. Having
pcache, rcache, and then file_cache was a bit awkward.
Hopefully it's also more clear that cfg.fcache_size and
filecfg.fcache_size are related.
- Config in struct lfs3_cfg is named a bit more consistently, well, if
you ignore gc_*_* options.
- Less typing.
Though this gets into pretty subjective naming territory. May revert
this if the new terms are uncomfortable after use.
So:
- cfg.gc_repoplookahead_thresh -> cfg.gc_relookahead_thresh
- cfg.gc_repopgbmap_thresh -> cfg.gc_regbmap_thresh
- cfg.gbmap_repop_thresh -> cfg.gbmap_re_thresh
- LFS3_*_REPOPLOOKAHEAD -> LFS3_*_RELOOKAHEAD
- LFS3_*_REPOPGBMAP -> LFS3_*_REGBMAP
Mainly trying to reduce the mouthful that is REPOPLOOKAHEAD and
REPOPGBMAP.
As a plus this also avoids potential confusion of "repop" as a push/pop
related operation.
This drops LFS3_t_MUTATED in favor of just using LFS3_t_CKPOINTED
everywhere:
1. These meant roughly the same thing, with LFS3_t_MUTATED being a bit
tighter at the cost of needing to be explicitly set.
2. The implicit setting of LFS3_t_CKPOINTED by lfs3_alloc_ckpoint -- a
function that already needs to be called before mutation -- means we
have one less thing to worry about.
Implicit properties like LFS3_t_CKPOINTED are great for building a
reliable system. Manual flags like LFS3_t_MUTATED, not so much.
3. Why use two flags when we can get away with one?
The only downside is we may unnecessarily clobber gc/traversal work when
we don't actually mutate the filesystem. Failed file open calls are a
good example.
However this tradeoff seems well worth it for an overall simpler +
more reliable system.
---
Saves a bit of code:
code stack ctx
before: 37220 2352 688
after: 37160 (-0.2%) 2352 (+0.0%) 688 (+0.0%)
code stack ctx
gbmap before: 40184 2368 856
gbmap after: 40132 (-0.1%) 2368 (+0.0%) 856 (+0.0%)
Note: This affects the blocking lfs3_alloc_repopgbmap as well as
incremental gc/traversal repopulations. Now all repop attempts return
LFS3_ERR_NOSPC when we don't have space for the gbmap, motivation below.
This reverts the previous LFS3_t_NOSPC soft error, in which traversals
were allowed to continue some gc/traversal work when encountering
LFS3_ERR_NOSPC. This results in a simpler implementation and fewer error
cases to worry about.
Observation/motivation:
- The main motivation is noticing that when we're in low-space
conditions, we just start spamming gbmap repops even if they all fail.
That's really not great! We might as well just mark the flash as dead
if we're going to start spamming erases!
At least with an error the user can call rmgbmap to try to make
progress.
- If we're in a low-space condition, something else will probably return
LFS3_ERR_NOSPC anyways. Might as well report this early and simplify
our system.
- It's a simpler model, and littlefs3 is already much more complicated
than littlefs2. Maybe we should lean more towards a simpler system
at the cost of some niche optimizations.
---
This had the side-effect of causing more lfs3_alloc_ckpoints to return
errors during testing, which revealed a bug in our uz/uzd_fuzz tests:
- We weren't flushing after writes to the opened RDWR files, which could
cause delayed errors to occur during the later read checks in the
test.
Fortunately LFS3_O_FLUSH provides a quick and easy fix!
Note we _don't_ adopt this in all uz/uzd_fuzz tests, only those that
error. It's good to test both with and without LFS3_O_FLUSH to test
that read-flushing also works under stress.
Saves a bit of code:
code stack ctx
before: 37260 2352 688
after: 37220 (-0.1%) 2352 (+0.0%) 688 (+0.0%)
code stack ctx
gbmap before: 40220 2368 856
gbmap after: 40184 (-0.1%) 2368 (+0.0%) 856 (+0.0%)
This adds test_gc_nospc with more aggressive testing of gc/traversal
operations in low-space conditions. The original intention was to test
the new soft-ENOSPC traversal behavior, but instead it found a couple
unrelated bugs.
In my defense these involve some rather subtle filesystem interactions
and went unnoticed because we don't usually check data checksums:
1. lfs3_bd_flush had a rare chance where it could corrupt our
prog-aligned pcksum when (1) we bypass the pcache, allowing any
previous contents to stay there until flush/pcksum, and (2) some
other failed prog, in this case failing repopgbmaps due to the
low-space condition, leaves garbage in the pcache. When we flush
we corrupt the pcksum even though the old data belongs to an
unrelated block.
This resulted in CKDATA failing, though the failed check is a false
positive.
As a workaround, lfs3_bd_prog and lfs3_bd_prognext now discard _any_
unrelated pcache, even if bypassing the pcache. This should ensure
consistent behavior in all cases. Note we do something similar for
with the file cache in lfs3_file_write.
This means progs may not complete unless lfs3_bd_flush is called, but
I think we need to call lfs3_bd_flush in all cases anyways to ensure
power-loss safe behavior.
The end result should be a more reliable internal bd prog API.
2. On a successful traversal with LFS3_T_REPOPLOOKAHEAD and
LFS3_T_REPOPGBMAP we adopt both the new gbmap and lookahead buffer.
This is wrong! The lookahead buffer is not aware of the gbmap during
the traversal, and _can't_ be aware as the gbmap changes during
repopulation work. This is the whole reason we have the alloc
ckpoints and the in-flight window.
To fix, adopting the lookahead buffer is now conditional on _not_
adopting a new gbmap.
It makes the code a bit more messy, but this is the correct behavior.
Populating both the gbmap and lookahead buffere requires at least two
passes.
Code changes minimal:
code stack ctx
before: 37248 2352 688
after: 37260 (+0.0%) 2352 (+0.0%) 688 (+0.0%)
code stack ctx
gbmap before: 40204 2368 856
gbmap after: 40220 (+0.0%) 2368 (+0.0%) 856 (+0.0%)
To allow relaxing when LFS3_I_REPOPLOOKAHEAD and LFS3_I_REPOPGBMAP will
be set, potentially reducing gc workload after allocating only a couple
blocks.
The relevant cfg comments have quite a bit more info.
Note -1 (not the default, 0, maybe we should explicitly flip this?)
restores the previous functionality of setting these flags on the first
block allocation.
---
Also tweaked gbmap repops during gc/traversals to _not_ try to repop
unless LFS3_I_REPOPGBMAP is set. We probably should have done this from
the beginning since repopulating the gbmap writes to disk and is
potentially destructive.
Adds code, though hopefully we can claw this back with future config
rework:
code stack ctx
before: 37176 2352 684
after: 37208 (+0.1%) 2352 (+0.0%) 688 (+0.6%)
code stack ctx
gbmap before: 40024 2368 848
gbmap after: 40120 (+0.2%) 2368 (+0.0%) 856 (+0.9%)
- LFS3_T_COMPACT -> LFS3_T_COMPACTMETA
- gc_compact_thresh -> gc_compactmeta_thresh
And friends:
LFS3_M_COMPACTMETA 0x00000800 Compact metadata logs
LFS3_GC_COMPACTMETA 0x00000800 Compact metadata logs
LFS3_I_COMPACTMETA 0x00000800 Filesystem may have uncompacted metadata
LFS3_T_COMPACTMETA 0x00000800 Compact metadata logs
---
This does two things:
1. Highlights that LFS3_T_COMPACTMETA only interacts with metadata logs,
and has no effect on data blocks.
2. Better matches the verb+noun names used for other gc/traversal flags
(REPOPGBMAP, CKMETA, etc).
It is a bit more of a mouthful, but I'm not sure that's entirely a bad
thing. These are pretty low-level flags.
And friends:
LFS3_M_REPOPLOOKAHEAD 0x00000200 Repopulate lookahead buffer
LFS3_GC_REPOPLOOKAHEAD 0x00000200 Repopulate lookahead buffer
LFS3_I_REPOPLOOKAHEAD 0x00000200 Lookahead buffer is not full
LFS3_T_REPOPLOOKAHEAD 0x00000200 Repopulate lookahead buffer
To match LFS3_T_REPOPGBMAP, which is more-or-less the same operation.
Though this does turn into quite the mouthful...
There's a strong argument for naming this inline_size as that's more
likely what users expect, but shrub_size is just the more correct name
and avoids confusion around having multiple names for the same thing.
It also highlights that shrubs in littlefs3 are a bit different than
inline files in littlefs2, and that this config also affects large files
with a shrubbed root.
May rerevert this in the future, but probably only if there is
significant user confusion.
And tweaked a few related comments.
I'm still on the fence with this name, I don't think it's great, but it
at least betters describes the "repopulation" operation than
"rebuilding". The important distinction is that we don't throw away
information. Bad/erased block info (future) is still carried over into
the new gbmap snapshot, and persists unless you explicitly call
rmgbmap + mkgbmap.
So, adopting gbmap_repop_thresh for now to see if it's just a habit
thing, but may adopt a different name in the future.
As a plus, gbmap_repop_thresh is two characters shorter.
Christopher Haster