On Mon, Apr 18, 2022 at 09:39:06AM -0700, Bart Van Assche wrote: > 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_*' Try: find /sys/module/ -maxdepth 1 -type d | grep loop I get: /sys/module/nvme_loop Blame scripts/Makefile.lib which does the s|-|_| for KBUILD_MODNAME: name-fix-token = $(subst $(comma),_,$(subst -,_,$1)) name-fix = $(call stringify,$(call name-fix-token,$1)) basename_flags = -DKBUILD_BASENAME=$(call name-fix,$(basetarget)) modname_flags = -DKBUILD_MODNAME=$(call name-fix,$(modname)) \ -D__KBUILD_MODNAME=kmod_$(call name-fix-token,$(modname)) I'm thinking now it is best to do this hack here: diff --git a/common/rc b/common/rc index 67bba70..cff0eb2 100644 --- a/common/rc +++ b/common/rc @@ -390,7 +390,10 @@ _patient_rmmod_check_refcnt() # ourselves. _patient_rmmod() { - local module=$1 + # Since we are looking for a directory we must adopt the + # specific directory used by scripts/Makefile.lib for + # KBUILD_MODNAME + local module=$(echo $1 | sed 's|-|_|g') local max_tries_max=$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS local max_tries=0 local mod_ret=0 That would allow us to keep the old order, and in fact would be correct for the other targets as well. > > > > # 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. I suspect you ran into the issue of the refcnt being bumped by anything multipathd tried, and not being able to remove the module, but if it is adding *new* mpath devices that would seem like a bug which we'd need to address. The point to the patient module removal is to keep on trying until the recnt is 0 and if that fails wait and keep trying, until the the timeout. Anytihng userspace does, say multipathd does, like bdev_open(), would be yielded to. So I'd like to keep this change as it is exactly the sort of hack I am chasing after with this crusade. Let me know how you'd like to proceed so I can punt a v3. Luis