fix: compact when dir count hits 0x3ff #1137
Conversation
Tests passed ✓, Code: 17128 B (+0.1%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
|
geky
added
needs fix
|
Hi @dschendt, thanks for creating a PR, sorry about the late response. Digging through the history, it looks like this logic had a similar check at one point (48bd2bf), but was dropped during the big no-recursion refactor (#658). Thoughts on moving this check into the Some other notes:
|
|
Also worth noting this is side-stepped in littlefs3 (#1111) due to the use of 31-bit leb128 ids. Though that doesn't help in littlefs2. |
This matches the logic originally implemented in 48bd2bf, which was lost during the big no-recursion refactor 84da4c0. Other notes: - Checking >= 0xff matches the split logic during compaction (line 2158): end - split < 0xff - Grouping dir->erased || dir->count >= 0xff together makes it clear these share a common code path. - Checking for dir->count >= 0xff early avoids committing >8-bit ids to disk. The cat may already be out-of-the bag on this one, but opening the id space up to the full 10-bits should probably be on a non-patch release. Found by dschendt
|
Hi @dschendt, I went ahead and adopted the other tweaks to get this in on the impending patch release. Let us know if you see any issues with it. Worst case if this is merged before you see it, feel free to create another PR for a future patch. Thanks for the original PR! |
Tests passed ✓, Code: 17128 B (+0.1%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
|
|
@geky Sorry for the late reply, and thank you for taking the time to look at this and give your feedback. I haven't had much time to dig into your suggested changes but I'm not seeing any improvements on disk utilization with them (both before and after I get an error after less than 2,000 2-byte files). Even taking my original changes and changing the comparison to |
Curiously, the logic from 48bd2bf was incorrect, and would allow a commit to be tried if erased _or_ dir->count was at risk of overflow. That is clearly wrong, we should only try to commit if both conditions are met... Found again by dschendt
|
@dschendt, oh thank you for double checking. That's probably because the logic I pushed up was wrong. Curiously, it looks like the logic was backwards in 48bd2bf. Progs to unerased flash tend to get picked up as corrupt during littlefs's prog checks, which may explain why this went unnoticed. I pushed up what should be the correct logic, but can wait to bring this in. |
|
@geky No need to wait to pull this in anymore, it looks like it's working as expected for me now. My comment on waiting was just so I could figure out why your implementation wasn't doing what I expected. Seems you figured it out already and seeing your changes it makes sense now. Thanks! |
Tests passed ✓, Code: 17124 B (+0.1%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
|
|
@dschendt, cool beans. Time to get the party started. Thanks for the PR! |
3 participants
When dealing with large block sizes (128KiB, for example) and lots of small files (64 bytes and smaller, in the case of 128KiB block sizes), it's possible to run into the constraint where it seems dir->count can't exceed 1023. For example, here https://github.com/littlefs-project/littlefs/blob/master/lfs.c#L1372. This can result in cases where you can fail to create new files even though you have plenty of disk space left.