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

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

On 11/16/21 09:29, Luis Chamberlain wrote:
> for a while now. More wider testig is welcomed.
                               ^^^^^^
                               testing?

> @@ -427,14 +428,8 @@ stop_soft_rdma() {
>   		      echo "$i ..."
>   		      rdma link del "${i}" || echo "Failed to remove ${i}"
>   		done
> -	if ! unload_module rdma_rxe 10; then
> -		echo "Unloading rdma_rxe failed"
> -		return 1
> -	fi
> -	if ! unload_module siw 10; then
> -		echo "Unloading siw failed"
> -		return 1
> -	fi
> +	_patient_rmmod rdma_rxe || return 1
> +	_patient_rmmod 1siw  || return 1

The above clearly has not been tested. There is an easy to spot typo in 
the above change. Please test your changes before publishing these.

> +	if [[ ! -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then

Please use -n instead of ! -z.

> +	if [[ -f /sys/module/$module/refcnt ]]; then

Has this patch been analyzed with 'make check'? I expect checkpatch to 
complain about missing double quotes for the above statement.

> +# Patiently tries to wait to remove a module by ensuring first
      ^^^^^^^^^^^^^^^^^^^^^^^
Tries to wait patiently?

> +	if [[ ! -z $MODPROBE_REMOVE_PATIENT ]]; then

Please use -n instead of ! -z and surround the variable expansion with 
double quotes.

> +	# If we have extra time left. Use the time left to now try to
> +	# persistently remove the module. We do this because although through

What is persistent module removal? Is that perhaps removing a module 
such that it cannot be loaded again?

>   	for m in ib_srpt iscsi_target_mod target_core_pscsi target_core_iblock \
>   			 target_core_file target_core_stgt target_core_user \
>   			 target_core_mod
> -	do
> -		unload_module $m 10 || return $?
> -	done
> +	_patient_rmmod $m || return $1

Please proofread your changes and test your changes before posting 
these. I think that both "do" and "done" should have been preserved above.

Thanks,

Bart.