Re: [PATCH v2 1/3] btrfs: introduce BTRFS_EXCLOP_BALANCE_PAUSED exclusive state

[David Sterba <dsterba@suse.cz>]

On Tue, Nov 09, 2021 at 07:35:40PM +0800, Anand Jain wrote:
> On 8/11/21 10:28 pm, Nikolay Borisov wrote:
> > Current set of exclusive operation states is not sufficient to handle all
> > practical use cases. In particular there is a need to be able to add a
> > device to a filesystem that have paused balance. Currently there is no
> > way to distinguish between a running and a paused balance. Fix this by
> > introducing BTRFS_EXCLOP_BALANCE_PAUSED which is going to be set in 2
> > occasions:
> > 
> > 1. When a filesystem is mounted with skip_balance and there is an
> >     unfinished balance it will now be into BALANCE_PAUSED instead of
> >     simply BALANCE state.
> > 
> > 2. When a running balance is paused.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> >   fs/btrfs/ctree.h   |  3 +++
> >   fs/btrfs/ioctl.c   | 13 +++++++++++++
> >   fs/btrfs/volumes.c | 11 +++++++++--
> >   3 files changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 7553e9dc5f93..8376271bfed1 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -613,6 +613,7 @@ enum {
> >    */
> >   enum btrfs_exclusive_operation {
> >   	BTRFS_EXCLOP_NONE,
> > +	BTRFS_EXCLOP_BALANCE_PAUSED,
> >   	BTRFS_EXCLOP_BALANCE,
> >   	BTRFS_EXCLOP_DEV_ADD,
> >   	BTRFS_EXCLOP_DEV_REMOVE,
> > @@ -3305,6 +3306,8 @@ bool btrfs_exclop_start_try_lock(struct btrfs_fs_info *fs_info,
> >   				 enum btrfs_exclusive_operation type);
> >   void btrfs_exclop_start_unlock(struct btrfs_fs_info *fs_info);
> >   void btrfs_exclop_finish(struct btrfs_fs_info *fs_info);
> > +void btrfs_exclop_pause_balance(struct btrfs_fs_info *fs_info);
> > +
> >   
> >   /* file.c */
> >   int __init btrfs_auto_defrag_init(void);
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 92424a22d8d6..f4c05a9aab84 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -414,6 +414,15 @@ void btrfs_exclop_finish(struct btrfs_fs_info *fs_info)
> >   	sysfs_notify(&fs_info->fs_devices->fsid_kobj, NULL, "exclusive_operation");
> >   }
> > 
> 
> 
> 
> > +void btrfs_exclop_pause_balance(struct btrfs_fs_info *fs_info)
> > +{
> > +	spin_lock(&fs_info->super_lock);
> > +	ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
> > +	       fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD);
> > +	fs_info->exclusive_operation = BTRFS_EXCLOP_BALANCE_PAUSED;
> > +	spin_unlock(&fs_info->super_lock);
> > +}
> > +
> 
> This function can be more generic and replace its open coded version
> in a few places.
> 
>   btrfs_exclop_balance(fs_info, exclop)
>   {
> ::
> 	switch(exclop)
> 	{
> 		case BTRFS_EXCLOP_BALANCE_PAUSED:
> 	  		ASSERT(fs_info->exclusive_operation ==
> 				BTRFS_EXCLOP_BALANCE ||
>                   		fs_info->exclusive_operation ==
> 				BTRFS_EXCLOP_DEV_ADD);
> 			break;
> 		case BTRFS_EXCLOP_BALANCE:
> 			ASSERT(fs_info->exclusive_operation ==
> 				BTRFS_EXCLOP_BALANCE_PAUSED);
> 			break;
> 	}
> ::
>   }

I don't see this comment answered, I think it's a good point to do the
exclusive state change and assertions in the helper, there's too much
code repetition.