Re: [PATCH] xfs: change return value check to golden image check

Eric Sandeen
2016-02-19T17:16:49Z

On 2/19/16 11:07 AM, Zirong Lang wrote:
>
>
> ----- 原始邮件 -----
>> 发件人: "Zirong Lang" <zlang@redhat.com>
>> 收件人: "Eric Sandeen" <sandeen@sandeen.net>
>> 抄送: "Dave Chinner" <david@fromorbit.com>, fstests@vger.kernel.org, eguan@redhat.com
>> 发送时间: 星期六, 2016年 2 月 20日 上午 12:52:07
>> 主题: Re: [PATCH] xfs: change return value check to golden image check
>>
>>
>>
>> ----- 原始邮件 -----
>>> 发件人: "Eric Sandeen" <sandeen@sandeen.net>
>>> 收件人: "Zirong Lang" <zlang@redhat.com>, "Dave Chinner" <david@fromorbit.com>
>>> 抄送: fstests@vger.kernel.org, eguan@redhat.com
>>> 发送时间: 星期六, 2016年 2 月 20日 上午 12:35:08
>>> 主题: Re: [PATCH] xfs: change return value check to golden image check
>>>
>>>
>>>
>>> On 2/19/16 9:58 AM, Eric Sandeen wrote:
>>>>
>>>>
>>>> On 2/19/16 9:35 AM, Zirong Lang wrote:
>>>>>
>>>>>
>>>>> ----- 原始邮件 -----
>>>>>> 发件人: "Dave Chinner" <david@fromorbit.com>
>>>>>> 收件人: "Zorro Lang" <zlang@redhat.com>
>>>>>> 抄送: fstests@vger.kernel.org, eguan@redhat.com
>>>>>> 发送时间: 星期五, 2016年 2 月 19日 上午 9:33:16
>>>>>> 主题: Re: [PATCH] xfs: change return value check to golden image check
>>>>>>
>>>>>> On Fri, Feb 12, 2016 at 12:37:36AM +0800, Zorro Lang wrote:
>>>>>>> xfs/133 and xfs/138 use too much code to do "return value" check,
>>>>>>> it's not necessary. For the code can be more readable and clear,
>>>>>>> I change "return value" check to golden image check.
>>>>>>>
>>>>>>> Signed-off-by: Zorro Lang <zlang@redhat.com>
>>>>>>> ---
>>>>>>> tests/xfs/133 | 20 +++++++-------------
>>>>>>> tests/xfs/133.out | 7 +++++++
>>>>>>> tests/xfs/138 | 26 ++++++++++++--------------
>>>>>>> tests/xfs/138.out | 12 ++++++++++++
>>>>>>> 4 files changed, 38 insertions(+), 27 deletions(-)
>>>>>>
>>>>>> This cause a xfs/133 failure like this on my systems:
>>>>>>
>>>>>> --- tests/xfs/133.out 2016-02-19 10:40:57.043131919 +1100
>>>>>> +++ /home/dave/src/xfstests-dev/results//xfs/xfs/133.out.bad
>>>>>> 2016-02-19
>>>>>> 12:24:53.173589432 +1100
>>>>>> @@ -4,5 +4,6 @@
>>>>>> Filesystem Blocks Quota Limit Warn/Time Mounted on
>>>>>> SCRATCH_DEV 0 102400 204800 00 [--------] SCRATCH_MNT
>>>>>> === report command output ===
>>>>>> +(null) 0 0 0 00 [--------]
>>>>
>>>> I need to dig, but this may be a result of GETNEXTQUOTA additions to
>>>> xfs_quota.
>>>>
>>>> We can now find IDs on disk that don't exist in the user database, and
>>>> we would not have reported them before.
>>>>
>>>> Perhaps change the test to report ids not names, to debug it and see
>>>> which one it is finding?
>>>>
>>>> I'm guessing it's ID 0, but I have to think about whether that's correct
>>>> to show or not...
>>>
>>> Ok, with Zorro's help, we see that this is a result of GETNEXTQUOTA.
>>>
>>> With that in place, "report" shows all active quotas, skipping only
>>> if XFS_IS_DQUOT_UNINITIALIZED(). But project ID 0 has 4 inodes
>>> accounted for:
>>>
>>> # xfs_db -c "dquot -p 0" -c print /dev/...
>>> ...
>>> diskdq.bcount = 0
>>> diskdq.icount = 4
>>> diskdq.itimer = 0
>>> diskdq.btimer = 0
>>> ...
>>>
>>> We never reported ID 0 before, because it was not in the projects file.
>>> But it looks active, so GETNEXTQUOTA finds and returns it now.
>>>
>>> I'm not actually sure what the best way is to fix this; I was even on
>>> the fence about using GETNEXTQUOTA for project quotas at all, because
>>> we always have a local file of projects to iterate anyway.
>>>
>>> We could explicitly look up id 0 and not show it if it's not in the
>>> projects file.
>>>
>>> We could not use GETNEXTQUOTA in the kernel for project quotas.
>>>
>>> We could skip printing id 0 altogether in xfs_quota
>>>
>>> We could filter it out in the test ...
>>
>> Maybe the pquota 0 problem will effect other cases except xfs/133 (maybe not,
>> I haven't tested that). So if we think it's a case problem, we need to check
>> all cases which report/query xfs project quota.
>>
>> So I should wait for the decision about how to deal with GETNEXTQUOTA on
>> project quota.
>
> Hi Dave, Eric
>
> So What do should I do, for you can merge this patch?
>
> Send V2, add 'grep $qa_project'?
>
> Send V2, add project quota 0 output into 133.out ?
>
> Send another patch, add a common filter to filter project quota 0, and
> try to find and modify all related cases?

Let's decide what the right thing to do in kernelspace/userspace is first,
then we can adjust tests if needed.

-Eric