Re: [PATCH] btrfs: resize: Allow user to shrink missing device

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Anand Jain <anand.jain@oracle.com>, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org
Cc: Nathan Dehnel <ncdehnel@gmail.com>
Subject: Re: [PATCH] btrfs: resize: Allow user to shrink missing device
Date: Tue, 19 Nov 2019 15:54:43 +0800	[thread overview]
Message-ID: <70fe275c-5473-771d-3d3d-6aa8dbb94c39@gmx.com> (raw)
In-Reply-To: <9f35f91d-802f-9c93-11f5-fbdffe583a88@oracle.com>

[-- Attachment #1.1: Type: text/plain, Size: 5614 bytes --]

On 2019/11/19 下午3:40, Anand Jain wrote:
> 
> 
> On 11/18/19 8:02 PM, Qu Wenruo wrote:
>>
>>
>> On 2019/11/18 下午7:38, Anand Jain wrote:
>>> On 18/11/19 3:05 PM, Qu Wenruo wrote:
>>>> One user reported an use case where one device can't be replaced due to
>>>> tiny device size difference.
>>>>
>>>> Since it's a RAID10 fs, if we go regular "remove missing" it can take a
>>>> long time and even not be possible due to lack of space.
>>>>
>>>> So here we work around this situation by allowing user to shrink
>>>> missing
>>>> device.
>>>> Then user can go shrink the device first, then replace it.
>>>
>>>
>>>   Why not introduce --resize option in the replace command.
>>>   Which shall allow replace command to resize the source-device
>>>   to the size of the replace target-device.
>>
>> Nope, it won't work for degraded mount.
> 
>  Umm.. Why?

Try it.

Mount a raid1 fs with missing devices, degraded,
And then, try to resize the missing device, without this patch.

I should have made this point pretty clear in both the title and the
commit message.

Thanks,
Qu

> 
>  (IMHO reasoning adds clarity to the technical discussions. my 1c).
> 
>> That's the root problem the patch is going to solve.
> 
>  IMO this patch does not the solve the root of the problem and its
>  approach is wrong.
> 
>  The core problem as I see - currently we are determining the required
>  size for the replace-target by means of source-disk size, instead it
>  should be calculated by the source-disk space consumption.
>  Also warn if target is smaller than source and fail if target is
>  smaller than the consumption by the source-disk.
> 
>  IMO changing the size of the missing device is point less. What if
>  in between the resize and replace the missing device reappears
>  in the following unmount and mount.
> 
> Thanks, Anand
> 
>> Thanks,
>> Qu
>>
>>>
>>> Thanks, Anand
>>>
>>>> Reported-by: Nathan Dehnel <ncdehnel@gmail.com>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    fs/btrfs/ioctl.c | 29 +++++++++++++++++++++++++----
>>>>    1 file changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>> index de730e56d3f5..ebd2f40aca6f 100644
>>>> --- a/fs/btrfs/ioctl.c
>>>> +++ b/fs/btrfs/ioctl.c
>>>> @@ -1604,6 +1604,7 @@ static noinline int btrfs_ioctl_resize(struct
>>>> file *file,
>>>>        char *sizestr;
>>>>        char *retptr;
>>>>        char *devstr = NULL;
>>>> +    bool missing;
>>>>        int ret = 0;
>>>>        int mod = 0;
>>>>    @@ -1651,7 +1652,10 @@ static noinline int btrfs_ioctl_resize(struct
>>>> file *file,
>>>>            goto out_free;
>>>>        }
>>>>    -    if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) {
>>>> +
>>>> +    missing = test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state);
>>>> +    if (!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>>>> +        !missing) {
>>>>            btrfs_info(fs_info,
>>>>                   "resizer unable to apply on readonly device %llu",
>>>>                   devid);
>>>> @@ -1659,13 +1663,24 @@ static noinline int btrfs_ioctl_resize(struct
>>>> file *file,
>>>>            goto out_free;
>>>>        }
>>>>    -    if (!strcmp(sizestr, "max"))
>>>> +    if (!strcmp(sizestr, "max")) {
>>>> +        if (missing) {
>>>> +            btrfs_info(fs_info,
>>>> +                "'max' can't be used for missing device %llu",
>>>> +                   devid);
>>>> +            ret = -EPERM;
>>>> +            goto out_free;
>>>> +        }
>>>>            new_size = device->bdev->bd_inode->i_size;
>>>> -    else {
>>>> +    } else {
>>>>            if (sizestr[0] == '-') {
>>>>                mod = -1;
>>>>                sizestr++;
>>>>            } else if (sizestr[0] == '+') {
>>>> +            if (missing)
>>>> +                btrfs_info(fs_info,
>>>> +                "'+size' can't be used for missing device %llu",
>>>> +                       devid);
>>>>                mod = 1;
>>>>                sizestr++;
>>>>            }
>>>> @@ -1694,6 +1709,12 @@ static noinline int btrfs_ioctl_resize(struct
>>>> file *file,
>>>>                ret = -ERANGE;
>>>>                goto out_free;
>>>>            }
>>>> +        if (missing) {
>>>> +            ret = -EINVAL;
>>>> +            btrfs_info(fs_info,
>>>> +            "can not increase device size for missing device %llu",
>>>> +                   devid);
>>>> +        }
>>>>            new_size = old_size + new_size;
>>>>        }
>>>>    @@ -1701,7 +1722,7 @@ static noinline int btrfs_ioctl_resize(struct
>>>> file *file,
>>>>            ret = -EINVAL;
>>>>            goto out_free;
>>>>        }
>>>> -    if (new_size > device->bdev->bd_inode->i_size) {
>>>> +    if (!missing && new_size > device->bdev->bd_inode->i_size) {
>>>>            ret = -EFBIG;
>>>>            goto out_free;
>>>>        }
>>>>
>>>
>>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]