[PATCH 1/3] btrfs: qgroup: return EINVAL if level of parent is not higher than child's.

[PATCH 1/3] btrfs: qgroup: return EINVAL if level of parent is not higher than child's.

[Dongsheng Yang]

When we create a qgroup inheriting a qgroup, we need to check the level
of them. Otherwise, there is a chance where a qgroup can inherit another qgroup
at the same level.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c | 6 ++++++
 fs/btrfs/qgroup.h | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 953befd..c2983a4 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2221,6 +2221,12 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 				ret = -EINVAL;
 				goto out;
 			}
+
+			if ((srcgroup->qgroupid >> BTRFS_QGROUP_LEVEL_SHIFT) <=
+			    (objectid >> BTRFS_QGROUP_LEVEL_SHIFT)) {
+				ret = -EINVAL;
+				goto out;
+			}
 			++i_qgroups;
 		}
 	}
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 18cc68c..67291ff 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -20,6 +20,15 @@
 #define __BTRFS_QGROUP__
 
 /*
+ * The ID of a qgroup consists of a level and a id, looks like: 1/3.
+ * The 1 is the level means this qgroup is in the level 1 and the 3
+ * is the id of this qgroup in level 1. We use a u64 number store a
+ * qgroup ID, the first 16bits stands for the level and the last 48bits
+ * stands for the id.
+ */
+#define BTRFS_QGROUP_LEVEL_SHIFT	48
+
+/*
  * A description of the operations, all of these operations only happen when we
  * are adding the 1st reference for that subvolume in the case of adding space
  * or on the last reference delete in the case of subtraction.  The only
-- 
1.8.4.2

[PATCH 2/3] btrfs: qgroup: allow user to remove qgroup which has parent but no child.

[Dongsheng Yang]

When a qgroup has parents but no child, it should be removable in
Theory. But currently, we can not remove it when it has
either parent or child.

Example:
	# btrfs quota enable /mnt
	# btrfs qgroup create 1/0 /mnt
	# btrfs qgroup create 2/0 /mnt
	# btrfs qgroup assign 1/0 2/0 /mnt
	# btrfs qgroup show -pcre /mnt
qgroupid rfer  excl  max_rfer max_excl parent  child
-------- ----  ----  -------- -------- ------  -----
0/5      16384 16384 0        0        ---     ---
1/0      0     0     0        0        2/0     ---
2/0      0     0     0        0        ---     1/0

At this time, there is no subvol or qgroup depending on qgroup 1/0.
Just a qgroup 2/0 is its parent, but 2/0 can work well without
1/0. So I think 1/0 should be removalbe. But:
	# btrfs qgroup destroy 1/0 /mnt
ERROR: unable to destroy quota group: Device or resource busy

This patch remove the check of qgroup->parent in removing it.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 fs/btrfs/qgroup.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c2983a4..c3b1e4f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1048,7 +1048,7 @@ out:
 	return ret;
 }
 
-int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
+int __del_qgroup_relation(struct btrfs_trans_handle *trans,
 			      struct btrfs_fs_info *fs_info, u64 src, u64 dst)
 {
 	struct btrfs_root *quota_root;
@@ -1058,7 +1058,6 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
 	int ret = 0;
 	int err;
 
-	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
 	if (!quota_root) {
 		ret = -EINVAL;
@@ -1089,7 +1088,18 @@ exist:
 	del_relation_rb(fs_info, src, dst);
 	spin_unlock(&fs_info->qgroup_lock);
 out:
+	return ret;
+}
+
+int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
+			      struct btrfs_fs_info *fs_info, u64 src, u64 dst)
+{
+	int ret = 0;
+
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
+	ret = __del_qgroup_relation(trans, fs_info, src, dst);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
+
 	return ret;
 }
 
@@ -1132,6 +1142,7 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
+	struct btrfs_qgroup_list *list;
 	int ret = 0;
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
@@ -1146,15 +1157,24 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 		ret = -ENOENT;
 		goto out;
 	} else {
-		/* check if there are no relations to this qgroup */
-		if (!list_empty(&qgroup->groups) ||
-		    !list_empty(&qgroup->members)) {
+		/* check if there are no children of this qgroup */
+		if (!list_empty(&qgroup->members)) {
 			ret = -EBUSY;
 			goto out;
 		}
 	}
 	ret = del_qgroup_item(trans, quota_root, qgroupid);
 
+	while (!list_empty(&qgroup->groups)) {
+		list = list_first_entry(&qgroup->groups,
+					struct btrfs_qgroup_list, next_group);
+		ret = __del_qgroup_relation(trans, fs_info,
+					   qgroupid,
+					   list->group->qgroupid);
+		if (ret)
+			goto out;
+	}
+
 	spin_lock(&fs_info->qgroup_lock);
 	del_qgroup_rb(quota_root->fs_info, qgroupid);
 	spin_unlock(&fs_info->qgroup_lock);
-- 
1.8.4.2

[PATCH 3/3] btrfs: qgroup: destroy related qgroup when removing subvolume if needed.

[Dongsheng Yang]

When removing a subvol, if the related qgroup has no child qgroup, we should destroy
it at the same time. Also remove the TODO entry in qgroup.c.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 fs/btrfs/inode.c  | 4 ++++
 fs/btrfs/qgroup.c | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e687bb0..31de211 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -59,6 +59,7 @@
 #include "backref.h"
 #include "hash.h"
 #include "props.h"
+#include "qgroup.h"
 
 struct btrfs_iget_args {
 	struct btrfs_key *location;
@@ -4029,6 +4030,9 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
 	ret = btrfs_update_inode_fallback(trans, root, dir);
 	if (ret)
 		btrfs_abort_transaction(trans, root, ret);
+	ret = btrfs_remove_qgroup(trans, root->fs_info, objectid);
+	if (ret == -EBUSY)
+		ret = 0;
 out:
 	btrfs_free_path(path);
 	return ret;
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c3b1e4f..303c078 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -35,7 +35,6 @@
 #include "qgroup.h"
 
 /* TODO XXX FIXME
- *  - subvol delete -> delete when ref goes to 0? delete limits also?
  *  - reorganize keys
  *  - compressed
  *  - sync
-- 
1.8.4.2

Re: [PATCH 3/3] btrfs: qgroup: destroy related qgroup when removing subvolume if needed.

[Wang Shilong]

Hello,

> 
> When removing a subvol, if the related qgroup has no child qgroup, we should destroy
> it at the same time. Also remove the TODO entry in qgroup.c.

My two cents here:

Take a quick look at this, i am not sure whether this is right way to do it.

Actually, i think we can only remove a subvolume qgroup safely when both refer and excl are 0,
because for subvolume removal case, to make qgroup accounting right, we need walk tree.

At this time, qgroup walking might have not finished, if we remove this qgroup directly,
accounting for this qgroup will be skipped.

Maybe we could remove such qgroup when at qgroup accounting, we find an qgroup’s refer and excl
are 0, we could remove it, or we do it at the background..

Also, there is another risk, that is to say, if qgroup accounting not work right, So
we might need gurantee that subvolume it going to be deleted or have been deleted at the same time,
then we could remove qgroup safely etc.


> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
> fs/btrfs/inode.c  | 4 ++++
> fs/btrfs/qgroup.c | 1 -
> 2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e687bb0..31de211 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -59,6 +59,7 @@
> #include "backref.h"
> #include "hash.h"
> #include "props.h"
> +#include "qgroup.h"
> 
> struct btrfs_iget_args {
> 	struct btrfs_key *location;
> @@ -4029,6 +4030,9 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
> 	ret = btrfs_update_inode_fallback(trans, root, dir);
> 	if (ret)
> 		btrfs_abort_transaction(trans, root, ret);
> +	ret = btrfs_remove_qgroup(trans, root->fs_info, objectid);
> +	if (ret == -EBUSY)
> +		ret = 0;
> out:
> 	btrfs_free_path(path);
> 	return ret;
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index c3b1e4f..303c078 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -35,7 +35,6 @@
> #include "qgroup.h"
> 
> /* TODO XXX FIXME
> - *  - subvol delete -> delete when ref goes to 0? delete limits also?
>  *  - reorganize keys
>  *  - compressed
>  *  - sync
> -- 
> 1.8.4.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Best Regards,
Wang Shilong

Re: [PATCH 3/3] btrfs: qgroup: destroy related qgroup when removing subvolume if needed.

[Dongsheng Yang]

On 01/22/2015 02:16 PM, Wang Shilong wrote:
> Hello,
>
>> When removing a subvol, if the related qgroup has no child qgroup, we should destroy
>> it at the same time. Also remove the TODO entry in qgroup.c.
> My two cents here:
>
> Take a quick look at this, i am not sure whether this is right way to do it.
>
> Actually, i think we can only remove a subvolume qgroup safely when both refer and excl are 0,
> because for subvolume removal case, to make qgroup accounting right, we need walk tree.
>
> At this time, qgroup walking might have not finished, if we remove this qgroup directly,
> accounting for this qgroup will be skipped.

Good point!
>
> Maybe we could remove such qgroup when at qgroup accounting, we find an qgroup’s refer and excl
> are 0, we could remove it, or we do it at the background..
>
> Also, there is another risk, that is to say, if qgroup accounting not work right, So
> we might need gurantee that subvolume it going to be deleted or have been deleted at the same time,
> then we could remove qgroup safely etc.

Yes, you are right. This is not a good timing to remove the qgroup directly.
I need some more investigation for a better solution.

As I have some emergency to deal with in this period, maybe the V2 will
come a little late.
Sorry about it.

Thanx
Yang
>
>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>> fs/btrfs/inode.c  | 4 ++++
>> fs/btrfs/qgroup.c | 1 -
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index e687bb0..31de211 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -59,6 +59,7 @@
>> #include "backref.h"
>> #include "hash.h"
>> #include "props.h"
>> +#include "qgroup.h"
>>
>> struct btrfs_iget_args {
>> 	struct btrfs_key *location;
>> @@ -4029,6 +4030,9 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
>> 	ret = btrfs_update_inode_fallback(trans, root, dir);
>> 	if (ret)
>> 		btrfs_abort_transaction(trans, root, ret);
>> +	ret = btrfs_remove_qgroup(trans, root->fs_info, objectid);
>> +	if (ret == -EBUSY)
>> +		ret = 0;
>> out:
>> 	btrfs_free_path(path);
>> 	return ret;
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index c3b1e4f..303c078 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -35,7 +35,6 @@
>> #include "qgroup.h"
>>
>> /* TODO XXX FIXME
>> - *  - subvol delete -> delete when ref goes to 0? delete limits also?
>>  *  - reorganize keys
>>  *  - compressed
>>  *  - sync
>> -- 
>> 1.8.4.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Best Regards,
> Wang Shilong
>
> .
>