New API function lfs_find_free_blocks.

[opilat]

It performs a free block search which later will not slow down a write operation.

It helps to have predictable write times. In scenarios like storing data from a microphone. Write taken too long when littlefs does the free block search during it.

I am open to other solutions.

[geky]

Sorry about leaving this open for so long. It arrived at an inconvenient time and ended up flying under my radar.

I've been look at other ways of approaching this problem, but that's not an excuse to not merge this when it's likely immediately useful to users.

Thoughts on renaming this function to lfs_fs_gc with the intention of adding more garbage-collection functionality (persistence, metadata compaction, etc) in the future?

The only concern I can think of is that it may be preferable to have an incremental/interruptable garbage collector, but that would be much more involved, and we can always introduce a new API for this in the future.

---

I realize this is way past the statute of limitations for open PRs, so I can go ahead and take it over. This needs to be updated for consistent code style (4-space indentions, 80-column width) and needs at least minimal tests.

[geky]

After more thought, I don't think this will make it in to this minor release. It's a good idea, but needs a bit of work:

• I don't think lfs_find_free_blocks/lfs_fs_gc should error with LFS_ERR_NOSPC. It's possible blocks will be freed before the next allocation, and this error is likely to be raised when a user hasn't tested for it.
• lfs_find_free_blocks/lfs_fs_gc should only shift the lookahead buffer by the first clear bit so we don't skip any found free blocks in the next lookahead scan. The current code shifts the lookahead buffer by a whole lookahead_size because it is only called when the lookahead buffer is exhausted.
• The code should be reorganized so lfs_alloc calls some sort of lfs_alloc_scan function that lfs_find_free_blocks/lfs_fs_gc also calls so that big of logic is reused in both cases.

And the current minor release has already been delayed enough.

With a number of ideas around garbage-collection in #791, I think it would make more sense to look at this as a piece of a larger set of garbage-collection work.

[opilat]

What exactly do you mean by shifting lookahead by the first clear bit?
The current shift by the whole lookahead buffer seems reasonable. Can you please write how the following equation should look like for your satisfaction?

lfs->free.off = (lfs->free.off + lfs->free.size) % lfs->cfg->block_count;

If we keep the same offset shift then lfs_alloc can directly use the new function and reuse in this sense the code.

[geky-bot]

Tests passed ✓, Code: 16694 B (+0.2%), Stack: 1448 B (+1.1%), Structs: 788 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	16694 B (+0.2%)	1448 B (+1.1%)	788 B (+0.0%)	Lines	2320/2500 lines (+0.0%)
Readonly	6126 B (+0.0%)	448 B (+0.0%)	788 B (+0.0%)	Branches	1186/1508 branches (-0.0%)
Threadsafe	17522 B (+0.2%)	1448 B (+1.1%)	796 B (+0.0%)		Benchmarks
Multiversion	16770 B (+0.2%)	1448 B (+1.1%)	792 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18374 B (+0.1%)	1752 B (+0.9%)	792 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17330 B (+0.2%)	1440 B (+1.1%)	788 B (+0.0%)	Erased	1568888832 B (+0.0%)

[geky]

Sorry, I did write that confusingly.

Consider a new filesystem with say 32 blocks and a lookahead of 8 bits:

in-use
| .- free
v v
11000000_00000000_00000000_00000000
  ^- free.i
'---+---'
    '- free.off .. free.size

In the current implementation, calling lfs_find_free_blocks moves free.i here:

11000000_00000000_00000000_00000000
         ^- free.i
         '---+---'
             '- free.off..free.size

But it should move here:

11000000_00000000_00000000_00000000
  ^- free.i
  '----+---'
       '- free.off..free.size

Otherwise we miss free blocks. The implementation as-is isn't broken in the sense that it gives incorrect blocks, but:

1. It will mess with wear-leveling a bit.

2. We probably want lfs_find_Free_blocks to be a noop if called multiple times. Since it won't really make "progress" on repeated calls.

I think changing it to something like this would work:

lfs->free.off = (lfs->free.off + lfs->free.i) % lfs->cfg->block_count;

[opilat]

What if we allow users to pick a strategy? Your proposal is consistent in repeated calls and makes let's say necessary shift.

It may work very badly in specific cases like this (I am not sure whether and how frequently it may happen):

in-use
| .- free
v v
11011111_11100000_00000000_00000000
  ^- free.i
'---+---'
    '- free.off .. free.size

If this case is low probable or never may exist I am going to update the equation to your proposal.

Why doesn't the code use the free.i directly it should be equal to free.size when all block are used?

[geky]

That is good example of where the lookahead buffer performs poorly.

But I think that's more a flaw the approach of the lookahead buffer as a whole.

This example works poorly in both strategies:

in-use
| .- free
v v
11011111_11011111_11000000_00000000
  ^- free.i
'---+---'
    '- free.off .. free.size

Why doesn't the code use the free.i directly it should be equal to free.size when all block are used?

I don't think there was a reason. The previous code just wasn't written to handle free.i != free.size there.

[opilat]

The new function is updated to move the lookahead at the first free block. Would you like something else to be done?

[geky-bot]

Tests passed ✓, Code: 16694 B (+0.2%), Stack: 1448 B (+1.1%), Structs: 788 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	16694 B (+0.2%)	1448 B (+1.1%)	788 B (+0.0%)	Lines	2320/2500 lines (+0.0%)
Readonly	6126 B (+0.0%)	448 B (+0.0%)	788 B (+0.0%)	Branches	1186/1508 branches (-0.0%)
Threadsafe	17522 B (+0.2%)	1448 B (+1.1%)	796 B (+0.0%)		Benchmarks
Multiversion	16770 B (+0.2%)	1448 B (+1.1%)	792 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18374 B (+0.1%)	1752 B (+0.9%)	792 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17330 B (+0.2%)	1440 B (+1.1%)	788 B (+0.0%)	Erased	1568888832 B (+0.0%)

[geky-bot]

Tests passed ✓, Code: 16694 B (+0.2%), Stack: 1448 B (+1.1%), Structs: 788 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	16694 B (+0.2%)	1448 B (+1.1%)	788 B (+0.0%)	Lines	2320/2500 lines (+0.0%)
Readonly	6126 B (+0.0%)	448 B (+0.0%)	788 B (+0.0%)	Branches	1186/1508 branches (-0.0%)
Threadsafe	17522 B (+0.2%)	1448 B (+1.1%)	796 B (+0.0%)		Benchmarks
Multiversion	16770 B (+0.2%)	1448 B (+1.1%)	792 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18374 B (+0.1%)	1752 B (+0.9%)	792 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17330 B (+0.2%)	1440 B (+1.1%)	788 B (+0.0%)	Erased	1568888832 B (+0.0%)

[geky]

Hi @opilat, sorry, I somehow missed your last comment's question.

TODO list for this PR, from my perspective:

1. Add some testing over this function.
2. Rename to lfs_fs_gc to lay groundwork for future improvements, unless there is a good reason not to.
3. Non-functional nitpicks to keep the codebase consistent:
   1. Adopt consistent style (mainly 80-column limit I think).
   2. Move behind LFS_READONLY ifdef, I don't think there's a reason to make this function available in readonly mode.
   3. Add API boilerplate (LFS_TRACE, LFS_LOCK, there's a common pattern at the bottom of the lfs.c)

I went ahead and made these changes here, but forgot I don't have push access to this PR:
ozobot/littlefs@New-function-lfs_find_free_blocks...littlefs-project:littlefs:fs-gc

Are you able to either grant push access or merge the commits in the above?

If you're ok with these changes than this looks good to me for the next minor release.

[geky]

Hi @opilat, I've gone ahead and created #875 so this makes it into the next minor release.

Let me know if you see any issues with it, and thanks for the original PR.

[opilat]

Thank you for the updates @geky. #875 looks good to me. Should I close this PR?

[geky]

Sure thing, #875 should be coming in with #877 soon. Thanks again for the PR!