On Fri, Aug 10, 2018 at 9:46 AM, Qu Wenruo <quwenruo.bt...@gmx.com> wrote: > > > On 8/9/18 5:26 PM, Filipe Manana wrote: >> On Thu, Aug 9, 2018 at 8:45 AM, Qu Wenruo <w...@suse.com> wrote: >>> This bug is exposed by populating a high level qgroup, and then make it >>> orphan (high level qgroup without child) >> >> Same comment as in the kernel patch: >> >> "That sentence is confusing. An orphan, by definition [1], is someone >> (or something in this case) without parents. >> But you mention a group without children, so that should be named >> "childless" or simply say "without children". >> So one part of the sentence is wrong, either what is in parenthesis or >> what comes before them. >> >> [1] https://www.thefreedictionary.com/orphan >> " >> >>> with old qgroup numbers, and >>> finally do rescan. >>> >>> Normally rescan should zero out all qgroups' accounting number, but due >>> to a kernel bug which won't mark orphan qgroups dirty, their on-disk >>> data is not updated, thus old numbers remain and cause qgroup >>> corruption. >>> >>> Fixed by the following kernel patch: >>> "btrfs: qgroup: Dirty all qgroups before rescan" >>> >>> Reported-by: Misono Tomohiro <misono.tomoh...@jp.fujitsu.com> >>> Signed-off-by: Qu Wenruo <w...@suse.com> >>> --- >>> tests/btrfs/170 | 82 +++++++++++++++++++++++++++++++++++++++++++++ >>> tests/btrfs/170.out | 3 ++ >>> tests/btrfs/group | 1 + >>> 3 files changed, 86 insertions(+) >>> create mode 100755 tests/btrfs/170 >>> create mode 100644 tests/btrfs/170.out >>> >>> diff --git a/tests/btrfs/170 b/tests/btrfs/170 >>> new file mode 100755 >>> index 000000000000..bcf8b5c0e4f3 >>> --- /dev/null >>> +++ b/tests/btrfs/170 >>> @@ -0,0 +1,82 @@ >>> +#! /bin/bash >>> +# SPDX-License-Identifier: GPL-2.0 >>> +# Copyright (c) 2018 SUSE Linux Products GmbH. All Rights Reserved. >>> +# >>> +# FS QA Test 170 >>> +# >>> +# Test if btrfs can clear orphan (high level qgroup without child) qgroup's >>> +# accounting numbers during rescan. >>> +# Fixed by the following kernel patch: >>> +# "btrfs: qgroup: Dirty all qgroups before rescan" >>> +# >>> +seq=`basename $0` >>> +seqres=$RESULT_DIR/$seq >>> +echo "QA output created by $seq" >>> + >>> +here=`pwd` >>> +tmp=/tmp/$$ >>> +status=1 # failure is the default! >>> +trap "_cleanup; exit \$status" 0 1 2 3 15 >>> + >>> +_cleanup() >>> +{ >>> + cd / >>> + rm -f $tmp.* >>> +} >>> + >>> +# get standard environment, filters and checks >>> +. ./common/rc >>> +. ./common/filter >>> + >>> +# remove previous $seqres.full before test >>> +rm -f $seqres.full >>> + >>> +# real QA test starts here >>> + >>> +# Modify as appropriate. >>> +_supported_fs btrfs >>> +_supported_os Linux >>> +_require_scratch >>> + >>> +_scratch_mkfs > /dev/null 2>&1 >>> +_scratch_mount >>> + >>> + >>> +# Populate the fs >>> +_run_btrfs_util_prog subvolume create "$SCRATCH_MNT/subvol" >>> +_pwrite_byte 0xcdcd 0 1M "$SCRATCH_MNT/subvol/file1" | _filter_xfs_io > >>> /dev/null >>> + >>> +# Ensure that file reach disk, so it will also appear in snapshot >> >> # Ensure that buffered file data is persisted, so we won't have an >> empty file in the snapshot. >>> +sync >>> +_run_btrfs_util_prog subvolume snapshot "$SCRATCH_MNT/subvol" >>> "$SCRATCH_MNT/snapshot" >>> + >>> + >>> +_run_btrfs_util_prog quota enable "$SCRATCH_MNT" >>> +_run_btrfs_util_prog quota rescan -w "$SCRATCH_MNT" >>> + >>> +# Create high level qgroup >>> +_run_btrfs_util_prog qgroup create 1/0 "$SCRATCH_MNT" >>> + >>> +# Don't use _run_btrfs_util_prog here, as it can return 1 to info user >>> +# that qgroup is marked inconsistent, this is a bug in btrfs-progs, but >>> +# to ensure it will work, we just ignore the return value. >> >> Comment should go away IMHO. The preferred way is to call >> $BTRFS_UTIL_PROG and have failures noticed >> through differences in the golden output. There's no point in >> mentioning something that currently doesn't work >> if it's not used here. > > In this case, I think we still need to mention why we don't use > _run_btrfs_util_progs, in fact if we use _run_btrfs_util_progs, the test > will just fail due to the return value. > > In fact, it's a workaround and worthy noting IIRC.
Still disagree, because we are not checking the return value and rely on errors printing something to stderr/stdout. > >> >>> +$BTRFS_UTIL_PROG qgroup assign "$SCRATCH_MNT/snapshot" 1/0 "$SCRATCH_MNT" >>> + >>> +# Above assign will mark qgroup inconsistent due to the shared extents >> >> assign -> assignment >> >>> +# between subvol/snapshot/high level qgroup, do rescan here >>> +_run_btrfs_util_prog quota rescan -w "$SCRATCH_MNT" >> >> Use $BTRFS_UTIL_PROG directly instead, and adjust the golden output if >> needed. > > There is nothing special needed in "quota rescan". > > Only qgroup assign/remove could return 1 instead of 0. And why not use $BTRFS_UTIL_PROG? Not only that's the preferred way to do nowadays (I know many older tests use _run_btrfs_util_prog), but it will make this test consistent as right now it uses both. > >> >>> + >>> +# Now remove the qgroup relationship and make 1/0 orphan >>> +# Due to the shared extent outside of 1/0, we will mark qgroup inconsistent >>> +# and keep the number of qgroup 1/0 >> >> Missing "." at the end of the sentences. >> >>> +$BTRFS_UTIL_PROG qgroup remove "$SCRATCH_MNT/snapshot" 1/0 "$SCRATCH_MNT" >>> + >>> +# Above removal also marks qgroup inconsistent, rescan again >>> +_run_btrfs_util_prog quota rescan -w "$SCRATCH_MNT" >> >> Use $BTRFS_UTIL_PROG directly instead, and adjust the golden output if >> needed. > > The extra warning is not outputted by rescan, it's caused by qgroup > assign/remove as mentioned above. That still doesn't answer why not using $BTRFS_UTIL_PROG, and I don't see why it can't be used. thanks > > Thanks, > Qu > >> >> Thanks. >> >>> + >>> +# After the test, btrfs check will verify qgroup numbers to catch any >>> +# corruption. >>> + >>> +# success, all done >>> +status=0 >>> +exit >>> diff --git a/tests/btrfs/170.out b/tests/btrfs/170.out >>> new file mode 100644 >>> index 000000000000..9002199e48ed >>> --- /dev/null >>> +++ b/tests/btrfs/170.out >>> @@ -0,0 +1,3 @@ >>> +QA output created by 170 >>> +WARNING: quotas may be inconsistent, rescan needed >>> +WARNING: quotas may be inconsistent, rescan needed >>> diff --git a/tests/btrfs/group b/tests/btrfs/group >>> index b616c73d09bf..339c977135c0 100644 >>> --- a/tests/btrfs/group >>> +++ b/tests/btrfs/group >>> @@ -172,3 +172,4 @@ >>> 167 auto quick replace volume >>> 168 auto quick send >>> 169 auto quick send >>> +170 auto quick qgroup >>> -- >>> 2.18.0 >>> >>> -- >>> 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 >> >> >> > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
