feat(fatfs): add multi-sector readahead for move_window() (IDFGH-17499)#18439
feat(fatfs): add multi-sector readahead for move_window() (IDFGH-17499)#18439Nicba1010 wants to merge 2 commits into
Conversation
|
|
Adds a configurable readahead buffer (FATFS_WINDOW_SECTORS) that reads multiple consecutive sectors on a cache miss, amortizing per-transaction SPI overhead. Default of 1 preserves existing single-sector behavior. The readahead buffer is dynamically allocated via ff_memalloc() when FF_USE_DYN_BUFFER is enabled, ensuring DMA-capable/cache-aligned placement consistent with the win[] buffer.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared a fix for 1 of the 2 issues found in the latest run.
- ✅ Fixed: Dynamically allocated
ra_bufleaks on mount failure- I fixed the same mount-failure leak pattern for the dynamic filesystem window buffer by freeing stale allocations before remount and freeing/resetting the buffer on unmount regardless of
fs_type.
- I fixed the same mount-failure leak pattern for the dynamic filesystem window buffer by freeing stale allocations before remount and freeing/resetting the buffer on unmount regardless of
Or push these changes by commenting:
@cursor push b76ddd9a0a
Preview (b76ddd9a0a)
diff --git a/components/fatfs/src/ff.c b/components/fatfs/src/ff.c
--- a/components/fatfs/src/ff.c
+++ b/components/fatfs/src/ff.c
@@ -3513,8 +3513,9 @@
if (SS(fs) > FF_MAX_SS || SS(fs) < FF_MIN_SS || (SS(fs) & (SS(fs) - 1))) return FR_DISK_ERR;
#endif
#if FF_USE_DYN_BUFFER
- fs->win = ff_memalloc(SS(fs)); /* Allocate memory for sector buffer */
- if (!fs->win) return FR_NOT_ENOUGH_CORE;
+ if (fs->win) ff_memfree(fs->win); /* Discard sector buffer left by a failed mount */
+ fs->win = ff_memalloc(SS(fs)); /* Allocate memory for sector buffer */
+ if (!fs->win) return FR_NOT_ENOUGH_CORE;
#endif
/* Find an FAT volume on the hosting drive */
@@ -3768,8 +3769,10 @@
ff_mutex_delete(vol);
#endif
#if FF_USE_DYN_BUFFER
- if (cfs->fs_type) /* Check if the buffer was ever allocated */
- ff_memfree(cfs->win); /* Deallocate buffer allocated for the filesystem object */
+ if (cfs->win) { /* Deallocate buffer allocated for the filesystem object */
+ ff_memfree(cfs->win);
+ cfs->win = 0;
+ }
#endif
cfs->fs_type = 0; /* Invalidate the filesystem object to be unregistered */
}
@@ -3790,6 +3793,9 @@
#endif
#endif
fs->fs_type = 0; /* Invalidate the new filesystem object */
+#if FF_USE_DYN_BUFFER
+ fs->win = 0;
+#endif
FatFs[vol] = fs; /* Register it */
}This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 95b9c17. Configure here.
| #if FF_USE_DYN_BUFFER | ||
| BYTE* ra_buf; /* Readahead sector buffer (dynamically allocated) */ | ||
| #else | ||
| BYTE ra_buf[FF_MIN_SS * CONFIG_FATFS_WINDOW_SECTORS]; /* Readahead sector buffer */ |
There was a problem hiding this comment.
Static readahead buffer sized with wrong sector constant
High Severity
The static ra_buf is declared using FF_MIN_SS but the existing win buffer on line 191 correctly uses FF_MAX_SS. When FF_MIN_SS < FF_MAX_SS (e.g., SD card at 512 bytes and wear-levelling at 4096 bytes) and FF_USE_DYN_BUFFER is disabled, disk_read writes SS(fs) * CONFIG_FATFS_WINDOW_SECTORS bytes into ra_buf, which can be up to FF_MAX_SS * N — greatly exceeding the undersized FF_MIN_SS * N buffer and causing a stack/struct overflow.
Reviewed by Cursor Bugbot for commit 95b9c17. Configure here.
There was a problem hiding this comment.
Bugbot Autofix determined this is a false positive.
This codebase has no static ra_buf (or CONFIG_FATFS_WINDOW_SECTORS usage) in components/fatfs/src/ff.h, so the reported FF_MIN_SS readahead overflow path is not present.
This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| #if CONFIG_FATFS_WINDOW_SECTORS > 1 | ||
| fs->ra_buf = ff_memalloc(SS(fs) * CONFIG_FATFS_WINDOW_SECTORS); | ||
| if (!fs->ra_buf) { ff_memfree(fs->win); fs->win = 0; return FR_NOT_ENOUGH_CORE; } | ||
| #endif |
There was a problem hiding this comment.
Dynamically allocated ra_buf leaks on mount failure
Medium Severity
When FF_USE_DYN_BUFFER is enabled, ra_buf is allocated at line 3554 but the many early-return error paths after it (lines 3561, 3562, and throughout mount_volume) don't free it. The f_mount cleanup at line 3810 only frees buffers when cfs->fs_type is non-zero, but fs_type remains zero after a failed mount. Repeated mount attempts will overwrite fs->ra_buf with a new allocation each time, permanently leaking the previous one.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 95b9c17. Configure here.
There was a problem hiding this comment.
Hey @pacucha42 from what I can see the fs->win has the same leak as well if this is real? Or am I missing something? Is this the correct flow?
f_mount(fs, drv, 1)is called — this registersfs, setsfs->fs_type = 0(line 3835), then callsmount_volume()which allocateswin/ra_buf. Mount fails,fs_typestays0.- Format happens, then
f_mount(fs, drv, 1)is called again with the samefspointer. - In
f_mount(),cfs = FatFs[vol]gets the oldfs(line 3800). Sincecfsis non-NULL, it enters the cleanup block (line 3801). Butcfs->fs_type == 0, so theff_memfreeis skipped (line 3810). The oldwin/ra_bufpointers are now leaked. - Then
fsis re-registered (line 3836),fs_typeset to0again, andmount_volume()allocates freshwin/ra_buf, overwriting the leaked pointers.
github-actions
Bot
changed the title
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Guard readahead on SS(fs) == FF_MIN_SS so it only activates for small-sector drives (SD cards). WL flash with 4096-byte sectors bypasses readahead entirely because: - WL has no SPI per-transaction overhead, so readahead provides no benefit - With FF_USE_DYN_BUFFER enabled, ra_buf would waste 32KB (4096 * 8) of DMA-capable RAM on a buffer that is never useful - With FF_USE_DYN_BUFFER disabled, ra_buf is sized at FF_MIN_SS * N and a WL drive writing SS(fs) * N bytes would overflow it
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
4 participants
Adds a configurable readahead buffer (FATFS_WINDOW_SECTORS) that reads
multiple consecutive sectors on a cache miss, amortizing per-transaction
SPI overhead. Default of 1 preserves existing single-sector behavior.
The readahead buffer is dynamically allocated via ff_memalloc() when
FF_USE_DYN_BUFFER is enabled, ensuring DMA-capable/cache-aligned
placement consistent with the win[] buffer.
Description
Each
move_window()call currently issues a single-sectordisk_read(). On SD cards over SPI,per-transaction protocol overhead (CMD, R1 response, data token wait, CRC16, CS toggle)
dominates — measured at 661μs overhead vs 205μs actual data transfer per 512-byte sector
(76% overhead). Sequential metadata access patterns (directory scanning, FAT chain traversal)
issue hundreds of these back-to-back.
This PR adds a new Kconfig option
CONFIG_FATFS_WINDOW_SECTORS(default 1, range 1–16) thatcontrols how many consecutive sectors
move_window()reads per cache miss. On a miss, sectorsare read in a single multi-sector
disk_read()call (CMD18) into a readahead buffer; subsequentmove_window()calls for adjacent sectors are served from the buffer viamemcpywith no disk I/O.Benchmarked on an ESP32-S3 with SPI SD card at 20 MHz, listing 2249 files:
Cost:
FF_MIN_SS * (N-1)bytes of additional RAM (e.g. 3.5KB atWINDOW_SECTORS=8).Key implementation details:
#if CONFIG_FATFS_WINDOW_SECTORS > 1)FF_USE_DYN_BUFFERis enabled (default on master),ra_bufis allocated/freed viaff_memalloc()/ff_memfree()alongsidewin, ensuringproper DMA/cache alignment
sync_window), volume mount (check_fs), and directory cluster clearing (dir_clear)Related
None — standalone enhancement to the FatFS component. Applies to both
masterandrelease/v5.5.Testing
host_test,test_apps/dyn_buffers,test_apps/flash_wl) passes unchanged — defaultWINDOW_SECTORS=1compiles outall readahead code
dyn_buffersallocation count test (expects exactly 2 allocations ofFF_MAX_SS) is unaffected:ra_bufis a different size and onlyallocated when
WINDOW_SECTORS > 1WINDOW_SECTORS=8on SPI SD card (ESP32-S3): mount, create files/directories, list 2249 files, read, write, unmount — nodata corruption
esp_timer_get_time()instrumentation and FatFS source-level tracingWINDOW_SECTORS > 1yet — existing tests only exercise the default (no-op) path. Happy to add atest_appsvariant if desiredChecklist
Note
Medium Risk
Changes core FatFS sector caching/mount code paths and adds new buffering/allocation logic, which could affect data integrity or memory usage on constrained targets when enabled. Default config keeps behavior unchanged, reducing blast radius.
Overview
Adds new Kconfig
FATFS_WINDOW_SECTORS(default1, range1–16) to letmove_window()read and cache multiple consecutive sectors per miss to reduce SPI SD overhead.When
CONFIG_FATFS_WINDOW_SECTORS > 1,FATFSgains a readahead cache (ra_base/ra_count/ra_buf),move_window()serves hits viamemcpyand fills the cache via multi-sectordisk_read()with a single-sector fallback on failure; the cache is invalidated on writes (sync_window), mount/boot-sector checks (check_fs), and cluster clearing (dir_clear), andra_bufis allocated/freed alongsidewinunderFF_USE_DYN_BUFFER.Reviewed by Cursor Bugbot for commit 95b9c17. Bugbot is set up for automated code reviews on this repo. Configure here.