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