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

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

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

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