Re: [PATCH v3] btrfs: qgroup: Fix qgroup accounting when creating snapshot

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

Mark Fasheh wrote on 2016/04/14 14:42 -0700:
> Hi Qu,
>
> On Thu, Apr 14, 2016 at 01:38:40PM +0800, Qu Wenruo wrote:
>> Current btrfs qgroup design implies a requirement that after calling
>> btrfs_qgroup_account_extents() there must be a commit root switch.
>>
>> Normally this is OK, as btrfs_qgroup_accounting_extents() is only called
>> inside btrfs_commit_transaction() just be commit_cowonly_roots().
>>
>> However there is a exception at create_pending_snapshot(), which will
>> call btrfs_qgroup_account_extents() but no any commit root switch.
>>
>> In case of creating a snapshot whose parent root is itself (create a
>> snapshot of fs tree), it will corrupt qgroup by the following trace:
>> (skipped unrelated data)
>> ======
>> btrfs_qgroup_account_extent: bytenr = 29786112, num_bytes = 16384, nr_old_roots = 0, nr_new_roots = 1
>> qgroup_update_counters: qgid = 5, cur_old_count = 0, cur_new_count = 1, rfer = 0, excl = 0
>> qgroup_update_counters: qgid = 5, cur_old_count = 0, cur_new_count = 1, rfer = 16384, excl = 16384
>> btrfs_qgroup_account_extent: bytenr = 29786112, num_bytes = 16384, nr_old_roots = 0, nr_new_roots = 0
>> ======
>>
>> The problem here is in first qgroup_account_extent(), the
>> nr_new_roots of the extent is 1, which means its reference got
>> increased, and qgroup increased its rfer and excl.
>>
>> But at second qgroup_account_extent(), its reference got decreased, but
>> between these two qgroup_account_extent(), there is no switch roots.
>> This leads to the same nr_old_roots, and this extent just got ignored by
>> qgroup, which means this extent is wrongly accounted.
>>
>> Fix it by call commit_cowonly_roots() after qgroup_account_extent() in
>> create_pending_snapshot(), with needed preparation.
>>
>> Reported-by: Mark Fasheh <mfasheh@suse.de>
>
> Can you please CC me on this patch when you send it out? FYI it's customary
> to CC anyone listed here as well as significant reviewers of your patch
> (such as Filipe).

I'll add CC into the patch tag.

>
>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>> v2:
>>    Fix a soft lockup caused by missing switch_commit_root() call.
>>    Fix a warning caused by dirty-but-not-committed root.
>
> This version doesn't introduce any lockups that I encountered, thanks!
>
>
>> v3:
>>    Fix a difference behavior that btrfs qgroup will start accounting
>>    dropped roots if we are creating snapshots.
>>    Other than always account them in next transaction.
>
> This still corrupts the qgroup numbers if you do anything significant to the
> source subvolume. For example, this script shows a 16K difference. My guess
> is that we're missing accounting of some metadata somewhere?

After further investigation, the trigger is the level of the source 
subvolume.

And problem happens when cowing the leaf, which is not accounting 
correctly: (I also verified btrfs-debug-tree output, to exclude the 
possibility of rescan error)
------
qgroupid         rfer         excl     max_rfer     max_excl parent  child
--------         ----         ----     --------     -------- ------  -----
0/5          16.00KiB     16.00KiB         none         none ---     ---
populating '/mnt/test' with 8 inline files, 2K size
populating '/mnt/test' with 16 normal files, 1M size
sync
Create a snapshot of '/mnt/test' in '/mnt/test/snap1'
Create a snapshot of '/mnt/test' in '/mnt/test/snap2'
Create a snapshot of '/mnt/test' in '/mnt/test/snap3'
qgroupid         rfer         excl     max_rfer     max_excl parent  child
--------         ----         ----     --------     -------- ------  -----
0/5          16826368        16384         none         none ---     ---
0/258        16826368        16384         none         none ---     ---
0/259        16826368        16384         none         none ---     ---
0/260        16826368        16384         none         none ---     ---
quota rescan started
qgroupid         rfer         excl     max_rfer     max_excl parent  child
--------         ----         ----     --------     -------- ------  -----
0/5          16826368        32768         none         none ---     ---
0/258        16826368        32768         none         none ---     ---
0/259        16826368        32768         none         none ---     ---
0/260        16826368        32768         none         none ---     ---
------

I'll try to update the patch to v4 to fix this problem.

>
>
> #!/bin/bash
>
> DEV=/dev/vdb1
> MNT=/btrfs
>
> mkfs.btrfs -f $DEV
> mount -t btrfs $DEV $MNT
> btrfs quota enable $MNT
> mkdir "$MNT/snaps"
> mkdir "$MNT/data"
> echo "populate $MNT with some data"
> for i in `seq -w 0 640`; do
>      dd if=/dev/zero of="$MNT/data/file$i" bs=1M count=1 >&/dev/null
> done;
> for i in `seq -w 0 1`; do
>          S="$MNT/snaps/snap$i"
>          echo "create snapshot $S"
>          btrfs su snap $MNT $S;
> done;
> btrfs qg show $MNT
>
> umount $MNT
> btrfsck $DEV
>
>
> Sample output:
>
> btrfs-progs v4.4+20160122
> See http://btrfs.wiki.kernel.org for more information.
>
> Label:              (null)
> UUID:               a0b648b1-7a23-4213-9bc3-db02b8520efe
> Node size:          16384
> Sector size:        4096
> Filesystem size:    16.00GiB
> Block group profiles:
>    Data:             single            8.00MiB
>    Metadata:         DUP               1.01GiB
>    System:           DUP              12.00MiB
> SSD detected:       no
> Incompat features:  extref, skinny-metadata
> Number of devices:  1
> Devices:
>     ID        SIZE  PATH
>      1    16.00GiB  /dev/vdb1
>
> populate /btrfs with some data
> create snapshot /btrfs/snaps/snap0
> Create a snapshot of '/btrfs' in '/btrfs/snaps/snap0'
> create snapshot /btrfs/snaps/snap1
> Create a snapshot of '/btrfs' in '/btrfs/snaps/snap1'
> qgroupid         rfer         excl
> --------         ----         ----
> 0/5         641.34MiB     16.00KiB
> 0/258       641.34MiB     16.00KiB
> 0/259       641.34MiB     16.00KiB
> Checking filesystem on /dev/vdb1
> UUID: a0b648b1-7a23-4213-9bc3-db02b8520efe
> checking extents
> checking free space cache
> checking fs roots
> checking csums
> checking root refs
> checking quota groups
> Counts for qgroup id: 5 are different
> our:		referenced 672497664 referenced compressed 672497664
> disk:		referenced 672497664 referenced compressed 672497664
> our:		exclusive 49152 exclusive compressed 49152
> disk:		exclusive 16384 exclusive compressed 16384
> diff:		exclusive 32768 exclusive compressed 32768
> found 673562626 bytes used err is 0
> total csum bytes: 656384
> total tree bytes: 1425408
> total fs tree bytes: 442368
> total extent tree bytes: 98304
> btree space waste bytes: 385361
> file data blocks allocated: 672661504
>   referenced 672661504
> extent buffer leak: start 30965760 len 16384
> extent buffer leak: start 30998528 len 16384
> extent buffer leak: start 31014912 len 16384
>
>
>
> How are you testing this on your end?
>
>
>> ---
>>   fs/btrfs/transaction.c | 122 +++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 87 insertions(+), 35 deletions(-)
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index 43885e5..5ba0d9a 100644
>> @@ -1516,6 +1520,65 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>>   		goto fail;
>>   	}
>>
>> +	/*
>> +	 * Account qgroups before insert the dir item
>> +	 * As such dir item insert will modify parent_root, which could be
>> +	 * src root. If we don't do it now, wrong accounting may be inherited
>> +	 * to snapshot qgroup.
>> +	 *
>> +	 * For reason locking tree_log_mutex, see btrfs_commit_transaction()
>> +	 * comment
>> +	 */
>> +	mutex_lock(&root->fs_info->tree_log_mutex);
>> +
>> +	ret = commit_fs_roots(trans, root);
>> +	if (ret) {
>> +		mutex_unlock(&root->fs_info->tree_log_mutex);
>> +		goto fail;
>> +	}
>> +
>> +	ret = btrfs_qgroup_prepare_account_extents(trans, root->fs_info);
>> +	if (ret < 0) {
>> +		mutex_unlock(&root->fs_info->tree_log_mutex);
>> +		goto fail;
>> +	}
>> +	ret = btrfs_qgroup_account_extents(trans, root->fs_info);
>> +	if (ret < 0) {
>> +		mutex_unlock(&root->fs_info->tree_log_mutex);
>> +		goto fail;
>> +	}
>> +	/*
>> +	 * Now qgroup are all updated, we can inherit it to new qgroups
>> +	 */
>> +	ret = btrfs_qgroup_inherit(trans, fs_info,
>> +				   root->root_key.objectid,
>> +				   objectid, pending->inherit);
>> +	if (ret < 0) {
>> +		mutex_unlock(&root->fs_info->tree_log_mutex);
>> +		goto fail;
>> +	}
>> +	/*
>> +	 * qgroup_account_extents() must be followed by a
>> +	 * switch_commit_roots(), or next qgroup_account_extents() will
>> +	 * be corrupted
>> +	 */
>> +	ret = commit_cowonly_roots(trans, root);
>> +	if (ret) {
>> +		mutex_unlock(&root->fs_info->tree_log_mutex);
>> +		goto fail;
>> +	}
>> +	/*
>> +	 * Just like in btrfs_commit_transaction(), we need to
>> +	 * switch_commit_roots().
>> +	 * However this time we don't need to do a full one,
>> +	 * excluding tree root and chunk root should be OK.
>> +	 *
>> +	 * Also we don't want to free dropped roots here.
>> +	 * Only the final switch_commit_roots() will free them
>> +	 */
>> +	switch_commit_roots(trans->transaction, root->fs_info, 0);
>> +	mutex_unlock(&root->fs_info->tree_log_mutex);
>> +
>>   	ret = btrfs_insert_dir_item(trans, parent_root,
>>   				    dentry->d_name.name, dentry->d_name.len,
>>   				    parent_inode, &key,
>
> create_pending_snapshot() is too big as it is - could we please put this
> code in a separate function?
> 	--Mark

Not a problem.

Thanks,
Qu
>
> --
> Mark Fasheh
>
>