[PATCH v2 0/3] x Balance vs device add fixes

[PATCH v2 0/3] x Balance vs device add fixes

[Nikolay Borisov]

Here's v2 of the patchset allowinig to add a device if we have paused balanced.

Changes in v2:

 * Patch 1 now contains a btrfs_exclop_pause_balance() helper which reduces
 some code duplication and encapuslates ASSERTS

 * Also in patch 1 balance pause is now handled in btrfs_balance and not in the
 caller of btrfs_balance. The original code was buggy due to my misunderstanding
 when need_unlock code is executed in btrfs_ioctl_balance()

* Patch 3 now uses btrfs_exclop_pause_balance instead of duplicating code.

Nikolay Borisov (3):
  btrfs: introduce BTRFS_EXCLOP_BALANCE_PAUSED exclusive state
  btrfs: make device add compatible with paused balance in
    btrfs_exclop_start_try_lock
  btrfs: allow device add if balance is paused

 fs/btrfs/ctree.h   |  3 +++
 fs/btrfs/ioctl.c   | 39 +++++++++++++++++++++++++++++++++++----
 fs/btrfs/volumes.c | 11 +++++++++--
 3 files changed, 47 insertions(+), 6 deletions(-)

--
2.25.1

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

[Nikolay Borisov]

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);
+}
+
 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;
+				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");
 
-- 
2.25.1

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

[Josef Bacik]

On Mon, Nov 08, 2021 at 04:28:18PM +0200, 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);
> +}
> +
>  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;
> +				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;

Unused variable.  Thanks,

Josef

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

[Nikolay Borisov]

On 8.11.21 г. 16:57, Josef Bacik wrote:
> +	bool paused = false;


Argh, I thought I removed it .... David unless there are more changes
requested can you remove it during merge or I can resend as well.

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

[Anand Jain]

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");
>   
> 

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

[Nikolay Borisov]

<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) {

    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>

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

[Anand Jain]

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) {
> 
>      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.

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

[Nikolay Borisov]

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.

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

[David Sterba]

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.

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

[David Sterba]

On Tue, Nov 09, 2021 at 07:35:40PM +0800, Anand Jain wrote:
> 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;
> 	}
> ::
>   }

I don't see this comment answered, I think it's a good point to do the
exclusive state change and assertions in the helper, there's too much
code repetition.

[PATCH v2 2/3] btrfs: make device add compatible with paused balance in btrfs_exclop_start_try_lock

[Nikolay Borisov]

This is needed to enable device add to work in cases when a file system
has been mounted with 'skip_balance' mount option.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ioctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f4c05a9aab84..ad460dc38786 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -387,6 +387,7 @@ bool btrfs_exclop_start(struct btrfs_fs_info *fs_info,
  *
  * Compatibility:
  * - the same type is already running
+ * - when trying to add a device and balance has been paused
  * - not BTRFS_EXCLOP_NONE - this is intentionally incompatible and the caller
  *   must check the condition first that would allow none -> @type
  */
@@ -394,7 +395,9 @@ bool btrfs_exclop_start_try_lock(struct btrfs_fs_info *fs_info,
 				 enum btrfs_exclusive_operation type)
 {
 	spin_lock(&fs_info->super_lock);
-	if (fs_info->exclusive_operation == type)
+	if (fs_info->exclusive_operation == type ||
+	    (fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE_PAUSED &&
+	     type == BTRFS_EXCLOP_DEV_ADD))
 		return true;
 
 	spin_unlock(&fs_info->super_lock);
-- 
2.25.1

Re: [PATCH v2 2/3] btrfs: make device add compatible with paused balance in btrfs_exclop_start_try_lock

[Josef Bacik]

On Mon, Nov 08, 2021 at 04:28:19PM +0200, Nikolay Borisov wrote:
> This is needed to enable device add to work in cases when a file system
> has been mounted with 'skip_balance' mount option.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

[PATCH v2 3/3] btrfs: allow device add if balance is paused

[Nikolay Borisov]

Currently paused balance precludes adding a device since they are both
considered exclusive ops and we can have at most 1 running at a time.
This is problematic in case a filesystem encounters an ENOSPC situation
while balance is running, in this case the only thing the user can do
is mount the fs with "skip_balance" which pauses balance and delete some
data to free up space for balance. However, it should be possible to add
a new device when balance is paused.

Fix this by allowing device add to proceed when balance is paused.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ioctl.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ad460dc38786..0e77b538a17c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3159,13 +3159,25 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
 static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 {
 	struct btrfs_ioctl_vol_args *vol_args;
+	bool restore_op = false;
 	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD))
-		return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD)) {
+		if (!btrfs_exclop_start_try_lock(fs_info, BTRFS_EXCLOP_DEV_ADD))
+			return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
+
+		/*
+		 * We can do the device add because we have a paused balanced,
+		 * change the exclusive op type and remember we should bring
+		 * back the paused balance
+		 */
+		fs_info->exclusive_operation = BTRFS_EXCLOP_DEV_ADD;
+		btrfs_exclop_start_unlock(fs_info);
+		restore_op = true;
+	}
 
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args)) {
@@ -3181,7 +3193,10 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 
 	kfree(vol_args);
 out:
-	btrfs_exclop_finish(fs_info);
+	if (restore_op)
+		btrfs_exclop_pause_balance(fs_info);
+	else
+		btrfs_exclop_finish(fs_info);
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v2 3/3] btrfs: allow device add if balance is paused

[Josef Bacik]

On Mon, Nov 08, 2021 at 04:28:20PM +0200, Nikolay Borisov wrote:
> Currently paused balance precludes adding a device since they are both
> considered exclusive ops and we can have at most 1 running at a time.
> This is problematic in case a filesystem encounters an ENOSPC situation
> while balance is running, in this case the only thing the user can do
> is mount the fs with "skip_balance" which pauses balance and delete some
> data to free up space for balance. However, it should be possible to add
> a new device when balance is paused.
> 
> Fix this by allowing device add to proceed when balance is paused.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v2 0/3] x Balance vs device add fixes

[David Sterba]

On Mon, Nov 08, 2021 at 04:28:17PM +0200, Nikolay Borisov wrote:
> Here's v2 of the patchset allowinig to add a device if we have paused balanced.

I haven't figred out any problematic state. As the paused balance is
a note about the request filters and progress, restarting balance with a
new device will apply accordingly. The result will be of course
affected, eg. chunk layout or allocation order, but other than that I
think it's fine.

One remaining thing is to teach progs that there's a new state and how
it's compatible. This needs to mimic the logic in kernel. As it's a
simple change it can go to any release before the kernel changes get
released.