Re: [PATCH] check: Fix fs specfic imports when $FSTYPE!=$OLD_FSTYPE

["Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com>]

On 1/13/25 21:03, Ritesh Harjani (IBM) wrote:
> Zorro Lang <zlang@redhat.com> writes:
>
>> On Mon, Jan 13, 2025 at 02:22:20PM +0530, Nirjhar Roy (IBM) wrote:
>>> On 1/13/25 11:29, Zorro Lang wrote:
>>>> On Sun, Jan 12, 2025 at 03:21:51PM +0000, Nirjhar Roy (IBM) wrote:
>>>>> Bug Description:
>>>>>
>>>>> _test_mount function is failing with the following error:
>>>>> ./common/rc: line 4716: _xfs_prepare_for_eio_shutdown: command not found
>>>>> check: failed to mount /dev/loop0 on /mnt1/test
>>>>>
>>>>> when the second section in local.config file is xfs and the first section
>>>>> is non-xfs.
>>>>>
>>>>> It can be easily reproduced with the following local.config file
>>>>>
>>>>> [s2]
>>>>> export FSTYP=ext4
>>>>> export TEST_DEV=/dev/loop0
>>>>> export TEST_DIR=/mnt1/test
>>>>> export SCRATCH_DEV=/dev/loop1
>>>>> export SCRATCH_MNT=/mnt1/scratch
>>>>>
>>>>> [s1]
>>>>> export FSTYP=xfs
>>>>> export TEST_DEV=/dev/loop0
>>>>> export TEST_DIR=/mnt1/test
>>>>> export SCRATCH_DEV=/dev/loop1
>>>>> export SCRATCH_MNT=/mnt1/scratch
>>>>>
>>>>> ./check selftest/001
>>>>>
>>>>> Root cause:
>>>>> When _test_mount() is executed for the second section, the FSTYPE has
>>>>> already changed but the new fs specific common/$FSTYP has not yet
>>>>> been done. Hence _xfs_prepare_for_eio_shutdown() is not found and
>>>>> the test run fails.
>>>>>
>>>>> Fix:
>>>>> call _source_specific_fs $FSTYP at the correct call site of  _test_mount()
>>>>>
>>>>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>>>>> ---
>>>>>    check | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/check b/check
>>>>> index 607d2456..8cdbb68f 100755
>>>>> --- a/check
>>>>> +++ b/check
>>>>> @@ -776,6 +776,7 @@ function run_section()
>>>>>    	if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
>>>>>    		echo "RECREATING    -- $FSTYP on $TEST_DEV"
>>>>>    		_test_unmount 2> /dev/null
>>>>> +		[[ "$OLD_FSTYP" != "$FSTYP" ]] && _source_specific_fs $FSTYP
>>>> The _source_specific_fs is called when importing common/rc file:
>>>>
>>>>     # check for correct setup and source the $FSTYP specific functions now
>>>>     _source_specific_fs $FSTYP
>>>>
>>>>   From the code logic of check script:
>>>>
>>>>           if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
>>>>                   echo "RECREATING    -- $FSTYP on $TEST_DEV"
>>>>                   _test_unmount 2> /dev/null
>>>>                   if ! _test_mkfs >$tmp.err 2>&1
>>>>                   then
>>>>                           echo "our local _test_mkfs routine ..."
>>>>                           cat $tmp.err
>>>>                           echo "check: failed to mkfs \$TEST_DEV using specified options"
>>>>                           status=1
>>>>                           exit
>>>>                   fi
>>>>                   if ! _test_mount
>>>>                   then
>>>>                           echo "check: failed to mount $TEST_DEV on $TEST_DIR"
>>>>                           status=1
>>>>                           exit
>>>>                   fi
>>>>                   # TEST_DEV has been recreated, previous FSTYP derived from
>>>>                   # TEST_DEV could be changed, source common/rc again with
>>>>                   # correct FSTYP to get FSTYP specific configs, e.g. common/xfs
>>>>                   . common/rc
>>>>                   ^^^^^^^^^^^
>>>> we import common/rc at here.
>>>>
>>>> So I'm wondering if we can move this line upward, to fix the problem you
>>>> hit (and don't bring in regression :) Does that help?
>>>>
>>>> Thanks,
>>>> Zorro
>>> Okay so we can move ". common/rc" upward and then remove the following from
>>> "check" file:
>>>
>>>          if ! _test_mount
>>>          then
>>>              echo "check: failed to mount $TEST_DEV on $TEST_DIR"
>>>              status=1
>>>              exit
>>>          fi
>>>
>>> since . common/rc will call init_rc() in the end, which does a
>>> _test_mount(). Do you agree with this (Zorro and Ritesh)?
>>>
>>> I can make the changes and send a v2?
>> Hmm... the _init_rc doesn't do _test_mkfs,
> Yes, I had noticed that problem. So I felt sourcing fs specific file
> before _test_mkfs should be ok.
>
>> so you might need to do ". common/rc" after "_test_mkfs", rather than "_test_unmount".
>>
>> By checking the _init_rc, I think it can replace the _test_mount you metioned
>> above. Some details might need more testing, to make sure we didn't miss
>> anything wrong:)
> If moving . common/rc above _test_mount works, than that is a better
> approach than sourcing fs specific config file twice.

Yes, moving the ". common/rc" just after _test_mkfs and removing the 
_test_mount after fixes it the issue. I will do additional testing 
before sending a v2.

--NR

>
>
> -ritesh
>
>> Any review points from others?
>>
>> Thanks,
>> Zorro
>>
>>> --NR
>>>
>>>>
>>>>>    		if ! _test_mkfs >$tmp.err 2>&1
>>>>>    		then
>>>>>    			echo "our local _test_mkfs routine ..."
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>>
>>> -- 
>>> Nirjhar Roy
>>> Linux Kernel Developer
>>> IBM, Bangalore
>>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore