RIOT icon indicating copy to clipboard operation
RIOT copied to clipboard

vfs: increase FATFS_VFS_FILE_BUFFER_SIZE (again)

Open fabian18 opened this issue 1 year ago • 8 comments

Contribution description

With _FATFS_FILE_CACHE _FATFS_FILE_SEEK_PTR _FATFS_FILE_EXFAT the problem of #20297 is still present :/, so I increased FATFS_VFS_FILE_BUFFER_SIZE once again.

Testing procedure

CPU_MODEL = samd51j20a

In file included from /home/[email protected]//ext/RIOT/sys/include/fs/fatfs.h:28,
                 from /home/[email protected]//ext/RIOT/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:29:
/home/[email protected]//ext/RIOT/sys/include/vfs.h:147:9: note: '#pragma message: VFS_NAME_MAX is 41'
  147 | #pragma message "VFS_NAME_MAX is "XTSTR(VFS_NAME_MAX)
      |         ^~~~~~~
/home/[email protected]//ext/RIOT/sys/include/vfs.h:148:9: note: '#pragma message: _FATFS_FILE_CACHE is 512'
  148 | #pragma message "_FATFS_FILE_CACHE is "XTSTR(_FATFS_FILE_CACHE)
      |         ^~~~~~~
/home/[email protected]//ext/RIOT/sys/include/vfs.h:149:9: note: '#pragma message: _FATFS_FILE_SEEK_PTR is (4)'
  149 | #pragma message "_FATFS_FILE_SEEK_PTR is "XTSTR(_FATFS_FILE_SEEK_PTR)
      |         ^~~~~~~
/home/[email protected]//ext/RIOT/sys/include/vfs.h:150:9: note: '#pragma message: _FATFS_FILE_EXFAT is (44)'
  150 | #pragma message "_FATFS_FILE_EXFAT is "XTSTR(_FATFS_FILE_EXFAT)
      |         ^~~~~~~
In file included from /home/[email protected]//ext/RIOT/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:21:
/home/[email protected]//ext/RIOT/pkg/fatfs/fatfs_vfs/fatfs_vfs.c: In function '_mount':
/home/[email protected]//ext/RIOT/core/lib/include/assert.h:148:28: error: static assertion failed: "fatfs_file_desc_t must fit into VFS_FILE_BUFFER_SIZE"
  148 | #define static_assert(...) _Static_assert(__VA_ARGS__)
      |                            ^~~~~~~~~~~~~~
/home/[email protected]//ext/RIOT/pkg/fatfs/fatfs_vfs/fatfs_vfs.c:123:5: note: in expansion of macro 'static_assert'
  123 |     static_assert(VFS_FILE_BUFFER_SIZE >= sizeof(fatfs_file_desc_t),

Issues/PRs references

#20297

fabian18 avatar Feb 05 '24 08:02 fabian18

Murdock results

:x: FAILED

bf4bcebe69260c6c3e62d66d244a4367ea29d339 vfs: increase _FATFS_FILE_EXFAT

Success Failures Total Runtime
2713 0 9351 03m:27s

Artifacts

riot-ci avatar Feb 05 '24 09:02 riot-ci

Ah I think I know what's going on - the struct is of course going to be padded. How about instead

--- a/sys/include/vfs.h
+++ b/sys/include/vfs.h
@@ -66,9 +66,11 @@
 #include <sys/types.h> /* for off_t etc. */
 #include <sys/statvfs.h> /* for struct statvfs */
 
+#include "architecture.h"
 #include "sched.h"
 #include "clist.h"
 #include "iolist.h"
+#include "macros/math.h"
 #include "mtd.h"
 #include "xfa.h"
 
@@ -279,12 +281,12 @@ extern "C" {
  * @attention Put the check in the public header file (.h), do not put the check in the
  * implementation (.c) file.
  */
-#define VFS_FILE_BUFFER_SIZE MAX5(FATFS_VFS_FILE_BUFFER_SIZE,    \
+#define VFS_FILE_BUFFER_SIZE MATH_ALIGN(MAX5(FATFS_VFS_FILE_BUFFER_SIZE, \
                                   LITTLEFS_VFS_FILE_BUFFER_SIZE, \
                                   LITTLEFS2_VFS_FILE_BUFFER_SIZE,\
                                   SPIFFS_VFS_FILE_BUFFER_SIZE,   \
                                   LWEXT4_VFS_FILE_BUFFER_SIZE    \
-                                 )
+                                 ), ARCHITECTURE_WORD_BYTES)
 #endif
 
 #ifndef VFS_NAME_MAX

benpicco avatar Feb 06 '24 12:02 benpicco

That's good yes! It is the right direction, but for CFLAGS += -DVFS_NAME_MAX=48 it fails and for 49 it is passing again.

fabian18 avatar Feb 06 '24 13:02 fabian18

Maybe MATH_ALIGN(1 + MAX5(...))

fabian18 avatar Feb 06 '24 13:02 fabian18

Weird, does this only happen with exFAT support enabled? Then it might be that the 4 bytes need to be added to _FATFS_FILE_EXFAT

benpicco avatar Feb 06 '24 14:02 benpicco

Yes, with CFLAGS += -DFATFS_FFCONF_OPT_FS_EXFAT=0 it is passing with your provided patch. (Tested up to 52). That means I will have to try again with _FATFS_FILE_EXFAT = 48? + your patch

fabian18 avatar Feb 06 '24 14:02 fabian18

And VFS_DIR_BUFFER_SIZE also needs the alignment I suppose

fabian18 avatar Feb 06 '24 14:02 fabian18