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

[Josef Bacik <josef@toxicpanda.com>]

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