Re: [PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

Re: [PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

[Zhao Hongjiang]

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@redhat.com>
>> ---
>>   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.

And you can find more infomation in http://marc.info/?t=136123715300001&r=1&w=2 as i mentioned.
If this message is needed i'll add it in the patch v2.

Thanks
--Zhao Hongjiang

> Thanks
> --Rich
>
>


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

[Dave Chinner]

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@redhat.com>
> >> ---
> >>   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.

Perhaps the test should have more comments in it than "read this
URL" to explain what it is doing and what constitutes a failure?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

[Zhao Hongjiang]

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@redhat.com>
>>>> ---
>>>>   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.

> 
> 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!

Thanks,
Zhao Hongjiang
> 
> Cheers,
> 
> Dave.
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

[Dave Chinner]

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@redhat.com>
> >>>> ---
> >>>>   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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/3] xfstests: remove comments about creator in new

[Eryu Guan]

We have removed creator/owner info from each test case, remove the
'creator' comment in template too.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 new | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/new b/new
index 2bc1e8f..f712892 100755
--- a/new
+++ b/new
@@ -104,7 +104,7 @@ cat <<End-of-File >$id
 # Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 #-----------------------------------------------------------------------
 #
-# creator
+
 seq=\`basename \$0\`
 seqres=\$RESULT_DIR/\$seq
 echo "QA output created by \$seq"
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

[Eryu Guan]

1. add one space between # and test description
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@redhat.com>
---
 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.
 #
 # Based on a testcase from Li Zefan <lizefan@huawei.com>.
 #
@@ -25,10 +25,9 @@
 # Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 #-----------------------------------------------------------------------
 #
-# creator
-owner=zhaohongjiang@huawei.com
 
 seq=`basename $0`
+seqres=$RESULT_DIR/$seq
 echo "QA output created by $seq"
 
 status=1	# failure is the default!
@@ -40,8 +39,8 @@ _cleanup()
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
 # get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
+. ./common/rc
+. ./common/filter
 
 # real QA test starts here
 _supported_fs generic
@@ -71,6 +70,5 @@ _test_lseek
 
 # success, all done
 echo "*** done"
-rm -f $seq.full
 status=0
 exit
-- 
1.8.1.4

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

[Rich Johnston]

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@redhat.com>
> ---
>   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.

Thanks
--Rich

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfstests generic 310: fix common file path and other cleanups

[Eryu Guan]

On Mon, Apr 08, 2013 at 09:05:07AM -0500, 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.

Make sense, thanks for the review.
> 
> 
> 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@redhat.com>
> >---
> >  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.

I didn't look into the test deeply but I found the test seems to run for
hours on my 3.9-rc4 test box, not sure what goes wrong here.

Thanks,
Eryu
> 
> Thanks
> --Rich

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs