[PATCH] btrfs: qgroup: Don't populating excl numbers for snapshot src if it belongs to other qgroups

[PATCH] btrfs: qgroup: Don't populating excl numbers for snapshot src if it belongs to other qgroups

[Qu Wenruo]

When quota is enabled and we do a snapshot, we just update the 'excl'
number of both snapshot src and dst to src's 'rfer' - nodesize.

It's a quick hack to avoid quota rescan every time we create a snapshot
and it works if src doesn't belong to other qgroups.

But if we have higher level qgroups, such behavior only works for level
0 qgroups, and higher level qgroups don't get update, thus making qgroup
number inconsistent.

The problem of updating higher level qgroup numbers is, it's way to
complex.

Under the following case, it's pretty simple: (src is 257, dst is 258)
0/257 - 1/0, 0/258.

In this case, we only need to modify 1/0 to reduce its 'excl'

But under the following case, it will go out of control:

0/257 - 1/0, 0/258 - 1/1 (using -i option), 1/0 - 2/0, 1/1 - 2/0.

So to make it simple, if snapshot src has higher level qgroups, just
mark qgroup inconsistent and let later rescan to do its job.

Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index ec4351fd7537..2b3d2dd1b735 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2298,6 +2298,22 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 		if (!srcgroup)
 			goto unlock;
 
+		/*
+		 * If snapshot source is belonging to high level qgroups, it
+		 * will be a more complex to hack the numbers.
+		 * E.g. source is 257, snapshot is 258:
+		 * 0/257 - 1/0, creating snapshot 258 will need to update 1/0
+		 * It's too complex when higher level qgroup is involved.
+		 * Mark qgroup inconsistent for later rescan
+		 */
+		if (!list_empty(&srcgroup->groups)) {
+			btrfs_info_rl(fs_info,
+"src qgroup 0/%llu belongs to higher level qgroup, creating snapshot for it need qgroup rescan",
+				      srcid);
+			fs_info->qgroup_flags |=
+				BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
+			goto unlock;
+		}
 		/*
 		 * We call inherit after we clone the root in order to make sure
 		 * our counts don't go crazy, so at this point the only
-- 
2.18.0

Re: [PATCH] btrfs: qgroup: Don't populating excl numbers for snapshot src if it belongs to other qgroups

[Misono Tomohiro]

On 2018/08/08 15:04, Qu Wenruo wrote:
> When quota is enabled and we do a snapshot, we just update the 'excl'
> number of both snapshot src and dst to src's 'rfer' - nodesize.
> 
> It's a quick hack to avoid quota rescan every time we create a snapshot
> and it works if src doesn't belong to other qgroups.
> 
> But if we have higher level qgroups, such behavior only works for level
> 0 qgroups, and higher level qgroups don't get update, thus making qgroup
> number inconsistent.
> 
> The problem of updating higher level qgroup numbers is, it's way to
> complex.
> 
> Under the following case, it's pretty simple: (src is 257, dst is 258)
> 0/257 - 1/0, 0/258.
> 
> In this case, we only need to modify 1/0 to reduce its 'excl'
> 
> But under the following case, it will go out of control:
> 
> 0/257 - 1/0, 0/258 - 1/1 (using -i option), 1/0 - 2/0, 1/1 - 2/0.
> 
> So to make it simple, if snapshot src has higher level qgroups, just
> mark qgroup inconsistent and let later rescan to do its job.
> 
> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index ec4351fd7537..2b3d2dd1b735 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2298,6 +2298,22 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>  		if (!srcgroup)
>  			goto unlock;
>  
> +		/*
> +		 * If snapshot source is belonging to high level qgroups, it
> +		 * will be a more complex to hack the numbers.
> +		 * E.g. source is 257, snapshot is 258:
> +		 * 0/257 - 1/0, creating snapshot 258 will need to update 1/0
> +		 * It's too complex when higher level qgroup is involved.
> +		 * Mark qgroup inconsistent for later rescan
> +		 */
> +		if (!list_empty(&srcgroup->groups)) {
> +			btrfs_info_rl(fs_info,
> +"src qgroup 0/%llu belongs to higher level qgroup, creating snapshot for it need qgroup rescan",
> +				      srcid);
> +			fs_info->qgroup_flags |=
> +				BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
> +			goto unlock;
> +		}
>  		/*
>  		 * We call inherit after we clone the root in order to make sure
>  		 * our counts don't go crazy, so at this point the only
> 

Thanks for the quick fix.
Tested-by/Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>

However there is still another problem about removing relation:

(4.18-rc7 with above patch)
$ mkfs.btrfs -fq $DEV
$ mount $DEV /mnt

$ btrfs quota enable /mnt
$ btrfs qgroup create 1/0 /mnt
$ btrfs sub create /mnt/sub
$ btrfs qgroup assign 0/257 1/0 /mnt

$ dd if=/dev/urandom of=/mnt/sub/file bs=1k count=1000
$ btrfs sub snap /mnt/sub /mnt/snap
$ dmesg | tail -n 1
BTRFS info (device sdb7): src qgroup 0/257 belongs to higher level qgroup,
 creating snapshot for it need qgroup rescan

$ btrfs quota rescan -w /mnt
$ btrfs qgroup show -pcre /mnt
qgroupid         rfer         excl     max_rfer     max_excl parent  child
--------         ----         ----     --------     -------- ------  -----
0/5          16.00KiB     16.00KiB         none         none ---     ---
0/257      1016.00KiB     16.00KiB         none         none 1/0     ---
0/258      1016.00KiB     16.00KiB         none         none ---     ---
1/0        1016.00KiB     16.00KiB         none         none ---     0/257
                          
so far so good, but:

$ btrfs qgroup remove 0/257 1/0 /mnt
WARNING: quotas may be inconsistent, rescan needed
$ btrfs quota rescan -w /mnt
$ btrfs qgroup show -pcre  /mnt
qgoupid         rfer         excl     max_rfer     max_excl parent  child
--------         ----         ----     --------     -------- ------  -----
0/5          16.00KiB     16.00KiB         none         none ---     ---
0/257      1016.00KiB     16.00KiB         none         none ---     ---
0/258      1016.00KiB     16.00KiB         none         none ---     ---
1/0        1016.00KiB     16.00KiB         none         none ---     ---
           ^^^^^^^^^^     ^^^^^^^^ not cleared

It seems some fix is needed for rescan too.

Thanks,
Misono

Re: [PATCH] btrfs: qgroup: Don't populating excl numbers for snapshot src if it belongs to other qgroups

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4695 bytes --]



On 2018年08月08日 15:41, Misono Tomohiro wrote:
> On 2018/08/08 15:04, Qu Wenruo wrote:
>> When quota is enabled and we do a snapshot, we just update the 'excl'
>> number of both snapshot src and dst to src's 'rfer' - nodesize.
>>
>> It's a quick hack to avoid quota rescan every time we create a snapshot
>> and it works if src doesn't belong to other qgroups.
>>
>> But if we have higher level qgroups, such behavior only works for level
>> 0 qgroups, and higher level qgroups don't get update, thus making qgroup
>> number inconsistent.
>>
>> The problem of updating higher level qgroup numbers is, it's way to
>> complex.
>>
>> Under the following case, it's pretty simple: (src is 257, dst is 258)
>> 0/257 - 1/0, 0/258.
>>
>> In this case, we only need to modify 1/0 to reduce its 'excl'
>>
>> But under the following case, it will go out of control:
>>
>> 0/257 - 1/0, 0/258 - 1/1 (using -i option), 1/0 - 2/0, 1/1 - 2/0.
>>
>> So to make it simple, if snapshot src has higher level qgroups, just
>> mark qgroup inconsistent and let later rescan to do its job.
>>
>> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/qgroup.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index ec4351fd7537..2b3d2dd1b735 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -2298,6 +2298,22 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>  		if (!srcgroup)
>>  			goto unlock;
>>  
>> +		/*
>> +		 * If snapshot source is belonging to high level qgroups, it
>> +		 * will be a more complex to hack the numbers.
>> +		 * E.g. source is 257, snapshot is 258:
>> +		 * 0/257 - 1/0, creating snapshot 258 will need to update 1/0
>> +		 * It's too complex when higher level qgroup is involved.
>> +		 * Mark qgroup inconsistent for later rescan
>> +		 */
>> +		if (!list_empty(&srcgroup->groups)) {
>> +			btrfs_info_rl(fs_info,
>> +"src qgroup 0/%llu belongs to higher level qgroup, creating snapshot for it need qgroup rescan",
>> +				      srcid);
>> +			fs_info->qgroup_flags |=
>> +				BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
>> +			goto unlock;
>> +		}
>>  		/*
>>  		 * We call inherit after we clone the root in order to make sure
>>  		 * our counts don't go crazy, so at this point the only
>>
> 
> Thanks for the quick fix.
> Tested-by/Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> 
> However there is still another problem about removing relation:
> 
> (4.18-rc7 with above patch)
> $ mkfs.btrfs -fq $DEV
> $ mount $DEV /mnt
> 
> $ btrfs quota enable /mnt
> $ btrfs qgroup create 1/0 /mnt
> $ btrfs sub create /mnt/sub
> $ btrfs qgroup assign 0/257 1/0 /mnt
> 
> $ dd if=/dev/urandom of=/mnt/sub/file bs=1k count=1000
> $ btrfs sub snap /mnt/sub /mnt/snap
> $ dmesg | tail -n 1
> BTRFS info (device sdb7): src qgroup 0/257 belongs to higher level qgroup,
>  creating snapshot for it need qgroup rescan
> 
> $ btrfs quota rescan -w /mnt
> $ btrfs qgroup show -pcre /mnt
> qgroupid         rfer         excl     max_rfer     max_excl parent  child
> --------         ----         ----     --------     -------- ------  -----
> 0/5          16.00KiB     16.00KiB         none         none ---     ---
> 0/257      1016.00KiB     16.00KiB         none         none 1/0     ---
> 0/258      1016.00KiB     16.00KiB         none         none ---     ---
> 1/0        1016.00KiB     16.00KiB         none         none ---     0/257
>                           
> so far so good, but:
> 
> $ btrfs qgroup remove 0/257 1/0 /mnt
> WARNING: quotas may be inconsistent, rescan needed
> $ btrfs quota rescan -w /mnt
> $ btrfs qgroup show -pcre  /mnt
> qgoupid         rfer         excl     max_rfer     max_excl parent  child
> --------         ----         ----     --------     -------- ------  -----
> 0/5          16.00KiB     16.00KiB         none         none ---     ---
> 0/257      1016.00KiB     16.00KiB         none         none ---     ---
> 0/258      1016.00KiB     16.00KiB         none         none ---     ---
> 1/0        1016.00KiB     16.00KiB         none         none ---     ---
>            ^^^^^^^^^^     ^^^^^^^^ not cleared
> 
> It seems some fix is needed for rescan too.

In this particular case, it's not hard to fix.

In fact for deleting/assigning qgroup with rfer == excl case, it should
go through the quick accounting path.

But it looks like remove path doesn't go that path.

I'll try to fix it soon.

Thanks,
Qu

> 
> Thanks,
> Misono
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: qgroup: Don't populating excl numbers for snapshot src if it belongs to other qgroups

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5207 bytes --]



On 8/8/18 3:48 PM, Qu Wenruo wrote:
> 
> 
> On 2018年08月08日 15:41, Misono Tomohiro wrote:
>> On 2018/08/08 15:04, Qu Wenruo wrote:
>>> When quota is enabled and we do a snapshot, we just update the 'excl'
>>> number of both snapshot src and dst to src's 'rfer' - nodesize.
>>>
>>> It's a quick hack to avoid quota rescan every time we create a snapshot
>>> and it works if src doesn't belong to other qgroups.
>>>
>>> But if we have higher level qgroups, such behavior only works for level
>>> 0 qgroups, and higher level qgroups don't get update, thus making qgroup
>>> number inconsistent.
>>>
>>> The problem of updating higher level qgroup numbers is, it's way to
>>> complex.
>>>
>>> Under the following case, it's pretty simple: (src is 257, dst is 258)
>>> 0/257 - 1/0, 0/258.
>>>
>>> In this case, we only need to modify 1/0 to reduce its 'excl'
>>>
>>> But under the following case, it will go out of control:
>>>
>>> 0/257 - 1/0, 0/258 - 1/1 (using -i option), 1/0 - 2/0, 1/1 - 2/0.
>>>
>>> So to make it simple, if snapshot src has higher level qgroups, just
>>> mark qgroup inconsistent and let later rescan to do its job.
>>>
>>> Reported-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/qgroup.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index ec4351fd7537..2b3d2dd1b735 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -2298,6 +2298,22 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>>  		if (!srcgroup)
>>>  			goto unlock;
>>>  
>>> +		/*
>>> +		 * If snapshot source is belonging to high level qgroups, it
>>> +		 * will be a more complex to hack the numbers.
>>> +		 * E.g. source is 257, snapshot is 258:
>>> +		 * 0/257 - 1/0, creating snapshot 258 will need to update 1/0
>>> +		 * It's too complex when higher level qgroup is involved.
>>> +		 * Mark qgroup inconsistent for later rescan
>>> +		 */
>>> +		if (!list_empty(&srcgroup->groups)) {
>>> +			btrfs_info_rl(fs_info,
>>> +"src qgroup 0/%llu belongs to higher level qgroup, creating snapshot for it need qgroup rescan",
>>> +				      srcid);
>>> +			fs_info->qgroup_flags |=
>>> +				BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
>>> +			goto unlock;
>>> +		}
>>>  		/*
>>>  		 * We call inherit after we clone the root in order to make sure
>>>  		 * our counts don't go crazy, so at this point the only
>>>
>>
>> Thanks for the quick fix.
>> Tested-by/Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>>
>> However there is still another problem about removing relation:
>>
>> (4.18-rc7 with above patch)
>> $ mkfs.btrfs -fq $DEV
>> $ mount $DEV /mnt
>>
>> $ btrfs quota enable /mnt
>> $ btrfs qgroup create 1/0 /mnt
>> $ btrfs sub create /mnt/sub
>> $ btrfs qgroup assign 0/257 1/0 /mnt
>>
>> $ dd if=/dev/urandom of=/mnt/sub/file bs=1k count=1000
>> $ btrfs sub snap /mnt/sub /mnt/snap
>> $ dmesg | tail -n 1
>> BTRFS info (device sdb7): src qgroup 0/257 belongs to higher level qgroup,
>>  creating snapshot for it need qgroup rescan
>>
>> $ btrfs quota rescan -w /mnt
>> $ btrfs qgroup show -pcre /mnt
>> qgroupid         rfer         excl     max_rfer     max_excl parent  child
>> --------         ----         ----     --------     -------- ------  -----
>> 0/5          16.00KiB     16.00KiB         none         none ---     ---
>> 0/257      1016.00KiB     16.00KiB         none         none 1/0     ---
>> 0/258      1016.00KiB     16.00KiB         none         none ---     ---
>> 1/0        1016.00KiB     16.00KiB         none         none ---     0/257
>>                           
>> so far so good, but:
>>
>> $ btrfs qgroup remove 0/257 1/0 /mnt
>> WARNING: quotas may be inconsistent, rescan needed
>> $ btrfs quota rescan -w /mnt
>> $ btrfs qgroup show -pcre  /mnt
>> qgoupid         rfer         excl     max_rfer     max_excl parent  child
>> --------         ----         ----     --------     -------- ------  -----
>> 0/5          16.00KiB     16.00KiB         none         none ---     ---
>> 0/257      1016.00KiB     16.00KiB         none         none ---     ---
>> 0/258      1016.00KiB     16.00KiB         none         none ---     ---
>> 1/0        1016.00KiB     16.00KiB         none         none ---     ---
>>            ^^^^^^^^^^     ^^^^^^^^ not cleared
>>
>> It seems some fix is needed for rescan too.
> 
> In this particular case, it's not hard to fix.
> 
> In fact for deleting/assigning qgroup with rfer == excl case, it should
> go through the quick accounting path.
> 
> But it looks like remove path doesn't go that path.

My fault, in this case, since rfer != excl, so we can't go quick updating.

But on the other hand, if you don't remove the 0 level qgroup 0/257
directly, but using subvolume delete, qgroup should update 0/257 to rfer
= 0 and excl = 0, and then remove qgroup relationship should work
without the need to rescan.

Thanks,
Qu

> 
> I'll try to fix it soon.
> 
> Thanks,
> Qu
> 
>>
>> Thanks,
>> Misono
>>
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]