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

[Mark Fasheh <mfasheh@suse.de>]

On Fri, Apr 15, 2016 at 09:00:06AM +0800, Qu Wenruo wrote:
> 
> 
> 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).
> >
> >
> >>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?
> >
> >
> >#!/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
> 
> I recently found btrfsck --qgroup-report itself is not stable.

No, it works - I just don't think you're reading the output correctly.
Notice above in my example that btrfsck shows a difference in exclusive
counts? You don't have that below. Also it reports 'Counts for qgroup id: X
are different' when there is a difference as you see above.


> 
> So here I prefer to do the accounting check by qgroup rescan, and
> compare the binary output.
> >
> >
> >
> >How are you testing this on your end?
> 
> Much like yours, but with smaller fs size, about 16M.
> 
> The result shows pretty good:
> (As I didn't believe btrfsck --qgroup-report now, I use rescan to
> check the result)
> ------
> qgroupid         rfer         excl     max_rfer     max_excl parent  child
> --------         ----         ----     --------     -------- ------  -----
> 0/5          16.00KiB     16.00KiB         none         none ---     ---
> 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          16793600        16384         none         none ---     ---
> 0/258        16793600        16384         none         none ---     ---
> 0/259        16793600        16384         none         none ---     ---
> 0/260        16793600        16384         none         none ---     ---
> quota rescan started
> qgroupid         rfer         excl     max_rfer     max_excl parent  child
> --------         ----         ----     --------     -------- ------  -----
> 0/5          16793600        16384         none         none ---     ---
> 0/258        16793600        16384         none         none ---     ---
> 0/259        16793600        16384         none         none ---     ---
> 0/260        16793600        16384         none         none ---     ---
> ------
> 
> And yes, even after rescan, btrfsck --qgroup-report seems to report
> false alert.
> ------
> Counts for qgroup id: 5
> our:		referenced 16793600 referenced compressed 16793600
> disk:		referenced 16793600 referenced compressed 16793600
> our:		exclusive 16384 exclusive compressed 16384
> disk:		exclusive 16384 exclusive compressed 16384
> Counts for qgroup id: 258
> our:		referenced 16793600 referenced compressed 16793600
> disk:		referenced 16793600 referenced compressed 16793600
> our:		exclusive 16384 exclusive compressed 16384
> disk:		exclusive 16384 exclusive compressed 16384
> Counts for qgroup id: 259
> our:		referenced 16793600 referenced compressed 16793600
> disk:		referenced 16793600 referenced compressed 16793600
> our:		exclusive 16384 exclusive compressed 16384
> disk:		exclusive 16384 exclusive compressed 16384
> Counts for qgroup id: 260
> our:		referenced 16793600 referenced compressed 16793600
> disk:		referenced 16793600 referenced compressed 16793600
> our:		exclusive 16384 exclusive compressed 16384
> disk:		exclusive 16384 exclusive compressed 16384

This isn't reporting any inconsistency - if you look at the numbers the
'disk' and 'our' versions are all the same. Basicaly in this case you asked
for a summary and that's what you're getting.

Please try to use btrfsck to check qgroup inconsistencies, IMHO you will
find them much faster that way.
	--Mark

--
Mark Fasheh