diff mbox

ext4: add regression tests for ^extents punch hole

Message ID 4c557308eb4e62752dc8b513495cb6d46ca5775d.1424730653.git.osandov@osandov.com
State Not Applicable, archived
Headers show

Commit Message

Omar Sandoval Feb. 23, 2015, 10:39 p.m. UTC 
Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
ext4 filesystem with indirect blocks.

Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/ext4/005.out |  29 ++++++++++++++
 tests/ext4/group   |   1 +
 3 files changed, 145 insertions(+)
 create mode 100755 tests/ext4/005
 create mode 100644 tests/ext4/005.out

Comments

Dave Chinner Feb. 23, 2015, 10:46 p.m. UTC | #1 
On Mon, Feb 23, 2015 at 02:39:36PM -0800, Omar Sandoval wrote:
> Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
> fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
> ext4 filesystem with indirect blocks.
> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> ---
>  tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ext4/005.out |  29 ++++++++++++++
>  tests/ext4/group   |   1 +
>  3 files changed, 145 insertions(+)
>  create mode 100755 tests/ext4/005
>  create mode 100644 tests/ext4/005.out

What's ext4 specific about this test apart from the mkfs parameter?
Shouldn't it be generic and so test all the filesystems behave the
same?  i.e. when someone then runs

# MKFS_OPTIONS="-b size=1k -O ^extents" ./check -g auto

That will exercise this specific regression fix, not to mention give
much, much better test coverage of that configuration than just
making a single test use that config...

Cheers,

Dave.
Omar Sandoval Feb. 23, 2015, 11:11 p.m. UTC | #2 
On Tue, Feb 24, 2015 at 09:46:20AM +1100, Dave Chinner wrote:
> On Mon, Feb 23, 2015 at 02:39:36PM -0800, Omar Sandoval wrote:
> > Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
> > fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
> > ext4 filesystem with indirect blocks.
> > 
> > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> > ---
> >  tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/ext4/005.out |  29 ++++++++++++++
> >  tests/ext4/group   |   1 +
> >  3 files changed, 145 insertions(+)
> >  create mode 100755 tests/ext4/005
> >  create mode 100644 tests/ext4/005.out
> 
> What's ext4 specific about this test apart from the mkfs parameter?
> Shouldn't it be generic and so test all the filesystems behave the
> same?  i.e. when someone then runs
> 
> # MKFS_OPTIONS="-b size=1k -O ^extents" ./check -g auto
> 
> That will exercise this specific regression fix, not to mention give
> much, much better test coverage of that configuration than just
> making a single test use that config...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Hi, Dave,

This test isn't completely generic bcause the output is dependent on the
block size. In particular, fpunch+fiemap will have different results
based on the block size:

----
# mkfs.ext3 -b1024 /dev/sdb1
# mount /dev/sdb1 /mnt/test
# xfs_io -f -c 'pwrite 0 8192' /mnt/test/a
# xfs_io -c 'fpunch 0 1024' /mnt/test/a
# xfs_io -c fiemap /mnt/test/a
/mnt/test/a:
        0: [0..1]: hole
        1: [2..15]: 1028..1041
# umount /mnt/test
# mkfs.ext3 -b4096 /dev/sdb1
# mount /dev/sdb1 /mnt/test
# xfs_io -f -c 'pwrite 0 8192' /mnt/test/a
# xfs_io -c 'fpunch 0 1024' /mnt/test/a
# xfs_io -c fiemap /mnt/test/a
/mnt/test/a:
        0: [0..15]: 8192..8207
----

I could either remove the fiemap output from the test case and rely on
the md5sum or round all of the punches to some larger block size so it
will behave the same up to, say, 8k. Do either of those options sound
better?

Alternatively, is there a good way to have block size-dependent test
output? Then we could have the test adapt to different block sizes and
cover these regressions at any block size, not just 1k.

Thanks!
Eric Sandeen Feb. 23, 2015, 11:28 p.m. UTC | #3 
On 2/23/15 5:11 PM, Omar Sandoval wrote:
> On Tue, Feb 24, 2015 at 09:46:20AM +1100, Dave Chinner wrote:
>> On Mon, Feb 23, 2015 at 02:39:36PM -0800, Omar Sandoval wrote:
>>> Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
>>> fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
>>> ext4 filesystem with indirect blocks.
>>>
>>> Signed-off-by: Omar Sandoval <osandov@osandov.com>
>>> ---
>>>  tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  tests/ext4/005.out |  29 ++++++++++++++
>>>  tests/ext4/group   |   1 +
>>>  3 files changed, 145 insertions(+)
>>>  create mode 100755 tests/ext4/005
>>>  create mode 100644 tests/ext4/005.out
>>
>> What's ext4 specific about this test apart from the mkfs parameter?
>> Shouldn't it be generic and so test all the filesystems behave the
>> same?  i.e. when someone then runs
>>
>> # MKFS_OPTIONS="-b size=1k -O ^extents" ./check -g auto
>>
>> That will exercise this specific regression fix, not to mention give
>> much, much better test coverage of that configuration than just
>> making a single test use that config...
>>
>> Cheers,
>>
>> Dave.
>> -- 
>> Dave Chinner
>> david@fromorbit.com
> 
> Hi, Dave,
> 
> This test isn't completely generic bcause the output is dependent on the
> block size. In particular, fpunch+fiemap will have different results
> based on the block size:
> 
> ----
> # mkfs.ext3 -b1024 /dev/sdb1
> # mount /dev/sdb1 /mnt/test
> # xfs_io -f -c 'pwrite 0 8192' /mnt/test/a
> # xfs_io -c 'fpunch 0 1024' /mnt/test/a
> # xfs_io -c fiemap /mnt/test/a
> /mnt/test/a:
>         0: [0..1]: hole
>         1: [2..15]: 1028..1041
> # umount /mnt/test
> # mkfs.ext3 -b4096 /dev/sdb1
> # mount /dev/sdb1 /mnt/test
> # xfs_io -f -c 'pwrite 0 8192' /mnt/test/a
> # xfs_io -c 'fpunch 0 1024' /mnt/test/a
> # xfs_io -c fiemap /mnt/test/a
> /mnt/test/a:
>         0: [0..15]: 8192..8207
> ----
> 
> I could either remove the fiemap output from the test case and rely on
> the md5sum or round all of the punches to some larger block size so it
> will behave the same up to, say, 8k. Do either of those options sound
> better?
> 
> Alternatively, is there a good way to have block size-dependent test
> output? Then we could have the test adapt to different block sizes and
> cover these regressions at any block size, not just 1k.

Can you scale every operational offset by block size?  I think there are
other tests which do this sort of thing - look at _filter_bmap in test
xfs/194 maybe?

i.e. above you would do 'fpunch 0 $blocksize' not 'fpunch 0 1024'
(or blocksize/4, or whatever your intent is)

-Eric

> Thanks!
> 

--
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
Eric Sandeen Feb. 23, 2015, 11:43 p.m. UTC | #4 
On 2/23/15 5:28 PM, Eric Sandeen wrote:
> On 2/23/15 5:11 PM, Omar Sandoval wrote:
>> On Tue, Feb 24, 2015 at 09:46:20AM +1100, Dave Chinner wrote:
>>> On Mon, Feb 23, 2015 at 02:39:36PM -0800, Omar Sandoval wrote:
>>>> Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
>>>> fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
>>>> ext4 filesystem with indirect blocks.
>>>>
>>>> Signed-off-by: Omar Sandoval <osandov@osandov.com>
>>>> ---
>>>>  tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  tests/ext4/005.out |  29 ++++++++++++++
>>>>  tests/ext4/group   |   1 +
>>>>  3 files changed, 145 insertions(+)
>>>>  create mode 100755 tests/ext4/005
>>>>  create mode 100644 tests/ext4/005.out
>>>
>>> What's ext4 specific about this test apart from the mkfs parameter?
>>> Shouldn't it be generic and so test all the filesystems behave the
>>> same?  i.e. when someone then runs
>>>
>>> # MKFS_OPTIONS="-b size=1k -O ^extents" ./check -g auto
>>>
>>> That will exercise this specific regression fix, not to mention give
>>> much, much better test coverage of that configuration than just
>>> making a single test use that config...
>>>
>>> Cheers,
>>>
>>> Dave.
>>> -- 
>>> Dave Chinner
>>> david@fromorbit.com
>>
>> Hi, Dave,
>>
>> This test isn't completely generic bcause the output is dependent on the
>> block size. In particular, fpunch+fiemap will have different results
>> based on the block size:
>>
>> ----
>> # mkfs.ext3 -b1024 /dev/sdb1
>> # mount /dev/sdb1 /mnt/test
>> # xfs_io -f -c 'pwrite 0 8192' /mnt/test/a
>> # xfs_io -c 'fpunch 0 1024' /mnt/test/a
>> # xfs_io -c fiemap /mnt/test/a
>> /mnt/test/a:
>>         0: [0..1]: hole
>>         1: [2..15]: 1028..1041
>> # umount /mnt/test
>> # mkfs.ext3 -b4096 /dev/sdb1
>> # mount /dev/sdb1 /mnt/test
>> # xfs_io -f -c 'pwrite 0 8192' /mnt/test/a
>> # xfs_io -c 'fpunch 0 1024' /mnt/test/a
>> # xfs_io -c fiemap /mnt/test/a
>> /mnt/test/a:
>>         0: [0..15]: 8192..8207
>> ----
>>
>> I could either remove the fiemap output from the test case and rely on
>> the md5sum or round all of the punches to some larger block size so it
>> will behave the same up to, say, 8k. Do either of those options sound
>> better?
>>
>> Alternatively, is there a good way to have block size-dependent test
>> output? Then we could have the test adapt to different block sizes and
>> cover these regressions at any block size, not just 1k.
> 
> Can you scale every operational offset by block size?  I think there are
> other tests which do this sort of thing - look at _filter_bmap in test
> xfs/194 maybe?
> 
> i.e. above you would do 'fpunch 0 $blocksize' not 'fpunch 0 1024'
> (or blocksize/4, or whatever your intent is)

Or, I was talking to Dave about adding a fs-block-units output format
for fiemap... which might make life a lot simpler in cases like this,
though you'd still have to scale the tested offsets by fs blocks, but
that's not too hard.

-Eric

--
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
Lukas Czerner Feb. 24, 2015, 10:11 a.m. UTC | #5 
On Tue, 24 Feb 2015, Dave Chinner wrote:

> Date: Tue, 24 Feb 2015 09:46:20 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Omar Sandoval <osandov@osandov.com>
> Cc: fstests@vger.kernel.org, linux-ext4@vger.kernel.org
> Subject: Re: [PATCH] ext4: add regression tests for ^extents punch hole
> 
> On Mon, Feb 23, 2015 at 02:39:36PM -0800, Omar Sandoval wrote:
> > Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
> > fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
> > ext4 filesystem with indirect blocks.
> > 
> > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> > ---
> >  tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/ext4/005.out |  29 ++++++++++++++
> >  tests/ext4/group   |   1 +
> >  3 files changed, 145 insertions(+)
> >  create mode 100755 tests/ext4/005
> >  create mode 100644 tests/ext4/005.out
> 
> What's ext4 specific about this test apart from the mkfs parameter?
> Shouldn't it be generic and so test all the filesystems behave the
> same?  i.e. when someone then runs
> 
> # MKFS_OPTIONS="-b size=1k -O ^extents" ./check -g auto
> 
> That will exercise this specific regression fix, not to mention give
> much, much better test coverage of that configuration than just
> making a single test use that config...
> 
> Cheers,
> 
> Dave.

Hi Dave,

it's not that long ago when we discussed very similar case, where
directly in the test itself the author would specify mkfs options. I
had the same comment as you have here and you argued that the test
was made specifically to test that mkfs option. I agree.

In this case it's the same. This test was specifically designed for
this combination of options, because that's where the ext4 bug was.

If we want to make the test generic and use those options just in
ext4 case, I am fine with that, but the mkfs options needs to be a
part of the test nevertheless.

Thanks!
-Lukas

> 
--
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
Dave Chinner Feb. 24, 2015, 11:31 a.m. UTC | #6 
On Tue, Feb 24, 2015 at 11:11:04AM +0100, Lukáš Czerner wrote:
> On Tue, 24 Feb 2015, Dave Chinner wrote:
> 
> > Date: Tue, 24 Feb 2015 09:46:20 +1100
> > From: Dave Chinner <david@fromorbit.com>
> > To: Omar Sandoval <osandov@osandov.com>
> > Cc: fstests@vger.kernel.org, linux-ext4@vger.kernel.org
> > Subject: Re: [PATCH] ext4: add regression tests for ^extents punch hole
> > 
> > On Mon, Feb 23, 2015 at 02:39:36PM -0800, Omar Sandoval wrote:
> > > Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
> > > fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
> > > ext4 filesystem with indirect blocks.
> > > 
> > > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> > > ---
> > >  tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/ext4/005.out |  29 ++++++++++++++
> > >  tests/ext4/group   |   1 +
> > >  3 files changed, 145 insertions(+)
> > >  create mode 100755 tests/ext4/005
> > >  create mode 100644 tests/ext4/005.out
> > 
> > What's ext4 specific about this test apart from the mkfs parameter?
> > Shouldn't it be generic and so test all the filesystems behave the
> > same?  i.e. when someone then runs
> > 
> > # MKFS_OPTIONS="-b size=1k -O ^extents" ./check -g auto
> > 
> > That will exercise this specific regression fix, not to mention give
> > much, much better test coverage of that configuration than just
> > making a single test use that config...
> > 
> > Cheers,
> > 
> > Dave.
> 
> Hi Dave,
> 
> it's not that long ago when we discussed very similar case, where
> directly in the test itself the author would specify mkfs options. I
> had the same comment as you have here and you argued that the test
> was made specifically to test that mkfs option. I agree.

The case I remember and was basing this off was commit 448efe1
("generic/017: Do not create file systems with different block
sizes") where you made the argument that we shouldn't be setting
mkfs parameters inside the test and instead those specific cases
would be tested by using test-wide mkfs parameters....

I don't recall any other discussion, so maybe you should remind me
of it....

Cheers,

Dave.
Lukas Czerner Feb. 24, 2015, 11:52 a.m. UTC | #7 
On Tue, 24 Feb 2015, Dave Chinner wrote:

> Date: Tue, 24 Feb 2015 22:31:08 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Omar Sandoval <osandov@osandov.com>, fstests@vger.kernel.org,
>     linux-ext4@vger.kernel.org
> Subject: Re: [PATCH] ext4: add regression tests for ^extents punch hole
> 
> On Tue, Feb 24, 2015 at 11:11:04AM +0100, Lukáš Czerner wrote:
> > On Tue, 24 Feb 2015, Dave Chinner wrote:
> > 
> > > Date: Tue, 24 Feb 2015 09:46:20 +1100
> > > From: Dave Chinner <david@fromorbit.com>
> > > To: Omar Sandoval <osandov@osandov.com>
> > > Cc: fstests@vger.kernel.org, linux-ext4@vger.kernel.org
> > > Subject: Re: [PATCH] ext4: add regression tests for ^extents punch hole
> > > 
> > > On Mon, Feb 23, 2015 at 02:39:36PM -0800, Omar Sandoval wrote:
> > > > Linux commit 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
> > > > fixes several bugs in the FALLOC_FL_PUNCH_HOLE implementation for an
> > > > ext4 filesystem with indirect blocks.
> > > > 
> > > > Signed-off-by: Omar Sandoval <osandov@osandov.com>
> > > > ---
> > > >  tests/ext4/005     | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/ext4/005.out |  29 ++++++++++++++
> > > >  tests/ext4/group   |   1 +
> > > >  3 files changed, 145 insertions(+)
> > > >  create mode 100755 tests/ext4/005
> > > >  create mode 100644 tests/ext4/005.out
> > > 
> > > What's ext4 specific about this test apart from the mkfs parameter?
> > > Shouldn't it be generic and so test all the filesystems behave the
> > > same?  i.e. when someone then runs
> > > 
> > > # MKFS_OPTIONS="-b size=1k -O ^extents" ./check -g auto
> > > 
> > > That will exercise this specific regression fix, not to mention give
> > > much, much better test coverage of that configuration than just
> > > making a single test use that config...
> > > 
> > > Cheers,
> > > 
> > > Dave.
> > 
> > Hi Dave,
> > 
> > it's not that long ago when we discussed very similar case, where
> > directly in the test itself the author would specify mkfs options. I
> > had the same comment as you have here and you argued that the test
> > was made specifically to test that mkfs option. I agree.
> 
> The case I remember and was basing this off was commit 448efe1
> ("generic/017: Do not create file systems with different block
> sizes") where you made the argument that we shouldn't be setting
> mkfs parameters inside the test and instead those specific cases
> would be tested by using test-wide mkfs parameters....
> 
> I don't recall any other discussion, so maybe you should remind me
> of it....

Here it is

http://www.spinics.net/lists/fstests/msg00073.html

specifically your paragraph:

"No, I'm not advocating that at all. If the test has a specific
reason for overriding the user configuration, then it should.
Some configurations are rarely tested, and so having some tests that
exercise them even when other options are being tested is not a bad
thing. We catch problem with new changes much faster that way."

I do not really want to hold your words against you but the thing is
that I changed my mind since then and I do agree with that, because
it really is useful for testing specific cases where we already had
problems before. And this test is one of those cases.

-Lukas

> 
> Cheers,
> 
> Dave.
>
Dave Chinner Feb. 24, 2015, 12:49 p.m. UTC | #8 
On Tue, Feb 24, 2015 at 12:52:00PM +0100, Lukáš Czerner wrote:
> On Tue, 24 Feb 2015, Dave Chinner wrote:
> > > it's not that long ago when we discussed very similar case, where
> > > directly in the test itself the author would specify mkfs options. I
> > > had the same comment as you have here and you argued that the test
> > > was made specifically to test that mkfs option. I agree.
> > 
> > The case I remember and was basing this off was commit 448efe1
> > ("generic/017: Do not create file systems with different block
> > sizes") where you made the argument that we shouldn't be setting
> > mkfs parameters inside the test and instead those specific cases
> > would be tested by using test-wide mkfs parameters....
> > 
> > I don't recall any other discussion, so maybe you should remind me
> > of it....
> 
> Here it is
> 
> http://www.spinics.net/lists/fstests/msg00073.html
> 
> specifically your paragraph:
> 
> "No, I'm not advocating that at all. If the test has a specific
> reason for overriding the user configuration, then it should.
> Some configurations are rarely tested, and so having some tests that
> exercise them even when other options are being tested is not a bad
> thing. We catch problem with new changes much faster that way."

Ah, right, I said that was during the discussion about the commit I
quoted above. You convinced me that we shouldn't cater for special
cases like this and instead iterate mkfs/mount configurations.
On that basis I committed your patch to remove the special cases
from generic/017.

> I do not really want to hold your words against you but the thing is
> that I changed my mind since then and I do agree with that, because
> it really is useful for testing specific cases where we already had
> problems before. And this test is one of those cases.

<sigh>

/me shakes his head, wonders how other maintainers stay sane

I think the test should still be generic and block size independent,
but if you want to force ext4 to turn off the extents flag, then
use something like this:

[ "$FSTYP" = "ext4" ] && MKFS_OPTIONS="-O ^extents $MKFS_OPTIONS"

Cheers,

Dave.
Lukas Czerner Feb. 24, 2015, 2:58 p.m. UTC | #9 
On Tue, 24 Feb 2015, Dave Chinner wrote:

> Date: Tue, 24 Feb 2015 23:49:38 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Omar Sandoval <osandov@osandov.com>, fstests@vger.kernel.org,
>     linux-ext4@vger.kernel.org
> Subject: Re: [PATCH] ext4: add regression tests for ^extents punch hole
> 
> On Tue, Feb 24, 2015 at 12:52:00PM +0100, Lukáš Czerner wrote:
> > On Tue, 24 Feb 2015, Dave Chinner wrote:
> > > > it's not that long ago when we discussed very similar case, where
> > > > directly in the test itself the author would specify mkfs options. I
> > > > had the same comment as you have here and you argued that the test
> > > > was made specifically to test that mkfs option. I agree.
> > > 
> > > The case I remember and was basing this off was commit 448efe1
> > > ("generic/017: Do not create file systems with different block
> > > sizes") where you made the argument that we shouldn't be setting
> > > mkfs parameters inside the test and instead those specific cases
> > > would be tested by using test-wide mkfs parameters....
> > > 
> > > I don't recall any other discussion, so maybe you should remind me
> > > of it....
> > 
> > Here it is
> > 
> > http://www.spinics.net/lists/fstests/msg00073.html
> > 
> > specifically your paragraph:
> > 
> > "No, I'm not advocating that at all. If the test has a specific
> > reason for overriding the user configuration, then it should.
> > Some configurations are rarely tested, and so having some tests that
> > exercise them even when other options are being tested is not a bad
> > thing. We catch problem with new changes much faster that way."
> 
> Ah, right, I said that was during the discussion about the commit I
> quoted above. You convinced me that we shouldn't cater for special
> cases like this and instead iterate mkfs/mount configurations.
> On that basis I committed your patch to remove the special cases
> from generic/017.
> 
> > I do not really want to hold your words against you but the thing is
> > that I changed my mind since then and I do agree with that, because
> > it really is useful for testing specific cases where we already had
> > problems before. And this test is one of those cases.
> 
> <sigh>

Yes :)

> 
> /me shakes his head, wonders how other maintainers stay sane

I always though it was drugs. But in your case it might be burning rubber
and gas vapors ;)

> 
> I think the test should still be generic and block size independent,
> but if you want to force ext4 to turn off the extents flag, then
> use something like this:
> 
> [ "$FSTYP" = "ext4" ] && MKFS_OPTIONS="-O ^extents $MKFS_OPTIONS"

Ok, so let's look at this from a different angle. "-O ^extents"
applies for ext2 and ext3 file system. It would be sufficient for
this test to be generic if most people would be using ext4 driver
for ext2/3 file system which I am still not convinced about.

In ideal world we would not need this special case options and we
would just say this problem is for ext2/3 only so it'll be tested
with ext2 and ext3 file system and no special case for ext4 is
needed. However even when using ext4 driver, how many people are
regularly running tests on ext2/3 ?

On that basis I think that having this in the generic case

[ "$FSTYP" = "ext4" ] && MKFS_OPTIONS="-O ^extents $MKFS_OPTIONS"

is fair enough. But then again, what if we really want to run this
with extents as well ?

Omar, can you make the test generic and can this be reproduced on 4k
block size ? If not, can you make a generic reproducer which does
not depend on the block size ?

Also if we want to include the special case for ext4, we need to
have a function which allows us to alter the mkfs options without
completely overriding the user specified options. I think that there
is something like that for xfs, Omar can you do that for ext4 as
well ?

Thanks!
-Lukas

> 
> Cheers,
> 
> Dave.
>
Dave Chinner Feb. 24, 2015, 10:07 p.m. UTC | #10 
On Tue, Feb 24, 2015 at 03:58:06PM +0100, Lukáš Czerner wrote:
> On Tue, 24 Feb 2015, Dave Chinner wrote:
> > I think the test should still be generic and block size independent,
> > but if you want to force ext4 to turn off the extents flag, then
> > use something like this:
> > 
> > [ "$FSTYP" = "ext4" ] && MKFS_OPTIONS="-O ^extents $MKFS_OPTIONS"
> 
> Ok, so let's look at this from a different angle. "-O ^extents"
> applies for ext2 and ext3 file system. It would be sufficient for
> this test to be generic if most people would be using ext4 driver
> for ext2/3 file system which I am still not convinced about.
> 
> In ideal world we would not need this special case options and we
> would just say this problem is for ext2/3 only so it'll be tested
> with ext2 and ext3 file system and no special case for ext4 is
> needed. However even when using ext4 driver, how many people are
> regularly running tests on ext2/3 ?
> 
> On that basis I think that having this in the generic case
> 
> [ "$FSTYP" = "ext4" ] && MKFS_OPTIONS="-O ^extents $MKFS_OPTIONS"
> 
> is fair enough. But then again, what if we really want to run this
> with extents as well ?
> 
> Omar, can you make the test generic and can this be reproduced on 4k
> block size ? If not, can you make a generic reproducer which does
> not depend on the block size ?
> 
> Also if we want to include the special case for ext4, we need to
> have a function which allows us to alter the mkfs options without
> completely overriding the user specified options. I think that there
> is something like that for xfs, Omar can you do that for ext4 as
> well ?

It's built into the _scratch_mkfs_xfs wrapper, where if the test
supplies extra options and that conflicts with the CLI supplied
options it drops the CLI specific options and just uses the test
options.

I've mentioned this specificly in the past, too. i.e. that all
_scratch_mkfs_$FSTYP wrappers should be handling CLI vs test
specific options like this.... :/

Cheers,

Dave.
Theodore Ts'o Feb. 25, 2015, 12:24 a.m. UTC | #11 
On Tue, Feb 24, 2015 at 11:49:38PM +1100, Dave Chinner wrote:
> 
> Ah, right, I said that was during the discussion about the commit I
> quoted above. You convinced me that we shouldn't cater for special
> cases like this and instead iterate mkfs/mount configurations.

I was in favor of iterating over mkfs/mount configurations myself --
and that's something I do, but it takes around 20 hours to run all of
the iterations, so it's not something I'm doing _all_ that often.

> I think the test should still be generic and block size independent,
> but if you want to force ext4 to turn off the extents flag, then
> use something like this:

I suspect our current generic fsstress and fsx tests would catch this
already, and what I need to do is to make sure I add ext3/1k to my
test configurations (currently I test an ext3 configuration, and a 1k
block configuration, but I don't currently test ext3/1k together).
That would probably round out my full test iteration to a very
pleasing 24 hours or so, which is fine, although I wouldn't want it to
take much longer than that.


> /me shakes his head, wonders how other maintainers stay sane

They're coming to take me away, ha-haaa.
They're coming to take me away, ho ho, he he , ha ha,
To the happy home with trees and flowers and chirping birds
And basket weavers who sit and smile
And twiddle their thumbs and toes
And they're coming to take me away, ha-haaa!

:-)

						- 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
diff mbox

Patch

diff --git a/tests/ext4/005 b/tests/ext4/005
new file mode 100755
index 0000000..d6fb6e9
--- /dev/null
+++ b/tests/ext4/005
@@ -0,0 +1,115 @@ 
+#! /bin/bash
+# FS QA Test No. 005
+#
+# Test fpunch on an ^extents ext4 filesystem.
+# Regression test for commit:
+# 6f30b7e37a82 (ext4: fix indirect punch hole corruption)
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2015 Omar Sandoval.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+. ./common/punch
+
+_write_blocks()
+{
+	$XFS_IO_PROG -f -c "pwrite $((1024 * $2)) $((1024 * ($3 - $2)))" $SCRATCH_MNT/$1 | _filter_xfs_io
+	_scratch_unmount
+	_scratch_mount
+}
+
+_punch_blocks()
+{
+	$XFS_IO_PROG -c "fpunch $((1024 * $2)) $((1024 * ($3 - $2)))" $SCRATCH_MNT/$1
+}
+
+_check_blocks()
+{
+	$XFS_IO_PROG -c 'fiemap -v' $SCRATCH_MNT/$1 | _filter_hole_fiemap
+	_md5_checksum $SCRATCH_MNT/$1
+}
+
+_supported_fs ext3 ext4
+_supported_os Linux
+_need_to_be_root
+_require_scratch
+_require_xfs_io_command "fiemap"
+_require_xfs_io_command "fpunch"
+
+rm -f $seqres.full
+
+NDIR=12
+ADDRS=$((1024 / 4))
+
+$MKFS_EXT4_PROG -F -b 1024 -O ^extents $SCRATCH_DEV >> $seqres.full 2>&1
+_scratch_mount || _fail "couldn't mount fs"
+
+# Bug 1: whole level of indirection is not freed when end is first block of a
+# level.
+_write_blocks testfile1 0 $((NDIR + ADDRS + ADDRS * ADDRS + 4))
+_punch_blocks testfile1 0 $((NDIR + ADDRS + ADDRS * ADDRS))
+_check_blocks testfile1
+
+# Bug 2: end is at higher level than start, end shared branch is not freed.
+_write_blocks testfile2 0 $((NDIR + ADDRS + 2 * ADDRS + 5))
+_punch_blocks testfile2 $((NDIR + ADDRS + 2 * ADDRS)) $((NDIR + ADDRS + 2 * ADDRS + 4))
+_punch_blocks testfile2 0 $((NDIR + ADDRS + 2 * ADDRS + 4))
+_check_blocks testfile2
+
+# Bug 3: start and end are within one level of indirection, extra blocks are
+# freed because partial branches don't converge. (This bug also masks the
+# remaining 2. That is, the test cases for 4 and 5 are also necessarily
+# affected by bug 3.)
+_write_blocks testfile3 0 $((NDIR + ADDRS + 2 * ADDRS))
+_punch_blocks testfile3 $((NDIR + ADDRS + ADDRS / 2)) $((NDIR + ADDRS + ADDRS))
+_check_blocks testfile3
+
+# Bug 4: start and end are within one level of indirection, top of start is not
+# freed.
+_write_blocks testfile4 0 $((NDIR + ADDRS + 4 * ADDRS))
+_punch_blocks testfile4 $((NDIR + ADDRS + ADDRS)) $((NDIR + ADDRS + ADDRS + 1))
+_punch_blocks testfile4 $((NDIR + ADDRS + ADDRS + 1)) $((NDIR + ADDRS + 3 * ADDRS))
+_check_blocks testfile4
+
+# Bug 5: start and end are within one level of indirection, extra blocks beyond
+# end of range are freed when end has top branch.
+_write_blocks testfile5 0 $((NDIR + ADDRS + 4 * ADDRS))
+_punch_blocks testfile5 $((NDIR + ADDRS + 3 * ADDRS)) $((NDIR + ADDRS + 3 * ADDRS + ADDRS / 2))
+_punch_blocks testfile5 $((NDIR + ADDRS + 2)) $((NDIR + ADDRS + 3 * ADDRS + 2))
+_check_blocks testfile5
+
+# success, all done
+status=0
+exit
diff --git a/tests/ext4/005.out b/tests/ext4/005.out
new file mode 100644
index 0000000..763b4bb
--- /dev/null
+++ b/tests/ext4/005.out
@@ -0,0 +1,29 @@ 
+QA output created by 005
+wrote 67387392/67387392 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+0: [0..131607]: hole
+1: [131608..131615]: extent
+1f9503670a2a125a2110d5ad02e8f3ec
+wrote 803840/803840 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+0: [0..1567]: hole
+1: [1568..1569]: extent
+f1a1a9c1a1cbdb7203b487a14628dad2
+wrote 798720/798720 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+0: [0..791]: extent
+1: [792..1047]: hole
+2: [1048..1559]: extent
+b03901363691382cbe8ece10d66488f3
+wrote 1323008/1323008 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+0: [0..1047]: extent
+1: [1048..2071]: hole
+2: [2072..2583]: extent
+78f879dda9cc67b7f543d27bb7a39882
+wrote 1323008/1323008 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+0: [0..539]: extent
+1: [540..2327]: hole
+2: [2328..2583]: extent
+32b0d65d038d8804fd36ee8aedeff65b
diff --git a/tests/ext4/group b/tests/ext4/group
index e7f1f2a..f6db1da 100644
--- a/tests/ext4/group
+++ b/tests/ext4/group
@@ -7,6 +7,7 @@ 
 002 auto quick prealloc
 003 auto quick
 004 auto dump
+005 auto quick
 271 auto rw quick
 301 aio dangerous ioctl rw stress
 302 aio dangerous ioctl rw stress