| Message ID | 1460612320-19199-1-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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 extent buffer leak: start 30130176 len 16384 extent buffer leak: start 29884416 len 16384 extent buffer leak: start 30015488 len 16384 extent buffer leak: start 30146560 len 16384 ------ Anyway, there is something wrong with either btrfsck or kernel qgroup. Thanks, Qu > > >> --- >> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Fasheh wrote on 2016/04/15 09:00 -0700: > 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 > > You're right. I just read the output wrong. The code shows that if using --qgroup-report it will always report all qgroup info, no matter if it is corrupted. While if not using --qgroup-report, it will only report corrupted one. But even qgroup is wrong, btrfsck still return 0 not 1. I'd better fix the return value along with the extent buffer leaking. Thanks for pointing out this, it would be quite handy to detect qgroup bugs. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
====== 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> 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. 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. --- 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 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -126,7 +126,8 @@ static void clear_btree_io_tree(struct extent_io_tree *tree) } static noinline void switch_commit_roots(struct btrfs_transaction *trans, - struct btrfs_fs_info *fs_info) + struct btrfs_fs_info *fs_info, + int free_dropped_roots) { struct btrfs_root *root, *tmp; @@ -142,16 +143,18 @@ static noinline void switch_commit_roots(struct btrfs_transaction *trans, } /* We can free old roots now. */ - spin_lock(&trans->dropped_roots_lock); - while (!list_empty(&trans->dropped_roots)) { - root = list_first_entry(&trans->dropped_roots, - struct btrfs_root, root_list); - list_del_init(&root->root_list); - spin_unlock(&trans->dropped_roots_lock); - btrfs_drop_and_free_fs_root(fs_info, root); + if (free_dropped_roots) { spin_lock(&trans->dropped_roots_lock); + while (!list_empty(&trans->dropped_roots)) { + root = list_first_entry(&trans->dropped_roots, + struct btrfs_root, root_list); + list_del_init(&root->root_list); + spin_unlock(&trans->dropped_roots_lock); + btrfs_drop_and_free_fs_root(fs_info, root); + spin_lock(&trans->dropped_roots_lock); + } + spin_unlock(&trans->dropped_roots_lock); } - spin_unlock(&trans->dropped_roots_lock); up_write(&fs_info->commit_root_sem); } @@ -311,12 +314,13 @@ loop: * when the transaction commits */ static int record_root_in_trans(struct btrfs_trans_handle *trans, - struct btrfs_root *root) + struct btrfs_root *root, + int force) { - if (test_bit(BTRFS_ROOT_REF_COWS, &root->state) && - root->last_trans < trans->transid) { + if ((test_bit(BTRFS_ROOT_REF_COWS, &root->state) && + root->last_trans < trans->transid) || force) { WARN_ON(root == root->fs_info->extent_root); - WARN_ON(root->commit_root != root->node); + WARN_ON(root->commit_root != root->node && !force); /* * see below for IN_TRANS_SETUP usage rules @@ -331,7 +335,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans, smp_wmb(); spin_lock(&root->fs_info->fs_roots_radix_lock); - if (root->last_trans == trans->transid) { + if (root->last_trans == trans->transid && !force) { spin_unlock(&root->fs_info->fs_roots_radix_lock); return 0; } @@ -402,7 +406,7 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans, return 0; mutex_lock(&root->fs_info->reloc_mutex); - record_root_in_trans(trans, root); + record_root_in_trans(trans, root, 0); mutex_unlock(&root->fs_info->reloc_mutex); return 0; @@ -1383,7 +1387,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, dentry = pending->dentry; parent_inode = pending->dir; parent_root = BTRFS_I(parent_inode)->root; - record_root_in_trans(trans, parent_root); + record_root_in_trans(trans, parent_root, 0); cur_time = current_fs_time(parent_inode->i_sb); @@ -1420,7 +1424,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, goto fail; } - record_root_in_trans(trans, root); + record_root_in_trans(trans, root, 0); btrfs_set_root_last_snapshot(&root->root_item, trans->transid); memcpy(new_root_item, &root->root_item, sizeof(*new_root_item)); btrfs_check_and_init_root_item(new_root_item); @@ -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, @@ -1527,6 +1590,12 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, goto fail; } + /* + * Force parent root to be updated, as we recorded it before its + * last_trans == cur_transid + */ + record_root_in_trans(trans, parent_root, 1); + btrfs_i_size_write(parent_inode, parent_inode->i_size + dentry->d_name.len * 2); parent_inode->i_mtime = parent_inode->i_ctime = @@ -1559,23 +1628,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, goto fail; } - /* - * account qgroup counters before qgroup_inherit() - */ - ret = btrfs_qgroup_prepare_account_extents(trans, fs_info); - if (ret) - goto fail; - ret = btrfs_qgroup_account_extents(trans, fs_info); - if (ret) - goto fail; - ret = btrfs_qgroup_inherit(trans, fs_info, - root->root_key.objectid, - objectid, pending->inherit); - if (ret) { - btrfs_abort_transaction(trans, root, ret); - goto fail; - } - fail: pending->error = ret; dir_item_existed: @@ -2115,7 +2167,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, list_add_tail(&root->fs_info->chunk_root->dirty_list, &cur_trans->switch_commits); - switch_commit_roots(cur_trans, root->fs_info); + switch_commit_roots(cur_trans, root->fs_info, 1); assert_qgroups_uptodate(trans); ASSERT(list_empty(&cur_trans->dirty_bgs));