Messages in this thread | | | Date | Fri, 31 Mar 2023 19:45:33 +0200 | From | David Sterba <> | Subject | Re: [PATCH] btrfs: ioctl: fix inaccurate determination of exclusive_operation |
| |
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.
| |