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

From: Nikolay Borisov <nborisov@suse.com>
To: Hans van Kranenburg <Hans.van.Kranenburg@mendix.com>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Cc: Chris Mason <clm@fb.com>
Subject: Re: [PATCH 1/5] btrfs: Ensure at least 1g is free for balance
Date: Fri, 26 Oct 2018 15:16:16 +0300	[thread overview]
Message-ID: <6b03ed20-4ee6-4342-c287-618343f3b8ad@suse.com> (raw)
In-Reply-To: <34b2d8ff-d4f0-63a9-eb49-c26d9b84737c@mendix.com>

(Adding Chris to CC since he is the original author of the code)

On 26.10.2018 15:09, Hans van Kranenburg wrote:
> On 10/26/18 1:43 PM, Nikolay Borisov wrote:
>> The first part of balance operation is to shrink every constituting
>> device to ensure there is free space for chunk allocation.
> 
> A very useful thing to be able to do if there's no unallocated raw disk
> space left, is use balance to rewrite some blockgroup into the free
> space of other ones.
> 
> This does not need any raw space for a chunk allocation. Requiring so
> makes it unneccesarily more complicated for users to fix the situation
> they got in.

The current logic of the code doesn't preclude this use case since if we
can't shrink we just proceed to relocation. So even if 1g is replaced
with 5eb and shrink_device always fails balancing will still proceed.

However, all of this really makes me wonder do we even need this "first
stage" code in balancing (Chris, any thoughts?).

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