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

2013-08-09 Thread 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.f...@cn.fujitsu.com
 Reviewed-by: Miao Xie mi...@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
 

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-09 Thread 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.f...@cn.fujitsu.com
 Reviewed-by: Miao Xie mi...@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 sensi...@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

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-08 Thread 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.f...@cn.fujitsu.com
 Reviewed-by: Miao Xie mi...@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
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-06 Thread 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.f...@cn.fujitsu.com
Reviewed-by: Miao Xie mi...@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

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-06 Thread 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.f...@cn.fujitsu.com
 Reviewed-by: Miao Xie mi...@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
 

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html