Wang Shilong
2013-Aug-07 05:12 UTC
[PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Aug-07 05:12 UTC
[PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Aug-07 05:12 UTC
[PATCH 3/3] Btrfs: remove reduplicate check when disabling quota
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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Aug-07 05:16 UTC
Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
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 >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Josef Bacik
2013-Aug-08 13:20 UTC
Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Aug-09 09:12 UTC
Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
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 >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arne Jansen
2013-Aug-09 09:30 UTC
Re: [PATCH 1/3] Btrfs: fix oops when writing dirty qgroups to disk
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-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arne Jansen
2013-Aug-09 09:36 UTC
Re: [PATCH 3/3] Btrfs: remove reduplicate check when disabling quota
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> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arne Jansen
2013-Aug-09 09:42 UTC
Re: [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments
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 setYou 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;-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wang Shilong
2013-Aug-09 09:57 UTC
Re: [PATCH 2/3] Btrfs: move btrfs_free_qgroup_config() out of spin_lock and fix comments
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; > >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html