On Tue, Apr 09, 2013 at 03:25:49PM +0800, Zhao Hongjiang wrote: > On 2013-4-9 14:40, Dave Chinner wrote: > > On Tue, Apr 09, 2013 at 02:16:55PM +0800, Zhao Hongjiang wrote: > >> On 2013/4/8 22:05, Rich Johnston wrote: > >>> Hi Eryu, > >>> > >>> Thanks for this cleanup patch. I was going to revert patch "bbaf78c0" which introduced test generic/310 but will wait and see if Zhao will provide more information which could be added to this patch. > >>> > >>> > >>> On 04/07/2013 05:39 AM, Eryu Guan wrote: > >>>> 1. add one space between # and test description > >>> > >>> The rest of the changes look good, sorry I missed them when I reviewed . > >>> > >>>> 2. remove creator/owner info > >>>> 3. fix common/rc and common/filter path so they can be sourced correctly > >>>> 4. no need to remove $seq.full cause it's not used(or if verbose output > >>>> is needed, $seqres.full should be used) > >>>> > >>>> Signed-off-by: Eryu Guan <eguan@xxxxxxxxxx> > >>>> --- > >>>> tests/generic/310 | 12 +++++------- > >>>> 1 file changed, 5 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/tests/generic/310 b/tests/generic/310 > >>>> index ef51422..35baa23 100644 > >>>> --- a/tests/generic/310 > >>>> +++ b/tests/generic/310 > >>>> @@ -1,8 +1,8 @@ > >>>> #! /bin/bash > >>>> # FS QA Test No. 310 > >>>> # > >>>> -#Check if there are two threads,one keeps calling read() or lseek(), and > >>>> -#the other calling readdir(), both on the same directory fd. > >>>> +# Check if there are two threads,one keeps calling read() or lseek(), and > >>>> +# the other calling readdir(), both on the same directory fd. > >>>> # > >>> > >>> Hi Zhao, > >>> > >>> I did see both threads running at the same time, but the more I > >>> look at this, the more I am a loss as to what this test is > >>> doing. > >>> > >>> Will you expand this a little please. I should have asked for > >>> more justification the first time I reviewed this. Please > >>> provide what bug this is testing or what failure/weakness this > >>> test exposes. If there is a commit this is related to, please > >>> reference it. > >>> > >> When I ran it on ext2, ext3 and ext4 which has dir_index feature > >> disabled, I got something like this: > >> > >> EXT3-fs error (device loop1): ext3_readdir: bad entry in directory > >> #34817: rec_len is \ smaller than minimal - offset=993, inode=0, > >> rec_len=0, name_len=0 EXT3-fs error \ (device loop1): > >> ext3_readdir: bad entry in directory #34817: rec_len is smaller > >> than \ minimal - offset=1009, inode=0, rec_len=0, name_len=0 > >> EXT3-fs error (device loop1): \ ext3_readdir: bad entry in > >> directory #34817: rec_len is smaller than minimal - \ offset=993, > >> inode=0, rec_len=0, name_len=0 EXT3-fs error (device loop1): \ > >> ext3_readdir: bad entry in directory #34817: rec_len is smaller > >> than minimal - \ offset=1009, inode=0, rec_len=0, name_len=0 ... > >> > >> If we configured errors=remount-ro, the filesystem will become > >> read-only. > > > > So what is the criteria for a test failure? The test body is only > > reading from the filesystem, so a ro,remount won't cause an obvious > > failure of the test. > > There haven't a obvious criteria for a test failure, you should see it > from dmesg while you run the test. Which means for most cases (i.e. automated test harnesses), a test failure will go unnoticed. The test *must* detect failures if it is to be useful as a regression test. That's the whole point of a regression test harness - that you don't have to do any extra work to determine if a test has passed or failed as it is determined for you by the harness. > > Perhaps the test should have more comments in it than "read this > > URL" to explain what it is doing and what constitutes a failure? > > Yes, i'll add more comments to explain it! Given that there is a failure that can be captured, then I'd suggest that this needs to be captured as well... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
