- 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.
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%)
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.
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%)
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%)
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
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%)
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 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.
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...
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.
A big downside of LFS3_T_REBUILDGBMAP is the addition of an lfs3_btree_t
struct to _every_ traversal object.
Unfortunately, I don't see a way around this. We need to track the new
gbmap snapshot _somewhere_, and other options (such as a global gbmap.b_
snapshot) just move the RAM around without actually saving anything.
To at least mitigate this internally, this splits lfs3_trv_t into
distinct lfs3_trv_t, lfs3_mgc_t, and lfs3_mtrv_t structs that capture
only the relevant state for internal traversal layers:
- lfs3_mtree_traverse <- lfs3_mtrv_t
- lfs3_mtree_gc <- lfs3_mgc_t (contains lfs3_mtrv_t)
- lfs3_trv_read <- lfs3_trv_t (contains lfs3_mgc_t)
This minimizes the impact of the gbmap rebuild snapshots, and saves a
big chunk of RAM. As a plus it also saves RAM in the default build by
limiting the 2-block block queue to the high-level lfs3_trv_read API:
code stack ctx
before: 37176 2360 684
after: 37176 (+0.0%) 2352 (-0.3%) 684 (+0.0%)
code stack ctx
gbmap before: 40060 2432 848
gbmap after: 40024 (-0.1%) 2368 (-2.6%) 848 (+0.0%)
The main downside? Our field names are continuing in their
ridiculousness:
lfs3.gc.gc.t.b.h.flags // where else would the global gc flags be?
This adds LFS3_T_REBUILDGBMAP and friends, and enables incremental gbmap
rebuilds as a part of gc/traversal work:
LFS3_M_REBUILDGBMAP 0x00000400 Rebuild the gbmap
LFS3_GC_REBUILDGBMAP 0x00000400 Rebuild the gbmap
LFS3_I_REBUILDGBMAP 0x00000400 The gbmap is not full
LFS3_T_REBUILDGBMAP 0x00000400 Rebuild the gbmap
On paper, this is more or less identical to repopulating the lookahead
buffer -- traverse the filesystem, mark blocks as in-use, adopt the new
gbmap/lookahead buffer on success -- but a couple nuances make
rebuilding the gbmap a bit trickier:
- Unlike the lookahead buffer, which eagerly zeros in allocation, we
need an explicit zeroing pass before we start marking blocks as
in-use. This means multiple traversals can potentially conflict with
each other, risking the adoption of a clobbered gbmap.
- The gbmap, which stores information on disk, relies on block
allocation and the temporary "in-flight window" defined by allocator
ckpoints to avoid circular block states during gbmap rebuilds. This
makes gbmap rebuilds sensitive to allocator ckpoints, which we
consider more-or-less a noop in other parts of the system.
Though now that I'm writing this, it might have been possible to
instead include gbmap rebuild snapshots in fs traversals... but that
would probably have been much more complicated.
- Rebuilding the gbmap requires writing to disk and is generally much
more expensive/destructive. We want to avoid trying to rebuild the
gbmap when it's not possible to actually make progress.
On top of this, the current trv-clobber system is a delicate,
error-prone mess.
---
To simplify everything related to gbmap rebuilds, I added a new
internal traversal flag: LFS3_t_CKPOINTED:
LFS3_t_CKPOINTED 0x04000000 Filesystem ckpointed during traversal
LFS3_t_CKPOINTED is set, unconditionally, on all open traversals in
lfs3_alloc_ckpoint, and provides a simple, robust mechanism for checking
if _any_ allocator checkpoints have occured since a traversal was
started. Since lfs3_alloc_ckpoint is required before any block
allocation, this provides a strong guarantee that nothing funny happened
to any allocator state during a traversal.
This makes lfs3_alloc_ckpoint a bit less cheap, but the strong
guarantees that allocator state is unmodified during traversal are well
worth it.
This makes both lookahead and gbmap passes simpler, safer, and easier to
reason about.
I'd like to adopt something similar+stronger for LFs3_t_MUTATED, and
reduce this back to two flags, but that can be a future commit.
---
Unfortunately due to the potential for recursion, this ended up reusing
less logic between lfs3_alloc_rebuildgbmap and lfs3_mtree_gc than I had
hoped, but at like the main chunks (lfs3_alloc_remap,
lfs3_gbmap_setbptr, lfs3_alloc_adoptgbmap) could be split out into
common functions.
The result is a decent chunk of code and stack, but the value is high as
incremental gbmap rebuilds are the only option to reduce the latency
spikes introduced by the gbmap allocator (it's not significantly worse
than the lookahead buffer, but both do require traversing the entire
filesystem):
code stack ctx
before: 37164 2352 684
after: 37208 (+0.1%) 2360 (+0.3%) 684 (+0.0%)
code stack ctx
gbmap before: 39708 2376 848
gbmap after: 40100 (+1.0%) 2432 (+2.4%) 848 (+0.0%)
Note the gbmap build is now measured with LFS3_GBMAP=1, instead of
LFS3_YES_GBMAP=1 (maybe-gbmap) as before. This includes the cost of
mkgbmap, lfs3_f_isgbmap, etc.
Having gbmap/bmap used in different places for the same thing was
confusing. Preferring gbmap as it is consistent with other gstate (grm
queue, gcksums), even if it is a bit noisy.
It's interesting to note what didn't change:
- The BM* range tags: LFS3_TAG_BMFREE, etc. These already differs from
the GBMAP* prefix enough, and adopting GBM* would risk confusion for
actual gstate.
- The gbmap revdbg string: "bb~r". We don't have enough characters for
anything else!
- dbgbmap.py/dbgbmapsvg.py. These aren't actually related to the gbmap,
so the name difference is a good thing.
TLDR: This drops the idea of different bmap strategies/modes, and sorts
out most of the compile-time/runtime conditional bmap interactions.
---
Motivation: Benchmarking (at least up to the 32-bit word limit) has
shown the bmap will unlikely be a significant bottleneck, even on large
disks. The largest disks tend to be NAND, and NAND's ridiculous block
size limits pressure on block allocation.
There are still concerns for areas I haven't measured yet:
- SD/eMMC/FTL - Small blocks, so more pressure on block allocation. In
theory the logical block size can be artificially increased, but this
comes with a granularity tradeoff.
- I've only measured throughput, latency is a whole other story.
However, users have reported lfs3_fs_gc is useful for mitigating this,
so maybe latency is less of a concern now?
But while there may still be room for improvement via alternative bmap
strategies, the risk a concerning amount of complexity. Yes,
configuration gets more complicated, but the real issue is any bmap
strategies that try to track _deallocations_ (the original idea being
treediffing) risk falling leaking blocks if all cases aren't covered.
The current "bmap cache" strategy strikes a really nice balance where it
reduces _amortized_ block allocation -> ~O(log n) without RAM, while
retaining the safe, bug-resistant, single-source-of-truth properties
that come with lookahead-based allocation.
---
So, long story short, dropping other strategies, and now the presence of
the bmap is a boolean flag.
This is also the first format-specific flag:
- Define LFS3_BMAP to enable the bmap logic, but note by default the
bmap will still not be used.
- Define LFS3_YES_BMAP to force the bmap to be used.
- With LFS3_BMAP, passing LFS3_F_GBMAP to lfs3_format will include the
on-disk block-map.
- No flag is needed during mount, the presence of the bmap is determined
by the on-disk wcompat flags (LFS3_WCOMPAT_GBMAP). This also prevents
rw mounting if the bmap is not supported, but rdonly mounting is
allowed.
- Users can check if the bmap is in use via lfs3_fs_stat, which reports
LFS3_I_GBMAP in the flags field.
There's still some missing pieces, but these will be a bit more
involved:
- lfs3_fs_grow needs to be made bmap aware!
- We probably want something like lfs3_fs_mkgbmap and lfs3_fs_rmgbmap to
allow converting between bmap backed/not-backed filesystem images.
Code changes minimal:
code stack ctx
before: 37172 2352 684
after: 37172 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38844 2456 800
bmap after: 38852 (+0.0%) 2456 (+0.0%) 800 (+0.0%)
At least at a proof-of-concept level, there's still a lot of cleanup
needed.
To make things work, lfs3_alloc_ckpoint now takes an mdir, which
provides the target for gbmap gstate updates.
When the bmap is close to empty (configurable via bmap_scan_thresh), we
opportunistically rebuild it during lfs3_alloc_ckpoints. The nice thing
about lfs3_alloc_ckpoint is we know the state of all in-flight blocks,
so rebuilding the bmap just requires traversing the filesystem + in-RAM
state.
We might still fall back to the lookahead buffer, but in theory a well
tuned bmap_scan_thresh can prevent this from becoming a bottleneck (at
the cost of more frequent bmap rebuilds).
---
This is also probably a good time to resume measuring code/ram costs,
though it's worth repeating the above note about the bmap work still
needing cleanup:
code stack ctx
before: 36840 2368 684
after: 36920 (+0.2%) 2368 (+0.0%) 684 (+0.0%)
Haha, no, the bmap isn't basically free, it's just an opt-in features.
With -DLFS3_YES_BMAP=1:
code stack ctx
no bmap: 36920 2368 684
yes bmap: 38552 (+4.4%) 2472 (+4.4%) 812 (+18.7%)
- test_traversal -> test_trvs
- lfs3_traversal_t -> lfs3_trv_t
- lfs3_btraversal_t -> lfs3_btrv_t
- t -> trv
- bt -> btrv
- lfs3_traversal_* -> lfs3_trv_*
- lfs3_btraversal_* -> lfs3_btrv_*
The traversal type is becoming one of the more fundamental types in
littlefs, and if DIR and REG both get shortened names, it makes sense
for TRV to have one as well.
This also removes the temptation to use t for traversals, which is
probably an even worse name.
---
Note that lfs3_btree_traverse, lfs3_mtree_traverse, etc, remain
unaffected. This may change in the future, but it's interesting to note
that verbs seem to need much less typing than nouns.
- lfs3_omdir_t -> lfs3_handle_t
- lfs3.omdirs -> lfs3.handles
- o -> h
- lfs3_omdir_* -> lfs3_handle_*
- lfs3_omdir_ismidopen -> lfs3_mid_isopen
From conversations with users, the term "handle" or "file handle" seems
to be the most common/easily understood term for the lfs3_file_t struct
itself. It makes sense to adopt this in our codebase.
I usually dislike inventing new names for things when prefixes can imply
a relationship (size -> ssize, cache -> rcache, shrub -> bshrub, etc),
but lfs3_omdirs_t was probably a bit much.
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
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%)
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 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%)
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 moves all of the shrub tracking logic from lfsr_obshrub_t into
lfsr_bshrub_t, completely drops the lfsr_obshrub_t type, and changes all
lfsr_bshrub_* functions to take lfsr_bshrub_t instead of the mdir+shrub
pair.
This makes the lfsr_bshrub_* functions <-> lfsr_bshrub_t relationship
more consistent with other APIs, such as lfsr_btree_t:
- lfsr_bshrub_lookupnext(lfs, &file->o.o.mdir, &file->o.bshrub, ...)
+ lfsr_bshrub_lookupnext(lfs, &file->b, ...)
I think the reason why this design wasn't obvious before is because, at
least conceptually, having the lfsr_mdir_t live inside the lfsr_bshrub_t
is a bit weird. It's only thanks to lfsr_file_t invasively using the
internal lfsr_mdir_t that we can avoid duplicate lfsr_mdir_t objects.
This also reorganizes the structs in lfs.h a bit, and renames the
related file.o -> file.b fields (much needed because lfs->gc.t.o.o.mdir.
rbyd.blocks was starting to get _real_ confusing).
---
Unfortunately, reducing the number of arguments to lfsr_bshrub_*
functions did not save nearly as much code as I thought it would. It
even ended up with a net _increase_ of code, apparently due to needing
to recalculate the bshrub->shrub offset more often:
code stack ctx
before: 36476 2608 640
after: 36484 (+0.0%) 2608 (+0.0%) 640 (+0.0%)
Strange, but this rework is still worthwhile if only for the code
readability.
Since we dropped lfsr_gc_setflags/setsteps, it was no longer possible to
set gc_flags to zero (perfectly valid and useful for system bringup/
testing things). Supporting gc_flags=0 means it's not possible to
provide a default, but this is probably ok as users need to opt-in to
LFS_GC anyways.
Note that at least gc_steps=0 doesn't make sense, so the default there
is reasonable.
Fixing this also highlighted that gc_flags/steps are no longer mutable,
making the comment in lfs_init out-of-date. Dropping these saves a bit
of lfs_t size, so that's nice.
And then testing also revealed that LFS_GC_CKDATA implying LFS_GC_CKDATA
means it should probably clear the LFS_I_CKMETA flag as well.
---
And here I thought this was going to be just a simple test-writing
exercise!
Code changes:
code stack ctx
default before: 37792 2608 620
default after: 37792 (-0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37896 2608 768
gc after: 37848 (-0.1%) 2608 (+0.0%) 760 (-1.0%)
- LFS_I_INCONSISTENT -> LFS_I_MKCONSISTENT
- LFS_I_CANLOOKAHEAD -> LFS_I_LOOKAHEAD
- LFS_I_UNCOMPACTED -> LFS_I_COMPACT
- LFS_I_CANCKMETA -> LFS_I_CKMETA
- LFS_I_CANCKDATA -> LFS_I_CKDATA
This just makes everything easier to read/pattern match, even if it's
a bit inaccurate english-wise. The imperative transformations were also
wildly inconsistent...
- lfsr_gc -> lfsr_fs_gc
- lfsr_gc_unck -> lfsr_fs_unck
lfsr_fs_unck is surprisingly still useful in non-gc builds, since we
still have ckmeta/ckdata state. These flags can still be queried with
lfsr_fs_stat and cleared with lfsr_fs_ckmeta/ckdata/lfsr_traversal_t, so
it seems useful to keep this function around.
It's also a relatively cheap function.
Though this does mean it deserves a rename. Dropping the gc prefix
hopefully makes it clearer this function is not entirely gc-specific.
And since we no longer have lfsr_gc_setflags/setsteps, it makes sense to
rename lfsr_gc back to lfsr_fs_gc, to be consistent with the other
filesystem-wide utilities.
Code changes, apparently lfsr_fs_unck costs 12 bytes:
code stack ctx
default before: 37792 2608 620
default after: 37804 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37938 2608 768
gc after: 37940 (+0.0%) 2608 (+0.0%) 768 (+0.0%)
LFS_GC_CKMETA and LFS_GC_CKDATA are a bit unique in that their work is
never really done.
Where LFS_GC_MKCONSISTENT/COMPACT can prove things about the system,
LFS_GC_CKMETA/CKDATA can't, because it's always possible for new
bit-errors to develop. Even _during_ an LFS_GC_CKMETA/CKDATA traversal.
But while this is technically true, it's not a very useful state of
things for our lfsr_gc API...
---
What we really want is some way to know if ckmeta/ckdata has completed
"recently" (for some definition of recently), and to let users indicate
when they need another ckmeta/ckdata scan.
To try to solve this:
1. Added LFS_I_CANCKMETA and LFS_I_CANCKDATA to indicate when lfsr_gc
has not checked metadata/data.
These are set during mount (unless mounting with
LFS_M_CKMETA/CKDATA), and cleared when either lfsr_gc completes or
lfsr_fs_ckmeta/data is called. Once cleared, littlefs will not reset
them on its own.
2. Added lfsr_gc_unck to allow users to explicitly reset LFS_I_CKMETA
and/or LFS_I_CKDATA, which will tell lfsr_gc to check metadata/data
again on the next call.
There is some subtlety around clobbering ongoing traversals, but a
mask and some tests should prevent this from being a problem.
Currently, lfsr_gc_unck also allows clearing of other gc flags, but
I'm not sure there's any real use-case for this...
Note that you can still get the previous behavior if you just call
lfsr_gc_unck after every lfsr_gc call.
This also changes info flag behavior slightly in default mode, with
LFS_I_CANCKMETA/CANCKDATA telling you if metadata/data has been checked
since mount. Which does seem useful? Maybe these flags deserve a better
name?
Code changes:
code stack ctx
default before: 37796 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
default after: 37792 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37896 2608 768
gc after: 37938 (+0.1%) 2608 (+0.0%) 768 (+0.0.%)
Now that you can provide gc_flags/gc_steps in lfs_config, I think it's a
bit more clear that _mutating_ the flags/steps is a niche feature, and
not worth implementing/testing.
It raises the question why not have a similar lfsr_setflags or
lfsr_file_setflags, and the answer there is it would be a pain-in-the-
ass to make sure all possible corner cases are covered.
It actually already was a pain-in-the-ass to test lfsr_gcsetflags/
setsteps... but just because we already did the work is not a good
reason for keeping complexity around.
---
Note that most of the use cases for lfsr_gc_setflags/setsteps can be
covered by either remounting the filesystem or through the
lfsr_traversal_t APIs directly.
The end result is a bit of code savings when incremental gc is enabled:
code stack ctx
default before: 37796 2608 620
default after: 37796 (+0.0%) 2608 (+0.0%) 620 (+0.0%)
gc before: 37944 2608 768
gc after 37896 (-0.1%) 2608 (+0.0%) 768 (+0.0%)
Incremental gc, being stateful and not gc-able (ironic), was always
going to need to be conditionally compilable.
This moves incremental gc behind the LFS_GC define, so that we can focus
on the "default" costs. This cuts lfs_t in nearly half!
lfs_t with LFS_GC: 308
lfs_t without LFS_C: 168 (-45.5%)
This does save less code than one might expect though. We still need
most of the internal traversal/gc logic for things like block allocation
and orphan cleanup, so most of the savings is limited to the RAM storing
the incremental state:
code stack ctx
before: 37916 2608 768
after with LFS_CFG: 37944 (+0.1%) 2608 (+0.0%) 768 (+0.0%)
after without LFS_CFG: 37796 (-0.3%) 2608 (+0.0%) 620 (-19.3%)
On the flip side, this does mean most of the incremental gc
functionality is still availables in the lfsr_traversal_t APIs.
Applications with more advanced gc use-cases may actually benefit from
_not_ enabling the incremental gc APIs, and instead use the
lfsr_traversal_t APIs directly.
Before:
int lfsr_fs_gc(lfs_t *lfs, lfs_soff_t steps, uint32_t flags);
After:
int lfsr_gc(lfs_t *lfs);
int lfsr_gc_setflags(lfs_t *lfs, uint32_t flags);
int lfsr_gc_setsteps(lfs_t *lfs, lfs_soff_t steps);
---
The interesting thing about the lfsr_gc API is that the caller will
often be very different from whoever configures the system. One example
being an OS calling lfsr_gc in a background loop, while leaving
configuration up to the user.
The idea here, is instead of forcing the OS to come up with its own
stateful system to pass flags to lfsr_gc, we just embed this state in
littlefs directly. The whole point of lfsr_gc is that it's a stateful
system anyways.
Unfortunately this state does require a bit more logic to maintain,
which adds code/ctx cost:
code stack ctx
before: 37812 2608 752
after: 37916 (+0.3%) 2608 (+0.0%) 768 (+2.1%)
These are our current set of general-purpose high-level tests that can
be turned to when needing to test a wide range of filesystem operations.
They were getting a bit hard to keep track of without a consistent
prefix, especially since no individual test suite can actually use all
of them at the same time.
Now, finding these tests is as simple as: ./scripts/test.py -L *_spam_*
I also renamed a couple because their names were starting to get
ridiculous. I mean just look at
test_badblocks_alternating_spam_orphanzombiedir_fuzz...
- *_spam_orphanzombie_fuzz -> *_spam_oz_fuzz
- *_spam_orphanzombiedir_fuzz -> *_spam_ozd_fuzz
- *_spam_file_pl_fuzz -> *_spam_f_pl_fuzz
- *_spam_filedir_pl_fuzz -> *_spam_fd_pl_fuzz
Here are all of the current spam tests and contexts we use them in:
traversal badblocks relocations
| gc ck grow | powerloss exhaustion
dir_many y y y y y y
dir_fuzz y y y y y y y
file_many y y y y y y
file_fuzz y y y y y y y
fwrite_fuzz y y y y y
oz_fuzz y y y y y y y
ozd_fuzz y y y y y y y
f_pl_fuzz y y y
fd_pl_fuzz y y y
This is mainly to solve the weird check-hole where passing CKPROGS/
CKREADS as mount flags has no effect on lfsr_format (I mean, it'd be a
bit silly if it did somehow):
LFS_F_RDWR 0 // Format the filesystem as read and write
LFS_F_CKPROGS 0x00000010 // Check progs by reading back progged data
LFS_F_CKREADS 0x00000020 // Check reads via parity bits/checksums
This makes lfsr_format a more cumbersome interface, but I don't know if
this is necessarily a bad thing. There's always risk of data loss when
calling lfsr_format, so maybe it should be a pain to call.
At the very least, format flags may be useful in the future for
enabling/disabling format-time things such as the planned block-map,
parity-tree, etc. Though it's unclear if such significant settings
should be format flags or somehow encoded as fields in our config
struct.
---
The LFS_F_* format flags of course ended up conflicting with our
internal LFS_F_* flags, so I renamed most of the internal flags to match
the closest flag set they participate in:
- LFS_F_TYPE -> LFS_O_TYPE
- LFS_F_UNFLUSH -> LFS_O_UNFLUSH
- LFS_F_UNSYNC -> LFS_O_UNSYNC
- LFS_F_ORPHAN -> LFS_O_ORPHAN
- LFS_F_ZOMBIE -> LFS_O_ZOMBIE
- LFS_F_ORPHANS -> LFS_I_ORPHANS
- LFS_F_UNCOMPACTED -> LFS_I_UNCOMPACTED
- LFS_F_TSTATE -> LFS_T_TSTATE
- LFS_F_BTYPE -> LFS_T_BTYPE
- LFS_F_DIRTY -> LFS_T_DIRTY
- LFS_F_MUTATED -> LFS_T_MUTATED
This may make it a bit less clear which flags are a part of the public
API, vs intended only for internal use, but at the very least our asserts
in format/mount/open/etc should catch most of these mistakes.
---
Code cost ended up being pretty minimal. Actually negative. This is the
second time we're _adding_ a feature that somehow saves code, though the
reality for this one is we're really just pushing constants up into the
user's stack frame. Still, it's a good indication the cost of format
flags is small:
code stack
before: 36452 2680
after: 36448 (-0.0%) 2680 (+0.0%)
These are basically the same as lfsr_fs_ckmeta/ckdata but limited to a
single file. They may be useful when you need to validate a file but
don't want to bother validating the entire filesystem:
// Check a file for metadata errors
int lfsr_file_ckmeta(lfs_t *lfs, lfsr_file_t *file);
// Check a file for metadata + data errors
int lfsr_file_ckdata(lfs_t *lfs, lfsr_file_t *file);
I've also added test_ck to test these and added some more
lfsr_fs_ckmeta/ckdata tests there. These currently just test simple
full-block clobbering, but we should eventually test more interesting
error patterns.
Unfortunately lfsr_file_ckmeta/ckdata can't reuse the internal
lfsr_mtree_traverse in quite the same way lfsr_fs_ckmeta/ckdata can, so
they're actually a bit more expensive. Though keep in mind with
link-time gc you won't pay the cost unless you call these functions:
code stack
before: 36024 2696
after: 36368 (+1.0%) 2664 (-1.2%)
Oh, and the multiple calls to lfsr_btree/bshrub_traverse apparently
uninlined it out of lfsr_mtree_traverse, saving the stack cost in the
stack hot-path... Yay?
Christopher Haster