search

littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
5.25k stars 803 forks source link

Fix DivideByZero exception when filesystem is completely full. #966

Closed BrianPugh closed 7 months ago

BrianPugh commented 7 months ago

There were reports of a DivideByZero exception in esp_littlefs when the filesystem was completely full. @huming2207 was able to get a stacktrace, and it points to the logging line in this PR. We're still figuring out if this is the certainly the cause, but the current code looks like an overlooked bug from #912.

Might address https://github.com/joltwallet/esp_littlefs/issues/183

Original original thread: https://esp32.com/viewtopic.php?f=13&t=39197&p=130708

geky-bot commented 7 months ago
Tests passed ✓, Code: 17032 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 812 B (+0.0%) | | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 17032 B (+0.0%) | 1432 B (+0.0%) | 812 B (+0.0%) | Lines | 2391/2571 lines (+0.0%) | | Readonly | 6190 B (+0.0%) | 448 B (+0.0%) | 812 B (+0.0%) | Branches | 1243/1582 branches (+0.0%) | | Threadsafe | 17896 B (+0.0%) | 1432 B (+0.0%) | 820 B (+0.0%) | | **Benchmarks** | | Multiversion | 17096 B (+0.0%) | 1432 B (+0.0%) | 816 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18728 B (+0.0%) | 1736 B (+0.0%) | 816 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17712 B (+0.0%) | 1424 B (+0.0%) | 812 B (+0.0%) | Erased | 1568888832 B (+0.0%) |
geky commented 7 months ago

Hi @BrianPugh, thanks for this.

Yeah, it looks like a simple mistake. I think what happened is I rebased #912 from a branch started before #866, and the lfs->cfg->block_size -> lfs->block_size change technically wasn't a merge conflict.

Will merge, though we should probably have a test to prevent regression.

geky commented 7 months ago

A quick grep shows lfs->cfg->block_size is only used in lfs_mount_ and lfs_migrate_, so hopefully this is the only case of this.

huming2207 commented 7 months ago

@geky as per our quick tests, by changing the lfs->cfg->block_size to lfs->block_size will fix the crash for us.

geky commented 7 months ago

I've gone ahead and extended test_alloc to test with an inferred block count. It's a bit hacky since our test runner doesn't really expect config to be mutable, but it should work.

I think this is worthwhile as the block allocator is one of subsystems more likely to run into block_count-related edge cases.

I also tweaked the struct lfs_config comments for consistency and to avoid on-disk/off-disk ambiguity. But it's good to mention the inferring behavior of block_count=0.

Let me know if anything looks concerning.

geky commented 7 months ago

A bit humorous this only affects an LFS_DEBUG statement.

Many users turn these off for code savings so that likely made this issue less common.

geky-bot commented 7 months ago
Tests passed ✓, Code: 17032 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 812 B (+0.0%) | | Code | Stack | Structs | | Coverage | |:--|-----:|------:|--------:|:--|---------:| | Default | 17032 B (+0.0%) | 1432 B (+0.0%) | 812 B (+0.0%) | Lines | 2391/2571 lines (+0.0%) | | Readonly | 6190 B (+0.0%) | 448 B (+0.0%) | 812 B (+0.0%) | Branches | 1243/1582 branches (+0.0%) | | Threadsafe | 17896 B (+0.0%) | 1432 B (+0.0%) | 820 B (+0.0%) | | **Benchmarks** | | Multiversion | 17096 B (+0.0%) | 1432 B (+0.0%) | 816 B (+0.0%) | Readed | 29369693876 B (+0.0%) | | Migrate | 18728 B (+0.0%) | 1736 B (+0.0%) | 816 B (+0.0%) | Proged | 1482874766 B (+0.0%) | | Error-asserts | 17712 B (+0.0%) | 1424 B (+0.0%) | 812 B (+0.0%) | Erased | 1568888832 B (+0.0%) |
BrianPugh commented 7 months ago

all looks good to me!