fix: add Sync() calls to persist filesystem changes to storage #27
fix: add Sync() calls to persist filesystem changes to storage #27marcelfarres wants to merge 2 commits intotinygo-org:devfrom
Conversation
- Add Sync() after Format, Unmount, Remove, Rename, Mkdir - Add Sync() after File.Close, File.Sync, File.Truncate - Sync on OpenFile with O_CREATE/O_TRUNC flags - Fix cache invalidation in lfs_bd_flush - Add TestDirectoryPersistence and NestedDirectories tests"
Fix filesystem persistence on SD cardsProblemI was working with LittleFS on a Grand Central M4 with an SD card and noticed that directories and files I created weren't surviving a power cycle. I'd run InvestigationI traced the data flow through the code:
The problem: the SD card driver holds writes in a buffer for efficiency. Without explicitly calling Looking at the original code, I noticed that mutating operations just returned after calling the C function—no sync. The block device interface in What I changed
|
littlefs/go_lfs.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| if flags&(os.O_CREATE|os.O_TRUNC) != 0 { |
There was a problem hiding this comment.
I do not think it makes sense to call Sync() on an Open().
littlefs/go_lfs.go
Outdated
| if err := errval(C.lfs_file_sync(f.lfs.lfs, f.fileptr())); err != nil { | ||
| return err | ||
| } | ||
| if syncer, ok := f.lfs.dev.(tinyfs.Syncer); ok { |
There was a problem hiding this comment.
Actually Sync() on a Sync()? Seems like a yes! 👍
|
Hello @marcelfarres thanks for the code submission. Please see my comments. |
|
I probably did not notice the issue due to using flash pretty much all of the time. |
|
I am still investigating the fix, somehow I can not reproduce the error or the fix all the time? Also it seems that using different -opt levels influence the results (with the unpatch branch opt 0 1 2 have the bug, but s and z pass the tests). Any idea or experience on that? I want to be sure that the change does not add to the unreliable behavior. It also seems that SPI freq plays a role too? A bit puzzle. |
4d2ecf0 to
d6c6cd6
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to improve reliability of LittleFS operations on TinyGo targets by adding a compiler miscompilation workaround in littlefs/lfs.c, adjusting Go↔C callback ABI types, and introducing both automated and hardware-driven regression coverage.
Changes:
- Add an
optnoneworkaround tolfs_dir_fetchmatchinlittlefs/lfs.cto mitigate an LLVM-O2miscompilation. - Update exported Go block-device callbacks to use
uint32forsize(matchinglfs_size_t) to avoid ABI/type mismatches. - Add new directory-focused regression tests plus a hardware test harness and firmware example to reproduce/validate behavior on real boards.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/hw/main.go |
Adds a hardware test runner that builds/flashes “fixed/unfixed” variants by patching lfs.c and classifying serial output. |
tests/hw/go.mod |
Introduces a standalone Go module for the hardware test harness. |
tests/hw/go.sum |
Adds dependency sums for the hardware test harness module. |
tests/hw/.gitignore |
Ignores build outputs for the hardware harness (builds/, *.exe). |
littlefs/lfs.c |
Adds an __attribute__((optnone)) workaround and explanation comment on lfs_dir_fetchmatch. |
littlefs/go_lfs_test.go |
Expands directory test coverage (nested dirs, persistence, dirs-with-files, deep nesting). |
littlefs/go_lfs_callbacks.go |
Changes callback size parameter type to uint32 to match lfs_size_t. |
examples/sd_mkdir_test/main.go |
Adds a large firmware example for interactive serial-driven FS testing on SD + LittleFS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @deadprogram, this turned into a completely different fix than what I originally pushed. Long story short: the mkdir bug has nothing to do with Sync(). It's LLVM generating bad code for one specific function ( I spent a while trying to find a targeted workaround (volatile vars, memory barriers, noinline, etc.) but nothing short of disabling optimization on that one function works. So the fix is a single While I was at it I also:
Tested on Grand Central M4 + SD at PD: Sorry for the random co-pilot spam. |
LLVM's ThinLTO at -O2 miscompiles lfs_dir_fetchmatch(), causing directory metadata scans to return incorrect results. mkdir succeeds but subsequent stat/open fails with 'no directory entry'. The bug does not appear at -Oz (TinyGo's default), which is why the earlier Sync() approach seemed to help. Add __attribute__((optnone)) to lfs_dir_fetchmatch to prevent the miscompilation. Targeted approaches (volatile, noinline, memory barriers) were tested but none work - the issue is in how -O2 transforms the function's complex control flow as a whole. - littlefs/lfs.c: optnone on lfs_dir_fetchmatch, revert cache fix - littlefs/go_lfs.go: revert Sync() calls (not the actual fix) - littlefs/go_lfs_callbacks.go: fix uint32 size parameter - littlefs/go_lfs_test.go: directory persistence tests - examples/sd_mkdir_test/: hardware test firmware for SD card
d6c6cd6 to
af22fd9
Compare
I was trying to create folders & files in the SD card and it was not working.
This changes seem to make it functional again.
I test it on a M4 Gran Central, and seems to have dir persistent and creation even after mounting/un-moutning the file system.
Not an expert in any extend on TinyGo, hope it helps/you can test it/review it.
Should fix #9