[PATCH 0/3] Btrfs: qgroup: part-3: bug fixes for deleting subvolume.

[PATCH 0/3] Btrfs: qgroup: part-3: bug fixes for deleting subvolume.

[Dongsheng Yang]

Hi all,
	This is a part3 for qgroup, it is based on
	Btrfs: qgroup: part-2: bug fixes.

Dongsheng Yang (3):
  btrfs: qgroup: return EINVAL if level of parent is not higher than
    child's.
  btrfs: qgroup: allow to remove qgroup which has parent but no child.
  btrfs: qgroup: fix a wrong parameter of no_quota.

 fs/btrfs/extent-tree.c |  8 ++++++--
 fs/btrfs/qgroup.c      | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 7 deletions(-)

-- 
1.8.4.2

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

[Dongsheng Yang]

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

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

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 5a1463d..d519055 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2244,6 +2244,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 				ret = -EINVAL;
 				goto out;
 			}
+
+			if ((srcgroup->qgroupid >> 48) <= (objectid >> 48)) {
+				ret = -EINVAL;
+				goto out;
+			}
 			++i_qgroups;
 		}
 	}
-- 
1.8.4.2

[PATCH 2/3] btrfs: qgroup: allow 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 I think. 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 it.
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,
then we can remove a qgroup when it has a parent.

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 d519055..28b0aa5 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1062,7 +1062,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;
@@ -1072,7 +1072,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;
@@ -1103,7 +1102,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;
 }
 
@@ -1146,6 +1156,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);
@@ -1160,15 +1171,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: fix a wrong parameter of no_quota.

[Dongsheng Yang]

In function of __btrfs_mod_ref(), we are passing the no_quota=1
to process_func() anyway. It will make the numbers in qgroup be
wrong when deleting a subvolume. The data size in a deleted subvolume
will never be decressed from all related qgroups.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 fs/btrfs/extent-tree.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 652be74..f59809c 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 	int i;
 	int level;
 	int ret = 0;
+	int no_quota = 0;
 	int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *,
 			    u64, u64, u64, u64, u64, u64, int);
 
@@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 	else
 		parent = 0;
 
+	if (!root->fs_info->quota_enabled || !is_fstree(ref_root))
+		no_quota = 1;
+
 	for (i = 0; i < nritems; i++) {
 		if (level == 0) {
 			btrfs_item_key_to_cpu(buf, &key, i);
@@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 			key.offset -= btrfs_file_extent_offset(buf, fi);
 			ret = process_func(trans, root, bytenr, num_bytes,
 					   parent, ref_root, key.objectid,
-					   key.offset, 1);
+					   key.offset, no_quota);
 			if (ret)
 				goto fail;
 		} else {
@@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 			num_bytes = root->nodesize;
 			ret = process_func(trans, root, bytenr, num_bytes,
 					   parent, ref_root, level - 1, 0,
-					   1);
+					   no_quota);
 			if (ret)
 				goto fail;
 		}
-- 
1.8.4.2

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Filipe David Manana]

On Tue, Feb 10, 2015 at 10:24 AM, Dongsheng Yang
<yangds.fnst@cn.fujitsu.com> wrote:
> In function of __btrfs_mod_ref(), we are passing the no_quota=1
> to process_func() anyway. It will make the numbers in qgroup be
> wrong when deleting a subvolume. The data size in a deleted subvolume
> will never be decressed from all related qgroups.

How about getting a test case for xfstests for this?

thanks

>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/extent-tree.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 652be74..f59809c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>         int i;
>         int level;
>         int ret = 0;
> +       int no_quota = 0;
>         int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *,
>                             u64, u64, u64, u64, u64, u64, int);
>
> @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>         else
>                 parent = 0;
>
> +       if (!root->fs_info->quota_enabled || !is_fstree(ref_root))
> +               no_quota = 1;
> +
>         for (i = 0; i < nritems; i++) {
>                 if (level == 0) {
>                         btrfs_item_key_to_cpu(buf, &key, i);
> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>                         key.offset -= btrfs_file_extent_offset(buf, fi);
>                         ret = process_func(trans, root, bytenr, num_bytes,
>                                            parent, ref_root, key.objectid,
> -                                          key.offset, 1);
> +                                          key.offset, no_quota);
>                         if (ret)
>                                 goto fail;
>                 } else {
> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>                         num_bytes = root->nodesize;
>                         ret = process_func(trans, root, bytenr, num_bytes,
>                                            parent, ref_root, level - 1, 0,
> -                                          1);
> +                                          no_quota);
>                         if (ret)
>                                 goto fail;
>                 }
> --
> 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



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Dongsheng Yang]

On 02/10/2015 07:24 PM, Filipe David Manana wrote:
> On Tue, Feb 10, 2015 at 10:24 AM, Dongsheng Yang
> <yangds.fnst@cn.fujitsu.com> wrote:
>> In function of __btrfs_mod_ref(), we are passing the no_quota=1
>> to process_func() anyway. It will make the numbers in qgroup be
>> wrong when deleting a subvolume. The data size in a deleted subvolume
>> will never be decressed from all related qgroups.
> How about getting a test case for xfstests for this?

Good idea!!

Will do it.

Thanx
>
> thanks
>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   fs/btrfs/extent-tree.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 652be74..f59809c 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>>          int i;
>>          int level;
>>          int ret = 0;
>> +       int no_quota = 0;
>>          int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *,
>>                              u64, u64, u64, u64, u64, u64, int);
>>
>> @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>>          else
>>                  parent = 0;
>>
>> +       if (!root->fs_info->quota_enabled || !is_fstree(ref_root))
>> +               no_quota = 1;
>> +
>>          for (i = 0; i < nritems; i++) {
>>                  if (level == 0) {
>>                          btrfs_item_key_to_cpu(buf, &key, i);
>> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>>                          key.offset -= btrfs_file_extent_offset(buf, fi);
>>                          ret = process_func(trans, root, bytenr, num_bytes,
>>                                             parent, ref_root, key.objectid,
>> -                                          key.offset, 1);
>> +                                          key.offset, no_quota);
>>                          if (ret)
>>                                  goto fail;
>>                  } else {
>> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>>                          num_bytes = root->nodesize;
>>                          ret = process_func(trans, root, bytenr, num_bytes,
>>                                             parent, ref_root, level - 1, 0,
>> -                                          1);
>> +                                          no_quota);
>>                          if (ret)
>>                                  goto fail;
>>                  }
>> --
>> 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
>
>

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Dongsheng Yang]

Hi Josef and Mark,

I saw the commit message of 1152651a:

[btrfs: qgroup: account shared subtrees during snapshot delete]

that SUBTREE operation was introduced to account the quota number
in subvolume deleting.

But, I found it does not work well currently：

Create subvolume '/mnt/sub'
+ btrfs qgroup show -prce /mnt
qgroupid rfer excl max_rfer max_excl parent child
-------- ---- ---- -------- -------- ------ -----
0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
0/257 16.00KiB 16.00KiB 0.00B 0.00B --- ---
+ dd if=/dev/zero of=/mnt/sub/data bs=1024 count=100
100+0 records in
100+0 records out
102400 bytes (102 kB) copied, 0.000876526 s, 117 MB/s
+ sync
+ btrfs qgroup show -prce --raw /mnt
qgroupid rfer excl max_rfer max_excl parent child
-------- ---- ---- -------- -------- ------ -----
0/5 16384 16384 0 0 --- ---
0/257 118784 118784 0 0 --- ---
+ btrfs sub snap /mnt/sub /mnt/snap
Create a snapshot of '/mnt/sub' in '/mnt/snap'
+ sync
+ btrfs qgroup show -prce --raw /mnt
qgroupid rfer excl max_rfer max_excl parent child
-------- ---- ---- -------- -------- ------ -----
0/5 16384 16384 0 0 --- ---
0/257 118784 16384 0 0 --- ---
0/258 118784 16384 0 0 --- ---
+ btrfs sub delete /mnt/sub
Delete subvolume (no-commit): '/mnt/sub'
+ sync
+ btrfs qgroup show -prce --raw /mnt
qgroupid rfer excl max_rfer max_excl parent child
-------- ---- ---- -------- -------- ------ -----
0/5 16384 16384 0 0 --- ---
0/257 118784 16384 0 0 --- ---
0/258 118784 16384 0 0 --- ---

--------------------------------------------------------------------------- 
<------- WAIT for cleaner
[root@atest-guest linux_btrfs]# btrfs qgroup show -prce /mnt
qgroupid rfer excl max_rfer max_excl parent child
-------- ---- ---- -------- -------- ------ -----
0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
0/257 100.00KiB 0.00B 0.00B 0.00B --- --- <------- the data size was not 
cleaned from qgroup.
0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------- but the excl in 
the snapshot qgroup is increased to 116KB by SUBTREE operation

My question is:
(1), SUBTREE is not working well.
(2), why we need a rule-breaker in qgroup operations SUBTREE to do this 
work?
We can do it via the delayed-ref routine like the other qgroup operations.

with my patch here applied and 1152651a reverted, I got the result:
qgroupid rfer excl max_rfer max_excl parent child
-------- ---- ---- -------- -------- ------ -----
0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
0/257 0.00B 0.00B 0.00B 0.00B --- --- <----------- data and tree block 
both are cleaned
0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------ quota numbers in 
snapshot qgroup is correct.

(1), quota is working well now in subvolume deleting.
(2), all operations are inserted by delayed-ref.

So, the conclusion seems like that:
(1), commit 1152651a was introducing a SUBTREE operation to account the 
number in subvolume deleted.
(2). SUBTREE is not working well.
(3). we can solve the same problem in a better way without introducing a 
new operation of SUBTREE.

To be honest, I am suspecting myself *very much*. I trust you guys and 
believe there must be something I am missing.

Then I send this mail to ask your help.

Thanx in advance.
Yang


On 02/10/2015 06:24 PM, Dongsheng Yang wrote:
> In function of __btrfs_mod_ref(), we are passing the no_quota=1
> to process_func() anyway. It will make the numbers in qgroup be
> wrong when deleting a subvolume. The data size in a deleted subvolume
> will never be decressed from all related qgroups.
>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>   fs/btrfs/extent-tree.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 652be74..f59809c 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>   	int i;
>   	int level;
>   	int ret = 0;
> +	int no_quota = 0;
>   	int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *,
>   			    u64, u64, u64, u64, u64, u64, int);
>   
> @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>   	else
>   		parent = 0;
>   
> +	if (!root->fs_info->quota_enabled || !is_fstree(ref_root))
> +		no_quota = 1;
> +
>   	for (i = 0; i < nritems; i++) {
>   		if (level == 0) {
>   			btrfs_item_key_to_cpu(buf, &key, i);
> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>   			key.offset -= btrfs_file_extent_offset(buf, fi);
>   			ret = process_func(trans, root, bytenr, num_bytes,
>   					   parent, ref_root, key.objectid,
> -					   key.offset, 1);
> +					   key.offset, no_quota);
>   			if (ret)
>   				goto fail;
>   		} else {
> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
>   			num_bytes = root->nodesize;
>   			ret = process_func(trans, root, bytenr, num_bytes,
>   					   parent, ref_root, level - 1, 0,
> -					   1);
> +					   no_quota);
>   			if (ret)
>   				goto fail;
>   		}

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Dongsheng Yang]

Wait a minute, this patch seems not working well in accounting quota 
number when
deleting data shared by different subvolumes.

I will investigate more about it and send a V2 out.

Thanx
Yang

On 02/13/2015 05:38 PM, Dongsheng Yang wrote:hen
> Hi Josef and Mark,
>
> I saw the commit message of 1152651a:
>
> [btrfs: qgroup: account shared subtrees during snapshot delete]
>
> that SUBTREE operation was introduced to account the quota number
> in subvolume deleting.
>
> But, I found it does not work well currently：
>
> Create subvolume '/mnt/sub'
> + btrfs qgroup show -prce /mnt
> qgroupid rfer excl max_rfer max_excl parent child
> -------- ---- ---- -------- -------- ------ -----
> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
> 0/257 16.00KiB 16.00KiB 0.00B 0.00B --- ---
> + dd if=/dev/zero of=/mnt/sub/data bs=1024 count=100
> 100+0 records in
> 100+0 records out
> 102400 bytes (102 kB) copied, 0.000876526 s, 117 MB/s
> + sync
> + btrfs qgroup show -prce --raw /mnt
> qgroupid rfer excl max_rfer max_excl parent child
> -------- ---- ---- -------- -------- ------ -----
> 0/5 16384 16384 0 0 --- ---
> 0/257 118784 118784 0 0 --- ---
> + btrfs sub snap /mnt/sub /mnt/snap
> Create a snapshot of '/mnt/sub' in '/mnt/snap'
> + sync
> + btrfs qgroup show -prce --raw /mnt
> qgroupid rfer excl max_rfer max_excl parent child
> -------- ---- ---- -------- -------- ------ -----
> 0/5 16384 16384 0 0 --- ---
> 0/257 118784 16384 0 0 --- ---
> 0/258 118784 16384 0 0 --- ---
> + btrfs sub delete /mnt/sub
> Delete subvolume (no-commit): '/mnt/sub'
> + sync
> + btrfs qgroup show -prce --raw /mnt
> qgroupid rfer excl max_rfer max_excl parent child
> -------- ---- ---- -------- -------- ------ -----
> 0/5 16384 16384 0 0 --- ---
> 0/257 118784 16384 0 0 --- ---
> 0/258 118784 16384 0 0 --- ---
>
> --------------------------------------------------------------------------- 
> <------- WAIT for cleaner
> [root@atest-guest linux_btrfs]# btrfs qgroup show -prce /mnt
> qgroupid rfer excl max_rfer max_excl parent child
> -------- ---- ---- -------- -------- ------ -----
> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
> 0/257 100.00KiB 0.00B 0.00B 0.00B --- --- <------- the data size was 
> not cleaned from qgroup.
> 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------- but the excl in 
> the snapshot qgroup is increased to 116KB by SUBTREE operation
>
> My question is:
> (1), SUBTREE is not working well.
> (2), why we need a rule-breaker in qgroup operations SUBTREE to do 
> this work?
> We can do it via the delayed-ref routine like the other qgroup 
> operations.
>
> with my patch here applied and 1152651a reverted, I got the result:
> qgroupid rfer excl max_rfer max_excl parent child
> -------- ---- ---- -------- -------- ------ -----
> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
> 0/257 0.00B 0.00B 0.00B 0.00B --- --- <----------- data and tree block 
> both are cleaned
> 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------ quota numbers in 
> snapshot qgroup is correct.
>
> (1), quota is working well now in subvolume deleting.
> (2), all operations are inserted by delayed-ref.
>
> So, the conclusion seems like that:
> (1), commit 1152651a was introducing a SUBTREE operation to account 
> the number in subvolume deleted.
> (2). SUBTREE is not working well.
> (3). we can solve the same problem in a better way without introducing 
> a new operation of SUBTREE.
>
> To be honest, I am suspecting myself *very much*. I trust you guys and 
> believe there must be something I am missing.
>
> Then I send this mail to ask your help.
>
> Thanx in advance.
> Yang
>
>
> On 02/10/2015 06:24 PM, Dongsheng Yang wrote:
>> In function of __btrfs_mod_ref(), we are passing the no_quota=1
>> to process_func() anyway. It will make the numbers in qgroup be
>> wrong when deleting a subvolume. The data size in a deleted subvolume
>> will never be decressed from all related qgroups.
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   fs/btrfs/extent-tree.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 652be74..f59809c 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct 
>> btrfs_trans_handle *trans,
>>       int i;
>>       int level;
>>       int ret = 0;
>> +    int no_quota = 0;
>>       int (*process_func)(struct btrfs_trans_handle *, struct 
>> btrfs_root *,
>>                   u64, u64, u64, u64, u64, u64, int);
>>   @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct 
>> btrfs_trans_handle *trans,
>>       else
>>           parent = 0;
>>   +    if (!root->fs_info->quota_enabled || !is_fstree(ref_root))
>> +        no_quota = 1;
>> +
>>       for (i = 0; i < nritems; i++) {
>>           if (level == 0) {
>>               btrfs_item_key_to_cpu(buf, &key, i);
>> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct 
>> btrfs_trans_handle *trans,
>>               key.offset -= btrfs_file_extent_offset(buf, fi);
>>               ret = process_func(trans, root, bytenr, num_bytes,
>>                          parent, ref_root, key.objectid,
>> -                       key.offset, 1);
>> +                       key.offset, no_quota);
>>               if (ret)
>>                   goto fail;
>>           } else {
>> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct 
>> btrfs_trans_handle *trans,
>>               num_bytes = root->nodesize;
>>               ret = process_func(trans, root, bytenr, num_bytes,
>>                          parent, ref_root, level - 1, 0,
>> -                       1);
>> +                       no_quota);
>>               if (ret)
>>                   goto fail;
>>           }
>
> -- 
> 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
> .
>

[PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

This regression is introduced by two commits:

e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tests/btrfs/084     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/084.out |  3 ++
 tests/btrfs/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/btrfs/084
 create mode 100644 tests/btrfs/084.out

diff --git a/tests/btrfs/084 b/tests/btrfs/084
new file mode 100755
index 0000000..7b7c7ac
--- /dev/null
+++ b/tests/btrfs/084
@@ -0,0 +1,84 @@
+#! /bin/bash
+# FS QA Test No. 084
+#
+# This is a case for the regression introduced by
+#
+# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
+# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+    cd /
+    rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_need_to_be_root
+_supported_fs btrfs
+_supported_os Linux
+_require_scratch
+
+_scratch_mount
+
+$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null
+
+_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog quota enable $SCRATCH_MNT
+_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
+
+units=`_btrfs_qgroup_units`
+orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+
+_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
+
+timeout=100
+# There is a background thread doing the clean work
+for ((i=0; i<$timeout; i++))
+do
+	result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+	if (($orig_result != $result))
+	then
+		break
+	fi
+	sleep 1
+done
+
+$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out
new file mode 100644
index 0000000..7bf6dbf
--- /dev/null
+++ b/tests/btrfs/084.out
@@ -0,0 +1,3 @@
+QA output created by 084
+24576 24576
+0 0
diff --git a/tests/btrfs/group b/tests/btrfs/group
index fe82a9c..1be7392 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -86,3 +86,4 @@
 081 auto quick clone
 082 auto quick remount
 083 auto quick send
+084 qgroup
-- 
1.8.4.2

[PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

This regression is introduced by two commits:

e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tests/btrfs/084     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/084.out |  3 ++
 tests/btrfs/group   |  1 +
 3 files changed, 88 insertions(+)
 create mode 100755 tests/btrfs/084
 create mode 100644 tests/btrfs/084.out

diff --git a/tests/btrfs/084 b/tests/btrfs/084
new file mode 100755
index 0000000..7b7c7ac
--- /dev/null
+++ b/tests/btrfs/084
@@ -0,0 +1,84 @@
+#! /bin/bash
+# FS QA Test No. 084
+#
+# This is a case for the regression introduced by
+#
+# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
+# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+    cd /
+    rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_need_to_be_root
+_supported_fs btrfs
+_supported_os Linux
+_require_scratch
+
+_scratch_mount
+
+$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null
+
+_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog quota enable $SCRATCH_MNT
+_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
+
+units=`_btrfs_qgroup_units`
+orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+
+_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
+
+timeout=100
+# There is a background thread doing the clean work
+for ((i=0; i<$timeout; i++))
+do
+	result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+	if (($orig_result != $result))
+	then
+		break
+	fi
+	sleep 1
+done
+
+$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out
new file mode 100644
index 0000000..7bf6dbf
--- /dev/null
+++ b/tests/btrfs/084.out
@@ -0,0 +1,3 @@
+QA output created by 084
+24576 24576
+0 0
diff --git a/tests/btrfs/group b/tests/btrfs/group
index fe82a9c..1be7392 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -86,3 +86,4 @@
 081 auto quick clone
 082 auto quick remount
 083 auto quick send
+084 qgroup
-- 
1.8.4.2

Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Eryu Guan]

On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote:
> This regression is introduced by two commits:
> 
> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)

I think you should add more description about the issue you're going to
test. I see only failures like

[root@dhcp-66-86-11 xfstests]# diff -u tests/btrfs/084.out /root/xfstests/results//btrfs/084.out.bad                                                                  
--- tests/btrfs/084.out 2015-03-06 12:47:02.319000000 +0800
+++ /root/xfstests/results//btrfs/084.out.bad   2015-03-06
12:48:03.707000000 +0800
@@ -1,3 +1,3 @@
 QA output created by 084
 24576 24576
-0 0
+8192 0

but from the test I don't know why "8192 0" is expected result not "0 0"

> 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  tests/btrfs/084     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/084.out |  3 ++
>  tests/btrfs/group   |  1 +
>  3 files changed, 88 insertions(+)
>  create mode 100755 tests/btrfs/084
>  create mode 100644 tests/btrfs/084.out
> 
> diff --git a/tests/btrfs/084 b/tests/btrfs/084
> new file mode 100755
> index 0000000..7b7c7ac
> --- /dev/null
> +++ b/tests/btrfs/084
> @@ -0,0 +1,84 @@
> +#! /bin/bash
> +# FS QA Test No. 084
> +#
> +# This is a case for the regression introduced by
> +#
> +# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
> +# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +    cd /
> +    rm -f $tmp.*

Better to use a tab not 4 spaces, maybe "new" should be updated too (in
another patch)

> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_need_to_be_root
> +_supported_fs btrfs
> +_supported_os Linux
> +_require_scratch
> +
> +_scratch_mount

_scratch_mkfs first before you mount it.

And you need to remove $seqres.full before test, since you called
_run_btrfs_util_prog and it will dump commands and outputs to
$seqres.full, the file will keep growing over runs if you don't remove
it first.

rm -f $seqres.full

> +
> +$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null
> +
> +_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap
> +
> +_run_btrfs_util_prog quota enable $SCRATCH_MNT
> +_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
> +
> +units=`_btrfs_qgroup_units`
> +orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
> +
> +_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap
> +
> +_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
> +
> +timeout=100
> +# There is a background thread doing the clean work
> +for ((i=0; i<$timeout; i++))
> +do
> +	result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
> +	if (($orig_result != $result))
> +	then
> +		break
> +	fi
> +	sleep 1

I'm not sure if we need 100 iterations and sleeping 1 sec after each
iteration, that means we need 100s for a PASS run.

> +done

Please follow this code style, as Dave pointed before

for ; do
	# code
done

if ; then
	# code
fi
> +
> +$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out
> new file mode 100644
> index 0000000..7bf6dbf
> --- /dev/null
> +++ b/tests/btrfs/084.out
> @@ -0,0 +1,3 @@
> +QA output created by 084
> +24576 24576
> +0 0
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index fe82a9c..1be7392 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -86,3 +86,4 @@
>  081 auto quick clone
>  082 auto quick remount
>  083 auto quick send
> +084 qgroup

Missed auto group?

Thanks,
Eryu

Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

Hi Guan,  sorry for the late.

On 03/06/2015 01:06 PM, Eryu Guan wrote:
> On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote:
>> This regression is introduced by two commits:
>>
>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
> I think you should add more description about the issue you're going to
> test. I see only failures like
>
> [root@dhcp-66-86-11 xfstests]# diff -u tests/btrfs/084.out /root/xfstests/results//btrfs/084.out.bad
> --- tests/btrfs/084.out 2015-03-06 12:47:02.319000000 +0800
> +++ /root/xfstests/results//btrfs/084.out.bad   2015-03-06
> 12:48:03.707000000 +0800
> @@ -1,3 +1,3 @@
>   QA output created by 084
>   24576 24576
> -0 0
> +8192 0
>
> but from the test I don't know why "8192 0" is expected result not "0 0"

Sure, I will add more description in V2 to explain why we are expecting 
"0 0" here.
>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tests/btrfs/084     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/084.out |  3 ++
>>   tests/btrfs/group   |  1 +
>>   3 files changed, 88 insertions(+)
>>   create mode 100755 tests/btrfs/084
>>   create mode 100644 tests/btrfs/084.out
>>
>> diff --git a/tests/btrfs/084 b/tests/btrfs/084
>> new file mode 100755
>> index 0000000..7b7c7ac
>> --- /dev/null
>> +++ b/tests/btrfs/084
>> @@ -0,0 +1,84 @@
>> +#! /bin/bash
>> +# FS QA Test No. 084
>> +#
>> +# This is a case for the regression introduced by
>> +#
>> +# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>> +# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +    cd /
>> +    rm -f $tmp.*
> Better to use a tab not 4 spaces, maybe "new" should be updated too (in
> another patch)

Thanx, will send another patch for "./new".
>
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# real QA test starts here
>> +_need_to_be_root
>> +_supported_fs btrfs
>> +_supported_os Linux
>> +_require_scratch
>> +
>> +_scratch_mount
> _scratch_mkfs first before you mount it.
>
> And you need to remove $seqres.full before test, since you called
> _run_btrfs_util_prog and it will dump commands and outputs to
> $seqres.full, the file will keep growing over runs if you don't remove
> it first.
>
> rm -f $seqres.full

Great! thanx for your review.
>
>> +
>> +$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null
>> +
>> +_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap
>> +
>> +_run_btrfs_util_prog quota enable $SCRATCH_MNT
>> +_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
>> +
>> +units=`_btrfs_qgroup_units`
>> +orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
>> +
>> +_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap
>> +
>> +_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
>> +
>> +timeout=100
>> +# There is a background thread doing the clean work
>> +for ((i=0; i<$timeout; i++))
>> +do
>> +	result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
>> +	if (($orig_result != $result))
>> +	then
>> +		break
>> +	fi
>> +	sleep 1
> I'm not sure if we need 100 iterations and sleeping 1 sec after each
> iteration, that means we need 100s for a PASS run.

The 100sec is the limit for the loop, it means if the clean work is done 
in 100sec, we will
break immediately. We don't have to wait for 100s in each test.
>
>> +done
> Please follow this code style, as Dave pointed before
>
> for ; do
> 	# code
> done
>
> if ; then
> 	# code
> fi

Great,
>> +
>> +$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out
>> new file mode 100644
>> index 0000000..7bf6dbf
>> --- /dev/null
>> +++ b/tests/btrfs/084.out
>> @@ -0,0 +1,3 @@
>> +QA output created by 084
>> +24576 24576
>> +0 0
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index fe82a9c..1be7392 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -86,3 +86,4 @@
>>   081 auto quick clone
>>   082 auto quick remount
>>   083 auto quick send
>> +084 qgroup
> Missed auto group?

Yea, as this is my first test case for fstest, there are lots of silly 
problems in it.

  Thanx a lot for your review and help, will update it soon.

Thanx
Yang
>
> Thanks,
> Eryu
> .
>

Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

Hi Guan,  sorry for the late.

On 03/06/2015 01:06 PM, Eryu Guan wrote:
> On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote:
>> This regression is introduced by two commits:
>>
>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
> I think you should add more description about the issue you're going to
> test. I see only failures like
>
> [root@dhcp-66-86-11 xfstests]# diff -u tests/btrfs/084.out /root/xfstests/results//btrfs/084.out.bad
> --- tests/btrfs/084.out 2015-03-06 12:47:02.319000000 +0800
> +++ /root/xfstests/results//btrfs/084.out.bad   2015-03-06
> 12:48:03.707000000 +0800
> @@ -1,3 +1,3 @@
>   QA output created by 084
>   24576 24576
> -0 0
> +8192 0
>
> but from the test I don't know why "8192 0" is expected result not "0 0"

Sure, I will add more description in V2 to explain why we are expecting 
"0 0" here.
>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   tests/btrfs/084     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/084.out |  3 ++
>>   tests/btrfs/group   |  1 +
>>   3 files changed, 88 insertions(+)
>>   create mode 100755 tests/btrfs/084
>>   create mode 100644 tests/btrfs/084.out
>>
>> diff --git a/tests/btrfs/084 b/tests/btrfs/084
>> new file mode 100755
>> index 0000000..7b7c7ac
>> --- /dev/null
>> +++ b/tests/btrfs/084
>> @@ -0,0 +1,84 @@
>> +#! /bin/bash
>> +# FS QA Test No. 084
>> +#
>> +# This is a case for the regression introduced by
>> +#
>> +# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>> +# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
>> +#
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +    cd /
>> +    rm -f $tmp.*
> Better to use a tab not 4 spaces, maybe "new" should be updated too (in
> another patch)

Thanx, will send another patch for "./new".
>
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# real QA test starts here
>> +_need_to_be_root
>> +_supported_fs btrfs
>> +_supported_os Linux
>> +_require_scratch
>> +
>> +_scratch_mount
> _scratch_mkfs first before you mount it.
>
> And you need to remove $seqres.full before test, since you called
> _run_btrfs_util_prog and it will dump commands and outputs to
> $seqres.full, the file will keep growing over runs if you don't remove
> it first.
>
> rm -f $seqres.full

Great! thanx for your review.
>
>> +
>> +$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null
>> +
>> +_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap
>> +
>> +_run_btrfs_util_prog quota enable $SCRATCH_MNT
>> +_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
>> +
>> +units=`_btrfs_qgroup_units`
>> +orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
>> +
>> +_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap
>> +
>> +_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
>> +
>> +timeout=100
>> +# There is a background thread doing the clean work
>> +for ((i=0; i<$timeout; i++))
>> +do
>> +	result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
>> +	if (($orig_result != $result))
>> +	then
>> +		break
>> +	fi
>> +	sleep 1
> I'm not sure if we need 100 iterations and sleeping 1 sec after each
> iteration, that means we need 100s for a PASS run.

The 100sec is the limit for the loop, it means if the clean work is done 
in 100sec, we will
break immediately. We don't have to wait for 100s in each test.
>
>> +done
> Please follow this code style, as Dave pointed before
>
> for ; do
> 	# code
> done
>
> if ; then
> 	# code
> fi

Great,
>> +
>> +$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out
>> new file mode 100644
>> index 0000000..7bf6dbf
>> --- /dev/null
>> +++ b/tests/btrfs/084.out
>> @@ -0,0 +1,3 @@
>> +QA output created by 084
>> +24576 24576
>> +0 0
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index fe82a9c..1be7392 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -86,3 +86,4 @@
>>   081 auto quick clone
>>   082 auto quick remount
>>   083 auto quick send
>> +084 qgroup
> Missed auto group?

Yea, as this is my first test case for fstest, there are lots of silly 
problems in it.

  Thanx a lot for your review and help, will update it soon.

Thanx
Yang
>
> Thanks,
> Eryu
> .
>

Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Eryu Guan]

On Mon, Mar 16, 2015 at 01:06:52PM +0800, Dongsheng Yang wrote:
> Hi Guan,  sorry for the late.
> 
> On 03/06/2015 01:06 PM, Eryu Guan wrote:
> >On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote:
> >>This regression is introduced by two commits:
> >>
> >>e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
> >>1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)

[snip]

> >>+_cleanup()
> >>+{
> >>+    cd /
> >>+    rm -f $tmp.*
> >Better to use a tab not 4 spaces, maybe "new" should be updated too (in
> >another patch)
> 
> Thanx, will send another patch for "./new".

Just FYI, I've already sent out the fix, please see

http://www.spinics.net/lists/fstests/msg01073.html

Thanks,
Eryu Guan

Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

On 03/16/2015 01:33 PM, Eryu Guan wrote:
> On Mon, Mar 16, 2015 at 01:06:52PM +0800, Dongsheng Yang wrote:
>> Hi Guan,  sorry for the late.
>>
>> On 03/06/2015 01:06 PM, Eryu Guan wrote:
>>> On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote:
>>>> This regression is introduced by two commits:
>>>>
>>>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>>>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
> [snip]
>
>>>> +_cleanup()
>>>> +{
>>>> +    cd /
>>>> +    rm -f $tmp.*
>>> Better to use a tab not 4 spaces, maybe "new" should be updated too (in
>>> another patch)
>> Thanx, will send another patch for "./new".
> Just FYI, I've already sent out the fix, please see
>
> http://www.spinics.net/lists/fstests/msg01073.html
Great!! Thanx for your information. But I found some other 4 spaces in 
new, I am not sure
whether we also need to replace them with tab or not as shown below.

Thanx

commit 931340c0c5599ae2c6714df16c796ea24240a5a7
Author: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Date:   Mon Mar 16 01:15:53 2015 -0400

     new: replace 4 spaces with a tab.

     Sugguested-by: Eryu Guan <eguan@redhat.com>
     Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

diff --git a/new b/new
index 86f9075..f94daed 100755
--- a/new
+++ b/new
@@ -29,15 +29,15 @@ trap "rm -f /tmp/$$.; exit" 0 1 2 3 15

  _cleanup()
  {
-    :
+       :
  }

  SRC_GROUPS=`find tests -not -path tests -type d -printf "%f "`
  usage()
  {
-    echo "Usage $0 test_dir"
-    echo "Available dirs are: $SRC_GROUPS"
-    exit
+       echo "Usage $0 test_dir"
+       echo "Available dirs are: $SRC_GROUPS"
+       exit
  }

  [ $# -eq 0 ] && usage
@@ -46,8 +46,8 @@ shift

  if [ ! -f $tdir/group ]
  then
-    echo "Creating the $tdir/group index ..."
-    cat <<'End-of-File' >$tdir/group
+       echo "Creating the $tdir/group index ..."
+       cat <<'End-of-File' >$tdir/group
  # QA groups control
  #
  # define groups and default group owners
@@ -65,15 +65,15 @@ fi

  if [ ! -w $tdir/group ]
  then
-    chmod u+w $tdir/group
-    echo "Warning: making the index file \"$tdir/group\" writeable"
+       chmod u+w $tdir/group
+       echo "Warning: making the index file \"$tdir/group\" writeable"
  fi

  if make
  then
-    :
+       :
  else
-    echo "Warning: make failed -- some tests may be missing"
+       echo "Warning: make failed -- some tests may be missing"
  fi

  i=0
@@ -83,16 +83,16 @@ eof=1

  for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
  do
-    line=$((line+1))
-    if [ -z "$found" ] || [ "$found" == "#" ];then
+       line=$((line+1))
+       if [ -z "$found" ] || [ "$found" == "#" ];then
         continue
-    fi
-    i=$((i+1))
-    id=`printf "%03d" $i`
-    if [ "$id" != "$found" ];then
+       fi
+       i=$((i+1))
+       id=`printf "%03d" $i`
+       if [ "$id" != "$found" ];then
         eof=0
         break
-    fi
+       fi
  done
  if [ $eof -eq 1 ]; then
     line=$((line+1))
@@ -104,9 +104,9 @@ echo "Next test is $id"

  if [ -f $tdir/$id ]
  then
-    echo "Error: test $id already exists!"
-    _cleanup
-    exit 1
+       echo "Error: test $id already exists!"
+       _cleanup
+       exit 1
  fi

  echo -n "Creating skeletal script for you to edit ..."
@@ -148,8 +148,8 @@ trap "_cleanup; exit \\\$status" 0 1 2 3 15

  _cleanup()
  {
-    cd /
-    rm -f \$tmp.*
+       cd /
+       rm -f \$tmp.*
  }

  # get standard environment, filters and checks
@@ -184,39 +184,39 @@ ${EDITOR-vi} $tdir/$id
  if [ $# -eq 0 ]
  then

-    while true
-    do
+       while true
+       do
         echo -n "Add to group(s) [other] (? for list): "
         read ans
         [ -z "$ans" ] && ans=other
         if [ "X$ans" = "X?" ]
         then
-           for d in $SRC_GROUPS; do
+               for d in $SRC_GROUPS; do
                 l=$(sed -n < tests/$d/group \
-                   -e 's/#.*//' \
-                   -e 's/$/ /' \
-                   -e 's;\(^[0-9][0-9][0-9]\)\(.*$\);\2;p')
+                       -e 's/#.*//' \
+                       -e 's/$/ /' \
+                       -e 's;\(^[0-9][0-9][0-9]\)\(.*$\);\2;p')
                 grpl="$grpl $l"
-           done
-           lst=`for word in $grpl; do echo $word; done | sort| uniq `
-           echo $lst
+               done
+               lst=`for word in $grpl; do echo $word; done | sort| uniq `
+               echo $lst
         else
-           break
+               break
         fi
-    done
+       done
  else
-    # expert mode, groups are on the command line
-    #
-    for g in $*
-    do
+       # expert mode, groups are on the command line
+       #
+       for g in $*
+       do
         if grep "^$g[   ]" $tdir/group >/dev/null
         then
-           :
+               :
         else
-           echo "Warning: group \"$g\" not defined in $tdir/group"
+               echo "Warning: group \"$g\" not defined in $tdir/group"
         fi
-    done
-    ans="$*"
+       done
+       ans="$*"
  fi

>
> Thanks,
> Eryu Guan
> .
>

Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

On 03/16/2015 01:33 PM, Eryu Guan wrote:
> On Mon, Mar 16, 2015 at 01:06:52PM +0800, Dongsheng Yang wrote:
>> Hi Guan,  sorry for the late.
>>
>> On 03/06/2015 01:06 PM, Eryu Guan wrote:
>>> On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote:
>>>> This regression is introduced by two commits:
>>>>
>>>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>>>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
> [snip]
>
>>>> +_cleanup()
>>>> +{
>>>> +    cd /
>>>> +    rm -f $tmp.*
>>> Better to use a tab not 4 spaces, maybe "new" should be updated too (in
>>> another patch)
>> Thanx, will send another patch for "./new".
> Just FYI, I've already sent out the fix, please see
>
> http://www.spinics.net/lists/fstests/msg01073.html
Great!! Thanx for your information. But I found some other 4 spaces in 
new, I am not sure
whether we also need to replace them with tab or not as shown below.

Thanx

commit 931340c0c5599ae2c6714df16c796ea24240a5a7
Author: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Date:   Mon Mar 16 01:15:53 2015 -0400

     new: replace 4 spaces with a tab.

     Sugguested-by: Eryu Guan <eguan@redhat.com>
     Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

diff --git a/new b/new
index 86f9075..f94daed 100755
--- a/new
+++ b/new
@@ -29,15 +29,15 @@ trap "rm -f /tmp/$$.; exit" 0 1 2 3 15

  _cleanup()
  {
-    :
+       :
  }

  SRC_GROUPS=`find tests -not -path tests -type d -printf "%f "`
  usage()
  {
-    echo "Usage $0 test_dir"
-    echo "Available dirs are: $SRC_GROUPS"
-    exit
+       echo "Usage $0 test_dir"
+       echo "Available dirs are: $SRC_GROUPS"
+       exit
  }

  [ $# -eq 0 ] && usage
@@ -46,8 +46,8 @@ shift

  if [ ! -f $tdir/group ]
  then
-    echo "Creating the $tdir/group index ..."
-    cat <<'End-of-File' >$tdir/group
+       echo "Creating the $tdir/group index ..."
+       cat <<'End-of-File' >$tdir/group
  # QA groups control
  #
  # define groups and default group owners
@@ -65,15 +65,15 @@ fi

  if [ ! -w $tdir/group ]
  then
-    chmod u+w $tdir/group
-    echo "Warning: making the index file \"$tdir/group\" writeable"
+       chmod u+w $tdir/group
+       echo "Warning: making the index file \"$tdir/group\" writeable"
  fi

  if make
  then
-    :
+       :
  else
-    echo "Warning: make failed -- some tests may be missing"
+       echo "Warning: make failed -- some tests may be missing"
  fi

  i=0
@@ -83,16 +83,16 @@ eof=1

  for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'`
  do
-    line=$((line+1))
-    if [ -z "$found" ] || [ "$found" == "#" ];then
+       line=$((line+1))
+       if [ -z "$found" ] || [ "$found" == "#" ];then
         continue
-    fi
-    i=$((i+1))
-    id=`printf "%03d" $i`
-    if [ "$id" != "$found" ];then
+       fi
+       i=$((i+1))
+       id=`printf "%03d" $i`
+       if [ "$id" != "$found" ];then
         eof=0
         break
-    fi
+       fi
  done
  if [ $eof -eq 1 ]; then
     line=$((line+1))
@@ -104,9 +104,9 @@ echo "Next test is $id"

  if [ -f $tdir/$id ]
  then
-    echo "Error: test $id already exists!"
-    _cleanup
-    exit 1
+       echo "Error: test $id already exists!"
+       _cleanup
+       exit 1
  fi

  echo -n "Creating skeletal script for you to edit ..."
@@ -148,8 +148,8 @@ trap "_cleanup; exit \\\$status" 0 1 2 3 15

  _cleanup()
  {
-    cd /
-    rm -f \$tmp.*
+       cd /
+       rm -f \$tmp.*
  }

  # get standard environment, filters and checks
@@ -184,39 +184,39 @@ ${EDITOR-vi} $tdir/$id
  if [ $# -eq 0 ]
  then

-    while true
-    do
+       while true
+       do
         echo -n "Add to group(s) [other] (? for list): "
         read ans
         [ -z "$ans" ] && ans=other
         if [ "X$ans" = "X?" ]
         then
-           for d in $SRC_GROUPS; do
+               for d in $SRC_GROUPS; do
                 l=$(sed -n < tests/$d/group \
-                   -e 's/#.*//' \
-                   -e 's/$/ /' \
-                   -e 's;\(^[0-9][0-9][0-9]\)\(.*$\);\2;p')
+                       -e 's/#.*//' \
+                       -e 's/$/ /' \
+                       -e 's;\(^[0-9][0-9][0-9]\)\(.*$\);\2;p')
                 grpl="$grpl $l"
-           done
-           lst=`for word in $grpl; do echo $word; done | sort| uniq `
-           echo $lst
+               done
+               lst=`for word in $grpl; do echo $word; done | sort| uniq `
+               echo $lst
         else
-           break
+               break
         fi
-    done
+       done
  else
-    # expert mode, groups are on the command line
-    #
-    for g in $*
-    do
+       # expert mode, groups are on the command line
+       #
+       for g in $*
+       do
         if grep "^$g[   ]" $tdir/group >/dev/null
         then
-           :
+               :
         else
-           echo "Warning: group \"$g\" not defined in $tdir/group"
+               echo "Warning: group \"$g\" not defined in $tdir/group"
         fi
-    done
-    ans="$*"
+       done
+       ans="$*"
  fi

>
> Thanks,
> Eryu Guan
> .
>

[PATCH v2] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

This regression is introduced by two commits:

e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
changlog:
	v1:
	1. Fix lots of coding style problems pointed by Eryu.
	2. Add more description for the regression.

 tests/btrfs/083     | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/083.out |   3 ++
 tests/btrfs/group   |   1 +
 3 files changed, 122 insertions(+)
 create mode 100755 tests/btrfs/083
 create mode 100644 tests/btrfs/083.out

diff --git a/tests/btrfs/083 b/tests/btrfs/083
new file mode 100755
index 0000000..a5e85e3
--- /dev/null
+++ b/tests/btrfs/083
@@ -0,0 +1,118 @@
+#! /bin/bash
+# FS QA Test No. 083
+#
+# This is a case for the regression introduced by
+#
+# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
+# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
+#
+# The problem is shown as below:
+#	$ btrfs sub create /mnt/sub
+#	Create subvolume '/mnt/sub'
+#	$ fallocate -l 1M /mnt/sub/data
+#	$ btrfs quota enable /mnt
+#	$ sync
+#	$ btrfs qgroup show --raw /mnt
+#	qgroupid         rfer         excl
+#	--------         ----         ----
+#	0/5             16384        16384
+#	0/257         1064960      1064960
+#	$ btrfs sub dele /mnt/sub
+#	Delete subvolume (no-commit): '/mnt/sub'
+#
+#	... ...                    		<------------Wait for the cleaner_kthread, about 30s.
+#	$ btrfs qgroup show --raw /mnt
+#	qgroupid         rfer         excl
+#	--------         ----         ----
+#	0/5             16384        16384
+#	0/257         1048576      1048576	<-----------sub 0/257 is deleted, but the quota numbers are not 0.
+#
+#
+# What we are expecting is:
+#	$ btrfs qgroup show --raw /mnt
+#	$ qgroupid         rfer         excl
+#	--------         ----         ----
+#	0/5             16384        16384
+#	0/257               0            0
+#
+# Currently, just revert these two commits mentioned above, we
+# can get the expected result. But there are some more things
+# we need to consider.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_need_to_be_root
+_supported_fs btrfs
+_supported_os Linux
+_require_scratch
+
+rm -f $seqres.full
+
+_scratch_mkfs >>$seqres.full 2>&1
+_scratch_mount
+
+$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null
+
+_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog quota enable $SCRATCH_MNT
+_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
+
+units=`_btrfs_qgroup_units`
+orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+
+_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
+
+timeout=100
+# There is a background thread doing the clean work
+for ((i=0; i<$timeout; i++)); do
+	result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+	if (($orig_result != $result)); then
+		break
+	fi
+	sleep 1
+done
+
+$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/083.out b/tests/btrfs/083.out
new file mode 100644
index 0000000..e73797e
--- /dev/null
+++ b/tests/btrfs/083.out
@@ -0,0 +1,3 @@
+QA output created by 083
+24576 24576
+0 0
diff --git a/tests/btrfs/group b/tests/btrfs/group
index fd2fa76..e98a154 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -85,3 +85,4 @@
 080 auto snapshot
 081 auto quick clone
 082 auto quick remount
+083 auto qgroup
-- 
1.8.4.2

[PATCH v2] fstest: btrfs: add a test for quota number when deleting a subvol.

[Dongsheng Yang]

This regression is introduced by two commits:

e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
changlog:
	v1:
	1. Fix lots of coding style problems pointed by Eryu.
	2. Add more description for the regression.

 tests/btrfs/083     | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/083.out |   3 ++
 tests/btrfs/group   |   1 +
 3 files changed, 122 insertions(+)
 create mode 100755 tests/btrfs/083
 create mode 100644 tests/btrfs/083.out

diff --git a/tests/btrfs/083 b/tests/btrfs/083
new file mode 100755
index 0000000..a5e85e3
--- /dev/null
+++ b/tests/btrfs/083
@@ -0,0 +1,118 @@
+#! /bin/bash
+# FS QA Test No. 083
+#
+# This is a case for the regression introduced by
+#
+# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
+# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
+#
+# The problem is shown as below:
+#	$ btrfs sub create /mnt/sub
+#	Create subvolume '/mnt/sub'
+#	$ fallocate -l 1M /mnt/sub/data
+#	$ btrfs quota enable /mnt
+#	$ sync
+#	$ btrfs qgroup show --raw /mnt
+#	qgroupid         rfer         excl
+#	--------         ----         ----
+#	0/5             16384        16384
+#	0/257         1064960      1064960
+#	$ btrfs sub dele /mnt/sub
+#	Delete subvolume (no-commit): '/mnt/sub'
+#
+#	... ...                    		<------------Wait for the cleaner_kthread, about 30s.
+#	$ btrfs qgroup show --raw /mnt
+#	qgroupid         rfer         excl
+#	--------         ----         ----
+#	0/5             16384        16384
+#	0/257         1048576      1048576	<-----------sub 0/257 is deleted, but the quota numbers are not 0.
+#
+#
+# What we are expecting is:
+#	$ btrfs qgroup show --raw /mnt
+#	$ qgroupid         rfer         excl
+#	--------         ----         ----
+#	0/5             16384        16384
+#	0/257               0            0
+#
+# Currently, just revert these two commits mentioned above, we
+# can get the expected result. But there are some more things
+# we need to consider.
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2015 Fujitsu.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_need_to_be_root
+_supported_fs btrfs
+_supported_os Linux
+_require_scratch
+
+rm -f $seqres.full
+
+_scratch_mkfs >>$seqres.full 2>&1
+_scratch_mount
+
+$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null
+
+_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog quota enable $SCRATCH_MNT
+_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
+
+units=`_btrfs_qgroup_units`
+orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+
+_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap
+
+_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
+
+timeout=100
+# There is a background thread doing the clean work
+for ((i=0; i<$timeout; i++)); do
+	result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'`
+	if (($orig_result != $result)); then
+		break
+	fi
+	sleep 1
+done
+
+$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}'
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/083.out b/tests/btrfs/083.out
new file mode 100644
index 0000000..e73797e
--- /dev/null
+++ b/tests/btrfs/083.out
@@ -0,0 +1,3 @@
+QA output created by 083
+24576 24576
+0 0
diff --git a/tests/btrfs/group b/tests/btrfs/group
index fd2fa76..e98a154 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -85,3 +85,4 @@
 080 auto snapshot
 081 auto quick clone
 082 auto quick remount
+083 auto qgroup
-- 
1.8.4.2

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Dongsheng Yang]

[-- Attachment #1: Type: text/plain, Size: 6789 bytes --]

On 02/26/2015 02:05 PM, Dongsheng Yang wrote:
> Wait a minute, this patch seems not working well in accounting quota 
> number when
> deleting data shared by different subvolumes.
>
> I will investigate more about it and send a V2 out.
I have sent a fstest
  [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol.
for this problem I was trying to solve in this patch.

Please consider reverting the two commits introduced the problem:

e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)

as what I attached.

Thanx
>
> Thanx
> Yang
>
> On 02/13/2015 05:38 PM, Dongsheng Yang wrote:hen
>> Hi Josef and Mark,
>>
>> I saw the commit message of 1152651a:
>>
>> [btrfs: qgroup: account shared subtrees during snapshot delete]
>>
>> that SUBTREE operation was introduced to account the quota number
>> in subvolume deleting.
>>
>> But, I found it does not work well currently：
>>
>> Create subvolume '/mnt/sub'
>> + btrfs qgroup show -prce /mnt
>> qgroupid rfer excl max_rfer max_excl parent child
>> -------- ---- ---- -------- -------- ------ -----
>> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
>> 0/257 16.00KiB 16.00KiB 0.00B 0.00B --- ---
>> + dd if=/dev/zero of=/mnt/sub/data bs=1024 count=100
>> 100+0 records in
>> 100+0 records out
>> 102400 bytes (102 kB) copied, 0.000876526 s, 117 MB/s
>> + sync
>> + btrfs qgroup show -prce --raw /mnt
>> qgroupid rfer excl max_rfer max_excl parent child
>> -------- ---- ---- -------- -------- ------ -----
>> 0/5 16384 16384 0 0 --- ---
>> 0/257 118784 118784 0 0 --- ---
>> + btrfs sub snap /mnt/sub /mnt/snap
>> Create a snapshot of '/mnt/sub' in '/mnt/snap'
>> + sync
>> + btrfs qgroup show -prce --raw /mnt
>> qgroupid rfer excl max_rfer max_excl parent child
>> -------- ---- ---- -------- -------- ------ -----
>> 0/5 16384 16384 0 0 --- ---
>> 0/257 118784 16384 0 0 --- ---
>> 0/258 118784 16384 0 0 --- ---
>> + btrfs sub delete /mnt/sub
>> Delete subvolume (no-commit): '/mnt/sub'
>> + sync
>> + btrfs qgroup show -prce --raw /mnt
>> qgroupid rfer excl max_rfer max_excl parent child
>> -------- ---- ---- -------- -------- ------ -----
>> 0/5 16384 16384 0 0 --- ---
>> 0/257 118784 16384 0 0 --- ---
>> 0/258 118784 16384 0 0 --- ---
>>
>> --------------------------------------------------------------------------- 
>> <------- WAIT for cleaner
>> [root@atest-guest linux_btrfs]# btrfs qgroup show -prce /mnt
>> qgroupid rfer excl max_rfer max_excl parent child
>> -------- ---- ---- -------- -------- ------ -----
>> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
>> 0/257 100.00KiB 0.00B 0.00B 0.00B --- --- <------- the data size was 
>> not cleaned from qgroup.
>> 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------- but the excl 
>> in the snapshot qgroup is increased to 116KB by SUBTREE operation
>>
>> My question is:
>> (1), SUBTREE is not working well.
>> (2), why we need a rule-breaker in qgroup operations SUBTREE to do 
>> this work?
>> We can do it via the delayed-ref routine like the other qgroup 
>> operations.
>>
>> with my patch here applied and 1152651a reverted, I got the result:
>> qgroupid rfer excl max_rfer max_excl parent child
>> -------- ---- ---- -------- -------- ------ -----
>> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- ---
>> 0/257 0.00B 0.00B 0.00B 0.00B --- --- <----------- data and tree 
>> block both are cleaned
>> 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------ quota numbers 
>> in snapshot qgroup is correct.
>>
>> (1), quota is working well now in subvolume deleting.
>> (2), all operations are inserted by delayed-ref.
>>
>> So, the conclusion seems like that:
>> (1), commit 1152651a was introducing a SUBTREE operation to account 
>> the number in subvolume deleted.
>> (2). SUBTREE is not working well.
>> (3). we can solve the same problem in a better way without 
>> introducing a new operation of SUBTREE.
>>
>> To be honest, I am suspecting myself *very much*. I trust you guys 
>> and believe there must be something I am missing.
>>
>> Then I send this mail to ask your help.
>>
>> Thanx in advance.
>> Yang
>>
>>
>> On 02/10/2015 06:24 PM, Dongsheng Yang wrote:
>>> In function of __btrfs_mod_ref(), we are passing the no_quota=1
>>> to process_func() anyway. It will make the numbers in qgroup be
>>> wrong when deleting a subvolume. The data size in a deleted subvolume
>>> will never be decressed from all related qgroups.
>>>
>>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>>> ---
>>>   fs/btrfs/extent-tree.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 652be74..f59809c 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct 
>>> btrfs_trans_handle *trans,
>>>       int i;
>>>       int level;
>>>       int ret = 0;
>>> +    int no_quota = 0;
>>>       int (*process_func)(struct btrfs_trans_handle *, struct 
>>> btrfs_root *,
>>>                   u64, u64, u64, u64, u64, u64, int);
>>>   @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct 
>>> btrfs_trans_handle *trans,
>>>       else
>>>           parent = 0;
>>>   +    if (!root->fs_info->quota_enabled || !is_fstree(ref_root))
>>> +        no_quota = 1;
>>> +
>>>       for (i = 0; i < nritems; i++) {
>>>           if (level == 0) {
>>>               btrfs_item_key_to_cpu(buf, &key, i);
>>> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct 
>>> btrfs_trans_handle *trans,
>>>               key.offset -= btrfs_file_extent_offset(buf, fi);
>>>               ret = process_func(trans, root, bytenr, num_bytes,
>>>                          parent, ref_root, key.objectid,
>>> -                       key.offset, 1);
>>> +                       key.offset, no_quota);
>>>               if (ret)
>>>                   goto fail;
>>>           } else {
>>> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct 
>>> btrfs_trans_handle *trans,
>>>               num_bytes = root->nodesize;
>>>               ret = process_func(trans, root, bytenr, num_bytes,
>>>                          parent, ref_root, level - 1, 0,
>>> -                       1);
>>> +                       no_quota);
>>>               if (ret)
>>>                   goto fail;
>>>           }
>>
>> -- 
>> 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
>> .
>>
>
> -- 
> 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
> .
>


[-- Attachment #2: 0001-Revert-btrfs-qgroup-account-shared-subtrees-during-s.patch --]
[-- Type: text/x-patch, Size: 15583 bytes --]

>From c71244a6017d6917e0281c5390817fcf901b8ca1 Mon Sep 17 00:00:00 2001
From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Date: Tue, 3 Mar 2015 04:31:43 -0500
Subject: [PATCH 1/2] Revert "btrfs: qgroup: account shared subtrees during
 snapshot delete"

This reverts commit 1152651a081720ef6a8c76bb7da676e8c900ac30.

Conflicts:
	fs/btrfs/extent-tree.c
	fs/btrfs/qgroup.c
---
 fs/btrfs/extent-tree.c | 260 -------------------------------------------------
 fs/btrfs/qgroup.c      | 176 ---------------------------------
 fs/btrfs/qgroup.h      |   1 -
 3 files changed, 437 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 4d9f6c5..ae305fa 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -7499,219 +7499,6 @@ reada:
 	wc->reada_slot = slot;
 }
 
-static int account_leaf_items(struct btrfs_trans_handle *trans,
-			      struct btrfs_root *root,
-			      struct extent_buffer *eb)
-{
-	int nr = btrfs_header_nritems(eb);
-	int i, extent_type, ret;
-	struct btrfs_key key;
-	struct btrfs_file_extent_item *fi;
-	u64 bytenr, num_bytes;
-
-	for (i = 0; i < nr; i++) {
-		btrfs_item_key_to_cpu(eb, &key, i);
-
-		if (key.type != BTRFS_EXTENT_DATA_KEY)
-			continue;
-
-		fi = btrfs_item_ptr(eb, i, struct btrfs_file_extent_item);
-		/* filter out non qgroup-accountable extents  */
-		extent_type = btrfs_file_extent_type(eb, fi);
-
-		if (extent_type == BTRFS_FILE_EXTENT_INLINE)
-			continue;
-
-		bytenr = btrfs_file_extent_disk_bytenr(eb, fi);
-		if (!bytenr)
-			continue;
-
-		num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi);
-
-		ret = btrfs_qgroup_record_ref(trans, root->fs_info,
-					      root->objectid,
-					      bytenr, num_bytes,
-					      BTRFS_QGROUP_OPER_SUB_SUBTREE,
-					      BTRFS_QGROUP_TYPE_METADATA, 0);
-		if (ret)
-			return ret;
-	}
-	return 0;
-}
-
-/*
- * Walk up the tree from the bottom, freeing leaves and any interior
- * nodes which have had all slots visited. If a node (leaf or
- * interior) is freed, the node above it will have it's slot
- * incremented. The root node will never be freed.
- *
- * At the end of this function, we should have a path which has all
- * slots incremented to the next position for a search. If we need to
- * read a new node it will be NULL and the node above it will have the
- * correct slot selected for a later read.
- *
- * If we increment the root nodes slot counter past the number of
- * elements, 1 is returned to signal completion of the search.
- */
-static int adjust_slots_upwards(struct btrfs_root *root,
-				struct btrfs_path *path, int root_level)
-{
-	int level = 0;
-	int nr, slot;
-	struct extent_buffer *eb;
-
-	if (root_level == 0)
-		return 1;
-
-	while (level <= root_level) {
-		eb = path->nodes[level];
-		nr = btrfs_header_nritems(eb);
-		path->slots[level]++;
-		slot = path->slots[level];
-		if (slot >= nr || level == 0) {
-			/*
-			 * Don't free the root -  we will detect this
-			 * condition after our loop and return a
-			 * positive value for caller to stop walking the tree.
-			 */
-			if (level != root_level) {
-				btrfs_tree_unlock_rw(eb, path->locks[level]);
-				path->locks[level] = 0;
-
-				free_extent_buffer(eb);
-				path->nodes[level] = NULL;
-				path->slots[level] = 0;
-			}
-		} else {
-			/*
-			 * We have a valid slot to walk back down
-			 * from. Stop here so caller can process these
-			 * new nodes.
-			 */
-			break;
-		}
-
-		level++;
-	}
-
-	eb = path->nodes[root_level];
-	if (path->slots[root_level] >= btrfs_header_nritems(eb))
-		return 1;
-
-	return 0;
-}
-
-/*
- * root_eb is the subtree root and is locked before this function is called.
- */
-static int account_shared_subtree(struct btrfs_trans_handle *trans,
-				  struct btrfs_root *root,
-				  struct extent_buffer *root_eb,
-				  u64 root_gen,
-				  int root_level)
-{
-	int ret = 0;
-	int level;
-	struct extent_buffer *eb = root_eb;
-	struct btrfs_path *path = NULL;
-
-	BUG_ON(root_level < 0 || root_level > BTRFS_MAX_LEVEL);
-	BUG_ON(root_eb == NULL);
-
-	if (!root->fs_info->quota_enabled)
-		return 0;
-
-	if (!extent_buffer_uptodate(root_eb)) {
-		ret = btrfs_read_buffer(root_eb, root_gen);
-		if (ret)
-			goto out;
-	}
-
-	if (root_level == 0) {
-		ret = account_leaf_items(trans, root, root_eb);
-		goto out;
-	}
-
-	path = btrfs_alloc_path();
-	if (!path)
-		return -ENOMEM;
-
-	/*
-	 * Walk down the tree.  Missing extent blocks are filled in as
-	 * we go. Metadata is accounted every time we read a new
-	 * extent block.
-	 *
-	 * When we reach a leaf, we account for file extent items in it,
-	 * walk back up the tree (adjusting slot pointers as we go)
-	 * and restart the search process.
-	 */
-	extent_buffer_get(root_eb); /* For path */
-	path->nodes[root_level] = root_eb;
-	path->slots[root_level] = 0;
-	path->locks[root_level] = 0; /* so release_path doesn't try to unlock */
-walk_down:
-	level = root_level;
-	while (level >= 0) {
-		if (path->nodes[level] == NULL) {
-			int parent_slot;
-			u64 child_gen;
-			u64 child_bytenr;
-
-			/* We need to get child blockptr/gen from
-			 * parent before we can read it. */
-			eb = path->nodes[level + 1];
-			parent_slot = path->slots[level + 1];
-			child_bytenr = btrfs_node_blockptr(eb, parent_slot);
-			child_gen = btrfs_node_ptr_generation(eb, parent_slot);
-
-			eb = read_tree_block(root, child_bytenr, child_gen);
-			if (!eb || !extent_buffer_uptodate(eb)) {
-				ret = -EIO;
-				goto out;
-			}
-
-			path->nodes[level] = eb;
-			path->slots[level] = 0;
-
-			btrfs_tree_read_lock(eb);
-			btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
-			path->locks[level] = BTRFS_READ_LOCK_BLOCKING;
-
-			ret = btrfs_qgroup_record_ref(trans, root->fs_info,
-						root->objectid,
-						child_bytenr,
-						root->nodesize,
-						BTRFS_QGROUP_OPER_SUB_SUBTREE,
-						BTRFS_QGROUP_TYPE_METADATA, 0);
-			if (ret)
-				goto out;
-
-		}
-
-		if (level == 0) {
-			ret = account_leaf_items(trans, root, path->nodes[level]);
-			if (ret)
-				goto out;
-
-			/* Nonzero return here means we completed our search */
-			ret = adjust_slots_upwards(root, path, root_level);
-			if (ret)
-				break;
-
-			/* Restart search with new slots */
-			goto walk_down;
-		}
-
-		level--;
-	}
-
-	ret = 0;
-out:
-	btrfs_free_path(path);
-
-	return ret;
-}
-
 /*
  * helper to process tree block while walking down the tree.
  *
@@ -7815,7 +7602,6 @@ static noinline int do_walk_down(struct btrfs_trans_handle *trans,
 	int level = wc->level;
 	int reada = 0;
 	int ret = 0;
-	bool need_account = false;
 
 	generation = btrfs_node_ptr_generation(path->nodes[level],
 					       path->slots[level]);
@@ -7861,7 +7647,6 @@ static noinline int do_walk_down(struct btrfs_trans_handle *trans,
 
 	if (wc->stage == DROP_REFERENCE) {
 		if (wc->refs[level - 1] > 1) {
-			need_account = true;
 			if (level == 1 &&
 			    (wc->flags[0] & BTRFS_BLOCK_FLAG_FULL_BACKREF))
 				goto skip;
@@ -7925,16 +7710,6 @@ skip:
 			parent = 0;
 		}
 
-		if (need_account) {
-			ret = account_shared_subtree(trans, root, next,
-						     generation, level - 1);
-			if (ret) {
-				printk_ratelimited(KERN_ERR "BTRFS: %s Error "
-					"%d accounting shared subtree. Quota "
-					"is out of sync, rescan required.\n",
-					root->fs_info->sb->s_id, ret);
-			}
-		}
 		ret = btrfs_free_extent(trans, root, bytenr, blocksize, parent,
 				root->root_key.objectid, level - 1, 0, 0);
 		BUG_ON(ret); /* -ENOMEM */
@@ -8019,13 +7794,6 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 			else
 				ret = btrfs_dec_ref(trans, root, eb, 0);
 			BUG_ON(ret); /* -ENOMEM */
-			ret = account_leaf_items(trans, root, eb);
-			if (ret) {
-				printk_ratelimited(KERN_ERR "BTRFS: %s Error "
-					"%d accounting leaf items. Quota "
-					"is out of sync, rescan required.\n",
-					root->fs_info->sb->s_id, ret);
-			}
 		}
 		/* make block locked assertion in clean_tree_block happy */
 		if (!path->locks[level] &&
@@ -8151,8 +7919,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
 	int level;
 	bool root_dropped = false;
 
-	btrfs_debug(root->fs_info, "Drop subvolume %llu", root->objectid);
-
 	path = btrfs_alloc_path();
 	if (!path) {
 		err = -ENOMEM;
@@ -8278,24 +8044,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
 				goto out_end_trans;
 			}
 
-			/*
-			 * Qgroup update accounting is run from
-			 * delayed ref handling. This usually works
-			 * out because delayed refs are normally the
-			 * only way qgroup updates are added. However,
-			 * we may have added updates during our tree
-			 * walk so run qgroups here to make sure we
-			 * don't lose any updates.
-			 */
-			ret = btrfs_delayed_qgroup_accounting(trans,
-							      root->fs_info);
-			if (ret)
-				printk_ratelimited(KERN_ERR "BTRFS: Failure %d "
-						   "running qgroup updates "
-						   "during snapshot delete. "
-						   "Quota is out of sync, "
-						   "rescan required.\n", ret);
-
 			btrfs_end_transaction_throttle(trans, tree_root);
 			if (!for_reloc && btrfs_need_cleaner_sleep(root)) {
 				pr_debug("BTRFS: drop snapshot early exit\n");
@@ -8349,14 +8097,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root,
 	}
 	root_dropped = true;
 out_end_trans:
-	ret = btrfs_delayed_qgroup_accounting(trans, tree_root->fs_info);
-	if (ret)
-		printk_ratelimited(KERN_ERR "BTRFS: Failure %d "
-				   "running qgroup updates "
-				   "during snapshot delete. "
-				   "Quota is out of sync, "
-				   "rescan required.\n", ret);
-
 	btrfs_end_transaction_throttle(trans, tree_root);
 out_free:
 	kfree(wc);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 103a908..2acf059 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1262,50 +1262,6 @@ out:
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	return ret;
 }
-
-static int comp_oper_exist(struct btrfs_qgroup_operation *oper1,
-			   struct btrfs_qgroup_operation *oper2)
-{
-	/*
-	 * Ignore seq and type here, we're looking for any operation
-	 * at all related to this extent on that root.
-	 */
-	if (oper1->bytenr < oper2->bytenr)
-		return -1;
-	if (oper1->bytenr > oper2->bytenr)
-		return 1;
-	if (oper1->ref_root < oper2->ref_root)
-		return -1;
-	if (oper1->ref_root > oper2->ref_root)
-		return 1;
-	return 0;
-}
-
-static int qgroup_oper_exists(struct btrfs_fs_info *fs_info,
-			      struct btrfs_qgroup_operation *oper)
-{
-	struct rb_node *n;
-	struct btrfs_qgroup_operation *cur;
-	int cmp;
-
-	spin_lock(&fs_info->qgroup_op_lock);
-	n = fs_info->qgroup_op_tree.rb_node;
-	while (n) {
-		cur = rb_entry(n, struct btrfs_qgroup_operation, n);
-		cmp = comp_oper_exist(cur, oper);
-		if (cmp < 0) {
-			n = n->rb_right;
-		} else if (cmp) {
-			n = n->rb_left;
-		} else {
-			spin_unlock(&fs_info->qgroup_op_lock);
-			return -EEXIST;
-		}
-	}
-	spin_unlock(&fs_info->qgroup_op_lock);
-	return 0;
-}
-
 static int comp_oper(struct btrfs_qgroup_operation *oper1,
 		     struct btrfs_qgroup_operation *oper2)
 {
@@ -1397,25 +1353,6 @@ int btrfs_qgroup_record_ref(struct btrfs_trans_handle *trans,
 	oper->seq = atomic_inc_return(&fs_info->qgroup_op_seq);
 	INIT_LIST_HEAD(&oper->elem.list);
 	oper->elem.seq = 0;
-
-	trace_btrfs_qgroup_record_ref(oper);
-
-	if (type == BTRFS_QGROUP_OPER_SUB_SUBTREE) {
-		/*
-		 * If any operation for this bytenr/ref_root combo
-		 * exists, then we know it's not exclusively owned and
-		 * shouldn't be queued up.
-		 *
-		 * This also catches the case where we have a cloned
-		 * extent that gets queued up multiple times during
-		 * drop snapshot.
-		 */
-		if (qgroup_oper_exists(fs_info, oper)) {
-			kfree(oper);
-			return 0;
-		}
-	}
-
 	ret = insert_qgroup_oper(fs_info, oper);
 	if (ret) {
 		/* Shouldn't happen so have an assert for developers */
@@ -2025,116 +1962,6 @@ out:
 }
 
 /*
- * Process a reference to a shared subtree. This type of operation is
- * queued during snapshot removal when we encounter extents which are
- * shared between more than one root.
- */
-static int qgroup_subtree_accounting(struct btrfs_trans_handle *trans,
-				     struct btrfs_fs_info *fs_info,
-				     struct btrfs_qgroup_operation *oper)
-{
-	struct ulist *roots = NULL;
-	struct ulist_node *unode;
-	struct ulist_iterator uiter;
-	struct btrfs_qgroup_list *glist;
-	struct ulist *parents;
-	int ret = 0;
-	int err;
-	struct btrfs_qgroup *qg;
-	u64 root_obj = 0;
-	struct seq_list elem = {};
-
-	parents = ulist_alloc(GFP_NOFS);
-	if (!parents)
-		return -ENOMEM;
-
-	btrfs_get_tree_mod_seq(fs_info, &elem);
-	ret = btrfs_find_all_roots(trans, fs_info, oper->bytenr,
-				   elem.seq, &roots);
-	btrfs_put_tree_mod_seq(fs_info, &elem);
-	if (ret < 0)
-		goto out;
-
-	if (roots->nnodes != 1)
-		goto out;
-
-	ULIST_ITER_INIT(&uiter);
-	unode = ulist_next(roots, &uiter); /* Only want 1 so no need to loop */
-	/*
-	 * If we find our ref root then that means all refs
-	 * this extent has to the root have not yet been
-	 * deleted. In that case, we do nothing and let the
-	 * last ref for this bytenr drive our update.
-	 *
-	 * This can happen for example if an extent is
-	 * referenced multiple times in a snapshot (clone,
-	 * etc). If we are in the middle of snapshot removal,
-	 * queued updates for such an extent will find the
-	 * root if we have not yet finished removing the
-	 * snapshot.
-	 */
-	if (unode->val == oper->ref_root)
-		goto out;
-
-	root_obj = unode->val;
-	BUG_ON(!root_obj);
-
-	spin_lock(&fs_info->qgroup_lock);
-	qg = find_qgroup_rb(fs_info, root_obj);
-	if (!qg)
-		goto out_unlock;
-
-	if ((qg->type & oper->quota_type)) {
-		qg->excl += oper->num_bytes;
-		qg->excl_cmpr += oper->num_bytes;
-		qgroup_dirty(fs_info, qg);
-	}
-
-	/*
-	 * Adjust counts for parent groups. First we find all
-	 * parents, then in the 2nd loop we do the adjustment
-	 * while adding parents of the parents to our ulist.
-	 */
-	list_for_each_entry(glist, &qg->groups, next_group) {
-		err = ulist_add(parents, glist->group->qgroupid,
-				ptr_to_u64(glist->group), GFP_ATOMIC);
-		if (err < 0) {
-			ret = err;
-			goto out_unlock;
-		}
-	}
-
-	ULIST_ITER_INIT(&uiter);
-	while ((unode = ulist_next(parents, &uiter))) {
-		qg = u64_to_ptr(unode->aux);
-
-		if ((qg->type & oper->quota_type)) {
-			qg->excl += oper->num_bytes;
-			qg->excl_cmpr += oper->num_bytes;
-			qgroup_dirty(fs_info, qg);
-		}
-
-		/* Add any parents of the parents */
-		list_for_each_entry(glist, &qg->groups, next_group) {
-			err = ulist_add(parents, glist->group->qgroupid,
-					ptr_to_u64(glist->group), GFP_ATOMIC);
-			if (err < 0) {
-				ret = err;
-				goto out_unlock;
-			}
-		}
-	}
-
-out_unlock:
-	spin_unlock(&fs_info->qgroup_lock);
-
-out:
-	ulist_free(roots);
-	ulist_free(parents);
-	return ret;
-}
-
-/*
  * btrfs_qgroup_account_ref is called for every ref that is added to or deleted
  * from the fs. First, all roots referencing the extent are searched, and
  * then the space is accounted accordingly to the different roots. The
@@ -2173,9 +2000,6 @@ static int btrfs_qgroup_account(struct btrfs_trans_handle *trans,
 	case BTRFS_QGROUP_OPER_SUB_SHARED:
 		ret = qgroup_shared_accounting(trans, fs_info, oper);
 		break;
-	case BTRFS_QGROUP_OPER_SUB_SUBTREE:
-		ret = qgroup_subtree_accounting(trans, fs_info, oper);
-		break;
 	default:
 		ASSERT(0);
 	}
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 52cfdc2..9722888 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -60,7 +60,6 @@ enum btrfs_qgroup_operation_type {
 	BTRFS_QGROUP_OPER_ADD_SHARED,
 	BTRFS_QGROUP_OPER_SUB_EXCL,
 	BTRFS_QGROUP_OPER_SUB_SHARED,
-	BTRFS_QGROUP_OPER_SUB_SUBTREE,
 };
 
 struct btrfs_qgroup_operation {
-- 
1.8.4.2


[-- Attachment #3: 0002-Revert-Btrfs-__btrfs_mod_ref-should-always-use-no_qu.patch --]
[-- Type: text/x-patch, Size: 6600 bytes --]

>From 21b2e636b1beb005d39188608030793c41c1e4fc Mon Sep 17 00:00:00 2001
From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Date: Tue, 3 Mar 2015 06:59:19 -0500
Subject: [PATCH 2/2] Revert "Btrfs: __btrfs_mod_ref should always use
 no_quota"

This reverts commit e339a6b097c515a31ce230d498c44ff2e89f1cf4.
---
 fs/btrfs/ctree.c       | 20 ++++++++++----------
 fs/btrfs/ctree.h       |  4 ++--
 fs/btrfs/extent-tree.c | 24 +++++++++++++-----------
 3 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 14a72ed..a06cd4a 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -269,9 +269,9 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans,
 
 	WARN_ON(btrfs_header_generation(buf) > trans->transid);
 	if (new_root_objectid == BTRFS_TREE_RELOC_OBJECTID)
-		ret = btrfs_inc_ref(trans, root, cow, 1);
+		ret = btrfs_inc_ref(trans, root, cow, 1, 1);
 	else
-		ret = btrfs_inc_ref(trans, root, cow, 0);
+		ret = btrfs_inc_ref(trans, root, cow, 0, 1);
 
 	if (ret)
 		return ret;
@@ -1024,14 +1024,14 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 		if ((owner == root->root_key.objectid ||
 		     root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) &&
 		    !(flags & BTRFS_BLOCK_FLAG_FULL_BACKREF)) {
-			ret = btrfs_inc_ref(trans, root, buf, 1);
+			ret = btrfs_inc_ref(trans, root, buf, 1, 1);
 			BUG_ON(ret); /* -ENOMEM */
 
 			if (root->root_key.objectid ==
 			    BTRFS_TREE_RELOC_OBJECTID) {
-				ret = btrfs_dec_ref(trans, root, buf, 0);
+				ret = btrfs_dec_ref(trans, root, buf, 0, 1);
 				BUG_ON(ret); /* -ENOMEM */
-				ret = btrfs_inc_ref(trans, root, cow, 1);
+				ret = btrfs_inc_ref(trans, root, cow, 1, 1);
 				BUG_ON(ret); /* -ENOMEM */
 			}
 			new_flags |= BTRFS_BLOCK_FLAG_FULL_BACKREF;
@@ -1039,9 +1039,9 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 
 			if (root->root_key.objectid ==
 			    BTRFS_TREE_RELOC_OBJECTID)
-				ret = btrfs_inc_ref(trans, root, cow, 1);
+				ret = btrfs_inc_ref(trans, root, cow, 1, 1);
 			else
-				ret = btrfs_inc_ref(trans, root, cow, 0);
+				ret = btrfs_inc_ref(trans, root, cow, 0, 1);
 			BUG_ON(ret); /* -ENOMEM */
 		}
 		if (new_flags != 0) {
@@ -1058,11 +1058,11 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 		if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			if (root->root_key.objectid ==
 			    BTRFS_TREE_RELOC_OBJECTID)
-				ret = btrfs_inc_ref(trans, root, cow, 1);
+				ret = btrfs_inc_ref(trans, root, cow, 1, 1);
 			else
-				ret = btrfs_inc_ref(trans, root, cow, 0);
+				ret = btrfs_inc_ref(trans, root, cow, 0, 1);
 			BUG_ON(ret); /* -ENOMEM */
-			ret = btrfs_dec_ref(trans, root, buf, 1);
+			ret = btrfs_dec_ref(trans, root, buf, 1, 1);
 			BUG_ON(ret); /* -ENOMEM */
 		}
 		clean_tree_block(trans, root, buf);
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 8034d07..d58a6d3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3387,9 +3387,9 @@ int btrfs_reserve_extent(struct btrfs_root *root, u64 num_bytes,
 			 u64 min_alloc_size, u64 empty_size, u64 hint_byte,
 			 struct btrfs_key *ins, int is_data, int delalloc);
 int btrfs_inc_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
-		  struct extent_buffer *buf, int full_backref);
+		  struct extent_buffer *buf, int full_backref, int no_quota);
 int btrfs_dec_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
-		  struct extent_buffer *buf, int full_backref);
+		  struct extent_buffer *buf, int full_backref, int no_quota);
 int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
 				u64 bytenr, u64 num_bytes, u64 flags,
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ae305fa..313aff9 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3049,7 +3049,7 @@ out:
 static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 			   struct btrfs_root *root,
 			   struct extent_buffer *buf,
-			   int full_backref, int inc)
+			   int full_backref, int inc, int no_quota)
 {
 	u64 bytenr;
 	u64 num_bytes;
@@ -3103,7 +3103,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 			key.offset -= btrfs_file_extent_offset(buf, fi);
 			ret = process_func(trans, root, bytenr, num_bytes,
 					   parent, ref_root, key.objectid,
-					   key.offset, 1);
+					   key.offset, no_quota);
 			if (ret)
 				goto fail;
 		} else {
@@ -3111,7 +3111,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans,
 			num_bytes = root->nodesize;
 			ret = process_func(trans, root, bytenr, num_bytes,
 					   parent, ref_root, level - 1, 0,
-					   1);
+					   no_quota);
 			if (ret)
 				goto fail;
 		}
@@ -3122,15 +3122,15 @@ fail:
 }
 
 int btrfs_inc_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
-		  struct extent_buffer *buf, int full_backref)
+		  struct extent_buffer *buf, int full_backref, int no_quota)
 {
-	return __btrfs_mod_ref(trans, root, buf, full_backref, 1);
+	return __btrfs_mod_ref(trans, root, buf, full_backref, 1, no_quota);
 }
 
 int btrfs_dec_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
-		  struct extent_buffer *buf, int full_backref)
+		  struct extent_buffer *buf, int full_backref, int no_quota)
 {
-	return __btrfs_mod_ref(trans, root, buf, full_backref, 0);
+	return __btrfs_mod_ref(trans, root, buf, full_backref, 0, no_quota);
 }
 
 static int write_one_cache_group(struct btrfs_trans_handle *trans,
@@ -7553,9 +7553,9 @@ static noinline int walk_down_proc(struct btrfs_trans_handle *trans,
 	/* wc->stage == UPDATE_BACKREF */
 	if (!(wc->flags[level] & flag)) {
 		BUG_ON(!path->locks[level]);
-		ret = btrfs_inc_ref(trans, root, eb, 1);
+		ret = btrfs_inc_ref(trans, root, eb, 1, wc->for_reloc);
 		BUG_ON(ret); /* -ENOMEM */
-		ret = btrfs_dec_ref(trans, root, eb, 0);
+		ret = btrfs_dec_ref(trans, root, eb, 0, wc->for_reloc);
 		BUG_ON(ret); /* -ENOMEM */
 		ret = btrfs_set_disk_extent_flags(trans, root, eb->start,
 						  eb->len, flag,
@@ -7790,9 +7790,11 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	if (wc->refs[level] == 1) {
 		if (level == 0) {
 			if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF)
-				ret = btrfs_dec_ref(trans, root, eb, 1);
+				ret = btrfs_dec_ref(trans, root, eb, 1,
+						    wc->for_reloc);
 			else
-				ret = btrfs_dec_ref(trans, root, eb, 0);
+				ret = btrfs_dec_ref(trans, root, eb, 0,
+						    wc->for_reloc);
 			BUG_ON(ret); /* -ENOMEM */
 		}
 		/* make block locked assertion in clean_tree_block happy */
-- 
1.8.4.2

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Josef Bacik]

On 03/03/2015 06:18 AM, Dongsheng Yang wrote:
> On 02/26/2015 02:05 PM, Dongsheng Yang wrote:
>> Wait a minute, this patch seems not working well in accounting quota
>> number when
>> deleting data shared by different subvolumes.
>>
>> I will investigate more about it and send a V2 out.
> I have sent a fstest
>   [PATCH] fstest: btrfs: add a test for quota number when deleting a
> subvol.
> for this problem I was trying to solve in this patch.
>
> Please consider reverting the two commits introduced the problem:
>
> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
>

We aren't reverting these two commits, what we had was worse before than 
it is now.  We need to figure out why this is resulting in problems and 
fix that rather than throwing out a whole bunch of work.  Thanks,

Josef

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Mark Fasheh]

On Tue, Mar 03, 2015 at 09:00:36AM -0500, Josef Bacik wrote:
> On 03/03/2015 06:18 AM, Dongsheng Yang wrote:
>> On 02/26/2015 02:05 PM, Dongsheng Yang wrote:
>>> Wait a minute, this patch seems not working well in accounting quota
>>> number when
>>> deleting data shared by different subvolumes.
>>>
>>> I will investigate more about it and send a V2 out.
>> I have sent a fstest
>>   [PATCH] fstest: btrfs: add a test for quota number when deleting a
>> subvol.
>> for this problem I was trying to solve in this patch.
>>
>> Please consider reverting the two commits introduced the problem:
>>
>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
>>
>
> We aren't reverting these two commits, what we had was worse before than it 
> is now.  We need to figure out why this is resulting in problems and fix 
> that rather than throwing out a whole bunch of work.  Thanks,

Agreed, reverting these would re-introduce far more problems than it would
solve.
	--Mark

--
Mark Fasheh

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Dongsheng Yang]

On 03/04/2015 04:20 AM, Mark Fasheh wrote:
> On Tue, Mar 03, 2015 at 09:00:36AM -0500, Josef Bacik wrote:
>> On 03/03/2015 06:18 AM, Dongsheng Yang wrote:
>>> On 02/26/2015 02:05 PM, Dongsheng Yang wrote:
>>>> Wait a minute, this patch seems not working well in accounting quota
>>>> number when
>>>> deleting data shared by different subvolumes.
>>>>
>>>> I will investigate more about it and send a V2 out.
>>> I have sent a fstest
>>>    [PATCH] fstest: btrfs: add a test for quota number when deleting a
>>> subvol.
>>> for this problem I was trying to solve in this patch.
>>>
>>> Please consider reverting the two commits introduced the problem:
>>>
>>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
>>>
>> We aren't reverting these two commits, what we had was worse before than it
>> is now.  We need to figure out why this is resulting in problems and fix
>> that rather than throwing out a whole bunch of work.  Thanks,
> Agreed, reverting these would re-introduce far more problems than it would
> solve.

Hi Josef and Mark,

Sorry for the late.

Thanx for your reply and sorry I am not clear about the "far more 
problems".

 From the result below, I saw there are some diff reports in V4.0-rc1 
and when I
revert these two commits. It's consistent now.

But I think there must be something I am missing.  I am glad to 
investigate more
about it and solve it. But at first I need to know what's the problem 
SUBTREE is
trying to solve.

Example:
mkfs.btrfs  /dev/sdc -f
mount /dev/sdc /mnt
btrfs quota enable /mnt
btrfs sub create /mnt/sub
dd if=/dev/zero of=/mnt/sub/data bs=1024 count=1000
btrfs sub snapshot /mnt/sub /mnt/snap
btrfs qgroup create 1/1 /mnt
btrfs qgroup assign 0/257 1/1 /mnt
btrfs quota rescan -w /mnt
sync
btrfs sub delete /mnt/sub
-----------------------          <-------------wait for cleaner_kthread.
btrfs qgroup show /mnt
umount /mnt
btrfs check --qgroup-report /dev/sdc |grep diff

Result:
(1). V4.0-rc1
# btrfs qgroup show /mnt
qgroupid         rfer         excl
--------         ----         ----
0/5          16.00KiB     16.00KiB
0/257      1000.00KiB        0.00B
0/258      1016.00KiB   1016.00KiB
1/1        1000.00KiB        0.00B
# btrfs check --qgroup-report /dev/sdc |grep diff
Counts for qgroup id: 257 are different
diff:        referenced -1024000 referenced compressed -1024000
Counts for qgroup id: 281474976710657 are different
diff:        referenced -1024000 referenced compressed -1024000

(2). V4.0-rc1 with reverts of "96c92f" and "c04cad"
# btrfs qgroup show /mnt
qgroupid         rfer         excl
--------         ----         ----
0/5          16.00KiB     16.00KiB
0/257           0.00B        0.00B
0/258      1016.00KiB   1016.00KiB
1/1             0.00B        0.00B
# btrfs check --qgroup-report /dev/sdc |grep diff
#
*No diff in btrfs check --qgroup-report*


Thanx
> 	--Mark
>
> --
> Mark Fasheh
> .
>

Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota.

[Dongsheng Yang]

Okey, I found a patch from Mark 35 weeks ago which was not merged into xfstest:

[PATCH] xfstests/btrfs: add test for quota groups and drop snapshot

It looks can answer my question here.

Thanx
Yang

On Mon, Mar 16, 2015 at 12:59 PM, Dongsheng Yang
<yangds.fnst@cn.fujitsu.com> wrote:
> On 03/04/2015 04:20 AM, Mark Fasheh wrote:
>>
>> On Tue, Mar 03, 2015 at 09:00:36AM -0500, Josef Bacik wrote:
>>>
>>> On 03/03/2015 06:18 AM, Dongsheng Yang wrote:
>>>>
>>>> On 02/26/2015 02:05 PM, Dongsheng Yang wrote:
>>>>>
>>>>> Wait a minute, this patch seems not working well in accounting quota
>>>>> number when
>>>>> deleting data shared by different subvolumes.
>>>>>
>>>>> I will investigate more about it and send a V2 out.
>>>>
>>>> I have sent a fstest
>>>>    [PATCH] fstest: btrfs: add a test for quota number when deleting a
>>>> subvol.
>>>> for this problem I was trying to solve in this patch.
>>>>
>>>> Please consider reverting the two commits introduced the problem:
>>>>
>>>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota)
>>>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete)
>>>>
>>> We aren't reverting these two commits, what we had was worse before than
>>> it
>>> is now.  We need to figure out why this is resulting in problems and fix
>>> that rather than throwing out a whole bunch of work.  Thanks,
>>
>> Agreed, reverting these would re-introduce far more problems than it would
>> solve.
>
>
> Hi Josef and Mark,
>
> Sorry for the late.
>
> Thanx for your reply and sorry I am not clear about the "far more problems".
>
> From the result below, I saw there are some diff reports in V4.0-rc1 and
> when I
> revert these two commits. It's consistent now.
>
> But I think there must be something I am missing.  I am glad to investigate
> more
> about it and solve it. But at first I need to know what's the problem
> SUBTREE is
> trying to solve.
>
> Example:
> mkfs.btrfs  /dev/sdc -f
> mount /dev/sdc /mnt
> btrfs quota enable /mnt
> btrfs sub create /mnt/sub
> dd if=/dev/zero of=/mnt/sub/data bs=1024 count=1000
> btrfs sub snapshot /mnt/sub /mnt/snap
> btrfs qgroup create 1/1 /mnt
> btrfs qgroup assign 0/257 1/1 /mnt
> btrfs quota rescan -w /mnt
> sync
> btrfs sub delete /mnt/sub
> -----------------------          <-------------wait for cleaner_kthread.
> btrfs qgroup show /mnt
> umount /mnt
> btrfs check --qgroup-report /dev/sdc |grep diff
>
> Result:
> (1). V4.0-rc1
> # btrfs qgroup show /mnt
> qgroupid         rfer         excl
> --------         ----         ----
> 0/5          16.00KiB     16.00KiB
> 0/257      1000.00KiB        0.00B
> 0/258      1016.00KiB   1016.00KiB
> 1/1        1000.00KiB        0.00B
> # btrfs check --qgroup-report /dev/sdc |grep diff
> Counts for qgroup id: 257 are different
> diff:        referenced -1024000 referenced compressed -1024000
> Counts for qgroup id: 281474976710657 are different
> diff:        referenced -1024000 referenced compressed -1024000
>
> (2). V4.0-rc1 with reverts of "96c92f" and "c04cad"
> # btrfs qgroup show /mnt
> qgroupid         rfer         excl
> --------         ----         ----
> 0/5          16.00KiB     16.00KiB
> 0/257           0.00B        0.00B
> 0/258      1016.00KiB   1016.00KiB
> 1/1             0.00B        0.00B
> # btrfs check --qgroup-report /dev/sdc |grep diff
> #
> *No diff in btrfs check --qgroup-report*
>
>
> Thanx
>>
>>         --Mark
>>
>> --
>> Mark Fasheh
>> .
>>
>
> --
> 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

[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