Realistically, because our btree is protected by CoW checksums, the only
place we can end up with a cycle is in our mroot chain.
This is convenient, as we don't need our btree traversal state when
traversing the mroot chain, so we can put both the tortoise state and
btree traversal state into a union, theoretically saving some RAM.
Unfortunately stack measurements show no change, even though our mtree
traversal in on the hot path. I'm not sure why this is. My best guess is
that the RAM savings is beneath the compilation noise floor, since we
currently only ever create one of these structs.
- lfsr_mid_cmp no longer uses a union. This was undefined behavior and
the lfsr_mid_t type isn't word aligned, so this could break pretty badly
on machine/compiler change.
Also dropped ordering based on endianness, since we need to marshal
these into an int for the comparison anyways.
- Changed lfsr_mdir_cmp to use min/max functions as part of the
comparison. The result is also "ordered" now, though the ordering
is nonsensical. I guess the mrootanchor is less than all other mdirs?
Also considered only comparing a single min/max block, since it would
be an error for mdirs to share blocks, but note we rely on
lfsr_mdir_cmp to check for relocations in lfsr_mdir_commit. These
relocations can end up being partial in the case of bad block
detection.
Two reasons:
- The lfsr_data_t API is a bit too high-level for our rbyd functions,
which need to jump around inside the block, keep track of several
offsets simultaneously, check for boundary conditions, etc.
- Stack measurements showed a +1.6% stack increase, likely due to extra
lfsr_data_t copies.
Though there were some cases where adopting lfsr_data_t made sense,
mainly the parsing of the ecksum struct, and along the way some code was
cleaned up in rbyd fetch and rbyd compact, so after reverting we
actually ended up with less code/stack than when we started:
code stack
before: 21702 1992
lfsr_data_t: 21626 (-0.4%) 2024 (+1.6%)
after: 21666 (-0.2%) 1976 (-0.8%)
The low-level rbyd functions need to parse things (mostly tags), so why
not use our parsers? In theory this offers a bit more code reuse.
In theory we can also rely on lfsr_data_t to do bounds checking of
offsets in the block, in practice we need to setup those bounds
correctly for lfsr_data_t, so not so much...
Code/stack cost:
code stack
before: 21702 1992
after: 21626 (-0.4%) 2024 (+1.6%)
This is kind of messy. The fact that btrees encode any inlined
entry's types directly in the tag, and that btree have multiple tags
themselves (btree (future), mtree, ptree (future), gftree (future)),
means we need several extra parameters to make the btree to/from disk
functions work.
This is going to get more complex with file btrees having their own
inline system.
So for now I've moved the inlined to/from disk logic up into upper
layers, limiting btree to/from disk functions to only parse actual
btrees.
Since btree/branch to/from disk functions are basically the same thing
now, the two have been merged into the btree to/from disk functions.
Composable parsing functions always feel a bit weird to me in C. I don't
know if this is because of something C lacks, such as multiple return
values, or if composable parsers are just inherently awkward to describe
in procedural languages because of the different levels of state.
But I think the API here is pretty ok. The main idea is that data
parsers can be added as functions in the lfsr_data_* namespace that take
lfsr_data_t as a mutable reference, updating the lfsr_data_t's internal
state as data is parsed.
In practice you only need a couple of primitives, bytes, le32s, leb128s,
that touch the internals of lfsr_data_t, and the other parsers can be
built using these.
This leverages the pointer-like abstraction of lfsr_data_t, and avoids
needing to keep track of offsets. And thanks to lfsr_data_t being
relatively cheap to make copies, this API is relatively flexible.
Some other tweaks:
- Signed leb128 overflow detection is moved up into lfs_fromleb128.
littlefs now assumes _all_ leb128s are 31-bits, which is useful for
leveraging the sign bit internally.
This also fixes the an issue in overflow detection in lfs_fromleb128
which wouldn't catch overflows in the last byte of a >32-bit leb128.
- Most lfsr_data_t functions now take a pointer. This offered a small
bit of code savings and feels more natural in C. Though most functions
that accept lfsr_data_t still take a copy. Most of these functions
would need to make a copy anyways now that the parsers are consuming,
and these copies avoid concerns about shared state.
At 3-words, lfsr_data_t is right at that boundary of questionable
reasonableness for copying, but copying is a very useful feature of
this struct.
This ends up with some decent code/stack savings:
code stack
before: 22118 2048
after: 21722 (-1.8%) 1992 (-2.7%)
As much as it was a nice way to utilize all 96-bits of lfsr_data_t, the
added code and RAM cost just made this not worth it.
The main problem with inlining leb128s into lfsr_data_t is that
lfsr_data_t reads have to contend with a number of awkward corner cases
involving offsets into the not-yet-encoded leb128s.
Adding to this the extra overhead encoding the number of leb128s in
lfsr_data_t, and the fact that 2 leb128s is mostly useless when metadata
redundancy > 2, I'm reverting this back to only a single optional leb128
in lfsr_data_ts limited to lfsr_bd_progdata:
0 = in-device buffer 1 = on-disk data
.----+----+----+----. .----+----+----+----.
|0| size |.>|1| size |
|----+----+----+----| |----+----+----+----|
| (optional leb128) | | off |
|----+----+----+----| |----+----+----+----|
| buffer | | buffer |
'----+----+----+----' '----+----+----+----'
It's also worth noting that since this leb128 is limited to
lfsr_bd_progdata, it shouldn't add any code cost to readonly variants of
littlefs.
Another interesting thing is that, while this single-injected-leb128-
for-lfsr_bd_progdata sounds limited on paper, it covers a number of
convenient use cases:
- injecting directory-ids into name attributes
- programming single leb128s, such as bookmarks
- prefixing B-tree branches with weights (not-yet implemented)
The idea of this is:
1. Aside from the encoded size, our lfsr_data_t has space for 2 integers.
2. Our mdir addresses are exactly 2 leb128s.
3. We already need to be able to inject 1 leb128 for did entries.
So if we can cram our 2 leb128s inline into the lfsr_data_t, we should
be able to avoid the indirection, wasted space in lfsr_data_t, and
duplicate encoding costs for the mdir addresses.
Conveniently for us, there are exactly 2 unused bits in various fields,
thanks to our common 31-bit limits.
It's a bit awkward since we must assume our buffer pointer uses all
32-bits, but here are the current encodings:
00 = in-device buffer 10 = on-disk data
no leb128s no leb128s
.----+----+----+----. .----+----+----+----.
|0| size | |1| size |
|----+----+----+----| |----+----+----+----|
|0000000000000000000| |0| offset |
|----+----+----+----| |----+----+----+----|
| buffer | | block |
'----+----+----+----' '----+----+----+----'
01 = in-device buffer 11 = 2 leb128s
1 leb128
.----+----+----+----. .----+----+----+----.
|0| size | |1| size |
|----+----+----+----| |----+----+----+----|
|1| leb128 | |1| leb128 |
|----+----+----+----| |----+----+----+----|
| buffer | | leb128 |
'----+----+----+----' '----+----+----+----'
This encoding also presents a relatively nice code-path, since we can
treat the 2 leb128 case as an on-disk data reference with no size.
Unfortunately the initial measurements look, uh, really bad:
code stack
before: 22194 2048
after: 22426 (+1.0%) 2088 (+2.0%)
This needs more investigation, but from what I can tell so far the RAM
cost comes from the leb128 encoding buffer moving into the "hot path",
aka the deepest call stack in littlefs, which involves lfsr_data_read
as a part of mtree traversal as a part of block allocation.
I have no idea about the code cost though...
Generally the more creative you get with C macros, the more
unmaintainable your codebase becomes, but in this case I think a small
bit of macro sugar for the attribute lists in littlefs goes a long way
for making the internals flexible and readable.
Attribute lists generally look like this:
LFSR_ATTRS(
LFSR_ATTR(id, TAG, delta, DATA(data)),
LFSR_ATTR(id, TAG, delta, DATA(data)),
...
LFSR_ATTR(id, TAG, delta, DATA(data)))
Which more-or-less gets expanded to this:
((const lfsr_attr_t[]){
((lfsr_attr_t){id, LFSR_TAG_TAG, delta, LFSR_DATA_DATA(data)}),
((lfsr_attr_t){id, LFSR_TAG_TAG, delta, LFSR_DATA_DATA(data)}),
...
((lfsr_attr_t){id, LFSR_TAG_TAG, delta, LFSR_DATA_DATA(data)}),}),
attr_count
Note the use of preprocessor concatenation to put the TAG and DATA
identifiers in their respective namespaces. These can end up invoking
other macros, which allows attrs to be rather extensible.
Previously there were also LFSR_ATTR_ (note the trailing underscore)
macros to allow passing of variable tags/datas. This is replaced with
redundant macros which sort of "unwrap" themselves as a part of macro
expansion. This avoids a bunch of duplicate macro definitions.
#define LFSR_TAG_TAG(tag) (tag)
#define LFSR_DATA_DATA(data) (data)
So:
LFSR_ATTR(id, TAG(tag), delta, DATA(data))
Becomes:
((lfsr_attr_t){id, LFSR_TAG_TAG(tag), delta, LFSR_DATA_DATA(data)})
Becomes:
((lfsr_attr_t){id, tag, delta, data})
This checksum is used to keep track of if we have erased, and not yet
touched, the unused bytes trailing our current commit in the rbyd.
The working theory is that if any prog attempt is made, it will, most
likely, change the checksum of the contents, allowing littlefs to
determine if trailing erased-state is safe to use, even under powerloss.
littlefs can also perturb future data by a single bit, to force this
checksum to always be invalidated during normal operation.
The original name, "forward erased-state checksums (fcksum)", came from the
idea that the checksum "looks forward" into the next commit.
But after using them for a bit, I think the name is unnecessarily
confusing. It, uh, also looks a lot like a swear word. I think
shortening the name to just "erased-state checksums (ecksum)", even
though the previous name is already in use in a release, is reasonable.
---
It's probably hard to believe but the name change from fcrc -> ecrc
really was unrelated to the crc -> cksum change. But boy is it
convenient for avoiding an awkward name. A lot of these name changes
involved sed scripts, so I didn't notice how awkward fcksum would be to
use until writing this commit message.
The reason for this is to move away from the idea that littlefs is
strictly bound to CRCs and make the code more welcoming to other
checksum types, such as SHA256, etc.
Of course, changing the name doesn't really do anything. littlefs
actually _is_ strictly bound to CRCs in a couple ways that other
filesystems aren't. These would need to have workarounds for other
checksum types:
- We leverage the parity-preserving nature of (some) CRCs to not have
to also calculate the parity of metadata in rbyd commits.
- We leverage the linearity of CRCs to retroactively flip the
perturb bit in the cksum tag without needing to recalculate the
checksum. Though the fact we need to do this is because of how we
use parity above, so this may just not be needed for non-CRC
checksums.
- The plans for global-CRCs (not yet implemented) rely heavily on the
mathematical properties of CRC polynomials. This doesn't mean
global-CRCs can't work with other checksums, you would just need to
find a different type of polynomial.
Unless very obvious, all buf variables should be prefixed with the
related variable they are being used to encode. Unlike other common
variables, bufs need to be sized correctly for what they are encoding.
Sharing bufs between variables is most likely a coding mistake.
Also tried to move away from the single letter 'w' variables, at least
in the C source.
Originally it made sense to name the rbyd ids, well, ids, at least in
the internals of the rbyd functions. But this doesn't work well outside
of the rbyd code, where littlefs has to juggle several different id
types with different purposes:
- rid => rbyd-id, 31-bit index into an rbyd
- bid => btree-id, 31-bit index into a btree
- mid => mdir-id, 15-bit+15-bit index into the mtree
- did => directory-id, 31-bit unique identifier for directories
Even though context makes it clear which id the id refers to in the rbyd
internals, updating the name to rid makes it clearer that these are the
same type of id when looking at code both inside and outside the rbyd
functions.
For a couple reasons:
1. Organizing the overlaps this way avoid potential undefined behavior.
It turns out C does define the overlap the "initial sequence" of
union members, as long as the types are the same. But when we
overlapped the block with the size/tag fields in lfsr_btree_t, it was
probably undefined behavior.
At the very least, it would introduce a need for quite a bit of
preprocessing to make it work with different integer sizes and
redundancy levels.
2. Overlapping the blocks at the end of the rbyd struct means our block
array is natural ordered such that the first block is the "active"
block, i.e. the block with the most recent revision count that passes
checksums.
This has been useful as a debugging tool, so I would like to continue
the pattern. It is possible to mostly preserve this order with the
previous method by intentional reversing the block array when
logging or writing to disk, but it's a bit cumbersome.
2. It's unlikely we'll be able to use readonly variants of the rbyd/mdir
structs for RAM savings. Unfortunately C makes this too cumbersome.
Though if we do this should be revisited.
Here are the new overlaps. Note it's no longer possible to truncate the
types when readonly. If readonly struct are useful this will need to be
revisited again:
lfsr_rbyd_t lfsr_btree_t lfsr_mdir_t
8b 8b 8b 8b
.----+----+----+----.
8b 8b 8b 8b 8b 8b 8b 8b | mid.bid | mid.rid |
.----+----+----+----. .----+----+----+----. |----+----+----+----|
| weight |.>| weight | | weight |
|----+----+----+----| |----+----+----+----| |----+----+----+----|
| trunk | | tag | size | | trunk |
|----+----+----+----| |----+----+----+----| |----+----+----+----|
| off | | inlined data | | off |
|----+----+----+----| | | | |----+----+----+----|
| crc | | v | | crc |
|----+----+----+----| | | |----+----+----+----|
| block |..| |.>| blocks |
'----+----+----+----' '----+----+----+----' | |
| |
'----+----+----+----'
This turned out to be tricky.
At littlefs's core, we have the lfsr_rbyd_t struct. It is really
important this is as small as possible since littlefs creates many rbyd
copies in order to track state of metadata on disk.
Wrapping rbyd, we have the lfsr_btree_t struct, which can alternatively
contain a single inlined entry, accomplished by overlapping the width
field in both cases. And the lfsr_mdir_t struct, which tracks any redundant
blocks, and would be nice if the blocks lined up as neighbors so all blocks
involved in the mdir could be passed around as an array. Both of these
wrappers attempt to overlap fields of the lfsr_rbyd_t struct, which presents
a bit of a problem.
The solution here is to put the rbyd block field at the beginning of the
lfsr_rbyd_t struct, and use exactly 32-bits of padding in lfsr_btree_t
to overlap the width field even though it is not at the beginning of the
struct. To avoid inflating the lfsr_btree_t size, we sneak the inlined
size and tag into the overlapping padding. This will need special
handling if the size of these fields change, but saves a decent amount
of RAM:
lfsr_rbyd_t lfsr_btree_t lfsr_mdir_t
8b 8b 8b 8b
.----+----+----+----.
| mid.bid | mid.rid |
|----+----+----+----|
8b 8b 8b 8b 8b 8b 8b 8b | blocks |
.----+----+----+----. .----+----+----+----. | |
| block |..| tag |size|padd|.>| |
|----+----+----+----| |----+----+----+----| |----+----+----+----|
| weight |.>| weight | | weight |
|----+----+----+----| |----+----+----+----| |----+----+----+----|
| trunk | | inlined data | | trunk |
|----+----+----+----| | | | |----+----+----+----|
| off | | v | | off |
|----+----+----+----| | | |----+----+----+----|
| crc | | | | crc |
'----+----+----+----' '----+----+----+----' '----+----+----+----'
Also tried to reduce the amount of mdir usage in lfsr_mdir_commit by
better using only the arrays of relevant mdir blocks, to limited success.
- Updated LFSR_BTREE_INLINESIZE to properly include the overhead for
mdir pointers, which need 2 block addresses instead of 1. This adds
4 bytes to the lfsr_btree_t struct.
- Changed code that marks rbyds as "needing compaction" to use -1
instead of block_size. This can use a cheaper constant and helps
debugging.
- Changed the mid representation of root to 0.0 from ?.-1. The mid 0.0
is always reserved for the roots dstart, so it shouldn't be used for
any actual file. This disambiguates root vs special metadata mids and
is a step towards making mids unsigned.
It also saves a tiny bit of code since 0 comparisons are generally
cheaper and we can leverage the order-preserving conversion of mid
to an integer.
This didn't really work out as well as I had hoped. There were a few
ideas on how to encode the bid/rid tuple without sacrificing the
(currently 31-bit) integer limit, but these just introduced too much
complexity.
Ideas:
1. In theory, as the mdirs increase in size, the quantity of mdirs needed
for a given number of files decreases. If we say the number of files
fits in an integer of a given size, than we can model the mapping to
mdirs and rids roughly as the number of bits in that integer split
between the two.
Since the block_size is known, the we can find a rather conservative,
yet useful, estimate of the upper bound of rids, which ends up
being ~16 bytes ((2 alts + 1 null + 1 tag) * 4 bytes).
And since our btrees are perfectly balanced, this encoding should only
waste 1 or 2 bits due to rounding to rounding and sign encoding for
special values.
bbbbbbbb bbbbbbbb bbbbbbbr rrrrrrrr
'-----------+-----------''----+---'
| '-- log2(block_size/32)-bit rid
'-------------------- remaining-bit bid
Unfortunately, while this works ok on paper, and maximize the use of
the bits we have available for the mid, the implementation ended up
awkward and difficult to use.
We need to either calculate the relatively complciated log2 of the
block_size on the fly, or cache the value, and use it to shift the
mid around to extract the bid/rid when needed.
Unfortunately, perhaps due to the it being easy to use the bid/rid
directly, we use and mutate the bid/rid quite a bit. We mutate when
updating the mdirs, when decoding grms, when seeking mdirs, etc. If
anything, updating the mid in total is rarer than updating the
bid/rid component in complicated situations.
Note to mention this required access to the lfs config to even begin
decoding, complicating the API and making the result less efficient.
Initial (unoptimized, and not even tested) code size showed ~+800
bytes. So I decided to scrap this.
Maybe it will be worth investigating dynamic rid sizes later, to
increase the possible mtree size for a given mid width. Not sure.
2. Probably one of the worst ideas I've had so far, but it would solve
the mid encoding problem, is to use some form a floating point to
encode the bid/rid pair:
.----------.
v .+-.
bbbbbbbb bbbbbbbb bbbrrrrr rrrrssss
'-----------+-------''----+---''-+'
| | '-- rid bits
| '--------- variable rid
'----------------------- variable bid
An even worse idea would be to use IEEE floating point here. Yes it
would work, and probably work annoyingly well, but we it risk
bringing in a lot of standard conforming backbending that we really
don't care about.
The idea here is to sacrifice some bits to encode the ratio of rid
bits to bid bits. The value of this over the using the block_size is
that we can decode the bid and rid using all of the bits in the
integer alone. Avoiding memory access (and worse debugging) to load
any external constants.
As a plus, all mids in the system would have the same exponent,
simplifying comparisons and other operations.
But this is just trying to solve complexity by adding more
complexity, so I'm not even going to try implementing it.
Still, it's an interesting idea...
In the end I've gone with the KISS implementation. Use half-width
integers, in this case uint16s, for both the bid and rid:
bbbbbbbb bbbbbbbb rrrrrrrr rrrrrrrr
'-------+-------' '-------+-------'
| '-- 16-bit rid
'-------------------- 16-bit bid
This suffers from weakened limits around the number of rids in a block
and number of mdirs in the mtree, which is unfortunate. Still it is
probably worth the tradeoff for the RAM savings and encoding simplicity.
If the mdir is reasonably sized, this does probably approach a decent
distribution of rids and bids in 32-bits. But for outlier cases with
very small and very large mdirs, it risks premature out of bounds
errors.
To protect against mtree errors, we will probably need an additional
configuration option in the form of an mdir limit. Conveniently this
would also provide a way to enforce 2-block mode.
rid errors, on the other hand, depend on block_size/32, so we may not
need another configuration option and can rely on the block_size
to determine if the rids can overflow.
This is probably worth revisiting in the future. Fortunately, with
mdir_limit and block_size configuration options, it should be possible
to increase these limits in the future if this mid bid/rid design
changes.
code stack
before: 22126 2136
after: 22326 (+0.9%) 2088 (-2.2%)
This code size increase was unexpected. Maybe non-32-bit-aligned integers
cost more to load in thumb? Unsure.
The main intention here was to make the tracking of opened mdirs,
mostly opened lfsr_dir_t structs, simpler and more resilient to weird
corner cases. I'm not entirely sure this was successful.
The main changes:
- lfsr_dir_t now contains a full mdir for the dstart entry.
This makes it so that dstarts are not a special case when it comes
to mdir updates, though the fact that directories have 2 mdirs is
still an awkward case on its own.
I considered using two entries in the opened linked-list for this, but
it wouldn't have worked out that well. Both entries need to update the
directory position, so it would have required a third file type. We
would also have needed to make sure removed mdirs mark both mdirs as
removed, otherwise the position mdir would move around arbitrary into
possibly erronous values.
Instead the current solution treats the directory mdirs as a small
array of 2 mdirs, which is as hacky as it is hacky, but does get the
job done with little code duplication.
- Directory positions are updated a bit more intellegently.
Instead of checking if in range before updating, which requires access
to both mdirs and duplicate mid/rid comparison logic, position is
updated without regard for the beginning of the directory, and
un-updated if it was actually out of range of the directory.
This means we only need to compare the mids/rids for each mdir once.
This changes make it so that lfsr_dir_rewind is much cheaper, and
doesn't even need to go to disk. Though I'm not sure it's worth the RAM
increase...
Expanding the lfsr_dir_t dstart entry to a full mdir does a lot for
making mdir updates more consistent, but increases the lfsr_dir_t size
from 52 bytes to 76 bytes (+46.2%).
This is mostly to make it easier to merge mids/rids. Having a special
constant here is tricky when the mid/rid split point is dynamic.
Currently using rbyd.trunk=0 to indicate when an mdir is dropped. This
is nice as it preserves the last mid/rid, which is needed by the readdir
code, and it implicitly returns NOENT to all queries in
lfsr_rbyd_lookup.
When updating any opened mdirs to keep things in sync, we need to know
what rid the mdir is targeting in order to know which on-disk mdir it
should follow in the case of splits. Making this rid an actual member of
the mdir struct simplifies things.
This adds some RAM cost, though the plan is to merge the mid/rid into a
single integer, which requires this change and should actually save RAM
in the long run.
code stack
before: 22342
after: 22204 (-0.6%) 2144 (+1.1%)
This really just plays out what the compiler is already doing, so the
code/stack cost is more-or-less unchanged.
We can at least deduplicate the mroot_ copying of the potentially-failed
mdir_.
- We need to clamp dids to 31-bits. We were clamping to 32-bits
correctly, but we rely on dids fitting in 31-bits to fit them into our
lfsr_data_t type.
This does make our dids more dense when the mtree is near full, but
keeping dids 31-bits (or bound to the file size type) also gives us
more flexibility when it comes to deduplicating common leb128 operations.
- We weren't using the right mask during collision resolution. This was
just an oversight and an unimpactful fix.
Also saved 4 bytes, which is probably the cost of storing the outdated
constant in a nearby constant pool. We don't really care.
I completely forgot we terminate the inner nodes of the rbyd after
compaction with null tags. This means 3 extra tags per tag after
compaction, not 2.
This doesn't actually change the nearest-power-of-two for lfsr_mkdir,
but it does improve the bound for maximum rids, which I have some plans
for.
Instead of reading eagerly and retreating with the hopes of terminating
early (which almost never happens when compacting, since we need to find
the split_id). lfsr_rbyd_estimate now works inward from the first and
last id to find both the dsize and split_id.
One thing that helps this is the addition of a separate per-id
lfsr_rbyd_estimate, which will be useful for checking if the quantity of
file attributes overflows our mdir limitations.
lfsr_rbyd_estimate also now ignores the -1 id for split_id calculation,
since -1 ids are always cleaned up during splitting, though it does
include it in the calculated dsize so that the condition to split is
determined correctly.
---
This also required rebalance changes. Fortunately, one improvement here
is that we can make a simplifying assumption tha the number of tags
can't exceed the maximum possible number of tags in the calculated
dsize. So worst case, if every tag is empty, the maximum possible dsize
becomes 4*(2*log2(dsize/4))+dsize.
Though it's still unclear if rebalance is worth keeping. Current
comparison:
code stack
rebalance: 22362 2120
no_rebalance: 21922 (-2.0%) 2120 (+0.0%)
This means no special case for uninling-but-not-splitting, but allows
the entire split route to be deduplicated, simplifying things.
The main downside is that for littlefs to go from a single inlined mdir
filesystem to an mtree filesystem it requires a minimum of 2 mdir
allocations (4 blocks) in all cases. This can be avoided, but I think is
worth the tradeoff since it generally occurs once in a filesystem's
lifetime.
This does make 4 block block devices a bit awkward, but those geometries
are always going to be a bit awkward with littlefs's design. At least
this implementation avoids an unecessary B-tree node where possible...
code stack
before: 22586 2320
after: 22414 (-0.8%) 2120 (-8.6%)
I _think_, but haven't verified, the significant stack saving comes from
the fact that since there's one route through lfsr_mtree_split_,
lfsr_mtree_split_ can be inlined into lfsr_mdir_commit. This avoids the
marshalling of all its arguments for the function call, which I've
noticed can have a surprising cost.
---
Also fixed a bug where dstart was not updated with mid changes after
splits/drops. The mdir commit cleanup code has a lot of duplication now,
makes me wonder if there's a better way to structure this.
It turned out our dir-read-idempotent test never created non-dstart
neighbors. This was a bit of a problem since we relied on dstart entries
to know when our dir read terminates. If we seek to an invalid position
(in theory undefined behavior, but easily possible with concurrent
modifications to the directory), we can end up reading an unrealted,
non-dstart entry, and incorrectly reporting that entry as in our current
dir.
This fix reintroduces the did into the lfsr_dir_t struct and uses the
did to determine end-of-dir. This adds some RAM cost, but is more
resilient to any seeks that overshoot the end of the directory.
Using did is also a stronger guarantee we will never accidentally report
unrelated entries as a part of the current directory.
Also a bit of reordering lets us avoid a second mdir allocation, since
we can store the dstart mid/rid in the grm immediately. It's stack
allocated so just gets dropped if our dir turns out to not be empty.
code stack
before: 22654 2344
after: 22586 (-0.3%) 2320 (-1.0%)
There was an idea of making the necessary mid/rid adjustments to grm in
lfsr_mdir_commt implicitly.
Explored this, but:
1. It looked like the result would increase code size, though only by
a small (~12 byte) amount.
2. It wouldn't actually work, because lfr_mkdir needs to create a grm
for an mid/rid that doesn't actually exist at the time of commit.
Such a grm can't be created and survive any implicit mid/rid
adjustment.
So scratching that idea for now.
We never encode/decode the grm to/from disk and we always know the
buffer size statically.
Even when we calculate the size for the grm tag, we ignore the encoded
size and optimistically scan for the number of trailing zeros, giving us
a potentially smaller gdelta.
This change drops the encoded length completely in grm encoding/decoding
functions, assuming all related buffers are statically sized and padded
with zeros.
This also means you can't forget to zero the buffer when encoding, which
was already overlooked several times, leading to internal garbage on
disk. So that's nice.
This is entirely a pragmatic change, lfsr_mdir_commit already does
several hairy things with grm tags, decoding, fixing, reencoding, etc,
so it makes sense to move all the encoding logic into lfsr_mdir_commit.
This leads to a couple optimizations:
- We don't need to decode the grm to apply any last minute fixes.
- By allowing the grm arugment to be mutated (they are just sitting on
the stack anyways, we need a copy in case we back out of change due to
error), we can apply and save any grm fixes in the grm argument
itself.
This means we only need to fix the grm at most once, after any mtree
modifications.
Which in turn saves some code and stack cost:
code stack
before: 22930 2392
after: 22706 (-1.0%) 2344 (-2.0%)
Unfortunately the previous attempt to fix the dir seek system didn't
really work. Using a packed mid/rid integer for the offset is tempting,
but since mid/rid can change with any metadata id change in the
filesystem, dir tell offsets would become invalidated if you modified
files in unrelated directories, which isn't great and likely to catch
users by surprise.
This solution builds on the previous dir offset design, which tracks the
dstart-relative position independently from the current mid/rid in our
directory. To update this correctly when there are unrelated changes to
the filesystem, we need to know if metadata id changes are in the range
between our directories dstart and current mid/rid. This in turn means
we need to track our dstart. So our opened directories need three
separate pointers we need to update on every mdir commit:
dir->pos
|
.-------+-------.
a b c d e f g h i j k l m n o p
^ ^
| |
dir->dstart dir->mdir
This has quite a few moving parts, which I was hoping to avoid.
Fortunately we don't need a second mdir, so the RAM cost is pretty
small.
We can also drop dir->did, since the dstart mid/rid render it redundant,
which is interesting.
This is an attempt to fix issues with dir seeking in a filesystem
undergoing changes. The problem with the previous dstart-relative
position encoding is that if we deleted/created new entries outside of
our current directory, we didn't if they were inside or outside of the
current directory, so we couldn't always update our position correctly.
Instead of using a dstart-relative position, this solution crams both
the mid and rid into a single 31-bit integer. Things get a bit tight
here, so we use the current block_size as a heuristic for how many
possible rids we can ever have in a single mdir. The idea is the larger
the rid encoding needs to be, the smaller the mid encoding needs to be,
and we should, _roughly_, approach the same encoding limitation we would
have to dstart-relative position anyways.
Making some assumptions about the maximum possible number of rids in a
block gives us at most ~block_size/8 rids per mdir.
So for 4096 byte blocks (note the exact encoding is dynamic):
sbbbbbbb bbbbbbbb bbbbbbbr rrrrrrrr
^'-----------+----------''----+---'
'------------|----------------|----- sign bit (used for errors)
'----------------|----- 22-bit metadata bid
'----- 9-bit metadata rid
Note this introduced as new, significant limitation on the number of
total mdirs in the system. Normally I would be against this solution for
that reason, however if we adopt this encoding elsewhere in the system it
may improve some RAM cost and in general simplify things by being able to
store any mid in a single integer. More work needs to be done here...
This approach needs some fleshing out and has its own issues (the
offset returned by tell quickly becomes out of date if the filesystem
is modified, but is that really a problem?), but it improves over the
previous implementation by making tell always correct at that moment.
The idea here is to combine the current mtree size with the theoretical
upper bound on the number of directories in a single mdir, assuming our
block size, to give us a heuristic for did truncation that does not
require any extra state.
- Each directory needs 1 name tag, 1 did tag, and 1 dstart
- Each tag needs ~2 alts with our current compaction strategy
- Each tag/alt encodes to a minimum of 4 bytes
- We can also assume ~1/2 block utilization due to our split threshold
This gives us ~3*3*4*2 or ~72 bytes per directory at minimum, or
rounding down, ~block_size/32 directories per mdir.
This is a nice number because for common NOR flash geometry,
4096/32 = 128, so a filesystem with a single mdir encodes dids in a
single byte.
The biggest benefit though is being able to drop the mlimit state from
the lfs_t struct.
---
Unfortunately, this change revealed several bugs.
It turns out __builtin_clz in GCC is undefined at 0, which caused our
lfs_nlog2 function to return incorrect values at 1. This was causing
our dids to all collide when the mtree was inlined, which was resolved
by the linear scanning that resolves dids, but was severely limiting
what exactly our tests covered.
Now that this is fixed (with a simple if statement in lfs_nlog2,
lfs_nlog2 now always has defined behavior, even at 0), several bugs
needed fixing:
- We update the rid based on attrs in lfsr_mdir_commit before updating
the mdir. If we have multiple attrs this causes the assert on
rid-in-bounds to trigger incorrectly. Just removed that assert for now.
- We needed to adjust second grms if they are affected by the fixing
of the first grm.
- Directory position updates are incorrectly updated if an unrelated
weight change occurs before an opened directory, but is not a part of
that opened directory.
This is NOT fixed yet, the current implementation is just broken
enough that I've just ripped it out for now (it was causing the
read_with_rms test to fail because pos backed up into the "."/".."
entries).
This needs some thinking to fix.
Because of that last, unfixed bug, tests are not all passing at the
moment. To pass testing -DSEEK=0 is needed to disable the failing tests.
- Prevented removing and renaming of the root directory. This is done by
repurposing the INVAL error in lfsr_mtree_lookup to indicate the
found entry is the root.
The root entry has special behavior in almost every function, owing to
the fact it doesn't really have an mid/rid. So I think this is a
reasonable approach.
- Added support for lfsr_stat of the root directory.
- Fixed off-by-two in lfsr_dir_seek thanks to the "." and ".." entries.
Humorously there is a comment noting this but the code didn't
actually match the comment.
Mainly trying to match the tests over mkdir/rm, which seem to have a
good amount of coverage.
- Fixed issue where move's desination rid wasn't updated correctly if
the destination split.
- Prevented renaming into nonexistant directories.
- Fixed neighboring rid adjustment in rename (+1 not -1 silly).
- Fixed erronously updating the grm's rid during lfsr_fs_fixgrm. In the
"I can't believe this ever worked" category, it seems this usually
didn't cause issues since mid was often marked as removed, making the
erronously updated rid ignored.
Only simple tests right now, but the theory is sound.
This mainly required the addition of the fancy in-device move attribute,
which copies all tags associated with an rid from one rbyd to another in
a single transaction.
This is a carryover from the previous littlefs implementation, though it
is easier to implement here since it is effectively a range query on the
rbyd tree, which trees are really good at. This was intentional.
Oh and I suppose this also required implementing lfsr_rename, which has
a few corner cases to watch out for.
It is nice that both lfsr_remove and lfsr_rename can rely on
lfsr_fs_fixgrm to finish all of the removes, which wasn't previously
reasonable due to the overhead of deorphaning.
Ugh. I overlooked a weird corner case in rename's behavior that requires
changes to the grm to support.
POSIX's rename, which lfsr_rename is trying to match, supports renaming
files over existing files, effectively removing the previous file during
the rename.
This is supported, even if the files are directories, but with the
additional requirement that the previous directory is empty (matching
the behavior of lfsr_remove).
This creates a weird situation for littlefs. In order to remove
directories in littlefs, we need to atomically remove both the dstart
entry that reserves the directory's did and the directories entry in its
parent. This is made possible by using the grm to mark one entry as
pending removed while removing the other.
But in order to rename atomically, we need to use the grm to mark the
source of the rename as removed while creating/replacing the destination
of the rename.
So we end up needing two grms simultaneously.
This is extra annoying because the niche case of renaming a directory
over another empty directory is the only case where we need two grms,
but this requirement almost doubles the grm size both in-ram and
reserved in every mdir, from 11 bytes to 21 bytes, and increases the
lfs_t size by 28 bytes.
---
Anyways, this commit extends the grm to support up to two pending removes.
Fortunately the implementation was simple since we already have a type
field that can be extended, and grm operations just needed to be
changed from if statements to for loops.
Hopefully third times the charm.
The previous solution pretty bluntly did not work outside of the
recursive remove case, because the moment we mark the rid as deleted,
the directory positions no longer get updates. It's not possible to
update the directory position because we don't know how it maps into our
mtree without a full seek from the dstart.
After staring at it a bit, I think this solution should work:
1. Instead of marking the mid/rid as removed when dropping an mdir, we
set the weight to zero and the trunk to zero, causing mdir lookups to
return NOENT without actually going to disk.
This is very important since later mdirs could be allocated on the
same block, and going to disk can result in a corrupted lookup.
2. Eagerly seek to the next mid/rid after every lfsr_dir_read call. This
puts us in a position where rid can be >= the current mdir weight
without issues, and avoids degenerate cases that may be caused by
recursive removes.
3. If we remove an opened dir, instead of marking the mdir as deleted,
move the rid to the next rid. If the mdir was dropped, this leaves us
with rid == mdir weight, and the mdir trunk == 0.
The rid == mdir weight also occurs when we are creating a new file, so
we have a bit of common behavior we can rely on. We just need to make
sure that mdir updates respect the rid == mdir weight situation.
4. On each lfsr_dir_read call, we do an mtree seek of zero. This just
serves to fix our mdir if our rid == mdir weight, without much
additional code (yay for code reuse).
The use of weight=0, trunk=0, for a dropped mdir here is key, and makes
me wonder if this is a better indicator of a dropped mdir than another
reserved mid value. This probably deserves some investigation later.
Recursive removes is proving more challenging to implement than I
expected. The problem with the previous approach is that it moved the
mid into a potentially non-sensical position with the expectation it
would be updated in lfsr_dir_read because the rid overflows the current
weight (since dropping mdirs always set the weight to zero).
But if an unrelated mdir commit followed that happened to touch that
nonsense mid, the mdir would incorrectly be updated to the previous
block, causing problems for the dir's read state.
---
The solution here is to toss all of that out and rely solely on directory
position updates, which are a bit simpler.
So in lfsr_dir_read, if our mid/rid is deleted, we perform a full
rewind+seek to the new position. This can be more costly, but since the
most common case, recursive removal, leaves us with all mid/rids < pos
deleted, it should only add a single mtree lookup per lfsr_dir_read.
Also added prototypes for dir seek/tell/rewind, since we're using
they're logic for this. Though these aren't yet tested. These are built
on the new function lfsr_mtree_seek which captures the common logic of
seek over multiple mdirs in the mtree efficiently, and skips unnecessary
rid lookups where possible.
"Recursion" here just refers to the ability to remove entries in a
directory while iterating over it. This is very useful when you just
want a directory gone, and can be extended to a "true" recursive remove
straightforwardly. This mainly tests that mid/rid updates in opened
mdirs are correct.
To make this work, we need to update opened dirs differently than files,
since opened dirs do not get marked as removed when its rid is removed
and contain an additional position in the dir that needs to be updated.
To keep track of the different types, littlefs now contains 2
linked-lists for opened mdirs. Maybe these should be correctly typed,
but by hiding the specific types behind an array of mdir linked-lists,
we can more efficiently iterate over both lists when necessary.
We should probably compare this approach to the type-tagged approach in
the previous littlefs implementation, but I think the idea of an array
of type-hidden linked-lists just didn't come to me then. There was also
a bit more room in the mdir structs to hide a 1-bit type field. The mdir
structs here are getting pretty squeezed since they are used everywhere.
In theory this is pretty much the same as lfsr_mkdir, but backwards.
The main work was making the interactions between removing mids/rids and
the grm correct. This ends up meaning we just need to update the grm on
any mid/rid update the same way we update the list of opened mdirs.
On the plus side, it turned out to be possible to deduplicate the mdir
uninlining route a bit, by adding range argument to lfsr_mdir_commit_
and changing the write of the newly uninlined mtree/mdir to marking
mtree as dirty and then joining the common path.
This lets us move the pre-commit round of grm updates into a single
location in lfsr_mdir_commit, removing and extra function definition and
the related state marshalling while also simplifying the control-flow.
This also raises the question, can more lfsr_mdir_commit be deduplicated
more? Uninlining is a infrequent operation we don't really need to
optimize for.
---
Testing lfsr_remove also found a bug related to incorrect propagation of
when the mroot becomes "unerased" (when rbyd overflows). This raises the
concern that we're not propagating unerased-states very rigorously, and
unexpected errors may not allow the filesystem to resume.
This has never been in a very good place for littlefs, but would be
worth improving in the future.
Instead of truncating to exactly 28-bits for nice leb128 alignment, we
now truncate to ~the number of metadata entries, which must be >= ~2x
the number dids since each did needs a dir entry and dstart entry.
This has the downside of needing to actually keep track of an estimate
of the number of metadata entries, which is made a bit difficult due to
integer overflow issues (we can have more than 2^32 metadata entries),
but has the upside of allowing a full 2^32 number of dids worst case.
This is really unlikely, but it's nice to not need another configuration
option to control the did limit.
Another option would be to scale the hashes based on the number dids,
which would be a more direct solution. Unfortunately determining the
number of dids during mount requires a O(m*log(m)) scan of each rbyd
to find either dir entries or dstart entries. This solution can easily
end up with an overestimate, but only needs to weight of each rbyd which
can be (and already is) found in O(m).
To help with this, added TEST_PL, which is set to true when powerloss
testing. This way tests can check for stronger conditions (no EEXIST)
when not powerloss testing.
With TEST_PL, there's really no reason every test in t5_dirs shouldn't
be reentrant, and this gives us a huge improvement of test coverage very
cheaply.
---
The increased test coverage caught a bug, which is that gstate wasn't
being consumed properly when mtree uninlining. Humorously, this went
unnoticed because the most common form of mtree uninlining, mdir splitting,
ended up incorrectly consuming the gstate twice, which canceled itself
out since the consume operation is basically just xor.
Also added support for printing dstarts to dbglfs.py, to help debugging.
The grm bugs were mostly issues with:
1. Not maintaining the on-disk grm state in RAM (lfs->grm) correctly,
this needs to be updated correctly after every commit or littlefs
gets a confused.
2. lfsr_fs_fixgrm got a bit confused when it was missed when changing
the no-rm encoding from 0 to -2. Added some inline functions to help
avoid this in the future.
3. Leaking information due to mixing fixed sized and variable sized
encodings of the grm delta in places. This is a bit tricky to write
an assert for as we don't parse the full grm when we see a no-rm grm.
This implementation is in theory correct, but of course, being untested,
who knows?
Though this does come with remounting added to all of the directory
tests. This effectively tests that all of the directory creation tests
we have so far maintain grm=0 after each unmount-mount cycle. Which is
valuable.
This has, in theory, global-removes (grm) being written out as a part of
of directory creation, but they aren't used in any form and so may not
be being written correctly.
But it did require quite a bit of problem solving to get to this point
(the interactions between mtree splitsand grms is really annoying), so
it's worth a commit.
This bug was just overlooked in testing the mtree, fortunately dir
fuzzing found it. Though since this depends on neighboring mdirs, it
probably would have been found quicker with smaller block sizes. At the
moment I am only testing on NOR-liked geometry (4KiB blocks).
The fix is easy, we can use the difference in the mtree size to
determine if a split or drop happened in mdir commit, since at most one
of these can happen on any mdir commit.
Also added an explicit test for mid updates when splitting and dropping.
lfsr_stat is really a directory operation underneath, so it's good to
add to our testing while we are building up the dir tests.
It's interesting to note lfsr_stat and lfsr_dir_read are less
deduplicatable than their previous versions, since lfsr_stat can get
most of it's info from lfsr_mtree_pathlookup. Though there will probably
need to be some code sharing when we get to files with sizes.
Christopher Haster