On 19.11.18 г. 11:48 ч., fdman...@kernel.org wrote:
> From: Filipe Manana <fdman...@suse.com>
>
> If the quota enable and snapshot creation ioctls are called concurrently
> we can get into a deadlock where the task enabling quotas will deadlock
> on the fs_info->qgroup_ioctl_lock mutex because it attempts to lock it
> twice. The following time diagram shows how this happens.
>
> CPU 0 CPU 1
>
> btrfs_ioctl()
> btrfs_ioctl_quota_ctl()
> btrfs_quota_enable()
> mutex_lock(fs_info->qgroup_ioctl_lock)
> btrfs_start_transaction()
>
> btrfs_ioctl()
> btrfs_ioctl_snap_create_v2
> create_snapshot()
> --> adds snapshot to the
> list pending_snapshots
> of the current
> transaction
>
> btrfs_commit_transaction()
> create_pending_snapshots()
> create_pending_snapshot()
> qgroup_account_snapshot()
> btrfs_qgroup_inherit()
> mutex_lock(fs_info->qgroup_ioctl_lock)
> --> deadlock, mutex already locked
> by this task at
> btrfs_quota_enable()
>
> So fix this by adding a flag to the transaction handle that signals if the
> transaction is being used for enabling quotas (only seen by the task doing
> it) and do not lock the mutex qgroup_ioctl_lock at btrfs_qgroup_inherit()
> if the transaction handle corresponds to the one being used to enable the
> quotas.
>
> Fixes: 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating
> snapshot")
> Signed-off-by: Filipe Manana <fdman...@suse.com>
> ---
> fs/btrfs/qgroup.c | 10 ++++++++--
> fs/btrfs/transaction.h | 1 +
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index d4917c0cddf5..3aec3bfa3d70 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -908,6 +908,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> trans = NULL;
> goto out;
> }
> + trans->enabling_quotas = true;
>
> fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
> if (!fs_info->qgroup_ulist) {
> @@ -2250,7 +2251,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle
> *trans, u64 srcid,
> u32 level_size = 0;
> u64 nums;
>
> - mutex_lock(&fs_info->qgroup_ioctl_lock);
> + if (trans->enabling_quotas)
> + lockdep_assert_held(&fs_info->qgroup_ioctl_lock);
> + else
> + mutex_lock(&fs_info->qgroup_ioctl_lock);
> +
nit: That's a bit ugly for my taste, but I don't think the
alternative is any better:
ASSERT((trans->enabling_quotas &&
!lockdep_assert_held(&fs_info->qgroup_ioctl_lock)) || !trans->enabling_quotas)
if (!trans->enabling_quotas)
mutex_lock(...)
> if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
> goto out;
>
> @@ -2413,7 +2418,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle
> *trans, u64 srcid,
> unlock:
> spin_unlock(&fs_info->qgroup_lock);
> out:
> - mutex_unlock(&fs_info->qgroup_ioctl_lock);
> + if (!trans->enabling_quotas)
> + mutex_unlock(&fs_info->qgroup_ioctl_lock);
> return ret;
> }
>
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index 703d5116a2fc..a5553a1dee30 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -122,6 +122,7 @@ struct btrfs_trans_handle {
> bool reloc_reserved;
> bool sync;
> bool dirty;
> + bool enabling_quotas;
> struct btrfs_root *root;
> struct btrfs_fs_info *fs_info;
> struct list_head new_bgs;
>
