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

Josef Bacik
2021-11-02T14:30:52Z

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