| Message ID | 20230324031611.98986-1-xiaoshoukui@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: ioctl: fix inaccurate determination of exclusive_operation | expand |
On Thu, Mar 23, 2023 at 11:16:11PM -0400, xiaoshoukui wrote: > with fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD enter > btrfs_ioctl_add_dev function , exclusive_operation will be classified > as in paused balance operation. After return from btrfs_ioctl_add_dev, > exclusive_operation will be restore to BTRFS_EXCLOP_BALANCE_PAUSED which > is not its original state. Sorry, I don't understand what you mean. The paused balance and 'device add' are supposed to be compatible exclusive operations (see commit a174c0a2e857 ("btrfs: allow device add if balance is paused")). 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. > Signed-off-by: xiaoshoukui <xiaoshoukui@ruijie.com.cn> > --- > fs/btrfs/ioctl.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index a0ef1a1784c7..aab5fdb9445c 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2629,7 +2629,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg) > } > > if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD)) { > - if (!btrfs_exclop_start_try_lock(fs_info, BTRFS_EXCLOP_DEV_ADD)) > + if (fs_info->exclusive_operation != BTRFS_EXCLOP_BALANCE_PAUSED) This is removing the atomicity of the check so it's racy and could forcibly overwrite the exclusive operation to BTRFS_EXCLOP_DEV_ADD without the protecting the whole critical section. > return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS; > > /* > @@ -2637,8 +2637,9 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg) > * change the exclusive op type and remember we should bring > * back the paused balance > */ > + spin_lock(&fs_info->super_lock); What if there's another exclusive operation started before this lock is taken? > fs_info->exclusive_operation = BTRFS_EXCLOP_DEV_ADD; > - btrfs_exclop_start_unlock(fs_info); > + spin_unlock(&fs_info->super_lock); > restore_op = true; > }
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a0ef1a1784c7..aab5fdb9445c 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2629,7 +2629,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg) } if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD)) { - if (!btrfs_exclop_start_try_lock(fs_info, BTRFS_EXCLOP_DEV_ADD)) + if (fs_info->exclusive_operation != BTRFS_EXCLOP_BALANCE_PAUSED) return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS; /* @@ -2637,8 +2637,9 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg) * change the exclusive op type and remember we should bring * back the paused balance */ + spin_lock(&fs_info->super_lock); fs_info->exclusive_operation = BTRFS_EXCLOP_DEV_ADD; - btrfs_exclop_start_unlock(fs_info); + spin_unlock(&fs_info->super_lock); restore_op = true; }
with fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD enter btrfs_ioctl_add_dev function , exclusive_operation will be classified as in paused balance operation. After return from btrfs_ioctl_add_dev, exclusive_operation will be restore to BTRFS_EXCLOP_BALANCE_PAUSED which is not its original state. Signed-off-by: xiaoshoukui <xiaoshoukui@ruijie.com.cn> --- fs/btrfs/ioctl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)