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 <mfas...@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).
> Signed-off-by: Qu Wenruo <quwen...@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?
#!/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
--
Mark Fasheh
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
