| Message ID | 20171112133621.2474-1-tytso@mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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.
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 -- 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
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
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(-)