Message-ID: <4DD5C242.3010708@linux.vnet.ibm.com>
Date:	Thu, 19 May 2011 18:22:10 -0700
From:	Allison Henderson <achender@...ux.vnet.ibm.com>
To:	Dave Chinner <david@...morbit.com>
CC:	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Eric Sandeen <sandeen@...hat.com>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	xfs-oss <xfs@....sgi.com>
Subject: Re: [XFS Tests Punch Hole 2/3 v3] XFS TESTS: Add Fallocate Punch
 Hole Test Routines

On 5/19/2011 4:56 PM, Dave Chinner wrote:
> On Thu, May 19, 2011 at 11:26:23AM -0700, Allison Henderson wrote:
>> On 5/18/2011 6:31 PM, Dave Chinner wrote:
>>> On Wed, May 18, 2011 at 01:58:40PM -0700, Allison Henderson wrote:
>>>> This patch adds low level routines to common.punch
>>>> for populating test files and punching holes in them using
>>>> fallocate.  When a hole is punched, file is then analyzed to
>>>> verify that the hole was punched correctly.  These routines will
>>>> be used to test various corner cases in the new fallocate
>>>> punch hole tests.
>>>
>>> So what condition does this test cover that test 252 doesn't?
> ....
>>> ....
>>>
>>> I think maybe this test is trying to be too smart and do too much,
>>> and the verbosity is hurting my eyes. I'm giving up here because I
>>> think it overlaps greatly with test 252, and I can't see what
>>> addition failures this test would actually detect that fsx and 252
>>> wouldn't. If there are corner cases that 252 isn't covering that
>>> this test does, then I think they should be added to 252....
>>>
>>>
>>
>>
>> Hi there,
>>
>> Thx for the all the reviewing on this one.  Im not quite sure I
>> agree that the tests are analogous though.  I did some poking around
>> in xfsprogs which I believe is what test 252 is using to do perform
>> most of its operations.  I found the code where the hole gets
>> punched, but I didnt find any code that does any kind of analyzing
>> to verify that the hole was punched correctly.
>
> It's in the golden output - we use the "$map_cmd" output and filter
> it to the normalise the data/unwritten/hole pattern, and if that
> doesn't match the golden output, the test will fail. So it does
> indeed test that the hole is exactly where we expect it to be, just
> without needing the complex logic.
>
>> Maybe I over looked
>> it?  It kinda looks like the hole gets punched and it just checks
>> the return code (fpunch_f in io/prealloc.c right?).
>
> For the immediate exectution of the punch, yes. That code doesn't do
> the checking that there is a hole in the right place, though, which
> is what the golden output checks at the end of the test do.
>
>> The reason this concerns me is that because a lot of the bugs that I
>> worked out during development did not show up in the form of a bad
>> return code or kernel oops.  Initially the tests were not automated
>> as they are now.  They would just perform the operations and print
>> out info about the file, the fs, fragmentation etc, and I would just
>> go through the raw output to make sure that every thing added up, as
>> well as just looking for anything that was out of the ordinary.  To
>> be honest, I feel that I caught a lot more bugs before they started
>> just with a careful eye, than if I had just been watching return
>> codes.  The above routine was meant to automate that work for
>> xfstests, but sense I do not see anything in xfstests or xfsprogs
>> that is doing any kinda of analyzing, I cannot say that I think
>> removing this layer provides the same level of verification.
>
> Looking through the raw output in an automated fashion is exactly
> what the filter and golden image checks do.
>
>> Unfortunately it does sound like a lot of what is going would not
>> work on all file systems, but I would feel better if we at least
>> kept the hexdumps. The reason I'm diffing hexdumps in here is
>> because some files get quite large and can take a while to copy, but
>> if they are full of repeating data the hexdumps are small.
>
> You've got to read the files to produce hexdumps, md5sums or just
> plain diff the binary files, so there's no saving in what you
> propose anyway. IMO, it's just a poor way to compare two files. You
> should have a golden image, and compare the test file against the
> golden image. You don't need hexdump to do that, and the files. And
> given the files are small, there is no reason not to copy stuff
> around.
>
> The only thing that 252 does not do is compare the file contents to
> determine whether the zeros start and stop at the correct spot.
> Realistically, fsx will do a much better job of testing these corner
> cases from a data POV than any manual test could ever do. Hence
> it's quite valid to make the assumption that we don't need to test
> the data for zeros because we have other tests that do a better job
> of it...
>
> Remember that robust unit testing is not based around the concept of
> "we need to check everything in one test". It's based around the
> concept that a test should be as simple as possible and test one
> thing. Then you write another simple test to cover a different
> aspect of the same functionality, and the test suite as a whole then
> covers all the functionality needing to be verified.  fsx will cover
> the "user data being zeroed correctly" cases, test 252 covers the
> "hole being punched in the right place" cases. IOWs, there isn't One
> True Test to test hole punching is working correctly - it's the
> coverage provided by the entire suite that gives us the confidence
> that hole punching is working correctly.
>
>> They can
>> be placed in the golden output just the same I suppose.  As much as
>> I would like to include output about the extents, I do not know how
>> that would work since the file may be inherently fragmented
>> differently from test to test.
>
> That's the problem that the map output (extent) filtering fixes.
>
> And FWIW, there's a bunch of interesting hole punching tests in the
> DMAPI part of xfstests that is not normally run on mainline kernels
> (no dmapi support) because punching out extents is a primary
> functionality of a HSM and, as I've said before, a common place to
> find data corruption bugs. IOWs, XFS has had hole punch test
> coverage for a lot longer than the recent test cases have been
> around. Those are tests 145, 161, 175, 176 and 185.  If we want more
> robust hole punch coverage, taking those and making them run without
> needing dmapi or XFS specific interfaces would be a good place to
> start. We don't need to re-invent the wheel just to have generic
> tests....
>
> Cheers,
>
> Dave.


Hi Dave,

Thanks for all the explaining, I did not initially see how the tests 
were verifying the holes, but I think that is a sufficient test then.  I 
am ok with just adding the non overlapping tests, and I will take a peek 
at those older tests.

Also, there was one more test that I meant to be a part of this 
collection, but I was not finished with it at the time I submitted the 
patch for feedback.  Basically it checks to see if a hole can still be 
punched out when the disk is full.  In ext4 this is allowable because 
reserved space is used to allow the operation to proceed where it would 
have otherwise failed.  I'm not sure if this is also ext4 specific 
though.   Would this be another candidate for adding to 252?  Thx!

Allison Henderson

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists