| Message ID | 1474455620-25982-1-git-send-email-zlang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 21, 2016 at 07:00:20PM +0800, Zorro Lang wrote: > From linux v4.3 and xfsprogs v4.2, xfs_copy support to copy a V5 XFS. > Before that, copy a V5 XFS will cause target fs corruption, and only > use "-d" option can resolve that problem. > > For this old reason, xfstests use below patch to add '-d' option to > xfs_copy, make sure xfs_copy always use that option if it try to copy > a V5 XFS: > > 8346e53 common: append -d option to XFS_COPY_PROG when testing v5 xfs > > But xfs_copy full support v5 xfs now. So xfstest miss the coverage of > copy a V5 XFS without '-d'. For test this feature I did below things: > > 1. Revert commit 8346e53 > 2. Add a new common function _require_xfs_copy(), if a test try to > use old xfsprogs to copy a V5 xfs, it'll skip that test. > 3. Add above common function into all xfs_copy related cases. > 4. xfs/073 test V4 xfs forcibly by specify "-m crc=0" in case. I > think it's useless now, so remove it. > > There's a xfs_copy related case I didn't add _require_xfs_copy() > to it -- xfs/032, due to it tests "xfs_copy -d" directly. > > Signed-off-by: Zorro Lang <zlang@redhat.com> > --- > common/rc | 27 ++++++++++++++++++++++----- > tests/xfs/073 | 9 +++------ > tests/xfs/077 | 1 + > 3 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/common/rc b/common/rc > index 13afc6a..9ba073b 100644 > --- a/common/rc > +++ b/common/rc > @@ -3789,11 +3789,6 @@ init_rc() > # Figure out if we need to add -F ("foreign", deprecated) option to xfs_io > xfs_io -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \ > export XFS_IO_PROG="$XFS_IO_PROG -F" > - > - # xfs_copy doesn't work on v5 xfs yet without -d option > - if [ "$FSTYP" == "xfs" ] && [[ $MKFS_OPTIONS =~ crc=1 ]]; then > - export XFS_COPY_PROG="$XFS_COPY_PROG -d" > - fi > } > > # get real device path name by following link > @@ -3994,6 +3989,28 @@ _require_xfs_mkfs_without_validation() > fi > } > > +# Make sure xfs_copy full support to copy V5 XFS, due to old xfs_copy can't > +# yet copy V5 xfs without '-d'. But if xfs_copy can't copy V4 XFS, that's a > +# bug. > +_require_xfs_copy() I'd suggest name it as _require_xfs_copy_crc, which indicates we need xfs_copy crc support, like _require_xfs_mkfs_crc. > +{ > + _scratch_mkfs | _filter_mkfs 2>$tmp.mkfs >/dev/null Assuming we have $SCRATCH_DEV defined in a common helper doesn't seem right to me. Actually you don't have to use $SCRATCH_DEV in this test touch $tmp.img $MKFS_XFS_PROG -d file,name=$tmp.img,size=32m then test copy on this image $XFS_COPY_PROG $tmp.img $tmp.copy > + . $tmp.mkfs > + > + ${XFS_COPY_PROG} $SCRATCH_DEV $tmp.copy >/dev/null 2>&1 > + local rc=$? > + > + rm -f $tmp.mkfs > + rm -f $tmp.copy 2>/dev/null > + if [ $rc -ne 0 ]; then > + if [ $_fs_has_crcs -eq 1 ]; then > + _notrun "This test requires xfs_copy support to copy V5 xfs without -d" > + else > + _fail "xfs_copy can't copy a V4 xfs" I don't think this kind of "_fail" belongs to a "_require" helper, it should be done in tests. > + fi > + fi > +} > + After going through this function, I find that it's not always checking if xfs_copy has crc support, it checks the crc support only when _scratch_mkfs creates a crc enabled fs. This information is "hidden" in the helper, and this behavior doesn't match the function name and the comments. So I'd suggest the following, what do you think? Create a new helper _require_xfs_copy_crc() as I suggested above, which always checks if xfs_copy has crc support. Then call this helper from xfs/073 and xfs/077 only when we're testing v5 XFS. i.e. _scratch_mkfs_xfs -dsize=41m,agcount=2 | _filter_mkfs 2>$tmp.mkfs >>$seqres.full . $tmp.mkfs # we need xfs_copy CRC support if we're testing CRC enabled fs if [ $_fs_has_crcs -eq 1 ]; then _require_xfs_copy_crc fi _scratch_mount 2>/dev/null || _fail "initial scratch mount failed" So it's clear why we need xfs_copy crc support, this information is not "buried" in the original _require_xfs_copy_crc. Thanks, Eryu > init_rc > > ################################################################################ > diff --git a/tests/xfs/073 b/tests/xfs/073 > index 9e29223..02e45d5 100755 > --- a/tests/xfs/073 > +++ b/tests/xfs/073 > @@ -134,11 +134,12 @@ _require_attrs > [ -n "$XFS_COPY_PROG" ] || _notrun "xfs_copy binary not yet installed" > > _require_scratch > +_require_xfs_copy > _require_loop > > rm -f $seqres.full > > -_scratch_mkfs_xfs -m crc=0 -dsize=41m,agcount=2 >>$seqres.full 2>&1 > +_scratch_mkfs_xfs -dsize=41m,agcount=2 >>$seqres.full 2>&1 > _scratch_mount 2>/dev/null || _fail "initial scratch mount failed" > > echo > @@ -158,11 +159,7 @@ _verify_copy $imgs.image $SCRATCH_DEV $SCRATCH_MNT > > echo > echo === copying scratch device to single target, large ro device > -mkfs_crc_opts="-m crc=0" > -if [ -n "$XFS_MKFS_HAS_NO_META_SUPPORT" ]; then > - mkfs_crc_opts="" > -fi > -${MKFS_XFS_PROG} $mkfs_crc_opts -dfile,name=$imgs.source,size=100g \ > +${MKFS_XFS_PROG} -dfile,name=$imgs.source,size=100g \ > | _filter_mkfs 2>/dev/null > rmdir $imgs.source_dir 2>/dev/null > mkdir $imgs.source_dir > diff --git a/tests/xfs/077 b/tests/xfs/077 > index 007d05d..7bcbfb5 100755 > --- a/tests/xfs/077 > +++ b/tests/xfs/077 > @@ -51,6 +51,7 @@ _cleanup() > _supported_fs xfs > _supported_os Linux > _require_scratch > +_require_xfs_copy > _require_xfs_crc > _require_meta_uuid > > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 21, 2016 at 07:00:20PM +0800, Zorro Lang wrote: > From linux v4.3 and xfsprogs v4.2, xfs_copy support to copy a V5 XFS. > Before that, copy a V5 XFS will cause target fs corruption, and only > use "-d" option can resolve that problem. What has the kernel version got to do with xfs_copy support? What matters is whether xfsprogs supports a specific feature bit in the XFS superblock - that is what indicates whether xfs_copy requires the "-d" option or not.... > +# Make sure xfs_copy full support to copy V5 XFS, due to old xfs_copy can't > +# yet copy V5 xfs without '-d'. But if xfs_copy can't copy V4 XFS, that's a > +# bug. > +_require_xfs_copy() > +{ > + _scratch_mkfs | _filter_mkfs 2>$tmp.mkfs >/dev/null > + . $tmp.mkfs > + > + ${XFS_COPY_PROG} $SCRATCH_DEV $tmp.copy >/dev/null 2>&1 > + local rc=$? > + > + rm -f $tmp.mkfs > + rm -f $tmp.copy 2>/dev/null > + if [ $rc -ne 0 ]; then > + if [ $_fs_has_crcs -eq 1 ]; then > + _notrun "This test requires xfs_copy support to copy V5 xfs without -d" > + else > + _fail "xfs_copy can't copy a V4 xfs" > + fi > + fi > +} Far too over engineered - the regression tests check whether the functionality works correctly, not the _requires* check. The _requires* check are just for determining whether the operation is supported or not. As I said above, xfs_copy on v5 filesystems do not require the "-d" option if xfs_admin can change the UUID on v5 filesystems. i.e. if this works: # xfs_admin -U generate /dev/pmem1 Clearing log and setting UUID writing all SBs new UUID = b4e22c8b-1bfb-4307-a3f2-4f55b5c9d61d # echo $? 0 # Then xfs_copy does not require "-d" option. This works for both v4 and v5 filesystems, so this check can be done when setting up the $XFS_COPY_PROG variable - there is no need for a _requires rule to check this in every test. -Dave.
On 9/21/16 4:37 PM, Dave Chinner wrote: > On Wed, Sep 21, 2016 at 07:00:20PM +0800, Zorro Lang wrote: >> From linux v4.3 and xfsprogs v4.2, xfs_copy support to copy a V5 XFS. >> Before that, copy a V5 XFS will cause target fs corruption, and only >> use "-d" option can resolve that problem. > > What has the kernel version got to do with xfs_copy support? > > What matters is whether xfsprogs supports a specific feature bit in > the XFS superblock - that is what indicates whether xfs_copy > requires the "-d" option or not.... For V5, xfs_copy went through a few stages: 1) Corrupted the filesystem without -d 2) Refused to run without -d 3) Used meta_uuid to run without -d 4) Fixed a bug with multiple targets So it probably comes down to tests identifying /broken/ xfs_copy instances. ( 2) wasn't broken, it was just intentionally crippled, so I'm not sure how tests should handle that case.) Anyway, skip down ... >> +# Make sure xfs_copy full support to copy V5 XFS, due to old xfs_copy can't >> +# yet copy V5 xfs without '-d'. But if xfs_copy can't copy V4 XFS, that's a >> +# bug. >> +_require_xfs_copy() >> +{ >> + _scratch_mkfs | _filter_mkfs 2>$tmp.mkfs >/dev/null >> + . $tmp.mkfs >> + >> + ${XFS_COPY_PROG} $SCRATCH_DEV $tmp.copy >/dev/null 2>&1 >> + local rc=$? >> + >> + rm -f $tmp.mkfs >> + rm -f $tmp.copy 2>/dev/null >> + if [ $rc -ne 0 ]; then >> + if [ $_fs_has_crcs -eq 1 ]; then >> + _notrun "This test requires xfs_copy support to copy V5 xfs without -d" >> + else >> + _fail "xfs_copy can't copy a V4 xfs" >> + fi >> + fi >> +} > > Far too over engineered - the regression tests check whether the > functionality works correctly, not the _requires* check. The > _requires* check are just for determining whether the operation is > supported or not. > > As I said above, xfs_copy on v5 filesystems do not require the > "-d" option if xfs_admin can change the UUID on v5 filesystems. > i.e. if this works: > > # xfs_admin -U generate /dev/pmem1 > Clearing log and setting UUID > writing all SBs > new UUID = b4e22c8b-1bfb-4307-a3f2-4f55b5c9d61d > # echo $? > 0 > # "works," meaning "doesn't corrupt" I guess, right? 3.2.2 actually allowed it to proceed, but corrupted the filesystem. Then later it, too, was restricted on v5 filesystems, and later those restrictions were lifted. Honestly, (and Dave helped push me in this direction as well), I think the addition of "-d" should just be removed; we now have a binary that /works/ and there's no reason to avoid running broken binaries - the whole point of testing is to find out whether what you're testing works, or if it's broken. Skipping tests because they might fail leaves you with missing information about the environment you're testing. -Eric -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 21, 2016 at 05:07:15PM -0500, Eric Sandeen wrote: > On 9/21/16 4:37 PM, Dave Chinner wrote: > > On Wed, Sep 21, 2016 at 07:00:20PM +0800, Zorro Lang wrote: > > As I said above, xfs_copy on v5 filesystems do not require the > > "-d" option if xfs_admin can change the UUID on v5 filesystems. > > i.e. if this works: > > > > # xfs_admin -U generate /dev/pmem1 > > Clearing log and setting UUID > > writing all SBs > > new UUID = b4e22c8b-1bfb-4307-a3f2-4f55b5c9d61d > > # echo $? > > 0 > > # > > "works," meaning "doesn't corrupt" I guess, right? I meant "works" as in "fully supported". The requirement for "-d" with xfs_copy was present in the first release that supported CRCs (i.e. 3.2.0). That was added in commit a872b62 ("xfs_copy: band-aids for CRC filesystems") for in 3.2.0-rc1. > 3.2.2 actually > allowed it to proceed, but corrupted the filesystem. Sure, it had bugs. Which have since been fixed. :P > Then later it, > too, was restricted on v5 filesystems, and later those restrictions > were lifted. It was restricted from the first release, which is why I suggested just testing whether the UUID can be changed, because that covers all xfsprogs releases supporting CRCs up to the point where "-d" is no longer necessary... > Honestly, (and Dave helped push me in this direction as well), I think > the addition of "-d" should just be removed; we now have a binary that > /works/ and there's no reason to avoid running broken binaries - the > whole point of testing is to find out whether what you're testing works, > or if it's broken. Skipping tests because they might fail leaves you with > missing information about the environment you're testing. Yes, though my point was wider than that, and about _requires rules in general - they are simply checks for infrastructure an support being present on the system the test is being run on. They do not, and should not, try to determine if the functionality works or whether the version being used has a specific bug in it or not - the tests themselves will tell us that when they fail. A reminder to people running xfstests in QE environments for older distros: if there are tests that are known to fail because of a lack of support or known, WONTFIX bugs, you are supposed to avoid running them via the use of custom expunge files, not _requires rules. Keep distro release specific expunge files to list what tests should not be run (you can add comments to those files to document the reasons) in your QE configuration management environment and deploy them appropriately to your test targets.... Cheers, Dave.
diff --git a/common/rc b/common/rc index 13afc6a..9ba073b 100644 --- a/common/rc +++ b/common/rc @@ -3789,11 +3789,6 @@ init_rc() # Figure out if we need to add -F ("foreign", deprecated) option to xfs_io xfs_io -c stat $TEST_DIR 2>&1 | grep -q "is not on an XFS filesystem" && \ export XFS_IO_PROG="$XFS_IO_PROG -F" - - # xfs_copy doesn't work on v5 xfs yet without -d option - if [ "$FSTYP" == "xfs" ] && [[ $MKFS_OPTIONS =~ crc=1 ]]; then - export XFS_COPY_PROG="$XFS_COPY_PROG -d" - fi } # get real device path name by following link @@ -3994,6 +3989,28 @@ _require_xfs_mkfs_without_validation() fi } +# Make sure xfs_copy full support to copy V5 XFS, due to old xfs_copy can't +# yet copy V5 xfs without '-d'. But if xfs_copy can't copy V4 XFS, that's a +# bug. +_require_xfs_copy() +{ + _scratch_mkfs | _filter_mkfs 2>$tmp.mkfs >/dev/null + . $tmp.mkfs + + ${XFS_COPY_PROG} $SCRATCH_DEV $tmp.copy >/dev/null 2>&1 + local rc=$? + + rm -f $tmp.mkfs + rm -f $tmp.copy 2>/dev/null + if [ $rc -ne 0 ]; then + if [ $_fs_has_crcs -eq 1 ]; then + _notrun "This test requires xfs_copy support to copy V5 xfs without -d" + else + _fail "xfs_copy can't copy a V4 xfs" + fi + fi +} + init_rc ################################################################################ diff --git a/tests/xfs/073 b/tests/xfs/073 index 9e29223..02e45d5 100755 --- a/tests/xfs/073 +++ b/tests/xfs/073 @@ -134,11 +134,12 @@ _require_attrs [ -n "$XFS_COPY_PROG" ] || _notrun "xfs_copy binary not yet installed" _require_scratch +_require_xfs_copy _require_loop rm -f $seqres.full -_scratch_mkfs_xfs -m crc=0 -dsize=41m,agcount=2 >>$seqres.full 2>&1 +_scratch_mkfs_xfs -dsize=41m,agcount=2 >>$seqres.full 2>&1 _scratch_mount 2>/dev/null || _fail "initial scratch mount failed" echo @@ -158,11 +159,7 @@ _verify_copy $imgs.image $SCRATCH_DEV $SCRATCH_MNT echo echo === copying scratch device to single target, large ro device -mkfs_crc_opts="-m crc=0" -if [ -n "$XFS_MKFS_HAS_NO_META_SUPPORT" ]; then - mkfs_crc_opts="" -fi -${MKFS_XFS_PROG} $mkfs_crc_opts -dfile,name=$imgs.source,size=100g \ +${MKFS_XFS_PROG} -dfile,name=$imgs.source,size=100g \ | _filter_mkfs 2>/dev/null rmdir $imgs.source_dir 2>/dev/null mkdir $imgs.source_dir diff --git a/tests/xfs/077 b/tests/xfs/077 index 007d05d..7bcbfb5 100755 --- a/tests/xfs/077 +++ b/tests/xfs/077 @@ -51,6 +51,7 @@ _cleanup() _supported_fs xfs _supported_os Linux _require_scratch +_require_xfs_copy _require_xfs_crc _require_meta_uuid
From linux v4.3 and xfsprogs v4.2, xfs_copy support to copy a V5 XFS. Before that, copy a V5 XFS will cause target fs corruption, and only use "-d" option can resolve that problem. For this old reason, xfstests use below patch to add '-d' option to xfs_copy, make sure xfs_copy always use that option if it try to copy a V5 XFS: 8346e53 common: append -d option to XFS_COPY_PROG when testing v5 xfs But xfs_copy full support v5 xfs now. So xfstest miss the coverage of copy a V5 XFS without '-d'. For test this feature I did below things: 1. Revert commit 8346e53 2. Add a new common function _require_xfs_copy(), if a test try to use old xfsprogs to copy a V5 xfs, it'll skip that test. 3. Add above common function into all xfs_copy related cases. 4. xfs/073 test V4 xfs forcibly by specify "-m crc=0" in case. I think it's useless now, so remove it. There's a xfs_copy related case I didn't add _require_xfs_copy() to it -- xfs/032, due to it tests "xfs_copy -d" directly. Signed-off-by: Zorro Lang <zlang@redhat.com> --- common/rc | 27 ++++++++++++++++++++++----- tests/xfs/073 | 9 +++------ tests/xfs/077 | 1 + 3 files changed, 26 insertions(+), 11 deletions(-)