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

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

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.

Thanks, Anand

>   
>   struct btrfs_fs_info {
>   	u8 fsid[BTRFS_FSID_SIZE];
> @@ -991,7 +996,6 @@ struct btrfs_fs_info {
>   	/* restriper state */
>   	spinlock_t balance_lock;
>   	struct mutex balance_mutex;
> -	atomic_t balance_running;
>   	atomic_t balance_pause_req;
>   	atomic_t balance_cancel_req;
>   	struct btrfs_balance_control *balance_ctl;
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index c0482ecea11f..b62559dfb053 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2168,7 +2168,6 @@ static void btrfs_init_balance(struct btrfs_fs_info *fs_info)
>   {
>   	spin_lock_init(&fs_info->balance_lock);
>   	mutex_init(&fs_info->balance_mutex);
> -	atomic_set(&fs_info->balance_running, 0);
>   	atomic_set(&fs_info->balance_pause_req, 0);
>   	atomic_set(&fs_info->balance_cancel_req, 0);
>   	fs_info->balance_ctl = NULL;
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7c99f74c200e..3dfd5ab2807b 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4506,7 +4506,7 @@ void update_ioctl_balance_args(struct btrfs_fs_info *fs_info, int lock,
>   
>   	bargs->flags = bctl->flags;
>   
> -	if (atomic_read(&fs_info->balance_running))
> +	if (test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags))
>   		bargs->state |= BTRFS_BALANCE_STATE_RUNNING;
>   	if (atomic_read(&fs_info->balance_pause_req))
>   		bargs->state |= BTRFS_BALANCE_STATE_PAUSE_REQ;
> @@ -4558,7 +4558,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
>   	mutex_lock(&fs_info->balance_mutex);
>   	if (fs_info->balance_ctl) {
>   		/* this is either (2) or (3) */
> -		if (!atomic_read(&fs_info->balance_running)) {
> +		if (!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
>   			mutex_unlock(&fs_info->balance_mutex);
>   			/*
>   			 * Lock released to allow other waiters to continue,
> @@ -4567,7 +4567,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
>   			mutex_lock(&fs_info->balance_mutex);
>   
>   			if (fs_info->balance_ctl &&
> -			    !atomic_read(&fs_info->balance_running)) {
> +			    !test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
>   				/* this is (3) */
>   				need_unlock = false;
>   				goto locked;
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e95cc2f09fdd..a766d2f988c1 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3886,13 +3886,14 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>   		spin_unlock(&fs_info->balance_lock);
>   	}
>   
> -	atomic_inc(&fs_info->balance_running);
> +	ASSERT(!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags));
> +	set_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>   	mutex_unlock(&fs_info->balance_mutex);
>   
>   	ret = __btrfs_balance(fs_info);
>   
>   	mutex_lock(&fs_info->balance_mutex);
> -	atomic_dec(&fs_info->balance_running);
> +	clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>   
>   	if (bargs) {
>   		memset(bargs, 0, sizeof(*bargs));
> @@ -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));
>   
>   		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));
>   		atomic_dec(&fs_info->balance_pause_req);
>   	} else {
>   		ret = -ENOTCONN;
> @@ -4066,10 +4067,10 @@ int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
>   	 * if we are running just wait and return, balance item is
>   	 * deleted in btrfs_balance in this case
>   	 */
> -	if (atomic_read(&fs_info->balance_running)) {
> +	if (test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
>   		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));
>   		mutex_lock(&fs_info->balance_mutex);
>   	} else {
>   		mutex_unlock(&fs_info->balance_mutex);
> @@ -4085,7 +4086,8 @@ int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
>   		}
>   	}
>   
> -	BUG_ON(fs_info->balance_ctl || atomic_read(&fs_info->balance_running));
> +	BUG_ON(fs_info->balance_ctl ||
> +		test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags));
>   	atomic_dec(&fs_info->balance_cancel_req);
>   	mutex_unlock(&fs_info->balance_mutex);
>   	return 0;
>