[PATCH] generic/466: be more precise about which block sizes to use

[PATCH] generic/466: be more precise about which block sizes to use

[Theodore Ts'o]

The test previously blindly tried block sizes from 512 to 65536 and
relied on mkfs to fail.  This is problematic for a number of file
systems.  For example, when testing btrfs, _scratch_mkfs_sized simply
ignores the blocksize, so the test will pointlessly run the same test
multiple times.  For ext4, when encryption is enabled, the only block
size which is supported is the page size.

So define two new functions _fs_min_blocksize and _fs_max_blocksize,
and use it so that generic/466 will only try using the block sizes
that will work.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 common/rc         | 23 +++++++++++++++++++++++
 tests/generic/466 |  6 +++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 59d4b961..7aa747ad 100644
--- a/common/rc
+++ b/common/rc
@@ -1067,6 +1067,29 @@ _scratch_mkfs_blocksized()
     esac
 }
 
+_fs_min_blocksize()
+{
+    case $FSTYP in
+    ext4)
+	if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
+	    get_page_size
+	else
+            echo 1024
+	fi
+       ;;
+    btrfs)
+	get_page_size
+	;;
+    *)
+	echo 512
+    esac
+}
+
+_fs_max_blocksize()
+{
+    get_page_size
+}
+
 _scratch_resvblks()
 {
 	case $FSTYP in
diff --git a/tests/generic/466 b/tests/generic/466
index 07f24a74..a1d944d3 100755
--- a/tests/generic/466
+++ b/tests/generic/466
@@ -54,7 +54,11 @@ unset MKFS_OPTIONS
 
 echo "Starting test" > $seqres.full
 devsize=$(blockdev --getsize64 $SCRATCH_DEV)
-for blocksize in 512 1024 2048 4096 8192 16384 32768 65536; do
+min_blocksize=$(_fs_min_blocksize)
+max_blocksize=$(_fs_max_blocksize)
+
+for (( blocksize = min_blocksize ; blocksize <= max_blocksize ;
+       blocksize = blocksize * 2)); do
 	echo "+ Format blocksize $blocksize and mount" >> $seqres.full
 	_scratch_unmount > /dev/null 2>&1
 	# Try to format and mount with the given blocksize.  If they don't
-- 
2.11.0.rc0.7.gbe5a750

Re: [PATCH] generic/466: be more precise about which block sizes to use

[Darrick J. Wong]

On Mon, Dec 11, 2017 at 12:27:42PM -0500, Theodore Ts'o wrote:
> The test previously blindly tried block sizes from 512 to 65536 and
> relied on mkfs to fail.  This is problematic for a number of file
> systems.  For example, when testing btrfs, _scratch_mkfs_sized simply
> ignores the blocksize, so the test will pointlessly run the same test
> multiple times.  For ext4, when encryption is enabled, the only block
> size which is supported is the page size.
> 
> So define two new functions _fs_min_blocksize and _fs_max_blocksize,
> and use it so that generic/466 will only try using the block sizes
> that will work.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  common/rc         | 23 +++++++++++++++++++++++
>  tests/generic/466 |  6 +++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 59d4b961..7aa747ad 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1067,6 +1067,29 @@ _scratch_mkfs_blocksized()
>      esac
>  }
>  
> +_fs_min_blocksize()
> +{
> +    case $FSTYP in
> +    ext4)
> +	if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
> +	    get_page_size
> +	else
> +            echo 1024
> +	fi
> +       ;;
> +    btrfs)
> +	get_page_size
> +	;;
> +    *)
> +	echo 512

FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't
really tell until you run mkfs.xfs -N.

> +    esac
> +}
> +
> +_fs_max_blocksize()
> +{
> +    get_page_size

Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs.

--D

> +}
> +
>  _scratch_resvblks()
>  {
>  	case $FSTYP in
> diff --git a/tests/generic/466 b/tests/generic/466
> index 07f24a74..a1d944d3 100755
> --- a/tests/generic/466
> +++ b/tests/generic/466
> @@ -54,7 +54,11 @@ unset MKFS_OPTIONS
>  
>  echo "Starting test" > $seqres.full
>  devsize=$(blockdev --getsize64 $SCRATCH_DEV)
> -for blocksize in 512 1024 2048 4096 8192 16384 32768 65536; do
> +min_blocksize=$(_fs_min_blocksize)
> +max_blocksize=$(_fs_max_blocksize)
> +
> +for (( blocksize = min_blocksize ; blocksize <= max_blocksize ;
> +       blocksize = blocksize * 2)); do
>  	echo "+ Format blocksize $blocksize and mount" >> $seqres.full
>  	_scratch_unmount > /dev/null 2>&1
>  	# Try to format and mount with the given blocksize.  If they don't
> -- 
> 2.11.0.rc0.7.gbe5a750
> 
> --
> 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

Re: [PATCH] generic/466: be more precise about which block sizes to use

[Theodore Ts'o]

On Wed, Dec 13, 2017 at 09:36:29AM -0800, Darrick J. Wong wrote:
> > +	get_page_size
> > +	;;
> > +    *)
> > +	echo 512
> 
> FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't
> really tell until you run mkfs.xfs -N.

Ok, but I assume we should keep this at 512 since it could be a v4
file systems that are being tested?

> > +_fs_max_blocksize()
> > +{
> > +    get_page_size
> 
> Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs.

Really?  Does mmap work on a fuse2fs-mounted 64k-block ext4 file system?

I suppose can just force the block size to be 64k for fuse2fs,
although I don't think the using some file system like ext4 for
fuse2fs testing isn't going to work right now anyway, yes?

	      	   	     		     	       - Ted

Re: [PATCH] generic/466: be more precise about which block sizes to use

[Darrick J. Wong]

On Sun, Dec 31, 2017 at 01:45:08PM -0500, Theodore Ts'o wrote:
> On Wed, Dec 13, 2017 at 09:36:29AM -0800, Darrick J. Wong wrote:
> > > +	get_page_size
> > > +	;;
> > > +    *)
> > > +	echo 512
> > 
> > FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't
> > really tell until you run mkfs.xfs -N.
> 
> Ok, but I assume we should keep this at 512 since it could be a v4
> file systems that are being tested?

That depends on whether this function returns the smallest blocksize
guaranteed to pass mkfs given the current set of options or the
theoretical smallest blocksize supported by that fs given the right set
of options.  IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b
size=$(_fs_min_blocksize) -m crc=1" will fail.

> > > +_fs_max_blocksize()
> > > +{
> > > +    get_page_size
> > 
> > Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs.
> 
> Really?  Does mmap work on a fuse2fs-mounted 64k-block ext4 file system?

Yes:

# uname -a
Linux magnolia 4.14.8-67-magnolia #2 SMP PREEMPT Fri Dec 22 17:23:52 PST 2017 x86_64 x86_64 x86_64 GNU/Linux
# truncate -s 1g /tmp/a
# mkfs.ext4 -b 65536 -F /tmp/a
...
# fuse2fs /tmp/a /mnt
/tmp/a: Writing to the journal is not supported.
# fallocate -l 128k /mnt/a
# xfs_io -c 'mmap -rw 0 128k' -c 'mwrite -S 0x58 0 6144' /mnt/a
# od -tx1 -Ad -c /mnt/a
0000000  58  58  58  58  58  58  58  58  58  58  58  58  58  58  58  58
          X   X   X   X   X   X   X   X   X   X   X   X   X   X   X   X
*
0006144  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
         \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
*
0131072

> I suppose can just force the block size to be 64k for fuse2fs,
> although I don't think the using some file system like ext4 for
> fuse2fs testing isn't going to work right now anyway, yes?

I haven't tried it in some time, but the last time I ran xfstests
against fuse2fs it more or less worked (modulo all the fancy fallocate
stuff that it doesn't support).

--D

> 
> 	      	   	     		     	       - Ted

Re: [PATCH] generic/466: be more precise about which block sizes to use

[Darrick J. Wong]

On Tue, Jan 02, 2018 at 12:36:50PM -0800, Darrick J. Wong wrote:
> On Sun, Dec 31, 2017 at 01:45:08PM -0500, Theodore Ts'o wrote:
> > On Wed, Dec 13, 2017 at 09:36:29AM -0800, Darrick J. Wong wrote:
> > > > +	get_page_size
> > > > +	;;
> > > > +    *)
> > > > +	echo 512
> > > 
> > > FWIW XFS' minimum block size is 1k for v5 filesystems, though you can't
> > > really tell until you run mkfs.xfs -N.
> > 
> > Ok, but I assume we should keep this at 512 since it could be a v4
> > file systems that are being tested?
> 
> That depends on whether this function returns the smallest blocksize
> guaranteed to pass mkfs given the current set of options or the
> theoretical smallest blocksize supported by that fs given the right set
> of options.  IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b
> size=$(_fs_min_blocksize) -m crc=1" will fail.

...but we /could/ just try the known minimums with "_scratch_mkfs -b
size=XXX -N" because (so long as it doesn't get smart and try to
override our -b setting) it'll fail on any unsupported size.

> > > > +_fs_max_blocksize()
> > > > +{
> > > > +    get_page_size
> > > 
> > > Also, one can run xfstests against a fuse2fs-mounted 64k-block ext4 fs.
> > 
> > Really?  Does mmap work on a fuse2fs-mounted 64k-block ext4 file system?
> 
> Yes:
> 
> # uname -a
> Linux magnolia 4.14.8-67-magnolia #2 SMP PREEMPT Fri Dec 22 17:23:52 PST 2017 x86_64 x86_64 x86_64 GNU/Linux
> # truncate -s 1g /tmp/a
> # mkfs.ext4 -b 65536 -F /tmp/a
> ...
> # fuse2fs /tmp/a /mnt
> /tmp/a: Writing to the journal is not supported.
> # fallocate -l 128k /mnt/a
> # xfs_io -c 'mmap -rw 0 128k' -c 'mwrite -S 0x58 0 6144' /mnt/a
> # od -tx1 -Ad -c /mnt/a
> 0000000  58  58  58  58  58  58  58  58  58  58  58  58  58  58  58  58
>           X   X   X   X   X   X   X   X   X   X   X   X   X   X   X   X
> *
> 0006144  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
>          \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> *
> 0131072
> 
> > I suppose can just force the block size to be 64k for fuse2fs,
> > although I don't think the using some file system like ext4 for
> > fuse2fs testing isn't going to work right now anyway, yes?
> 
> I haven't tried it in some time, but the last time I ran xfstests
> against fuse2fs it more or less worked (modulo all the fancy fallocate
> stuff that it doesn't support).

(Maybe we should separate out ext4/fuse2fs?)

--D

> --D
> 
> > 
> > 	      	   	     		     	       - Ted
> --
> 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

Re: [PATCH] generic/466: be more precise about which block sizes to use

[Theodore Ts'o]

> > That depends on whether this function returns the smallest blocksize
> > guaranteed to pass mkfs given the current set of options or the
> > theoretical smallest blocksize supported by that fs given the right set
> > of options.  IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b
> > size=$(_fs_min_blocksize) -m crc=1" will fail.

Some of the discussion that we had on our weekly ext4 developer's
conference call.  One of the motivations for this patch is due to a
local change that I have in my xfstests tree, which adds:

	|| _die "${MKFS_PROG}.$FSTYP failed!"

to the ${MKFS_PROG}.$FSTYP call in _scratch_mkfs_sized and similar
functions.  The reason for that is there a number of tests which use
additional file system options, different block sizes, unusually small
file system sizes, etc. --- and 99.97% of them don't bother to do any
error checking.

So what ends up happening is the test will not notice that the mkfs
had failed, and would continue staggering on, sometimes succeeding,
and sometimes failing --- depending on what test was running
previously and the state of the scratch device.  Ugh.  Hence, my
addition of the "|| die ..."

The problem is that generic/466 is one of the few tests that actually
(a) deliberately feed arguments that cause _scratch_mkfs_sized to
fail, and (b) actually does error checking.

Darrick mentioned on the call that he's seen similar problems which
he's had to track down when testing xfs, and it was simiarly annoying.

So instead of it being a local hack in my xfstests tree, perhaps this
is something we should try to fix more generally.  There a number of I
could imagine going about this.

1) The approach I took in this patch, which is requires accurately
determine min and max blocksizes --- and then to add error checks to
all of the mkfs invocations in _scratch_mkfs_blocksized for all file
systems.

2) Add an argument to _scratch_mkfs_blockized() and similar functions
which indicates that error checking should _not_ be done, since it
will be done by the caller (such as generic/466).  And then add error
checks in _scratch_mkfs_blocksized() and similar functions unless the
optional flag is passed by the test.

3)  Sweep through all of the tests scripts adding error checks.

It seems to me (2) might be the simplest approach, although (1) does
speed up the test somewhat, and so maybe we should do (1) as well as
(2).  Eryu, do you have any thoughts/opinions?

> > I haven't tried it in some time, but the last time I ran xfstests
> > against fuse2fs it more or less worked (modulo all the fancy fallocate
> > stuff that it doesn't support).
> 
> (Maybe we should separate out ext4/fuse2fs?)

Darrick explained this require bind mounting mount.fuse2fs on
mount.ext4, and other unnatural hacks.  It seems to me that it might
be useful to add first class support for fuse2fs to xfstests, with
arguments ala overlayfs to specify what the fuse driver (such as
fuse2fs) should be used; what should be used for mkfs, etc.

Once we do this, we can use a different max blocksize for fuse2fs.

     	   	    	      		- Ted

					

Re: [PATCH] generic/466: be more precise about which block sizes to use

[Eryu Guan]

On Thu, Jan 04, 2018 at 10:28:43PM -0500, Theodore Ts'o wrote:
> > > That depends on whether this function returns the smallest blocksize
> > > guaranteed to pass mkfs given the current set of options or the
> > > theoretical smallest blocksize supported by that fs given the right set
> > > of options.  IOWs, if this is a v5 fs being tested, then "mkfs.xfs -b
> > > size=$(_fs_min_blocksize) -m crc=1" will fail.
> 
> Some of the discussion that we had on our weekly ext4 developer's
> conference call.  One of the motivations for this patch is due to a
> local change that I have in my xfstests tree, which adds:
> 
> 	|| _die "${MKFS_PROG}.$FSTYP failed!"
> 
> to the ${MKFS_PROG}.$FSTYP call in _scratch_mkfs_sized and similar
> functions.  The reason for that is there a number of tests which use
> additional file system options, different block sizes, unusually small
> file system sizes, etc. --- and 99.97% of them don't bother to do any
> error checking.

My first reaction is to fix _scratch_mkfs_sized so it could retry with
only user specified mkfs options, if the first mkfs failed due to
conflicts between MKFS_OPTIONS and addtional mkfs options specified in
the tests. (We've done similar update to _scratch_mkfs in commit
596a068bf130 ("fstests: teach _scratch_mkfs to handle mkfs option
conflicts"), and Dave just brought up that _scratch_mkfs_sized couldn't
handle conflicts in mkfs options just a few days ago.)

And we can fix the tests that fail mkfs due to small fs size, enlarge
the default fs size so mkfs succeeds even with additional mkfs options
specified.

But I'd like to know some exact example tests that fail for you, so I
can dig into them more.

> 
> So what ends up happening is the test will not notice that the mkfs
> had failed, and would continue staggering on, sometimes succeeding,
> and sometimes failing --- depending on what test was running
> previously and the state of the scratch device.  Ugh.  Hence, my
> addition of the "|| die ..."

Yeah, this sometimes does happen. Historically we kind prefer letting
the tests continue even if mkfs and/or mount failed, so the tests could
run in some uncommon setups and exercise some corner cases (this did
help find bugs that are hard to hit before), and in most cases tests
would fail in the end so we noticed such failures.

But we would check mkfs and/or mount failure explicitly if the test will
do destructive operations on scratch dev, e.g. freeze $SCRATCH_DEV or
fulfill $SCRATCH_MNT, so tests won't destroy the underlying filesystem
(usually root fs).

Thanks,
Eryu

> 
> The problem is that generic/466 is one of the few tests that actually
> (a) deliberately feed arguments that cause _scratch_mkfs_sized to
> fail, and (b) actually does error checking.
> 
> Darrick mentioned on the call that he's seen similar problems which
> he's had to track down when testing xfs, and it was simiarly annoying.
> 
> So instead of it being a local hack in my xfstests tree, perhaps this
> is something we should try to fix more generally.  There a number of I
> could imagine going about this.
> 
> 1) The approach I took in this patch, which is requires accurately
> determine min and max blocksizes --- and then to add error checks to
> all of the mkfs invocations in _scratch_mkfs_blocksized for all file
> systems.
> 
> 2) Add an argument to _scratch_mkfs_blockized() and similar functions
> which indicates that error checking should _not_ be done, since it
> will be done by the caller (such as generic/466).  And then add error
> checks in _scratch_mkfs_blocksized() and similar functions unless the
> optional flag is passed by the test.
> 
> 3)  Sweep through all of the tests scripts adding error checks.
> 
> It seems to me (2) might be the simplest approach, although (1) does
> speed up the test somewhat, and so maybe we should do (1) as well as
> (2).  Eryu, do you have any thoughts/opinions?
> 
> > > I haven't tried it in some time, but the last time I ran xfstests
> > > against fuse2fs it more or less worked (modulo all the fancy fallocate
> > > stuff that it doesn't support).
> > 
> > (Maybe we should separate out ext4/fuse2fs?)
> 
> Darrick explained this require bind mounting mount.fuse2fs on
> mount.ext4, and other unnatural hacks.  It seems to me that it might
> be useful to add first class support for fuse2fs to xfstests, with
> arguments ala overlayfs to specify what the fuse driver (such as
> fuse2fs) should be used; what should be used for mkfs, etc.
> 
> Once we do this, we can use a different max blocksize for fuse2fs.
> 
>      	   	    	      		- Ted
> 
> 					
> --
> 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