Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6165 bytes --]



On 2018/11/19 下午7:52, Filipe Manana wrote:
> On Mon, Nov 19, 2018 at 11:35 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2018/11/19 下午7:13, Filipe Manana wrote:
>>> On Mon, Nov 19, 2018 at 11:09 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2018/11/19 下午5:48, fdmanana@kernel.org wrote:
>>>>> From: Filipe Manana <fdmanana@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()
>>>>
>>>> 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@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;
>>>>
>>>> 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.
>>
>> But it's the btrfs_qgroup_inherit() using the trans handler to avoid
>> dead lock.
>>
>> What makes sure btrfs_qgroup_inherit() get the exactly same trans
>> handler allocated here?
> 
> If it's the other task (the one creating a snapshot) that starts the
> transaction commit,
> it will have to wait for the task enabling quotas to release the
> transaction - once that task
> also calls commit_transaction(), it will skip doing the commit itself
> and wait for the snapshot
> one to finish the commit, while holding the qgroup mutex (this part I
> missed before).
> So yes we'll have to use a bit in the transaction itself instead.
> 
>>
>>>
>>>>
>>>> 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.
>>
>> Anyway, current naming doesn't really show why we could skip mutex
>> locking. Just hope to get some name better.
> 
> No name will ever show you that.
> You'll always have to see where  and how it's used, unless you want a
> name like "dont_lock_mutex_because_we_locked_it_at_btrfs...".

(Personally speaking I indeed prefer this one naming as it doesn't
exceed 80 chars yet)

Your statement makes sense, just keep current naming.

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>>
>>>
>>>>
>>>> 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;
>>>>>
>>>>
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]