On Thu, Nov 22, 2018 at 10:17:33AM +0200, Nikolay Borisov wrote: > The first step fo the rebalance process, ensuring there is 1mb free on > each device. This number seems rather small. And in fact when talking > to the original authors their opinions were: > > "man that's a little bonkers" > "i don't think we even need that code anymore" > "I think it was there to make sure we had room for the blank 1M at the > beginning. I bet it goes all the way back to v0" > "we just don't need any of that tho, i say we just delete it" > > Clearly, this piece of code has lost its original intent throughout > the years. It doesn't really bring any real practical benefits to the > relocation process. That I would agree, > No functional changes. though this sounds too bold given what code it removes. Shrink, grow in a transaction, all of that can fail for various reasons and would have visible effects. I'd rather reserve the 'no functional changes' statement for cases where it's really just renaming or restructuring the code without any side effects. > - ret = btrfs_shrink_device(device, old_size - size_to_free); > - if (ret == -ENOSPC) > - break; Shrink can be pretty heavy even before it fails with ENOSPC, I think the runtime of balance will go down. And it could be even noticeable, as it has to go through all chunks twice before it really fails. Measuring that would be good as it's a thing that can be a hilight of pull request/change overviews. I'll add the patch to for-next for testing, please update te changelog with the potential effects. Thanks.
