This is an interesting optimization made possible by our attr-lists now
only operating on one mid. We can now discard the entire attr-list based
on if that mid is in the commit's filter range.
Unfortunately, while I had hoped this would lead to more
simplifications, we can't really push this up through many functions:
- While this eager discard works for splits, lfsr_btree_commit can also
merge, which affects two separate bids. The attrs on these bids need
to be split over the new btree inner-nodes, so we end up still needing
filtering in lfsr_rbyd_appendattrs.
We could move the filtering up into lfsr_btree_commit, but would that
really gain anything?
- lfsr_mdir_commit_ needs the filter range because we leverage this in
higher-layers to for lfsr_mdir_commit_ to omit non--1 attrs when
committing to newly hollow mroots.
In theory it might still be possible to push this up into
lfsr_mdir_commit, but we would still need to unconditionally commit
during mdir splits to append the cksum. This ends up with duplicate
function calls which ends up annoyingly expensive. Though maybe there
is a conditional count trick that could avoid this?
At least the code changes are ok:
code stack
before: 33884 2896
after: 33808 (-0.2%) 2896 (+0.0%)
This unfortunately makes inserts inconsistent between rbyd/btrees:
insert(rid=-1) => rid=0
insert(bid=-1) => bid=0
insert(mid=0) => mid=0
But seems to integrate the best throughout the rest of the codebase:
- No awkward rid=-1 encoding in the mid, mid=1.2 => bid=1, rid=2
- No need to tweak mid encoding when writing grms to disk
- Behavior of unrelated files in the mdir behave consistently
irregardless of if our tag is an insert or not:
- mid' >= mid => mid'=mid'+delta
- mid' < mid => mid'=mid'
This is convenient because only the mid updates trigger tweaks of
unrelated mids, rbyds/btrees don't really have this problem.
- We already have to do a bit of tweaking in lfsr_mdir_namelookup, since
we're converting from "buckets" in the rbyd to ids we'd insert into.
Mainly namelookup of left-most name returns rid/bid=0, but for mdirs
should return mid=-1 (now mid=0):
left-most left-most+1 left-most+2
rid/bid: 0 0 1
mid (before): -1 0 1
mid (after): 0 1 2
I think this may be a reasonable compromise between allowing splits in
rbyd/btrees, and intuitive behavior for insertions in the mdirs.
That, or I've just been staring at this code for too long...
code stack
before: 33876 2896
after: 33888 (+0.0%) 2896 (+0.0%)
The recent change to internally track mids as mid=mid+1 leaked onto disk
through the grm. This is currently the only place we actually write mids
to disk.
The mid=mid+1 encoding is a bit of a hack and probably should not be the
actual on-disk representation, since there are other ways to encode this
internally.
I did try to write some tests for this, but because the bug is on both
the encoding and decoding side it's difficult without reading the mdir
directly. I only noticed with the dbg scripts started throwing random
errors. Fortunately a regression here is unlikely.
This atypical but not unreasonable behavior (most array insert functions
I've ran into like to insert _before_ the current index) makes split
commits no longer special behavior of appendattrs/commit, and seems to
fit better into rbyd append logic (though admittedly, some of the rbyd
append logic gets really weird with the whole right-leaning business).
Though this does come with a couple downsides:
- All rbyd-based data structures need to be able to represent a -1 id
so we can insert into the first id. This is not a problems for
rids/bids, but we need to tweak mids to support mid.rid=-1.
The best solution I could come up with was to just increment rid by
one, so, assuming mbits=8:
- mid=0x100 => bid=0x100, rid=-1
- mid=0x101 => bid=0x100, rid=0
- mid=-1 => bid=-1, rid=-1
- We need to be really careful with splits over our attr-list, since
these can line up between the rid create tags reference and other
following tags intended to stick to the new rid.
This required some special handling in lfsr_rbyd_appendattrs and
lfsr_mdir_commit__.
Other than that this change is quite promising, and removed what felt
like a bunch of hacks adjusting mids in lfsr_file_carve.
code stack
before: 33992 2904
after: 33868 (-0.4%) 2896 (-0.3%)
It turns out we never really need to commit to two unrelated rids in a
single commit. And some data structures, mainly btrees/bshrubs, don't
even allow commits to unrelated rids.
Well, sort of. There are some cases that seem to require unrelated rids,
but these are easy enough to work around:
1. btree/mdir splits/merges end up with two rids - but these either
converge or diverge from one rid, so as long as we assume sequential
inserts/deletes operate on the _neighboring_ rid, things work out.
2. grms/etc commit to mid=-1 irregardless of the file mid - but these
are also very special flags that are already handled differently to
manage the global state updates, nothing new was needed here.
So, in theory, we can move the rids out of the lfsr_attr_t struct and
infer and rid changes as we play out the attr-list, saving 4 bytes
(~17%) from every attr we allocate on the stack.
As a plus, we remove the need to manually calculate the changes to the
rid in the attr-list, reducing the likelihood of bugs here and saving a
decent amount of code.
Unfortunately the code/stack savings from this change were a bit
disappointing. The extra rid parameter in every commit function added
quite a bit of overhead, and we have to do some funky memmoves in
lfsr_file_carve to account for the new strict attr-list order:
code stack lfsr_attr_t
before: 33924 2912 24
after: 33992 (+0.2%) 2904 (-0.3%) 20 (-16.7%)
Still, this decreases the amount of code that can contain bugs, and more
closely matches the actual behavior of lfsr_btree/bshrub_commit.
Someone should really get around to updating the rbyd/btree/mtree
tests... Well, at least the non-internal (dir/dread/file/fwrite/etc)
tests are working.
Unreachable tag holes, null tags that _should_ be unreachable but
actually are reachable, are an unfortunate quirk to our alt tag
encoding. Because we only have an altgt, not altge, our "unreachable"
tag ends up encoded with an altgt 0, an alt, which you may notice, does
not guarantee unreachability.
Fortunately, tag 0, the null tag, should intentionally be unused. So as
long as we never lookup tag 0, nothing should break.
If you do lookup tag 0, you end up with spurious null tags, which can
complicate things.
The solution here is a tag_ = max(tag, 1) in lfsr_rbyd_lookupnext.
---
One interesting thing to note, as I was writing these tests I discovered
that setting tag=max(tag,1) in lfsr_rbyd_appendattr had no effect.
appendattr needs zip the rbyd tree to keep everything connected during
range removals, so tag=0/tag=1 both end up with the same tree.
So might as well drop the tag=max(tag,1) in lfsr_rbyd_appendattr.
A side effect of this, both before and after this commit, is that any
null tag holes created during range removals sort of stick around until
the next compaction.
---
Why altgt and not altge? altgt is the inverse of altle, requiring only
a single bit flip to flip between the two. And trust me, it would be
much more costly to make altle/altgt flips more complicated than a bit
flip.
---
Why altgt/altle and not altge/altlt? This is because our rbyds are
right-leaning, that is, lookups always find the requested rid+tag, or
the next smallest rid+tag.
Consider a simple tree:
<5
.----'|
>=2 |
|'-. |
1 2 5
What should lookup(3) return? If we are right-leaning, the answer
_should_ be 5. But we need to take the <5 branch to determine if there
is a hidden 3 or 4 in that subtree.
altgt/altle does not have that problem:
<=2
.----'|
>1 |
|'-. |
1 2 5
It might seem like you can workaround this by conservatively using the
neighbor +1 as the alt target, but this runs into tag overflow problems.
UATTR(0xff)+1 (0x057f+1) becomes UATTR(0x100) (0x0580) which is not
allowed due to reserving bit 7 for future subtype extensions.
Maybe you can workaround this workaround by using (tag+0x81)&~0x80
anywhere you need to increment (including lookupnext/iteration calls!),
but this becomes a bit of a mess. And there are still concerns about
overflows at the 0x77f boundary and 0xf7f boundary.
Like SUBWIDE, SUPWIDE allows for "mask-like" operation during rbyd
commits, where you replace an entire subrange of tags with a single tag.
- SUBWIDE - Replace all subtypes of the given suptype - Useful for
changing the subtype of an attr, for example replacing a BTREE with a
BSHRUB.
- SUPWIDE - Replace all suptypes of the given rid - Useful for changing
the suptype of an attr, for example replacing a REG file with an
ORPHAN file.
These are effectively the same modifier, just with different ranges.
One benefit is this simplifies mid-level operations a bit, rename,
remove, etc, and decreases the stack cost of the related attr lists.
Though this isn't on the hot-path, so not measurable:
code stack
before: 33956 2912
after: 33928 (-0.1%) 2912 (+0.0%)
But the real motivation for this change is to remove cases where
lfsr_mdir_commit needs to operate on multiple mids. There may be an API
simplification here.
We still have an lfsr_shrub_t, it's just a simple alias of lfsr_rbyd_t.
The only difference between these two structs was that lfsr_rbyd_t had
the eoff/cksum fields, to enable incremental commits, and lfsr_shrub_t
had the estimate field, to keep track of the current shrub estimate so
we evict before overflow.
Unfortunately C makes this overlap a bit annoying. We can either add a
union, making a mess of field accesses, or use probably problematic
casting of structs with common initial sequences.
Instead of dealing with this headache, I'm just going to shove the
shrub estimate into the rbyd's eoff field and ignore the name abuse.
In normal rbyd use, eoff does effectively contain the on-disk size of
the rbyd, so it's not too far from its intended use...
This does move our estimate to overlap the eoff field instead of the
cksum field, which means we need to be a bit more careful about setting
erased state for btrees. This adds a small code cost:
code stack
before: 33928 2912
after: 33956 (+0.1%) 2912 (+0.0%)
Shrubness should have always been a property of lfsr_rbyd_t.
You know you've made a good design decision when things just sort of
fall into place and the code somehow becomes cleaner.
The downside of this change is accessing rbyd trunks requires a mask,
which is annoying, but the upside is we don't need to signal shrubness
via extra booleans in internal functions anymore.
The funny thing is, the actual motivation for this change is was just to
free up a bit in our tag encoding. Simplifying some of the internal
functions was just a nice side effect.
code stack
before: 33940 2928
after: 33928 (-0.0%) 2912 (-0.5%)
So:
switch (cond) {
case 0:;
// first case
break;
case 1:;
// second case
break;
default:;
// default case
break;
}
This basically adopts our current label style for the case statements in
switch statements. It initially looks like quite a monstrosity, but I
think it does a good job at highlighting that case statements in C are
no safer than labels and gotos.
I would not use this style in a language with better scoping in switch
statements.
I'd prefer not to use switch statements, their scoping rules in C are
just too error-prone, and the compiler usually optimizes things out
anyways, but there are some places where switch statements are clearly
the correct organization -- state machines such as lfsr_traversal_read
for example.
If you're curious about the ':;' ending, this is used in our current
style for labels to avoid "declaration is not a statement" warnings.
Which I think is just a bit of leftover from C historically not having
mixed statements/declarations.
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.
If it's convenient, _and_ saves a bit of code, I don't really see the
reasoning to drop it.
At least added an LFSR_ATTR_IF macro to try to reduce the C expression
noise.
Also I'm keeping the explicit noop grows in lfsr_btree_commit. I think
it's a good decision to prevent accidental non-nooping in the future. If
these grows didn't noop, it wouldn't be an error, but a minor parasitic
drain on performance/wear. Not great...
code stack
before: 34052 2928
after: 33940 (-0.3%) 2928 (+0.0%)
So instead of using C's ternary operator everywhere:
(condition)
? LFSR_ATTR(rid, tag, delta, data)
: LFSR_ATTR_NOOP
Use incremental attr allocation instead:
lfsr_attr_t attrs[1];
lfs_size_t attr_count = 0;
if (condition) {
attrs[attr_count++] = LFSR_ATTR(rid, tag, delta, data);
}
LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t));
Incremental attr allocation is more flexible, allowing nested conditions
and conditions that span multiple attrs without sacrificing readability,
though at a verbosity cost.
We already need this for lfsr_btree_commit and lfsr_file_carve, adopting
it everywhere we need conditional attrs allows us to drop the noop attr
and avoid messy and hard-to-read C expressions.
This also changes the lfsr_btree_commit to explicitly omit noop grows.
We were relying on lfsr_rbyd_appendattr implicitly skipping these to
avoid unnecessary attr commits, but I think it's probably better to make
these noops explicit.
This does add some code cost though, I'm guessing sequential conditional
attrs landing at different offsets complicates code generation a bit:
code stack
before: 33940 2928
after: 34052 (+0.3%) 2928 (+0.0%)
Unfortunately we can't apply the valuable overflow assertions we use
elsewhere, classic C array arguments devolving into pointers before
sizeof. It's unfortunate there's no way to get this information even
for static-sized array arguments.
code stack
before: 33976 2944
after: 33940 (-0.1%) 2928 (-0.5%)
This is just too useful a namespace to not have in the low-level file
code.
This also replaces the ftree namespace with bshrub, which is a bit of a
more concrete term?
Note that some of the bshrub functions still take lfsr_file_t instead
of lfsr_bshrub_t. In _theory_ these could take 3 pointers (mdir+bshrub+
bshrub_), but this adds a surprising amount of code cost and we really
don't gain anything.
- Rely on rbyd lookup ENOENT now that that is cheap
- Adopt internal traverse_ function with shrub bool to indicate if root
should be included, similar to btree_commit_
Code changes:
code stack
before: 33956 2944
after: 33976 (+0.1%) 2944 (+0.0%)
We really shouldn't go to disk in cases like these, it's not worth the
code tradeoff. This led to concious decisions to avoid out-of-bound
lookups in higher-layers, which sort of defeats any benefit of this
potential optimization.
code stack
before: 33948 2944
after: 33956 (+0.0%) 2944 (+0.0%)
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.
Since we need these for lfsr_dir_t (named b and p), we might as well
adopt one in lfsr_file_t (named m). This at least avoids a cast when
enrolling/unenrolling in the opened-mdir list.
This is just too enticing a simplification to avoid, even at a RAM cost.
By tracking the dir's bookmark as a separate mdir, we can trivially
deduplicate the logic to update dirs' mdirs. But this does significantly
increase the size of our lfsr_dir_t with the bookmark's type/flags/rbyd/
etc, which usually goes unused.
I guess this does optimize lfsr_dir_rewind... Is that ever a bottleneck?
Code changes:
code stack lfsr_dir_t
before: 34048 2944 48
after: 33956 (-0.3%) 2944 (+0.0%) 80 (+66.7%)
Long story short, lfsr_mkdir needs to atomically create two somewhat
arbitrary mid entries: the bookmark and the dir entry. We can't, so we
fake it with our grm:
1. Create bookmark mid, set grm to delete bookmark mid
2. Create dir mid, zeroing grm
This works well enough, with the _small_ (read not small) caveat that
creating the bookmark mid can indirectly change the dir mid in
surprising ways.
The original plan was to take advantage of our opened-mdir tracking to
track an on-stack mdir pointing to where the dir mid should go. This
gets tricky, because, by definition of not being created yet, the dir
mid doesn't actually have an mid we can track. But this shouldn't be any
issue if we track the mid _after_ where we should insert right?
Wrong.
The issue is mdir splits. Which are especially nefarious because the
vast majority of the time mdirs don't split. And the split has to happen
_exactly_ between the tracked mid and where we would have inserted. Rare,
but possible. Which is why this went undetected for so long.
Consider this example of creating dir c:
.---------------.
|bmk|reg|reg|reg|
| a | b | d | e |
'---------------'
^
'-- track reg d (to insert dir c)
First insert the did, but oh no! a split occured!
.-------.
|mdr|mdr|
| | d |
'-|---|-'
.---' '---.
v v
.-------. .-------.
|bmk|reg| |reg|reg|
| a | b | | d | e |
'-------' '-------'
^
'-- track reg d (to insert dir c)
Now insert the dir c before reg d:
.-------.
|mdr|mdr|
| | d |<--------------------.
'-|---|-' |
.---' '-----. ???????????????????
v v dir c unreachable?
.-------. .-----------. ???????????????????
|bmk|reg| |dir|reg|reg| |
| a | b | | c | d | e | |
'-------' '-----------' |
^ |
'---------------------'
Problem, dir c became unreasonable.
Some options:
1. I _think_ things work if you track the mid _before_ where we want to
insert, iff we track the dir mid.
1. Our parent directory always at least contains a bookmark mid, so
it's not possible for our bookmark to be inserted between the mid
_before_ our dir mid and our dir mid.
This is possible if we track the bookmark mid, and would break
things.
2. mdir splits use the first name in the right mdir, so even if we
split on the mid immediately after the mid _before_ our dir mid,
inserting the dir mid should not make it unreachable.
But, as experience has shown, this whole tracking thing is very
fragile.
2. Create an orphan file and track that, replacing it atomically after
we've created our bookmark.
This works, but trades read overhead for write overhead. Even though
it's O(log^2 n) reads vs O(1) writes, write overhead always takes
priority and should be minimized. Flash is destructive after all.
3. Rip our the on-stack mdir tracking and just do an additional name
lookup after the bookmark is created.
mdir tracking, while clever, has created enough difficult to find bugs
and enough headache that I think its time is up. This commit implements
option 3.
Besides, it's not like lfsr_mkdir is really a performance sensitive
function anyways...
Code changes:
code stack
before: 34036 2944
after: 34048 (+0.0%) 2944 (+0.0%)
Much like the lfsr_o_* functions, I think we should avoid too many
convenience layers for what really are operations on struct fields.
Otherwise you quickly end up with a lot of boilerplate that just saves a
couple extra characters at invocation. Characters that also help convey
what is being accessed.
This avoids implicit info, mid.mid=-1 implying a bad path, and mid.mid=0
implying the root directory, at a tradeoff of potentially making the
returned error codes a bit confusing (0 means the file is NOT found!).
Here are the now possible return codes, aside from lower-level errors
(IO, CORRUPT, etc):
- 0 => path is valid, file NOT found
- EXIST => path is valid, file found
- INVAL => path is valid, but points to root
- NOENT => path is NOT valid, intermediate dir missing
- NOTDIR => path is NOT valid, intermediate dir is not a dir
Since the root has no real mdir entry, I think the special INVAL return
code is warranted. It needs special behavior in relevant functions
anyways.
Note that orphaned files still need special handling.
Code changes:
code stack
before: 33944 2944
after: 34036 (+0.3%) 2944 (+0.0%)
By moving this to such a low-level, this ensures consistent reporting of
grms without needed special cases in every high-level function.
The use of the orphan type is very convenient here, as it avoids a whole
category of potential bugs around mismatched mids<->mdir weights.
Also added lfsr_grm_isrm, though ironically it has less use after these
changes.
This required duplication of some rbyd/mdir lookup convenience
functions, so unfortunately it's not a win code-wise, but I think the
resilience to future bugs is worth it.
Heck, it already revealed one minor bug in the return value of
lfsr_mtree_pathlookup when an intermediary path name is grmed/orphaned:
code stack
before: 33812 2944
after: 33944 (+0.4%) 2944 (+0.0%)
This effectively reverts lazy mdir drops, which were needed to handle
orphaned mdirs.
Now that we orphan at the mid level, not the mdir level, mdir orphans
are never created. And it shouldn't be too much of a burden to prohibit
orphaned mdir in the future.
Now only the mroot is allowed to be an empty mdir, though mdirs may
contain only orphaned files.
This saves some code. To be honest I was hoping for more, but I think
the new mdir commit organization prevents aggressive function inlining.
Though I'm not sure code savings warrant the early, difficult to
understand code structure:
code stack
before: 33948 2944
after: 33816 (-0.4%) 2944 (+0.0%)
We were just missing a check here to make sure orphaned files aren't
open in-device (these aren't really orphaned because we still have a
reference).
This can't happen during mount, but can happen if fixorphans is
triggered because of an orphaned/zombied file.
Also added a test over this case to prevent regression.
Actually the test was harder to implement than the fix.
I was originally avoiding naming these orphans, as they're _technically_
not orphans. They do exist in the mtree. But the name orphan just
describes this types purpose too well.
This does lead to some confusing terms, such as the fact that orphan
files can be non-orphaned if there are any in-device references. But I
think this makes sense?
- LFSR_TAG_SCRATCH -> LFSR_TAG_ORPHAN
- LFSR_F_UNCREAT -> LFSR_F_ORPHAN
- test_fscratch.toml -> test_forphan.toml
- Fixed fsync tests, which needed more lfsr_file_sync calls so multiple
file handles can be opened correctly.
Though this points out there's no way to open a rdonly file on an
uncreated file until sync is called... But I guess you wouldn't be
able to recieve broadcasts until sync anyways? at which point the file
would be created?
- Update mtree tests based on the new remove behavior for regular files.
Before this changed the mid to -1, now it points to the next mid with
the zombie flag set. Upper layers use this to migrate mdirs to a
scratch file if necessary.
- Removed the orphaned mdir test. We don't create orphaned mdirs
anymore.
Technically, orphaned mdirs are currently possible if we lose power in
the middle of the mtree update, but this is a bug and should be fixed
(previous revisions did not have this issue).
With this I think it's safe to say file renaming is decently tested.
The increased range of sizes means we should be testing a good range of
sprout/shrub/btree file structs.
This requires two things:
1. Any opened file handles need to have their mid/mdir updated after the
rename succeeds.
2. Any shrubs/sprouts need to be copied over to the new mdir, even if
they aren't in-tree.
The LFSR_TAG_MOVE operation is starting to look an awfully lot like
lfsr_mdir_compact... Unfortunately lfsr_mdir_compact, uh, compacts,
whereas LFSR_TAG_MOVE appends to the rbyd like normal, so it's not clear
exactly _how_ to deduplicate.
A "zombie file" is a term I just made up to describe what happens when
you remove a file that is currently open.
To match POSIX, the opened file handle should still be available for
reading/writing, even though the file doesn't really exist in the
filesystem anymore.
We don't have inodes, which makes this a bit more complicated, but this
is where scratch files are handy again. By creating a scratch file when
we remove an opened file, we preserve the mid slot for the file's
sprout/shrub. We also mark the opened file as desync, so the existing
orphan reclaimation circuitry kicks in when the last file handle is
closed.
Really the only difference between zombie files and desync files is what
happens when you call lfsr_file_sync:
- Desynced lfsr_file_sync => Become synced, broadcast file state.
- Zombied lfsr_file_sync => Return ENOENT, you can't sync a zombie.
This _is_ a bit different from POSIX, where sync on a removed file
returns 0. I considered returning 0 in this case, but with all the extra
behavior around sync/desync state, I figured returning ENOENT was
clearer at indicating to the user sync is no longer possible.
Worst case, ENOENT is not returned from sync for any other reason, so
users can always treat ENOENT and 0 as the same in higher layers. The
zombie file is already desynced, so close will never error.
---
Implementation wise, zombies get a bit crazy.
Fortunately they add little extra code, but they make up for it by
adding extra subtlety. Zombie files introduce a ton of corner cases, now
even directories can have zombied shrubs.
This means more tests.
- Seemingly unrelated operations need to be able to remove scratch files
(mkdir, rename, etc).
- UNCREAT state needs to be broadcasted in seemingly unrelated
operations (mkdir, rename, etc).
- Zombied files need to be copied over during seemingly unrelated rename
operations.
- And I'm sure more corner cases I'm already forgetting.
One interesting tweak that simplifies things that's worth mentioning is
the change to the implicitly file mid updates on rm in lfsr_mdir_commit.
For non-reg files, an rm attr causes lfsr_mdir_commit to increment the
mid to the next mid in the mtree. This is the correct behavior for dirs,
traversals, etc.
Previously, reg files were a special case that marks the mid as -1. But
by changing this to also increment the mid, as well as set the zombie
flag, upper layers can broadcast zombie changes by simply creating a new
file and then deleting the old file in the same commit.
This seems to Just Work^TM, and avoids needing to do additional state
broadcasting in upper layers, which gets tricky since we may not know
exactly what the new mid is post-mdir-commit.
Downside: The order matters, we need to create the new file first. This
violates the normal delete-then-insert order we use elsewhere to avoid
overflow issues. This isn't that bad here, since we increment by at
most 1. But it is something to be wary of...
Still, this is much better than any other option I can think of right
now.
---
Uh, ignore the test_fscratch_rename* tests for now. I somehow forgot
file renaming was not yet implemented...
This is a fun corner case. What happens when you close a desynced
scratch file?
The obvious answer seems to be just remove the scratch file in
lfsr_file_close.
But then what if the file is rdonly? desynced because of an error?
We really shouldn't write to disk at all when closing a desync or rdonly
file. This needs to be a hard rule.
So the only option is to defer the work until later somehow.
Fortunately, we already have several mechanisms that lead to a very nice
solution. I'm very happy with this:
1. There's nothing that says our in-device grm queue needs to always
match what's on-disk (we need a separate copy for xoring anyways
because of the risk of leb128 encoding differences). So if we have
<=2 orphans, we can just push these onto our grm.
On the next write operation, the normal grm fixing code takes over
and removes the pending orphans O(1).
2. If we have >2 orphans, the best we can do is mark the filesystem as
having orphans, and trigger an orphan scan on the next write
operation O(nlogn).
But how often do you think littlefs's use cases will end up with >2
orphans?
Note we also need to scan the opened-file list to make sure we're the
_last_ reference to the scratch file. Otherwise we corrupt other opened
file handles!
---
This commit also includes a fix for a bug where the traversal mdir fell
out of sync when dropping mdirs as a part of scratch file cleanup. Found
when adding more tests, this would cause scratch files to go
unreclaimed.
"Scratch files" are a new file type added to solve the zero-sized
file problem. Though they have a few other uses that may be quite
valuable.
The "zero-sized file problem" is a common surprise for users, where what
seems like a simple file create+write operation:
lfs_file_open(&lfs, &file, "hi",
LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL);
lfs_file_write(&lfs, &file, "hello!", strlen("hello!"));
lfs_file_close(&lfs, &file);
Can end up create a zero-sized file under powerloss, breaking user
assumptions and their code.
The tricky thing is that this is actually correct behavior as defined by
POSIX. `open` with O_CREAT creats a file entry immediately, which is
initially zero-sized. And the fact that power can be lost between `open`
and `close` isn't really avoidable.
But this is a common enough footgun that it's probably worth deviating
from POSIX here.
But how to avoid zero-sized files exactly? First thought: Delay the file
creation until sync/close, tracking uncreated files in-device until
then. This solves the problem and avoids any intermediary state if we
lose power, but came with a number of headaches:
1. Since we delay file creation, we don't immediately write the filename
to disk on open. This implies we need to keep the filename allocated
in RAM until the first sync/close call.
The requirement to keep the filename allocated for new files until
first sync/close could be added to open, and with the option to call
sync immediately to save the filename (and accept the risk of
zero-sized files), I don't think it would be _that_ bad of an API.
But it would still be pretty bad. Extra bad because 1. there's no
way to warn on misuse at compile-time, 2. use-after-free bugs have a
tendency to go unnoticed annoyingly often, 3. it's a regression from
the previous API, and 4. who the heck reads the more-or-less same
`open` documentation for every filesystem they adopt.
2. Without an allocated mid, tracking files internally gets a lot
harder. The best option I could think of was to keep the opened-file
linked-list sorted by mid + (in-device) file name.
This did not feel like a great solutiona and was going to add more
code cost.
3. Handling mdir splits containing uncreated files adds another
headache. Complicated lfsr_mdir_estimate further as it needs to
decide in which mdir the uncreated files will end up, and potentially
split on a filename that isn't even created yet.
4. Since the number of uncreated files can be potentially unbounded, you
can't prevent an mdir from filling up with only uncreated files. On
disk this ends up looking like an "empty" mdir, which need specially
handling in littlefs to reclaim after powerloss.
Support for empty mdirs -- the orphaned mdir scan -- was already
added earlier. We already scan each mdir to build gstate, so it
doesn't really add much cost.
Notice that last bullet point? We already scan each mdir during mount.
Why not, instead of scanning for orphaned mdirs, scan for orphaned
files?
So this leads to the idea of "scratch files". Instead of actually
delaying file creation, fake it. Create a scratch file during open, and
on the first sync/close, convert it to a regular file. If we lose power,
scan for scratch files during mount, and remove them on first write.
Some tradeoffs:
1. The orphan scan for scratch files is a bit more expensive than for
mdirs on storage with large block sizes. We need to look at each file
entry vs just each mdir, which pushed the runtime up to O(BlogB) vs
O(B).
Though if you also consider large mtrees, the worst case is still
O(nlogn).
2. Creating intermediate scratch files adds another commit to file
creation.
This is probably not a big issue for flash, but may be more of a
concern on devices with large prog sizes.
3. Scratch files complicate unrelated mkdir/rename/etc code a bit, since
we need to consider what happens when the dest is a scratch file.
But the end result is simple. And simple is good. Both for
implementation headaches, and code size. Even if the on-disk state is
conceptually more complicated.
You may have noticed these scratch files are basically isomorphic to
just setting an "uncreated" flag on the file, and that's true. There may
have been a simpler route to end up with the design, but hey, as long as
it works.
As a plus, scratch files present a solution for a couple other things:
1. Removing an open file can become a scratch file until closed.
2. Scratch files can be used as temporary files. Open a file with
O_DESYNC and never call sync and you have yourself a temporary file.
Maybe in the future we should add O_TMPFILE to avoid the need for
unique filenames, but that is low priority.
Revealed by more correct error reporting in lfsr_file_sync, we need to
not just return errors in lfsr_file_close before we release the file
resources/remove from opened linked-list.
This should, in theory, be a transparent change for users
(https://xkcd.com/1172).
The motivation for this change:
1. Basically everyone uses O_RDONLY=0, O_WRONLY=1, O_RDWR=2, so
deviating from this ad-hoc standard risks surprising POSIX-familiar
users, though may confused POSIX-unfamiliar users.
But for the latter, we really shouldn't allow them to fall into the
trap that O_RDONLY | O_WRONLY == O_RDWR, because this will not work
on basically any other POSIX-like system.
2. I realized one benefit of the POSIX encoding is that it reserves the
value 3. Maybe this could be useful in the future?
Being able to create a file that neither readable nor writable isn't
all that useful...
Also, if you really think about the literal meaning of O_RDONLY |
O_WRONLY, these are negations. So O_RDONLY | O_WRONLY means you can only
write and only read? That sounds like an oxymoron.
Of course no one should be relying on these exact values, but these are
embedded systems! Someone somewhere is going to hack something together
that expects these to be their historically expected value. And we
shouldn't make things any harder for them unless there's a good reason.
This is a compromise on consistency and not breaking expected
invariants.
The problem: rdonly files can become unsynced:
1. file is opened rdonly + desync
2. the same file is opened and written to
3. we try to sync our original file handle
What we want:
1. sync should ensure disk + files are in-sync
2. rdonly implies sync should not write to disk
Without desync, and in other systems, this is not a problem, because
rdonly files can never become unsynced.
But with desync, a state (albiet a roundabout one) can be reached where
we can't satisfy both of these invariants.
I wanted to just assert on syncing a rdonly file, but this is supported
on POSIX and other systems, and it makes sense that you would want to
unconditionally call sync in certain circumstances (ensuring close can't
write to disk for example).
So adopts the approach of allowing flush and sync on rdonly files when
possible, and when not possible, sync simply returns LFS_ERR_INVAL and
makes it the user's problem.
For the above example, this has the side effect of making the rdonly file
desync again, so close can complete without touching disk.
As a plus, a desynced rdonly file can now be used to test if a file has
been written to. Though I'm not sure when this would be useful... Or
if it's a good idea to suggest this use of the API...
Reading more into POSIX, it seems that most of the write functions do
have special behavior built into what would implicitly be a noop.
It's difficult to find, since it usually doesn't matter, but consider
the m_time field. The following operations do _not_ update m_time:
- write when size=0
- truncate when size does not change
- fruncate when size does not change
I think it's safe to extend these to sync broadcasts in littlefs, and
only guarantee sync broadcasts when the file state has changed (even
though that may mean other file handles may remain out-of-date!).
In this interpretation, the "write operations" described in POSIX more
mean the implicit write operations effected by write/truncate/fruncate.
That being said, it's not clear what the best approach is, desync files
make this all a bit more muddled... This may also be reverted.
This is an extension of the noop-sync after unrelated write-sync after
desync corner case:
op a state b state
in-sync in-sync
desync(b) in-sync desync
write(a) unsync desync
sync(a) in-sync' desync
sync(b) in-sync in-sync
But instead of explicitly calling lfsr_file_sync, what if you implicitly
triggered sync through something like a write on a file with the
LFS_O_SYNC flag, but not a normal write, a noop write, write(0)?
If the definition of LFS_O_SYNC is taken literally as "lfsr_file_write
and friends implicitly call lfsr_file_sync after every call", then this
should behave just as if lfsr_file_sync had been called, and
unconditionally broadcast the sync. Since this is the simplest
interpretation, I think this is what we should implement.
Added tests, and adopted this behavior. Fortunately this just involves
some small gotos (https://xkcd.com/292):
code stack
before: 33020 2976
after: 33026 (+0.0%) 2976 (+0.0%)
There are a number of nuanced cases to watch out for when mixing sync,
desync, and "noop syncs" (sync when no write operation has occured):
1. Noop-sync after unrelated write:
op a state b state
in-sync in-sync
write(a) unsync in-sync
sync(b) in-sync in-sync
In this case, a should be clobbered by b when b syncs. But this
gets tricky since b is still up to date with the disk, so b's
unsynced flag is not set.
The solution here is to just unconditionally broadcast all sync
operations irregardless of on-disk state. This is all in-device
anyways, so it shouldn't really add any overhead.
2. Noop-sync after unrelated write-sync after desync:
op a state b state
in-sync in-sync
desync(b) in-sync desync
write(a) unsync desync
sync(a) in-sync' desync
sync(b) in-sync in-sync
In this case, a should again be clobbered by b, even though a is
in-sync with the disk. This is not tricky because of a's state, but
because b doesn't know it is no longer in-sync with the disk.
The solution here is to set the unsynced flag on all desynced files
when an unrelated file is synced. This way, b knows it needs to
update disk if sync is called. We already scan all opened files to
update in-sync files, so this has very little cost.
3. Readonly-sync after unrelated write-sync after desync?
This is basically the same as 2., but involves a readonly file:
op a state b state (rdonly)
in-sync in-sync
desync(b) in-sync desync
write(a) unsync desync
sync(a) in-sync' desync
sync(b) ??? in-sync
In this case, I have no idea what should happen.
I would guess the least surprising result would be for b to write
its contents to a/disk? Bringing everything in-sync?
But this implies that b, a readonly file, should write to disk.
This isn't the only place a read operation would result in a write.
RDWR files, for example, can flush buffers during a file read. But at
least there, the file is open RDWR, not strictly RDONLY.
It seems like writing during sync on a readonly file breaks some sort
of invariant users expect.
But the alternative: Dropping the current state of b in favor of a's
state, is inconsistent with sync on WRONLY/RDWR files, and seems like
it breaks some sort of invariant about sync modifying the current
file's state...
Given this situation, I think the best course of action is to just
disallow sync on readonly files. It is now an assert.
There is some precedent for this, upstream we already omit sync when
compiled in LFS_READONLY mode. Though this does deviate from POSIX
behavior...
Worst case, by asserting, this leaves us free to introduce different
readonly-sync behavior in the future without breaking backwards
compatibility.
---
Maybe there should be some sort of lfsr_file_resync function to
discard current changes? Though this can be done with a close+open
cycle, so I think the value would be low.
Added tests over these cases and fixed where they broke, except for 3.,
lfsr_file_sync and lfsr_file_flush get asserts now to prevent their use
on readonly files.
Also added a couple more specific tests to cover cases I was concerned
about.
This should avoid confusion between "multiple handles" and "multiple
files" (name undecided) test suites.
It also fits well because this suite really is just testing nuanced
sync/desync behavior.
While the multi per-type linked-lists were cool and could save RAM in
some structs (at the cost of RAM in the lfs_t struct), this is simpler,
and simpler is good.
The motivation to revert:
1. I noticed most file types have some sort of flags: files,
traversals (future), (not dirs but maybe in the future). These flags
can be merged with the type field to give us typed mdirs at almost
no RAM cost.
2. Using a single linked-list makes it cheaper to add more file types,
which may be useful for managing bookmarks (differently) and scratch
files.
This comes at a runtime cost, since all scans look at all opened
structs, but we really, _really_ don't care about a constant non-IO
runtime cost.
There are code benefits, since we don't need nested iterators to access
all opened mdirs, but also some code cost when we want to filter by
type. As expected stack took a small hit. Humorously, the struct savings
in lfs_t perfectly canceled out the struct hit to lfsr_dir_t:
code stack structs
before: 32992 2968 1080
after: 33004 (+0.0%) 2976 (+0.3%) 1080 (+0.0%)
Old heuristic: estimate + commit > shrub_size/2
New heuristic: estimate > shrub_size/2 || estimate + commit > shrub_size
The goal here is two fold:
1. Prevent shrubs from exceeding shrub_size
2. Avoid runaway performance issues with repeatedly recalculating the
exact estimate as a shrub approaches shrub_size
The 1/2 factor helps with 2., by evicting early, much like how our rbyds
determine when to split.
Since pending commits aren't included in the exact estimate, previously
we just added the pending commit estimate to the exact estimate before
checking our shrub heuristic. This gave us a nice single heuristic.
But it's important to note our shrubs are actually quite small. And our
commit estimate is really quite conservative. So there's real risk of
penalizing our shrubs to the point where they're difficult to leverage
on real geometry. At this scale, the extra ~40 B per commit attr
assuming uncompressed leb128s has a real impact.
If we consider that our shrub eviction heuristic is simulating a small
rbyd, it's interesting to note that rbyds are not penalized for pending
commits. Pending commits are simply required to always fit in
block_size/2. Doesn't fit? Error.
We don't quite have that freedom in the shrubs, but we can avoid
penalizing shrubs for commits, as long as we also check that the
estimate + commit does not exceed the hard shrub_size limit.
This heuristic probably deserves more scrutiny in the future, but this
at least seemed like a reasonable optimization to make.
This readds lfsr_ftree_t, however this time its not involved in the file
staging, has no operations of its own, and really just acts as a
namespace for the file's bnull/bsprout/bptr/bshrub/btree struct.
I think this is a good way to organize things.
Code impact is also minimal:
code stack
before: 32874 2952
after: 32984 (+0.3%) 2968 (+0.5%)
Christopher Haster