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
