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

[Nikolay Borisov <nborisov@suse.com>]

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.

> 
> 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 ||
>>
>