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

[Nikolay Borisov <nborisov@suse.com>]

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
>