Re: [PATCH] xfs/097: Remove wrong broken assignment operation

[Yang Xu <xuyang2018.jy@cn.fujitsu.com>]

on 2019/10/22 9:55, Darrick J. Wong wrote:
> On Tue, Oct 22, 2019 at 09:49:48AM +0800, Yang Xu wrote:
>>
>>
>> on 2019/10/21 23:50, Darrick J. Wong wrote:
>>> On Mon, Oct 21, 2019 at 08:09:39PM +0800, Yang Xu wrote:
>>>>
>>>>
>>>>    on 2019/10/15 14:27, Yang Xu wrote:
>>>>>
>>>>>
>>>>> on 2019/10/15 0:39, Darrick J. Wong wrote:
>>>>>> On Tue, Oct 08, 2019 at 10:39:59AM +0800, Yang Xu wrote:
>>>>>>>
>>>>>>>
>>>>>>> on 2019/10/07 23:12, Darrick J. Wong wrote:
>>>>>>>> On Mon, Oct 07, 2019 at 03:15:15PM +0800, Yang Xu wrote:
>>>>>>>>> On old kernel, since commit ded188b8609 ("xfs: Fix the
>>>>>>>>> situation that mount
>>>>>>>>> operation rejects corrupted XFS") running this case got
>>>>>>>>> the mismatched output,
>>>>>>>>> as below:
>>>>>>>>
>>>>>>>> But why did the output mismatch?  Did the fs heal itself?  Did
>>>>>>>> allocating 5 more files somehow avoid touching the finobt?  Is the
>>>>>>>> assignment logic in the loop broken?
>>>>>>>
>>>>>>> The output mismatch because on old kernel, we can mount the
>>>>>>> corrupted xfs
>>>>>>> and touch action will be refused. so broken is equal to 0.
>>>>>>> The fs doesn't heal ifself.
>>>>>>> allocating 5 more file will touch the finobt.
>>>>>>>
>>>>>>> You can see this url
>>>>>>> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=ded188b86096e2845e59dedae6050c7f254a96b
>>>>>>>
>>>>>>>
>>>>>>> eg xfs/087, they all delete "broken=0" before allocationg 5 more file.
>>>>>>> commit ded188b86 compatibled old kernel(permit mount and refuse
>>>>>>> touch) and
>>>>>>> new kernel(refuse mount) behavior on corrupted xfs.  Or, I misunderstand
>>>>>>> this case?
>>>>>>
>>>>>> How old is the kernel?  At some point (4.10, I think?) we added a patch
>>>>>> to reserve metadata blocks for future free inode btree expansion.  That
>>>>>> required us to count the blocks in the finobt, at which point xfs/097's
>>>>>> behavior changed such that the fs doesn't mount after the test corrupts
>>>>>> the finobt.
>>>>> I test this case on kernel-3.10.0-1062.el7.x86_64.
>>>>> I find the patch you said to reserve metadata blocks for future free
>>>>> inode btree expansion. This kernel doesn't backport this commit 76d771b4
>>>>> ("xfs: use per-AG reservations for the finobt"), so it permmits to
>>>>> mount.
>>>>>
>>>>> I can understand your meaning. But from xfstests commit ded188b86, it
>>>>> looks like refuse touch or refuse mount is acceptable for xfstests.
>>>>>
>>>>> Also, xfs/087 is a similar case but it sets broken=1 instead of broken
>>>>> =0.  Before this kernel commit 76d771b4, xfs/087(xfs/097) permits mount
>>>>> and refuse touch, after this commit, xfs/087(xfs/097) refuses mount.
>>>>> I think we should keep xfs/097 consistent with xfs/087. What do you
>>>>> think about it?
>>>>>
>>>>> ps:my patch is intend to fix the inconsistent of broken assignment
>>>>> operation that xfstests commit ded188b86 introduced.
>>>> Hi Darrick
>>>>     Do you have some questions on this patch?
>>>
>>> Does it still pass on upstreeam 5.4?
>>
>> Of course. It still can pass on upstream 5.4.
>>
>> --------------------------------------
>> echo "+ mount image && modify files"
>> broken=1
>> //on kernel with commit d771b4 ("xfs: use per-AG reservations for the
>> //finobt", it will not run into this if judgement, so broken=1.
>> //on kernel without this kernel commit, it will run into this if //judgement
>> and touch will be refused, so broken is still equal to 1.
>> if _try_scratch_mount >> $seqres.full 2>&1; then
>>          for x in `seq 65 70`; do
>>                  touch "${TESTFILE}.${x}" 2> /dev/null && broken=0
>>          done
>>          umount "${SCRATCH_MNT}"
>> fi
>> --------------------------------------
> 
> Ah, ok.  Looks good to me then.  Sorry I was a little slow on the
> uptake. :/
It doesn't matter. Thanks for your review.
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
>>
>>>
>>> --D
>>>
>>>> Hi Eryu
>>>>      What do you think about this patch(I only want to keep xfs/097 consistent
>>>> with xfs/087).
>>>>>
>>>>> Thanks
>>>>> Yang Xu
>>>>>
>>>>>>
>>>>>> --D
>>>>>>
>>>>>>>>
>>>>>>>> --D
>>>>>>>>
>>>>>>>>> -----------------------------------
>>>>>>>>>      + check fs
>>>>>>>>>      + corrupt image
>>>>>>>>>      + mount image && modify files
>>>>>>>>> -broken: 1
>>>>>>>>> +broken: 0
>>>>>>>>>      + repair fs
>>>>>>>>>      + mount image (2)
>>>>>>>>> ------------------------------------
>>>>>>>>>
>>>>>>>>> It fails because the broken is always equal to 0 when
>>>>>>>>> _try_scratch_mount
>>>>>>>>> succeed. So remove this wrong assignment operation.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
>>>>>>>>> ---
>>>>>>>>>      tests/xfs/097 | 2 --
>>>>>>>>>      1 file changed, 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/tests/xfs/097 b/tests/xfs/097
>>>>>>>>> index 1cb7d69c..20791738 100755
>>>>>>>>> --- a/tests/xfs/097
>>>>>>>>> +++ b/tests/xfs/097
>>>>>>>>> @@ -81,8 +81,6 @@ done
>>>>>>>>>      echo "+ mount image && modify files"
>>>>>>>>>      broken=1
>>>>>>>>>      if _try_scratch_mount >> $seqres.full 2>&1; then
>>>>>>>>> -
>>>>>>>>> -    broken=0
>>>>>>>>>          for x in `seq 65 70`; do
>>>>>>>>>              touch "${TESTFILE}.${x}" 2> /dev/null && broken=0
>>>>>>>>>          done
>>>>>>>>> -- 
>>>>>>>>> 2.18.1
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
>