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

From: Goldwyn Rodrigues <rgoldwyn@suse.de>
To: Nikolay Borisov <nborisov@suse.com>
Cc: Josef Bacik <josef@toxicpanda.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 0/3] Balance vs device add fixes
Date: Tue, 2 Nov 2021 12:25:28 -0500	[thread overview]
Message-ID: <20211102172528.se6voh75geqjjltf@zuko> (raw)
In-Reply-To: <516c7eaf-3fb2-fe61-08f8-ac4201752121@suse.com>

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