On Mon, Nov 19, 2018 at 11:09 AM Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote:
>
>
>
> On 2018/11/19 下午5:48, fdmanana@xxxxxxxxxx wrote:
> > From: Filipe Manana <fdmanana@xxxxxxxx>
> >
> > 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()
>
> The backtrace looks valid.
>
> >
> > 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 <fdmanana@xxxxxxxx>
> > ---
> > 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;
>
> Should we put enabling_quotas bit into btrfs_transaction instead of
> btrfs_trans_handle?
Why?
Only the task which is enabling quotas needs to know about it.
>
> Isn't it possible to have different trans handle pointed to the same
> transaction?
Yes.
>
> And I'm not really sure about the naming "enabling_quotas".
> What about "quota_ioctl_mutex_hold"? (Well, this also sounds awful)
Too long.
>
> Thanks,
> Qu
>
> >
> > 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);
> > +
> > 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;
> >
>
