* [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments
2013-08-07 5:12 [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk Wang Shilong
@ 2013-08-07 5:12 ` Wang Shilong
2013-08-09 9:42 ` Arne Jansen
2013-08-07 5:12 ` [PATCH 3/3] Btrfs: remove reduplicate check when disabling quota Wang Shilong
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Wang Shilong @ 2013-08-07 5:12 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox
btrfs_free_qgroup_config() is not only called by open/close_ctree(),but
also btrfs_disable_quota().And for btrfs_disable_quota(),we have set
'quota_root' to be null before calling btrfs_free_qgroup_config(),so it
is safe to cleanup in-memory structures without lock held.
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
fs/btrfs/qgroup.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 3b103e2..b809616 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -432,8 +432,10 @@ out:
}
/*
- * This is only called from close_ctree() or open_ctree(), both in single-
- * treaded paths. Clean up the in-memory structures. No locking needed.
+ * This is called from close_ctree() or open_ctree() or btrfs_quota_disable(),
+ * first two are in single-treaded paths.And for the third one, we have set
+ * quota_root to be null with qgroup_lock held before, so it is safe to clean
+ * up the in-memory structures without qgroup_lock held.
*/
void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
{
@@ -937,9 +939,10 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
fs_info->pending_quota_state = 0;
quota_root = fs_info->quota_root;
fs_info->quota_root = NULL;
- btrfs_free_qgroup_config(fs_info);
spin_unlock(&fs_info->qgroup_lock);
+ btrfs_free_qgroup_config(fs_info);
+
if (!quota_root) {
ret = -EINVAL;
goto out;
--
1.8.0.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments
2013-08-07 5:12 ` [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments Wang Shilong
@ 2013-08-09 9:42 ` Arne Jansen
2013-08-09 9:57 ` Wang Shilong
0 siblings, 1 reply; 10+ messages in thread
From: Arne Jansen @ 2013-08-09 9:42 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, miaox
On 07.08.2013 07:12, Wang Shilong wrote:
> btrfs_free_qgroup_config() is not only called by open/close_ctree(),but
> also btrfs_disable_quota().And for btrfs_disable_quota(),we have set
> 'quota_root' to be null before calling btrfs_free_qgroup_config(),so it
> is safe to cleanup in-memory structures without lock held.
Why do you do this? Did the spinlock gave you any trouble?
>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
> fs/btrfs/qgroup.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 3b103e2..b809616 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -432,8 +432,10 @@ out:
> }
>
> /*
> - * This is only called from close_ctree() or open_ctree(), both in single-
> - * treaded paths. Clean up the in-memory structures. No locking needed.
> + * This is called from close_ctree() or open_ctree() or btrfs_quota_disable(),
> + * first two are in single-treaded paths.And for the third one, we have set
You could use the opportunity and correct the typo in 'single-treaded'. Also,
there's a space missing after the period.
-Arne
> + * quota_root to be null with qgroup_lock held before, so it is safe to clean
> + * up the in-memory structures without qgroup_lock held.
> */
> void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
> {
> @@ -937,9 +939,10 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
> fs_info->pending_quota_state = 0;
> quota_root = fs_info->quota_root;
> fs_info->quota_root = NULL;
> - btrfs_free_qgroup_config(fs_info);
> spin_unlock(&fs_info->qgroup_lock);
>
> + btrfs_free_qgroup_config(fs_info);
> +
> if (!quota_root) {
> ret = -EINVAL;
> goto out;
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments
2013-08-09 9:42 ` Arne Jansen
@ 2013-08-09 9:57 ` Wang Shilong
0 siblings, 0 replies; 10+ messages in thread
From: Wang Shilong @ 2013-08-09 9:57 UTC (permalink / raw)
To: Arne Jansen; +Cc: linux-btrfs, miaox
On 08/09/2013 05:42 PM, Arne Jansen wrote:
> On 07.08.2013 07:12, Wang Shilong wrote:
>> btrfs_free_qgroup_config() is not only called by open/close_ctree(),but
>> also btrfs_disable_quota().And for btrfs_disable_quota(),we have set
>> 'quota_root' to be null before calling btrfs_free_qgroup_config(),so it
>> is safe to cleanup in-memory structures without lock held.
>
> Why do you do this? Did the spinlock gave you any trouble?
Because other places such as btrfs_qgroup_reserve() will try spin_lock.
and we should relase spin_lock as soon as possible, luckily we are safe
to move btrfs_free_qgroup_config() out of spin_lock.
>
>>
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
>> ---
>> fs/btrfs/qgroup.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 3b103e2..b809616 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -432,8 +432,10 @@ out:
>> }
>>
>> /*
>> - * This is only called from close_ctree() or open_ctree(), both in single-
>> - * treaded paths. Clean up the in-memory structures. No locking needed.
>> + * This is called from close_ctree() or open_ctree() or btrfs_quota_disable(),
>> + * first two are in single-treaded paths.And for the third one, we have set
>
> You could use the opportunity and correct the typo in 'single-treaded'. Also,
> there's a space missing after the period.
I will fix typo.
Thank,
Wang
>
> -Arne
>
>> + * quota_root to be null with qgroup_lock held before, so it is safe to clean
>> + * up the in-memory structures without qgroup_lock held.
>> */
>> void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
>> {
>> @@ -937,9 +939,10 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>> fs_info->pending_quota_state = 0;
>> quota_root = fs_info->quota_root;
>> fs_info->quota_root = NULL;
>> - btrfs_free_qgroup_config(fs_info);
>> spin_unlock(&fs_info->qgroup_lock);
>>
>> + btrfs_free_qgroup_config(fs_info);
>> +
>> if (!quota_root) {
>> ret = -EINVAL;
>> goto out;
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] Btrfs: remove reduplicate check when disabling quota
2013-08-07 5:12 [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk Wang Shilong
2013-08-07 5:12 ` [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments Wang Shilong
@ 2013-08-07 5:12 ` Wang Shilong
2013-08-09 9:36 ` Arne Jansen
2013-08-07 5:16 ` [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk Wang Shilong
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Wang Shilong @ 2013-08-07 5:12 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox
We have checked 'quota_root' with qgroup_ioctl_lock held before,So
here the check is reduplicate, remove it.
Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
fs/btrfs/qgroup.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index b809616..df2841d 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -943,11 +943,6 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
btrfs_free_qgroup_config(fs_info);
- if (!quota_root) {
- ret = -EINVAL;
- goto out;
- }
-
ret = btrfs_clean_quota_tree(trans, quota_root);
if (ret)
goto out;
--
1.8.0.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] Btrfs: remove reduplicate check when disabling quota
2013-08-07 5:12 ` [PATCH 3/3] Btrfs: remove reduplicate check when disabling quota Wang Shilong
@ 2013-08-09 9:36 ` Arne Jansen
0 siblings, 0 replies; 10+ messages in thread
From: Arne Jansen @ 2013-08-09 9:36 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, miaox
On 07.08.2013 07:12, Wang Shilong wrote:
> We have checked 'quota_root' with qgroup_ioctl_lock held before,So
> here the check is reduplicate, remove it.
>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
> fs/btrfs/qgroup.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index b809616..df2841d 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -943,11 +943,6 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
>
> btrfs_free_qgroup_config(fs_info);
>
> - if (!quota_root) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> ret = btrfs_clean_quota_tree(trans, quota_root);
> if (ret)
> goto out;
Reviewed-by: Arne Jansen <sensille@gmx.net>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
2013-08-07 5:12 [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk Wang Shilong
2013-08-07 5:12 ` [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments Wang Shilong
2013-08-07 5:12 ` [PATCH 3/3] Btrfs: remove reduplicate check when disabling quota Wang Shilong
@ 2013-08-07 5:16 ` Wang Shilong
2013-08-08 13:20 ` Josef Bacik
2013-08-09 9:30 ` Arne Jansen
4 siblings, 0 replies; 10+ messages in thread
From: Wang Shilong @ 2013-08-07 5:16 UTC (permalink / raw)
To: sensille; +Cc: linux-btrfs, miaox
On 08/07/2013 01:12 PM, Wang Shilong wrote:
> When disabling quota, we should clear out list 'dirty_qgroups',otherwise,
> we will get oops if enabling quota again. Fix this by abstracting similar
> code from del_qgroup_rb().
Hello Arne,
Would you pleae review this patch and other cleanup patches,
i just hit this oops when i want to reproduce memory leak of qgroups.
Thanks,
Wang
>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
> fs/btrfs/qgroup.c | 43 ++++++++++++++-----------------------------
> 1 file changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 64a9e3c..3b103e2 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -157,18 +157,11 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
> return qgroup;
> }
>
> -/* must be called with qgroup_lock held */
> -static int del_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
> +static void __del_qgroup_rb(struct btrfs_qgroup *qgroup)
> {
> - struct btrfs_qgroup *qgroup = find_qgroup_rb(fs_info, qgroupid);
> - struct btrfs_qgroup_list *list;
> + struct btrfs_qgroup_list *list = NULL;
>
> - if (!qgroup)
> - return -ENOENT;
> -
> - rb_erase(&qgroup->node, &fs_info->qgroup_tree);
> list_del(&qgroup->dirty);
> -
> while (!list_empty(&qgroup->groups)) {
> list = list_first_entry(&qgroup->groups,
> struct btrfs_qgroup_list, next_group);
> @@ -185,7 +178,18 @@ static int del_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
> kfree(list);
> }
> kfree(qgroup);
> +}
> +
> +/* must be called with qgroup_lock held */
> +static int del_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
> +{
> + struct btrfs_qgroup *qgroup = find_qgroup_rb(fs_info, qgroupid);
>
> + if (!qgroup)
> + return -ENOENT;
> +
> + rb_erase(&qgroup->node, &fs_info->qgroup_tree);
> + __del_qgroup_rb(qgroup);
> return 0;
> }
>
> @@ -435,30 +439,11 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
> {
> struct rb_node *n;
> struct btrfs_qgroup *qgroup;
> - struct btrfs_qgroup_list *list;
>
> while ((n = rb_first(&fs_info->qgroup_tree))) {
> qgroup = rb_entry(n, struct btrfs_qgroup, node);
> rb_erase(n, &fs_info->qgroup_tree);
> -
> - while (!list_empty(&qgroup->groups)) {
> - list = list_first_entry(&qgroup->groups,
> - struct btrfs_qgroup_list,
> - next_group);
> - list_del(&list->next_group);
> - list_del(&list->next_member);
> - kfree(list);
> - }
> -
> - while (!list_empty(&qgroup->members)) {
> - list = list_first_entry(&qgroup->members,
> - struct btrfs_qgroup_list,
> - next_member);
> - list_del(&list->next_group);
> - list_del(&list->next_member);
> - kfree(list);
> - }
> - kfree(qgroup);
> + __del_qgroup_rb(qgroup);
> }
> /*
> * we call btrfs_free_qgroup_config() when umounting
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
2013-08-07 5:12 [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk Wang Shilong
` (2 preceding siblings ...)
2013-08-07 5:16 ` [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk Wang Shilong
@ 2013-08-08 13:20 ` Josef Bacik
2013-08-09 9:12 ` Wang Shilong
2013-08-09 9:30 ` Arne Jansen
4 siblings, 1 reply; 10+ messages in thread
From: Josef Bacik @ 2013-08-08 13:20 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, sensille, miaox
On Wed, Aug 07, 2013 at 01:12:29PM +0800, Wang Shilong wrote:
> When disabling quota, we should clear out list 'dirty_qgroups',otherwise,
> we will get oops if enabling quota again. Fix this by abstracting similar
> code from del_qgroup_rb().
>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
Can we get an xfstest for this, or at the very least a generic xfstest to
exercise qgroups in general so I can be sure all these qgroup patches I take
don't cause regressions? Thanks,
Josef
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
2013-08-08 13:20 ` Josef Bacik
@ 2013-08-09 9:12 ` Wang Shilong
0 siblings, 0 replies; 10+ messages in thread
From: Wang Shilong @ 2013-08-09 9:12 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, sensille, miaox
On 08/08/2013 09:20 PM, Josef Bacik wrote:
> On Wed, Aug 07, 2013 at 01:12:29PM +0800, Wang Shilong wrote:
>> When disabling quota, we should clear out list 'dirty_qgroups',otherwise,
>> we will get oops if enabling quota again. Fix this by abstracting similar
>> code from del_qgroup_rb().
>>
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
>
> Can we get an xfstest for this, or at the very least a generic xfstest to
> exercise qgroups in general so I can be sure all these qgroup patches I take
> don't cause regressions? Thanks,
Yeah, we have a test suite ourself for btrfs qgroup, and i wil change it integrated into
xfstest later.
Thanks,
Wang
>
> Josef
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
2013-08-07 5:12 [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk Wang Shilong
` (3 preceding siblings ...)
2013-08-08 13:20 ` Josef Bacik
@ 2013-08-09 9:30 ` Arne Jansen
4 siblings, 0 replies; 10+ messages in thread
From: Arne Jansen @ 2013-08-09 9:30 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, miaox
On 07.08.2013 07:12, Wang Shilong wrote:
> When disabling quota, we should clear out list 'dirty_qgroups',otherwise,
> we will get oops if enabling quota again. Fix this by abstracting similar
> code from del_qgroup_rb().
>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
> fs/btrfs/qgroup.c | 43 ++++++++++++++-----------------------------
> 1 file changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 64a9e3c..3b103e2 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -157,18 +157,11 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
> return qgroup;
> }
>
> -/* must be called with qgroup_lock held */
> -static int del_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
> +static void __del_qgroup_rb(struct btrfs_qgroup *qgroup)
> {
> - struct btrfs_qgroup *qgroup = find_qgroup_rb(fs_info, qgroupid);
> - struct btrfs_qgroup_list *list;
> + struct btrfs_qgroup_list *list = NULL;
Why do you initialize list to NULL here? It's always assigned
before used.
otherwise,
Reviewed-by: Arne Jansen <sensille@gmx.net>
>
> - if (!qgroup)
> - return -ENOENT;
> -
> - rb_erase(&qgroup->node, &fs_info->qgroup_tree);
> list_del(&qgroup->dirty);
> -
> while (!list_empty(&qgroup->groups)) {
> list = list_first_entry(&qgroup->groups,
> struct btrfs_qgroup_list, next_group);
> @@ -185,7 +178,18 @@ static int del_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
> kfree(list);
> }
> kfree(qgroup);
> +}
> +
> +/* must be called with qgroup_lock held */
> +static int del_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
> +{
> + struct btrfs_qgroup *qgroup = find_qgroup_rb(fs_info, qgroupid);
>
> + if (!qgroup)
> + return -ENOENT;
> +
> + rb_erase(&qgroup->node, &fs_info->qgroup_tree);
> + __del_qgroup_rb(qgroup);
> return 0;
> }
>
> @@ -435,30 +439,11 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
> {
> struct rb_node *n;
> struct btrfs_qgroup *qgroup;
> - struct btrfs_qgroup_list *list;
>
> while ((n = rb_first(&fs_info->qgroup_tree))) {
> qgroup = rb_entry(n, struct btrfs_qgroup, node);
> rb_erase(n, &fs_info->qgroup_tree);
> -
> - while (!list_empty(&qgroup->groups)) {
> - list = list_first_entry(&qgroup->groups,
> - struct btrfs_qgroup_list,
> - next_group);
> - list_del(&list->next_group);
> - list_del(&list->next_member);
> - kfree(list);
> - }
> -
> - while (!list_empty(&qgroup->members)) {
> - list = list_first_entry(&qgroup->members,
> - struct btrfs_qgroup_list,
> - next_member);
> - list_del(&list->next_group);
> - list_del(&list->next_member);
> - kfree(list);
> - }
> - kfree(qgroup);
> + __del_qgroup_rb(qgroup);
> }
> /*
> * we call btrfs_free_qgroup_config() when umounting
^ permalink raw reply [flat|nested] 10+ messages in thread