| Message ID | 4c557308eb4e62752dc8b513495cb6d46ca5775d.1424730653.git.osandov@osandov.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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!
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 fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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, 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 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, 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.
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. >
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.
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. >
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.
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 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/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
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