[PATCH] common: rework _require_ext4_mkfs_feature

[PATCH] common: rework _require_ext4_mkfs_feature

[Theodore Ts'o]

In all of the places where we need check to see if mkfs.ext4 can
support a set of file system features, we also should be checking to
see if the kernel can support those file system features.  So rename
_require_ext4_mkfs_feature to _require_ext4_feature, and actually
format the file system in $SCRATCH.  To avoid running mkfs twice in
most tests, we will teach the tests to assume that
_require_ext4_feature actually leaves $SCRATCH formatted with a file
system with those features.

Also remove a completely unneeded _require_ext4_mkfs_feature "64bit"
from ext4/306.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---

This is a replacement for "ext4/026: skip test if kernel does not
support the ea_inode feature"

 common/rc      | 28 ++++++++++------------------
 tests/ext4/003 |  3 +--
 tests/ext4/025 |  3 +--
 tests/ext4/026 |  3 +--
 tests/ext4/306 |  1 -
 5 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/common/rc b/common/rc
index da6213a0..3febf16f 100644
--- a/common/rc
+++ b/common/rc
@@ -1899,31 +1899,23 @@ _require_scratch_ext4_crc()
 	_scratch_unmount
 }
 
-# Check the specified feature whether it is available in mkfs.ext4 or not.
-_require_ext4_mkfs_feature()
+# Check whether the specified feature whether it is supported by
+# mkfs.ext4 and the kernel.
+_require_ext4_feature()
 {
-	local feature=$1
-	local testfile=/tmp/$$.ext4_mkfs
+	local feature="$1"
+	local sz="$2"
 
 	if [ -z "$feature" ]; then
-                echo "Usage: _require_ext4_mkfs_feature feature"
+                echo "Usage: _require_ext4_feature feature"
                 exit 1
         fi
 
-	touch $testfile
-	local result=$($MKFS_EXT4_PROG -F -O $feature -n $testfile 512m 2>&1)
-	rm -f $testfile
-	echo $result | grep -q "Invalid filesystem option" && \
-		_notrun "mkfs.ext4 doesn't support $feature feature"
-}
-
-# this test requires the ext4 kernel support bigalloc feature
-#
-_require_ext4_bigalloc()
-{
-	$MKFS_EXT4_PROG -F -O bigalloc $SCRATCH_DEV 512m >/dev/null 2>&1
+	$MKFS_EXT4_PROG -F $MKFS_OPTIONS -O "$feature" \
+	    $SCRATCH_DEV $sz >>$seqres.full 2>&1 \
+		|| _notrun "mkfs.ext4 doesn't support $feature feature"
 	_scratch_mount >/dev/null 2>&1 \
-	   || _notrun "Ext4 kernel doesn't support bigalloc feature"
+		|| _notrun "Kernel doesn't support the ext4 feature: $feature"
 	_scratch_unmount
 }
 
diff --git a/tests/ext4/003 b/tests/ext4/003
index 9be40178..2498632f 100755
--- a/tests/ext4/003
+++ b/tests/ext4/003
@@ -38,8 +38,7 @@ _supported_fs ext4
 _supported_os Linux
 
 _require_scratch
-_require_ext4_mkfs_feature "bigalloc"
-_require_ext4_bigalloc
+_require_ext4_feature "bigalloc" "512m"
 
 rm -f $seqres.full
 
diff --git a/tests/ext4/025 b/tests/ext4/025
index 49ecb462..013c1940 100755
--- a/tests/ext4/025
+++ b/tests/ext4/025
@@ -48,10 +48,9 @@ _supported_fs ext4
 _supported_os Linux
 _require_scratch_nocheck
 _require_command "$DEBUGFS_PROG" debugfs
-_require_ext4_mkfs_feature "bigalloc,meta_bg,^resize_inode"
+_require_ext4_feature "bigalloc,meta_bg,^resize_inode"
 
 echo "Create ext4 fs and modify first_meta_bg's value"
-_scratch_mkfs "-O bigalloc,meta_bg,^resize_inode" >> $seqres.full 2>&1
 
 # set a big enough first_meta_bg to trigger buffer overrun
 # 842150400 is from original fuzzed ext4 image in bug report
diff --git a/tests/ext4/026 b/tests/ext4/026
index 94a737ce..828b9b89 100755
--- a/tests/ext4/026
+++ b/tests/ext4/026
@@ -50,9 +50,8 @@ _supported_fs ext4
 _supported_os Linux
 _require_scratch
 _require_attrs
-_require_ext4_mkfs_feature ea_inode
+_require_ext4_feature "ea_inode"
 
-_scratch_mkfs_ext4 -O ea_inode >/dev/null 2>&1
 _scratch_mount
 
 # Sets an extended attribute on a file.
diff --git a/tests/ext4/306 b/tests/ext4/306
index be765e6a..fa3b782d 100755
--- a/tests/ext4/306
+++ b/tests/ext4/306
@@ -44,7 +44,6 @@ _supported_fs ext4
 _supported_os Linux
 
 _require_scratch
-_require_ext4_mkfs_feature "64bit"
 
 rm -f $seqres.full
 
-- 
2.11.0.rc0.7.gbe5a750

Re: [PATCH] common: rework _require_ext4_mkfs_feature

[Eryu Guan]

On Sun, Nov 12, 2017 at 08:36:21AM -0500, Theodore Ts'o wrote:
> In all of the places where we need check to see if mkfs.ext4 can
> support a set of file system features, we also should be checking to
> see if the kernel can support those file system features.  So rename
> _require_ext4_mkfs_feature to _require_ext4_feature, and actually
> format the file system in $SCRATCH.  To avoid running mkfs twice in
> most tests, we will teach the tests to assume that
> _require_ext4_feature actually leaves $SCRATCH formatted with a file
> system with those features.

Hmm, I don't think this the correct usage of _require rules. We use
_require rule to check if the requirements have been met, but don't
count on it to do any actual work. And we could always create a small
filesystem (say 512m) if we want to reduce the mkfs time in the _require
rule.

> 
> Also remove a completely unneeded _require_ext4_mkfs_feature "64bit"
> from ext4/306.
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> 
> This is a replacement for "ext4/026: skip test if kernel does not
> support the ea_inode feature"
> 
>  common/rc      | 28 ++++++++++------------------
>  tests/ext4/003 |  3 +--
>  tests/ext4/025 |  3 +--
>  tests/ext4/026 |  3 +--
>  tests/ext4/306 |  1 -
>  5 files changed, 13 insertions(+), 25 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index da6213a0..3febf16f 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1899,31 +1899,23 @@ _require_scratch_ext4_crc()
>  	_scratch_unmount
>  }
>  
> -# Check the specified feature whether it is available in mkfs.ext4 or not.
> -_require_ext4_mkfs_feature()
> +# Check whether the specified feature whether it is supported by
> +# mkfs.ext4 and the kernel.
> +_require_ext4_feature()

How about naming it as _require_scratch_ext4_feature? So it indicates
that we assume $SCRATCH_DEV is present here and acts as a reminder to
_require_scratch first.

>  {
> -	local feature=$1
> -	local testfile=/tmp/$$.ext4_mkfs
> +	local feature="$1"
> +	local sz="$2"

Assign sz=512m unconditionally?

>  
>  	if [ -z "$feature" ]; then
> -                echo "Usage: _require_ext4_mkfs_feature feature"
> +                echo "Usage: _require_ext4_feature feature"
>                  exit 1
>          fi
>  
> -	touch $testfile
> -	local result=$($MKFS_EXT4_PROG -F -O $feature -n $testfile 512m 2>&1)
> -	rm -f $testfile
> -	echo $result | grep -q "Invalid filesystem option" && \
> -		_notrun "mkfs.ext4 doesn't support $feature feature"
> -}
> -
> -# this test requires the ext4 kernel support bigalloc feature
> -#
> -_require_ext4_bigalloc()
> -{
> -	$MKFS_EXT4_PROG -F -O bigalloc $SCRATCH_DEV 512m >/dev/null 2>&1
> +	$MKFS_EXT4_PROG -F $MKFS_OPTIONS -O "$feature" \
> +	    $SCRATCH_DEV $sz >>$seqres.full 2>&1 \
> +		|| _notrun "mkfs.ext4 doesn't support $feature feature"
>  	_scratch_mount >/dev/null 2>&1 \
> -	   || _notrun "Ext4 kernel doesn't support bigalloc feature"
> +		|| _notrun "Kernel doesn't support the ext4 feature: $feature"
>  	_scratch_unmount
>  }
>  
> diff --git a/tests/ext4/003 b/tests/ext4/003
> index 9be40178..2498632f 100755
> --- a/tests/ext4/003
> +++ b/tests/ext4/003
> @@ -38,8 +38,7 @@ _supported_fs ext4
>  _supported_os Linux
>  
>  _require_scratch
> -_require_ext4_mkfs_feature "bigalloc"
> -_require_ext4_bigalloc
> +_require_ext4_feature "bigalloc" "512m"
>  
>  rm -f $seqres.full
>  
> diff --git a/tests/ext4/025 b/tests/ext4/025
> index 49ecb462..013c1940 100755
> --- a/tests/ext4/025
> +++ b/tests/ext4/025
> @@ -48,10 +48,9 @@ _supported_fs ext4
>  _supported_os Linux
>  _require_scratch_nocheck
>  _require_command "$DEBUGFS_PROG" debugfs
> -_require_ext4_mkfs_feature "bigalloc,meta_bg,^resize_inode"
> +_require_ext4_feature "bigalloc,meta_bg,^resize_inode"
>  
>  echo "Create ext4 fs and modify first_meta_bg's value"
> -_scratch_mkfs "-O bigalloc,meta_bg,^resize_inode" >> $seqres.full 2>&1
>  
>  # set a big enough first_meta_bg to trigger buffer overrun
>  # 842150400 is from original fuzzed ext4 image in bug report
> diff --git a/tests/ext4/026 b/tests/ext4/026
> index 94a737ce..828b9b89 100755
> --- a/tests/ext4/026
> +++ b/tests/ext4/026
> @@ -50,9 +50,8 @@ _supported_fs ext4
>  _supported_os Linux
>  _require_scratch
>  _require_attrs
> -_require_ext4_mkfs_feature ea_inode
> +_require_ext4_feature "ea_inode"
>  
> -_scratch_mkfs_ext4 -O ea_inode >/dev/null 2>&1
>  _scratch_mount
>  
>  # Sets an extended attribute on a file.
> diff --git a/tests/ext4/306 b/tests/ext4/306
> index be765e6a..fa3b782d 100755
> --- a/tests/ext4/306
> +++ b/tests/ext4/306
> @@ -44,7 +44,6 @@ _supported_fs ext4
>  _supported_os Linux
>  
>  _require_scratch
> -_require_ext4_mkfs_feature "64bit"

This is actually for old distros that mkfs.ext4 doesn't support 64bit
feature, e.g. RHEL6. I think it's still meanful for now.

Thanks,
Eryu

>  
>  rm -f $seqres.full
>  
> -- 
> 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] common: rework _require_ext4_mkfs_feature

[Theodore Ts'o]

On Tue, Nov 14, 2017 at 07:43:15PM +0800, Eryu Guan wrote:
> Hmm, I don't think this the correct usage of _require rules. We use
> _require rule to check if the requirements have been met, but don't
> count on it to do any actual work. And we could always create a small
> filesystem (say 512m) if we want to reduce the mkfs time in the _require
> rule.

I'm trying to avoid duplicating work; in most of the cases where we
call _require_ext4_feature, we immediately create a file system with
the given features.

My original version of the patch to modify an ext4 test simply open
coded the check for the failure and then ran "_notrun".  Which was
criticized because the reviewers thought that it should be factored
out into common code.  That's fine, but if we have to call mke2fs
twice, that would be sad....

> How about naming it as _require_scratch_ext4_feature? So it indicates
> that we assume $SCRATCH_DEV is present here and acts as a reminder to
> _require_scratch first.

Sure.  How about simply including _require_scratch into the function?

> 
> >  {
> > -	local feature=$1
> > -	local testfile=/tmp/$$.ext4_mkfs
> > +	local feature="$1"
> > +	local sz="$2"
> 
> Assign sz=512m unconditionally?

In half of the cases we actually want to format the file system to
take up the whole disk.  So if sz is not specified, we end up omitting
the size parameter to mke2fs, which is by design.

> > +++ b/tests/ext4/306
> > @@ -44,7 +44,6 @@ _supported_fs ext4
> >  _supported_os Linux
> >  
> >  _require_scratch
> > -_require_ext4_mkfs_feature "64bit"
> 
> This is actually for old distros that mkfs.ext4 doesn't support 64bit
> feature, e.g. RHEL6. I think it's still meanful for now.

The funny thing is that ext4/306 is actually formatting the file
system *without* 64-bit feature.  So the _require_ext4_mkfs_feature is
really trying to make sure that this mke2fs doesn't fail:

$MKFS_EXT4_PROG -F -O ^extents,^64bit $SCRATCH_DEV 512m

So what we can probably do is this:

features="^extents"
if grep -q 64bit /etc/mke2fs.conf ; then
    features="^extents,^64bit"
fi
$MKFS_EXT4_PROG -F -O $features $SCRATCH_DEV 512m

						- Ted

[PATCH -v3] common: rework _require_ext4_mkfs_feature

[Theodore Ts'o]

On Tue, Nov 14, 2017 at 06:38:49PM -0500, Theodore Ts'o wrote:
> 
> Sure.  How about simply including _require_scratch into the function?

Nope, that won't work, since we need _require_scratch_nocheck for one
of the tests.

OK, here's V3 of the patch.  WDYT?

						- Ted
commit b9d9da8b901acc5b6cb5da7149b7e8ce986e436e
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Sat Nov 11 23:28:51 2017 -0500

    common: rework _require_ext4_mkfs_feature
    
    In all of the places where we need check to see if mkfs.ext4 can
    support a set of file system features, we also should be checking to
    see if the kernel can support those file system features.  So rename
    _require_ext4_mkfs_feature to _require_ext4_feature, and actually
    format the file system in $SCRATCH.  To avoid running mkfs twice in
    most tests, we will teach the tests to assume that
    _require_ext4_feature actually leaves $SCRATCH formatted with a file
    system with those features.
    
    Also allow ext4/306 to run on systems where mke2fs doesn't support the
    "64bit" option.
    
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/common/rc b/common/rc
index da6213a0..6f6901c4 100644
--- a/common/rc
+++ b/common/rc
@@ -1899,31 +1899,23 @@ _require_scratch_ext4_crc()
 	_scratch_unmount
 }
 
-# Check the specified feature whether it is available in mkfs.ext4 or not.
-_require_ext4_mkfs_feature()
+# Check whether the specified feature whether it is supported by
+# mkfs.ext4 and the kernel.
+_require_scratch_ext4_feature()
 {
-	local feature=$1
-	local testfile=/tmp/$$.ext4_mkfs
+	local feature="$1"
+	local sz="$2"
 
 	if [ -z "$feature" ]; then
-                echo "Usage: _require_ext4_mkfs_feature feature"
+                echo "Usage: _require_scratch_ext4_feature feature [size]"
                 exit 1
         fi
 
-	touch $testfile
-	local result=$($MKFS_EXT4_PROG -F -O $feature -n $testfile 512m 2>&1)
-	rm -f $testfile
-	echo $result | grep -q "Invalid filesystem option" && \
-		_notrun "mkfs.ext4 doesn't support $feature feature"
-}
-
-# this test requires the ext4 kernel support bigalloc feature
-#
-_require_ext4_bigalloc()
-{
-	$MKFS_EXT4_PROG -F -O bigalloc $SCRATCH_DEV 512m >/dev/null 2>&1
+	$MKFS_EXT4_PROG -F $MKFS_OPTIONS -O "$feature" \
+	    $SCRATCH_DEV $sz >>$seqres.full 2>&1 \
+		|| _notrun "mkfs.ext4 doesn't support $feature feature"
 	_scratch_mount >/dev/null 2>&1 \
-	   || _notrun "Ext4 kernel doesn't support bigalloc feature"
+		|| _notrun "Kernel doesn't support the ext4 feature: $feature"
 	_scratch_unmount
 }
 
diff --git a/tests/ext4/003 b/tests/ext4/003
index 9be40178..bdce00d0 100755
--- a/tests/ext4/003
+++ b/tests/ext4/003
@@ -38,8 +38,7 @@ _supported_fs ext4
 _supported_os Linux
 
 _require_scratch
-_require_ext4_mkfs_feature "bigalloc"
-_require_ext4_bigalloc
+_require_scratch_ext4_feature "bigalloc" "512m"
 
 rm -f $seqres.full
 
diff --git a/tests/ext4/025 b/tests/ext4/025
index 49ecb462..3e32e7f1 100755
--- a/tests/ext4/025
+++ b/tests/ext4/025
@@ -48,10 +48,9 @@ _supported_fs ext4
 _supported_os Linux
 _require_scratch_nocheck
 _require_command "$DEBUGFS_PROG" debugfs
-_require_ext4_mkfs_feature "bigalloc,meta_bg,^resize_inode"
+_require_scratch_ext4_feature "bigalloc,meta_bg,^resize_inode"
 
 echo "Create ext4 fs and modify first_meta_bg's value"
-_scratch_mkfs "-O bigalloc,meta_bg,^resize_inode" >> $seqres.full 2>&1
 
 # set a big enough first_meta_bg to trigger buffer overrun
 # 842150400 is from original fuzzed ext4 image in bug report
diff --git a/tests/ext4/026 b/tests/ext4/026
index 94a737ce..832e4646 100755
--- a/tests/ext4/026
+++ b/tests/ext4/026
@@ -50,9 +50,8 @@ _supported_fs ext4
 _supported_os Linux
 _require_scratch
 _require_attrs
-_require_ext4_mkfs_feature ea_inode
+_require_scratch_ext4_feature "ea_inode"
 
-_scratch_mkfs_ext4 -O ea_inode >/dev/null 2>&1
 _scratch_mount
 
 # Sets an extended attribute on a file.
diff --git a/tests/ext4/306 b/tests/ext4/306
index be765e6a..70f281db 100755
--- a/tests/ext4/306
+++ b/tests/ext4/306
@@ -44,12 +44,15 @@ _supported_fs ext4
 _supported_os Linux
 
 _require_scratch
-_require_ext4_mkfs_feature "64bit"
 
 rm -f $seqres.full
 
 # Make a small ext4 fs with extents disabled & mount it
-$MKFS_EXT4_PROG -F -O ^extents,^64bit $SCRATCH_DEV 512m >> $seqres.full 2>&1
+features="^extents"
+if grep -q 64bit /etc/mke2fs.conf ; then
+    features="^extents,^64bit"
+fi
+$MKFS_EXT4_PROG -F -O "$features" $SCRATCH_DEV 512m >> $seqres.full 2>&1
 _scratch_mount || _fail "couldn't mount fs"
 
 # Create a small non-extent-based file

Re: [PATCH -v3] common: rework _require_ext4_mkfs_feature

[Eryu Guan]

On Tue, Nov 14, 2017 at 07:30:37PM -0500, Theodore Ts'o wrote:
> On Tue, Nov 14, 2017 at 06:38:49PM -0500, Theodore Ts'o wrote:
> > 
> > Sure.  How about simply including _require_scratch into the function?
> 
> Nope, that won't work, since we need _require_scratch_nocheck for one
> of the tests.
> 
> OK, here's V3 of the patch.  WDYT?
> 
> 						- Ted
> commit b9d9da8b901acc5b6cb5da7149b7e8ce986e436e
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Sat Nov 11 23:28:51 2017 -0500
> 
>     common: rework _require_ext4_mkfs_feature
>     
>     In all of the places where we need check to see if mkfs.ext4 can
>     support a set of file system features, we also should be checking to
>     see if the kernel can support those file system features.  So rename
>     _require_ext4_mkfs_feature to _require_ext4_feature, and actually
>     format the file system in $SCRATCH.  To avoid running mkfs twice in
>     most tests, we will teach the tests to assume that
>     _require_ext4_feature actually leaves $SCRATCH formatted with a file
>     system with those features.

I have to say I still don't like the idea of relying on a _require rule
to do the actual mkfs work, I'd rather run mkfs twice. That way we keep
consistent semantics and usages of all _require rules across fstests (we
already have several other rules do so, like _require_scratch_xfs_crc),
and give people least surprise when reading tests calling such rules.
And we can always create a small filesystem by default, say 512m, in
_require_scratch_ext4_feature to reduce the mkfs time and mitigate the
downside of mkfs twice.

>     
>     Also allow ext4/306 to run on systems where mke2fs doesn't support the
>     "64bit" option.

This part looks good to me. Thanks!

Eryu

Re: [PATCH] common: rework _require_ext4_mkfs_feature

[Dave Chinner]

On Tue, Nov 14, 2017 at 07:43:15PM +0800, Eryu Guan wrote:
> On Sun, Nov 12, 2017 at 08:36:21AM -0500, Theodore Ts'o wrote:
> > In all of the places where we need check to see if mkfs.ext4 can
> > support a set of file system features, we also should be checking to
> > see if the kernel can support those file system features.  So rename
> > _require_ext4_mkfs_feature to _require_ext4_feature, and actually
> > format the file system in $SCRATCH.  To avoid running mkfs twice in
> > most tests, we will teach the tests to assume that
> > _require_ext4_feature actually leaves $SCRATCH formatted with a file
> > system with those features.
> 
> Hmm, I don't think this the correct usage of _require rules. We use
> _require rule to check if the requirements have been met, but don't
> count on it to do any actual work. And we could always create a small
> filesystem (say 512m) if we want to reduce the mkfs time in the _require
> rule.

Implement, like in mkfs.xfs, the "-N" flag (a.k.a "--dry-run").
It does option parsing and geometry calculations, outputs the config
that would be made, then exits before doing any actual IO. Hence we
can check mkfs feature support on XFS, and only take the time to
make a filesystem if we need to check that kernel support exists.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common: rework _require_ext4_mkfs_feature

[Theodore Ts'o]

On Wed, Nov 15, 2017 at 11:56:55AM +1100, Dave Chinner wrote:
> Implement, like in mkfs.xfs, the "-N" flag (a.k.a "--dry-run").
> It does option parsing and geometry calculations, outputs the config
> that would be made, then exits before doing any actual IO. Hence we
> can check mkfs feature support on XFS, and only take the time to
> make a filesystem if we need to check that kernel support exists.

mke2fs has a -n option, but that's not what I'm trying to address.

Now that we want to *mount* the file system to check to see if the
kernel can support the file system feature, we can no longer use the
-n option.  And since the tests all need to create the file system
*anyway*, it seemed rather pointless to create it once, then do the
trial mount, and then creating the file system a second time.

      	     	      	       	   	       - Ted