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 <nbori...@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 ||
>>
>
