This drops the requirement that all file types are introduced with a
related wcompat flag. Instead, the wcompat flag is only required if
modification _would_ leak resources, and we treat unknown file types as
though they are regular files.
This allows modification of unknown file types without the risk of
breaking anything.
To compare with before the unknown-type rework:
Before:
> Unknown file types are allowed and may leak resources if modified,
> so attempted modification (rename/remove) will error with
> LFS_ERR_NOTSUP.
Now:
> Unknown file types are allowed but must not leak resources if
> modified. If an unknown file type would leak resources, it should set
> a related wcompat flag to only allow mounting RDONLY.
Note this includes directories, which can leak bookmarks if removed, so
filesystems using directories should set the LFSR_WCOMPAT_DIR flag.
But we no longer need the LFSR_WCOMPAT_REG/LFSR_WCOMPAT_STICKYNOTE
flags.
---
The real tricky part was getting lfsr_rename to work with unknown types,
as this broke the invariant that we only ever commit tags we know about.
Fixing this required:
- Fetching the non-unknown-mapped tag in lfsr_rename
- Mapping all name tags to LFSR_TAG_NAME in lfsr_rbyd_appendrattr_
- Adopting LFSR_RATTR_NAME for bookmark name tags
This was broken by the above lfsr_rbyd_appendrattr_ change, but it's
probably good to handle these the same as other name tags anyways.
This adds a bit of code, but not enough that I think this isn't worth
it (or worth a build-time option):
code stack ctx
before: 35924 2440 640
after: 35992 (+0.0%) 2440 (+0.0%) 640 (+0.0%)
This changes how we approach unknown file types.
Before:
> Unknown file types are allowed and may leak resources if modified,
> so attempted modification (rename/remove) will error with
> LFS_ERR_NOTSUP.
Now:
> Unknown file types are only allowed in RDONLY mode. This avoids the
> whole leaking resources headache.
Additionally, unknown types are now mapped to LFS_TYPE_UNKNOWN, instead
of just being forwarded to the user. This allows us to add internal
types/tags to the LFSR_TAG_NAME type space without worrying about
conflicts with future types:
- reg -> LFS_TYPE_REG
- dir -> LFS_TYPE_DIR
- stickynote -> LFS_TYPE_STICKYNOTE
- everything else -> LFS_TYPE_UNKNOWN
Thinking about potential future types, it seems most (symlinks,
compressed files, etc) can be better implemented via custom attributes.
Using custom attributes doesn't mean the filesystem _can't_ inject
special behavior, and custom attributes allow for perfect backwards
compatibility.
So with future types less likely, forwarding type info to users is less
important (and potentially error prone). Instead, allowing on-disk +
internal types to be represented densely is much more useful.
And it avoids setting an upper bound on future types prematurely.
---
This also includes a minor rcompat/wcompat rework. Since we're probably
going to end up with 32-bit rcompat flags anyways, might as well make
them more human-readable (nibble-aligned):
LFS_RCOMPAT_NONSTANDARD 0x00000001 Non-standard filesystem format
LFS_RCOMPAT_WRONLY 0x00000002 Reading is disallowed
LFS_RCOMPAT_BMOSS 0x00000010 Files may use inlined data
LFS_RCOMPAT_BSPROUT 0x00000020 Files may use block pointers
LFS_RCOMPAT_BSHRUB 0x00000040 Files may use inlined btrees
LFS_RCOMPAT_BTREE 0x00000080 Files may use btrees
LFS_RCOMPAT_MMOSS 0x00000100 May use an inlined mdir
LFS_RCOMPAT_MSPROUT 0x00000200 May use an mdir pointer
LFS_RCOMPAT_MSHRUB 0x00000400 May use an inlined mtree
LFS_RCOMPAT_MTREE 0x00000800 May use an mdir btree
LFS_RCOMPAT_GRM 0x00001000 Global-remove in use
LFS_WCOMPAT_NONSTANDARD 0x00000001 Non-standard filesystem format
LFS_WCOMPAT_RDONLY 0x00000002 Writing is disallowed
LFS_WCOMPAT_REG 0x00000010 Regular file types in use
LFS_WCOMPAT_DIR 0x00000020 Directory file types in use
LFS_WCOMPAT_STICKYNOTE 0x00000040 Stickynote file types in use
LFS_WCOMPAT_GCKSUM 0x00001000 Global-checksum in use
---
Code changes:
code stack ctx
before: 35928 2440 640
after: 35924 (-0.0%) 2440 (+0.0%) 640 (+0.0%)
Now that LFS_TYPE_STICKYNOTE is a real type users can interact with, it
makes sense to group it with REG/DIR. This also has the side-effect of
making these contiguous.
---
LFSR_TAG_BOOKMARKs, however, are still hidden from the user. This
unfortunately means there will be a bit of a jump if we ever add
LFS_TYPE_SYMLINK in the future, but I'm starting to wonder if that's the
best way to approach symlinks in littlefs...
If instead LFS_TYPE_SYMLINKS were implied via custom attribute, you
could avoid the headache that comes with adding a new tag encoding, and
allow perfect compatibility with non-symlink drivers. Win win.
This seems like a better approach for _all_ of the theoretical future
types (compressed files, device files, etc), and avoids the risk of
oversaturating the type space.
---
This had a surprising impact on code for just a minor encoding tweak. I
guess the contiguousness pushed the compiler to use tables/ranges for
more things? Or maybe 3 vs 5 is just an easier constant to encode?
code stack ctx
before: 35952 2440 640
after: 35928 (-0.1%) 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%)
This may be useful for compression in the future, where compression +
noise can result in blocks _larger_ than the expected weight.
Thinking about how compression might be integrated into littlefs, it
would be nice if such a topology did _not_ trigger asserts. This would
allow littlefs images to interact with compressed files at least a
little bit (rename/remove could be very useful), even if the compression
algorithm isn't supported.
Supporting this requires only a single clamp in lfsr_file_lookupleaf,
but it's a little bit more costly than you might expect:
code stack ctx
before: 35692 2440 640
after: 35740 (+0.1%) 2440 (+0.0%) 640 (+0.0%)
This is due to internal API awkwardness:
1. LFSR_DATA_TRUNCATE is surprisingly costly
2. We need to create a local weight copy in case the caller's is NULL
- test_fwrite_reversed_litmus_fragments
- test_fwrite_reversed_litmus_blocks
- test_fwrite_freversed
- test_fwrite_freversed_litmus_fragments
- test_fwrite_freversed_litmus_blocks
- test_fwrite_truncate_pos
- test_fwrite_fruncate_pos
And hey, they found some bugs:
- crystal_thresh=-1 was broken due to integer overflow in some signed
math.
Fortunately when crystal_thresh=-1 we can just skip the crystal
lookups entirely. This saves a btree lookup in fully-fragmented files.
- We were including empty fragments in our crystal size, when we should
only use them to determine crystal boundaries, like bptrs.
This is a common case for the first entry in a sparse file.
- We weren't updating pos on fruncate. fruncate's effect on pos was
actually not tested at all.
Which raises the question, what should the behavior be? Match
lfsr_file_truncate and leave the pos unaffected?
I ended up having fruncate update the file pos to keep the same pos
relative to the end, as I figured this would have the least surprise
for users. So lfsr_file_read should return the same bytes unless
clobbered.
This is almost a mirror image of lfsr_file_truncate, except we don't
allow negative positions, so fruncating more than pos forces pos to 0.
---
This behavior is now covered in a couple tests:
- test_fwrite_truncate_pos
- test_fwrite_fruncate_pos
- test_fwrite_freversed
- test_fwrite_freversed_litmus_fragments
- test_fwrite_freversed_litmus_blocks
Code changes:
code stack ctx
before: 35688 2440 640
after: 35692 (+0.0%) 2440 (+0.0%) 640 (+0.0%)
This lets you specify mount/format flags globally, via -DLFS_YES_REVDBG,
for example.
In addition to the convenience of not needing to edit code, these flags
may also be able to reduce code cost by eliminating the various flag
checks and untaken code paths.
At the moment this relies on dead code elimination via the lfsr_m_is*
functions, to keep the codebase from exploding too much.
This tweaks a number of extended revision count things:
- Added LFS_REVDBG, which adds debug info to revision counts.
This initializes the bottom 12 bits of every revision count with a
hint based on rbyd type, which may be useful when debugging:
- 68 69 21 v0 (hi!.) => mroot anchor
- 6d 72 7e v0 (mr~.) => mroot
- 6d 64 7e v0 (md~.) => mdir
- 62 74 7e v0 (bt~.) => file btree node
- 62 6d 7e v0 (bm~.) => mtree node
This may be overwritten by the recycle counter if it overlaps, worst
case the recycle counter takes up the entire revision count, but these
have been chosen to at least keep some info if partially overwritten.
To make this work required the LFS_i_INMTREE hack (yay global state),
but a hack for debug info isn't the end of the world.
Note we don't have control over data blocks, so there's always a
chance they end up containing what looks like one of the above
revision counts.
- Renamed LFS_NOISY -> LFS_REVNOISE
- LFS_REVDBG and LFS_REVNOISE are incompatible, so using both asserts.
This also frees up the theoretical 0x00000030 state for an additional
rev mode in the future.
- Adopted LFS_REVNOISE (and LFS_REVDBG) in btree nodes as well.
If you need rev noise, you probably want it in all rbyds/metadata
blocks, not just mdirs.
---
This had no effect on the default code size, but did affect
LFS_REVNOISE:
code stack ctx
before: 35688 2440 640
after: 35688 (+0.0%) 2440 (+0.0%) 640 (+0.0%)
revnoise before: 35744 2440 640
revnoise after: 35880 (+0.4%) 2440 (+0.0%) 640 (+0.0%)
default: 35688 2440 640
revdbg: 35912 (+0.6%) 2448 (+0.3%) 640 (+0.0%)
revnoise: 35880 (+0.5%) 2440 (+0.0%) 640 (+0.0%)
This is based off the parity impl in Sean Eron Anderson's Bit Twiddling
Hacks, who attributes the idea to Mathew Hendry.
Basically the idea is to encode a small lookup table in an integer, and
extract using a shift + mask:
.-- LFSR_TAG_MASK0
.|-- LFSR_TAG_MASK2
.||-- LFSR_TAG_MASK8
.|||-- LFSR_TAG_MASK12
vvvv
0x0fff & (-1U << ((0xc820 >> (4*((tag >> 12) & 0x3))) & 0xf))
'--.-' ^ '--------.--------'
key mask gcc complains w/o this mask bits
Saves a bit of code at the cost of some stack. I guess because GCC is
trying to avoid multiple constant pool lookups? This may just be
compiler noise:
code stack ctx
before: 35692 2432 640
after: 35688 (-0.0%) 2440 (+0.3%) 640 (+0.0%)
Velociraptors inbound.
This eliminates dags (directed acyclic graphs) from file bshrubs/btrees,
which were the only source of dags in the filesystem. This means
littlefs is now strictly a pure tree, in that no blocks have more than
one parent (ignoring in-RAM references!).
Up until this point, dags could be created in file bshrubs/btrees via
random writes that place fragments in the middle of a block:
.-------------. .-------------------.
| aaaaaaaaaaa | -> | aaaaa | b | aaaaa |
'-------------' '-------------------'
| | v |
v | .-. |
.-------------. | |b| |
| aaaaaaaaaaa | v '-' v
'-------------' .-------------.
| aaaaaaaaaaa |
'-------------'
Now, fragments that would create dags instead trigger block
recrystallization, rewriting the left sibling into a new block if
necessary:
.-------------. .----------------.
| aaaaaaaaaaa | -> | aaaaab | aaaaa |
'-------------' '----------------'
| | '-.
v v v
.-------------. .--------. .-------.
| aaaaaaaaaaa | | aaaaab | | aaaaa |
'-------------' '--------' '-------'
Allowing dags was great for random-write performance, but it creates
problems for future planned features:
1. Current plans for more advanced block allocators rely on blocks only
having one parent. Otherwise it's difficult to know which reference
is the last reference to a block.
2. Dags create a really funny problem for error correction via block
redundancy. Naively, if you try to repair blocks every time you
encounter a given block error, you will end up exploding the block
into n copies, 1 for every parent. Not great!
---
Eliminating these dags was a bit... tricky...
Originally I was planning to just alloc/rewrite blocks in
lfsr_file_carve, but it turns out we can make lfsr_file_flush_ do all
the work with an extra would-dag checks. Handling dags in
lfsr_file_flush_ also gives us a chance to merge any pending data and
get the most out of the block rewrite.
This does give us a bit of technical debt in that we will probably still
need the block splitting in lfsr_file_carve for future features
(advanced hole APIs, alternative write strategies, etc), but it's
probably worth it for code savings in the default build.
Unfortunately this does add to the mess that is lfsr_file_flush_'s
control flow graph:
lfsr_file_flush_
|
v
.--> lookup left crystal .--> lookup left sibling <-.
| | | | |
| v | v |
| erased? | dag? (new!) |
| .---------y n | .---------y n |
| | v | | v |
| | lookup right crystal | | lookup right sibling |
| | | | | | |
| | v | | v |
| | >=crystal_thresh? | | coalesce |
| | y n------------' | | |
| | v | v |
| | lookup left neighbor | carve-----------'
| | | |
| | v |
| | erased? |
| +---------y n |
| | v |
| | alloc <---+-------'
| | | |
| | v |
| '---> crystallize |
| | |
| v |
| good? |
| y n------'
| v
'----------carve
I did scratch my head for a bit trying to think if there was a better
way to organize this, but came up empty.
It looks complicated, but we really only have two* loops (ignoring the
relocation loop): One that crystallizes blocks, and one that coalesces
fragments. The problem is that we end jumping between the two depending
on what we find in the btree.
In a sane system, this would be implemented as mutually recursive
functions, but this is littlefs, the whole point is that we don't use
recursion.
---
The good news is that this added surprisingly little code (and saved
stack?):
code stack ctx
before: 35600 2448 640
after: 35692 (+0.3%) 2432 (-0.7%) 640 (+0.0%)
- Trying to prefer crystal over compact verbiage to try to avoid
confusion with metadata/rbyd compaction
- crystal_thresh >= block_size implying a fully-fragmented file was a
mistake, it should be crystal_thresh > block_size.
crystal_thresh == block_size has the behavior of waiting until the
last moment to crystallize a block, but this still breaks the
fully-fragmented random-write guarantee.
This changed during development, so the comment was probably just
outdated.
Bit of a silly, but problematic, bug, probably introduced during the
various lfsr_bptr_t/lfsr_data_t reworks, but basically we never actually
fragmented the last fragment in a bptr.
We were fragmenting all fragments in a bptr _above_ fragment_size, but
then we'd stop at the last fragment and keep it around as a bptr,
completely wasting all of the work to fragment the block. The reason for
the different behavior being that we can combine the last fragment with
the carved data to avoid an additional commit.
Fortunately the solution is pretty non-invasive. We can just assume any
bptrs <= fragment_size should be written out as fragments.
Added test_fwrite_truncate_litmus_fragment and
test_fwrite_fruncate_litmus_fragment to catch this in the future.
Code changes:
code stack ctx
before: 35588 2448 640
after: 35600 (+0.0%) 2448 (+0.0%) 640 (+0.0%)
- dropped lfsr_btree_commitleaf
- dropped lfsr_bshrub_commitleaf
- dropped lfsr_file_commitleaf
The problem is that, thanks to rbyd compactions/splits/merges/etc, we
end up leaving the leaf rbyd in a more-or-less undefined state.
I was trying to adopt commitleaf in lfsr_file_carve, the function with
the most glaring potential for commitleaf, but the leaf rbyd behavior is
extremely error prone and requires quite a bit of extra circuitry to use
correctly.
The end result looked like it would need more code, more stack
(lfsr_file_carve _is_ on the stack hot path), for a minor speed
improvement. So I decided to drop the idea. We can probably expect file
carving to be dominated by progs/erases anyways.
lfsr_btree_commit_ still needs to lookup parent rbyds, so it would have
only saved ~1 out of O(log_b n) btree lookups (though this may still be
significant given the ridiculous branching factor of btrees).
---
But it _is_ interesting to note that there is still potential
performance savings on the floor if we didn't care about code size.
Without necessarily sacrificing our bounded RAM constraint.
I could imagine a build in the future that prioritizes performance over
code size by strictly using leaf rbyd functions, iterating over leaf
rbyds before iterating the parent btree, etc.
But that's the future. Simply getting things working is the priority
right now.
---
Saves a bit of code/stack:
code stack ctx
before: 35600 2456 640
after: 35588 (-0.0%) 2448 (-0.3%) 640 (+0.0%)
Note that lookupleaf is still useful for the case where bids have
multiple attrs attached (none so far, but the plan is for the dedup tree
to leverage this).
This should have been updated when we dropped becksums (way back in
5fa85583!), we only ever need at most 3 rattrs to complete a carve
operation (left sibling, rattr, right sibling).
Just a free 24 byte stack savings sitting right there:
code stack ctx
before: 35600 2480 640
after: 35600 (+0.0%) 2456 (-1.0%) 640 (+0.0%)
So now crystal_thresh only controls when fragments are compacted into
blocks, while fragment_thresh controls when blocks are broken into
fragments. Setting fragment_thresh=-1 will follow crystal_thresh and
keeps the previous behavior.
These were already two separate pieces of logic, so it makes sense to
provide two separate knobs for tuning.
Setting fragment_thresh lower than crystal_thresh has some potential to
reduce hysteresis in cases where random writes push blocks close to
crystal_thresh. It will be interesting to explore this more when
benchmarking.
---
The additional config option adds a bit of code/ctx, but hopefully that
will go away in the future config rework:
code stack ctx
before: 35584 2480 636
after: 35600 (+0.0%) 2480 (+0.0%) 640 (+0.6%)
This lets us cram in one more mask for potential redund bits:
name tag mask
LFSR_TAG_MASK0 0x0000 0x0fff ---- 1111 1111 1111
LFSR_TAG_MASK2 0x1000 0x0ffc ---- 1111 1111 11--
LFSR_TAG_MASK8 0x2000 0x0f00 ---- 1111 ---- ----
LFSR_TAG_MASK12 0x3000 0x0000 ---- ---- ---- ----
'.-' '.-' '---.---'
mode bits -' | | ^
suptype ------' | |
subtype --------------' |
redund bits ------------------'
I toyed around with a bitwise alternative to the lookup table, but
couldn't come up with anything simpler than these:
- 0xfff & ~((((1<<((i>>1)*8))-1) << ((i&1)*4)) | ((1<<(i*2))-1))
- 0xfff & ~((1 << (((i>>1)*8)+((i&1)<<(1+(i>>1)))))-1)
- 0xfff & ~((1<<(2*i*i))-1) (requires multiply and 32-bit shift)
---
This also replaces the mdir/rbyd/btree/mtree lookup/sublookup/suplookup
functions with a single flexible lookup function that accepts tag masks.
This ended up adding a bit of code/stack (the extra NULL args are
surprisingly pricey), but will hopefully make the redund bits
easier/cheaper to use:
code stack ctx
before: 35548 2472 636
after: 35584 (+0.1%) 2480 (+0.3%) 636 (+0.0%)
This was a surprising side-effect the script rework: Realizing the
internal btree/rbyd lookup APIs were awkwardly inconsistent and could be
improved with a couple tweaks:
- Adopted lookupleaf name for functions that return leaf rbyds/mdirs.
There's an argument this should be called lookupnextleaf, since it
returns the next bid, unlike lookup, but I'm going to ignore that
argument because:
1. A non-next lookupleaf doesn't really make sense for trees where
you don't have to fetch the leaf (the mtree)
2. It would be a bit too verbose
- Adopted commitleaf name for functions that accept leaf rbyds.
This makes the lfsr_bshrub_commit -> lfsr_btree_commit__ mess a bit
more readable.
- Strictly limited lookup and lookupnext to return rattrs, even in
complex trees like the mtree.
Most use cases will probably stick to the lookupleaf variants, but at
least the behavior will be consistent.
- Strictly limited lookup to expect a known bid/rid.
This only really matters for lfsr_btree/bshrub_lookup, which as a
quirk of their implementation _can_ lookup both bid + rattr at the
same time. But I don't think we'll need this functionality, and
limited the behavior may allow for future optimizations.
Note there is no lfsr_file_lookup. File btrees currently only ever
have a single leaf rattr, so this API doesn't really make sense.
Internal API changes:
- lfsr_btree_lookupnext_ -> lfsr_btree_lookupleaf
- lfsr_btree_lookupnext -> lfsr_btree_lookupnext
- lfsr_btree_lookup -> lfsr_btree_lookup
- added lfsr_btree_namelookupleaf
- lfsr_btree_namelookup -> lfsr_btree_namelookup
- lfsr_btree_commit__ -> lfsr_btree_commit_
- lfsr_btree_commit_ -> lfsr_btree_commitleaf
- lfsr_btree_commit -> lfsr_btree_commit
- added lfsr_bshrub_lookupleaf
- lfsr_bshrub_lookupnext -> lfsr_bshrub_lookupnext
- lfsr_bshrub_lookup -> lfsr_bshrub_lookup
- lfsr_bshrub_commit_ -> lfsr_bshrub_commitleaf
- lfsr_bshrub_commit -> lfsr_bshrub_commit
- lfsr_mtree_lookup -> lfsr_mtree_lookupleaf
- added lfsr_mtree_lookupnext
- added lfsr_mtree_lookup
- added lfsr_mtree_namelookupleaf
- lfsr_mtree_namelookup -> lfsr_mtree_namelookup
- added lfsr_file_lookupleaf
- lfsr_file_lookupnext -> lfsr_file_lookupnext
- added lfsr_file_commitleaf
- lfsr_file_commit -> lfsr_file_commit
Also added lookupnext to Mdir/Mtree in the dbg scripts.
Unfortunately this did add both code and stack, but only because of the
optional mdir returns in the mtree lookups:
code stack ctx
before: 35520 2440 636
after: 35548 (+0.1%) 2472 (+1.3%) 636 (+0.0%)
The exception being LFS_DEBUG. A bit inconsistent, but the at least
consistent with LFS_ERR* vs LFS_ERROR, and may help reduce name
conflicts:
- LFS_DEBUGRBYDFETCHES -> LFS_DBGRBYDFETCHES
- LFS_DEBUGRBYDBALANCE -> LFS_DBGRBYDBALANCE
- LFS_DEBUGRBYDCOMMITS -> LFS_DBGRBYDCOMMITS
- LFS_DEBUGBTREEFETCHES -> LFS_DBGBTREEFETCHES
- LFS_DEBUGBTREECOMMITS -> LFS_DBGBTREECOMMITS
- LFS_DEBUGMDIRFETCHES -> LFS_DBGMDIRFETCHES
- LFS_DEBUGMDIRCOMMITS -> LFS_DBGMDIRCOMMITS
- LFS_DEBUGALLOCS -> LFS_DBGALLOCS
So mbid=0 now implies the mdir is not inlined.
Downsides:
- A bit more work to calculate
- May lose information due to masking everything when mtree.weight==0
- Risk of confusion when in-lfs.c state doesn't match (mbid=-1 is
implied by mtree.weight==0)
Upsides:
- Includes more information about the topology of the mtree
- Avoids multiple dbgmbids for the same physical mdir
Also added lfsr_dbgmbid and lfsr_dbgmrid to help make logging
easier/more consistent.
And updated dbg scripts.
- mdir_bits -> mbits
- lfsr_mid_bid -> lfsr_mbid
- lfsr_mid_rid -> lfsr_mrid
These now match the naming in the dbg scripts.
I feel like this is more terse in a way that is also more readable, but
maybe that's just me.
So now:
(block_size)
mbits = nlog2(----------) = nlog2(block_size) - 3
( 8 )
Instead of:
( (block_size))
mbits = nlog2(floor(----------)) = nlog2(block_size & ~0x7) - 3
( ( 8 ))
This makes the post-log - 3 formula simpler, which we probably want to
prefer as it avoids a division. And ceiling is arguably more intuitive
corner case behavior.
This may seem like a minor detail, but because mbits is purely
block_size derived and not configurable, any quirks here will become
a permanent compatibility requirement.
And hey, it saves a couple bytes (I'm not really sure why, the division
should've been optimized to a shift):
code stack ctx
before: 35528 2440 636
after: 35520 (-0.0%) 2440 (+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.
I think passing around rattr.u.datas was undefined behavior, since we
could end up un-unioning it as le32/leb128/lleb128 later.
Taking rattr.u directly in LFSR_RATTR_ means we can't use LFSR_RATTR_ to
create lfsr_rattr_ts from scratch, since rattr.u is not actually a named
type, but we conveniently don't need to do this. And maybe that's a good
thing since it forces the typed variants?
Also renamed rattr.u.data_count -> rattr.u.count now that rattr.u.data
is no longer the default interface in lfsr_rattr_t.
---
Curiously saves a tiny bit of code. If anything I'd have expected a code
increase:
code stack ctx
before: 35536 2440 636
after: 35528 (-0.0%) 2440 (+0.0%) 636 (+0.0%)
This does a couple things:
- Makes attr-lists a bit more self-documenting.
- Adds a bit more type-safety. The LFSR_RATTR_* macros should be able to
reject types that don't match the expected encoding.
- Makes it easier to adjust dsize estimates at one location.
Specifically, this makes it harder to forget bptr's LFSR_BPTR_DSIZE.
---
Surprisingly this did have a small impact on code size. I'm not entirely
sure why, but considering how much of the codebase this touches I'm just
going to chalk this up to compiler noise:
code stack ctx
before: 35488 2440 636
after: 35536 (+0.1%) 2440 (+0.0%) 636 (+0.0%)
lfsr_file_carve seems the hardest hit:
function (0 added, 0 removed) osize nsize dsize
lfsr_file_open 16 20 +4 (+25.0%)
lfsr_file_carve 1316 1356 +40 (+3.0%)
lfsr_remove 408 412 +4 (+1.0%)
TOTAL 35488 35536 +48 (+0.1%)
Mainly just for self-documentation reasons.
This may also make it easier to add LFSR_RATTR_BUF-specific asserts/
tweaks/etc, and helps future refactoring.
But functionally LFSR_RATTR_BUF is equivalent to LFSR_RATTR for now.
No code changes.
Mainly for self-documentation reasons. This is identical to
LFSR_RATTR_LEB128 except for an additional assert in
lfsr_rbyd_appendrattr_.
I was considering removing the little-leb128 concept, but it is still
helping reduce the worst-case size of the various structs we write to
disk. LFSR_BRANCH_DSIZE, for example, contributes heavily to our stack
hot-path.
No code changes.
The only requirement we have for dsize estimates is to help calculate
shrub estimates. So why bother with dsize for attrs that are never
committed to shrubs?
At the moment, the only tags we commit to shrubs are:
- LFSR_TAG_DATA
- LFSR_TAG_BLOCK
- LFSR_TAG_BRANCH
This saves a decent chunk of code:
code stack
before: 35556 2440 636
after: 35488 (-0.2%) 2440 (+0.0%) 636 (+0.0%)
No idea how long this return has been missing, but we should never
ignore a returned err without at least an assert. And this one should
definitely not be an assert.
Code changes minimal:
code stack ctx
before: 35552 2440 636
after: 35556 (+0.0%) 2440 (+0.0%) 636 (+0.0%)
This was the one lazy attr that did _not_ save us code or stack, and in
fact hurt us a bit. Fortunately our lazy attr scheme still allows for
eager attr encoding, so we can revert just this one attr.
The reason is because we need to keep the children branches around as we
recursively commit up the btree, and, even assuming worst-case, the
branch encoding takes up way less RAM than an active rbyd.
Compare for yourself:
in-RAM rbyd: on-disk branch:
.---+---+---+---. .---+- -+- -+- -+- -.
| weight | | block |
+---+---+---+---+ +---+- -+- -+- -+- -'
| blocks | | trunk |
+ + +---+- -+- -+- -+
| | | cksum |
+---+---+---+---+ '---+---+---+---'
|s| trunk |
+---+---+---+---+
|p| eoff |
+---+---+---+---+
| cksum |
'---+---+---+---'
'-------.-------' '---------.---------'
24 bytes 13 bytes
Note we also don't have to care about alignment issues when passing
around the raw encoding.
There may also be something going on with the compiler assuming all
lfsr_rbyd_t pointers may alias, but it's a bit hard to tell.
Saves both code and stack:
code stack ctx
before: 35592 2472 636
after: 35552 (-0.1%) 2440 (-1.3%) 636 (+0.0%)
With the new internal LFSR_RATTR API, there's really no reason to keep
these around.
At one point these were useful for both the implicit lvalues and
automatic buffer size, but GCC's problems with compound-literals and
code size made them almost always backfire.
Now, they're mostly obsolete thanks to the new LFSR_RATTR_* macros.
We do still have a couple LFSR_DATA_* macros (LFSR_DATA_BUF,
LFSR_DATA_SLICE, etc), but these are a bit more fundamental to the
lfsr_data_t type.
This finishes the eager -> lazy attr encoding rework.
Which makes it a good time to look at the total savings from adopting
lazy attr encoding, though there's still a bit of tinkering to do (eager
branches, cksum tags, etc):
code stack ctx
before lazy-attrs: 36280 2576 636
after lazy-attrs: 35592 (-1.9%) 2472 (-4.0%) 636 (+0.0%)
A ~free 688 byte savings in code and 104 bytes in stack is not bad.
Now that rattr.u.cat is no longer in use, the mysterious code size
increase is gone...
Still no idea why this as _any_ impact on code size though:
code stack ctx
before: 35596 2472 636
after: 35592 (-0.0%) 2472 (+0.0%) 636 (+0.0%)
This fully adopts LFSR_RATTR__ and friends:
- LFSR_RATTR -> LFSR_RATTR__ or LFSR_RATTR_DATA__
- LFSR_RATTR_BUF -> LFSR_RATTR__
- LFSR_RATTR_CAT -> LFSR_RATTR_CAT__
- LFSR_RATTR_NOOP -> LFSR_RATTR_NOOP__
- LFSR_RATTR_NAME -> LFSR_RATTR_NAME__
Note the new LFSR_RATTR__ macro also lets us a drop the special rattr
macros, at the cost of a bit less type safety:
- LFSR_RATTR_RATTRS -> LFSR_RATTR__
- LFSR_RATTR_MOVE -> LFSR_RATTR__
- LFSR_RATTR_GRM -> LFSR_RATTR__ (we weren't using this?)
- LFSR_RATTR_SHRUBCOMMIT -> LFSR_RATTR__
Curiously, this ended up adding ~88 bytes to lfsr_file_carve:
function (0 added, 0 removed) osize nsize dsize
lfsr_file_carve 1228 1316 +88 (+7.2%)
lfsr_mdir_commit 2144 2152 +8 (+0.4%)
lfsr_mdir_commit__ 1192 1188 -4 (-0.3%)
lfsr_file_truncate 184 182 -2 (-1.1%)
lfsr_mount 98 96 -2 (-2.0%)
TOTAL 35508 35596 +88 (+0.2%)
I'm really not sure why, all I can think of is maybe the change from a
forced-inline function to a macro added a bunch of compiler noise?
Still, 80 bytes is not worth two competing LFSR_RATTR APIs. Though
it may be worth looking into this in the future.
Total code changes:
code stack ctx
before: 35508 2472 636
after: 35596 (+0.2%) 2472 (+0.0%) 636 (+0.0%)
- LFSR_TAG_BRANCH -+-> lfsr_data_frombranch
- LFSR_TAG_SHRUBBRANCH -'
This was a bit more involved than the others, since it requires changing
what we store in lfsr_bscratch_t.
I did poke around a bit with trying to reduce the total number of rbyd
allocations in lfsr_btree_commit__, but unfortunately we need both the
sibling rbyds and attr rbyds allocated at the same time while we
recursively figure out how to split/merge/commit.
I also almost forgot to include the dsize estimate, but fortunately this
was caught by our tests. We _do_ need branch dsizes, since they
contribute to any non-inlined bshrub's shrub estimate!
---
This was unfortunately a net-negative. We were actually getting a lot of
value from the smaller encoded branch size:
code stack ctx
before: 35472 2440 636
after: 35508 (+0.1%) 2472 (+1.3%) 636 (+0.0%)
It may be worth reverting this, but I want to see how it interacts when
all of the existing eager rattr encoding logic has been removed.
This one is interesting in that we don't just encode to a buffer, but
need to express the concatenation of did + name somehow. Fortunately we
can still leverage the cat circuitry by setting data_count=-2:
- LFSR_TAG_NAME -+-> cat(fromleb128(did), name)
- LFSR_TAG_REG -+
- LFSR_TAG_DIR -+
- LFSR_TAG_STICKYNOTE -'
This does break our shrub estimate for name attrs (currently names have
no technical limit), which would be an issue, but we just happen to never
commit names to shrubs.
In theory you _could_ accurately estimate name attrs if you limited
names to <=(2^15)-5, but I figured this wouldn't be worth the extra code
cost in LFSR_RATTR_NAME__... Especially since it would just go unused...
Saves a nice bit of code, though no stack since we currently don't
allocate any names on the hot-path (lfsr_file_truncate):
code stack ctx
before: 35580 2440 636
after: 35472 (-0.3%) 2440 (+0.0%) 636 (+0.0%)
Note this does _not_ include LFSR_TAG_BOOKMARK, which only contains the
did and can avoid a stack allocation if encoded as a single leb128 attr.
Though breaking up the LFSR_TAG_NAME types does risk a more complicated
switch-case-table...
- LFSR_TAG_GEOMETRY ---> lfsr_data_fromgeometry
Not much to say about this one, LFSR_TAG_GEOMETRY is a bit of an
outlier.
I did consider deduplicating with the mptr encoder, but decided that
would be too hacky, and create problems for future metadata redundancy
things.
Still saves code though, which is nice:
code stack ctx
before: 35632 2440 636
after: 35580 (-0.1%) 2440 (+0.0%) 636 (+0.0%)
- LFSR_TAG_RCOMPAT -+-> lfsr_data_fromle32
- LFSR_TAG_WCOMPAT -+
- LFSR_TAG_OCOMPAT -+
- LFSR_TAG_GCKSUMDELTA -'
- LFSR_TAG_NAMELIMIT -+-> lfsr_data_fromleb128
- LFSR_TAG_FILELIMIT -+
- LFSR_TAG_BOOKMARK -+
- LFSR_TAG_DID -'
This is nice mainly from an internal API standpoint. Single le32/leb128
attrs should be pretty lightweight, and it's nice for the API to reflect
that.
With a bit of tinkering with the internal lfsr_rattr_t type, we can even
pass these directly in the lfsr_rattr_t struct itself, so no need to
keep single le32/leb128 attrs on the stack:
buffer rattr: cat attr: le32/leb128 attr:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
| tag |0|size | | tag |1|count| | tag |0|dsize|
+---+---+---+---+ +---+---+---+---+ +---+---+---+---+
| weight | | weight | | weight |
+---+---+---+---+ .. +---+---+---+---+ .. +---+---+---+---+
| ptr -------. | ptr -------. | le32/leb128 |
'---+---+---+---' | '---+---+---+---' | '---+---+---+---'
.---+---+---+---. | .---+---+---+---. |
| data |<' |mm| size |<'
: : : +---+---+---+---+
| data |
+ +
| |
+---+---+---+---+
|mm| size |
: : :
While tinkering I also ended up renaming a couple things:
- rattr.cat -> rattr.u.datas, rattr.u.buffer, rattr.u.etc
- rattr.count -> rattr.data_count
- added lfsr_rattr_dtag for ignoring on-disk/explicit-data tags
- lfsr_rattr_size -> lfsr_rattr_dsize
Surprisingly very little code savings though. I guess we don't use
single le32/leb128 attrs enough to overcome the added complexity to
lfsr_rbyd_appendrattr_'s switch-case-table?
code stack ctx
before: 35636 2440 636
after: 35632 (-0.0%) 2440 (+0.0%) 636 (+0.0%)
That or there's something else weird going on with this union and
compiler assumptions. Attempting to adopt .u.etc in LFSR_RATTR__ alone
adds ~100 bytes of code, even though both .u.etc and .u.cat are the same
type (const void *)...
Not entirely sure what's going on...
- LFSR_TAG_BLOCK -+-> lfsr_data_frombptr
- LFSR_TAG_SHRUBBLOCK -'
This is where lazy attr encoding causes some problems for our shrub
estimate calculation. As a workaround, lfsr_rattr_t includes the
worst-case encoding size (LFSR_BPTR_DSIZE) in the otherwise-unused count
field, which avoids needing a second tag lookup.
This makes our shrub estimate a little bit worse, but is unavoidable
without reencoding bptr attrs.
On the bright side, this led to a bit of simplication in
lfsr_file_carve.
Saves more code and stack:
code stack ctx
before: 35848 2504 636
after: 35636 (-0.6%) 2440 (-2.6%) 636 (+0.0%)
The idea here is to move as much attr encoding logic as possible into
lfsr_rbyd_appendrattr_, so we don't encode most attrs until the last
minute, right before we write the tag+data to disk.
This has some pretty big theoretical benefits:
- Deduplicates encoding logic, so most attrs will only have a single
lfsr_data_from* call in the entire system.
This saves code size used for function calls, stack allocations, etc.
- In theory, _significantly_ better stack usage.
The main downside with eager encoding is that we need a buffer to
hold the encoding, and this buffer needs to stay allocated while all
of the commit machinery does its work.
This ends up stacking when any low-level attr buffers in
lfsr_btree_commit/lfsr_mdir_commit/etc, even though we don't _really_
need all of these attrs encoded at the same time.
Heck, we don't even need all of the attrs in the same _commit_ to be
encoded at the same time.
Lazily encoding avoids all of this.
- It's actually a nicer internal API, and means less risk we lose/
misallocate one of the encoding buffers.
The main downside is this makes attr encodings less gc-able. However, so
far it seems like you need most tags the moment you try to write to the
filesystem, and unwanted code costs can be worked around by allowing
more code to be conditionally compiled-out (at a testing cost).
This also means we don't know the actual on-disk attr size until we're
writing attrs out to disk. Fortunately, we've ended up relying on attr
size less than I thought we would. We still need it for shrub estimates,
but we can use the worst-case encoding size (LFSR_BPTR_DSIZE) there.
---
To start, this adopts lazy attr encoding for most of the obvious/
less-involved attrs:
- LFSR_TAG_BSHRUB ---> lfsr_data_fromshrub
- LFSR_TAG_BTREE -+-> lfsr_data_frombtree
- LFSR_TAG_MTREE -'
- LFSR_TAG_MROOT -+-> lfsr_data_frommptr
- LFSR_TAG_MDIR -'
- LFSR_TAG_ECKSUM ---> lfsr_data_fromecksum
Of interesting note is LFSR_TAG_BSHRUB. These changes actually make
shrub trunk encoding less of a special case, which _must_ be lazily
encoded due to last minute shrub changes caused by mdir compactions,
relocations, etc. This lets us drop the unique LFSR_TAG_SHRUBTRUNK
handling.
Though it does risk bugs if a future refactor ever reverts to eager
encoding... I've tried to highlight this with comments around
LFSR_TAG_BSHRUB's encoding.
These changes also required moving a significant number of the
LFSR_*_DSIZE macros around so they are declared before
lfsr_rbyd_appendrattr_. This is unfortunate as it moves them farther
away from from the related lfsr_data_from* implementations, but as far
as I'm aware there's no way around this.
We also need to _not_ lazily encode when an attr is in the concatenated-
data form (count < 0), or else this breaks mdir compaction. This has the
interesting side-effect of still allowing eager encoding with
LFSR_DATA_BUF, which, while less efficient, is very useful for our
tests.
---
So far, code/stack changes look promising:
code stack ctx
before: 36280 2576 636
after: 35848 (-1.2%) 2504 (-2.8%) 636 (+0.0%)
This is the correct name for our rbyd attr type, even if it requires a
bit more typing.
lfsr_attr_t would be a better name, but that conflicts with our
user-facing attrs.
While I think shrub_size is probably the more correct name at a
technical level, inline_size is probably more what users expect and
doesn't require a deeper understanding of filesystem details.
The only risk is that users may think inline_size has no effect on large
files, when in fact it still controls how much of the btree root can be
inlined.
There's also the point that sticking with inline_size maintains
compatibility with both the upstream version and any future version that
has other file representations.
May revisit this, but renaming to lfs->cfg->inline_size for now.
Now that we no longer have bmoss files, inline_size and shrub_size are
effectively the same thing.
We weren't using this, so no code change, but it does save a word of
ctx:
code stack ctx
before: 36280 2576 640
after: 36280 (+0.0%) 2576 (+0.0%) 636 (-0.6%)
At the cost of a bit more frequent estimate scans.
Saves a tiny bit of code:
code stack ctx
before: 36300 2576 640
after: 36280 (-0.1%) 2576 (+0.0%) 640 (+0.0%)
May revisit this in the future, but this is the best solution I can
think of right now, that doesn't run into duplicate macro-argument
side-effect issues...
Statement expressions would be another solution, but that's even less
portable!
At the moment this is only used for functions that implement
LFSR_DATA_* and LFSR_RAT_* macros. These _need_ to be inlined to avoid a
large code-size explosion, and GCC seems to have issues with this.
For most of the other inlinable functions, relying on C99's inline +
compiler heuristics seems to be fine.
Code changes:
before: 36304 2576 640
no-forceinline: 36460 (+0.4%) 2664 (+3.4%) 640 (+0.0%)
yes-forceinline: 36300 (-0.0%) 2576 (+0.0%) 640 (+0.0%)
This breaks down the in-progress lfsr_bptr_t in lfsr_file_flush_ into
its components (block, off, eoff, cksum), until right before we write it
to disk.
This may seem counterproductive. Some paths, such as block appends,
start with an lfsr_bptr_t, but breaking it down this way avoids the
lfsr_bptr_t encoding noise and gives the compiler a better chance to
optimize things.
The end result is a nice bit of code/stack savings, and a slightly more
readable function:
code stack ctx
before: 36320 2584 640
after: 36304 (-0.0%) 2576 (-0.3%) 640 (+0.0%)
Christopher Haster