This was provided as a courtesy to hopefully make custom attributes more
easy to use, however the zeroing turned out to be a bit complicated when
syncing custom attributes across multiple open files.
Implicitly zeroing trailing buffer space is also inconsistent with the
other APIs in the filesystem, such as lfs_file_read, so this commit
removes the behavior.
If you need to handle differently sized custom attributes, you can
either pre-zero the custom attribute buffers, or use lfs_getattr to find
the on-disk size of custom attributes explicitly.
This is a bit of a complicated area for the custom-attribute API without
much precedent. littlefs allows users to provide custom attributes in
the lfs_file_config struct, which get written along with other file
metadata.
Sounds great on paper, but the devil is in the details. When does the
metadata actually get written?
What about this case?
lfs_file_opencfg(lfs, file, "path", LFS_O_WRONLY, cfg_with_attrs);
lfs_file_close(lfs, file); // does not write metadata
This normally doesn't write out metadata! We've opened the file for
writing, but made no changes, so normally littlefs doesn't bother to
commit anything to disk.
Before, as a courtesy, littlefs marked the file as dirty if it noticed
the file was opened for writing with custom attributes, but this is
inaccurate could to leave to problems after a file is synced:
lfs_file_opencfg(lfs, file, "path", LFS_O_WRONLY, cfg_with_attrs);
lfs_file_sync(lfs, file);
change_attrs();
lfs_file_close(lfs, file); // does not write metadata
Unfortunately, it isn't easy to know when metadata needs to be written.
Custom attributes are provided as read-only pointers to buffers which
may be updated without additional filesystem calls, this means we don't
know if custom attributes have actually changed on the device side. If
they haven't changed, writing out metadata on every sync would be
wasteful.
Another solution would be to compare our device-side attributes with
the disk-side attributes every sync, but that would be even more
expensive.
---
So for now, the simpliest and most efficient solution wins. Custom
attributes attached to open files, are not written unless the file data
itself changes.
Note that explicit calls to lfs_setattr always update on-disk
attributes, and opening a file with LFS_O_CREATE | LFS_O_TRUNC will also
always update the on-disk attributes (though not with just LFS_O_CREAT!).
There are a few ways we could provide an API that manually forces a write
of custom attributes, such as lfs_file_setattr, though without dynamic
memory, providing these APIs gets a bit complicated. So for now we will
see if users run into issues with the current scheme.
Compared to other filesystems, littlefs's handling of open files may
come across as a bit odd, especially when you open the same file with
multiple file handles.
This commit addresses this by forcing all open readable file handles to
be consistent if any open writable file handle is synced though either
lfs_file_sync or lfs_file_close. This means open readable file handles
always mirror the state of the filesystem on disk.
To do this we again rely on the internal linked-list of open file
handles, marking files as clean, copying over the written file
cache, and synchronizing any custom attributes attached to the file
handles.
Note, this still needs cleanup and tests
---
Why was the previous behavior?
One of the nifty mechanism in littlefs is the ability to have multiple
device-side copies of a file that share copy-on-write blocks of data.
This is very useful for staging any amount of changes, which may live either
in RAM caches or allocated-but-not-committed blocks on disk, that can be
atomically updated in a single commit. After this change, littlefs still uses
this update mechanism to track open files, meaning if you lose power, the
entire file will revert to what was written at the last lfs_file_sync.
Because this mechanism already exists, it was easy enough to rely on
this to handle multiple open file handles gracefully. Each file handle
gets its own copy-on-write copy of the contents at time of open, and and
writes are managed independently of other open files.
This behavior was idiosyncratic, but consistent, though after some time
enough users raised feedback that this behavior needed to be reassessed.
Now multiple open files should conform to what's found in other
filesystem APIs, at a small code cost to manage syncing open files.
This indirectly solves an issue with lfs_file_rawclose asserting
when lfs_file_opencfg errors since appending to the mlist occurs
after open. It also may speed up some of the internal operations such as
the lfs_file_write used to resolve unflushed data.
The idea behind adopting mlist over flags is that realistically it's
unlikely for the user to open a significant number of files (enough for
big O to kick in). That being said, moving the mlist asserts into the
API wrappers does protect some of the internal operations from scaling
based on the number of open files.
This removes quite a bit of extra code needed to entertwine the
LFS_TRACE calls into the original funcions.
Also changed temporary return type to match API declaration where
necessary.
- Stayed on non-system include for lfs_util.h for now
- Named internal functions "lfs_functionraw"
- Merged lfs_fs_traverseraw
- Added LFS_LOCK/UNLOCK macros
- Changed LFS_THREADSAFE from 1/0 to defined/undefined to
match LFS_READONLY
- expand functions
- add comment
- rename functions
- fix locking issue in format and mount
- use global include
- fix ac6 linker issue
- use the global config file
- address review comments
- minor cleanup
- minor cleanup
- review comments
- undef unavailable function declarations altogether
- even less code, assert on write attempts
- remove LFS_O_WRONLY and other flags when compiling with LFS_READONLY
- do not annotate #endif, as requested
- move ifdef before comments blocks, rework dangling opening bracket
- ifdef file flags that are not needed in read-only mode
- slight refactor
- ifdef LFS_F_ERRED out as well
Previously we only bumped the revision count if an eviction would occur
immediately (and possibly corrupt littlefs). This works, but does risk
an unoptimal superblock size if an almost-exhausted superblock was
allocated during lfs_format.
As pointed out by tim-nordell-nimbelink, we can align the revision count
to maximize the number of block cycles without breaking the existing
requirements of increasing revision counts.
As an added benefit, littlefs's wear-leveling should behave more
consistently after this change.
This bug was exposed by the bad-block tests due to changes to block
allocation, but could have been hit before these changes.
In flash, when blocks fail, they don't fail in a predictable manner. To
account for this, the bad-block tests check a number of failure
behaviors. The interesting one here is "LFS_TESTBD_BADBLOCK_ERASENOOP",
in which bad blocks can not be erased or programmed, and are stuck with
the data written at the time the blocks go bad.
This is actually a pretty realistic failure behavior, since flash needs a
large voltage to force the electrons of the floating gates. Though
realistically, such a failure would like corrupt the data a bit, not leave the
underlying data perfectly intact.
LFS_TESTBD_BADBLOCK_ERASENOOP is rather interesting to test for because it
means bad blocks can end up with perfectly valid CRCs after a failed write,
confusing littlefs.
---
In this case, we had the perfect series of operations such that a test
was repeatedly writing the same sequence of metadata commits to the same
block, which eventually goes bad, leaving the block stuck with metadata
that occurs later in the sequence.
What this means is that after the first commit, the metadata block
contained both the first and second commits, even though the loop in the
test hadn't reached that point yet.
expected actual
.----------. .----------.
| commit 1 | | commit 1 |
| crc 1 | | crc 1 |
| | | commit 2 <-- (from previous iteration)
| | | crc 2 |
'----------' '----------'
To protect against this, littlefs normally compares the written CRC
against the expected CRC, but because this was the exact same data that
it was going to write, this CRCs end up the same.
Ah! But doesn't littlefs also encode the state of the next page to keep
track of if the next page has been erased or not? Wouldn't that change
between iterations?
It does! In a single bit in the CRC-tag. But thanks to some incorrect
logic attempting to avoid an extra condition in the loop for writing out
padding commits, the CRC that littlefs checked against was the CRC
immediately before we include the "is-next-page-erased" bit.
Changing the verification check to use the same CRC as what is used to
verify commits on fetch solves this problem.
Not sure how this went unnoticed, I guess this is the first bug that
needed in-depth inspection after the a last-minute argument cleanup
in the debug scripts.
This rather interesting corner-case arises in lfs_dir_alloc anytime the
uninitialized revision count happens to be a multiple of block_cycles+1.
For example, the source of the bug found by tim-nordell-nimbelink:
rev = 2742492087
block_cycles = 100
2742492087 % (100+1) = 0
The reason for this weird block_cycles+1 case is due to a fix for a
previous bug in fe957de. To avoid aliasing, which would cause metadata
pairs to wear unevenly, block_cycles incremented to the next odd number.
Normally, littlefs tweaks the revision count of blocks during
lfs_dir_alloc in order to make sure evictions can't happen on the first
compact. Otherwise, higher-level logic such as lfs_format would break.
However, this wasn't updated with the aliasing fix in fe957de, so
lfs_dir_alloc was only rounding the revision count to the nearest even
number.
The current fix is to change the logic in lfs_dir_alloc to explicitly
check for the eviction condition and increment if eviction would occur.
Found by tim-nordell-nimbelink
As noted by gtaska, we are sitting on a better hash-combining function
than xor: CRC. Previous issues with xor were solvable, but relying on
xor for this isn't really worth the risk when we already have a CRC
function readily available.
To quote a study found by gtaska:
https://michiel.buddingh.eu/distribution-of-hash-values
> CRC32 seems to score really well, but its graph is skewed by the results
> of Dataset 5 (binary numbers), which may or may not be too synthetic to
> be considered a fair benchmark. But even if you substract the results
> from that test, it does not fare significantly worse than other,
> cryptographic hash functions.
On first read, randomizing the allocators offset may seem appropriate
for lfs_alloc_reset. However, it ends up using the filesystem-fed
pseudorandom seed in situations it wasn't designed for.
As noted by gtaska, the combination of using xors for feeding the seed
and multiple traverses of the same CRCs can cause the seed to flip to
zeros with concerning frequency.
Removed the randomization from lfs_alloc_reset, leaving it in only
lfs_mount.
Found by gtaska
Modulus of the offset by block_size was clearly a typo, and should be
block_count. Interesting to note that later moduluses during alloc
calculations prevents this from breaking anything, but as gtaska notes it
could skew the wear-leveling distribution.
Found by guiserle and gtaska
As introduced in #297, I created a python wrapper for littlefs. The wrapper supports two API's: A C-like API which is the same as in C and a more pythonic API which is easier to use if you are more the python guy. The wrapper is built with littlefs 2.2.1 at the moment.
__VA_ARGS__ are frustrating in C. Even for their main purpose (printf),
they fall short in that they don't have a _portable_ way to have zero
arguments after the format string in a printf call.
Even if we detect compilers and use ##__VA_ARGS__ where available, GCC
emits a warning with -pedantic that is _impossible_ to explicitly
disable.
This commit contains the best solution we can think of. A bit of
indirection that adds a hidden "%s" % "" to the end of the format
string. This solution does not work everywhere as it has a runtime
cost, but it is hopefully ok for debug statements.
- Standardized littlefs debug statements to use hex prefixes and
brackets for printing pairs.
- Removed the entry behavior for readtree and made -t the default.
This is because 1. the CTZ skip-list parsing was broken, which is not
surprising, and 2. the entry parsing was more complicated than useful.
This functionality may be better implemented as a proper filesystem
read script, complete with directory tree dumping.
- Changed test.py's --gdb argument to take [init, main, assert],
this matches the names of the stages in C's startup.
- Added printing of tail to all mdir dumps in readtree/readmdir.
- Added a print for if any mdirs are corrupted in readtree.
- Added debug script side-effects to .gitignore.
