Re: [PATCH v2 12/16] btrfs: track running balance in a simpler way

[David Sterba <dsterba@suse.cz>]

On Fri, Apr 20, 2018 at 01:58:11PM +0200, David Sterba wrote:
> On Fri, Apr 20, 2018 at 03:52:24PM +0800, Anand Jain wrote:
> > 
> > 
> > On 04/20/2018 12:33 AM, David Sterba wrote:
> > > Currently fs_info::balance_running is 0 or 1 and does not use the
> > > semantics of atomics. The pause and cancel check for 0, that can happen
> > > only after __btrfs_balance exits for whatever reason.
> > > 
> > > Parallel calls to balance ioctl may enter btrfs_ioctl_balance multiple
> > > times but will block on the balance_mutex that protects the
> > > fs_info::flags bit.
> > > 
> > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > ---
> > >   fs/btrfs/ctree.h   |  6 +++++-
> > >   fs/btrfs/disk-io.c |  1 -
> > >   fs/btrfs/ioctl.c   |  6 +++---
> > >   fs/btrfs/volumes.c | 18 ++++++++++--------
> > >   4 files changed, 18 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > > index 011cb9a8a967..fda94a264eb7 100644
> > > --- a/fs/btrfs/ctree.h
> > > +++ b/fs/btrfs/ctree.h
> > > @@ -726,6 +726,11 @@ struct btrfs_delayed_root;
> > >    * (device replace, resize, device add/delete, balance)
> > >    */
> > >   #define BTRFS_FS_EXCL_OP			16
> > > +/*
> > > + * Indicate that balance has been set up from the ioctl and is in the main
> > > + * phase. The fs_info::balance_ctl is initialized.
> > > + */
> > > +#define BTRFS_FS_BALANCE_RUNNING		17
> > 
> > 
> >   Change looks good to me as such and its a good idea to drop
> >     fs_info::balance_running;
> > 
> >   However instead of making BTRFS_FS_BALANCE_RUNNING part of
> >     btrfs_fs_info::flags
> >   pls make it part of
> >     btrfs_balance_control::flags
> > 
> >   Because:
> >   We already have BTRFS_BALANCE_RESUME there.
> >   And at fs_info level BTRFS_FS_EXCL_OP tells someone excl is running.
> >   And if its a balance (either in running or implicit-paused state)
> >   then btrfs_fs_info::balance_ctl is not NULL.
> 
> This would work fine, if the btrfs_balance_control::flags were not copy
> of the ioctl structure. There are the filter flags stored there, in
> addition to BTRFS_BALANCE_RESUME.
> 
> >From that point it's the balance ioctl interface and adding the internal
> runtime flag does not seem to fit.
> 
> The status bit could be tracked separatelly in btrfs_balance_control so
> it does not use the whole filesystem flag namespace, as it's always used
> under balance mutex.
> 
> As this is simple change to the patch, I'll do that.

Ok not that simple, the running status is checked outside of
balance_mutex and there's one more assertion that does not expect the
balance_ctl to exist:

@@ -4031,16 +4032,16 @@ int btrfs_pause_balance(struct btrfs_fs_info *fs_info)
                return -ENOTCONN;
        }

-       if (atomic_read(&fs_info->balance_running)) {
+       if (test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
                atomic_inc(&fs_info->balance_pause_req);
                mutex_unlock(&fs_info->balance_mutex);

                wait_event(fs_info->balance_wait_q,
-                          atomic_read(&fs_info->balance_running) == 0);
+                          !test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags));

here it's unlocked


                mutex_lock(&fs_info->balance_mutex);
                /* we are good with balance_ctl ripped off from under us */
-               BUG_ON(atomic_read(&fs_info->balance_running));
+               BUG_ON(test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags));

and rewriting the code so this could be checked the same way is not a simple
fixup as I expected.

As there's still the extra balance mutex lock/unlock after the volume
mutex removal, I'll have a look how this could be cleaned up further.

                atomic_dec(&fs_info->balance_pause_req);
        } else {
                ret = -ENOTCONN;