[PATCH 1/2] xfstests: Introduce get_block_size() helper

[PATCH 1/2] xfstests: Introduce get_block_size() helper

[Lukas Czerner]

Currently many tests and other functions uses it's own way to get block
size of the file system. Introduce get_block_size(), a generic way to
get block size of mounted file system and use that instead.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 common/attr       | 4 ++--
 common/punch      | 2 +-
 common/rc         | 9 +++++++++
 tests/generic/240 | 2 +-
 tests/generic/256 | 2 +-
 tests/generic/308 | 2 +-
 tests/shared/298  | 2 +-
 7 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/common/attr b/common/attr
index 42a1a16..4c70c9e 100644
--- a/common/attr
+++ b/common/attr
@@ -251,7 +251,7 @@ _sort_getfattr_output()
 if [ "$FSTYP" == "xfs" -o "$FSTYP" == "udf" ]; then
 	MAX_ATTRS=1000
 else # Assume max ~1 block of attrs
-	BLOCK_SIZE=`stat -f $TEST_DIR | grep "Block size" | cut -d " " -f3`
+	BLOCK_SIZE=`get_block_size $TEST_DIR`
 	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
 	let MAX_ATTRS=$BLOCK_SIZE/40
 fi
@@ -262,7 +262,7 @@ export MAX_ATTRS
 if [ "$FSTYP" == "xfs" -o "$FSTYP" == "udf" -o "$FSTYP" == "btrfs" ]; then
 	MAX_ATTRVAL_SIZE=64
 else # Assume max ~1 block of attrs
-	BLOCK_SIZE=`stat -f $TEST_DIR | grep "Block size" | cut -d " " -f3`
+	BLOCK_SIZE=`get_block_size $TEST_DIR`
 	# leave a little overhead
 	let MAX_ATTRVAL_SIZE=$BLOCK_SIZE-256
 fi
diff --git a/common/punch b/common/punch
index f2d538c..237b4d8 100644
--- a/common/punch
+++ b/common/punch
@@ -557,7 +557,7 @@ _test_generic_punch()
 	if [ "$remove_testfile" ]; then
 		rm -f $testfile
 	fi
-	block_size=`stat -f $TEST_DIR | grep "Block size" | cut -d " " -f3`
+	block_size=`get_block_size $TEST_DIR`
 	$XFS_IO_PROG -f -c "truncate $block_size" \
 		-c "pwrite 0 $block_size" $sync_cmd \
 		-c "$zero_cmd 128 128" \
diff --git a/common/rc b/common/rc
index 2c83340..95030ae 100644
--- a/common/rc
+++ b/common/rc
@@ -2281,6 +2281,15 @@ _short_dev()
 	echo `basename $(_real_dev $1)`
 }
 
+get_block_size()
+{
+	if [ -z $1 ] || [ ! -d $1 ]; then
+		echo "Missing mount point argument for get_block_size"
+		exit 1
+	fi
+	echo `stat -f -c %S $1`
+}
+
 init_rc
 
 ################################################################################
diff --git a/tests/generic/240 b/tests/generic/240
index 74dbba6..44ba0d3 100755
--- a/tests/generic/240
+++ b/tests/generic/240
@@ -61,7 +61,7 @@ rm -f $seqres.full
 rm -f $TEST_DIR/aiodio_sparse
 
 logical_block_size=`_min_dio_alignment $TEST_DEV`
-fs_block_size=`stat -f $TEST_DIR | grep "Block size:" | awk '{print $3}'`
+fs_block_size=`get_block_size $TEST_DIR`
 file_size=$((8 * $fs_block_size))
 
 if [ $fs_block_size -le $logical_block_size ]; then
diff --git a/tests/generic/256 b/tests/generic/256
index e6cc7dc..92c4f30 100755
--- a/tests/generic/256
+++ b/tests/generic/256
@@ -170,7 +170,7 @@ _scratch_mount
 # Test must be able to write files with non-root permissions
 chmod 777 $SCRATCH_MNT
 
-block_size=`stat -f $SCRATCH_DEV | grep "Block size" | cut -d " " -f3`
+block_size=`get_block_size $SCRATCH_MNT`
 _test_full_fs_punch $(( $block_size * 2 )) $block_size 500 $SCRATCH_MNT/252.$$ $block_size
 
 status=0 ; exit
diff --git a/tests/generic/308 b/tests/generic/308
index 8037c08..5646486 100755
--- a/tests/generic/308
+++ b/tests/generic/308
@@ -48,7 +48,7 @@ _supported_os Linux
 rm -f $seqres.full
 echo "Silence is golden"
 
-block_size=`stat -f -c %s $TEST_DIR`
+block_size=`get_block_size $TEST_DIR`
 
 # On unpatched ext4, if an extent exists which includes the block right
 # before the maximum file offset, and the block for the maximum file offset
diff --git a/tests/shared/298 b/tests/shared/298
index 8211da3..c1a6316 100755
--- a/tests/shared/298
+++ b/tests/shared/298
@@ -164,7 +164,7 @@ echo "done."
 
 echo -n "Comparing holes to the reported space from FS..."
 # Get block size
-block_size=$(stat -f -c "%S" $loop_mnt/)
+block_size=$(get_block_size $loop_mnt/)
 sectors_per_block=`expr $block_size / 512`
 
 # Obtain free space from filesystem
-- 
1.8.3.1

[PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Lukas Czerner]

User takes care about specifying mkfs options he wishes to test and the
test itself should not change it if it's not strictly necessary for the
test itself.

In this case it is not necessary and we should only test configuration
provided by the user. Moreover if the block size was already specified
some mkfs utilities does not handle multiple of the same parameters and
the mkfs utility fails making it re-try with only provided options
(ignoring what user specified), which is wrong.

In this case it's also a problem for btrfs file system which does not
support block size < page size.

Fix it by removing the mkfs, and testing existing configuration only.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 tests/generic/017     | 46 +++++++++++++++++-----------------------------
 tests/generic/017.out |  2 --
 2 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/tests/generic/017 b/tests/generic/017
index 13b7254..11705bf 100755
--- a/tests/generic/017
+++ b/tests/generic/017
@@ -49,41 +49,29 @@ _do_die_on_error=y
 testfile=$SCRATCH_MNT/$seq.$$
 BLOCKS=10240
 
-for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
+BSIZE=`get_block_size $SCRATCH_MNT`
 
-	length=$(($BLOCKS * $BSIZE))
-	case $FSTYP in
-	xfs)
-	_scratch_mkfs -b size=$BSIZE >> $seqres.full 2>&1
-	;;
-	ext4)
-	_scratch_mkfs -b $BSIZE >> $seqres.full 2>&1
-	;;
-	esac
-	_scratch_mount >> $seqres.full 2>&1
+length=$(($BLOCKS * $BSIZE))
 
-	# Write file
-	$XFS_IO_PROG -f -c "pwrite 0 $length" -c fsync $testfile > /dev/null
+# Write file
+$XFS_IO_PROG -f -c "pwrite 0 $length" -c fsync $testfile > /dev/null
 
-	# Collapse alternate blocks 
-	for (( i = 1; i <= 7; i++ )); do
-		for (( j=0; j < $(($BLOCKS/(2**$i))); j++ )); do
-			offset=$(($j*$BSIZE))
-			$XFS_IO_PROG -c "fcollapse $offset $BSIZE" $testfile > /dev/null
-		done
+# Collapse alternate blocks 
+for (( i = 1; i <= 7; i++ )); do
+	for (( j=0; j < $(($BLOCKS/(2**$i))); j++ )); do
+		offset=$(($j*$BSIZE))
+		$XFS_IO_PROG -c "fcollapse $offset $BSIZE" $testfile > /dev/null
 	done
+done
 
-	# Check if 80 extents are present
-	$XFS_IO_PROG -c "fiemap -v" $testfile | grep "^ *[0-9]*:" |wc -l
-
-	_check_scratch_fs
-	if [ $? -ne 0 ]; then
-		status=1
-		exit
-	fi
+# Check if 80 extents are present
+$XFS_IO_PROG -c "fiemap -v" $testfile | grep "^ *[0-9]*:" |wc -l
 
-	umount $SCRATCH_MNT
-done
+_check_scratch_fs
+if [ $? -ne 0 ]; then
+	status=1
+	exit
+fi
 
 # success, all done
 status=0
diff --git a/tests/generic/017.out b/tests/generic/017.out
index cc524ac..ea9a7a8 100644
--- a/tests/generic/017.out
+++ b/tests/generic/017.out
@@ -1,4 +1,2 @@
 QA output created by 017
 80
-80
-80
-- 
1.8.3.1

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Filipe David Manana]

On Mon, Jun 23, 2014 at 3:54 PM, Lukas Czerner <lczerner@redhat.com> wrote:
> User takes care about specifying mkfs options he wishes to test and the
> test itself should not change it if it's not strictly necessary for the
> test itself.
>
> In this case it is not necessary and we should only test configuration
> provided by the user. Moreover if the block size was already specified
> some mkfs utilities does not handle multiple of the same parameters and
> the mkfs utility fails making it re-try with only provided options
> (ignoring what user specified), which is wrong.
>
> In this case it's also a problem for btrfs file system which does not
> support block size < page size.
>
> Fix it by removing the mkfs, and testing existing configuration only.
>
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Tested-by: Filipe Manana <fdmanana@gmail.com>

Works for btrfs. So please ignore the workaround patch I sent earlier today.
Thanks Lukas

> ---
>  tests/generic/017     | 46 +++++++++++++++++-----------------------------
>  tests/generic/017.out |  2 --
>  2 files changed, 17 insertions(+), 31 deletions(-)
>
> diff --git a/tests/generic/017 b/tests/generic/017
> index 13b7254..11705bf 100755
> --- a/tests/generic/017
> +++ b/tests/generic/017
> @@ -49,41 +49,29 @@ _do_die_on_error=y
>  testfile=$SCRATCH_MNT/$seq.$$
>  BLOCKS=10240
>
> -for (( BSIZE = 1024; BSIZE <= 4096; BSIZE *= 2 )); do
> +BSIZE=`get_block_size $SCRATCH_MNT`
>
> -       length=$(($BLOCKS * $BSIZE))
> -       case $FSTYP in
> -       xfs)
> -       _scratch_mkfs -b size=$BSIZE >> $seqres.full 2>&1
> -       ;;
> -       ext4)
> -       _scratch_mkfs -b $BSIZE >> $seqres.full 2>&1
> -       ;;
> -       esac
> -       _scratch_mount >> $seqres.full 2>&1
> +length=$(($BLOCKS * $BSIZE))
>
> -       # Write file
> -       $XFS_IO_PROG -f -c "pwrite 0 $length" -c fsync $testfile > /dev/null
> +# Write file
> +$XFS_IO_PROG -f -c "pwrite 0 $length" -c fsync $testfile > /dev/null
>
> -       # Collapse alternate blocks
> -       for (( i = 1; i <= 7; i++ )); do
> -               for (( j=0; j < $(($BLOCKS/(2**$i))); j++ )); do
> -                       offset=$(($j*$BSIZE))
> -                       $XFS_IO_PROG -c "fcollapse $offset $BSIZE" $testfile > /dev/null
> -               done
> +# Collapse alternate blocks
> +for (( i = 1; i <= 7; i++ )); do
> +       for (( j=0; j < $(($BLOCKS/(2**$i))); j++ )); do
> +               offset=$(($j*$BSIZE))
> +               $XFS_IO_PROG -c "fcollapse $offset $BSIZE" $testfile > /dev/null
>         done
> +done
>
> -       # Check if 80 extents are present
> -       $XFS_IO_PROG -c "fiemap -v" $testfile | grep "^ *[0-9]*:" |wc -l
> -
> -       _check_scratch_fs
> -       if [ $? -ne 0 ]; then
> -               status=1
> -               exit
> -       fi
> +# Check if 80 extents are present
> +$XFS_IO_PROG -c "fiemap -v" $testfile | grep "^ *[0-9]*:" |wc -l
>
> -       umount $SCRATCH_MNT
> -done
> +_check_scratch_fs
> +if [ $? -ne 0 ]; then
> +       status=1
> +       exit
> +fi
>
>  # success, all done
>  status=0
> diff --git a/tests/generic/017.out b/tests/generic/017.out
> index cc524ac..ea9a7a8 100644
> --- a/tests/generic/017.out
> +++ b/tests/generic/017.out
> @@ -1,4 +1,2 @@
>  QA output created by 017
>  80
> -80
> -80
> --
> 1.8.3.1
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Dave Chinner]

On Mon, Jun 23, 2014 at 04:54:15PM +0200, Lukas Czerner wrote:
> User takes care about specifying mkfs options he wishes to test and the
> test itself should not change it if it's not strictly necessary for the
> test itself.
> 
> In this case it is not necessary and we should only test configuration
> provided by the user. Moreover if the block size was already specified
> some mkfs utilities does not handle multiple of the same parameters and
> the mkfs utility fails making it re-try with only provided options
> (ignoring what user specified), which is wrong.

I disagree strongly with this justification. The test is perfectly
fine: it is allowed to do whatever it wants with mkfs and mount
options.

It is up to the implementations of the specific FSTYP mkfs
implementation called from _scratch_mkfs to handle
conflicting/unsupported options sanely, not the test....

> In this case it's also a problem for btrfs file system which does not
> support block size < page size.

i.e. _scratch_mkfs_btrfs needs to either filter that out or
_fail/_not_run the test if it can't....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Lukáš Czerner]

On Tue, 24 Jun 2014, Dave Chinner wrote:

> Date: Tue, 24 Jun 2014 14:38:35 +1000
> From: Dave Chinner <david@fromorbit.com>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: fstests@vger.kernel.org, fdmanana@gmail.com
> Subject: Re: [PATCH 2/2] generic/017: Do not create file systems with
>     different block sizes
> 
> On Mon, Jun 23, 2014 at 04:54:15PM +0200, Lukas Czerner wrote:
> > User takes care about specifying mkfs options he wishes to test and the
> > test itself should not change it if it's not strictly necessary for the
> > test itself.
> > 
> > In this case it is not necessary and we should only test configuration
> > provided by the user. Moreover if the block size was already specified
> > some mkfs utilities does not handle multiple of the same parameters and
> > the mkfs utility fails making it re-try with only provided options
> > (ignoring what user specified), which is wrong.
> 
> I disagree strongly with this justification. The test is perfectly
> fine: it is allowed to do whatever it wants with mkfs and mount
> options.
> 
> It is up to the implementations of the specific FSTYP mkfs
> implementation called from _scratch_mkfs to handle
> conflicting/unsupported options sanely, not the test....

I agree, the test should not care about those options at all. That's
why I removed it. What was done in this test was entirely
unnecessary and was not done in any other similar test before.

Yes, we can test all block size in every test from now on, but
what's the point ? We have a config file and the user can set
whatever block size he wants to test. I do not dispute that there is
a time when we really want to set a specific mkfs options in the
test, but this is not the case at all.

Moreover, if I want to test specific mkfs configuration the current
approach might not work as it will replace the options.

-Lukas

> 
> > In this case it's also a problem for btrfs file system which does not
> > support block size < page size.
> 
> i.e. _scratch_mkfs_btrfs needs to either filter that out or
> _fail/_not_run the test if it can't....
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Dave Chinner]

On Tue, Jun 24, 2014 at 06:50:15AM +0200, Lukáš Czerner wrote:
> On Tue, 24 Jun 2014, Dave Chinner wrote:
> 
> > Date: Tue, 24 Jun 2014 14:38:35 +1000
> > From: Dave Chinner <david@fromorbit.com>
> > To: Lukas Czerner <lczerner@redhat.com>
> > Cc: fstests@vger.kernel.org, fdmanana@gmail.com
> > Subject: Re: [PATCH 2/2] generic/017: Do not create file systems with
> >     different block sizes
> > 
> > On Mon, Jun 23, 2014 at 04:54:15PM +0200, Lukas Czerner wrote:
> > > User takes care about specifying mkfs options he wishes to test and the
> > > test itself should not change it if it's not strictly necessary for the
> > > test itself.
> > > 
> > > In this case it is not necessary and we should only test configuration
> > > provided by the user. Moreover if the block size was already specified
> > > some mkfs utilities does not handle multiple of the same parameters and
> > > the mkfs utility fails making it re-try with only provided options
> > > (ignoring what user specified), which is wrong.
> > 
> > I disagree strongly with this justification. The test is perfectly
> > fine: it is allowed to do whatever it wants with mkfs and mount
> > options.
> > 
> > It is up to the implementations of the specific FSTYP mkfs
> > implementation called from _scratch_mkfs to handle
> > conflicting/unsupported options sanely, not the test....
> 
> I agree, the test should not care about those options at all.

That's not what I said. I said the test is perfectly entitled
to test whatever options it wants to.

The user specified configs are *behavioural modifiers*, not a
draconian "this is all we test" rule. 

> That's
> why I removed it. What was done in this test was entirely
> unnecessary and was not done in any other similar test before.

Not true.  There are tests that override user specified configs all
over the place. There's at least 50 calls to _scratch_mkfs_xfs with
specific options across all the XFS tests, some of which
specifically override block size so they can test sub-page block
size behaviour.

Indeed, there's a bunch of specific options passed into
_scratch_mkfs in the btrfs tests, too, so this sort of constrained
test configuration behaviour is not limited to just XFS.

IOWs, what needs to happen here is that btrfs needs to grow an
equivalent _scratch_mkfs_btrfs that drops conflicting options and
filters unsupported options so that tests don't need to care about
what a filesystem supports or doesn't support....

> Yes, we can test all block size in every test from now on, but
> what's the point?  We have a config file and the user can set
> whatever block size he wants to test. I do not dispute that there
> is a time when we really want to set a specific mkfs options in
> the test, but this is not the case at all.

Most people don't test multiple different block sizes, so unless
there is excessive runtime saying "but you can specify it manually"
is not a valid reason for preventing a single test from iterating
over multiple mkfs and/or mount configurations.

> Moreover, if I want to test specific mkfs configuration the current
> approach might not work as it will replace the options.

Only if they conflict. And in that case, it's better to simply run
the test the way it was intended to be run than it is to fail or
not_run it...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4490 bytes --]

On Tue, 24 Jun 2014, Dave Chinner wrote:

> Date: Tue, 24 Jun 2014 15:48:54 +1000
> From: Dave Chinner <david@fromorbit.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: fstests@vger.kernel.org, fdmanana@gmail.com
> Subject: Re: [PATCH 2/2] generic/017: Do not create file systems with
>     different block sizes
> 
> On Tue, Jun 24, 2014 at 06:50:15AM +0200, Lukáš Czerner wrote:
> > On Tue, 24 Jun 2014, Dave Chinner wrote:
> > 
> > > Date: Tue, 24 Jun 2014 14:38:35 +1000
> > > From: Dave Chinner <david@fromorbit.com>
> > > To: Lukas Czerner <lczerner@redhat.com>
> > > Cc: fstests@vger.kernel.org, fdmanana@gmail.com
> > > Subject: Re: [PATCH 2/2] generic/017: Do not create file systems with
> > >     different block sizes
> > > 
> > > On Mon, Jun 23, 2014 at 04:54:15PM +0200, Lukas Czerner wrote:
> > > > User takes care about specifying mkfs options he wishes to test and the
> > > > test itself should not change it if it's not strictly necessary for the
> > > > test itself.
> > > > 
> > > > In this case it is not necessary and we should only test configuration
> > > > provided by the user. Moreover if the block size was already specified
> > > > some mkfs utilities does not handle multiple of the same parameters and
> > > > the mkfs utility fails making it re-try with only provided options
> > > > (ignoring what user specified), which is wrong.
> > > 
> > > I disagree strongly with this justification. The test is perfectly
> > > fine: it is allowed to do whatever it wants with mkfs and mount
> > > options.
> > > 
> > > It is up to the implementations of the specific FSTYP mkfs
> > > implementation called from _scratch_mkfs to handle
> > > conflicting/unsupported options sanely, not the test....
> > 
> > I agree, the test should not care about those options at all.
> 
> That's not what I said. I said the test is perfectly entitled
> to test whatever options it wants to.
> 
> The user specified configs are *behavioural modifiers*, not a
> draconian "this is all we test" rule. 
> 
> > That's
> > why I removed it. What was done in this test was entirely
> > unnecessary and was not done in any other similar test before.
> 
> Not true.  There are tests that override user specified configs all
> over the place. There's at least 50 calls to _scratch_mkfs_xfs with
> specific options across all the XFS tests, some of which
> specifically override block size so they can test sub-page block
> size behaviour.

I said similar test, like other fallocate variants. Looking at the
tests there are three xfs tests that set block size and all of them
seems to have a reason to do so. None of them iterate through all
possible variants of block size.

> 
> Indeed, there's a bunch of specific options passed into
> _scratch_mkfs in the btrfs tests, too, so this sort of constrained
> test configuration behaviour is not limited to just XFS.
> 
> IOWs, what needs to happen here is that btrfs needs to grow an
> equivalent _scratch_mkfs_btrfs that drops conflicting options and
> filters unsupported options so that tests don't need to care about
> what a filesystem supports or doesn't support....
> 
> > Yes, we can test all block size in every test from now on, but
> > what's the point?  We have a config file and the user can set
> > whatever block size he wants to test. I do not dispute that there
> > is a time when we really want to set a specific mkfs options in
> > the test, but this is not the case at all.
> 
> Most people don't test multiple different block sizes, so unless
> there is excessive runtime saying "but you can specify it manually"
> is not a valid reason for preventing a single test from iterating
> over multiple mkfs and/or mount configurations.

That applies to most of the tests and most of the mkfs/mount
options. And what you're asking for is that people will start to
write tests which iterate through their favourite options within the
test itself for no real reason. The result will be that those of us
who actually run xfstests for different configuration will get much
longer runtime.

I think that xfstests should have a policy not to allow this to
happen, but you obviously think otherwise...

-Lukas

> 
> > Moreover, if I want to test specific mkfs configuration the current
> > approach might not work as it will replace the options.
> 
> Only if they conflict. And in that case, it's better to simply run
> the test the way it was intended to be run than it is to fail or
> not_run it...
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Dave Chinner]

On Tue, Jun 24, 2014 at 08:17:17AM +0200, Lukáš Czerner wrote:
> On Tue, 24 Jun 2014, Dave Chinner wrote:
> > > That's
> > > why I removed it. What was done in this test was entirely
> > > unnecessary and was not done in any other similar test before.
> > 
> > Not true.  There are tests that override user specified configs all
> > over the place. There's at least 50 calls to _scratch_mkfs_xfs with
> > specific options across all the XFS tests, some of which
> > specifically override block size so they can test sub-page block
> > size behaviour.
> 
> I said similar test, like other fallocate variants. Looking at the
> tests there are three xfs tests that set block size and all of them
> seems to have a reason to do so. None of them iterate through all
> possible variants of block size.

True, but we do iterate over other options, like all the different
quota configurations.

My point is that tests should be able to iterate if they need to
exercise boundary conditions. It doesn't matter if it is block size
or quota or btree leaf size or allocation cluster size. The point is
that we can iterate them in a single test *all the time* rather on
the rare occasion that someone runs with the specific test that
exercises the corner case in question.

> > Indeed, there's a bunch of specific options passed into
> > _scratch_mkfs in the btrfs tests, too, so this sort of constrained
> > test configuration behaviour is not limited to just XFS.
> > 
> > IOWs, what needs to happen here is that btrfs needs to grow an
> > equivalent _scratch_mkfs_btrfs that drops conflicting options and
> > filters unsupported options so that tests don't need to care about
> > what a filesystem supports or doesn't support....
> > 
> > > Yes, we can test all block size in every test from now on, but
> > > what's the point?  We have a config file and the user can set
> > > whatever block size he wants to test. I do not dispute that there
> > > is a time when we really want to set a specific mkfs options in
> > > the test, but this is not the case at all.
> > 
> > Most people don't test multiple different block sizes, so unless
> > there is excessive runtime saying "but you can specify it manually"
> > is not a valid reason for preventing a single test from iterating
> > over multiple mkfs and/or mount configurations.
> 
> That applies to most of the tests and most of the mkfs/mount
> options. And what you're asking for is that people will start to
> write tests which iterate through their favourite options within the
> test itself for no real reason.

No, I'm not advocating that at all. If the test has a specific
reason for overriding the user configuration, then it should.
Some configurations are rarely tested, and so having some tests that
exercise them even when other options are being tested is not a bad
thing. We catch problem with new changes much faster that way.

IOWs, if there is a need to exercise configuration corner cases,
then it can be done within a single test. If it's not testing such
corner cases, then iteration is not necessary. And for this test,
it's exercising sub-page block size behaviour, and so iteration over
different block sizes is just fine....

> The result will be that those of us
> who actually run xfstests for different configuration will get much
> longer runtime.

Not if we review tests appropriately. ;)

Are you having problems with the runtime of this test?

> I think that xfstests should have a policy not to allow this to
> happen, but you obviously think otherwise...

... because the long standing policy has been "tests can force
whatever configuration they need to exercise the functionality in
question". You're advocating that we don't allow tests to do this,
i.e so the only time sub-page block size functionality is tested is
when the entire test suite is run with that specific configuration.

The idea of a regression test suite is to exercise as much
functionality as it can as quickly as it can. To adopt apolicy that
excludes entire classes of code paths unless the user specifically
configures the test suite to exercise those paths is Just Plain
Wrong...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Lukáš Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6263 bytes --]

On Tue, 24 Jun 2014, Dave Chinner wrote:

> Date: Tue, 24 Jun 2014 16:45:26 +1000
> From: Dave Chinner <david@fromorbit.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: fstests@vger.kernel.org, fdmanana@gmail.com
> Subject: Re: [PATCH 2/2] generic/017: Do not create file systems with
>     different block sizes
> 
> On Tue, Jun 24, 2014 at 08:17:17AM +0200, Lukáš Czerner wrote:
> > On Tue, 24 Jun 2014, Dave Chinner wrote:
> > > > That's
> > > > why I removed it. What was done in this test was entirely
> > > > unnecessary and was not done in any other similar test before.
> > > 
> > > Not true.  There are tests that override user specified configs all
> > > over the place. There's at least 50 calls to _scratch_mkfs_xfs with
> > > specific options across all the XFS tests, some of which
> > > specifically override block size so they can test sub-page block
> > > size behaviour.
> > 
> > I said similar test, like other fallocate variants. Looking at the
> > tests there are three xfs tests that set block size and all of them
> > seems to have a reason to do so. None of them iterate through all
> > possible variants of block size.
> 
> True, but we do iterate over other options, like all the different
> quota configurations.
> 
> My point is that tests should be able to iterate if they need to
> exercise boundary conditions. It doesn't matter if it is block size
> or quota or btree leaf size or allocation cluster size. The point is
> that we can iterate them in a single test *all the time* rather on
> the rare occasion that someone runs with the specific test that
> exercises the corner case in question.
> 
> > > Indeed, there's a bunch of specific options passed into
> > > _scratch_mkfs in the btrfs tests, too, so this sort of constrained
> > > test configuration behaviour is not limited to just XFS.
> > > 
> > > IOWs, what needs to happen here is that btrfs needs to grow an
> > > equivalent _scratch_mkfs_btrfs that drops conflicting options and
> > > filters unsupported options so that tests don't need to care about
> > > what a filesystem supports or doesn't support....
> > > 
> > > > Yes, we can test all block size in every test from now on, but
> > > > what's the point?  We have a config file and the user can set
> > > > whatever block size he wants to test. I do not dispute that there
> > > > is a time when we really want to set a specific mkfs options in
> > > > the test, but this is not the case at all.
> > > 
> > > Most people don't test multiple different block sizes, so unless
> > > there is excessive runtime saying "but you can specify it manually"
> > > is not a valid reason for preventing a single test from iterating
> > > over multiple mkfs and/or mount configurations.
> > 
> > That applies to most of the tests and most of the mkfs/mount
> > options. And what you're asking for is that people will start to
> > write tests which iterate through their favourite options within the
> > test itself for no real reason.
> 
> No, I'm not advocating that at all. If the test has a specific
> reason for overriding the user configuration, then it should.

But that's my point, this particular test does not have specific
reason for it. It is mainly testing extent tree manipulation and
even though I agree that there might be bugs relating page_size >
block_size, it is no different than fsx, fsstress or any punch_hole,
zero_range, fallocate test.

It is doing:

"Test multiple fallocate collapse range calls on same file."

> Some configurations are rarely tested, and so having some tests that
> exercise them even when other options are being tested is not a bad
> thing. We catch problem with new changes much faster that way.

I agree, but block size is not one of those, if it is, well then we
have a different problem entirely.

> 
> IOWs, if there is a need to exercise configuration corner cases,
> then it can be done within a single test. If it's not testing such
> corner cases, then iteration is not necessary. And for this test,
> it's exercising sub-page block size behaviour, and so iteration over
> different block sizes is just fine....

Every test that's testing multiple block sizes is exercising
sub-page block size behaviour, that no argument. This test however,
unlike generic/022, generic/016, generic/021 and generic/012 is not
testing block size related corner case.

It's also worth noting that those tests mentioned above does not
iterate over different block sizes.

> 
> > The result will be that those of us
> > who actually run xfstests for different configuration will get much
> > longer runtime.
> 
> Not if we review tests appropriately. ;)
> 
> Are you having problems with the runtime of this test?

I do, it takes around 200s on xfs to run which is definitely one
of the longer running tests and given that I usually do run multiple
tests with different configurations (block size included), than it is a
waste.

With this patch, it's obviously third of the time.

> 
> > I think that xfstests should have a policy not to allow this to
> > happen, but you obviously think otherwise...
> 
> ... because the long standing policy has been "tests can force
> whatever configuration they need to exercise the functionality in
> question". You're advocating that we don't allow tests to do this,
> i.e so the only time sub-page block size functionality is tested is
> when the entire test suite is run with that specific configuration.

That's not what I am advocating for, I am saying that there should
be a specific reason to do this, like for example specific corner
case you mentioned or a bug which only appeared with specific
configuration, or any other valid reason. Testing sub page allocation
is useful for huge portion of our tests, but we certainly do not want
every test to do this on their own. But again there are some tests
where this might be justifiable (like those mentioned above and
those certainly run much *much* faster than 017).

-Lukas

> 
> The idea of a regression test suite is to exercise as much
> functionality as it can as quickly as it can. To adopt apolicy that
> excludes entire classes of code paths unless the user specifically
> configures the test suite to exercise those paths is Just Plain
> Wrong...
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH 2/2] generic/017: Do not create file systems with different block sizes

[Filipe David Manana]

On Tue, Jun 24, 2014 at 5:38 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Jun 23, 2014 at 04:54:15PM +0200, Lukas Czerner wrote:
>> User takes care about specifying mkfs options he wishes to test and the
>> test itself should not change it if it's not strictly necessary for the
>> test itself.
>>
>> In this case it is not necessary and we should only test configuration
>> provided by the user. Moreover if the block size was already specified
>> some mkfs utilities does not handle multiple of the same parameters and
>> the mkfs utility fails making it re-try with only provided options
>> (ignoring what user specified), which is wrong.
>
> I disagree strongly with this justification. The test is perfectly
> fine: it is allowed to do whatever it wants with mkfs and mount
> options.
>
> It is up to the implementations of the specific FSTYP mkfs
> implementation called from _scratch_mkfs to handle
> conflicting/unsupported options sanely, not the test....
>
>> In this case it's also a problem for btrfs file system which does not
>> support block size < page size.
>
> i.e. _scratch_mkfs_btrfs needs to either filter that out or
> _fail/_not_run the test if it can't....

For this specific test, having _scratch_mkfs_btrfs filtering it would
still make the test fail on btrfs, as it can pass an offset and length
that aren't multiples of the page size to fcollapse (which makes
xfs_io print an error message to stderr, -EINVAL).

Having it fail/_not_run, wouldn't allow to test for a block size of
4kb, which is the only that works for btrfs on systems with page size
of 4Kb.

How could we make it run on btrfs for the supported case?
I'm interested in being able to run this on btrfs, it's a good tester
for collapse range.

Thanks

>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] xfstests: Introduce get_block_size() helper

[Filipe David Manana]

On Mon, Jun 23, 2014 at 3:54 PM, Lukas Czerner <lczerner@redhat.com> wrote:
> Currently many tests and other functions uses it's own way to get block
> size of the file system. Introduce get_block_size(), a generic way to
> get block size of mounted file system and use that instead.
>
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Tested-by: Filipe Manana <fdmanana@gmail.com>

Works for btrfs. So please ignore the workaround patch I sent earlier today.
Thanks Lukas

> ---
>  common/attr       | 4 ++--
>  common/punch      | 2 +-
>  common/rc         | 9 +++++++++
>  tests/generic/240 | 2 +-
>  tests/generic/256 | 2 +-
>  tests/generic/308 | 2 +-
>  tests/shared/298  | 2 +-
>  7 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/common/attr b/common/attr
> index 42a1a16..4c70c9e 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -251,7 +251,7 @@ _sort_getfattr_output()
>  if [ "$FSTYP" == "xfs" -o "$FSTYP" == "udf" ]; then
>         MAX_ATTRS=1000
>  else # Assume max ~1 block of attrs
> -       BLOCK_SIZE=`stat -f $TEST_DIR | grep "Block size" | cut -d " " -f3`
> +       BLOCK_SIZE=`get_block_size $TEST_DIR`
>         # user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
>         let MAX_ATTRS=$BLOCK_SIZE/40
>  fi
> @@ -262,7 +262,7 @@ export MAX_ATTRS
>  if [ "$FSTYP" == "xfs" -o "$FSTYP" == "udf" -o "$FSTYP" == "btrfs" ]; then
>         MAX_ATTRVAL_SIZE=64
>  else # Assume max ~1 block of attrs
> -       BLOCK_SIZE=`stat -f $TEST_DIR | grep "Block size" | cut -d " " -f3`
> +       BLOCK_SIZE=`get_block_size $TEST_DIR`
>         # leave a little overhead
>         let MAX_ATTRVAL_SIZE=$BLOCK_SIZE-256
>  fi
> diff --git a/common/punch b/common/punch
> index f2d538c..237b4d8 100644
> --- a/common/punch
> +++ b/common/punch
> @@ -557,7 +557,7 @@ _test_generic_punch()
>         if [ "$remove_testfile" ]; then
>                 rm -f $testfile
>         fi
> -       block_size=`stat -f $TEST_DIR | grep "Block size" | cut -d " " -f3`
> +       block_size=`get_block_size $TEST_DIR`
>         $XFS_IO_PROG -f -c "truncate $block_size" \
>                 -c "pwrite 0 $block_size" $sync_cmd \
>                 -c "$zero_cmd 128 128" \
> diff --git a/common/rc b/common/rc
> index 2c83340..95030ae 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2281,6 +2281,15 @@ _short_dev()
>         echo `basename $(_real_dev $1)`
>  }
>
> +get_block_size()
> +{
> +       if [ -z $1 ] || [ ! -d $1 ]; then
> +               echo "Missing mount point argument for get_block_size"
> +               exit 1
> +       fi
> +       echo `stat -f -c %S $1`
> +}
> +
>  init_rc
>
>  ################################################################################
> diff --git a/tests/generic/240 b/tests/generic/240
> index 74dbba6..44ba0d3 100755
> --- a/tests/generic/240
> +++ b/tests/generic/240
> @@ -61,7 +61,7 @@ rm -f $seqres.full
>  rm -f $TEST_DIR/aiodio_sparse
>
>  logical_block_size=`_min_dio_alignment $TEST_DEV`
> -fs_block_size=`stat -f $TEST_DIR | grep "Block size:" | awk '{print $3}'`
> +fs_block_size=`get_block_size $TEST_DIR`
>  file_size=$((8 * $fs_block_size))
>
>  if [ $fs_block_size -le $logical_block_size ]; then
> diff --git a/tests/generic/256 b/tests/generic/256
> index e6cc7dc..92c4f30 100755
> --- a/tests/generic/256
> +++ b/tests/generic/256
> @@ -170,7 +170,7 @@ _scratch_mount
>  # Test must be able to write files with non-root permissions
>  chmod 777 $SCRATCH_MNT
>
> -block_size=`stat -f $SCRATCH_DEV | grep "Block size" | cut -d " " -f3`
> +block_size=`get_block_size $SCRATCH_MNT`
>  _test_full_fs_punch $(( $block_size * 2 )) $block_size 500 $SCRATCH_MNT/252.$$ $block_size
>
>  status=0 ; exit
> diff --git a/tests/generic/308 b/tests/generic/308
> index 8037c08..5646486 100755
> --- a/tests/generic/308
> +++ b/tests/generic/308
> @@ -48,7 +48,7 @@ _supported_os Linux
>  rm -f $seqres.full
>  echo "Silence is golden"
>
> -block_size=`stat -f -c %s $TEST_DIR`
> +block_size=`get_block_size $TEST_DIR`
>
>  # On unpatched ext4, if an extent exists which includes the block right
>  # before the maximum file offset, and the block for the maximum file offset
> diff --git a/tests/shared/298 b/tests/shared/298
> index 8211da3..c1a6316 100755
> --- a/tests/shared/298
> +++ b/tests/shared/298
> @@ -164,7 +164,7 @@ echo "done."
>
>  echo -n "Comparing holes to the reported space from FS..."
>  # Get block size
> -block_size=$(stat -f -c "%S" $loop_mnt/)
> +block_size=$(get_block_size $loop_mnt/)
>  sectors_per_block=`expr $block_size / 512`
>
>  # Obtain free space from filesystem
> --
> 1.8.3.1
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] xfstests: Introduce get_block_size() helper

[Dave Chinner]

On Mon, Jun 23, 2014 at 04:54:14PM +0200, Lukas Czerner wrote:
> Currently many tests and other functions uses it's own way to get block
> size of the file system. Introduce get_block_size(), a generic way to
> get block size of mounted file system and use that instead.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Common helper, needs to be preceeded by a "_". i.e.  _get_block_size()

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com