[PATCH] xfs/259: handle minimum block size more precisely

[PATCH] xfs/259: handle minimum block size more precisely

[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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs/259: handle minimum block size more precisely

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs/259: handle minimum block size more precisely

[Dave Chinner]

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

Re: [PATCH] xfs/259: handle minimum block size more precisely

[Christoph Hellwig]

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

Re: [PATCH] xfs/259: handle minimum block size more precisely

[Dave Chinner]

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

Re: [PATCH] xfs/259: handle minimum block size more precisely

[Eryu Guan]

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

Re: [PATCH] xfs/259: handle minimum block size more precisely

[Eryu Guan]

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

Re: [PATCH] xfs/259: handle minimum block size more precisely

[Dave Chinner]

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