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

Goldwyn Rodrigues
2021-11-02T17:25:33Z

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