[PATCH] xfstests-bld: add exclude file for ext3 tests

[PATCH] xfstests-bld: add exclude file for ext3 tests

[Eric Whitney]

Add an exclude file for the ext3 test case to prevent failure reports
from tests that exercise unsupported online defrag functionality.

Two online defrag tests - ext4/307 and /308 - are not included because
they contain explicit requirements for fallocate support that prevents
them from running on an emulated ext3 file system.

It may be possible to modify the excluded tests at a future date so they
will not run unless the test file system is extent mapped.  For now, this
patch is an expedient measure to reduce testing noise.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 kvm-xfstests/test-appliance/files/root/conf/ext3.exclude | 6 ++++++
 1 file changed, 6 insertions(+)
 create mode 100644 kvm-xfstests/test-appliance/files/root/conf/ext3.exclude

diff --git a/kvm-xfstests/test-appliance/files/root/conf/ext3.exclude b/kvm-xfstests/test-appliance/files/root/conf/ext3.exclude
new file mode 100644
index 0000000..c46c79b
--- /dev/null
+++ b/kvm-xfstests/test-appliance/files/root/conf/ext3.exclude
@@ -0,0 +1,6 @@
+# ext4's ext3 emulation does not support on-line
+# defrag, which requires extent-based files
+ext4/301
+ext4/302
+ext4/303
+ext4/304
-- 
2.1.4

Re: [PATCH] xfstests-bld: add exclude file for ext3 tests

[Dave Chinner]

On Sat, Feb 20, 2016 at 10:51:57AM -0500, Eric Whitney wrote:
> Add an exclude file for the ext3 test case to prevent failure reports
> from tests that exercise unsupported online defrag functionality.

ext3 should not run these tests because of the _requires_defrag
check in these tests results in a _notrun command being run
when FSTYP=ext3.

> Two online defrag tests - ext4/307 and /308 - are not included because
> they contain explicit requirements for fallocate support that prevents
> them from running on an emulated ext3 file system.

Same here.

> It may be possible to modify the excluded tests at a future date so they
> will not run unless the test file system is extent mapped.  For now, this
> patch is an expedient measure to reduce testing noise.
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> ---
>  kvm-xfstests/test-appliance/files/root/conf/ext3.exclude | 6 ++++++
>  1 file changed, 6 insertions(+)
>  create mode 100644 kvm-xfstests/test-appliance/files/root/conf/ext3.exclude
> 
> diff --git a/kvm-xfstests/test-appliance/files/root/conf/ext3.exclude b/kvm-xfstests/test-appliance/files/root/conf/ext3.exclude
> new file mode 100644
> index 0000000..c46c79b
> --- /dev/null
> +++ b/kvm-xfstests/test-appliance/files/root/conf/ext3.exclude
> @@ -0,0 +1,6 @@
> +# ext4's ext3 emulation does not support on-line
> +# defrag, which requires extent-based files
> +ext4/301
> +ext4/302
> +ext4/303
> +ext4/304

So what we really need to understand is why _require_defrag is not
preventing these tests beign run on an ext3 filesystem?

Eric, what is the output of these tests when it is run on an ext3
filesytsem? What is the fstyp you are testing?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfstests-bld: add exclude file for ext3 tests

[Theodore Ts'o]

On Sun, Feb 21, 2016 at 11:34:11AM +1100, Dave Chinner wrote:
> On Sat, Feb 20, 2016 at 10:51:57AM -0500, Eric Whitney wrote:
> > Add an exclude file for the ext3 test case to prevent failure reports
> > from tests that exercise unsupported online defrag functionality.
> 
> ext3 should not run these tests because of the _requires_defrag
> check in these tests results in a _notrun command being run
> when FSTYP=ext3.

In this particular case, the test is being run with

FSTYP=ext4
MKFS_OPTIONS="-q -O ^extents,^flex_bg,^uninit_bg,^64bit,^metadata_csum,^huge_file,^dir_nlink,^extra_isize"
EXT_MOUNT_OPTIONS="nodelalloc"

This is why _requires_defrag is passing and so ext4/307 and ext4/308
is allowed to run.

Historically, this test was set up in this fashion because I wanted to
make sure the ext4 kernel code would be used, and not the code found
in fs/ext3 (this was before fs/ext3 was removed from the kernel).
Since we want to support testing on ancient kernels (either 3.10
android kernels or 2.6.34 RHEL6 kernels), using this strategy for
testing ext4's support for file systems with ext3 features is
something that still makes sense to do, and so I would want to keep
running tests using this setup.

> > Two online defrag tests - ext4/307 and /308 - are not included because
> > they contain explicit requirements for fallocate support that prevents
> > them from running on an emulated ext3 file system.
> 
> Same here.

So probably the right answer here is to change _require_defrag so that
for EXT4, to also add the assertion:

_require_xfs_io_command "falloc"

I'll note this is a not quite guaranteed to be correct because
_require_xfs_io_command tests to see whether or not falloc works on
TEST_DEV, and these tests are actually create a test file system on
SCRATCH_DEV, and in theory TEST_DEV and SCRATCH_DEV could have
different file system features.  That's because xfstests never runs
mkfs on TEST_DEV, but SCRATCH_DEV does get mkfs'ed and in theory the
file system features set by MKFS_OPTIONS could be different from what
exists on TEST_DEV.

I'm willing to consider this a test configuration error, and certainly
kvm-xfstests doesn't ever set up such a arguably non-sensible test
configuration.

						- Ted

Re: [PATCH] xfstests-bld: add exclude file for ext3 tests

[Theodore Ts'o]

On Sat, Feb 20, 2016 at 09:57:54PM -0500, Theodore Ts'o wrote:
> 
> So probably the right answer here is to change _require_defrag so that
> for EXT4, to also add the assertion:
> 
> _require_xfs_io_command "falloc"

So like this...  (BTW what's up with my other pending xfstests
patches?  Would you like me to resend a whole back with the
reviewed-by lines merged in?)

						- Ted

>From a1cd89ca74d8e8cd345b88e62a5eabab70aed62e Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Sun, 21 Feb 2016 00:12:00 -0500
Subject: [PATCH] defrag: require falloc support for ext4 defrag

The e4defrag program requires the use of fallocate.  Certain ext4 file
system configurations which don't enable extents do not support
fallocate, so we should skip the defrag tests in that case.

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 common/defrag | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/defrag b/common/defrag
index 942593e..f3d9e43 100644
--- a/common/defrag
+++ b/common/defrag
@@ -28,6 +28,7 @@ _require_defrag()
 	;;
     ext4|ext4dev)
         DEFRAG_PROG="$E4DEFRAG_PROG"
+	_require_xfs_io_command "falloc"
 	;;
     btrfs)
 	DEFRAG_PROG="$BTRFS_UTIL_PROG filesystem defragment"
-- 
2.5.0

Re: [PATCH] xfstests-bld: add exclude file for ext3 tests

[Dave Chinner]

On Sat, Feb 20, 2016 at 09:57:54PM -0500, Theodore Ts'o wrote:
> On Sun, Feb 21, 2016 at 11:34:11AM +1100, Dave Chinner wrote:
> > On Sat, Feb 20, 2016 at 10:51:57AM -0500, Eric Whitney wrote:
> > > Add an exclude file for the ext3 test case to prevent failure reports
> > > from tests that exercise unsupported online defrag functionality.
> > 
> > ext3 should not run these tests because of the _requires_defrag
> > check in these tests results in a _notrun command being run
> > when FSTYP=ext3.
> 
> In this particular case, the test is being run with
> 
> FSTYP=ext4
> MKFS_OPTIONS="-q -O ^extents,^flex_bg,^uninit_bg,^64bit,^metadata_csum,^huge_file,^dir_nlink,^extra_isize"
> EXT_MOUNT_OPTIONS="nodelalloc"
> 
> This is why _requires_defrag is passing and so ext4/307 and ext4/308
> is allowed to run.

Ok, I suspected that this was the reason for it occurring, but
that's not really testing "ext3" in the true sense - it's a modified
ext4 on-disk variant that happens to match the ext3 configuration.

> > > Two online defrag tests - ext4/307 and /308 - are not included because
> > > they contain explicit requirements for fallocate support that prevents
> > > them from running on an emulated ext3 file system.
> > 
> > Same here.
> 
> So probably the right answer here is to change _require_defrag so that
> for EXT4, to also add the assertion:
> 
> _require_xfs_io_command "falloc"
> 
> I'll note this is a not quite guaranteed to be correct because
> _require_xfs_io_command tests to see whether or not falloc works on
> TEST_DEV, and these tests are actually create a test file system on
> SCRATCH_DEV, and in theory TEST_DEV and SCRATCH_DEV could have
> different file system features.  That's because xfstests never runs
> mkfs on TEST_DEV, but SCRATCH_DEV does get mkfs'ed and in theory the
> file system features set by MKFS_OPTIONS could be different from what
> exists on TEST_DEV.

Right - that's normally why we have "_require_scratch_foo" so that
it's specific which device requires that functionality. Seems easy
enough to fix that problem.

back to the ext4-as-ext3, defrag requires extents to be enabled in
the ext4 filesystem, right?  So you could check whether than flag is
set easily enough with something like this:

dumpe2fs -h $dev |grep features |grep -q extent
if [ $? -ne 0 ]; then
	_notrun "Filesystem does not support defrag operations"
fi

That would work for everything with a fstyp of ext4, wouldn't it?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfstests-bld: add exclude file for ext3 tests

[Theodore Ts'o]

On Sun, Feb 21, 2016 at 05:23:56PM +1100, Dave Chinner wrote:
> 
> Ok, I suspected that this was the reason for it occurring, but
> that's not really testing "ext3" in the true sense - it's a modified
> ext4 on-disk variant that happens to match the ext3 configuration.

Yes, although this is also how ext4 emulates fs/ext3 behavior --
either because fs/ext3 has been removed, as it has in newer kernels,
or because (as most distributions have been shipping for a while)
fs/ext3 has been disabled via Kconfig, and CONFIG_EXT4_USE_FOR_EXT23
is enabled.  I'm not doing this any more, but for a while I had been
using an ext3 file system for the root file system in the KVM images
so I could do things with ext4 for testing purposes without
interfering with the ability of the KVM image to boot.  So in practice
it is effectively the same as testing "ext3" using some white-box
knowledge of how ext4 emulates ext3 under the covers.


> > I'll note this is a not quite guaranteed to be correct because
> > _require_xfs_io_command tests to see whether or not falloc works on
> > TEST_DEV, and these tests are actually create a test file system on
> > SCRATCH_DEV, and in theory TEST_DEV and SCRATCH_DEV could have
> > different file system features.  That's because xfstests never runs
> > mkfs on TEST_DEV, but SCRATCH_DEV does get mkfs'ed and in theory the
> > file system features set by MKFS_OPTIONS could be different from what
> > exists on TEST_DEV.
> 
> Right - that's normally why we have "_require_scratch_foo" so that
> it's specific which device requires that functionality. Seems easy
> enough to fix that problem.

For what it's worth here are the list of generic tests that (a) have
_require_scratch and (b) have _require_xfs_io_command "falloc":

032 038 042 071 094 103 223 250 252 274 299 300 312 324

So it probably does make sense to have a _require_scratch_falloc
helper macro, although in practice I rather doubt folks have been
tripped up by this until now.  Still, it would be more correct.

Alternative, it might be interesting to consider would be a test at
the beginning of the xfstests run where the feature set for the
TEST_DEV is compared against SCRATCH_DEV when created using
_scratch_mkfs and if they differ, to issue a warning.  This would be
an ext4-specific thing, and I'm not sure if you would consider this
within scope of xfstests, or whether this is something that belongs in
a test-runner framework such as gce-xfstests.  I can see arguments for
this either way, although I would lean towards putting such a sanity
check in kvm-xfstests/gce-xfstests.  What is your preference?

> back to the ext4-as-ext3, defrag requires extents to be enabled in
> the ext4 filesystem, right?  So you could check whether than flag is
> set easily enough with something like this:
> 
> dumpe2fs -h $dev |grep features |grep -q extent
> if [ $? -ne 0 ]; then
> 	_notrun "Filesystem does not support defrag operations"
> fi
> 
> That would work for everything with a fstyp of ext4, wouldn't it?

Yes, or use something like a _require_scratch_falloc helper.

     	    	      	     			     - Ted

Re: [PATCH] xfstests-bld: add exclude file for ext3 tests

[Dave Chinner]

On Sun, Feb 21, 2016 at 11:58:51AM -0500, Theodore Ts'o wrote:
> On Sun, Feb 21, 2016 at 05:23:56PM +1100, Dave Chinner wrote:
> > > I'll note this is a not quite guaranteed to be correct because
> > > _require_xfs_io_command tests to see whether or not falloc works on
> > > TEST_DEV, and these tests are actually create a test file system on
> > > SCRATCH_DEV, and in theory TEST_DEV and SCRATCH_DEV could have
> > > different file system features.  That's because xfstests never runs
> > > mkfs on TEST_DEV, but SCRATCH_DEV does get mkfs'ed and in theory the
> > > file system features set by MKFS_OPTIONS could be different from what
> > > exists on TEST_DEV.
> > 
> > Right - that's normally why we have "_require_scratch_foo" so that
> > it's specific which device requires that functionality. Seems easy
> > enough to fix that problem.
> 
> For what it's worth here are the list of generic tests that (a) have
> _require_scratch and (b) have _require_xfs_io_command "falloc":
> 
> 032 038 042 071 094 103 223 250 252 274 299 300 312 324
> 
> So it probably does make sense to have a _require_scratch_falloc
> helper macro, although in practice I rather doubt folks have been
> tripped up by this until now.  Still, it would be more correct.

Well, that's a different problem, unrealted to the fact that
_require_defrag is giving the wrong answer to certain ext4
configurations.

As it is, I'm not sure that it's at all common that two filesystems
of the same type on the same kernel will give different answers to
fallocate. That seems like a corner case of ext4's  whacky feature
matrix, and so is is not something we typically need to care about,
nor is it something anyone should have to remember when writing or
reviewing a new test....

> Alternative, it might be interesting to consider would be a test at
> the beginning of the xfstests run where the feature set for the
> TEST_DEV is compared against SCRATCH_DEV when created using
> _scratch_mkfs and if they differ, to issue a warning.  This would be
> an ext4-specific thing, and I'm not sure if you would consider this
> within scope of xfstests, or whether this is something that belongs in
> a test-runner framework such as gce-xfstests.  I can see arguments for
> this either way, although I would lean towards putting such a sanity
> check in kvm-xfstests/gce-xfstests.  What is your preference?

I don't think it's particularly sane to make xfstests try to support
ext3 configurations with FSTYP=ext4 when you can just use
FSTYP=ext3 and everything just works.

> > back to the ext4-as-ext3, defrag requires extents to be enabled in
> > the ext4 filesystem, right?  So you could check whether than flag is
> > set easily enough with something like this:
> > 
> > dumpe2fs -h $dev |grep features |grep -q extent
> > if [ $? -ne 0 ]; then
> > 	_notrun "Filesystem does not support defrag operations"
> > fi
> > 
> > That would work for everything with a fstyp of ext4, wouldn't it?
> 
> Yes, or use something like a _require_scratch_falloc helper.

I'd much prefer that we fix the _require_defrag helper to correctly
determine if an ext4 filesystem can support defrag rather than
overload some other helper with undocumented behaviour that may be
completely unrelated to the test at hand and rely on people to
remember that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com