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

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

On 8/11/21 10:28 pm, Nikolay Borisov wrote:
> Current set of exclusive operation states is not sufficient to handle all
> practical use cases. In particular there is a need to be able to add a
> device to a filesystem that have paused balance. Currently there is no
> way to distinguish between a running and a paused balance. Fix this by
> introducing BTRFS_EXCLOP_BALANCE_PAUSED which is going to be set in 2
> occasions:
> 
> 1. When a filesystem is mounted with skip_balance and there is an
>     unfinished balance it will now be into BALANCE_PAUSED instead of
>     simply BALANCE state.
> 
> 2. When a running balance is paused.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/ctree.h   |  3 +++
>   fs/btrfs/ioctl.c   | 13 +++++++++++++
>   fs/btrfs/volumes.c | 11 +++++++++--
>   3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 7553e9dc5f93..8376271bfed1 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -613,6 +613,7 @@ enum {
>    */
>   enum btrfs_exclusive_operation {
>   	BTRFS_EXCLOP_NONE,
> +	BTRFS_EXCLOP_BALANCE_PAUSED,
>   	BTRFS_EXCLOP_BALANCE,
>   	BTRFS_EXCLOP_DEV_ADD,
>   	BTRFS_EXCLOP_DEV_REMOVE,
> @@ -3305,6 +3306,8 @@ bool btrfs_exclop_start_try_lock(struct btrfs_fs_info *fs_info,
>   				 enum btrfs_exclusive_operation type);
>   void btrfs_exclop_start_unlock(struct btrfs_fs_info *fs_info);
>   void btrfs_exclop_finish(struct btrfs_fs_info *fs_info);
> +void btrfs_exclop_pause_balance(struct btrfs_fs_info *fs_info);
> +
>   
>   /* file.c */
>   int __init btrfs_auto_defrag_init(void);
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 92424a22d8d6..f4c05a9aab84 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -414,6 +414,15 @@ void btrfs_exclop_finish(struct btrfs_fs_info *fs_info)
>   	sysfs_notify(&fs_info->fs_devices->fsid_kobj, NULL, "exclusive_operation");
>   }
> 



> +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?

-Anand

> +				spin_unlock(&fs_info->super_lock);
>   				need_unlock = false;
>   				goto locked;
>   			}
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index cc80f2a97a0b..e87f9ac440c2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4355,8 +4355,10 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>   	ret = __btrfs_balance(fs_info);
>   
>   	mutex_lock(&fs_info->balance_mutex);
> -	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
> +	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req)) {
>   		btrfs_info(fs_info, "balance: paused");
> +		btrfs_exclop_pause_balance(fs_info);
> +	}
>   	/*
>   	 * Balance can be canceled by:
>   	 *
> @@ -4406,6 +4408,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>   static int balance_kthread(void *data)
>   {
>   	struct btrfs_fs_info *fs_info = data;
> +	bool paused = false;
>   	int ret = 0;
>   
>   	mutex_lock(&fs_info->balance_mutex);
> @@ -4432,6 +4435,10 @@ int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
>   		return 0;
>   	}
>   
> +	spin_lock(&fs_info->super_lock);
> +	ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED);
> +	fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE;
> +	spin_unlock(&fs_info->super_lock);
>   	/*
>   	 * A ro->rw remount sequence should continue with the paused balance
>   	 * regardless of who pauses it, system or the user as of now, so set
> @@ -4500,7 +4507,7 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
>   	 * is in a paused state and must have fs_info::balance_ctl properly
>   	 * set up.
>   	 */
> -	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE))
> +	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE_PAUSED))
>   		btrfs_warn(fs_info,
>   	"balance: cannot set exclusive op status, resume manually");
>   
>