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%)
One less struct to worry about, and less code/stack pressure from
passing around multiple pointers.
There were some naming collisions:
- lfsr_ftree_size -> lfsr_file_bsize
- lfsr_ftree_read -> lfsr_file_read_
- lfsr_ftree_flush -> lfsr_file_flush_
I'm not sure this should be the final result. There are definitely some
rough spots, the hacky "pseudo-file" in lfsr_traversal_t for example.
Having a name specific to file btrees was also useful for naming/
documenting things...
But the code savings are hard to shake a stick at:
code stack
before: 33260 3024
after: 32874 (-1.2%) 2952 (-2.4%)
Direct block pointers are turning out to be a bit of an awkward file
representation for littlefs. Thanks to shrubs, direct block pointers
really don't offer that much in terms of disk savings.
Direct bptrs save ~40 B:
direct bptr: 1 attr + 1 bptr
40 B + 24 B = 64 B
indirect bshrub: 2 attr + 1 trunk + 1 bptr
2*40 B + 10 B + 24 B = 114 B
δ = +40 B (+78.1%)
Which is nice, but not really significant on disk. Their original
motivation was to avoid the cost of a btree root node for one block
files. But this can now be avoided with bshrubs, which also generalizes
to other few-block files.
I can see the argument for carving out a special case for entirely
inlined files. +~40B may be a significant cost there. But I'm just not
seeing the value for bptrs.
But direct bptrs exist as a natural extension of littlefs's design.
Files can have:
1. nothing, null data,
2. a data entry (bptr/bsprout)
3. a bshrub/btree of data entries (bptr/bsprout)
Prohibiting direct bptrs, would be a bit strange, and a future version
of littlefs may find direct bptrs useful. Say, for example, a version
that doesn't support bshrubs, suddenly bptrs become more valuable.
So this is a compromise:
1. Support reading of bptrs, this is not that much extra work on top of
supporting bsprouts. Though we do need to be aware of them in the
block allocator.
2. Convert bptrs to bshrubs/btrees on first write.
3. Ignore any extra bptr metadata, becksums, cids, etc. These add an
additional attr which complicates things.
Downside: We may lose out on potential erased-state when writing to
files created on a different device that uses bptrs. Upside: Simpler
code and a bit of code savings.
code stack
before: 33260 3024
after: 33136 (-0.4%) 3000 (-0.8%)
Ok, maybe not that much code savings...
This is an attempt to simplify things a bit by moving more logic into
the ftree layer, instead of spreading things around between the
bshrub/bsprout functions.
Now, functionality is organized into high-level ftree operations and
low-level shrub/sprout operations, which only care about the inlined
portion of the shrub/sprout. No more lfsr_bshrub_commit/
lfsr_bshrub_commit__ which were mostly unrelated.
This also adds a lfsr_shrub_t type, which, by taking advantage of the
unused write-related rbyd fields to store the shrub estimate, has the
same size as lfsr_rbyd_t, but can still be casted to an rbyd/btree for
use in readonly rbyd/btree functions.
I considered merging shrub/sprout esimate and shrub/sprout compact into
some sort of ftree_estimate/compact, but it's not obvious what the
benefit would be, so leaving that on the table for now.
---
One nice change is our staging copies are now at the ftree level
(ftree.u and ftree.u_, maybe not the best names, but this is what I've
been using for unions where the name doesn't really matter, god I want
unnamed unions). This simplifies staging, and avoids staging issues
where the underlying type changes.
---
A bit unrelated, but necessary to integrate lfsr_ftree_traverse, a
generalized lfsr_tinfo_t type for all traversal functions was added
(adopted from lfsr_traversal_t really). This is a straightforward tagged
union with relevant traversal types.
The benefit of a generalized tinfo type is better chance we can just
pass the tinfo pointer through multiple layers.
Code changes:
code stack
before: 33368 2984
after: 33260 (-0.3%) 3024 (+1.3%)
These now take, and use, the staging portion of the bshrub/bsprout. No
more hacky casts.
In theory this risks increasing RAM cost, but it's fortunately not on
the hot-path:
code stack
before: 33380 2984
after: 33368 (-0.0%) 2984 (+0.0%)
- lfsr_data_fromtrunk -> lfsr_data_fromshrub
- lfsr_data_readtrunk -> lfsr_data_readshrub
- lfsr_bshrub_commit__ -> lfsr_shrub_commit
This also changes things to prefer lfsr_rbyd_t as a shrub
representation at the mdir-commit-level.
Note, I think if we ever add file snapshots for idempotent errors again,
I don't think adding mdir/next back into the ftree is the best way to
structure this.
Instead, adding a separate linked-list for tracking bshrubs would work
without adding redundant mdir copies to the ftree struct.
Fortunately, in our current version, we don't need to track on-stack
ftrees. Actually, we don't make on-stack ftree copies at all...
While convenient, file.size is redundant info. Redundant info always
has the risk of falling out-of-sync, creating difficult to find bugs.
This was made especially apparent with dropping file-level idempotent
errors, which make possible file states quite a bit more complex (we've
given up on fully reverting errors, but we don't want errors to make the
filesystem inconsistent).
Replacing file.size with an inlinable function that derives the file
size removes this risk without too much cost. As a plus, lfsr_file_t is
one word smaller:
code stack lfsr_file_t
before: 33286 2968 112
after: 33278 (-0.0%) 2976 (+0.3%) 108 (-3.6%)
This is a big compromise in robustness in the face of errors, vs code
size and stack size.
To be clear, errors here refer to runtime errors, such as ENOSPC, EIO,
etc, not on-disk errors, though disk errors could result in a runtime
error if unrecoverable. The question is what happens to the filesystem
after it reports one of these errors to the user. Operations are
necessarily interrupted, so some in-device state may be lost.
The way I see it, there are three options, increasing in robustness, but
also increasing in code/stack cost:
1. Leave the filesystem in an inconsistent state, require an
unmount+mount cycle to continue using the filesystem.
2. Use on-stack copies to prevent corrupted state until disk commits
complete. This does not protect against intermediary states during
file operations.
3. Use on-stack copies and file snapshots to fully revert any failed
filesystem operation.
My initial thought, since this is supposed to be a robust filesystem,
was that we should try 3., fully revert any failed filesystem operation.
With some stack tradeoff, this isn't too much of a problem, until we get
to files. Files present some real problems:
1. File bshrubs need to be tracked in order to be compacted correctly.
This means our on-stack copies need to be tracked, which complicates
things a bit.
2. Bshrub estimates need to conservatively include all snapshots to
avoid compaction issues. This means if we are tracking on-stack
copies, we are effectively multiplying bshrub cost by ~3x vs ~2x.
3. We only have one file buffer. Being able to revert buffer updates
would require either unecessary disk flushes and some weird mechanism
to handle small files, or ~2x the RAM cost.
See the previous commit for more info on this.
These issues can _probably_ be worked around, with some tradeoffs, but I
think the writing is on the wall. Full reverts on errors just isn't
worth the cost for littlefs's use case.
With the snapshotting features of littlefs, it shouldn't be too hard to
still handle errors gracefully in littlefs, either by keeping two file
handles around, or reopening the file after an error.
This adds cost at the user-level, but consider that the alternative is
that all users pay roughly this cost at the filesystem-level.
Maybe in the future we should additional LFS_GLASS/LFS_TEMPERED modes to
provide all three of the above options? Let the user chose their
robustness vs code/RAM tradeoff?
---
To be clear, this change makes it so all filesystem operations are
error-idempotent, with the exception of the _contents_ of files after an
error. If an error occurs during a file operation, the contents of that
file is undefined (but also desynced, so disk is unaffected).
Code changes:
code stack
before: 33964 3080
after: 33286 (-2.0%) 2968 (-3.6%)
With these changes, O_SYNC/O_FLUSH are also much cheaper to implement.
We can see their specific costs, which, to be honest, is a bit more than
I expected since these are now just a flag check and function call:
code stack
default: 33174 2944
O_SYNC: 33224 (+0.2%) 2968 (+0.8%)
O_SYNC+O_FLUSH: 33286 (+0.3%) 2968 (+0.8%)
Now mixing in truncate/fruncate, along with desync<->sync state
transitions.
Found bugs:
- Fixed propagating LFS_F_UNSYNCED/LFS_F_UNFLUSHED state during sync
broadcasts. This is important for tracking small files correctly.
- We were not clearing the btree erased-state of other opened file
handles when we started using it, leading other file handles to have
out-of-date erased-state.
I considered moving this into lfsr_btree_commit, but file btrees are
really the only place where shared references make sense, and it feels
weird to scan file btrees every time we commit to the mtree.
- Fixed syncs not propagating to other file handles when file is synced
with disk.
It's interesting that lfsr_file_sync can actually have an effect on
the system when the disk in is-sync.
- Added O_FLUSH/O_SYNC support to lfsr_file_truncate/fruncate. This
omission was just an oversight.
Unfortunately this did add quite a bit more complexity to both
functions.
You may notice in the fix for that last bug, that lfsr_file_ftruncate
sort of drops the ball with regards to error-idempotency. This is
because, as I was trying to figure out how to recoverably move the
buffer around when fruncating small files, I realized we don't handle
small files in lfsr_file_write correctly w.r.t. error-idempotency, and
that fixing this may be intractable...
The issue is how handle overwrites for unflushed buffers.
In general, the correct thing to do when an incoming write overlaps our
file buffer, is to just write over the buffer with the new data.
Ah, but if we do this, how do we get the old data back if we run into an
error writing the data to disk? It's gone!
For normal files, this is not an issue. We can always flush to disk to
reclaim our buffer, and since a flush doesn't change the file contents,
it's fine to make this our new fallback state.
But for small files, flush is a noop, we keep these entirely in RAM.
There are some possible workarounds:
- Flush small files to disk before overwriting, sort of defeats the
purpose of caching these in RAM...
- Reread small files from disk, because that's definitely what you want
to do when you hit an error...
Also, to always have something we can read from disk implies flush
on overwrite, see above.
- Sacrificing half our buffer for staging small files. Because RAM cost
is totally not a priority...
Long story short, rethinking idempotent errors.
A recent change, motivated by user feedback, was to delay write buffer
flushes as much as possible. Before, littlefs would always flush the
buffer during lfs_file_seek, but now, buffer flushes can be delayed all
the way to lfsr_file_read, or even skipped entirely thanks to bypassing
reads.
This is all fine and dandy, except it's easy to imagine a use case where
a user might really not want a _write_ error to pop out of a _read_
call.
With this new behavior, avoiding this situation is impossible.
So enters a function common to other filesystems: lfsr_file_flush.
However it's value is quite a bit different here. Unlike flush in other
filesystems, this flush does not necessarily make data accessible on
disk. It only writes to the pending file snapshot, which is not
accessible until lfsr_file_sync.
This makes flush a function with a rather narrow scope in littlefs
(pretty much just preventing write errors in read), but since we had
already implemented this function for internal plumbing, it adds _very_
little cost.
I'm more concerned about potential user confusion around sync vs flush.
Curiously, exposing lfsr_file_flush actually _saved_ code size for some
reason. Not sure what would make that happen:
code stack
before: 33544 3072
flush: 33536 (-0.0%) 3072 (+0.0%)
flush+O_FLUSH: 33548 (+0.0%) 3072 (+0.0%)
The motivation for this comes from the observation that many users call
sync on every file write. Much more than I expected. I think one reason
is in embedded systems it's common to just write structs to disk, either
the whole file or to a log.
O_SYNC exists in POSIX/Lunix/etc, so it makes sense to provide in
littlefs. In theory it's just one extra function call, and may even save
in total application cost (though we don't measure this) by reducing the
number of function calls at the application-level.
---
Unfortunately in-practice turned out to be quite a bit different than
in-theory... The main culprit being the improved guarantees around error
atomicity...
The ideal guarantee is that if there is an error during a write, the
entire write operation is reverted. Combining this with O_SYNC means we
need to hold a copy of the origin file state all thwe way through our
sync call. This got a bit messy...
The annoying part isn't even the functionality! Our system of tracking
btree/bshrub snapshots is quite robust! The problems were entirely with:
1. Figuring out how the heck to avoid clobbering the old file buffer
state.
2. Figuring out how the internal APIs should work while passing around a
bunch of staging state.
For 1., fortunately, thanks to bypassing writes, and some careful
pointer manipulation, we can void buffer clobbing. And for 2. just some
internal API work was needed. Internally all syncs end up in
lfsr_ftree_sync, though this feels a bit clumsy since the functionality
is not really ftree related...
Unfortunately, all of this added up to quite a bit more code cost than
I had hoped. In theory, adding some sort of LFS_CERAMIC/LFS_GLASS modes
that relax error atomicity for code size could help with most of this?
But it needs some thought:
code stack
before: 33324 3072
after: 33544 (+0.7%) 3072 (+0.0%)
We end up passing intmax_t pointers around, but without a cast. This
results in a warning. Adding a cast fixes the warning. This is in the
printing logic, not the actual comparison, so hiding warnings with this
cast is not a concern here.
I also flipped the type we compare with to use the right-hand side. The
pretty-assert code already treats the right-hand as the "expected" value
(I wonder if this is an english language quirk), so I think it makes
sense to use the right-hand side as the "expected" type.
While I'm happy to have figured out the previous range update logic, it
is quite complicated, and that translates to code cost. Updating the
buffer with the write tail is 1. simpler, and 2. keeps the buffer
updated with the most recent relevant data.
Relying on the previous bypassing/buffer strategy would have been error
prone anyways. The optimization could fall apart the moment you
_increase_ the buffer size, and it doesn't extend well to multiple open
file handles.
The problem with multiple file handles is we don't know what range of
bytes have been affected when we broadcast a sync. So we just overwrite
all other open file buffers.
Fortunately, LFS_O_DESYNC provides an interesting way to avoid this, and
optimize in-file readd->writes.
Code changes, not really that much in the grand scheme of things. For
comparison I included the code cost for unconditionally clearing the
buffer during bypassing writes:
code stack
update overlap: 33356 3072
clear buffer: 33292 (-0.2%) 3072 (+0.0%)
overwrite buffer: 33316 (-0.1%) 3072 (+0.0%)
None of the available options sit well with me.
Worst case writes states after an error:
1. Maintain on-stack snapshots for entire write operation:
on-disk: abcdefghijklmnopqrstuvwxyz
write: JKLMN
error!
on-disk: abcdefghiJKlmnopqrstuvwxyz
2. Maintain on-stack snapshots for lfsr_ftree_carve:
on-disk: abcdefghijklmnopqrstuvwxyz
write: JKLMN
error!
on-disk: abcdefghijklmnopqrstuvwxyz
3. Don't maintain on-stack snapshots, rely on btree/bshrub atomicity:
on-disk: abcdefghijklmnopqrstuvwxyz
write: JKLMN
error!
on-disk: abcdstuvwxyz
Something else to consider, the on-stack snapshots increase pressure on
the available shrub_size, which must include all tracked bshrubs in the
mdir, and currently doesn't deduplicate more than checking for identical
trunks. In effect, shrubs are limited to ~shrub_size/3, which isn't
great...
Since we can't get rid of the extra shrub cost when atomic carve
operations, I'm going to revert this, since we might as well just track
all file operations and provide a fully atomic API... Element of least
surprise and all thath...
But this revert may itself be reverted in the future.
Maybe we should provide some sort of LFS_LESSATOMIC flag to allow opt-in
to non-atomic file writes for code/stack savings?
This is an attempt to reduce the overhead of on-stack snapshots during
writes, by relaxing the gaurantees provided by lfsr_file_write during
errors.
Before, thanks to the on-stack snapshot, file writes could revert to the
previous state if an error occurred. Now, on-stack snapshots are limited
to lfsr_ftree_carve, so only the state change in lfsr_ftree_carve is
reverted.
This should behave relatively predictably, since lfsr_ftree_flush calls
lfsr_ftree_carve in a normal order. If an error occurs, some, none, or
all of the data is actually written. For truncate/fruncate, there is
only one call to carve, so these remain atomic.
It's tempting to want to push this lower. If you push the atomic
operations down to the btree/bshrub level, individual btree/bshrub commits
are already atomic, so tracking on-stack bshrubs could be dropped
completely. But failures at the sub-carve level get weird! Thanks to
block crystallization and the use of order-statistic operations, the
resulting file can be quite unpredictable. For example:
on-disk: abcdefghijklmnopqrstuvwxyz
write: JKLMN
error!
on-disk: abcdstuvwxyz
With atomic carves, worst case is something like this:
on-disk: abcdefghijklmnopqrstuvwxyz
write: JKLMN
error!
on-disk: abcdefghiJKlmnopqrstuvwxyz
Thanks to file-level snapshotting, the original file can still be
recovered (and is on-disk until sync is called), but the
unpredictability is concerning.
Alternatively, if we had btree range removals, lfsr_ftree_carve could be
entirely atomic (and more efficient).
Unfortunately, btree range removals still seem like they would be
difficult to implement, and will probably be out of scope for some time.
The added code cost will also likely outweigh any savings from dropping
on-stack bshrub tracking. Still, this is probably worth looking into in
the future.
Code changes:
code stack
before: 33356 3072
after: 33006 (-1.0%) 2992 (-2.6%)
The condition for checking if the left fragment could be coalesced was
wrong, preventing a common coalescing chance in linear rewrites, and
leaving a weird 1-flush-sized alignment issue in the fragments:
Was: lfsr_data_size(&bptr.data) < lfs->cfg->fragment_size
Should be: fragment_end - (bid-(weight-1)) <= lfs->cfg->fragment_size
This was correct for the right fragment coalescing, not sure how the
left ended up messed up.
(Actually I do know why, the math here is dense, complex, and subtle,
a nasty combo.)
Desynchronized files are a new concept intended to capture some useful
quirks of the previous multiple-open-file behavior.
This adds:
- LFS_O_DESYNC - Mark a file as desync during open
- lfsr_file_desync - Mark a file as desync whenever
- lfsr_file_sync - Mark a file as NOT desync, and sync the file
Desynced files:
1. Don't recieve updates from writes to other file handles. This makes
desynced files act as a sort of snapshot of the file at the time it
was marked desync.
2. Don't call lfsr_file_sync on close. Unless lfsr_file_sync is
explicitly called, changes to desynced files are not reflected on
disk and not broadcasted to other file handles.
A side-effect of 2., is that this gives you a quick way to abort a file
write. Marking a file as desync and then closing the file will never
error.
Additionally, if an error occurs during a write operation, the file is
implicitly marked as desync. This provides graceful write aborting in
unlikely error cases. This has actually always been a feature in
littlefs, it was just named differently and didn't have an optional
recovery mode.
Since littlefs actually has to do more work to keep files in sync, the
desync feature is quite cheap:
code stack
before: 33324 3072
after: 33360 (+0.1%) 3072 (+0.0%)
- Uninitialized warning in lfsr_mtree_namelookup because we adjust
mdir.mid unconditionally even if there is an error.
Putting the mid adjustment behind a check for ENOENT _should_ fix
this, but try as I might, GCC just can't figure out what's going on.
Adding a blanket rid_=0 in lfsr_mdir_namelookup is the best workaround
I can think of right now.
- Uninitialized warnings caused by not knowing if err is <0 in functions
where err gets merged with positive results.
I think this stems all the way from bd functions, since those poke
outside of littlefs. But it's a good idea to assert on this in
functions where err needs to be <0 anyways.
The __builtin_unreachable in lfs_util.h makes this double as a
compiler hint even in release mode, which is nice.
- GCC thinks rid is uninitialized in lfsr_btree_commit when checking
if left sibling can be merged.
As far as I can tell this actually can't happen because of the above
parent.trunk=0 check. It's not to hard to look at all code paths that
set parent.trunk=!0, they also set rid to some valid value.
I think the cause is GCC giving up because gotos are involved.
These uninitialized warnings are especially annoying since they are tied
to optimization passes. Which means they only show up when compiling -Os
and show up inconsistently (I don't know if there's more behind -O3 for
example).
This combined with frequent false positives, exacerbated by our heavy
use of out-pointers, makes this warning quite frustrating to deal with.
I would be tempted to turn this warning off if is wasn't so valuable.
Now setting estimate=-1 will force a recalculation on the next bshrub
commit. This centralizing the annoying bshrub estimate calculation, so
code that allocs/fetches bshrubs can be simplifies to just struct
assignment.
As a plus, you don't pay the estimate calculation cost when only reading
a file.
Also dropped lfsr_bshrub_alloc/fetch, these weren't really useful
functions and their naming could be misleading.
I wonder if bshrubs will ever stop feeling like a big hack.
Also tweaked bshrub estimates a bit so they only include
bsprouts/bshrubs. I think we can get away with this by including
max(bptr, trunk, btree) + shrub_size when we get around to calculating
file limits.
We are now including an extra tag when estimating bsprout size, but
that's not the end of the world. At least all calls to
lfsr_bsprout_estimate__ are consistent now.
These changes were more for code organization/runtime improvements, the
benefit to code cost is minimal:
code stack
before: 33364 3072
after: 33336 (-0.1%) 3072 (+0.0%)
We weren't updating other opened files with bshrub estimates, which
meant there was a risk bshrub estimates fall out of date, and mdir
commits lock up with ERANGE.
We need more testing over these estimate conditions. Unfortunately
they're a big headache in the new design...
The bshrub estimate now expects to include all synced struct contents,
so we need to set it to something greater than zero when converting from
a bsprout/bptr/btree.
But honestly, all of this bshrub estimate stuff needs work...
1. We used a temporary bshrub outside of the tracked ftree to commit
without clobbering the ftree. This breaks the invariant that all
bshrubs are tracked during mdir commit.
By luck, the bshrubs were still working. They normally don't get
staged properly without this invariant, but:
1. These are always new bshrubs, so no state for mdir compact to be
aware of.
2. These are always immediately SHRUBCOMMITed, so they happen to get
staged.
But we can avoid breaking this invariant by encoding the commit
first. We may clobber the ftree, but this is what our on-stack ftree
copies are for.
2. We weren't including becksums when converting bptrs to bshrubs.
Maybe we shouldn't care about this, since we don't create direct
bptrs in normal operation. But I guess we'll be compat friendly for
now...
Now, when files are synced, they broadcast their disk changes to any other
opened file handles. In effect, all open files match disk after a sync
call to any opened file handle pointing to that file.
This was a much requested feature, as the previous behavior (multiple
opened file handles maintain independent snapshots) is pretty different
from other filesystems. It's also quite difficult to implement outside
of the filesystem, since you need to track all opened files, requiring
either unbounded RAM or a known upper limit.
---
A bit unrelated, but this commit also changes bshrub estimate
calculation to include all opened file handles. This adds some annoying
complexity, but is necessary to prevent sporadic ERANGE errors when
the same file is opened multiple times.
The current implementation just refetches on-disk metadata. This adds
some maybe unnecessary metadata lookups, but simplifies things by
avoiding the tracking of on-disk sprout/shrub size, which risks falling
out of date. Keep in mind we only recalculate the estimate every
~inline_size/2 bytes written.
Just like lfsr_mdir_estimate, this scales O(n^2) with the number of
opened files (this are basically the same function... hmmm... can they
be deduplicated?). This is unlikely to be a problem for littlefs's use
case, but just something to be aware of.
Code changes:
code stack
before: 32920 3032
after: 33192 (+0.8%) 3048 (+0.5%)
- Reworked *_estimate functions to use swapping bounds/variables much
like lfsr_rbyd_appendattr.
- Merged *_estimate_ into *_estimate. Mainly for (perhaps misdirected)
code cleanliness reasons. The compiler is already going to be inlining
these single-calls since inlining is always worthwhile.
- Renamed internal mdir-commit-related functions to have the __ suffix
even if there's not a direct naming conflict.
This is a bit of a weird naming scheme, but it's useful to hinting
that all the *__ functions are at the same logic level. It also hints
that you probably shouldn't call these unless you're dealing with mdir
internals. Functions like lfsr_mdir_compact__ will probably just break
if called outside of lfsr_mdir_commit.
- Moved *_estimate functions to be closer to *_compact functions, since
these are closely related (*_estimate is basically a soft *_compact).
These changes saved a bit of code, but added a surprising amount stack
cost. I'm guessing this is related using swap functions. Maybe ptr
aliasingis causes problem, or swapping breaks compiler invariants about
variable locations:
code stack
before: 32948 2984
after: 32920 (-0.1%) 3032 (+1.6%)
Maybe we should consider an alternative impl for both the *_estimate
and *_appendattr variable swapping...
This adds more internal functions to deduplicate things:
- lfsr_bsprout_isbsprout
- lfsr_bsprout_isbleaf
- lfsr_bsprout_size
- lfsr_bsprout_cmp
- lfsr_bshrub_commit__
- lfsr_bsprout_estimate__
- lfsr_bshrub_estimate__
- lfsr_bsprout_compact__
- lfsr_bshrub_compact__
This doesn't actually save that much code, I guess our attr-list
iterations are quite cheap, but it does make mdir_commit__/compact__
a bit easier to read:
code stack
before: 32964 2984
after: 32948 (+0.0%) 2984 (+0.0%)
Since we don't need this for bshrub/bsprout negotiation, we might as
well drop it to keep concerns separate.
code stack
before: 32980 3008
after: 32964 (+0.0%) 2984 (-0.5%)
Now that bshrubs are limited to files, we might as well lean into it.
Instead of relying on extra traversals through our attr-list, we now
rely on bsprouts/bshrubs always existing in the opened mdir list. This
is a much more robust way to deduplicate bsprout/bshrub compaction
operations. The previous implementation already had a bug since
bsprout/bshrub calculation was forgotten from lfsr_mdir_estimate_.
This is a direct tradeoff of code and RAM. Though note the previous
impl was incomplete and would likely need a bit more code:
code stack
before: 33184 2944
after: 32980 (-0.6%) 3000 (+1.9%)
Now instead of relying on lfsr_f_isunsynced, which will probably have
issues with multiple opened files, we do O(n^2) scan sover the opened
file list to figure out what hasn't already been compacted.
What's interesting is the slight difference in lfsr_mdir_estimate_ and
lfsr_mdir_compact__. In lfsr_mdir_compact__, by updating all opened
bshrubs/bsprouts, we trivially prevent duplicate compactions. But for
lfsr_mdir_estimate_, we need to prevent duplicates without modification.
So instead, we only include the _last_ reference in the opened file
list. This avoids duplicates while also avoiding multiple mdir lookups.
The remaining puzzle is including in-flight in-attr bshrubs. Traversing
over two data-structures is already complicated/expensive. Traversing
over three might be a bit much...
Before, rdonly buffers were dropped, requiring a re-read for files open
RDWR. Now the buffer is updated with whatever data overlaps.
This doesn't add _that_ much code cost, and may be beneficial for
certain rd/wr patterns in different parts of a file (we don't drop
buffers at all in bypassing writes). Though it may be better to open two
separate file handles in this use case...
We will need this logic anyways for updating multiple in-sync opened
file handles. And maybe this logic can be deduplicated then.
code stack
before: 32848 2944
after 32908 (+0.2%) 2944 (+0.0%)
This can happend if we read some data into our buffer (or write+flush+seek
in some weird pattern) and then do a bypassing write that overlaps the
buffer.
The solution here is to make sure the buffer is cleared when doing
bypassing writes.
In theory, we could be a bit smarter by checking and only clearing when
overlap occurs, or even updating the buffer with the new contents like
we do in the bd cache layer, but the value would probably be minimal.
File writes are already expected to clobber buffered reads.
This is more a cludge than a real fix, we should probably be handled in
the bd-wrapper layer, but that's all a mess right now and slated for
future work.
The problem is that our "rewinding" of block-level progs for alignment
leaves our bd caches with stale data. This is normally fine, but then we
go read the stale data to calculate our becksum.
Dropping our caches before calculating the becksums fixes this, at least
temporarily.
This will always fail due to prog alignment, but we won't notice because
the align flag gets set in the flush loop. So it's not a _hard_ error,
but results in an unnecessary btree commit and a bunch of extra reading.
Some of these checks aren't necessary if crystal_thresh >= prog_size,
which should always be the case, but wanting crystal_size=0 to be a
shorthand for "no crystallization" makes it's possible. Maybe we should
set crystal_thresh = lfs_max32(crystal_thresh, prog_size) in the
future...
Christopher Haster