| Message ID | 20221221002922.1528013-1-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: update patien module remover usage | expand |
On Tue, Dec 20, 2022 at 04:29:22PM -0800, Luis Chamberlain wrote: > kmod now has support for the patient module remover but > it uses --wait instead of -p, and it does not have an option > to wait forever. So let's just deprecate the "forever" option, > and use update our code to use --wait. > > Since blktests is also getting support, and since they actually > use 'make check' with consistent shellcheck checks, embrace the > implementation there so we at least match solutions. That way if > there are bugs in one we can fix them in the other project as well. > The style changes are minor. > > A few functional changes brought forward from the solution embraced > by blktests > > * sanity check for the modules sysfs directory to replace "-" with > "_" and document why we do that > * do not run if the module does not exist or is not loaded, that's > the addition of: > [ ! -e "/sys/module/$module_sys" ] && return 0 > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > > Although the blktests patch is not yet merged its on its v3 now. > I *have not tested* this patch yet on fstests... once I do I'll > poke back here. > > README | 3 +-- > common/config | 31 +++++++++++++++++++------------ > common/module | 28 ++++++++++++---------------- > 3 files changed, 32 insertions(+), 30 deletions(-) > > diff --git a/README b/README > index 4c4f22f853de..b2d4744593f3 100644 > --- a/README > +++ b/README > @@ -222,8 +222,7 @@ Kernel/Modules related configuration: > test invocations. This assumes that the name of the module is the same > as FSTYP. > - Set MODPROBE_PATIENT_RM_TIMEOUT_SECONDS to specify the amount of time we > - should try a patient module remove. The default is 50 seconds. Set this > - to "forever" and we'll wait forever until the module is gone. > + should try a patient module remove. The default is 50 seconds. > - Set KCONFIG_PATH to specify your preferred location of kernel config > file. The config is used by tests to check if kernel feature is enabled. > > diff --git a/common/config b/common/config > index b2802e5e0af1..8bc6b3d2ae3f 100644 > --- a/common/config > +++ b/common/config > @@ -256,11 +256,15 @@ if [[ "$UDEV_SETTLE_PROG" == "" || ! -d /proc/net ]]; then > fi > export UDEV_SETTLE_PROG > > -# Set MODPROBE_PATIENT_RM_TIMEOUT_SECONDS to "forever" if you want the patient > -# modprobe removal to run forever trying to remove a module. > +_has_modprobe_patient() > +{ > + modprobe --help >& /dev/null || return 1 > + modprobe --help | grep -q "\-\-wait" || return 1 ^^^^ This might cause error output from grep, better to use "--", e.g: grep -q -- "--wait" > + return 0 > +} > + > MODPROBE_REMOVE_PATIENT="" > -modprobe --help >& /dev/null && modprobe --help | grep -q -1 "remove-patiently" > -if [[ $? -ne 0 ]]; then > +if ! _has_modprobe_patient; then > if [[ -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then > # We will open code our own implementation of patient module > # remover in fstests. Use a 50 second default. > @@ -268,22 +272,25 @@ if [[ $? -ne 0 ]]; then > fi > else > MODPROBE_RM_PATIENT_TIMEOUT_ARGS="" > - if [[ ! -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then > - if [[ "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" != "forever" ]]; then > - MODPROBE_PATIENT_RM_TIMEOUT_MS="$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000))" > - MODPROBE_RM_PATIENT_TIMEOUT_ARGS="-t $MODPROBE_PATIENT_RM_TIMEOUT_MS" > + if [[ -n "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then > + MODPROBE_PATIENT_RM_TIMEOUT_MS="$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000))" > + MODPROBE_RM_PATIENT_TIMEOUT_ARGS="--wait $MODPROBE_PATIENT_RM_TIMEOUT_MS" > + if [[ "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" == "forever" ]]; "then" missed. Others make sense to me. Better to give it a basic test, to make sure it works :) Thanks, Zorro > + echo "Warning: deprecated MODPROBE_PATIENT_RM_TIMEOUT_SECONDS forever setting" > + echo "Just set this to a high value if you want this to linger for a long time" > + exit 1 > fi > else > # We export MODPROBE_PATIENT_RM_TIMEOUT_SECONDS here for parity > - # with environments without support for modprobe -p, but we > + # with environments without support for modprobe --wait, but we > # only really need it exported right now for environments which > - # don't have support for modprobe -p to implement our own > + # don't have support for modprobe --wait to implement our own > # patient module removal support within fstests. > export MODPROBE_PATIENT_RM_TIMEOUT_SECONDS="50" > MODPROBE_PATIENT_RM_TIMEOUT_MS="$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000))" > - MODPROBE_RM_PATIENT_TIMEOUT_ARGS="-t $MODPROBE_PATIENT_RM_TIMEOUT_MS" > + MODPROBE_RM_PATIENT_TIMEOUT_ARGS="--wait $MODPROBE_PATIENT_RM_TIMEOUT_MS" > fi > - MODPROBE_REMOVE_PATIENT="modprobe -p $MODPROBE_RM_PATIENT_TIMEOUT_ARGS" > + MODPROBE_REMOVE_PATIENT="modprobe -r $MODPROBE_RM_PATIENT_TIMEOUT_ARGS" > fi > export MODPROBE_REMOVE_PATIENT > > diff --git a/common/module b/common/module > index 6efab71d348e..bd205dafeaff 100644 > --- a/common/module > +++ b/common/module > @@ -107,9 +107,9 @@ _patient_rmmod_check_refcnt() > # MODPROBE_PATIENT_RM_TIMEOUT_SECONDS prior to including this file. > # If you want this to try forever just set MODPROBE_PATIENT_RM_TIMEOUT_SECONDS > # to the special value of "forever". This applies to both cases where kmod > -# supports the patient module remover (modrobe -p) and where it does not. > +# supports the patient module remover (modrobe --wait) and where it does not. > # > -# If your version of kmod supports modprobe -p, we instead use that > +# If your version of kmod supports modprobe --wait, we instead use that > # instead. Otherwise we have to implement a patient module remover > # ourselves. > _patient_rmmod() > @@ -119,6 +119,12 @@ _patient_rmmod() > local max_tries=0 > local mod_ret=0 > local refcnt_is_zero=0 > + # Since we are looking for a directory we must adopt the > + # specific directory used by scripts/Makefile.lib for > + # KBUILD_MODNAME > + local module_sys=${module//-/_} > + > + [ ! -e "/sys/module/$module_sys" ] && return 0 > > if [[ ! -z $MODPROBE_REMOVE_PATIENT ]]; then > $MODPROBE_REMOVE_PATIENT $module > @@ -131,20 +137,13 @@ _patient_rmmod() > > max_tries=$max_tries_max > > - # We must use a string check as otherwise if max_tries is set to > - # "forever" and we don't use a string check we can end up skipping > - # entering this loop. > while [[ "$max_tries" != "0" ]]; do > - _patient_rmmod_check_refcnt $module > - if [[ $? -eq 0 ]]; then > + if _patient_rmmod_check_refcnt "$module_sys"; then > refcnt_is_zero=1 > break > fi > sleep 1 > - if [[ "$max_tries" == "forever" ]]; then > - continue > - fi > - let max_tries=$max_tries-1 > + ((max_tries--)) > done > > if [[ $refcnt_is_zero -ne 1 ]]; then > @@ -169,17 +168,14 @@ _patient_rmmod() > # https://bugzilla.kernel.org/show_bug.cgi?id=212337 > # https://bugzilla.kernel.org/show_bug.cgi?id=214015 > while [[ $max_tries != 0 ]]; do > - if [[ -d /sys/module/$module ]]; then > + if [[ -d /sys/module/$module_sys ]]; then > modprobe -r $module 2> /dev/null > mod_ret=$? > if [[ $mod_ret == 0 ]]; then > break; > fi > sleep 1 > - if [[ "$max_tries" == "forever" ]]; then > - continue > - fi > - let max_tries=$max_tries-1 > + ((max_tries--)) > else > break > fi > -- > 2.35.1 >
diff --git a/README b/README index 4c4f22f853de..b2d4744593f3 100644 --- a/README +++ b/README @@ -222,8 +222,7 @@ Kernel/Modules related configuration: test invocations. This assumes that the name of the module is the same as FSTYP. - Set MODPROBE_PATIENT_RM_TIMEOUT_SECONDS to specify the amount of time we - should try a patient module remove. The default is 50 seconds. Set this - to "forever" and we'll wait forever until the module is gone. + should try a patient module remove. The default is 50 seconds. - Set KCONFIG_PATH to specify your preferred location of kernel config file. The config is used by tests to check if kernel feature is enabled. diff --git a/common/config b/common/config index b2802e5e0af1..8bc6b3d2ae3f 100644 --- a/common/config +++ b/common/config @@ -256,11 +256,15 @@ if [[ "$UDEV_SETTLE_PROG" == "" || ! -d /proc/net ]]; then fi export UDEV_SETTLE_PROG -# Set MODPROBE_PATIENT_RM_TIMEOUT_SECONDS to "forever" if you want the patient -# modprobe removal to run forever trying to remove a module. +_has_modprobe_patient() +{ + modprobe --help >& /dev/null || return 1 + modprobe --help | grep -q "\-\-wait" || return 1 + return 0 +} + MODPROBE_REMOVE_PATIENT="" -modprobe --help >& /dev/null && modprobe --help | grep -q -1 "remove-patiently" -if [[ $? -ne 0 ]]; then +if ! _has_modprobe_patient; then if [[ -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then # We will open code our own implementation of patient module # remover in fstests. Use a 50 second default. @@ -268,22 +272,25 @@ if [[ $? -ne 0 ]]; then fi else MODPROBE_RM_PATIENT_TIMEOUT_ARGS="" - if [[ ! -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then - if [[ "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" != "forever" ]]; then - MODPROBE_PATIENT_RM_TIMEOUT_MS="$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000))" - MODPROBE_RM_PATIENT_TIMEOUT_ARGS="-t $MODPROBE_PATIENT_RM_TIMEOUT_MS" + if [[ -n "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then + MODPROBE_PATIENT_RM_TIMEOUT_MS="$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000))" + MODPROBE_RM_PATIENT_TIMEOUT_ARGS="--wait $MODPROBE_PATIENT_RM_TIMEOUT_MS" + if [[ "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" == "forever" ]]; + echo "Warning: deprecated MODPROBE_PATIENT_RM_TIMEOUT_SECONDS forever setting" + echo "Just set this to a high value if you want this to linger for a long time" + exit 1 fi else # We export MODPROBE_PATIENT_RM_TIMEOUT_SECONDS here for parity - # with environments without support for modprobe -p, but we + # with environments without support for modprobe --wait, but we # only really need it exported right now for environments which - # don't have support for modprobe -p to implement our own + # don't have support for modprobe --wait to implement our own # patient module removal support within fstests. export MODPROBE_PATIENT_RM_TIMEOUT_SECONDS="50" MODPROBE_PATIENT_RM_TIMEOUT_MS="$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000))" - MODPROBE_RM_PATIENT_TIMEOUT_ARGS="-t $MODPROBE_PATIENT_RM_TIMEOUT_MS" + MODPROBE_RM_PATIENT_TIMEOUT_ARGS="--wait $MODPROBE_PATIENT_RM_TIMEOUT_MS" fi - MODPROBE_REMOVE_PATIENT="modprobe -p $MODPROBE_RM_PATIENT_TIMEOUT_ARGS" + MODPROBE_REMOVE_PATIENT="modprobe -r $MODPROBE_RM_PATIENT_TIMEOUT_ARGS" fi export MODPROBE_REMOVE_PATIENT diff --git a/common/module b/common/module index 6efab71d348e..bd205dafeaff 100644 --- a/common/module +++ b/common/module @@ -107,9 +107,9 @@ _patient_rmmod_check_refcnt() # MODPROBE_PATIENT_RM_TIMEOUT_SECONDS prior to including this file. # If you want this to try forever just set MODPROBE_PATIENT_RM_TIMEOUT_SECONDS # to the special value of "forever". This applies to both cases where kmod -# supports the patient module remover (modrobe -p) and where it does not. +# supports the patient module remover (modrobe --wait) and where it does not. # -# If your version of kmod supports modprobe -p, we instead use that +# If your version of kmod supports modprobe --wait, we instead use that # instead. Otherwise we have to implement a patient module remover # ourselves. _patient_rmmod() @@ -119,6 +119,12 @@ _patient_rmmod() local max_tries=0 local mod_ret=0 local refcnt_is_zero=0 + # Since we are looking for a directory we must adopt the + # specific directory used by scripts/Makefile.lib for + # KBUILD_MODNAME + local module_sys=${module//-/_} + + [ ! -e "/sys/module/$module_sys" ] && return 0 if [[ ! -z $MODPROBE_REMOVE_PATIENT ]]; then $MODPROBE_REMOVE_PATIENT $module @@ -131,20 +137,13 @@ _patient_rmmod() max_tries=$max_tries_max - # We must use a string check as otherwise if max_tries is set to - # "forever" and we don't use a string check we can end up skipping - # entering this loop. while [[ "$max_tries" != "0" ]]; do - _patient_rmmod_check_refcnt $module - if [[ $? -eq 0 ]]; then + if _patient_rmmod_check_refcnt "$module_sys"; then refcnt_is_zero=1 break fi sleep 1 - if [[ "$max_tries" == "forever" ]]; then - continue - fi - let max_tries=$max_tries-1 + ((max_tries--)) done if [[ $refcnt_is_zero -ne 1 ]]; then @@ -169,17 +168,14 @@ _patient_rmmod() # https://bugzilla.kernel.org/show_bug.cgi?id=212337 # https://bugzilla.kernel.org/show_bug.cgi?id=214015 while [[ $max_tries != 0 ]]; do - if [[ -d /sys/module/$module ]]; then + if [[ -d /sys/module/$module_sys ]]; then modprobe -r $module 2> /dev/null mod_ret=$? if [[ $mod_ret == 0 ]]; then break; fi sleep 1 - if [[ "$max_tries" == "forever" ]]; then - continue - fi - let max_tries=$max_tries-1 + ((max_tries--)) else break fi
kmod now has support for the patient module remover but it uses --wait instead of -p, and it does not have an option to wait forever. So let's just deprecate the "forever" option, and use update our code to use --wait. Since blktests is also getting support, and since they actually use 'make check' with consistent shellcheck checks, embrace the implementation there so we at least match solutions. That way if there are bugs in one we can fix them in the other project as well. The style changes are minor. A few functional changes brought forward from the solution embraced by blktests * sanity check for the modules sysfs directory to replace "-" with "_" and document why we do that * do not run if the module does not exist or is not loaded, that's the addition of: [ ! -e "/sys/module/$module_sys" ] && return 0 Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- Although the blktests patch is not yet merged its on its v3 now. I *have not tested* this patch yet on fstests... once I do I'll poke back here. README | 3 +-- common/config | 31 +++++++++++++++++++------------ common/module | 28 ++++++++++++---------------- 3 files changed, 32 insertions(+), 30 deletions(-)