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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- 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 --]