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