Re: [PATCH v2 1/3] btrfs: introduce BTRFS_EXCLOP_BALANCE_PAUSED exclusive state

[Nikolay Borisov <nborisov@suse.com>]

On 10.11.21 г. 10:56, Anand Jain wrote:
> 
> 
> On 9/11/21 11:33 pm, Nikolay Borisov wrote:
>> <snip>
>>
>>>
>>>> +void btrfs_exclop_pause_balance(struct btrfs_fs_info *fs_info)
>>>> +{
>>>> +    spin_lock(&fs_info->super_lock);
>>>> +    ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
>>>> +           fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD);
>>>> +    fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE_PAUSED;
>>>> +    spin_unlock(&fs_info->super_lock);
>>>> +}
>>>> +
>>>
>>> This function can be more generic and replace its open coded version
>>> in a few places.
>>>
>>>   btrfs_exclop_balance(fs_info, exclop)
>>>   {
>>> ::
>>>      switch(exclop)
>>>      {
>>>          case BTRFS_EXCLOP_BALANCE_PAUSED:
>>>                ASSERT(fs_info->exclusive_operation ==
>>>                  BTRFS_EXCLOP_BALANCE ||
>>>                           fs_info->exclusive_operation ==
>>>                  BTRFS_EXCLOP_DEV_ADD);
>>>              break;
>>>          case BTRFS_EXCLOP_BALANCE:
>>>              ASSERT(fs_info->exclusive_operation ==
>>>                  BTRFS_EXCLOP_BALANCE_PAUSED);
>>>              break;
>>>      }
>>> ::
>>>   }
>>>
>>>
>>>>    static int btrfs_ioctl_getversion(struct file *file, int __user
>>>> *arg)
>>>>    {
>>>>        struct inode *inode = file_inode(file);
>>>> @@ -4020,6 +4029,10 @@ static long btrfs_ioctl_balance(struct file
>>>> *file, void __user *arg)
>>>>                if (fs_info->balance_ctl &&
>>>>                    !test_bit(BTRFS_FS_BALANCE_RUNNING,
>>>> &fs_info->flags)) {
>>>
>>>
>>>>                    /* this is (3) */
>>>> +                spin_lock(&fs_info->super_lock);
>>>> +                ASSERT(fs_info->exclusive_operation ==
>>>> BTRFS_EXCLOP_BALANCE_PAUSED);
>>>> +                fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE;
>>>
>>> Here you set the status to BALANCE running. Why do we do it so early
>>> without even checking if the user cmd is a resume? Like a few lines
>>> below?
>>>
>>>      4064 if (bargs->flags & BTRFS_BALANCE_RESUME) {
>>>
>>> I guess it is because of the legacy balance ioctl.
>>>
>>>      4927 case BTRFS_IOC_BALANCE:
>>>      4928 return btrfs_ioctl_balance(file, NULL);
>>>
>>> Could you confirm?
>>
>>
>> Actually no, I thought that just because we are in (3) (based on the
>> comments) the right thing would be done. However, that's clearly not the
>> case...
>>
>> I wonder whether putting the code under the & BALANCE_RESUME branch is
>> sufficient because as you pointed out the v1 ioctl doesn't handle args
>> at all. If I'm reading the code correctly balance ioctl v1 can't really
>> resume balance because it will always return with :
>>
> 
> 
>>      20         if (fs_info->balance_ctl) {
As this part of the code is very confusing I think it is better to split
the balance v1 and v2 codes into separate functions.
>>
>>      19                 ret = -EINPROGRESS;
>>
>>      18                 goto out_bargs;
>>
>>      17         }
>>
>> OTOH if I put the code right before we call btrfs_balance then there's
>> no way to distinguish we are starting from paused state
>>
>> <snip>
> 
> Yeah looks like the legacy code did not resume the balance, it supported
> the pause though or may be the trick was to remount to resume the
> balance?
> 
> As this part of the code is very confusing I think it is better to split
> the balance v1 and v2 codes into separate functions.


Actually V1 is going to be deprecated so I think the way forward is to
move the resume under the & BALANCE_RESUME branch.