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@xxxxxxxx>
> > > ---
> > > 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
