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

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

On 4/15/22 18:49, Luis Chamberlain wrote:
> -	if ! modprobe -r null_blk || ! modprobe null_blk "$@" "${zoned}" ; then
> -		return 1
> -	fi
> +	_patient_rmmod null_blk || return 1
> +	modprobe null_blk "$@" "${zoned}" || 1

"1" is not a valid command. Should "|| 1" perhaps be changed into "|| 
return 1"?

> +_has_modprobe_patient()
> +{
> +	modprobe --help >& /dev/null || return 1
> +	modprobe --help | grep -q -1 "remove-patiently" || return 1
> +	return 0
> +}

I can't find the meaning of "-1" in the grep man page. Did I perhaps 
overlook something?

> +# checks the refcount and returns 0 if we can safely remove the module. rmmod
> +# does this check for us, but we can use this to also iterate checking for this
> +# refcount before we even try to remove the module. This is useful when using
> +# debug test modules which take a while to quiesce.
> +_patient_rmmod_check_refcnt()
> +{
> +	local module=$1
> +	local refcnt=0
> +
> +	if [[ -f "/sys/module/$module/refcnt" ]]; then
> +		refcnt=$(cat "/sys/module/$module/refcnt" 2>/dev/null)
> +		if [[ $? -ne 0 || $refcnt -eq 0 ]]; then
> +			return 0
> +		fi
> +		return 1
> +	fi
> +	return 0
> +}

Hmm ... why is the check for existence of the refcnt separate from 
reading the refcnt? I think that just reading the refcnt should be 
sufficient. Additionally, that will avoid the race where the module is 
unloaded after the check and before the refcnt is read.

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

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

Thanks,

Bart.