Re: [PATCH v2] blktests: replace module removal with patient module removal

[Bart Van Assche <bvanassche@acm.org>]

On 4/18/22 08:57, Luis Chamberlain wrote:
> On Fri, Apr 15, 2022 at 09:01:03PM -0700, Bart Van Assche wrote:
>> On 4/15/22 18:49, Luis Chamberlain wrote:
>>> -	modprobe -r nvme-"${nvme_trtype}" 2>/dev/null
>>> -	if [[ "${nvme_trtype}" != "loop" ]]; then
>>> -		modprobe -r nvmet-"${nvme_trtype}" 2>/dev/null
>>> -	fi
>>> -	modprobe -r nvmet 2>/dev/null
>>> +	if [[ "${nvme_trtype}" == "loop" ]]; then
>>> +		_patient_rmmod nvme_"${nvme_trtype}"
>>> +        else
>>> +                _patient_rmmod nvme-"${nvme_trtype}"
>>> +                _patient_rmmod nvmet-"${nvme_trtype}"
>>> +        fi
>>> +	_patient_rmmod nvmet 2>/dev/null
>>
>> The statement _patient_rmmod nvme-"${nvme_trtype}" occurs twice in the above
>> code. How about preserving the structure of the existing code such that that
>> statement only occurs once?
> 
> There is one call for nvme-"${nvme_trtype}", the other is for the
> underscore version, so there are no two calls.
> 
> Did I miss something?

It's only now that I see the underscore/hyphen difference in the if and else
branches. It is not clear to me why this behavior change has been introduced?
The following command produces no output on my test setup:

find /lib/modules -name 'nvme_*'

>>>    # Unload the SRP initiator driver.
>>>    stop_srp_ini() {
>>> -	local i
>>> -
>>>    	log_out
>>> -	for ((i=40;i>=0;i--)); do
>>> -		remove_mpath_devs || return $?
>>> -		unload_module ib_srp >/dev/null 2>&1 && break
>>> -		sleep 1
>>> -	done
>>> -	if [ -e /sys/module/ib_srp ]; then
>>> -		echo "Error: unloading kernel module ib_srp failed"
>>> -		return 1
>>> -	fi
>>> -	unload_module scsi_transport_srp || return $?
>>> +	remove_mpath_devs || return $?
>>> +	_patient_rmmod ib_srp || return 1
>>> +	_patient_rmmod scsi_transport_srp || return $?
>>>    }
>>
>> Removing the loop from around remove_mpath_devs is wrong. It is important
>> that that loop is preserved.
> 
> Why ? Can you test and verify?

If I remember correctly I put remove_mpath_devs call inside the loop because
multipathd keeps running while the loop is ongoing and hence can modify paths
while the loop is running.

Thanks,

Bart.