thr3ads.net - Btrfs devel - [PATCH V3 0/5] introduce a mutex lock and fix missing checks [Apr 2013]

If this information is useful, please help other people find it:
Share via:

Wang Shilong

2013-Apr-07 10:50 UTC

[PATCH V3 0/5] introduce a mutex lock and fix missing checks

This patchset introduces a mutex lock ''qgroup_ioctl_lock''
to protect all the user change for quota operations.

Without an extra lock, race condition can not be avoided
between operations in disk and in memory.

Holding mutex lock, we can fix missing check before creating
a qgroup(whether ''src'' and ''dst'' exist).And
we can guarantee
that all the qgroups in ''inherit'' dose exist before tracking
qgroup.

V2->V3:
	rename ''quota_lock'' to ''qgroup_ioctl_lock''
and holding
	''qgroup_ioctl_lock'' inside qgroup.c to avoid serializing
	operations.
V1->V2:
	use quota configurations in memory to speed up checks.

Wang Shilong (5):
  Btrfs: introduce a mutex lock for btrfs quota operations
  Btrfs: remove some unnecessary spin_lock usages
  Btrfs: fix missing check before creating a qgroup relation
  Btrfs: fix missing check in the btrfs_qgroup_inherit()
  Btrfs: fix a warning when updating qgroup limit

 fs/btrfs/ctree.h   |   3 ++
 fs/btrfs/disk-io.c |   1 +
 fs/btrfs/qgroup.c  | 136 ++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 92 insertions(+), 48 deletions(-)

-- 
1.7.11.7

--
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-Apr-07 10:50 UTC

head link

[PATCH V3 1/5] Btrfs: introduce a mutex lock for btrfs quota operations

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

The original code has one spin_lock ''qgroup_lock'' to protect
quota
configurations in memory. If we want to add a BTRFS_QGROUP_INFO_KEY,
it will be added to Btree firstly, and then update configurations in
memory,however, a race condition may happen between these operations.
For example:
	->add_qgroup_info_item()
		->add_qgroup_rb()

For the above case, del_qgroup_info_item() may happen just before
add_qgroup_rb().

What''s worse, when we want to add a qgroup relation:
	->add_qgroup_relation_item()
		->add_qgroup_relations()

We don''t have any checks whether ''src'' and
''dst'' exist before
add_qgroup_relation_item(), a race condition can also happen for
the above case.

To avoid race condition and have all the necessary checks, we introduce
a mutex lock ''qgroup_ioctl_lock'', and we make all the user
change operations
protected by the mutex lock.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/ctree.h   |  3 ++
 fs/btrfs/disk-io.c |  1 +
 fs/btrfs/qgroup.c  | 82 +++++++++++++++++++++++++++++++++++++-----------------
 3 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6e81860..15bd72b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1584,6 +1584,9 @@ struct btrfs_fs_info {
 	struct rb_root qgroup_tree;
 	spinlock_t qgroup_lock;
 
+	/* protect user change for quota operations */
+	struct mutex qgroup_ioctl_lock;
+
 	/* list of dirty qgroups to be written at next commit */
 	struct list_head dirty_qgroups;
 
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4bae789..238f5cc 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2251,6 +2251,7 @@ int open_ctree(struct super_block *sb,
 	mutex_init(&fs_info->dev_replace.lock);
 
 	spin_lock_init(&fs_info->qgroup_lock);
+	mutex_init(&fs_info->qgroup_ioctl_lock);
 	fs_info->qgroup_tree = RB_ROOT;
 	INIT_LIST_HEAD(&fs_info->dirty_qgroups);
 	fs_info->qgroup_seq = 1;
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index e3598fa..5837cb5 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -793,6 +793,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	int ret = 0;
 	int slot;
 
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	spin_lock(&fs_info->qgroup_lock);
 	if (fs_info->quota_root) {
 		fs_info->pending_quota_state = 1;
@@ -902,6 +903,7 @@ out_free_root:
 		kfree(quota_root);
 	}
 out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
@@ -912,10 +914,11 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
 	struct btrfs_root *quota_root;
 	int ret = 0;
 
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	spin_lock(&fs_info->qgroup_lock);
 	if (!fs_info->quota_root) {
 		spin_unlock(&fs_info->qgroup_lock);
-		return 0;
+		goto out;
 	}
 	fs_info->quota_enabled = 0;
 	fs_info->pending_quota_state = 0;
@@ -924,8 +927,10 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
 	btrfs_free_qgroup_config(fs_info);
 	spin_unlock(&fs_info->qgroup_lock);
 
-	if (!quota_root)
-		return -EINVAL;
+	if (!quota_root) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = btrfs_clean_quota_tree(trans, quota_root);
 	if (ret)
@@ -946,6 +951,7 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
 	free_extent_buffer(quota_root->commit_root);
 	kfree(quota_root);
 out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
@@ -961,24 +967,28 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle
*trans,
 	struct btrfs_root *quota_root;
 	int ret = 0;
 
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
-	if (!quota_root)
-		return -EINVAL;
+	if (!quota_root) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = add_qgroup_relation_item(trans, quota_root, src, dst);
 	if (ret)
-		return ret;
+		goto out;
 
 	ret = add_qgroup_relation_item(trans, quota_root, dst, src);
 	if (ret) {
 		del_qgroup_relation_item(trans, quota_root, src, dst);
-		return ret;
+		goto out;
 	}
 
 	spin_lock(&fs_info->qgroup_lock);
 	ret = add_relation_rb(quota_root->fs_info, src, dst);
 	spin_unlock(&fs_info->qgroup_lock);
-
+out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
@@ -989,9 +999,12 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle
*trans,
 	int ret = 0;
 	int err;
 
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
-	if (!quota_root)
-		return -EINVAL;
+	if (!quota_root) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = del_qgroup_relation_item(trans, quota_root, src, dst);
 	err = del_qgroup_relation_item(trans, quota_root, dst, src);
@@ -1002,7 +1015,8 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle
*trans,
 	del_relation_rb(fs_info, src, dst);
 
 	spin_unlock(&fs_info->qgroup_lock);
-
+out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
@@ -1013,9 +1027,12 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 	struct btrfs_qgroup *qgroup;
 	int ret = 0;
 
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
-	if (!quota_root)
-		return -EINVAL;
+	if (!quota_root) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = add_qgroup_item(trans, quota_root, qgroupid);
 
@@ -1025,7 +1042,8 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 
 	if (IS_ERR(qgroup))
 		ret = PTR_ERR(qgroup);
-
+out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
@@ -1036,9 +1054,12 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 	struct btrfs_qgroup *qgroup;
 	int ret = 0;
 
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
-	if (!quota_root)
-		return -EINVAL;
+	if (!quota_root) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* check if there are no relations to this qgroup */
 	spin_lock(&fs_info->qgroup_lock);
@@ -1046,7 +1067,8 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 	if (qgroup) {
 		if (!list_empty(&qgroup->groups) ||
!list_empty(&qgroup->members)) {
 			spin_unlock(&fs_info->qgroup_lock);
-			return -EBUSY;
+			ret = -EBUSY;
+			goto out;
 		}
 	}
 	spin_unlock(&fs_info->qgroup_lock);
@@ -1056,7 +1078,8 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 	spin_lock(&fs_info->qgroup_lock);
 	del_qgroup_rb(quota_root->fs_info, qgroupid);
 	spin_unlock(&fs_info->qgroup_lock);
-
+out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
@@ -1064,12 +1087,16 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info, u64 qgroupid,
 		       struct btrfs_qgroup_limit *limit)
 {
-	struct btrfs_root *quota_root = fs_info->quota_root;
+	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
 	int ret = 0;
 
-	if (!quota_root)
-		return -EINVAL;
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
+	quota_root = fs_info->quota_root;
+	if (!quota_root) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = update_qgroup_limit_item(trans, quota_root, qgroupid,
 				       limit->flags, limit->max_rfer,
@@ -1096,7 +1123,8 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 
 unlock:
 	spin_unlock(&fs_info->qgroup_lock);
-
+out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
@@ -1394,11 +1422,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle
*trans,
 	struct btrfs_qgroup *dstgroup;
 	u32 level_size = 0;
 
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_enabled)
-		return 0;
+		goto out;
 
-	if (!quota_root)
-		return -EINVAL;
+	if (!quota_root) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/*
 	 * create a tracking group for the subvol itself
@@ -1525,6 +1556,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 unlock:
 	spin_unlock(&fs_info->qgroup_lock);
 out:
+	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
 
-- 
1.7.11.7

--
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-Apr-07 10:50 UTC

head link

[PATCH V3 2/5] Btrfs: remove some unnecessary spin_lock usages

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

We use mutex lock to protect all the user change operations.
So when we are calling find_qgroup_rb() to check whether qgroup
exists, we don''t have to hold spin_lock.

Besides, when enabling/disabling quota, it must be single thread
when operations come here. spin lock must be firstly used to
clear quota_root when disabling quota, while enabling quota, spin
lock must be used to complete the last assign work.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 5837cb5..466bcd6 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -98,7 +98,7 @@ struct btrfs_qgroup_list {
 	struct btrfs_qgroup *member;
 };
 
-/* must be called with qgroup_lock held */
+/* must be called with qgroup_ioctl_lock held */
 static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
 					   u64 qgroupid)
 {
@@ -794,13 +794,10 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 	int slot;
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
-	spin_lock(&fs_info->qgroup_lock);
 	if (fs_info->quota_root) {
 		fs_info->pending_quota_state = 1;
-		spin_unlock(&fs_info->qgroup_lock);
 		goto out;
 	}
-	spin_unlock(&fs_info->qgroup_lock);
 
 	/*
 	 * initially create the quota tree
@@ -862,14 +859,11 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
 			if (ret)
 				goto out_free_path;
 
-			spin_lock(&fs_info->qgroup_lock);
 			qgroup = add_qgroup_rb(fs_info, found_key.offset);
 			if (IS_ERR(qgroup)) {
-				spin_unlock(&fs_info->qgroup_lock);
 				ret = PTR_ERR(qgroup);
 				goto out_free_path;
 			}
-			spin_unlock(&fs_info->qgroup_lock);
 		}
 		ret = btrfs_next_item(tree_root, path);
 		if (ret < 0)
@@ -884,13 +878,12 @@ out_add_root:
 	if (ret)
 		goto out_free_path;
 
-	spin_lock(&fs_info->qgroup_lock);
 	qgroup = add_qgroup_rb(fs_info, BTRFS_FS_TREE_OBJECTID);
 	if (IS_ERR(qgroup)) {
-		spin_unlock(&fs_info->qgroup_lock);
 		ret = PTR_ERR(qgroup);
 		goto out_free_path;
 	}
+	spin_lock(&fs_info->qgroup_lock);
 	fs_info->quota_root = quota_root;
 	fs_info->pending_quota_state = 1;
 	spin_unlock(&fs_info->qgroup_lock);
@@ -915,11 +908,9 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
 	int ret = 0;
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
-	spin_lock(&fs_info->qgroup_lock);
-	if (!fs_info->quota_root) {
-		spin_unlock(&fs_info->qgroup_lock);
+	if (!fs_info->quota_root)
 		goto out;
-	}
+	spin_lock(&fs_info->qgroup_lock);
 	fs_info->quota_enabled = 0;
 	fs_info->pending_quota_state = 0;
 	quota_root = fs_info->quota_root;
@@ -1062,16 +1053,13 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle
*trans,
 	}
 
 	/* check if there are no relations to this qgroup */
-	spin_lock(&fs_info->qgroup_lock);
 	qgroup = find_qgroup_rb(fs_info, qgroupid);
 	if (qgroup) {
 		if (!list_empty(&qgroup->groups) ||
!list_empty(&qgroup->members)) {
-			spin_unlock(&fs_info->qgroup_lock);
 			ret = -EBUSY;
 			goto out;
 		}
 	}
-	spin_unlock(&fs_info->qgroup_lock);
 
 	ret = del_qgroup_item(trans, quota_root, qgroupid);
 
@@ -1108,20 +1096,17 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 		       (unsigned long long)qgroupid);
 	}
 
-	spin_lock(&fs_info->qgroup_lock);
-
 	qgroup = find_qgroup_rb(fs_info, qgroupid);
 	if (!qgroup) {
 		ret = -ENOENT;
-		goto unlock;
+		goto out;
 	}
+	spin_lock(&fs_info->qgroup_lock);
 	qgroup->lim_flags = limit->flags;
 	qgroup->max_rfer = limit->max_rfer;
 	qgroup->max_excl = limit->max_excl;
 	qgroup->rsv_rfer = limit->rsv_rfer;
 	qgroup->rsv_excl = limit->rsv_excl;
-
-unlock:
 	spin_unlock(&fs_info->qgroup_lock);
 out:
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
-- 
1.7.11.7

--
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-Apr-07 10:50 UTC

head link

[PATCH V3 3/5] Btrfs: fix missing check before creating a qgroup relation

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

Step to reproduce:
		mkfs.btrfs <disk>
		mount <disk> <mnt>
		btrfs quota enable <mnt>
		btrfs qgroup assign 0/1 1/1 <mnt>
		umount <mnt>
		btrfs-debug-tree <disk> | grep QGROUP
If we want to add a qgroup relation, we should gurantee that
''src'' and ''dst'' exist, otherwise, such
qgroup relation should
not be allowed to create.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 466bcd6..575e7e4 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -956,6 +956,8 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle
*trans,
 			      struct btrfs_fs_info *fs_info, u64 src, u64 dst)
 {
 	struct btrfs_root *quota_root;
+	struct btrfs_qgroup *parent;
+	struct btrfs_qgroup *member;
 	int ret = 0;
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
@@ -964,6 +966,12 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle
*trans,
 		ret = -EINVAL;
 		goto out;
 	}
+	member = find_qgroup_rb(fs_info, src);
+	parent = find_qgroup_rb(fs_info, dst);
+	if (!member || !parent) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	ret = add_qgroup_relation_item(trans, quota_root, src, dst);
 	if (ret)
-- 
1.7.11.7

--
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-Apr-07 10:50 UTC

head link

[PATCH V3 4/5] Btrfs: fix missing check in the btrfs_qgroup_inherit()

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

The original code forgot to check ''inherit'', we should
gurantee that all the qgroups in the struct ''inherit'' exist.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 575e7e4..d0870c8 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1414,6 +1414,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 	struct btrfs_qgroup *srcgroup;
 	struct btrfs_qgroup *dstgroup;
 	u32 level_size = 0;
+	u64 nums;
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_enabled)
@@ -1424,6 +1425,20 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle
*trans,
 		goto out;
 	}
 
+	if (inherit) {
+		i_qgroups = (u64 *)(inherit + 1);
+		nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
+		       2 * inherit->num_excl_copies;
+		for (i = 0; i < nums; ++i) {
+			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
+			if (!srcgroup) {
+				ret = -EINVAL;
+				goto out;
+			}
+			++i_qgroups;
+		}
+	}
+
 	/*
 	 * create a tracking group for the subvol itself
 	 */
-- 
1.7.11.7

--
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-Apr-07 10:50 UTC

head link

[PATCH V3 5/5] Btrfs: fix a warning when updating qgroup limit

From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>

Step to reproduce:
	mkfs.btrfs <disk>
	mount <disk> <mnt>
	btrfs quota enable <mnt>
	btrfs qgroup limit 0/1 <mnt>
	dmesg

If the relative qgroup dosen''t exist, flag
''BTRFS_QGROUP_STATUS_
FLAG_INCONSISTENT'' will be set, and print the noise message.
This is wrong, we can just move find_qgroup_rb() before
update_qgroup_limit_item().this dosen''t change the logic of the
function. But it can avoid unnecessary noise message and wrong set of flag.

Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index d0870c8..deaa2e8 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1094,6 +1094,11 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 		goto out;
 	}
 
+	qgroup = find_qgroup_rb(fs_info, qgroupid);
+	if (!qgroup) {
+		ret = -ENOENT;
+		goto out;
+	}
 	ret = update_qgroup_limit_item(trans, quota_root, qgroupid,
 				       limit->flags, limit->max_rfer,
 				       limit->max_excl, limit->rsv_rfer,
@@ -1104,11 +1109,6 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 		       (unsigned long long)qgroupid);
 	}
 
-	qgroup = find_qgroup_rb(fs_info, qgroupid);
-	if (!qgroup) {
-		ret = -ENOENT;
-		goto out;
-	}
 	spin_lock(&fs_info->qgroup_lock);
 	qgroup->lim_flags = limit->flags;
 	qgroup->max_rfer = limit->max_rfer;
-- 
1.7.11.7

--
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-Apr-07 11:19 UTC

head link

Re: [PATCH V3 0/5] introduce a mutex lock and fix missing checks

Hello Arne,
	
	This patch-set mainly do two kind of things:
	1> introduce a mutex lock to avoid race conditions
	2> fix missing check before creating qgroup relations and
	btrfs_qgroup_inherit().

	I think the above two things should be fixed. However,
	this patch-set don''t have ioctl checks, i want to make it
	a RFC patch:

	all the quota configurations are loaded in memory,
	so before operating in disk we can have a check if
	it is really necessary to operate in disk.[for example, creating
	a group has been existed].

	However, if we don''t have this check, it should work
	ok for us too. Maybe to have a check in memory before will
        speed up the operations if operation should return errors.

	What do you think of ioctls checks?


Thanks,
Wang
	 > between operations in disk and in memory.
> 
> Holding mutex lock, we can fix missing check before creating
> a qgroup(whether ''src'' and ''dst''
exist).And we can guarantee
> that all the qgroups in ''inherit'' dose exist before
tracking
> qgroup.
> 
> V2->V3:
> 	rename ''quota_lock'' to
''qgroup_ioctl_lock'' and holding
> 	''qgroup_ioctl_lock'' inside qgroup.c to avoid serializing
> 	operations.
> V1->V2:
> 	use quota configurations in memory to speed up checks.
> 
> Wang Shilong (5):
>  Btrfs: introduce a mutex lock for btrfs quota operations
>  Btrfs: remove some unnecessary spin_lock usages
>  Btrfs: fix missing check before creating a qgroup relation
>  Btrfs: fix missing check in the btrfs_qgroup_inherit()
>  Btrfs: fix a warning when updating qgroup limit
> 
> fs/btrfs/ctree.h   |   3 ++
> fs/btrfs/disk-io.c |   1 +
> fs/btrfs/qgroup.c  | 136
++++++++++++++++++++++++++++++++++-------------------
> 3 files changed, 92 insertions(+), 48 deletions(-)
> 
> -- 
> 1.7.11.7
> 
--
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

Btrfs devel - Apr 2013 - [PATCH V3 0/5] introduce a mutex lock and fix missing checks

[PATCH V3 0/5] introduce a mutex lock and fix missing checks

[PATCH V3 1/5] Btrfs: introduce a mutex lock for btrfs quota operations

[PATCH V3 2/5] Btrfs: remove some unnecessary spin_lock usages

[PATCH V3 3/5] Btrfs: fix missing check before creating a qgroup relation

[PATCH V3 4/5] Btrfs: fix missing check in the btrfs_qgroup_inherit()

[PATCH V3 5/5] Btrfs: fix a warning when updating qgroup limit

Re: [PATCH V3 0/5] introduce a mutex lock and fix missing checks