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

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

On 1/13/25 18:41, Zorro Lang wrote:
> 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, so you might need to do
> ". common/rc" after "_test_mkfs", rather than "_test_unmount".
Yes, we should place ". common/rc" after, _test_mkfs.
>
> 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:)
Yes, makes sense.
>
> 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