| Message ID | 20160220155157.GA19420@localhost.localdomain |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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