[PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check

[PATCH V2 0/6] add a mutex lock to avoid race condition and have all the ioctls check

[Wang Shilong]

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

[PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations

[Wang Shilong]

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

Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations

[Arne Jansen]

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

Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations

[Wang Shilong]

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

Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations

[Jan Schmidt]

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]

Re: [PATCH V2 1/6] Btrfs: introduce a mutex lock for btrfs quota operations

[Wang Shilong]

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]

[PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages

[Wang Shilong]

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

Re: [PATCH V2 2/6] Btrfs: remove some unnecessary spin_lock usages

[Arne Jansen]

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

[PATCH V2 3/6] Btrfs: fix missing check before updating qgroup limit

[Wang Shilong]

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

[PATCH V2 4/6] Btrfs: fix missing check before creating/destroying a qgroup

[Wang Shilong]

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

[PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation

[Wang Shilong]

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

Re: [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation

[Josef Bacik]

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

Re: [PATCH V2 5/6] Btrfs: fix missing check before assigning/removing qgroup relation

[Wang Shilong]

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

[PATCH V2 6/6] Btrfs: fix missing check before btrfs_qgroup_inherit()

[Wang Shilong]

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