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.
