Repository navigation
Several fixes + testing thereof - #70
Conversation
A volume mounted dirty has its FSInfo free-cluster count discarded, because an a dirty flag indicates an interrupted mount, which means it might be wrong. In the dirty moutn case, we scan the whole FAT to count free clusters, which costs tens of seconds to minutes on a large card. Previously, this update was not persisted and the volume stayed dirty, meaning we had to do this scan every boot. Now, we track whether the count was recomputed during the current mount. If it was, write it back to FSInfo immediately, and let flush clear the dirty flag. Conversely, we track if the free cluster count was set on disk, but we forced it to Unknown on a dirty mount, and DID NOT set it via a FAT scan. In these cases, we persist the outdated value, because cluster allocation seeks to this position and then scans. Consider: the on-disk marker indidcats we had used 100M clusters but we mount dirty and therefore force it to Unknown, we write, and we have now used 101M. In this case it's better for the cluster allocator to seek to the outdated count of 100M and scan 1M clusters to find the next free one, than to see Unknown, and force a full re-scan of all 101M clusters. This does NOT clear the dirty bit. We also fix a minor logic bug: set_dirty_flag could not clear the flag as it OR-ed the requested value into the mount-time snapshot from the BPB, so set_dirty_flag(false) was a no-op and one unclean shutdown left a volume dirty for ever.
Cluster numbers come straight off the disk, and a directory entry left half-written by a power cut points to cluster 0x0FFFFFFF (the flash erase pattern). Some calculations derive from this value without any validation, e.g. offset_from_cluster does `first_data_sector + (cluster - 2) * sectors_per_cluster` on whatever cluster number it was handed. Obviously with 0xFFFFFFF that overflows, causing a panic or nonsensical results. This adds is_valid_cluster and makes offset_from_cluster fallible, so a cluster number from outside the valid range for this volume becomes Error::CorruptedFileSystem at the caller rather than a bad seek.
`File::is_dir` reported `false` whenever the file had no directory entry behind it, which is the case for the root directory (and any directory opened by cluster number rather than by walking to it from its parent). Only directories have their clusters zeroed, so the erroneous `false` for the root dir means as soon as the root has more than one cluster's worth of directory entries, we allocate a new cluster, and whatever bytes already existed are interpreted as (garbled) entries.
The MS FAT Spec says we should set the cluster of a .. entry that points to the root to cluster 0, not whatever cluster the root dir is actually on. A minor spec compliance issue, but one that fsck.vfat complains about.
`read_status_flags` combined a live read of FAT entry 1 with `bpb`'s copy of the boot-sector flag, which is captured at mount and never updated. `set_dirty_flag` maintains `current_status_flags` instead, so once the flag had been cleared during a session, this kept reporting the volume dirty until it was re-mounted.
format.rs built its volumes inline; the tests that follow need the same thing. Move the pieces into a fixture module containing a formatted FileSystem backed by memory, deterministic pseudo-random, and have format.rs use it. This is the groundwork for later testing commits
The free-cluster count and next-free hint are what spare a large card a full FAT scan on every mount, so they have to survive being written and read back. These check that a recomputed count is persisted rather than held only in memory, and that the allocation hint advances instead of sending the allocator back to scan from the start.
|
So, the next two PRs I have depend on both the bug fixes and the testing infrastructure in this PR. Because I can't find a way to submit a stacked PR without write access to this repo, to make it easier to review I've created the next two PRs within my fork. Obviously merging them there won't do anything, but for now they are more or less RFCs anyway. Here they are, so please consider your C's RF'd :) I'm preparing another one, an STM32F4/U5 based example that effectively implements f3write as a write speed test, that can also be byte-for-byte verified on the host with f3read (in my case to ensure that there were no bugs with large cards + fat32, which uncovered some of the above bugs). |
* Drop the free-cluster-count persistence and dirty-flag clearing entirely. * Give the highest valid cluster number one definition. * Propagate corrupt cluster numbers out of `File::abs_pos` rather than folding them into `None`. * Recognise the root directory by its cluster number instead of inferring it from a missing directory entry Tests: * Cover the `..` cluster number directly. * Correct the allocation-hint docs: that test passes on master, because `set_next_free_cluster` has always dirtied FSInfo. It is a regression guard, not a demonstration of new behaviour. * Invert the discarded-count test to assert "unknown" is written, and assert the volume is still dirty afterwards * Make MemDisk behave like a fixed-size device, by errroring on writes past the end, rather than growing the buffer and hiding the out-of-range write. * Shrink to 512-byte clusters on a 40 MiB volume (still FAT320-sized), for efficiency. The per-file slot arithmetic was also wrong: the 28-character names take 3 LFN entries plus the short name, not one.
|
Thanks for taking a look, @MabezDev! I think I've addressed all your specific comments. Are you generally happy with the addition of the corpus testing harness? If so, I'll go through and tidy it up a bit, because right now it's still rather verbose LLM output which I'd prefer to clean up a bit before merge. If you'd prefer to keep only the fixes I'll not bother cleaning it, and just remove it. I've resolved several of your comments by just reverting any write to the dirty bit. This means that after a single dirty unmount, the FS will require a complete FAT rescan every subsequent mount to know free space etc, which was why I originally made this change as it caused a ~2-3minute delay to know free space. However, with the repair patch series (gekkonid#2), one can eat this cost once and recover, so that at least solves my issues (assuming that feature eventually gets merged). Let me know if you would prefer a different solution. |
The corpus tests were originally written to aid debugging, and so were initially just pure LLM vomit. Here, I've reworked a lot of the code, including: * Add external verification using `fsck.fat` to cross-check against the simplistic fat32 parser and checker in corpus/*.rs * Abstract common utilities into corpus/utils.rs, instead of having them duplicated at the top of many test files * Conglomerate the several test files (one per "fix" commit in this series) into a single set of corpus tests, to avoid proliferating too many small test files This hopefully turns these tests into something a bit more mergeable.
This removes some minor changes W.R.T. master that are the result of reverting the function but not the exact diff of earlier commits rejected in review. Also reword for clarity a couple more comments
|
I've just bit the bullet and cleaned up the testing code, which should now be far less sloppy. Notable improvements:
I'm still not sure what to do about |
If we pass write a buffer that roundly fits a sector (or multiple), that buffer is handed directly to the underlying peripheral without copying via some buffer of our own. This means that whole-sector(s) buffers passed to write MUST be DMA compatible: namely stored in RAM and word aligned. Buffers from flash (including const [0u8; 512] in write_zeros) don't fufil this, and on some chips (notably esp32s3) will pass DMA an incompatible buffer and therefore crash This patch ensures that we use a local word-aligned stack buffer to write a sector of zeros.
| // underlying sdio peripheral to error if we pass a flash addresss directly. | ||
| #[repr(align(4))] | ||
| struct ZeroBuf([u8; 512]); | ||
| let zeros = ZeroBuf([0_u8; 512]); |
There was a problem hiding this comment.
Host tests never see this path: they write through MemDisk, and BufStream's aligned-write test does not call write_zeros. A flash-const regression would stay green here.
There was a problem hiding this comment.
The dir_overflow tests cover this path, but you're right they don't test for flash const. I'm not sure of the best way to assert that portably in host tests.
Could this perhaps be better handled by the sdio crate? i.o.w. write_all at the sdio/spi layer should check that the memory range is DMA compatible on the specific hw, and if it is not, then write via a 512b stack buffer like a r-m-w cycle would (without r-m part of r-m-w in this case, of course)? Thoughts? CC @xoviat and @bugadani who were the main authors of the new sdio abstraction & stm/esp impls.
|
@kdm9 No pressure, but I think this is really close! I think with these last bits sorted we can merge this :). |
|
sorry @MabezDev v. busy couple of weeks here, will get to this later this week or this weekend. |
* Fail empty on fsck.fat and non-zero exit, in case of kill/crash * Consolidate several overlapping tests * Add tests of invalid cluster handling, assert invalid clusters raise Error::CorruptedFileSystem * Edit comments for clarity
As foreshadowed on matrix, here is a cherry-picking of several somewhat related fixes.
The first five commits contain reasonably atomic fixes, and I've tried to be rather descriptive in the commit messages. In summary:
Try to persist a free cluster count wherever possible, to speed up allocation and estimation of remaining spaceDon't write a cached dirty bit, if we have done something to unset it.This (and subsequent PRs) have been squashed and rebased from a rather long and unwieldy chain of commits in an internal vendored copy (in a subtree of a fw repo). That rebase process, and some bug-finding, was done with AI assistance. I wrote the original versions of most of the fix code, most comments, and all commit messages.
However, the testing is more or less pure AI, although I have reviewed it. In particularly, during debugging, I had an AI implement a testing harness for corrupted FS images, and some of these five fixes were found as part of that debugging, and so their tests depend on the corpus testing infrastructure that was initially only really intended for debugging.
Because of this, and in case you think it's useful, I've left the tests they way they were in my internal vendored copy, however if you prefer this just be a minimal set of bug fixes (or if we should make more thorough and concise version of this testing approach), please let me know and I'll remove the three testing commits.
best,
K