* [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
2013-03-28 10:54 [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check Wang Shilong
@ 2013-03-28 10:53 ` Wang Shilong
2013-03-30 23:41 ` Arne Jansen
2013-04-02 12:19 ` Jan Schmidt
2013-03-28 10:54 ` [PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages Wang Shilong
` (4 subsequent siblings)
5 siblings, 2 replies; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 10:53 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox, wangsl-fnst
From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
This patch introduces mutex lock 'quota_lock', and makes
all the user change for quota protected by quota_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/ioctl.c | 16 ++++++++++++----
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6e81860..a11a8ed 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 operations for quota */
+ struct mutex quota_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 fe82d08..4552f14 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2250,6 +2250,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->quota_lock);
fs_info->qgroup_tree = RB_ROOT;
INIT_LIST_HEAD(&fs_info->dirty_qgroups);
fs_info->qgroup_seq = 1;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 222ce84..e2950f1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -752,7 +752,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0)
goto out_up_read;
-
+ mutex_lock(&BTRFS_I(dir)->root->fs_info->quota_lock);
if (snap_src) {
error = create_snapshot(snap_src, dir, dentry, name, namelen,
async_transid, readonly, inherit);
@@ -762,6 +762,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
}
if (!error)
fsnotify_mkdir(dir, dentry);
+ mutex_unlock(&BTRFS_I(dir)->root->fs_info->quota_lock);
out_up_read:
up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem);
out_dput:
@@ -3693,6 +3694,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
goto drop_write;
}
+ mutex_lock(&root->fs_info->quota_lock);
down_read(&root->fs_info->subvol_sem);
if (sa->cmd != BTRFS_QUOTA_CTL_RESCAN) {
trans = btrfs_start_transaction(root, 2);
@@ -3728,6 +3730,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
out:
kfree(sa);
up_read(&root->fs_info->subvol_sem);
+ mutex_unlock(&root->fs_info->quota_lock);
drop_write:
mnt_drop_write_file(file);
return ret;
@@ -3754,6 +3757,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
goto drop_write;
}
+ mutex_lock(&root->fs_info->quota_lock);
trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);
@@ -3775,6 +3779,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
out:
kfree(sa);
+ mutex_unlock(&root->fs_info->quota_lock);
drop_write:
mnt_drop_write_file(file);
return ret;
@@ -3805,11 +3810,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
ret = -EINVAL;
goto out;
}
-
+ mutex_lock(&root->fs_info->quota_lock);
trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);
- goto out;
+ goto out_unlock;
}
/* FIXME: check if the IDs really exist */
@@ -3824,6 +3829,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
if (err && !ret)
ret = err;
+out_unlock:
+ mutex_unlock(&root->fs_info->quota_lock);
out:
kfree(sa);
drop_write:
@@ -3852,7 +3859,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
ret = PTR_ERR(sa);
goto drop_write;
}
-
+ mutex_lock(&root->fs_info->quota_lock);
trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);
@@ -3874,6 +3881,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
out:
kfree(sa);
+ mutex_unlock(&root->fs_info->quota_lock);
drop_write:
mnt_drop_write_file(file);
return ret;
--
1.7.7.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check
@ 2013-03-28 10:54 Wang Shilong
2013-03-28 10:53 ` [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations Wang Shilong
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 10:54 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox, wangsl-fnst
The original code has only one spin_lock 'qgroup_lock' to protect quota
configurations on memory. If we want to add a BTRFS_QGROUP_INFO_KEY,
it will be added to Btree firstly and then update quota configurations
on memory, however,a race condition may happen between these operations.
For example:
->add_qqroup_info_item()
->add_qgroup_rb()
For the above case, del_qgroup_info_item() may happen before add_qgroup_rb().
What's worse, when we want to add a qgroup relations:
->add_qgroup_relation_item()
->add_qgroup_relations()
We don't have any checks whether 'src' and 'dst' exists before
add_qgroup_relation_item(), a race condition can also happen for the above case.
To avoid race conditions and have all the necessay checks, we introduce
a mutex 'quota_lock', and we make all the user change operations protected by
the mutex_lock.
The benefit of mutex_lock is more, with a mutex lock, we can remove
some spin_lock usages thus easing the burden of spin_lock 'qgroup_lock'.
V1->V2:
use quota configurations on memory to speed up ioctl checks
Wang Shilong (6):
Btrfs: introduce a mutex lock for btrfs quota operations
Btrfs: remove some unnecessary spin_lock usages
Btrfs: fix missing check before updating qgroup limit
Btrfs: fix missing check before creating/destroying a qgroup
Btrfs: fix missing check before assigning/removing qgroup relation
Btrfs: fix missing check before btrfs_qgroup_inherit()
fs/btrfs/ctree.h | 10 +++
fs/btrfs/disk-io.c | 1 +
fs/btrfs/ioctl.c | 43 ++++++++++----
fs/btrfs/qgroup.c | 167 +++++++++++++++++++++++++++++++++-------------------
4 files changed, 150 insertions(+), 71 deletions(-)
--
1.7.7.6
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages
2013-03-28 10:54 [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check Wang Shilong
2013-03-28 10:53 ` [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations Wang Shilong
@ 2013-03-28 10:54 ` Wang Shilong
2013-03-30 23:41 ` Arne Jansen
2013-03-28 10:54 ` [PATCH V2 3/6] Btrfs: fix missing check before updating qgroup limit Wang Shilong
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 10:54 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox, wangsl-fnst
From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
We use mutex_lock to protect all the user change operaions.
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 to here.Spin_lock must be fistly
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 | 42 +++++++++++++++---------------------------
1 files changed, 15 insertions(+), 27 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index e3598fa..7df372a 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -42,7 +42,6 @@
* - limit
* - caches fuer ulists
* - performance benchmarks
- * - check all ioctl parameters
*/
/*
@@ -98,7 +97,11 @@ struct btrfs_qgroup_list {
struct btrfs_qgroup *member;
};
-/* must be called with qgroup_lock held */
+/*
+ * don't need to be held by spin_lock since
+ * all the quota configurations on memory has been protected
+ * by mutex quota_lock.
+ */
static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
u64 qgroupid)
{
@@ -793,13 +796,10 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
int ret = 0;
int slot;
- 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;
+ return ret;
}
- spin_unlock(&fs_info->qgroup_lock);
/*
* initially create the quota tree
@@ -808,7 +808,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
BTRFS_QUOTA_TREE_OBJECTID);
if (IS_ERR(quota_root)) {
ret = PTR_ERR(quota_root);
- goto out;
+ return ret;
}
path = btrfs_alloc_path();
@@ -861,14 +861,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)
@@ -883,13 +880,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);
@@ -901,7 +897,6 @@ out_free_root:
free_extent_buffer(quota_root->commit_root);
kfree(quota_root);
}
-out:
return ret;
}
@@ -912,11 +907,10 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
struct btrfs_root *quota_root;
int ret = 0;
- spin_lock(&fs_info->qgroup_lock);
- if (!fs_info->quota_root) {
- spin_unlock(&fs_info->qgroup_lock);
+ if (!fs_info->quota_root)
return 0;
- }
+
+ spin_lock(&fs_info->qgroup_lock);
fs_info->quota_enabled = 0;
fs_info->pending_quota_state = 0;
quota_root = fs_info->quota_root;
@@ -1041,15 +1035,12 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
return -EINVAL;
/* 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);
+ if (!list_empty(&qgroup->groups) ||
+ !list_empty(&qgroup->members))
return -EBUSY;
- }
}
- spin_unlock(&fs_info->qgroup_lock);
ret = del_qgroup_item(trans, quota_root, qgroupid);
@@ -1081,20 +1072,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;
+ return ret;
}
+ 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);
return ret;
--
1.7.7.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 3/6] Btrfs: fix missing check before updating qgroup limit
2013-03-28 10:54 [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check Wang Shilong
2013-03-28 10:53 ` [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations Wang Shilong
2013-03-28 10:54 ` [PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages Wang Shilong
@ 2013-03-28 10:54 ` Wang Shilong
2013-03-28 10:54 ` [PATCH V2 4/6] Btrfs: fix missing check before creating/destroying a qgroup Wang Shilong
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 10:54 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox, wangsl-fnst
From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
We should check whether a qgroup exists before updating qgroup limit,
if the relative qgroup dosen't exsit, return -EEXIST and do not join
a transaction, fix it.
Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
fs/btrfs/ctree.h | 1 +
fs/btrfs/ioctl.c | 18 ++++++++++--------
fs/btrfs/qgroup.c | 15 ++++++++++++---
3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a11a8ed..3fc393d 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3094,6 +3094,7 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
struct btrfs_root *root, u64 bytes_used,
u64 type, u64 chunk_objectid, u64 chunk_offset,
u64 size);
+int btrfs_may_limit_qgroup(struct btrfs_root *root, u64 qgroupid);
int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
struct btrfs_root *root, u64 group_start);
void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e2950f1..7c72b12 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3859,20 +3859,22 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
ret = PTR_ERR(sa);
goto drop_write;
}
- mutex_lock(&root->fs_info->quota_lock);
- trans = btrfs_join_transaction(root);
- if (IS_ERR(trans)) {
- ret = PTR_ERR(trans);
- goto out;
- }
-
qgroupid = sa->qgroupid;
if (!qgroupid) {
/* take the current subvol as qgroup */
qgroupid = root->root_key.objectid;
}
- /* FIXME: check if the IDs really exist */
+ mutex_lock(&root->fs_info->quota_lock);
+ ret = btrfs_may_limit_qgroup(root, qgroupid);
+ if (ret)
+ goto out;
+
+ trans = btrfs_join_transaction(root);
+ if (IS_ERR(trans)) {
+ ret = PTR_ERR(trans);
+ goto out;
+ }
ret = btrfs_limit_qgroup(trans, root->fs_info, qgroupid, &sa->lim);
err = btrfs_end_transaction(trans, root);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 7df372a..bed0e22 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1059,9 +1059,6 @@ int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
struct btrfs_qgroup *qgroup;
int ret = 0;
- if (!quota_root)
- return -EINVAL;
-
ret = update_qgroup_limit_item(trans, quota_root, qgroupid,
limit->flags, limit->max_rfer,
limit->max_excl, limit->rsv_rfer,
@@ -1665,3 +1662,15 @@ void assert_qgroups_uptodate(struct btrfs_trans_handle *trans)
trans->delayed_ref_elem.seq);
BUG();
}
+
+int btrfs_may_limit_qgroup(struct btrfs_root *root, u64 qgroupid)
+{
+ struct btrfs_qgroup *qgroup = NULL;
+
+ if (!root->fs_info->quota_root)
+ return -EINVAL;
+ qgroup = find_qgroup_rb(root->fs_info, qgroupid);
+ if (!qgroup)
+ return -ENOENT;
+ return 0;
+}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 4/6] Btrfs: fix missing check before creating/destroying a qgroup
2013-03-28 10:54 [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check Wang Shilong
` (2 preceding siblings ...)
2013-03-28 10:54 ` [PATCH V2 3/6] Btrfs: fix missing check before updating qgroup limit Wang Shilong
@ 2013-03-28 10:54 ` Wang Shilong
2013-03-28 10:54 ` [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation Wang Shilong
2013-03-28 10:54 ` [PATCH V2 6/6] Btrfs: fix missing check before btrfs_qgroup_inherit() Wang Shilong
5 siblings, 0 replies; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 10:54 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox, wangsl-fnst
From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
For creating a qgroup, if the qgroup exsits, return -EEXIST.
For destroying a qgroup, if there are qgroup relations, return -EBUSY,
if the qgroup dosen't exist, return -ENOENT.
Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
fs/btrfs/ctree.h | 2 ++
fs/btrfs/ioctl.c | 5 ++++-
fs/btrfs/qgroup.c | 45 ++++++++++++++++++++++++++-------------------
3 files changed, 32 insertions(+), 20 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 3fc393d..9e4ccb3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3831,6 +3831,8 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 src, u64 dst);
int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 src, u64 dst);
+int btrfs_may_create_qgroup(struct btrfs_root *root,
+ struct btrfs_ioctl_qgroup_create_args *sa);
int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 qgroupid,
char *name);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7c72b12..f66d622 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3811,13 +3811,16 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
goto out;
}
mutex_lock(&root->fs_info->quota_lock);
+ ret = btrfs_may_create_qgroup(root, sa);
+ if (ret)
+ goto out_unlock;
+
trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);
goto out_unlock;
}
- /* FIXME: check if the IDs really exist */
if (sa->create) {
ret = btrfs_create_qgroup(trans, root->fs_info, sa->qgroupid,
NULL);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index bed0e22..6f57a98 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1003,15 +1003,13 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 qgroupid, char *name)
{
- struct btrfs_root *quota_root;
+ struct btrfs_root *quota_root = fs_info->quota_root;
struct btrfs_qgroup *qgroup;
int ret = 0;
- quota_root = fs_info->quota_root;
- if (!quota_root)
- return -EINVAL;
-
ret = add_qgroup_item(trans, quota_root, qgroupid);
+ if (ret)
+ return ret;
spin_lock(&fs_info->qgroup_lock);
qgroup = add_qgroup_rb(fs_info, qgroupid);
@@ -1026,22 +1024,9 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 qgroupid)
{
- struct btrfs_root *quota_root;
- struct btrfs_qgroup *qgroup;
+ struct btrfs_root *quota_root = fs_info->quota_root;
int ret = 0;
- quota_root = fs_info->quota_root;
- if (!quota_root)
- return -EINVAL;
-
- /* check if there are no relations to this qgroup */
- qgroup = find_qgroup_rb(fs_info, qgroupid);
- if (qgroup) {
- if (!list_empty(&qgroup->groups) ||
- !list_empty(&qgroup->members))
- return -EBUSY;
- }
-
ret = del_qgroup_item(trans, quota_root, qgroupid);
spin_lock(&fs_info->qgroup_lock);
@@ -1674,3 +1659,25 @@ int btrfs_may_limit_qgroup(struct btrfs_root *root, u64 qgroupid)
return -ENOENT;
return 0;
}
+
+int btrfs_may_create_qgroup(struct btrfs_root *root,
+ struct btrfs_ioctl_qgroup_create_args *sa)
+{
+ struct btrfs_qgroup *qgroup = NULL;
+
+ if (!root->fs_info->quota_root)
+ return -EINVAL;
+
+ qgroup = find_qgroup_rb(root->fs_info, sa->qgroupid);
+ if (sa->create) {
+ if (qgroup)
+ return -EEXIST;
+ return 0;
+ }
+ if (!qgroup)
+ return -ENOENT;
+ /* check if there are no relations to this qgroup */
+ if (!list_empty(&qgroup->groups) || !list_empty(&qgroup->members))
+ return -EBUSY;
+ return 0;
+}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation
2013-03-28 10:54 [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check Wang Shilong
` (3 preceding siblings ...)
2013-03-28 10:54 ` [PATCH V2 4/6] Btrfs: fix missing check before creating/destroying a qgroup Wang Shilong
@ 2013-03-28 10:54 ` Wang Shilong
2013-03-28 13:21 ` Josef Bacik
2013-03-28 10:54 ` [PATCH V2 6/6] Btrfs: fix missing check before btrfs_qgroup_inherit() Wang Shilong
5 siblings, 1 reply; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 10:54 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox, wangsl-fnst
From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
If the specified qgroup relation dosen't exist, removing qgroup operations
will return -ENOENT;
If the specified qgroup relation exists, assigning qgroup relations will
return -EEXIST.
Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
fs/btrfs/ctree.h | 2 ++
fs/btrfs/ioctl.c | 4 +++-
fs/btrfs/qgroup.c | 40 ++++++++++++++++++++++++++++++----------
3 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 9e4ccb3..1ca1c63 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3827,6 +3827,8 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
int btrfs_quota_disable(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info);
int btrfs_quota_rescan(struct btrfs_fs_info *fs_info);
+int btrfs_may_assign_relation(struct btrfs_root *root,
+ struct btrfs_ioctl_qgroup_assign_args *sa);
int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 src, u64 dst);
int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f66d622..701d812 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3758,13 +3758,15 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
}
mutex_lock(&root->fs_info->quota_lock);
+ ret = btrfs_may_assign_qgroup(root, sa);
+ if (ret)
+ goto out;
trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);
goto out;
}
- /* FIXME: check if the IDs really exist */
if (sa->assign) {
ret = btrfs_add_qgroup_relation(trans, root->fs_info,
sa->src, sa->dst);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 6f57a98..0e09ac6 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -952,13 +952,9 @@ int btrfs_quota_rescan(struct btrfs_fs_info *fs_info)
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_root *quota_root = fs_info->quota_root;
int ret = 0;
- quota_root = fs_info->quota_root;
- if (!quota_root)
- return -EINVAL;
-
ret = add_qgroup_relation_item(trans, quota_root, src, dst);
if (ret)
return ret;
@@ -979,14 +975,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans,
int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 src, u64 dst)
{
- struct btrfs_root *quota_root;
+ struct btrfs_root *quota_root = fs_info->quota_root;
int ret = 0;
int err;
- quota_root = fs_info->quota_root;
- if (!quota_root)
- return -EINVAL;
-
ret = del_qgroup_relation_item(trans, quota_root, src, dst);
err = del_qgroup_relation_item(trans, quota_root, dst, src);
if (err && !ret)
@@ -1681,3 +1673,31 @@ int btrfs_may_create_qgroup(struct btrfs_root *root,
return -EBUSY;
return 0;
}
+
+int btrfs_may_assign_qgroup(struct btrfs_root *root,
+ struct btrfs_ioctl_qgroup_assign_args *sa)
+{
+
+ struct btrfs_qgroup *parent = NULL;
+ struct btrfs_qgroup *member = NULL;
+ struct btrfs_qgroup_list *list;
+
+ if (!root->fs_info->quota_root)
+ return -EINVAL;
+
+ member = find_qgroup_rb(root->fs_info, sa->src);
+ parent = find_qgroup_rb(root->fs_info, sa->dst);
+ if (!member || !parent)
+ return -ENOENT;
+
+ list_for_each_entry(list, &member->groups, next_group) {
+ if (list->group == parent) {
+ if (sa->assign)
+ return -EEXIST;
+ return 0;
+ }
+ }
+ if (sa->assign)
+ return 0;
+ return -ENOENT;
+}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V2 6/6] Btrfs: fix missing check before btrfs_qgroup_inherit()
2013-03-28 10:54 [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check Wang Shilong
` (4 preceding siblings ...)
2013-03-28 10:54 ` [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation Wang Shilong
@ 2013-03-28 10:54 ` Wang Shilong
5 siblings, 0 replies; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 10:54 UTC (permalink / raw)
To: linux-btrfs; +Cc: sensille, miaox, wangsl-fnst
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 before
btrfs_qgroup_inherit().
Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
---
fs/btrfs/ctree.h | 2 ++
fs/btrfs/ioctl.c | 6 ++++++
fs/btrfs/qgroup.c | 29 ++++++++++++++++++++++++++---
3 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 1ca1c63..0e73385 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3855,6 +3855,8 @@ int btrfs_qgroup_account_ref(struct btrfs_trans_handle *trans,
struct btrfs_delayed_extent_op *extent_op);
int btrfs_run_qgroups(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info);
+int btrfs_may_inherit_qgroup(struct btrfs_root *root,
+ struct btrfs_qgroup_inherit *inherit);
int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
struct btrfs_qgroup_inherit *inherit);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 701d812..96fda7e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -753,6 +753,11 @@ static noinline int btrfs_mksubvol(struct path *parent,
if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0)
goto out_up_read;
mutex_lock(&BTRFS_I(dir)->root->fs_info->quota_lock);
+
+ error = btrfs_may_inherit_qgroup(BTRFS_I(dir)->root, inherit);
+ if (error)
+ goto out_unlock_mutex;
+
if (snap_src) {
error = create_snapshot(snap_src, dir, dentry, name, namelen,
async_transid, readonly, inherit);
@@ -762,6 +767,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
}
if (!error)
fsnotify_mkdir(dir, dentry);
+out_unlock_mutex:
mutex_unlock(&BTRFS_I(dir)->root->fs_info->quota_lock);
out_up_read:
up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 0e09ac6..14fca77 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1359,9 +1359,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
if (!fs_info->quota_enabled)
return 0;
- if (!quota_root)
- return -EINVAL;
-
/*
* create a tracking group for the subvol itself
*/
@@ -1701,3 +1698,29 @@ int btrfs_may_assign_qgroup(struct btrfs_root *root,
return 0;
return -ENOENT;
}
+
+int btrfs_may_inherit_qgroup(struct btrfs_root *root,
+ struct btrfs_qgroup_inherit *inherit)
+{
+ u64 i = 0;
+ u64 *i_qgroups = NULL;
+ u64 nums = 0;
+ struct btrfs_qgroup *qgroup = NULL;
+
+ if (!inherit)
+ return 0;
+ if (!root->fs_info->quota_root)
+ return -EINVAL;
+
+ 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) {
+ qgroup = find_qgroup_rb(root->fs_info, *i_qgroups);
+ if (!qgroup)
+ return -EINVAL;
+
+ ++i_qgroups;
+ }
+ return 0;
+}
--
1.7.7.6
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation
2013-03-28 10:54 ` [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation Wang Shilong
@ 2013-03-28 13:21 ` Josef Bacik
2013-03-28 14:06 ` Wang Shilong
0 siblings, 1 reply; 14+ messages in thread
From: Josef Bacik @ 2013-03-28 13:21 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, sensille, miaox, wangsl-fnst
On Thu, Mar 28, 2013 at 04:54:46AM -0600, Wang Shilong wrote:
> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>
> If the specified qgroup relation dosen't exist, removing qgroup operations
> will return -ENOENT;
>
> If the specified qgroup relation exists, assigning qgroup relations will
> return -EEXIST.
>
> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
Fails to compile with
fs/btrfs/ioctl.c: In function ‘btrfs_ioctl_qgroup_assign’:
fs/btrfs/ioctl.c:3771:2: error: implicit declaration of function
‘btrfs_may_assign_qgroup’ [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
Please fix and resubmit. Thanks,
Josef
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation
2013-03-28 13:21 ` Josef Bacik
@ 2013-03-28 14:06 ` Wang Shilong
0 siblings, 0 replies; 14+ messages in thread
From: Wang Shilong @ 2013-03-28 14:06 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, sensille, miaox, wangsl-fnst
Hello,
> On Thu, Mar 28, 2013 at 04:54:46AM -0600, Wang Shilong wrote:
>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>
>> If the specified qgroup relation dosen't exist, removing qgroup operations
>> will return -ENOENT;
>>
>> If the specified qgroup relation exists, assigning qgroup relations will
>> return -EEXIST.
>>
>> Signed-off-by: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
>
> Fails to compile with
>
> fs/btrfs/ioctl.c: In function ‘btrfs_ioctl_qgroup_assign’:
> fs/btrfs/ioctl.c:3771:2: error: implicit declaration of function
> ‘btrfs_may_assign_qgroup’ [-Werror=implicit-function-declaration]
> cc1: some warnings being treated as errors
>
> Please fix and resubmit. Thanks,
Sorry for such stupid errors, i promise that i have compile and test patch-set.
Things go like this, because when i compile kernel with patch-set , this errors happen,
i just fix it but don't update commit.
However, when i pass the test, i forget the compile errors…..
Next time, i will take care of that…
Thanks very much for your testing!
Thanks,
Wang
>
> Josef
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
2013-03-28 10:53 ` [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations Wang Shilong
@ 2013-03-30 23:41 ` Arne Jansen
2013-03-31 1:12 ` Wang Shilong
2013-04-02 12:19 ` Jan Schmidt
1 sibling, 1 reply; 14+ messages in thread
From: Arne Jansen @ 2013-03-30 23:41 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, miaox, wangsl-fnst
On 03/28/13 11:53, Wang Shilong wrote:
> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>
> This patch introduces mutex lock 'quota_lock', and makes
> all the user change for quota protected by quota_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/ioctl.c | 16 ++++++++++++----
> 3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 6e81860..a11a8ed 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 operations for quota */
> + struct mutex quota_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 fe82d08..4552f14 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2250,6 +2250,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->quota_lock);
> fs_info->qgroup_tree = RB_ROOT;
> INIT_LIST_HEAD(&fs_info->dirty_qgroups);
> fs_info->qgroup_seq = 1;
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 222ce84..e2950f1 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -752,7 +752,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
>
> if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0)
> goto out_up_read;
> -
> + mutex_lock(&BTRFS_I(dir)->root->fs_info->quota_lock);
> if (snap_src) {
> error = create_snapshot(snap_src, dir, dentry, name, namelen,
> async_transid, readonly, inherit);
> @@ -762,6 +762,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
> }
> if (!error)
> fsnotify_mkdir(dir, dentry);
> + mutex_unlock(&BTRFS_I(dir)->root->fs_info->quota_lock);
You are completely serializing subvolume operations here. I'd prefer if
you'd move the lock to a lower level to only protect the quota
operations. Can't you move the lock completely to qgroup.c?
> out_up_read:
> up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem);
> out_dput:
> @@ -3693,6 +3694,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
> goto drop_write;
> }
>
> + mutex_lock(&root->fs_info->quota_lock);
> down_read(&root->fs_info->subvol_sem);
> if (sa->cmd != BTRFS_QUOTA_CTL_RESCAN) {
> trans = btrfs_start_transaction(root, 2);
> @@ -3728,6 +3730,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
> out:
> kfree(sa);
> up_read(&root->fs_info->subvol_sem);
> + mutex_unlock(&root->fs_info->quota_lock);
> drop_write:
> mnt_drop_write_file(file);
> return ret;
> @@ -3754,6 +3757,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
> goto drop_write;
> }
>
> + mutex_lock(&root->fs_info->quota_lock);
> trans = btrfs_join_transaction(root);
> if (IS_ERR(trans)) {
> ret = PTR_ERR(trans);
> @@ -3775,6 +3779,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>
> out:
> kfree(sa);
> + mutex_unlock(&root->fs_info->quota_lock);
> drop_write:
> mnt_drop_write_file(file);
> return ret;
> @@ -3805,11 +3810,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
> ret = -EINVAL;
> goto out;
> }
> -
> + mutex_lock(&root->fs_info->quota_lock);
> trans = btrfs_join_transaction(root);
> if (IS_ERR(trans)) {
> ret = PTR_ERR(trans);
> - goto out;
> + goto out_unlock;
> }
>
> /* FIXME: check if the IDs really exist */
> @@ -3824,6 +3829,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
> if (err && !ret)
> ret = err;
>
> +out_unlock:
> + mutex_unlock(&root->fs_info->quota_lock);
> out:
> kfree(sa);
> drop_write:
> @@ -3852,7 +3859,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
> ret = PTR_ERR(sa);
> goto drop_write;
> }
> -
> + mutex_lock(&root->fs_info->quota_lock);
> trans = btrfs_join_transaction(root);
> if (IS_ERR(trans)) {
> ret = PTR_ERR(trans);
> @@ -3874,6 +3881,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
>
> out:
> kfree(sa);
> + mutex_unlock(&root->fs_info->quota_lock);
> drop_write:
> mnt_drop_write_file(file);
> return ret;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages
2013-03-28 10:54 ` [PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages Wang Shilong
@ 2013-03-30 23:41 ` Arne Jansen
0 siblings, 0 replies; 14+ messages in thread
From: Arne Jansen @ 2013-03-30 23:41 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, miaox, wangsl-fnst
On 03/28/13 11:54, Wang Shilong wrote:
> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>
> We use mutex_lock to protect all the user change operaions.
> 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 to here.Spin_lock must be fistly
> 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 | 42 +++++++++++++++---------------------------
> 1 files changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index e3598fa..7df372a 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -42,7 +42,6 @@
> * - limit
> * - caches fuer ulists
> * - performance benchmarks
> - * - check all ioctl parameters
> */
>
> /*
> @@ -98,7 +97,11 @@ struct btrfs_qgroup_list {
> struct btrfs_qgroup *member;
> };
>
> -/* must be called with qgroup_lock held */
instead it must be called with quota_lock held. The rest looks
correct to me.
-Arne
> +/*
> + * don't need to be held by spin_lock since
> + * all the quota configurations on memory has been protected
> + * by mutex quota_lock.
> + */
> static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
> u64 qgroupid)
> {
> @@ -793,13 +796,10 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> int ret = 0;
> int slot;
>
> - 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;
> + return ret;
> }
> - spin_unlock(&fs_info->qgroup_lock);
>
> /*
> * initially create the quota tree
> @@ -808,7 +808,7 @@ int btrfs_quota_enable(struct btrfs_trans_handle *trans,
> BTRFS_QUOTA_TREE_OBJECTID);
> if (IS_ERR(quota_root)) {
> ret = PTR_ERR(quota_root);
> - goto out;
> + return ret;
> }
>
> path = btrfs_alloc_path();
> @@ -861,14 +861,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)
> @@ -883,13 +880,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);
> @@ -901,7 +897,6 @@ out_free_root:
> free_extent_buffer(quota_root->commit_root);
> kfree(quota_root);
> }
> -out:
> return ret;
> }
>
> @@ -912,11 +907,10 @@ int btrfs_quota_disable(struct btrfs_trans_handle *trans,
> struct btrfs_root *quota_root;
> int ret = 0;
>
> - spin_lock(&fs_info->qgroup_lock);
> - if (!fs_info->quota_root) {
> - spin_unlock(&fs_info->qgroup_lock);
> + if (!fs_info->quota_root)
> return 0;
> - }
> +
> + spin_lock(&fs_info->qgroup_lock);
> fs_info->quota_enabled = 0;
> fs_info->pending_quota_state = 0;
> quota_root = fs_info->quota_root;
> @@ -1041,15 +1035,12 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
> return -EINVAL;
>
> /* 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);
> + if (!list_empty(&qgroup->groups) ||
> + !list_empty(&qgroup->members))
> return -EBUSY;
> - }
> }
> - spin_unlock(&fs_info->qgroup_lock);
>
> ret = del_qgroup_item(trans, quota_root, qgroupid);
>
> @@ -1081,20 +1072,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;
> + return ret;
> }
> + 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);
>
> return ret;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
2013-03-30 23:41 ` Arne Jansen
@ 2013-03-31 1:12 ` Wang Shilong
0 siblings, 0 replies; 14+ messages in thread
From: Wang Shilong @ 2013-03-31 1:12 UTC (permalink / raw)
To: Arne Jansen; +Cc: linux-btrfs, miaox, wangsl-fnst
Hello Arne,
> On 03/28/13 11:53, Wang Shilong wrote:
>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>
>> This patch introduces mutex lock 'quota_lock', and makes
>> all the user change for quota protected by quota_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/ioctl.c | 16 ++++++++++++----
>> 3 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 6e81860..a11a8ed 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 operations for quota */
>> + struct mutex quota_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 fe82d08..4552f14 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2250,6 +2250,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->quota_lock);
>> fs_info->qgroup_tree = RB_ROOT;
>> INIT_LIST_HEAD(&fs_info->dirty_qgroups);
>> fs_info->qgroup_seq = 1;
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 222ce84..e2950f1 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -752,7 +752,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
>>
>> if (btrfs_root_refs(&BTRFS_I(dir)->root->root_item) == 0)
>> goto out_up_read;
>> -
>> + mutex_lock(&BTRFS_I(dir)->root->fs_info->quota_lock);
>> if (snap_src) {
>> error = create_snapshot(snap_src, dir, dentry, name, namelen,
>> async_transid, readonly, inherit);
>> @@ -762,6 +762,7 @@ static noinline int btrfs_mksubvol(struct path *parent,
>> }
>> if (!error)
>> fsnotify_mkdir(dir, dentry);
>> + mutex_unlock(&BTRFS_I(dir)->root->fs_info->quota_lock);
>
> You are completely serializing subvolume operations here. I'd prefer if
> you'd move the lock to a lower level to only protect the quota
> operations. Can't you move the lock completely to qgroup.c?
I have thought about this.
It really should not start a transaction if the checks fail.So i have all the necessary checks
before a transaction!
For user quota change operations, i think it is ok to serialize the operations.
The only place may be create_snapshot()/create_subvolume(). Maybe i can move the mutex_lock
to the place where btrfs_qgroup_inherit() is called.
Thanks,
wang
>
>> out_up_read:
>> up_read(&BTRFS_I(dir)->root->fs_info->subvol_sem);
>> out_dput:
>> @@ -3693,6 +3694,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>> goto drop_write;
>> }
>>
>> + mutex_lock(&root->fs_info->quota_lock);
>> down_read(&root->fs_info->subvol_sem);
>> if (sa->cmd != BTRFS_QUOTA_CTL_RESCAN) {
>> trans = btrfs_start_transaction(root, 2);
>> @@ -3728,6 +3730,7 @@ static long btrfs_ioctl_quota_ctl(struct file *file, void __user *arg)
>> out:
>> kfree(sa);
>> up_read(&root->fs_info->subvol_sem);
>> + mutex_unlock(&root->fs_info->quota_lock);
>> drop_write:
>> mnt_drop_write_file(file);
>> return ret;
>> @@ -3754,6 +3757,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>> goto drop_write;
>> }
>>
>> + mutex_lock(&root->fs_info->quota_lock);
>> trans = btrfs_join_transaction(root);
>> if (IS_ERR(trans)) {
>> ret = PTR_ERR(trans);
>> @@ -3775,6 +3779,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>>
>> out:
>> kfree(sa);
>> + mutex_unlock(&root->fs_info->quota_lock);
>> drop_write:
>> mnt_drop_write_file(file);
>> return ret;
>> @@ -3805,11 +3810,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
>> ret = -EINVAL;
>> goto out;
>> }
>> -
>> + mutex_lock(&root->fs_info->quota_lock);
>> trans = btrfs_join_transaction(root);
>> if (IS_ERR(trans)) {
>> ret = PTR_ERR(trans);
>> - goto out;
>> + goto out_unlock;
>> }
>>
>> /* FIXME: check if the IDs really exist */
>> @@ -3824,6 +3829,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
>> if (err && !ret)
>> ret = err;
>>
>> +out_unlock:
>> + mutex_unlock(&root->fs_info->quota_lock);
>> out:
>> kfree(sa);
>> drop_write:
>> @@ -3852,7 +3859,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
>> ret = PTR_ERR(sa);
>> goto drop_write;
>> }
>> -
>> + mutex_lock(&root->fs_info->quota_lock);
>> trans = btrfs_join_transaction(root);
>> if (IS_ERR(trans)) {
>> ret = PTR_ERR(trans);
>> @@ -3874,6 +3881,7 @@ static long btrfs_ioctl_qgroup_limit(struct file *file, void __user *arg)
>>
>> out:
>> kfree(sa);
>> + mutex_unlock(&root->fs_info->quota_lock);
>> drop_write:
>> mnt_drop_write_file(file);
>> return ret;
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
2013-03-28 10:53 ` [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations Wang Shilong
2013-03-30 23:41 ` Arne Jansen
@ 2013-04-02 12:19 ` Jan Schmidt
2013-04-02 12:26 ` Wang Shilong
1 sibling, 1 reply; 14+ messages in thread
From: Jan Schmidt @ 2013-04-02 12:19 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-btrfs, sensille, miaox, wangsl-fnst
Hi Wang,
On Thu, March 28, 2013 at 11:53 (+0100), Wang Shilong wrote:
> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>
> This patch introduces mutex lock 'quota_lock', and makes
> all the user change for quota protected by quota_lock.
Can you please add a few lines why this lock is needed? I.e., which ioctls fail
without that kind of synchronization?
> 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/ioctl.c | 16 ++++++++++++----
> 3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 6e81860..a11a8ed 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 operations for quota */
> + struct mutex quota_lock;
Having fs_info->qgroup_lock and fs_info->quota_lock going to be a major source
of confusion. I'd call the new one qgroup_ioctl_lock or ioctl_qgroup_lock instead.
Furthermore, the term "quota" was intentionally left unused to leave room for
other quota implementations later (user quota).
And, please, use --thread with git send-email for related patches to get correct
headers.
Thanks,
-Jan
> [snip]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations
2013-04-02 12:19 ` Jan Schmidt
@ 2013-04-02 12:26 ` Wang Shilong
0 siblings, 0 replies; 14+ messages in thread
From: Wang Shilong @ 2013-04-02 12:26 UTC (permalink / raw)
To: Jan Schmidt; +Cc: linux-btrfs, sensille, miaox, wangsl-fnst
Hello Jan,
> Hi Wang,
>
> On Thu, March 28, 2013 at 11:53 (+0100), Wang Shilong wrote:
>> From: Wang Shilong <wangsl-fnst@cn.fujitsu.com>
>>
>> This patch introduces mutex lock 'quota_lock', and makes
>> all the user change for quota protected by quota_lock.
>
> Can you please add a few lines why this lock is needed? I.e., which ioctls fail
> without that kind of synchronization?
I have explained it clearly in [PATCH V2 0/6].
http://marc.info/?l=linux-btrfs&m=136446806332680&w=2
>
>> 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/ioctl.c | 16 ++++++++++++----
>> 3 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 6e81860..a11a8ed 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 operations for quota */
>> + struct mutex quota_lock;
>
> Having fs_info->qgroup_lock and fs_info->quota_lock going to be a major source
> of confusion. I'd call the new one qgroup_ioctl_lock or ioctl_qgroup_lock instead.
It is reasonable, but i will move the mutex lock to group.c to avoid serialize
operations.
>
> Furthermore, the term "quota" was intentionally left unused to leave room for
> other quota implementations later (user quota).
>
> And, please, use --thread with git send-email for related patches to get correct
> headers.
Thanks for correcting me~.
Thanks,
Wang
>
> Thanks,
> -Jan
>
>> [snip]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-04-02 12:26 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-28 10:54 [PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check Wang Shilong
2013-03-28 10:53 ` [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations Wang Shilong
2013-03-30 23:41 ` Arne Jansen
2013-03-31 1:12 ` Wang Shilong
2013-04-02 12:19 ` Jan Schmidt
2013-04-02 12:26 ` Wang Shilong
2013-03-28 10:54 ` [PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages Wang Shilong
2013-03-30 23:41 ` Arne Jansen
2013-03-28 10:54 ` [PATCH V2 3/6] Btrfs: fix missing check before updating qgroup limit Wang Shilong
2013-03-28 10:54 ` [PATCH V2 4/6] Btrfs: fix missing check before creating/destroying a qgroup Wang Shilong
2013-03-28 10:54 ` [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation Wang Shilong
2013-03-28 13:21 ` Josef Bacik
2013-03-28 14:06 ` Wang Shilong
2013-03-28 10:54 ` [PATCH V2 6/6] Btrfs: fix missing check before btrfs_qgroup_inherit() Wang Shilong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.