[PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk

[PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk

[Wang Shilong]

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;
 
-	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
-- 
1.8.0.1

[PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments

[Wang Shilong]

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

[PATCH 3/3] Btrfs: remove reduplicate check when disabling quota

[Wang Shilong]

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

Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk

[Wang Shilong]

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
> 

Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk

[Josef Bacik]

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

Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk

[Wang Shilong]

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
> 

Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk

[Arne Jansen]

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

Re: [PATCH 3/3] Btrfs: remove reduplicate check when disabling quota

[Arne Jansen]

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>

Re: [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments

[Arne Jansen]

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;

Re: [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments

[Wang Shilong]

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;
> 
>