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

[Anand Jain <anand.jain@oracle.com>]

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