* [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
@ 2018-11-19 9:48 fdmanana
2018-11-19 10:07 ` Nikolay Borisov
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: fdmanana @ 2018-11-19 9:48 UTC (permalink / raw)
To: linux-btrfs
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()
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;
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;
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 9:48 [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation fdmanana
@ 2018-11-19 10:07 ` Nikolay Borisov
2018-11-19 11:09 ` Qu Wenruo
2018-11-19 14:15 ` [PATCH v2] " fdmanana
2 siblings, 0 replies; 17+ messages in thread
From: Nikolay Borisov @ 2018-11-19 10:07 UTC (permalink / raw)
To: fdmanana, linux-btrfs
On 19.11.18 г. 11: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()
>
> 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;
>
> 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;
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 9:48 [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation fdmanana
2018-11-19 10:07 ` Nikolay Borisov
@ 2018-11-19 11:09 ` Qu Wenruo
2018-11-19 11:13 ` Filipe Manana
2018-11-19 14:15 ` [PATCH v2] " fdmanana
2 siblings, 1 reply; 17+ messages in thread
From: Qu Wenruo @ 2018-11-19 11:09 UTC (permalink / raw)
To: fdmanana, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 3954 bytes --]
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?
Isn't it possible to have different trans handle pointed to the same
transaction?
And I'm not really sure about the naming "enabling_quotas".
What about "quota_ioctl_mutex_hold"? (Well, this also sounds awful)
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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 11:09 ` Qu Wenruo
@ 2018-11-19 11:13 ` Filipe Manana
2018-11-19 11:35 ` Qu Wenruo
0 siblings, 1 reply; 17+ messages in thread
From: Filipe Manana @ 2018-11-19 11:13 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
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.
>
> 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;
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 11:13 ` Filipe Manana
@ 2018-11-19 11:35 ` Qu Wenruo
2018-11-19 11:52 ` Filipe Manana
0 siblings, 1 reply; 17+ messages in thread
From: Qu Wenruo @ 2018-11-19 11:35 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 4923 bytes --]
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?
>
>>
>> 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;
>>>
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 11:35 ` Qu Wenruo
@ 2018-11-19 11:52 ` Filipe Manana
2018-11-19 12:14 ` Qu Wenruo
2018-11-19 14:15 ` Filipe Manana
0 siblings, 2 replies; 17+ messages in thread
From: Filipe Manana @ 2018-11-19 11:52 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
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...".
>
> 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;
> >>>
> >>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 11:52 ` Filipe Manana
@ 2018-11-19 12:14 ` Qu Wenruo
2018-11-19 14:15 ` Filipe Manana
1 sibling, 0 replies; 17+ messages in thread
From: Qu Wenruo @ 2018-11-19 12:14 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 11:52 ` Filipe Manana
2018-11-19 12:14 ` Qu Wenruo
@ 2018-11-19 14:15 ` Filipe Manana
1 sibling, 0 replies; 17+ messages in thread
From: Filipe Manana @ 2018-11-19 14:15 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Mon, Nov 19, 2018 at 11:52 AM Filipe Manana <fdmanana@kernel.org> 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.
That (using a flag in the transaction itself) wouldn't be good, it would allow
concurrent and unprotected access to qgroup stuff at
btrfs_qgroup_inherit() by anyone who
calls it (currently only subvolume creation). Fortunately there's a
much simpler solution in v2.
>
> >
> > >
> > >>
> > >> 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...".
>
> >
> > 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;
> > >>>
> > >>
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 9:48 [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation fdmanana
2018-11-19 10:07 ` Nikolay Borisov
2018-11-19 11:09 ` Qu Wenruo
@ 2018-11-19 14:15 ` fdmanana
2018-11-19 14:48 ` Qu Wenruo
2 siblings, 1 reply; 17+ messages in thread
From: fdmanana @ 2018-11-19 14:15 UTC (permalink / raw)
To: linux-btrfs
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, or the task creating a snapshot tries to commit the transaction
while the task enabling quota waits for the former task to commit the
transaction while holding the mutex. The following time diagrams show how
both cases happen.
First scenario:
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()
Second scenario:
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()
--> waits for task at
CPU 0 to release
its transaction
handle
btrfs_commit_transaction()
--> sees another task started
the transaction commit first
--> releases its transaction
handle
--> waits for the transaction
commit to be completed by
the task at CPU 1
create_pending_snapshot()
qgroup_account_snapshot()
btrfs_qgroup_inherit()
mutex_lock(fs_info->qgroup_ioctl_lock)
--> deadlock, task at CPU 0
has the mutex locked but
it is waiting for us to
finish the transaction
commit
So fix this by setting the quota enabled flag in fs_info after committing
the transaction at btrfs_quota_enable(). This ends up serializing quota
enable and snapshot creation as if the snapshot creation happened just
before the quota enable request. The quota rescan task, scheduled after
committing the transaction in btrfs_quote_enable(), will do the accounting.
Fixes: 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
V2: Added second deadlock example to changelog and changed the fix to deal
with that case as well.
fs/btrfs/qgroup.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index d4917c0cddf5..ae1358253b7b 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
btrfs_abort_transaction(trans, ret);
goto out_free_path;
}
- spin_lock(&fs_info->qgroup_lock);
- fs_info->quota_root = quota_root;
- set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
- spin_unlock(&fs_info->qgroup_lock);
ret = btrfs_commit_transaction(trans);
trans = NULL;
if (ret)
goto out_free_path;
+ /*
+ * Set quota enabled flag after committing the transaction, to avoid
+ * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
+ * creation.
+ */
+ spin_lock(&fs_info->qgroup_lock);
+ fs_info->quota_root = quota_root;
+ set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
+ spin_unlock(&fs_info->qgroup_lock);
+
ret = qgroup_rescan_init(fs_info, 0, 1);
if (!ret) {
qgroup_rescan_zero_tracking(fs_info);
--
2.11.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 14:15 ` [PATCH v2] " fdmanana
@ 2018-11-19 14:48 ` Qu Wenruo
2018-11-19 15:24 ` Filipe Manana
2018-11-19 15:36 ` Nikolay Borisov
0 siblings, 2 replies; 17+ messages in thread
From: Qu Wenruo @ 2018-11-19 14:48 UTC (permalink / raw)
To: fdmanana, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 6195 bytes --]
On 2018/11/19 下午10:15, 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, or the task creating a snapshot tries to commit the transaction
> while the task enabling quota waits for the former task to commit the
> transaction while holding the mutex. The following time diagrams show how
> both cases happen.
>
> First scenario:
>
> 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()
>
> Second scenario:
>
> 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()
> --> waits for task at
> CPU 0 to release
> its transaction
> handle
>
> btrfs_commit_transaction()
> --> sees another task started
> the transaction commit first
> --> releases its transaction
> handle
> --> waits for the transaction
> commit to be completed by
> the task at CPU 1
>
> create_pending_snapshot()
> qgroup_account_snapshot()
> btrfs_qgroup_inherit()
> mutex_lock(fs_info->qgroup_ioctl_lock)
> --> deadlock, task at CPU 0
> has the mutex locked but
> it is waiting for us to
> finish the transaction
> commit
>
> So fix this by setting the quota enabled flag in fs_info after committing
> the transaction at btrfs_quota_enable(). This ends up serializing quota
> enable and snapshot creation as if the snapshot creation happened just
> before the quota enable request. The quota rescan task, scheduled after
> committing the transaction in btrfs_quote_enable(), will do the accounting.
>
> Fixes: 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>
> V2: Added second deadlock example to changelog and changed the fix to deal
> with that case as well.
>
> fs/btrfs/qgroup.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index d4917c0cddf5..ae1358253b7b 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> btrfs_abort_transaction(trans, ret);
> goto out_free_path;
> }
> - spin_lock(&fs_info->qgroup_lock);
> - fs_info->quota_root = quota_root;
> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> - spin_unlock(&fs_info->qgroup_lock);
>
> ret = btrfs_commit_transaction(trans);
> trans = NULL;
> if (ret)
> goto out_free_path;
The main concern here is, if we don't set qgroup enabled bit before we
commit transaction, there will be a window where new tree modification
could happen before QGROUP_ENABLED bit set.
There may be some qgroup reserved space related problem in such case,
but I'm not 100% sure to foresee such problem.
The best way to do this is, commit trans first, and before any other one
trying to start transaction, we set the bit.
However I can't find such infrastructure now (I still remember we used
to have a pending bit to change quota enabled flag, but removed later)
Thanks,
Qu
>
> + /*
> + * Set quota enabled flag after committing the transaction, to avoid
> + * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
> + * creation.
> + */
> + spin_lock(&fs_info->qgroup_lock);
> + fs_info->quota_root = quota_root;
> + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> + spin_unlock(&fs_info->qgroup_lock);
> +
> ret = qgroup_rescan_init(fs_info, 0, 1);
> if (!ret) {
> qgroup_rescan_zero_tracking(fs_info);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 14:48 ` Qu Wenruo
@ 2018-11-19 15:24 ` Filipe Manana
2018-11-20 0:32 ` Qu Wenruo
2018-11-19 15:36 ` Nikolay Borisov
1 sibling, 1 reply; 17+ messages in thread
From: Filipe Manana @ 2018-11-19 15:24 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Mon, Nov 19, 2018 at 2:48 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2018/11/19 下午10:15, 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, or the task creating a snapshot tries to commit the transaction
> > while the task enabling quota waits for the former task to commit the
> > transaction while holding the mutex. The following time diagrams show how
> > both cases happen.
> >
> > First scenario:
> >
> > 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()
> >
> > Second scenario:
> >
> > 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()
> > --> waits for task at
> > CPU 0 to release
> > its transaction
> > handle
> >
> > btrfs_commit_transaction()
> > --> sees another task started
> > the transaction commit first
> > --> releases its transaction
> > handle
> > --> waits for the transaction
> > commit to be completed by
> > the task at CPU 1
> >
> > create_pending_snapshot()
> > qgroup_account_snapshot()
> > btrfs_qgroup_inherit()
> > mutex_lock(fs_info->qgroup_ioctl_lock)
> > --> deadlock, task at CPU 0
> > has the mutex locked but
> > it is waiting for us to
> > finish the transaction
> > commit
> >
> > So fix this by setting the quota enabled flag in fs_info after committing
> > the transaction at btrfs_quota_enable(). This ends up serializing quota
> > enable and snapshot creation as if the snapshot creation happened just
> > before the quota enable request. The quota rescan task, scheduled after
> > committing the transaction in btrfs_quote_enable(), will do the accounting.
> >
> > Fixes: 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >
> > V2: Added second deadlock example to changelog and changed the fix to deal
> > with that case as well.
> >
> > fs/btrfs/qgroup.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > index d4917c0cddf5..ae1358253b7b 100644
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> > btrfs_abort_transaction(trans, ret);
> > goto out_free_path;
> > }
> > - spin_lock(&fs_info->qgroup_lock);
> > - fs_info->quota_root = quota_root;
> > - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> > - spin_unlock(&fs_info->qgroup_lock);
> >
> > ret = btrfs_commit_transaction(trans);
> > trans = NULL;
> > if (ret)
> > goto out_free_path;
>
> The main concern here is, if we don't set qgroup enabled bit before we
> commit transaction, there will be a window where new tree modification
> could happen before QGROUP_ENABLED bit set.
That doesn't seem to make much sense to me, if I understood correctly.
Essentially you're saying stuff done to any tree in the the
transaction we use to
enable quotas must be accounted for. In that case the quota enabled bit should
be done as soon as the transaction is started, because before we set
it and after
we started (or joined) a transaction, a lot could of modifications may
have happened.
Nevertheless I don't think it's unexpected for anyone to have the
accounting happening
only after the quota enable ioctl returns success.
>
> There may be some qgroup reserved space related problem in such case,
> but I'm not 100% sure to foresee such problem.
>
>
> The best way to do this is, commit trans first, and before any other one
> trying to start transaction, we set the bit.
> However I can't find such infrastructure now (I still remember we used
> to have a pending bit to change quota enabled flag, but removed later)
>
> Thanks,
> Qu
>
> >
> > + /*
> > + * Set quota enabled flag after committing the transaction, to avoid
> > + * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
> > + * creation.
> > + */
> > + spin_lock(&fs_info->qgroup_lock);
> > + fs_info->quota_root = quota_root;
> > + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> > + spin_unlock(&fs_info->qgroup_lock);
> > +
> > ret = qgroup_rescan_init(fs_info, 0, 1);
> > if (!ret) {
> > qgroup_rescan_zero_tracking(fs_info);
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 14:48 ` Qu Wenruo
2018-11-19 15:24 ` Filipe Manana
@ 2018-11-19 15:36 ` Nikolay Borisov
2018-11-20 0:30 ` Qu Wenruo
1 sibling, 1 reply; 17+ messages in thread
From: Nikolay Borisov @ 2018-11-19 15:36 UTC (permalink / raw)
To: Qu Wenruo, fdmanana, linux-btrfs
On 19.11.18 г. 16:48 ч., Qu Wenruo wrote:
> There may be some qgroup reserved space related problem in such case,
> but I'm not 100% sure to foresee such problem.
But why is this a problem - we always queue quota rescan following QUOTA
enable, that should take care of proper accounting, no?
>
>
> The best way to do this is, commit trans first, and before any other one
> trying to start transaction, we set the bit.
> However I can't find such infrastructure now (I still remember we used
> to have a pending bit to change quota enabled flag, but removed later)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 15:36 ` Nikolay Borisov
@ 2018-11-20 0:30 ` Qu Wenruo
0 siblings, 0 replies; 17+ messages in thread
From: Qu Wenruo @ 2018-11-20 0:30 UTC (permalink / raw)
To: Nikolay Borisov, fdmanana, linux-btrfs
On 2018/11/19 下午11:36, Nikolay Borisov wrote:
>
>
> On 19.11.18 г. 16:48 ч., Qu Wenruo wrote:
>> There may be some qgroup reserved space related problem in such case,
>> but I'm not 100% sure to foresee such problem.
>
> But why is this a problem - we always queue quota rescan following QUOTA
> enable, that should take care of proper accounting, no?
For reserved data/metadata space, not qgroup numbers.
But it turns out we have already handle such case for data.
So I'm overreacting to this problem.
>
>>
>>
>> The best way to do this is, commit trans first, and before any other one
>> trying to start transaction, we set the bit.
>> However I can't find such infrastructure now (I still remember we used
>> to have a pending bit to change quota enabled flag, but removed later)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-19 15:24 ` Filipe Manana
@ 2018-11-20 0:32 ` Qu Wenruo
2018-11-22 13:12 ` David Sterba
0 siblings, 1 reply; 17+ messages in thread
From: Qu Wenruo @ 2018-11-20 0:32 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 7669 bytes --]
On 2018/11/19 下午11:24, Filipe Manana wrote:
> On Mon, Nov 19, 2018 at 2:48 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2018/11/19 下午10:15, 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, or the task creating a snapshot tries to commit the transaction
>>> while the task enabling quota waits for the former task to commit the
>>> transaction while holding the mutex. The following time diagrams show how
>>> both cases happen.
>>>
>>> First scenario:
>>>
>>> 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()
>>>
>>> Second scenario:
>>>
>>> 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()
>>> --> waits for task at
>>> CPU 0 to release
>>> its transaction
>>> handle
>>>
>>> btrfs_commit_transaction()
>>> --> sees another task started
>>> the transaction commit first
>>> --> releases its transaction
>>> handle
>>> --> waits for the transaction
>>> commit to be completed by
>>> the task at CPU 1
>>>
>>> create_pending_snapshot()
>>> qgroup_account_snapshot()
>>> btrfs_qgroup_inherit()
>>> mutex_lock(fs_info->qgroup_ioctl_lock)
>>> --> deadlock, task at CPU 0
>>> has the mutex locked but
>>> it is waiting for us to
>>> finish the transaction
>>> commit
>>>
>>> So fix this by setting the quota enabled flag in fs_info after committing
>>> the transaction at btrfs_quota_enable(). This ends up serializing quota
>>> enable and snapshot creation as if the snapshot creation happened just
>>> before the quota enable request. The quota rescan task, scheduled after
>>> committing the transaction in btrfs_quote_enable(), will do the accounting.
>>>
>>> Fixes: 6426c7ad697d ("btrfs: qgroup: Fix qgroup accounting when creating snapshot")
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>
>>> V2: Added second deadlock example to changelog and changed the fix to deal
>>> with that case as well.
>>>
>>> fs/btrfs/qgroup.c | 14 ++++++++++----
>>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index d4917c0cddf5..ae1358253b7b 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>>> btrfs_abort_transaction(trans, ret);
>>> goto out_free_path;
>>> }
>>> - spin_lock(&fs_info->qgroup_lock);
>>> - fs_info->quota_root = quota_root;
>>> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>>> - spin_unlock(&fs_info->qgroup_lock);
>>>
>>> ret = btrfs_commit_transaction(trans);
>>> trans = NULL;
>>> if (ret)
>>> goto out_free_path;
>>
>> The main concern here is, if we don't set qgroup enabled bit before we
>> commit transaction, there will be a window where new tree modification
>> could happen before QGROUP_ENABLED bit set.
>
> That doesn't seem to make much sense to me, if I understood correctly.
> Essentially you're saying stuff done to any tree in the the
> transaction we use to
> enable quotas must be accounted for. In that case the quota enabled bit should
> be done as soon as the transaction is started, because before we set
> it and after
> we started (or joined) a transaction, a lot could of modifications may
> have happened.
> Nevertheless I don't think it's unexpected for anyone to have the
> accounting happening
> only after the quota enable ioctl returns success.
The problem is not accounting, the qgroup number won't cause problem.
It's the reserved space. Like some dirty pages are dirtied before quota
enabled, but at run_dealloc() time quota is enabled.
For such case we have io_tree based method to avoid underflow so it
should be OK.
So v2 patch looks OK.
Thanks,
Qu
>
>>
>> There may be some qgroup reserved space related problem in such case,
>> but I'm not 100% sure to foresee such problem.
>>
>>
>> The best way to do this is, commit trans first, and before any other one
>> trying to start transaction, we set the bit.
>> However I can't find such infrastructure now (I still remember we used
>> to have a pending bit to change quota enabled flag, but removed later)
>>
>> Thanks,
>> Qu
>>
>>>
>>> + /*
>>> + * Set quota enabled flag after committing the transaction, to avoid
>>> + * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
>>> + * creation.
>>> + */
>>> + spin_lock(&fs_info->qgroup_lock);
>>> + fs_info->quota_root = quota_root;
>>> + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>>> + spin_unlock(&fs_info->qgroup_lock);
>>> +
>>> ret = qgroup_rescan_init(fs_info, 0, 1);
>>> if (!ret) {
>>> qgroup_rescan_zero_tracking(fs_info);
>>>
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-20 0:32 ` Qu Wenruo
@ 2018-11-22 13:12 ` David Sterba
2018-11-22 13:46 ` Qu Wenruo
0 siblings, 1 reply; 17+ messages in thread
From: David Sterba @ 2018-11-22 13:12 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Filipe Manana, linux-btrfs
On Tue, Nov 20, 2018 at 08:32:42AM +0800, Qu Wenruo wrote:
> >>> @@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> >>> btrfs_abort_transaction(trans, ret);
> >>> goto out_free_path;
> >>> }
> >>> - spin_lock(&fs_info->qgroup_lock);
> >>> - fs_info->quota_root = quota_root;
> >>> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> >>> - spin_unlock(&fs_info->qgroup_lock);
> >>>
> >>> ret = btrfs_commit_transaction(trans);
> >>> trans = NULL;
> >>> if (ret)
> >>> goto out_free_path;
> >>
> >> The main concern here is, if we don't set qgroup enabled bit before we
> >> commit transaction, there will be a window where new tree modification
> >> could happen before QGROUP_ENABLED bit set.
> >
> > That doesn't seem to make much sense to me, if I understood correctly.
> > Essentially you're saying stuff done to any tree in the the
> > transaction we use to
> > enable quotas must be accounted for. In that case the quota enabled bit should
> > be done as soon as the transaction is started, because before we set
> > it and after
> > we started (or joined) a transaction, a lot could of modifications may
> > have happened.
> > Nevertheless I don't think it's unexpected for anyone to have the
> > accounting happening
> > only after the quota enable ioctl returns success.
>
> The problem is not accounting, the qgroup number won't cause problem.
>
> It's the reserved space. Like some dirty pages are dirtied before quota
> enabled, but at run_dealloc() time quota is enabled.
>
> For such case we have io_tree based method to avoid underflow so it
> should be OK.
>
> So v2 patch looks OK.
Does that mean reviewed-by? In case there's a evolved discussion under a
patch, a clear yes/no is appreciated and an explicit Reviewed-by even
more. I'm about to add this patch to rc4 pull, thre's still some time to
add the tag. Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-22 13:12 ` David Sterba
@ 2018-11-22 13:46 ` Qu Wenruo
2018-11-22 14:42 ` David Sterba
0 siblings, 1 reply; 17+ messages in thread
From: Qu Wenruo @ 2018-11-22 13:46 UTC (permalink / raw)
To: dsterba, Filipe Manana, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 2371 bytes --]
On 2018/11/22 下午9:12, David Sterba wrote:
> On Tue, Nov 20, 2018 at 08:32:42AM +0800, Qu Wenruo wrote:
>>>>> @@ -1013,16 +1013,22 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>>>>> btrfs_abort_transaction(trans, ret);
>>>>> goto out_free_path;
>>>>> }
>>>>> - spin_lock(&fs_info->qgroup_lock);
>>>>> - fs_info->quota_root = quota_root;
>>>>> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>>>>> - spin_unlock(&fs_info->qgroup_lock);
>>>>>
>>>>> ret = btrfs_commit_transaction(trans);
>>>>> trans = NULL;
>>>>> if (ret)
>>>>> goto out_free_path;
>>>>
>>>> The main concern here is, if we don't set qgroup enabled bit before we
>>>> commit transaction, there will be a window where new tree modification
>>>> could happen before QGROUP_ENABLED bit set.
>>>
>>> That doesn't seem to make much sense to me, if I understood correctly.
>>> Essentially you're saying stuff done to any tree in the the
>>> transaction we use to
>>> enable quotas must be accounted for. In that case the quota enabled bit should
>>> be done as soon as the transaction is started, because before we set
>>> it and after
>>> we started (or joined) a transaction, a lot could of modifications may
>>> have happened.
>>> Nevertheless I don't think it's unexpected for anyone to have the
>>> accounting happening
>>> only after the quota enable ioctl returns success.
>>
>> The problem is not accounting, the qgroup number won't cause problem.
>>
>> It's the reserved space. Like some dirty pages are dirtied before quota
>> enabled, but at run_dealloc() time quota is enabled.
>>
>> For such case we have io_tree based method to avoid underflow so it
>> should be OK.
>>
>> So v2 patch looks OK.
>
> Does that mean reviewed-by? In case there's a evolved discussion under a
> patch, a clear yes/no is appreciated and an explicit Reviewed-by even
> more. I'm about to add this patch to rc4 pull, thre's still some time to
> add the tag. Thanks.
>
I'd like to add reviewed-by tab, but I'm still not 100% if this will
cause extra qgroup reserved space related problem.
At least from my side, I can't directly see a case where it will cause
problem.
Does such case mean a reviewed-by tag? Or something LGTM-but-uncertain?
Thanks,
Qu
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation
2018-11-22 13:46 ` Qu Wenruo
@ 2018-11-22 14:42 ` David Sterba
0 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2018-11-22 14:42 UTC (permalink / raw)
To: Qu Wenruo; +Cc: dsterba, Filipe Manana, linux-btrfs
On Thu, Nov 22, 2018 at 09:46:44PM +0800, Qu Wenruo wrote:
> >>> it and after
> >>> we started (or joined) a transaction, a lot could of modifications may
> >>> have happened.
> >>> Nevertheless I don't think it's unexpected for anyone to have the
> >>> accounting happening
> >>> only after the quota enable ioctl returns success.
> >>
> >> The problem is not accounting, the qgroup number won't cause problem.
> >>
> >> It's the reserved space. Like some dirty pages are dirtied before quota
> >> enabled, but at run_dealloc() time quota is enabled.
> >>
> >> For such case we have io_tree based method to avoid underflow so it
> >> should be OK.
> >>
> >> So v2 patch looks OK.
> >
> > Does that mean reviewed-by? In case there's a evolved discussion under a
> > patch, a clear yes/no is appreciated and an explicit Reviewed-by even
> > more. I'm about to add this patch to rc4 pull, thre's still some time to
> > add the tag. Thanks.
>
> I'd like to add reviewed-by tab, but I'm still not 100% if this will
> cause extra qgroup reserved space related problem.
>
> At least from my side, I can't directly see a case where it will cause
> problem.
>
> Does such case mean a reviewed-by tag? Or something LGTM-but-uncertain?
It means that we can keep the patch in testing branch for a bit longer,
there's still time to put it to a later rc once there's enough
certainty.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-11-22 14:42 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 9:48 [PATCH] Btrfs: fix deadlock when enabling quotas due to concurrent snapshot creation fdmanana
2018-11-19 10:07 ` Nikolay Borisov
2018-11-19 11:09 ` Qu Wenruo
2018-11-19 11:13 ` Filipe Manana
2018-11-19 11:35 ` Qu Wenruo
2018-11-19 11:52 ` Filipe Manana
2018-11-19 12:14 ` Qu Wenruo
2018-11-19 14:15 ` Filipe Manana
2018-11-19 14:15 ` [PATCH v2] " fdmanana
2018-11-19 14:48 ` Qu Wenruo
2018-11-19 15:24 ` Filipe Manana
2018-11-20 0:32 ` Qu Wenruo
2018-11-22 13:12 ` David Sterba
2018-11-22 13:46 ` Qu Wenruo
2018-11-22 14:42 ` David Sterba
2018-11-19 15:36 ` Nikolay Borisov
2018-11-20 0:30 ` Qu Wenruo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.