Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update LittleFS, fix some memory leaks and remove limits to number of opened files #6

Merged
merged 7 commits into from
Apr 17, 2020

Conversation

X-Ryl669
Copy link
Contributor

This is a large PR, but I can't really split it in smaller one.
I've:

  1. updated LittleFS to a new version
  2. removed the max file limitation and reduced heap space used
  3. made the storage of the file path optional (and selectable in menuconfig), only a hash is stored instead
  4. reduced the binary footprint of the built by removing useless part in the code and string array storage
  5. updated readme and tests

Just a note on how the new table of file descriptor is written:

  • lfs_file_t is now a chained list (this is used for deallocating files upon unmounting), and it's not using a static char array anymore for storing file path (if selected in menuconfig, the file path string is allocated at the same time as the file descriptor)
  • Instead of the hack with the bitcount & bitfield for file descriptors, I'm maintaining an (heap) allocated chained list of file descriptor, and a heap allocated array of pointers to the file descriptors (which is realloc'd upon larger requirement). This means that you only pay for what you use (if you use 4x fd simultaneously, it'll allocate a array with 4x void *). I've removed all the cruft about fd_used bitfield. Do you know that bitfields are useless without #pragma pack because compiler will align the next field anyway so you're not saving anything here, and also generate more code than plain word usage because of required masking and shifting?
  • I've reworked the acquire_fd/ free_fd / search_fd so it does not need recursive mutex anymore, and also not leaking anymore upon error.

@X-Ryl669
Copy link
Contributor Author

Forgot to add, that if you disable path storage toward only storing hashes, then fstat method is disabled (but not stat). That's the only method that really requires the file's path, and I think it's rarely used anyway so it's a not a big deal.

@BrianPugh
Copy link
Member

hey @X-Ryl669 ! I'll review this over the coming days, thanks for the contribution!

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Apr 15, 2020

Ok, I've fixed the conflict with new fsync support, everything should apply correctly from now on.

@BrianPugh
Copy link
Member

I'm making my own tweaks (code comments, checks, other stuff) to your pr on branch pr6. Or is there a way i can directly edit your PR?

Regardless, I'll let you know when i'm done fiddling wtih it.

@X-Ryl669
Copy link
Contributor Author

X-Ryl669 commented Apr 16, 2020

I've commented in your commits. Overall, that's great additions (and one bug I've missed! Thanks)
There is a mistake in the cache_size resize code:

  1. cache_size and fd_count are uint16_t so they never can be > 65535.
  2. There can't be more than 64 fds opened at a time in esp-idf VFS, so there's no point trying deal above this value, it'll fail much earlier. So a simple if (cache_size >= 32) error is enough to avoid such case.
  3. Due to this, the maximum cache size is 256 bytes, so it does not make sense to try to shrink it (since the code to shrink it will likely consume more that 256 bytes) upon deallocation of fd. My logic being that if the user software required more than 32 fds opened at a time, it's likely to need it again, so shrinking/enlarging will only produce heap fragmentation and increase code latency to get a new fd since it'll need to enlarge. Think about what happens if the average fd count is 32 when doing 31=>32=>33=>32=>31=>32=>33. You'll end shrinking/enlarging each step.

There is one mistake about the hash comparison you're removing.

Computing hash is not only useful for removing file's path storage, it's also a huge time saver while searching for file based on their file path (a very common operation in the FS).
Let's say you have M = 32 fds opened, with a path in the form of log_YYYYMMDD.txt of size N

Operation Without hash With hash
Creating hash 0 (code must be changed not to compute the hash) O(N) basically 1 shift + add per byte
Searching FD O(N)*M comparisons, which is almost like O(N^2) O(N) only for computing the needle's hash, then only O(1)
Deallocation O(1) O(1)

So when using hashes, you ends up with a much lower computation time with almost no overhead. It's a shame we can't use C++ as we could be even be able to compute hash at compile-time and remove all O(N) in the table above.

@BrianPugh
Copy link
Member

alright @X-Ryl669 I think pr6 is ready. Let me know if you think any changes need to be made before merging to master.

The hardest thing to debug was figuring out why the heap was corrupting when disabling CONFIG_LITTLEFS_USE_ONLY_HASH. Turns out the issue was some implicit pointer math. Essentially the file pointer wasn't pointing at the intended area (file_ptr + sizeof(file)), it was actually pointing to file_ptr + 4*sizeof(file), which is clearly not what we wanted.

@BrianPugh
Copy link
Member

BrianPugh commented Apr 17, 2020

actually there seems to be some peculiar deadlock when using filepaths; investigating... seems to be related to using filepaths along with my mtime stuff. I'll have to investigate more tomorrow.

@X-Ryl669
Copy link
Contributor Author

It's a shame because I've did almost the same as you did about enabling heap checking (except that I've enabled heap leaking). I haven't committed those changes since I thought that they are only "test" code and not actual useful code.
Maybe it should be disabled once the tests are ok ?

BTW, there are many leaks in the current code, due to esp-idf allocating locks on-the-fly in the VFS and never deallocating them. Here are the stack for each allocation without deallocation:

3 allocations trace (80 entry buffer)
84 bytes (@ 0x3ffb4d84) allocated CPU 0 ccount 0x4fb74a88 caller 0x400886be:0x4008891b:0x40082f09:0x40082f33:0x40083080:0x400d9fd9:0x400d3cec:0x400e81b0
0x400886be: xQueueGenericCreate at ~/esp/esp-idf/components/freertos/queue.c:392
0x4008891b: xQueueCreateMutex at ~/esp/esp-idf/components/freertos/queue.c:499
0x40082f09: lock_init_generic at ~/esp/esp-idf/components/newlib/locks.c:80
0x40082f33: lock_acquire_generic at ~/esp/esp-idf/components/newlib/locks.c:135
0x40083080: _lock_acquire_recursive at ~/esp/esp-idf/components/newlib/locks.c:172
0x400d9fd9: _vfprintf_r at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/newlib/newlib/libc/stdio/vfprintf.c:855
0x400d3cec: printf at /builds/idf/crosstool-NG/.build/HOST-x86_64-apple-darwin12/xtensa-esp32-elf/src/newlib/newlib/libc/stdio/printf.c:59
0x400e81b0: lfs_dir_fetchmatch at /Users/cyril/esp/nvse_app/components/esp_littlefs/src/littlefs/lfs.c:999

84 bytes (@ 0x3ffb4ddc) allocated CPU 0 ccount 0x4fb7ac64 caller 0x400886be:0x4008891b:0x40082f09:0x40082f33:0x40083080:0x400e3a42:0x400e2788:0x4000bd86
0x400886be: xQueueGenericCreate at ~/esp/esp-idf/components/freertos/queue.c:392
0x4008891b: xQueueCreateMutex at ~/esp/esp-idf/components/freertos/queue.c:499
0x40082f09: lock_init_generic at ~/esp/esp-idf/components/newlib/locks.c:80
0x40082f33: lock_acquire_generic at ~/esp/esp-idf/components/newlib/locks.c:135
0x40083080: _lock_acquire_recursive at ~/esp/esp-idf/components/newlib/locks.c:172
0x400e3a42: uart_write at ~/esp/esp-idf/components/vfs/vfs_uart.c:196
0x400e2788: esp_vfs_write at ~/esp/esp-idf/components/vfs/vfs.c:420 (discriminator 4)

84 bytes (@ 0x3ffb4e34) allocated CPU 0 ccount 0x55c97778 caller 0x400886be:0x4008891b:0x40082f09:0x40082f33:0x40083070:0x400e25ed:0x400e72d8:0x400e4089
0x400886be: xQueueGenericCreate at ~/esp/esp-idf/components/freertos/queue.c:392
0x4008891b: xQueueCreateMutex at ~/esp/esp-idf/components/freertos/queue.c:499
0x40082f09: lock_init_generic at ~/esp/esp-idf/components/newlib/locks.c:80
0x40082f33: lock_acquire_generic at ~/esp/esp-idf/components/newlib/locks.c:135
0x40083070: _lock_acquire at ~/esp/esp-idf/components/newlib/locks.c:168
0x400e25ed: esp_vfs_unregister at ~/esp/esp-idf/components/vfs/vfs.c:187
0x400e72d8: esp_vfs_littlefs_unregister at ~/esp/nvse_app/components/esp_littlefs/src/esp_littlefs.c:194
0x400e4089: test_teardown at ~/esp/nvse_app/components/esp_littlefs/src/test/test_littlefs.c:734

252 bytes 'leaked' in trace (3 allocations)
total allocations 21 total frees 18
MALLOC_CAP_8BIT potential leak: Before 279976 bytes free, After 279708 bytes free (delta 268)
MALLOC_CAP_32BIT potential leak: Before 364928 bytes free, After 364660 bytes free (delta 268)
~/esp/nvse_app/components/esp_littlefs/src/test/test_littlefs.c:36:can initialize LittleFS in erased partition:PASS

For the deadlock you're seeing, I've spot that vfs_open is calling vfs_mtime with lock taken while the other method are not. Maybe you can change the mutex type to non recursive (I think I've removed all recursive parts, TBC) so you'll have the watchdog trigger where it's locking and this will give you a callstack.

@BrianPugh
Copy link
Member

alright, i fixed the deadlock, and i don't think any of those "memory leaks" are real. Wish there was a way to ignore these false positives.

@X-Ryl669
Copy link
Contributor Author

Seems like it's good to go!

@BrianPugh
Copy link
Member

one last change im making; I'm making HASH_ONLY not the default option; this is just to maintain 100% backwards compatability and to default to a "safer" (barely) option. People can reconfigure this in their menuconfig or sdkconfig.defaults

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants