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

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

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.

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