Re: [PATCH 4/4] f2fs/009: detect and repair nlink corruption

[Chao Yu <chao@kernel.org>]

On 3/11/25 03:48, Zorro Lang wrote:
> On Mon, Mar 10, 2025 at 05:59:23PM +0800, Chao Yu wrote:
>> On 3/10/25 16:00, Zorro Lang wrote:
>>> On Thu, Mar 06, 2025 at 04:18:09PM +0800, Chao Yu wrote:
>>>> This is a regression test to check whether fsck can handle corrupted
>>>> nlinks correctly, it uses inject.f2fs to inject nlinks w/ wrong value,
>>>> and expects fsck.f2fs can detect such corruption and do the repair.
>>>>
>>>> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>> ---
>>>
> 
> [snip]
> 
>>>> +	$F2FS_INJECT_PROG --node --mb i_links --nid $ino --val $nlink $SCRATCH_DEV >> $seqres.full
>>>> +	if [ $? != 0 ]; then
>>>> +		exit
>>>> +	fi
>>>> +
>>>> +	export FSCK_OPTIONS="-f"
>>>
>>> You've set below code in _repair_scratch_fs():
>>>
>>>     f2fs)
>>>         fsck -t $FSTYP -f $SCRATCH_DEV
>>>         ;;
>>>
>>> The FSCK_OPTIONS="-f" is useless.
>>>
>>>> +	_repair_scratch_fs >> $seqres.full
>>>> +	if [ $? != 1 ]; then
>>>
>>> What does $?=1 mean? Does $?=1 mean finding corruption and fixed, $?=0 mean no corruption?
>>
>> That's correct.
>>
>>>
>>> If you want to detect there's a corruption, then fix it, then check if it's fixed. How about
>>> calling _check_scratch_fs at first to get the corruption error you expect, then call
>>> _repair_scratch_fs to fix it. Then call _check_scratch_fs to make sure the corruption is
>>> fixed?
>>>
>>> Something likes (just a rough example)
>>>
>>> _check_scratch_fs >>$seqres.full 2>&1 && _fail "can't find corruption"
>>
>> You mean this?
>>
>> export FSCK_OPTIONS="--dry-run"
>> _check_scratch_fs >>$seqres.full 2>&1 || _fail "can't find corruption"
> 
> No,
> 
>>
>> We need to export FSCK_OPTIONS w/ "--dry-run", otherwise _check_scratch_fs
>> will be stuck once it detects corruption.
> 
> If so, you might need to give _check_scratch_fs (and _check_test_fs) a f2fs
> specific handling. Due to _check_scratch_fs aims to do "check" only,
> _repair_scratch_fs aims to do "repair", they have different target. When
> we call _check_scratch_fs, we hope it reports pass or corruption then return,
> neither "repair" nor "stuck". So if I understand correct, you might need:

Thank you for the explanation, now it's clear to me for use of those functions.

> 
> _check_scratch_fs()
> {
> 	case $FSTYP in
> 	...
> 	f2fs)
> 		FSCK_OPTIONS="--dry-run" _check_generic_filesystem $device

It seems not work for me, due to fsck can not accept long parameter
started w/ --? I didn't dig into this now.

fsck -t f2fs --dry-run /dev/vdb
fsck from util-linux 2.40.2

fsck -t f2fs  /dev/vdb -- --dry-run
fsck from util-linux 2.40.2
Info: Dry run
Info: MKFS version
...
Done: 0.071639 secs

So I used this in v2:

+    f2fs)
+        if [ "$FSCK_OPTIONS" == "--dry-run" ]; then
+            fsck -t $FSTYP $device -- $FSCK_OPTIONS >> $seqres.full 2>&1
+        else
+            _check_generic_filesystem $device
+        fi
+        ;;

Any suggestion? What about introducing _check_f2fs_filesystem and reuse
codes in _check_generic_filesystem as much as possible, then replace
fsck w/ $F2FS_FSCK_PROG in order to use --dry-run.

Thanks,

> 		;;
> 	...
> }
> 
> Or you have any better way to do f2fs check :)
> 
> Thanks,
> Zorro
> 
>>
>>> _repair_scratch_fs >> $seqres.full
>>> _check_scratch_fs
>>>
>>>> +		_fail "fsck can not detect and repair zero nlink corruption "$i
>>>> +		exit
>>>> +	fi
>>>> +
>>>> +	export FSCK_OPTIONS=""
>>>> +	_check_scratch_fs >> $seqres.full
>>>
>>> I think _check_scratch_fs outputs nothing if run passed, right?
>>>
>>> _check_scratch_fs calls _check_generic_filesystem for f2fs, the FSCK_OPTIONS
>>> is "null" by default, so above FSCK_OPTIONS="" is useless too.
>>>
>>>> +	if [ $? != 0 ]; then
>>>> +		_fail "fsck hasn't fixed nlink corruption "$i
>>>> +		exit
>>>> +	fi
>>>> +
>>>> +	_scratch_mount >> $seqres.full
>>>
>>> ">> $seqres.full" isn't necessary.
>>
>> Will update according to you comments, thanks a lot.
>>
>> Thanks,
>>
>>>
>>>> +	_scratch_unmount
>>>> +done
>>>> +
>>>> +echo "Silence is golden"
>>>> +
>>>> +status=0
>>>> +exit
>>>> diff --git a/tests/f2fs/009.out b/tests/f2fs/009.out
>>>> new file mode 100644
>>>> index 00000000..7e977155
>>>> --- /dev/null
>>>> +++ b/tests/f2fs/009.out
>>>> @@ -0,0 +1,2 @@
>>>> +QA output created by 009
>>>> +Silence is golden
>>>> -- 
>>>> 2.48.1
>>>>
>>>>
>>>
>>
>