* [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-07 11:05 ` Eryu Guan 0 siblings, 0 replies; 16+ messages in thread From: Eryu Guan @ 2016-04-07 11:05 UTC (permalink / raw) To: fstests; +Cc: xfs, Eryu Guan 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 + 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" -- 2.5.5 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-07 11:05 ` Eryu Guan 0 siblings, 0 replies; 16+ messages in thread From: Eryu Guan @ 2016-04-07 11:05 UTC (permalink / raw) To: fstests; +Cc: Eryu Guan, xfs 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 + 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" -- 2.5.5 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely 2016-04-07 11:05 ` Eryu Guan @ 2016-04-07 15:46 ` Christoph Hellwig -1 siblings, 0 replies; 16+ messages in thread From: Christoph Hellwig @ 2016-04-07 15:46 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, xfs Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-07 15:46 ` Christoph Hellwig 0 siblings, 0 replies; 16+ messages in thread From: Christoph Hellwig @ 2016-04-07 15:46 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, xfs Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely 2016-04-07 11:05 ` Eryu Guan @ 2016-04-07 21:32 ` Dave Chinner -1 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2016-04-07 21:32 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, xfs 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-07 21:32 ` Dave Chinner 0 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2016-04-07 21:32 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, xfs 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely 2016-04-07 21:32 ` Dave Chinner @ 2016-04-07 23:48 ` Christoph Hellwig -1 siblings, 0 replies; 16+ messages in thread From: Christoph Hellwig @ 2016-04-07 23:48 UTC (permalink / raw) To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-07 23:48 ` Christoph Hellwig 0 siblings, 0 replies; 16+ messages in thread From: Christoph Hellwig @ 2016-04-07 23:48 UTC (permalink / raw) To: Dave Chinner; +Cc: Eryu Guan, fstests, xfs 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. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely 2016-04-07 23:48 ` Christoph Hellwig @ 2016-04-11 0:02 ` Dave Chinner -1 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2016-04-11 0:02 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Eryu Guan, fstests, xfs 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-11 0:02 ` Dave Chinner 0 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2016-04-11 0:02 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Eryu Guan, fstests, xfs 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely 2016-04-11 0:02 ` Dave Chinner @ 2016-04-11 3:12 ` Eryu Guan -1 siblings, 0 replies; 16+ messages in thread From: Eryu Guan @ 2016-04-11 3:12 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, fstests, xfs 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-11 3:12 ` Eryu Guan 0 siblings, 0 replies; 16+ messages in thread From: Eryu Guan @ 2016-04-11 3:12 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, fstests, xfs 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 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely 2016-04-11 0:02 ` Dave Chinner @ 2016-04-11 11:38 ` Eryu Guan -1 siblings, 0 replies; 16+ messages in thread From: Eryu Guan @ 2016-04-11 11:38 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, fstests, xfs 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-11 11:38 ` Eryu Guan 0 siblings, 0 replies; 16+ messages in thread From: Eryu Guan @ 2016-04-11 11:38 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, fstests, xfs 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 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely 2016-04-11 11:38 ` Eryu Guan @ 2016-04-12 21:01 ` Dave Chinner -1 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2016-04-12 21:01 UTC (permalink / raw) To: Eryu Guan; +Cc: Christoph Hellwig, fstests, xfs 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. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] xfs/259: handle minimum block size more precisely @ 2016-04-12 21:01 ` Dave Chinner 0 siblings, 0 replies; 16+ messages in thread From: Dave Chinner @ 2016-04-12 21:01 UTC (permalink / raw) To: Eryu Guan; +Cc: Christoph Hellwig, fstests, xfs 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. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-04-12 21:01 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-07 11:05 [PATCH] xfs/259: handle minimum block size more precisely Eryu Guan 2016-04-07 11:05 ` Eryu Guan 2016-04-07 15:46 ` Christoph Hellwig 2016-04-07 15:46 ` Christoph Hellwig 2016-04-07 21:32 ` Dave Chinner 2016-04-07 21:32 ` Dave Chinner 2016-04-07 23:48 ` Christoph Hellwig 2016-04-07 23:48 ` Christoph Hellwig 2016-04-11 0:02 ` Dave Chinner 2016-04-11 0:02 ` Dave Chinner 2016-04-11 3:12 ` Eryu Guan 2016-04-11 3:12 ` Eryu Guan 2016-04-11 11:38 ` Eryu Guan 2016-04-11 11:38 ` Eryu Guan 2016-04-12 21:01 ` Dave Chinner 2016-04-12 21:01 ` Dave Chinner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.