| Message ID | 1460027155-4222-1-git-send-email-eguan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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 Thu, Apr 07, 2016 at 07:05:55PM +0800, Eryu Guan wrote: > Currently xfs/259 checks $TEST_DIR for CRC support status to determine > if 512 block size should be tested. But this doesn't always work. For > example, when TEST_DEV is mkfs'ed with "-m crc=0" mkfs option, using > mkfs.xfs binary with CRC being the default. > > What should be really checked is whether mkfs.xfs creates CRC enabled > XFS by default. So introduce a new flag XFS_MKFS_CRC_DEFAULT for this > purpose, and do the check based on it in xfs/259. > > Signed-off-by: Eryu Guan <eguan@redhat.com> > --- > > This is actually the second attempt to fix this issue, because Christoph was > not satisfied with the first attempt, and me either (after thinking about it > more :-)). Hope it works this time. > > common/config | 8 ++++++++ > tests/xfs/259 | 4 +--- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/common/config b/common/config > index cacd815..13bd307 100644 > --- a/common/config > +++ b/common/config > @@ -270,8 +270,16 @@ $MKFS_XFS_PROG -N -d file,name=/tmp/crc_check.img,size=32m -m crc=0 \ > if [ $? -ne 0 ]; then > XFS_MKFS_HAS_NO_META_SUPPORT=true > fi > +# check if v5 xfs is default > +XFS_MKFS_CRC_DEFAULT=0 > +$MKFS_XFS_PROG -N -d file,name=/tmp/crc_check.img,size=32m 2>&1 | grep -q crc=1 > +if [ $? -eq 0 ]; then > + XFS_MKFS_CRC_DEFAULT=1 > +fi > rm -f /tmp/crc_check.img That tests the mkfs configuration that is applied to the scratch device.... > diff --git a/tests/xfs/259 b/tests/xfs/259 > index 16c1935..3150ff3 100755 > --- a/tests/xfs/259 > +++ b/tests/xfs/259 > @@ -51,9 +51,7 @@ testfile=$TEST_DIR/259.image > # Test various sizes slightly less than 4 TB. Need to handle different > # minimum block sizes for CRC enabled filesystems, but use a small log so we > # don't write lots of zeros unnecessarily. > -xfs_info $TEST_DIR | _filter_mkfs 2> $tmp.mkfs > /dev/null > -. $tmp.mkfs This tests the configuration of the test device, which is not controlled by the test harness, so can be different to the configuration being used for the scratch device. > -if [ $_fs_has_crcs -eq 1 ]; then > +if [ $XFS_MKFS_CRC_DEFAULT -eq 1 ]; then IOWs, this is not an not equivalent test. Cheers, Dave.
On Fri, Apr 08, 2016 at 07:32:31AM +1000, Dave Chinner wrote: > > diff --git a/tests/xfs/259 b/tests/xfs/259 > > index 16c1935..3150ff3 100755 > > --- a/tests/xfs/259 > > +++ b/tests/xfs/259 > > @@ -51,9 +51,7 @@ testfile=$TEST_DIR/259.image > > # Test various sizes slightly less than 4 TB. Need to handle different > > # minimum block sizes for CRC enabled filesystems, but use a small log so we > > # don't write lots of zeros unnecessarily. > > -xfs_info $TEST_DIR | _filter_mkfs 2> $tmp.mkfs > /dev/null > > -. $tmp.mkfs > > This tests the configuration of the test device, which is not > controlled by the test harness, so can be different to the > configuration being used for the scratch device. > > > -if [ $_fs_has_crcs -eq 1 ]; then > > +if [ $XFS_MKFS_CRC_DEFAULT -eq 1 ]; then > > IOWs, this is not an not equivalent test. And I think that's the whole point of this change :) Previously it tested what the TEST_DIR did, which was wrong for this test. Now it tests what mkfs does by default (including for the scratch dev), which is what we really want here. -- 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 Thu, Apr 07, 2016 at 04:48:37PM -0700, Christoph Hellwig wrote: > On Fri, Apr 08, 2016 at 07:32:31AM +1000, Dave Chinner wrote: > > > diff --git a/tests/xfs/259 b/tests/xfs/259 > > > index 16c1935..3150ff3 100755 > > > --- a/tests/xfs/259 > > > +++ b/tests/xfs/259 > > > @@ -51,9 +51,7 @@ testfile=$TEST_DIR/259.image > > > # Test various sizes slightly less than 4 TB. Need to handle different > > > # minimum block sizes for CRC enabled filesystems, but use a small log so we > > > # don't write lots of zeros unnecessarily. > > > -xfs_info $TEST_DIR | _filter_mkfs 2> $tmp.mkfs > /dev/null > > > -. $tmp.mkfs > > > > This tests the configuration of the test device, which is not > > controlled by the test harness, so can be different to the > > configuration being used for the scratch device. > > > > > -if [ $_fs_has_crcs -eq 1 ]; then > > > +if [ $XFS_MKFS_CRC_DEFAULT -eq 1 ]; then > > > > IOWs, this is not an not equivalent test. > > And I think that's the whole point of this change :) > > Previously it tested what the TEST_DIR did, which was wrong for this > test. Now it tests what mkfs does by default (including for the scratch > dev), which is what we really want here. Which is not at all clear from the patch description. Seriously, though, this does not belong in common/config. We already have a helper function to check what mkfs supports (i.e. _scratch_mkfs_xfs_supported()), and if we just want a bare check then factor this into a _mkfs_xfs_supported() and supply the parameters specific to the test. Indeed, this is basically what we do with _require_xfs_mkfs_crc(); the same thing should be done, but without the "notrun" if -m crc s not supported... Cheers, Dave.
On Mon, Apr 11, 2016 at 10:02:38AM +1000, Dave Chinner wrote: > On Thu, Apr 07, 2016 at 04:48:37PM -0700, Christoph Hellwig wrote: > > On Fri, Apr 08, 2016 at 07:32:31AM +1000, Dave Chinner wrote: > > > > diff --git a/tests/xfs/259 b/tests/xfs/259 > > > > index 16c1935..3150ff3 100755 > > > > --- a/tests/xfs/259 > > > > +++ b/tests/xfs/259 > > > > @@ -51,9 +51,7 @@ testfile=$TEST_DIR/259.image > > > > # Test various sizes slightly less than 4 TB. Need to handle different > > > > # minimum block sizes for CRC enabled filesystems, but use a small log so we > > > > # don't write lots of zeros unnecessarily. > > > > -xfs_info $TEST_DIR | _filter_mkfs 2> $tmp.mkfs > /dev/null > > > > -. $tmp.mkfs > > > > > > This tests the configuration of the test device, which is not > > > controlled by the test harness, so can be different to the > > > configuration being used for the scratch device. > > > > > > > -if [ $_fs_has_crcs -eq 1 ]; then > > > > +if [ $XFS_MKFS_CRC_DEFAULT -eq 1 ]; then > > > > > > IOWs, this is not an not equivalent test. > > > > And I think that's the whole point of this change :) > > > > Previously it tested what the TEST_DIR did, which was wrong for this > > test. Now it tests what mkfs does by default (including for the scratch > > dev), which is what we really want here. > > Which is not at all clear from the patch description. > > Seriously, though, this does not belong in common/config. We already > have a helper function to check what mkfs supports (i.e. > _scratch_mkfs_xfs_supported()), and if we just want a bare check > then factor this into a _mkfs_xfs_supported() and supply the > parameters specific to the test. Will do. > > Indeed, this is basically what we do with _require_xfs_mkfs_crc(); > the same thing should be done, but without the "notrun" if -m crc > s not supported... Thanks for reviewing! Eryu -- 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 Mon, Apr 11, 2016 at 10:02:38AM +1000, Dave Chinner wrote: > On Thu, Apr 07, 2016 at 04:48:37PM -0700, Christoph Hellwig wrote: > > On Fri, Apr 08, 2016 at 07:32:31AM +1000, Dave Chinner wrote: > > > > diff --git a/tests/xfs/259 b/tests/xfs/259 > > > > index 16c1935..3150ff3 100755 > > > > --- a/tests/xfs/259 > > > > +++ b/tests/xfs/259 > > > > @@ -51,9 +51,7 @@ testfile=$TEST_DIR/259.image > > > > # Test various sizes slightly less than 4 TB. Need to handle different > > > > # minimum block sizes for CRC enabled filesystems, but use a small log so we > > > > # don't write lots of zeros unnecessarily. > > > > -xfs_info $TEST_DIR | _filter_mkfs 2> $tmp.mkfs > /dev/null > > > > -. $tmp.mkfs > > > > > > This tests the configuration of the test device, which is not > > > controlled by the test harness, so can be different to the > > > configuration being used for the scratch device. > > > > > > > -if [ $_fs_has_crcs -eq 1 ]; then > > > > +if [ $XFS_MKFS_CRC_DEFAULT -eq 1 ]; then > > > > > > IOWs, this is not an not equivalent test. > > > > And I think that's the whole point of this change :) > > > > Previously it tested what the TEST_DIR did, which was wrong for this > > test. Now it tests what mkfs does by default (including for the scratch > > dev), which is what we really want here. > > Which is not at all clear from the patch description. > > Seriously, though, this does not belong in common/config. We already > have a helper function to check what mkfs supports (i.e. > _scratch_mkfs_xfs_supported()), and if we just want a bare check > then factor this into a _mkfs_xfs_supported() and supply the > parameters specific to the test. > > Indeed, this is basically what we do with _require_xfs_mkfs_crc(); > the same thing should be done, but without the "notrun" if -m crc > s not supported... Looking into _require_xfs_mkfs_crc() and _scratch_mkfs_xfs_supported(), I noticed that they are not the helpers I want. They are testing whether mkfs.xfs supports CRC (or other mkfs options), what I want is what's the default behavior of mkfs.xfs (CRC enabled or not). Maybe I can just move the detection code to common/rc? Thanks, Eryu -- 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 Mon, Apr 11, 2016 at 07:38:11PM +0800, Eryu Guan wrote: > On Mon, Apr 11, 2016 at 10:02:38AM +1000, Dave Chinner wrote: > > Which is not at all clear from the patch description. > > > > Seriously, though, this does not belong in common/config. We already > > have a helper function to check what mkfs supports (i.e. > > _scratch_mkfs_xfs_supported()), and if we just want a bare check > > then factor this into a _mkfs_xfs_supported() and supply the > > parameters specific to the test. > > > > Indeed, this is basically what we do with _require_xfs_mkfs_crc(); > > the same thing should be done, but without the "notrun" if -m crc > > s not supported... > > Looking into _require_xfs_mkfs_crc() and _scratch_mkfs_xfs_supported(), > I noticed that they are not the helpers I want. They are testing whether > mkfs.xfs supports CRC (or other mkfs options), what I want is what's the > default behavior of mkfs.xfs (CRC enabled or not). All this, just to avoid testing on an invalid block size when CRCs are enabled. I really don't see why this needs changes to generic infrastructure - it's a test specific problem. How about you simply reverse the block size order that is tested, and capture the output of the actual mkfs command that is being tested, and determine if 512 byte block sizes should be tested based on that output? i.e. for b in 4096 2038 1024 512; do if [ $b -eq 512 -a $_fs_has_crcs -ne 1 ]; then break; fi .... mkfs -b $b .... . $tmp.mkfs done Cheers, Dave.
diff --git a/common/config b/common/config index cacd815..13bd307 100644 --- a/common/config +++ b/common/config @@ -270,8 +270,16 @@ $MKFS_XFS_PROG -N -d file,name=/tmp/crc_check.img,size=32m -m crc=0 \ if [ $? -ne 0 ]; then XFS_MKFS_HAS_NO_META_SUPPORT=true fi +# check if v5 xfs is default +XFS_MKFS_CRC_DEFAULT=0 +$MKFS_XFS_PROG -N -d file,name=/tmp/crc_check.img,size=32m 2>&1 | grep -q crc=1 +if [ $? -eq 0 ]; then + XFS_MKFS_CRC_DEFAULT=1 +fi rm -f /tmp/crc_check.img + export XFS_MKFS_HAS_NO_META_SUPPORT +export XFS_MKFS_CRC_DEFAULT # new doesn't need config file parsed, we can stop here if [ "$iam" == "new" ]; then diff --git a/tests/xfs/259 b/tests/xfs/259 index 16c1935..3150ff3 100755 --- a/tests/xfs/259 +++ b/tests/xfs/259 @@ -51,9 +51,7 @@ testfile=$TEST_DIR/259.image # Test various sizes slightly less than 4 TB. Need to handle different # minimum block sizes for CRC enabled filesystems, but use a small log so we # don't write lots of zeros unnecessarily. -xfs_info $TEST_DIR | _filter_mkfs 2> $tmp.mkfs > /dev/null -. $tmp.mkfs -if [ $_fs_has_crcs -eq 1 ]; then +if [ $XFS_MKFS_CRC_DEFAULT -eq 1 ]; then blocksize=1024 sizes_to_check="1024 2048 4096" echo "Trying to make (4 TB - 512) B long xfs fs image"
Currently xfs/259 checks $TEST_DIR for CRC support status to determine if 512 block size should be tested. But this doesn't always work. For example, when TEST_DEV is mkfs'ed with "-m crc=0" mkfs option, using mkfs.xfs binary with CRC being the default. What should be really checked is whether mkfs.xfs creates CRC enabled XFS by default. So introduce a new flag XFS_MKFS_CRC_DEFAULT for this purpose, and do the check based on it in xfs/259. Signed-off-by: Eryu Guan <eguan@redhat.com> --- This is actually the second attempt to fix this issue, because Christoph was not satisfied with the first attempt, and me either (after thinking about it more :-)). Hope it works this time. common/config | 8 ++++++++ tests/xfs/259 | 4 +--- 2 files changed, 9 insertions(+), 3 deletions(-)