[PATCH 0/3] Balance vs device add fixes

[PATCH 0/3] Balance vs device add fixes

[Nikolay Borisov]

This series enables adding of a device when balance is paused (i.e an fs is mounted
with skip_balance options). This is needed to give users a chance to gracefully
handle an ENOSPC situation in the face of running balance. To achieve this introduce
a new exclop - BALANCE_PAUSED which is made compatible with device add. More
details in each patche.

I've tested this with an fstests which I will be posting in a bit.

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   |  1 +
 fs/btrfs/ioctl.c   | 49 +++++++++++++++++++++++++++++++++++++++-------
 fs/btrfs/volumes.c | 23 ++++++++++++++++++----
 fs/btrfs/volumes.h |  2 +-
 4 files changed, 63 insertions(+), 12 deletions(-)

--
2.25.1

[PATCH 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   |  1 +
 fs/btrfs/ioctl.c   | 20 +++++++++++++++++---
 fs/btrfs/volumes.c | 23 +++++++++++++++++++----
 fs/btrfs/volumes.h |  2 +-
 4 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 7553e9dc5f93..b1916fa548b6 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,
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index c474f7e24163..4aa6191889a3 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3982,6 +3982,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	struct btrfs_ioctl_balance_args *bargs;
 	struct btrfs_balance_control *bctl;
+	bool paused = false;
 	bool need_unlock; /* for mut. excl. ops lock */
 	int ret;
 
@@ -4019,6 +4020,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;
 			}
@@ -4100,7 +4105,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 	 */
 	need_unlock = false;
 
-	ret = btrfs_balance(fs_info, bctl, bargs);
+	ret = btrfs_balance(fs_info, bctl, bargs, &paused);
 	bctl = NULL;
 
 	if ((ret == 0 || ret == -ECANCELED) && arg) {
@@ -4114,8 +4119,17 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 	kfree(bargs);
 out_unlock:
 	mutex_unlock(&fs_info->balance_mutex);
-	if (need_unlock)
-		btrfs_exclop_finish(fs_info);
+	if (need_unlock) {
+		if (paused) {
+			/* Transition directly to BALANCE_PAUSED */
+			spin_lock(&fs_info->super_lock);
+			ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE);
+			fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE_PAUSED;
+			spin_unlock(&fs_info->super_lock);
+		} else {
+			btrfs_exclop_finish(fs_info);
+		}
+	}
 out:
 	mnt_drop_write_file(file);
 	return ret;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 546bf1146b2d..593583307f55 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4219,7 +4219,7 @@ static void describe_balance_start_or_resume(struct btrfs_fs_info *fs_info)
  */
 int btrfs_balance(struct btrfs_fs_info *fs_info,
 		  struct btrfs_balance_control *bctl,
-		  struct btrfs_ioctl_balance_args *bargs)
+		  struct btrfs_ioctl_balance_args *bargs, bool *paused)
 {
 	u64 meta_target, data_target;
 	u64 allowed;
@@ -4355,8 +4355,11 @@ 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");
+		if (paused)
+			*paused = true;
+	}
 	/*
 	 * Balance can be canceled by:
 	 *
@@ -4406,13 +4409,21 @@ 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);
 	if (fs_info->balance_ctl)
-		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL);
+		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL, &paused);
 	mutex_unlock(&fs_info->balance_mutex);
 
+	if (paused) {
+		spin_lock(&fs_info->super_lock);
+		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE);
+		fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE_PAUSED;
+		spin_unlock(&fs_info->super_lock);
+	}
+
 	return ret;
 }
 
@@ -4432,6 +4443,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 +4515,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");
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 3b8130680749..babcf447104b 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -541,7 +541,7 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
 int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_balance(struct btrfs_fs_info *fs_info,
 		  struct btrfs_balance_control *bctl,
-		  struct btrfs_ioctl_balance_args *bargs);
+		  struct btrfs_ioctl_balance_args *bargs, bool *paused);
 void btrfs_describe_block_groups(u64 flags, char *buf, u32 size_buf);
 int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info);
 int btrfs_recover_balance(struct btrfs_fs_info *fs_info);
-- 
2.25.1

[PATCH 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 4aa6191889a3..05969e2f4536 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -386,6 +386,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
  */
@@ -393,7 +394,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

[PATCH 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 | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 05969e2f4536..8e34dceee96b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3149,13 +3149,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)) {
@@ -3171,7 +3183,13 @@ 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) {
+		spin_lock(&fs_info->super_lock);
+		fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE_PAUSED;
+		spin_unlock(&fs_info->super_lock);
+	} else {
+		btrfs_exclop_finish(fs_info);
+	}
 	return ret;
 }
 
-- 
2.25.1

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

[Anand Jain]

On 01/11/2021 19:53, Nikolay Borisov wrote:
> This series enables adding of a device when balance is paused (i.e an fs is mounted
> with skip_balance options). This is needed to give users a chance to gracefully
> handle an ENOSPC situation in the face of running balance. To achieve this introduce
> a new exclop - BALANCE_PAUSED which is made compatible with device add. More
> details in each patche.
> 

Have you thought about allowing the device-add during the 
balance-running, not only at the balance-paused? Is it much complicated? 
If not, then we could drop the new exclop state BALANCE_PAUSED altogether.

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

[Josef Bacik]

On Tue, Nov 02, 2021 at 12:52:11PM +0800, Anand Jain wrote:
> On 01/11/2021 19:53, Nikolay Borisov wrote:
> > This series enables adding of a device when balance is paused (i.e an fs is mounted
> > with skip_balance options). This is needed to give users a chance to gracefully
> > handle an ENOSPC situation in the face of running balance. To achieve this introduce
> > a new exclop - BALANCE_PAUSED which is made compatible with device add. More
> > details in each patche.
> > 
> 
> Have you thought about allowing the device-add during the balance-running,
> not only at the balance-paused? Is it much complicated? If not, then we
> could drop the new exclop state BALANCE_PAUSED altogether.
> 

We could be changing the raid settings, adding chunks before the new device is
added and then ending up with chunks that don't have all of their stripes.  The
number of bugs that could show up is scary, I think this is the safer choice for
now.  Thanks,

Josef

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

[Josef Bacik]

On Mon, Nov 01, 2021 at 01:53:21PM +0200, Nikolay Borisov wrote:
> This series enables adding of a device when balance is paused (i.e an fs is mounted
> with skip_balance options). This is needed to give users a chance to gracefully
> handle an ENOSPC situation in the face of running balance. To achieve this introduce
> a new exclop - BALANCE_PAUSED which is made compatible with device add. More
> details in each patche.
> 
> I've tested this with an fstests which I will be posting in a bit.
> 
> 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   |  1 +
>  fs/btrfs/ioctl.c   | 49 +++++++++++++++++++++++++++++++++++++++-------
>  fs/btrfs/volumes.c | 23 ++++++++++++++++++----
>  fs/btrfs/volumes.h |  2 +-
>  4 files changed, 63 insertions(+), 12 deletions(-)
> 

A few things

1) Can we integrate the flipping into helpers?  Something like

	btrfs_exclop_change_state(PAUSED);

   So the locking and stuff is all with the code that messes with the exclop?

2) The existing helpers do WRITE_ONCE(), is that needed here?  I assume not
   because we're not actually exiting our exclop state, but still seems wonky.

3) Maybe have an __btrfs_exclop_finish(type), so instead of 

	if (paused) {
		do thing;
	} else {
		btrfs_exclop_finish();
	}

  you can instead do

	type = BTRFS_EXCLOP_NONE;
	if (pause stuff) {
		do things;
		type = BTRFS_EXCLOP_BALANCE_PAUSED;
	}

	/* other stuff. */
	__btrfs_exclop_finish(type);

then btrfs_exclop_finish just does __btrfs_exclop_finish(NONE);

Thanks,

Josef

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

[Nikolay Borisov]

On 2.11.21 г. 16:30, Josef Bacik wrote:
> On Mon, Nov 01, 2021 at 01:53:21PM +0200, Nikolay Borisov wrote:
>> This series enables adding of a device when balance is paused (i.e an fs is mounted
>> with skip_balance options). This is needed to give users a chance to gracefully
>> handle an ENOSPC situation in the face of running balance. To achieve this introduce
>> a new exclop - BALANCE_PAUSED which is made compatible with device add. More
>> details in each patche.
>>
>> I've tested this with an fstests which I will be posting in a bit.
>>
>> 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   |  1 +
>>  fs/btrfs/ioctl.c   | 49 +++++++++++++++++++++++++++++++++++++++-------
>>  fs/btrfs/volumes.c | 23 ++++++++++++++++++----
>>  fs/btrfs/volumes.h |  2 +-
>>  4 files changed, 63 insertions(+), 12 deletions(-)
>>
> 
> A few things
> 
> 1) Can we integrate the flipping into helpers?  Something like
> 
> 	btrfs_exclop_change_state(PAUSED);
> 
>    So the locking and stuff is all with the code that messes with the exclop?

Right, I left the code flipping balance->paused opencoded because that's
really a special case. By all means I can add a specific helper so that
the ASSERT is not lost as well. The reason I didn't do it in the first
place is because PAUSED is really "special" in the sense it can be
entered only from BALANCE and it's not really generic. If you take a
look how btrfs_exclop_start does it for example, it simply checks we
don't have a running op and simply sets it to whatever is passed

> 
> 2) The existing helpers do WRITE_ONCE(), is that needed here?  I assume not>    because we're not actually exiting our exclop state, but still
seems wonky.

That got me thinking in the first place and actually initially I had a
patch which removed it. However, I *think* it might be required since
exclusive_operation is accessed without a lock ini the sysfs code i.e.
btrfs_exclusive_operation_show so I guess that's why we need it.

Goldwyn, what's your take on this?

> 
> 3) Maybe have an __btrfs_exclop_finish(type), so instead of 
> 
> 	if (paused) {
> 		do thing;
> 	} else {
> 		btrfs_exclop_finish();
> 	}
> 
>   you can instead do
> 
> 	type = BTRFS_EXCLOP_NONE;
> 	if (pause stuff) {
> 		do things;
> 		type = BTRFS_EXCLOP_BALANCE_PAUSED;
> 	}
> 
> 	/* other stuff. */
> 	__btrfs_exclop_finish(type);
> 
> then btrfs_exclop_finish just does __btrfs_exclop_finish(NONE);

I'm having a hard time seeing how this would increase readability. What
should go into the __btrfs_exclop_finish function?


> Thanks,
> 
> Josef
> 

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

[Josef Bacik]

On Tue, Nov 02, 2021 at 05:25:32PM +0200, Nikolay Borisov wrote:
> 
> 
> On 2.11.21 г. 16:30, Josef Bacik wrote:
> > On Mon, Nov 01, 2021 at 01:53:21PM +0200, Nikolay Borisov wrote:
> >> This series enables adding of a device when balance is paused (i.e an fs is mounted
> >> with skip_balance options). This is needed to give users a chance to gracefully
> >> handle an ENOSPC situation in the face of running balance. To achieve this introduce
> >> a new exclop - BALANCE_PAUSED which is made compatible with device add. More
> >> details in each patche.
> >>
> >> I've tested this with an fstests which I will be posting in a bit.
> >>
> >> 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   |  1 +
> >>  fs/btrfs/ioctl.c   | 49 +++++++++++++++++++++++++++++++++++++++-------
> >>  fs/btrfs/volumes.c | 23 ++++++++++++++++++----
> >>  fs/btrfs/volumes.h |  2 +-
> >>  4 files changed, 63 insertions(+), 12 deletions(-)
> >>
> > 
> > A few things
> > 
> > 1) Can we integrate the flipping into helpers?  Something like
> > 
> > 	btrfs_exclop_change_state(PAUSED);
> > 
> >    So the locking and stuff is all with the code that messes with the exclop?
> 
> Right, I left the code flipping balance->paused opencoded because that's
> really a special case. By all means I can add a specific helper so that
> the ASSERT is not lost as well. The reason I didn't do it in the first
> place is because PAUSED is really "special" in the sense it can be
> entered only from BALANCE and it's not really generic. If you take a
> look how btrfs_exclop_start does it for example, it simply checks we
> don't have a running op and simply sets it to whatever is passed
> 
> > 
> > 2) The existing helpers do WRITE_ONCE(), is that needed here?  I assume not>    because we're not actually exiting our exclop state, but still
> seems wonky.
> 
> That got me thinking in the first place and actually initially I had a
> patch which removed it. However, I *think* it might be required since
> exclusive_operation is accessed without a lock ini the sysfs code i.e.
> btrfs_exclusive_operation_show so I guess that's why we need it.
> 
> Goldwyn, what's your take on this?
> 
> > 
> > 3) Maybe have an __btrfs_exclop_finish(type), so instead of 
> > 
> > 	if (paused) {
> > 		do thing;
> > 	} else {
> > 		btrfs_exclop_finish();
> > 	}
> > 
> >   you can instead do
> > 
> > 	type = BTRFS_EXCLOP_NONE;
> > 	if (pause stuff) {
> > 		do things;
> > 		type = BTRFS_EXCLOP_BALANCE_PAUSED;
> > 	}
> > 
> > 	/* other stuff. */
> > 	__btrfs_exclop_finish(type);
> > 
> > then btrfs_exclop_finish just does __btrfs_exclop_finish(NONE);
> 
> I'm having a hard time seeing how this would increase readability. What
> should go into the __btrfs_exclop_finish function?
> 

btrfs_exclop_finish would become __btrfs_exclop_finish(type) and do all the
work, but instead of setting NONE it would set type.

Honestly I could go either way, having a helper would make it more readable than
it is, because then its

if (pause)
	btrfs_exclop_pause();
else
	btrfs_exclop_finish();

I'm not strong on this, I think having a helper instead of open coding helps
given the number of places it's used.  Perhaps just doing that step will make it
clean enough.  Thanks,

Josef

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

[Goldwyn Rodrigues]

On 17:25 02/11, Nikolay Borisov wrote:
> 
> 
> On 2.11.21 г. 16:30, Josef Bacik wrote:
> > On Mon, Nov 01, 2021 at 01:53:21PM +0200, Nikolay Borisov wrote:
> >> This series enables adding of a device when balance is paused (i.e an fs is mounted
> >> with skip_balance options). This is needed to give users a chance to gracefully
> >> handle an ENOSPC situation in the face of running balance. To achieve this introduce
> >> a new exclop - BALANCE_PAUSED which is made compatible with device add. More
> >> details in each patche.
> >>
> >> I've tested this with an fstests which I will be posting in a bit.
> >>
> >> 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   |  1 +
> >>  fs/btrfs/ioctl.c   | 49 +++++++++++++++++++++++++++++++++++++++-------
> >>  fs/btrfs/volumes.c | 23 ++++++++++++++++++----
> >>  fs/btrfs/volumes.h |  2 +-
> >>  4 files changed, 63 insertions(+), 12 deletions(-)
> >>
> > 
> > A few things
> > 
> > 1) Can we integrate the flipping into helpers?  Something like
> > 
> > 	btrfs_exclop_change_state(PAUSED);
> > 
> >    So the locking and stuff is all with the code that messes with the exclop?
> 
> Right, I left the code flipping balance->paused opencoded because that's
> really a special case. By all means I can add a specific helper so that
> the ASSERT is not lost as well. The reason I didn't do it in the first
> place is because PAUSED is really "special" in the sense it can be
> entered only from BALANCE and it's not really generic. If you take a
> look how btrfs_exclop_start does it for example, it simply checks we
> don't have a running op and simply sets it to whatever is passed
> 
> > 
> > 2) The existing helpers do WRITE_ONCE(), is that needed here?  I assume not>    because we're not actually exiting our exclop state, but still
> seems wonky.
> 
> That got me thinking in the first place and actually initially I had a
> patch which removed it. However, I *think* it might be required since
> exclusive_operation is accessed without a lock ini the sysfs code i.e.
> btrfs_exclusive_operation_show so I guess that's why we need it.
> 
> Goldwyn, what's your take on this?

Yes, btrfs_exclusive_operation_show() does not lock so it would deem
necessary.

But do we really need to use *_ONCE, assuming btrfs_exclusive_operation
fits in 8 bits?


-- 
Goldwyn

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

[Nikolay Borisov]

On 2.11.21 г. 19:25, Goldwyn Rodrigues wrote:
> But do we really need to use *_ONCE, assuming btrfs_exclusive_operation
> fits in 8 bits?
> 


The way I understand it based on the LWN articles is that the effect of
_ONCE is twofold:

1. It prevents theoretical torn writes + forces the compiler to always
issue the access i.e prevent it being optimized out - this could be moot
in our case.

2. It serves a documentation purpose where it states "this variable is
accessed in multithreaded contexts, possibly without an explicit lock" -
and this IMO is quite helpful in this particular context.