Re: [PATCH 1/5] btrfs: Ensure at least 1g is free for balance

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2018/10/26 下午8:08, Nikolay Borisov wrote:
> 
> 
> On 26.10.2018 15:04, Qu Wenruo wrote:
>>
>>
>> On 2018/10/26 下午7:43, Nikolay Borisov wrote:
>>> The first part of balance operation is to shrink every constituting
>>> device to ensure there is free space for chunk allocation. However, the code
>>> has been buggy ever since its introduction since calculating the space to shrink
>>> the device by was bounded by 1 mb. Most likely the original intention was to
>>> have an upper bound of 1g and not 1m, since the largest chunk size is 1g.
>>
>> Minor nitpick, largest chunk size -> largest chunk stripe size.
>>
>> As for data chunk, it's possible to get a 10G chunk, but still only 1G
>> stripe up limit.
>>
>>> This 
>>> means the first stage in __btrfs_balance so far has been a null op since it
>>> effectively freed just a single megabyte.
>>>
>>> Fix this by setting an upper bound of size_to_free of 1g. 
>>
>> One question come to me naturally, what if we failed to shrink the device?
>>
>> In fact if btrfs_shrink_device() returns ENOSPC we just skip to
>> relocation part, so it doesn't look like to cause regression.
>>
>> If this can be mentioned in the commit message, it would save reviewer
>> minutes to read the code.
> 
> Will incorporate it in v2.
> 
>>
>>
>>
>> BTW, I think for that (ret == ENOSPC) after btrfs_shrink_device(), we
>> should continue other than break, to get more chance to secure
>> unallocated space.
> 
> I agree but this should be done in a separate patch, this one deals with
> the silly upper bound of 1m.

No problem, just a hint for a new patch :)

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/volumes.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index f435d397019e..8b0fd7bf3447 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -3467,7 +3467,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>>>  	list_for_each_entry(device, devices, dev_list) {
>>>  		old_size = btrfs_device_get_total_bytes(device);
>>>  		size_to_free = div_factor(old_size, 1);
>>> -		size_to_free = min_t(u64, size_to_free, SZ_1M);
>>> +		size_to_free = min_t(u64, size_to_free, SZ_1G);
>>>  		if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) ||
>>>  		    btrfs_device_get_total_bytes(device) -
>>>  		    btrfs_device_get_bytes_used(device) > size_to_free ||
>>>
>>