On 2018/11/19 下午7:13, Filipe Manana wrote:
> On Mon, Nov 19, 2018 at 11:09 AM Qu Wenruo <quwenruo.bt...@gmx.com> wrote:
>>
>>
>>
>> On 2018/11/19 下午5: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()
>>
>> 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 <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;
>>
>> 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?
>
>>
>> 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.
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;
>>>
>>
signature.asc
Description: OpenPGP digital signature
