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

[David Sterba <dsterba@suse.cz>]

On Wed, Nov 10, 2021 at 11:31:25AM +0200, Nikolay Borisov wrote:
> 
> 
> 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.

I think we don't need to take v1 into account anymore. If the
deprecation goes to 5.16 and the device add / balance pause
compatibility into 5.17, we can actually remove v1 in the same release
so there's not even a chance to get to some weird state.