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.
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?
And:
- Tweaked the behavior of gbmap.window/known to _not_ match disk.
gbmap.known matching disk is what required a separate
lookahead.bmapped in the first place, but we never use both fields.
- _Don't_ revert gbmap on failed mdir commits!
This was broken! If we reverted we risked inheriting outdated
in-flight block information.
This could be fixed by also zeroing lookahead.bmapped, but would force
a gbmap rebuild. And why? The only interaction between mdir commit and
the gbmap is block allocation, which is intentionally allowed to go
out-of-sync to relax issues like this.
Note we still revert in lfs3_fs_grow, the new gbmap we create there is
incompatible with the previous disk size.
As a part of these changes, gbmap.window now behaves roughly the same as
gbmap.known and updates eagerly on block allocation.
This makes lookahead.window and gbmap.window somewhat redundant, but
simplifies the relevant logic (especially due to how lookahead.window
lags behind lookahead.off).
---
A bunch of bugs fell out-of-this, the interactions with lfs3_fs_mkgbmap
and lfs3_fs_grow being especially tricky, but fortunately our testing is
doing a good job.
At least the code changes were minimal, saves a bit of RAM:
code stack ctx
no-gbmap before: 37168 2352 684
no-gbmap after: 37168 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
maybe-gbmap before: 39688 2392 852
maybe-gbmap after: 39720 (+0.1%) 2376 (-0.7%) 848 (-0.5%)
code stack ctx
yes-gbmap before: 39156 2392 852
yes-gbmap after: 39208 (+0.1%) 2376 (-0.7%) 848 (-0.5%)
These two functions allow changing whether or not the gbmap is in use
after format:
// Enable the global on-disk block-map
//
// Returns a negative error code on failure. Does nothing if a gbmap
// already exists.
int lfs3_fs_mkgbmap(lfs3_t *lfs3);
// Disable the global on-disk block-map
//
// Returns a negative error code on failure. Does nothing if no gbmap
// is found.
int lfs3_fs_rmgbmap(lfs3_t *lfs3);
rmgbmap was easy enough, but implementing mkgbmap turned out to be
surprisingly tricky due to how gstate permeates the system:
- Even if we zero gstate when we removing the gbmap, mounting the
image on a driver that doesn't understand the gbmap results in garbage
gstate over time as mdir compacts drop unknown gdeltas.
I think this sort of implicit gdelta cleanup is a good thing, but the
possibility of garbage gstate is a bit annoying.
Example A: the dbg scripts are currently printing a bunch of warnings
for corrupt gstate that can be safely ignored.
To support recovering from garbage gstate in mkgbmap, I changed
lfs3_fs_commitgdelta to _always_ track p state even when disabled. We
already needed to do this in lfs3_fs_flush/consumegdelta anyways,
since we don't know if the gbmap is used until parsing wcompat flags.
- The commit that enables the gbmap is tricky. We need the gbmap enabled
to calculate the new gdelta, but we also need it disabled so we don't
traverse the existing gbmap_p (which may be garbage).
As a workaround I added gbmap.b_p, which is in theory redundant with
gbmap_p, but (1) avoids needing to decode gbmap_p during traversals,
and (2) allows the two to temporarily fall out-of-sync in mkgbmap.
This means we potentially have 5 (!) snaphots flying around when
rebuilding the gbmap, which is starting to get a bit silly. But this
was also motivated by gbmap_p decoding adding roughly the same amount
of RAM to lfs3_mtree_traverse_, so the total RAM usage should in
theory be roughly the same.
There might be a better solution, but this at least gets mkgbmap
working. The gbmap builds are not our most RAM senstive configurations
anyways.
---
Also added a couple more tests in test_gbmap to test these:
- test_gbmap_files
- test_gbmap_rmgbmap
- test_gbmap_mkgbmap
- test_gbmap_rmmkgbmap
- test_gbmap_mkrmgbmap
And an explicit wraparound test to test_alloc. This was loosely implied
by the nospc tests, but it's probably better to have an explicit test.
The only downside is this implementation is limited to files:
- test_alloc_wraparound_files
---
Note we are currently dealing with three different configurations:
no-gbmap (the default), yes-gbmap (LFS3_YES_GBMAP), and maybe-gbmap
(LFS3_GBMAP + LFS3_F_GBMAP at runtime).
It only makes sense to include these in maybe-gbmap mode, so this is the
only mode with a notable code increase. However these functions are
relatively cheap. The stack/ctx changes also affect yes-gbmap, but
should mostly cancel out, see above:
code stack ctx
no-gbmap before: 37168 2352 684
no-gbmap after: 37168 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
maybe-gbmap before: 39292 2456 800
maybe-gbmap after: 39688 (+1.0%) 2392 (-2.6%) 852 (+6.5%)
code stack ctx
yes-gbmap before: 39116 2456 800
yes-gbmap after: 39156 (+0.1%) 2392 (-2.6%) 852 (+6.5%)
In lfs3_fs_grow, we need to update any gbmaps to match the new disk
size. The actual patch to the gbmap is easy, but it does get a bit
delicate since we need to feed the gbmap with an allocator in the new
disk size.
Fortunately, the opportunistism of the gbmap allocator avoids any
catch-22 issues, as long as we make sure to not trigger any gbmap
rebuilds.
Adds a bit of code, but not much:
code stack ctx
before: 37168 2352 684
after: 37168 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
gbmap before: 39000 2456 800
gbmap after: 39116 (+0.3%) 2456 (+0.0%) 800 (+0.0%)
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.
The main change is error propagation in lfs3_alloc_ckpoint. Since
lfs3_alloc_ckpoint writes to disk during bmap rebuilds, it can now fail
in all sorts of ways. Fortunately lfs3_alloc_ckpoint should only ever be
called by write operations, where these errors are be expected.
With bmap rebuild errors now reported correctly, this unblocks most of
the remaining test TODOs:
- Passing test_badblocks
- Passing test_ck
- Passing test_trvs
With this, LFS3_YES_BMAP is now passing all but two tests, which are
still ifndef-disabled as a temporary measure:
- test_btree - We make some low-level assumptions about the lookahead
allocator when testing btrees. It's probably not worth trying to get
this passing with the bmap allocator.
- test_grow - This one does need fixing! We currently don't update
on-disk bmaps correctly when growing the filesystem.
Code changes minimal:
code stack ctx
before: 36912 2368 684
after: 36912 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38456 2400 812
bmap after: 38512 (+0.1%) 2400 (+0.0%) 812 (+0.0%)
There's really no reason to immediately commit the bmap to disk, at
least no until the first mdir commit, when we need to at least discard
the previous bmap state.
We already do all the gstate handling in lfs3_mdir_commit anyways, and
piggybacking on mdir commit lets us get rid of the annoying extra mdir
param in lfs3_alloc_ckpoint.
This does mean a slightly higher risk of needing to re-rebuild the bmap
after a powerloss, but in theory only if the user does something weird
like writing to a file and never calling sync. Most on-disk operations
terminate in an mdir commit as that's how any state change becomes
atomically visibile in littlefs.
Saves a nice bit of stack:
code stack ctx
before: 36920 2368 684
after: 36920 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38552 2472 812
bmap after: 38464 (-0.2%) 2400 (-2.9%) 812 (+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%)
The neat thing about the on-disk bmap is that it's a range tree. We can
leverage order-statistic properties to compactly represent ranges of
similar blocks.
However, this does make updating the bmap slightly more complicated...
Note this includes both the lfs3_config -> lfs3_cfg structs as well as
the LFS3_CONFIG -> LFS3_CFG include define:
- LFS3_CONFIG -> LFS3_CFG
- struct lfs3_config -> struct lfs3_cfg
- struct lfs3_file_config -> struct lfs3_file_cfg
- struct lfs3_*bd_config -> struct lfs3_*bd_cfg
- cfg -> cfg
We were already using cfg as the variable name everywhere. The fact that
these names were different was an inconsistency that should be fixed
since we're committing to an API break.
LFS3_CFG is already out-of-date from upstream, and there's plans for a
config rework, but I figured I'd go ahead and change it as well to lower
the chances it gets overlooked.
---
Note this does _not_ affect LFS3_TAG_CONFIG. Having the on-disk vs
driver-level config take slightly different names is not a bad thing.
- 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_mtree_traverse_
- lfs3_mtree_traverse
- lfs3_mtree_gc
I like this one if only for the reduced API noise. All of these layers
need to inspect the tag to know what to do, moving the tag to the return
position means less mucking around with points in our core traversal
logic.
Shaves off a bit more code:
code stack ctx
before: 36348 2336 656
after: 36260 (-0.2%) 2336 (+0.0%) 656 (+0.0%)
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%)
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.
While they are a bit more annoying to call, init functions give the
compiler a chance to deduplicate common struct initialization logic. So
we should probably prefer init functions for any structs larger than a
couple words.
The cost of each init is small, but it really adds up!
code stack ctx
before: 38036 2608 752
after: 37844 (-0.5%) 2608 (+0.0%) 752 (+0.0%)
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%)
I thought it was a bit funny we test various disk sizes in test_grow,
but no where else! test_grow actually found several bugs when reworking
the lookahead buffer related to small disks, so I figured we should have
some more intentional tests... And behold! A bug!
The issue is that we implicitly call lfs_alloc_ckpoint in
lfsr_mdir_commit. Originally the thinking was that this would be fine
since any in-flight blocks should be committed to a tracked btree/bshrub
first, but lfsr_bshrub_commit goes _through_ lfsr_mdir_commit. Bit of a
problem.
So if we call lfsr_bshrub_commit to add a recently allocated block, it
may end up calling lfsr_mdir_commit, erronously ckpointing the
allocator, and then clobbering the new block if the mdir needs to be
relocated, split, etc.
---
The fix here is to just move lfs_alloc_ckpoint out of lfsr_mdir_commit.
This adds a bit of noise, but it's probably a good thing for alloc
ckpoints to be explicit.
At least lfs_alloc_ckpoint is cheap:
code stack
before: 36412 2680
after: 36472 (+0.2%) 2680 (+0.0%)
This has been a long-time coming, mount flags are just too useful for
configuring a filesystem at runtime.
Currently this is limited to LFS_M_RDONLY and LFS_M_CKPROGS, but there
are a few more planned in the future:
LFS_M_RDWR = 0x0000, // Mount the filesystem as read and write
LFS_M_RDONLY = 0x0001, // Mount the filesystem as readonly
LFS_M_STRICT* = 0x0002, // Error if on-disk config does not match
LFS_M_FORCE* = 0x0004, // Ignore compat flags, mount readonly
LFS_M_FORCEWITHRECKLESSABANDON*
= 0x0008, // Ignore compat flags, mount read write
LFS_M_CKPROGS = 0x0010, // Check progs by reading back progged data
LFS_M_CKREADS* = 0x0020, // Check reads via checksums
* Hypothetical
As a convenience, we also return mount flags in the struct lfs_fsinfo's
flags field as their relevant LFS_I_* variants. Though only to match
statvfs, and only because it's cheap, littlefs's API is low-level and we
should expect users to know what flags they passed to lfsr_mount.
As for the new mount flags:
- LFS_M_RDONLY - For consistency with existing APIs, this just asserts
on write operations, which makes it a bit useless... But the info flag
LFS_I_RDONLY may be useful for falling back to a readonly mode if
we encounter on-disk compat issues.
At least if implement the theoretical LFS_UNTRUSTED_USER mode
LFS_M_RDONLY could become a runtime error.
- LFS_M_RDWR - This really just exists to compliment LFS_M_RDONLY and to
match LFS_O_RDONLY/LFS_O_RDWR. It's just an alias for 0, and I don't
think there will ever be a reason to make it non-0 (but I can always
be wrong!).
- LFS_M_CKPROGS - This replaces the check_progs config option and avoids
using a full byte to store a bool.
We should probably also have a compile-time option to compile this out
(LFS_NO_CKPROGS?), but that's a future thing to do.
This ended up adding a surprising bit of code, considering we're just
moving flags around, and noise in lfs_alloc added a bit of stack again:
code stack
before: 35880 2672
after: 35932 (+0.1%) 2680 (+0.3%)
It's probably a bad reason, but this avoids wasting too much time
figuring out how to name things.
Now most traversal functions return an lfsr_tag_t + lfsr_bptr_t pair,
which is enough to describe the current relevant traversal objects:
tag=LFSR_TAG_MDIR => (lfsr_mdir_t*)bptr.data.u.buffer
tag=LFSR_TAG_BRANCH => (lfsr_rbyd_t*)bptr.data.u.buffer
tag=LFSR_TAG_DATA => bptr.data
tag=LFSR_TAG_BPTR => bptr
This would be a bit better if lfsr_data_t's buffer field was a void*,
but that would mess with byte-level arithmetic, which is more common
with lfsr_data_ts.
This also adopts the fragmented/optional out-params used elsewhere in
the codebase. I thought this would add quite a bit more stack cost,
since we need redundant tags/bptrs to make lfsr_mtree_traverse/
lfsr_mtree_gc work, but surprisingly not:
code stack
before: 35256 2680
after: 35228 (-0.1%) 2680 (+0.0%)
It seems we make up the extra stack cost of redundant tags/bptrs by
giving the compiler more stack-alloc flexibility, tighter per-function
return types, and opting-out of tags/bptrs in most low-level traversals:
lfs_alloc mainly.
But if the fragmented/optional out-params is net harmful for code/stack
size, we should reconsider the pattern system-wide. This does probably
deserve a second look in the future...
This solves the issue of multiple mdirs/rbyds in lfsr_mtree_gc, where
it's easy for traversal state to fall out of sync when mutating parts of
the filesystem.
Is it good design, with self-referential pointers making everything more
entangled? Not sure!
This saves a bit of stack, but adds a bit of code, which makes sense,
pointer chasing can be costly. But both of these changes are well below
the compiler noise floor:
code stack
before: 35228 2688
after: 35256 (+0.1%) 2680 (-0.3%)
So now files and traversals contain several nested structs:
file <-- lfsr_file_t
file.o <-- lfsr_obshrub_t
file.o.o <-- lfsr_omdir_t
This gets a bit ugly, but it's really the only way to make the compiler
happy when also with C's annoying strict aliasing rules.
This also makes lfsr_traversal_t a simple alias of lfsr_mtraversal_t,
with lfsr_mtraversal_t now including all of the obshrub/omdir state.
This simplifies things internally, and allows lfsr_mtree_gc to assert on
opened-list enrollment, but risks increased stack cost for all of the
unused fields.
Fortunately this stack cost turned out to not be that significant:
code stack
before: 35264 2680 (+0.0%)
after: 35256 (-0.0%) 2688 (+0.3%)
Separated out omdir/mdir and mtraversal. You still need to allocate an
mdir for mtraversal to work, but this avoids the extra cost of omdir's
linked-list.
To avoid _too_ many pointers, I duplicated the flags field into both
lfsr_traversal_t and lfsr_mtraversal_t. This is basically free since we
end up with a bunch of padding for mtraversal's state field, but comes
with the risk of getting confused when the two flag fields don't match
in the future.
I also merged the intermediary btype field into flags to avoid yet
another single-byte field, where it fits comfortably in 3-bits.
Note that the mdir can be uninitialized in cases where we don't need to
worry about traversal clobbering.
---
This has the same problems as separating out mdirs/bshrubs in bshrub
functions: more stack/code to move the multiple pointers around, but is
necessary to avoid strict aliasing issues. There's no way to represent
overlapping omdir/mdir/mtraversal struct in standard C99 otherwise.
The end result saves a bit of code, but adds a bit of stack:
code stack
before: 34576 2632
after: 34524 (-0.2%) 2640 (+0.3%)
Though these numbers may be close enough to the compiler noise floor to
not really care about...
This makes mtree implicit in most of littlefs's core functions, which
simplifies things. It also makes lfsr_mtree_traverse naming consistent
with other mtree-esque operation.
Renames:
- Renamed lfsr_fs_weight -> lfsr_mtree_weight (implicit mtree)
- Renamed lfsr_mtree_weight -> lfsr_mtree_weight_ (explicit mtree)
- Renamed lfsr_fs_traverse* -> lfsr_mtree_traverse*
- Renamed LFSR_TSTATE_* -> LFSR_MTRAVERSAL_*
Implicit mtree functions, note these are pretty much the backbone of
littlefs:
- lfsr_mtree_weight
- lfsr_mtree_lookup
- lfsr_mtree_seek
- lfsr_mtree_namelookup
- lfsr_mtree_pathlookup
- lfsr_mtree_traverse
This makes the naming is a bit inconsistent with lfsr_btree_*,
lfsr_rbyd_*, etc, but sometimes rules needs to bend a bit.
Besides, most of these functions needed access to the mroot anyways, so
it's not like they were really ever able to operate on independent
mtrees correctly.
And you can't complain about the code savings:
code stack
before: 34562 2624
after: 34426 (-0.4%) 2624 (+0.0%)
This sort of turned into a complete refactor of lfs_alloc in order to
move/reuse the lookahead buffer filling logic into lfsr_fs_traverse.
lfs_alloc now calls lfsr_fs_traverse to fill the lookahead buffer when
no more blocks are available, but also you can too with lfsr_traversal_t
+ LFS_T_LOOKAHEAD.
The one big caveat being if any mutation happens to the filesystem, any
incomplete lookahead needs to be tossed out. To help with this,
lfsr_traversal_read now returns LFS_ERR_BUSY (-16) instead of
LFS_ERR_NOENT (-2) if the filesystem has been modified since the
traversal was opened.
Note that by default lfsr_traversal_t will still try to keep traversing
blocks, but can be told to terminate immediately with LFS_T_EXCL.
Continuing the traversal is probably desired for checking checksums,
debugging, etc, as otherwise you could end up looping over only the
first couple blocks in a write-heavy system, but if you are trying to
populate the lookahead buffer you probably want to just abort and start
over.
I considered adding a flags field to lfs_tinfo for this, but decided
against it since it would be the only place in the current API where we
don't use error codes to convey behavior-changing information. Though
this may be worth reconsidering at some point...
---
In reworking lfs_alloc, a lot of the internal logic was broken up into
specific functions:
- lfs_alloc_ckpoint - checkpoint the allocator
- lfs_alloc_discard - discard any lookahead
- lfs_alloc_shift - discard/shift lookahead if progress can be made
- lfs_alloc_markinuse - mark a block as in-use
- lfs_alloc_markfree - mark any remaining blocks as free
- lfs_alloc_findnext - find the next free block in lookahead
If anything this probably makes lfs_alloc more readable, though the
original motivation was to allow lfsr_traversal_t to only shift/zero the
lookahead buffer if there's a chance we can make progress.
This was based on upstream work by opilat and myself.
Code changes:
code stack
before: 34226 2560
after: 34474 (+0.7%) 2552 (-0.3%)
This adds the lfsr_traversal_t object, which encapsulates a traversal
over all blocks in the filesystem.
This replaces the earlier lfs_fs_traverse function, but is sort of
"inside-out" in that instead of taking a callback, an lfsr_traversal_t
object can be read from to return lfs_tinfo structs that describe the
blocks in our system:
lfsr_traversal_open(&lfs, &t) => 0;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_MDIR;
tinfo.block => 0x0;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_MDIR;
tinfo.block => 0x1;
lfsr_traversal_read(&lfs, &t, &tinfo) => 0;
tinfo.btype => LFS_BTYPE_DATA;
tinfo.block => 0x42;
lfsr_traversal_read(&lfs, &t, &tinfo) => LFS_ERR_NOENT;
lfsr_traversal_close(&lfs, &t) => 0;
This is more flexible, allowing for aborted traversals, yielding,
rewinding, etc, but also more complicated to implement, since it
requires all traversal state to be stored explicitly.
Fortunately, since we needed to reimplement filesystem traversals
anyways, I was able to build this into the new system from the start
using a small state machine to drive the traversal internally. So all
that was really needed was a bit of window dressing, adding
LFS_TYPE_TRAVERSAL to track open traversals, logic to handle
invalidating traversals on file close, mutation, etc...
Which, uh, that last one is not implemented yet. Interactions with other
filesystem operations gets messy, so I figured I'd go ahead and commit
what is currently working.
Ugh, and tests. The biggest downside of adding lfsr_traversal_t is how
many more corner-cases it adds to the system...
lfsr_traversal_t is going to be a work-in-progress for a bit...
---
lfsr_traversal_t also adds a really interesting path towards more access
to advanced low-level operations, such as checking metadata/data
checksums, incrementally progressing the garbage collector, even
repairing bad metadata/data blocks eventually.
Currently implemented is LFS_T_CKMETADATA and LFS_T_CKDATA to check
metadata and data checksums respectively. This is the first feature that
actually allows you to validate data checksums.
Code changes so far:
code stack
before: 33886 2560
after: 34226 (+1.0%) 2560 (+0.0%)
We've been wasting a lot of test cycles thanks to REMOUNT. Using a test
define for this effectively duplicates the test, when we really just
want to run more post-test code without additional mutation.
The main reason for REMOUNT has been to save typing, which, well, is not
a bad reason, these tests involve a lot of typing...
But this is probably a hammer/nail situation. If we replace these with a
small post-test loop, we can save quite a bit of time:
make test -j before: 5791.9s
make test -j after: 5123.8s (-11.5%)
Some tests still use a REMOUNT define, but these should be limited to
cases where remount actually changes the test's behavior.
A much requested feature, this allows much finer control of how RAM is
allocated for the system.
It was difficult to introduce this in previous versions of littlefs due
to how we steal caches during certain file operations, but now we don't
do that and treat the caches much more transparently.
Managing separate cache sizes does add a bit of code, but this is well
worth the potential for RAM savings due to increased flexibility:
code stack
before: 33656 2632
after: 33714 (+0.2%) 2640 (+0.3%)
Also interesting to note this reduces alignment requirements for the
rcache/pcache, since they don't need to share alignment, and completely
removes any alignment requirement from the file buffers.
This doesn't really help us all that much right now, but will be useful
for the future-planned block map and being able to cache pre-erased
blocks.
Though the lack of erasing when allocating new mdirs raises some
questions... Oh well, future problems.
Code changes:
code stack
before: 33856 2880
after: 33864 (+0.0%) 2880 (+0.0%)
So:
x = (cond) ? yes : no;
Where there are always parentheses around the condition, even if not
required for disambiguity. Additional parentheses are always allowed,
but the parenthesized condition helps signal that a ternary operator is
coming earlier in the expression.
This style has grown on me as I think it helps code readability. It
reminds me of the required parentheses for if/while statements.
Might as well adopt codebase-wide.
The size field in lfs_info doesn't really make sense for stat/dir_read
when the file is a directory. Still, we should probably set it to 0 os
it's not uninitialized.
Fortunately we were already setting size=0 in _most_ cases, this commit
is mostly just checking for size=0 in more test cases.
Looking forward, bptr checksums provide an easy mechanism to validate
data residing in blocks. This extends the merkle-tree-like nature of the
filesystem all the way down to the data level, and is common in other
COW filesystems.
Two interesting things to note:
1. We don't actually check data-level checksums yet, but we do calculate
data-level checksums unconditionally.
Writing checksums is easy, but validating checksums is a bit more
tricky. This is made a bit harder for littlefs, since we can't hold
an entire block of data in RAM, so we have to choose between separate
bus transactions for checksum + data reads, or extremely expensive
overreads every read.
Note this already exists at the metadata-level, the separate bus
transactions for rbyd fetch + rbyd lookup means we _are_ susceptible
to a very small window where bit errors can get through.
But anyways, writing checksums is easy. And has basically no cost
since we are already processing the data for our write. So we might
as well write the data-level checksums at all times, even if we
aren't validating at the data-level.
2. To make bptr checksums work cheaply we need an additional cksize
field to indicate how much data is checksummed.
This field seems redundant when we already have the bptr's data size,
but if we didn't have this field, we would be forced to recalculate
the checksum every time a block is sliced. This would be
unreasonable.
The immutable cksize field does mean we may be checksumming more data
than we need to when validating, but we should be avoiding small
block slices anyways for storage cost reasons.
This does add some stack cost because our bptr struct is larger now:
code stack
before: 31200 2768
after: 31272 (+0.2%) 2800 (+1.1%)
The idea here is to revert moving redund blocks into lfsr_rbyd_t, and
instead just keep a redundant copy of the rbyd blocks in the redund
blocks in lfsr_mdir_t.
Surprisingly, extra overhead in lfsr_mdir_t ended up with worse stack
usage than extra overhead in lfsr_rbyd_t. I guess we end up allocated
more mdirs than rbyds, which makes a bit of sense given how complicated
lfsr_mdir_commit is:
code stack structs
redund union: 30976 2496 1072
redund in rbyd: 30948 (-0.1%) 2528 (+1.3%) 1100 (+2.6%)
redund in mdir: 31000 (+0.1%) 2536 (+1.6%) 1092 (+1.8%)
The mdir option does seem to improve struct overhead, but this hasn't
been a reliable measurement since it doesn't take into account how many
of each struct is allocated.
Given that the mdir option is inferior in both code and stack cost, and
requires more care to keep the rbyd/redund blocks in sync, I think I'm
going to revert this for now but keep the commit in the commit history
since it's an interesting comparison.
This simplifies dependent structs with redundancy, mainly lfsr_mdir_t,
at a significant RAM cost:
code stack structs
before: 30976 2496 1072
after: 30948 (-0.1%) 2528 (+1.3%) 1100 (+2.6%)
Which, to be honest, is not as bad as I thought it would be. Though it
is still pretty bad for no new features.
The motivation for this change:
1. The organization of the previous lfsr_mdir_t struct was a bit hacky
and relied on exact padding so the redund block array and rbyd block
lined up at the right offset.
2. The previous organization prevented theoretical "read-only rbyd
structs" that could omit write-related fields, e.g. eoff and cksum.
This idea is currently unused.
3. The current mdir=level-1, btree/data=level-0 redund design makes this
RAM tradeoff pretty bad, but in theory higher btree redund levels
would need the extra redund blocks in the rbyd struct anyways.
Still, the RAM impact to the current default configuration means this
should probably be reverted...
- Renamed mdir->u.m to mdir->u.mdir.
- Prefer mdir->u.rbyd.* where possible.
- Changed file/dir mdirs to be stored directly, requiring a cast to
lfsr_openedmdir_t to enroll in the opened mdir list.
The original name was a bit of a mouthful.
Also dropped the default crystal_size in the test/bench runners
block_size/4 -> block_size/8. I'm already noticing large amounts of
inflation when blocks are fragmented, though I am experimenting with a
rather small fragment_size right now.
Future benchmarks/experimentation is required to figure out good values
for these.
Ended up changing the name of lfsr_mtree_traversal_t -> lfsr_traversal_t,
since this behaves more like a filesytem-wide traversal than an mtree
traversal (it returns several typed objects, not mdirs like the other
mtree functions for one).
As a part of this changeset, lfsr_btraversal_t (was lfsr_btree_traversal_t)
and lfsr_traversal_t no longer return untyped lfsr_data_ts, but instead
return specialized lfsr_{b,t}info_t structs. We weren't even using
lfsr_data_t for its original purpose in lfsr_traversal_t.
Also changed lfsr_traversal_next -> lfsr_traversal_read, you may notice
at this point the changes are intended to make lfsr_traversal_t look
more like lfsr_dir_t for consistency.
---
Internally lfsr_traversal_t now uses a full state machine with its own
enum due to the complexity of traversing the filesystem incrementally.
Because creating diagrams is fun, here's the current full state machine,
though note it will need to be extended for any
parity-trees/free-trees/etc:
mrootanchor
|
v
mrootchain
.-' |
| v
| mtree ---> openedblock
'-. | ^ | ^
v v | v |
mdirblock openedbtree
| ^
v |
mdirbtree
I'm not sure I'm happy with the current implementation, and eventually
it will need to be able to handle in-place repairs to the blocks it
sees, so this whole thing may need a rewrite.
But in the meantime, this passes the new clobber tests in test_alloc, so
it should be enough to prove the file implementation works. (which is
definitely is not fully tested yet, and some bugs had to be fixed for
the new tests in test_alloc to pass).
---
Speaking of test_alloc.
The inherent cyclic dependency between files/dirs/alloc makes it a bit
hard to know what order to test these bits of functionality in.
Originally I was testing alloc first, because it seems you need to be
confident in your block allocator before you can start testing
higher-level data structures.
But I've gone ahead and reversed this order, testing alloc after
files/dirs. This is because of an interesting observation that if alloc
is broken, you can always increase the test device's size to some absurd
number (-DDISK_SIZE=16777216, for example) to kick the can down the
road.
Testing in this order allows alloc to use more high-level APIs and
focus on corner cases where the allocator's behavior requires subtlety
to be correct (e.g. ENOSPC).
- mbits -> mleaf_bits
- mlimit -> mleaf_limit
- mweight -> mleaf_weight
- lfsr_mridmask -> lfsr_midrmask
- lfsr_mbidmask -> lfsr_midbmask
This is a bit tricky to name, since we want to clarify it's not the
mtree limit and not the mdir's actual rbyd weight. But this also risks
confusing around the difference between mdirs/mleaves (mdirs are
mtree's leaves).
- Added lfsr_mdir_lookupnext, for iteration through only a single mid.
This is useful for MOVE attributes.
- Renamed LFSR_MDIR_MROOTANCHOR -> LFSR_MROOTANCHOR.
- Renamed functions that operate on mdir blocks lfsr_mdir_* ->
lfsr_mblocks_*.
- Reordered arguments in lfsr_mdir_fetch.
- Renamed mrid_bits/mbid_weight -> mbits/mweight.
Christopher Haster