Re: [PATCH] btrfs: ioctl: fix inaccurate determination of exclusive_operation

[David Sterba]

On Tue, Mar 28, 2023 at 05:43:35AM -0400, xiaoshoukui wrote:
> > Have you found some bug with the above or is there other combination of
> > the exclusive operations that should not work? The changes to the state
> > values are the same, besides the wrong locking.
> 
> Yes, there is a racy bewteen btrfs_exclop_balance and btrfs_exclop_finish
> in btrfs_ioctl_add_dev, when cocurrently adding multiple devices to the
> same mnt point. That will cause the assertion in btrfs_exclop_balance to fail.
> 
> > void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
> > 			  enum btrfs_exclusive_operation op)
> > {
> > 	switch (op) {
> > 	case BTRFS_EXCLOP_BALANCE_PAUSED:
> > 		spin_lock(&fs_info->super_lock);
> > 		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
> > 		       fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD);
> 
> when btrfs_exclop_finish function was executed before the ASSERT, the
> fs_info->exclusive_operation will change to BTRFS_EXCLOP_NONE. So this
> assert will failed.
>
> Please review whether we should patch the assert to add BTRFS_EXCLOP_NONE condtion.
> I'll post a patch if needed. thx.

Yeah I think the assertion should also check for NONE status. The paused
balance makes the state tracking harder but in user-started (manual or
scripted) commands it's typically not racing.

btrfs_exclop_start_try_lock does not allow to do the change from
none -> op mandating an explicit btrfs_exclop_start first but the
assertions do not care about that.