blktests: use patient module remover
v4 cover blktests: use patient module remover
This rebases the patient module remover series onto the current blktests tree and addresses all the feedback from v3 from a long time ago [0]. This came up recently as Swarna noticed the same flaky bug and I got reminded to re-send a new iteraiton.
We now have the modprobe --wait upstream so use that if avaiable.
The patient module remover addresses race conditions where module removal can fail due to userspace temporarily bumping the refcount (e.g., via blkdev_open() calls). If your version of kmod supports modprobe --wait, we use that. Otherwise we implement our own patient module remover.
[0] https://lore.kernel.org/all/[email protected]/T/#u
Changes in v4:
- Rebased onto current blktests master
- Fixed built-in module handling: _patient_rmmod() now checks for the existence of /sys/module/$module_sys/refcnt to detect built-in modules and returns success early (Shinichiro Kawasaki)
- Fixed timeout parameter timing issue: MODPROBE_PATIENT_RM_TIMEOUT_SECONDS is now evaluated dynamically inside _patient_rmmod() rather than at source time, allowing tests to override the timeout value (Shinichiro Kawasaki)
- Preserved SKIP_REASONS logic in _init_null_blk() when modprobe fails, so tests requiring loadable modules are properly skipped rather than failing (Shinichiro Kawasaki)
- Fixed typo in comment: "modprobe -p" -> "modprobe --wait" (Luis Chamberlain)
- Fixed path reference bug: used $module_sys instead of $module for /sys/module path checks (Luis Chamberlain)
- Combined nested while conditions for cleaner code style (Shinichiro Kawasaki)
- Removed redundant $module printing in error messages (Shinichiro Kawasaki)
- Updated to use --wait=TIMEOUT_MSEC syntax (with =) to match the upstream kmod implementation
Luis Chamberlain (2):
blktests: replace module removal with patient module removal tests/srp/rc: replace module removal with patient module removal
common/multipath-over-rdma | 11 +--- common/null_blk | 6 +- common/nvme | 9 +-- common/rc | 126 +++++++++++++++++++++++++++++++++++++ common/scsi_debug | 14 ++--- tests/srp/rc | 19 ++---- 6 files changed, 148 insertions(+), 37 deletions(-)
Signature · 1 line
2.51.0
PATCH v4 1/2 blktests: replace module removal with patient module removal
A long time ago, in a galaxy far, far away...
I ran into some odd scsi_debug false positives with fstests. This prompted me to look into them given these false positives prevents me from moving forward with establishing a test baseline with high number of cycles. That is, this stupid issue was prevening creating high confidence in testing.
I reported it [0] and exchanged some ideas with Doug. However, in the end, despite efforts to help things with scsi_debug there were still issues lingering which seemed to defy our expectations upstream. One of the last hanging fruit issues is and always has been that userspace expectations for proper module removal has been broken, so in the end I have demonstrated this is a generic issue [1].
Long ago a WAIT option for module removal was added... that was then removed as it was deemed not needed as folks couldn't figure out when these races happened. The races are actually pretty easy to trigger, it was just never properly documented. A simpe blkdev_open() will easily bump a module refcnt, and these days many thing scan do that sort of thing.
The proper solution is to implement then a patient module removal on kmod and that has been merged now as modprobe --wait=MSEC option. We need a work around to open code a similar solution for users of old versions of kmod. An open coded solution for fstests exists there for over a year now. This now provides the respective blktests implementation.
[0] https://bugzilla.kernel.org/show_bug.cgi?id=212337
[1] https://bugzilla.kernel.org/show_bug.cgi?id=214015
--- common/multipath-over-rdma | 11 +--- common/null_blk | 6 +- common/nvme | 9 +-- common/rc | 126 +++++++++++++++++++++++++++++++++++++ common/scsi_debug | 14 ++--- tests/srp/rc | 4 +- 6 files changed, 145 insertions(+), 25 deletions(-)
common/multipath-over-rdma +3 -8
@@ -4,6 +4,7 @@ # # Functions and global variables used by the srp tests. +. common/rc . common/shellcheck . common/null_blk
@@ -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 siw || return 1 } >>"$FULL" }
common/null_blk +4 -2
@@ -5,6 +5,7 @@ # null_blk helper functions. . common/shellcheck +. common/rc _have_null_blk() { _have_driver null_blk
@@ -38,7 +39,8 @@ _init_null_blk() { local args=("$@") if (( RUN_FOR_ZONED )); then args+=("zoned=1"); fi - if ! modprobe -r null_blk || ! modprobe null_blk "${args[@]}" ; then + _patient_rmmod null_blk || return 1 + if ! modprobe null_blk "${args[@]}"; then SKIP_REASONS+=("requires modular null_blk") return 1 fi
@@ -79,5 +81,5 @@ _configure_null_blk() { _exit_null_blk() { _remove_null_blk_devices udevadm settle - modprobe -r -q null_blk + _patient_rmmod null_blk }
common/nvme +5 -4
@@ -4,6 +4,7 @@ # nvme helper functions. . common/shellcheck +. common/rc def_traddr="127.0.0.1" def_adrfam="ipv4"
@@ -210,13 +211,13 @@ _cleanup_nvmet() { if [[ "${nvme_trtype}" == "fc" ]]; then _nvme_fcloop_del_lport "${def_local_wwnn}" "${def_local_wwpn}" - modprobe -rq nvme-fcloop 2>/dev/null + _patient_rmmod nvme-fcloop fi - modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null + _patient_rmmod nvme-"${nvme_trtype}" if [[ "${nvme_trtype}" != "loop" ]]; then - modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null + _patient_rmmod nvmet-"${nvme_trtype}" fi - modprobe -rq nvmet 2>/dev/null + _patient_rmmod nvmet if [[ "${nvme_trtype}" == "rdma" ]]; then stop_soft_rdma fi
common/rc +126 -0
129 lines not on this page · raw message
common/scsi_debug +5 -9
@@ -4,6 +4,8 @@ # # scsi_debug helper functions. +. common/rc + _have_scsi_debug() { _have_driver scsi_debug }
@@ -98,14 +100,8 @@ _init_scsi_debug() { args+=(zbc=host-managed zone_nr_conv=0) fi - if ! _unload_module scsi_debug 10; then - echo "Unloading scsi_debug failed" >&2 - return 1 - fi - if ! modprobe scsi_debug "${args[@]}"; then - echo "Loading scsi_debug ${args[*]} failed" >&2 - return 1 - fi + _patient_rmmod scsi_debug || return 1 + modprobe scsi_debug "${args[@]}" || return 1 udevadm settle
@@ -180,7 +176,7 @@ _exit_scsi_debug() { udevadm settle if _module_file_exists scsi_debug; then - _unload_module scsi_debug 10 + _patient_rmmod scsi_debug return fi
tests/srp/rc +2 -2
@@ -502,7 +502,7 @@ start_lio_srpt() { if modinfo ib_srpt | grep -q '^parm:[[:blank:]]*rdma_cm_port:'; then opts+=("rdma_cm_port=${srp_rdma_cm_port}") fi - _unload_module ib_srpt + _patient_rmmod ib_srpt modprobe ib_srpt "${opts[@]}" || return $? i=0 for r in "${vdev_path[@]}"; do
@@ -564,7 +564,7 @@ stop_lio_srpt() { target_core_file target_core_stgt target_core_user \ target_core_mod do - _unload_module $m 10 || return $? + _patient_rmmod $m || return $? done }
Signature · 1 line
2.51.0
Re: [PATCH v4 1/2] blktests: replace module removal with patient module removal
Quoted 6 lines · Luis Chamberlain
Long ago a WAIT option for module removal was added... that was then removed as it was deemed not needed as folks couldn't figure out when these races happened. The races are actually pretty easy to trigger, it was just never properly documented. A simpe
blkdev_open()will easily bump a module refcnt, and these days many thing scan do that sort of thing.
It would be appreciated if "thing" could be replaced with a more specific description of what actually happens.
Quoted 6 lines
The proper solution is to implement then a patient module removal on kmod and that has been merged now as modprobe --wait=MSEC option. We need a work around to open code a similar solution for users of old versions of kmod. An open coded solution for fstests exists there for over a year now. This now provides the respective blktests implementation.
How can it be concluded what the proper solution is without explaining the root cause first? Please add an explanation of the root cause. I assume that the root cause is that some references are dropped asynchronously after module removal has been requested for the first time?
common/rc _min() { · line 745
Quoted 6 lines
common/rc +6
-0+_has_modprobe_patient() +{ + modprobe --help >& /dev/null || return 1 + modprobe --help | grep -q "\-\-wait" || return 1 + return 0 +}
Please combine the above two modprobe invocations into a single invocation and leave out the superfluous return statements, e.g. as follows:
modprobe --help |& grep -q -- --wait
Additionally, wouldn't has_modprobe_wait be a better name for this function?
common/rc _min() { · line 748
Quoted 2 lines
common/rc +2
-0+# Check whether modprobe --wait is supported and set up the patient module +# remover command. This is evaluated at source time, so we need to handle
remover -> removal?
Quoted 5 lines
common/rc +5
-0+# 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()
common/rc _min() { · line 774
Quoted 10 lines
common/rc +10
-0+{ + local module=$1 + local refcnt=0 + + refcnt=$(cat "/sys/module/$module/refcnt" 2>/dev/null) + if [[ $? -ne 0 || $refcnt -eq 0 ]]; then + return 0 + fi + return 1 +}
The refcnt initializer can be removed safely and the return statements can be removed too.
common/rc _min() { · line 800
Quoted 4 lines
common/rc +4
-0+ # Check if module is built-in or not loaded + if [[ ! -d "/sys/module/$module_sys" ]]; then + return 0 + fi
Is the comment above the if-statement correct? /sys/module/${module_sys} is also created for built-in kernel drivers that set the .owner field, isn't it? See also the sysfs_create_link(... "module") calls in drivers/base/.
common/rc _min() { · line 826
Quoted 10 lines
common/rc +10
-0+ max_tries=$max_tries_max + + while [[ "$max_tries" != "0" ]]; do + if _patient_rmmod_check_refcnt "$module_sys"; then + refcnt_is_zero=1 + break + fi + sleep 1 + ((max_tries--)) + done
Has it been considered to use a for-loop for the above code? E.g. something like
for ((max_tries=max_tries_max; max_tries != 0; max_tries--)); do
...
done
Thanks,
Bart.
Re: [PATCH v4 1/2] blktests: replace module removal with patient module removal
Quoted 17 lines · Bart Van Assche
Quoted 6 lines · Luis Chamberlain
Long ago a WAIT option for module removal was added... that was then removed as it was deemed not needed as folks couldn't figure out when these races happened. The races are actually pretty easy to trigger, it was just never properly documented. A simpe
blkdev_open()will easily bump a module refcnt, and these days many thing scan do that sort of thing.It would be appreciated if "thing" could be replaced with a more specific description of what actually happens.
Quoted 6 lines
The proper solution is to implement then a patient module removal on kmod and that has been merged now as modprobe --wait=MSEC option. We need a work around to open code a similar solution for users of old versions of kmod. An open coded solution for fstests exists there for over a year now. This now provides the respective blktests implementation.
Quoted 5 lines
How can it be concluded what the proper solution is without explaining the root cause first? Please add an explanation of the root cause. I assume that the root cause is that some references are dropped asynchronously after module removal has been requested for the first time?
I agree that the clarification on the above points will improve the commit message. Said that, I do not think lack of the clarifications should delay the application of this series.
Swarna, Luis, if there is any URL to the "flaky bug" that Swaran faced, please share. I can add it as a Link tag to show another reason for this series.
[...]
Quoted 9 lines
common/rc _min() { · line 800
Quoted 4 lines
common/rc +4
-0+ # Check if module is built-in or not loaded + if [[ ! -d "/sys/module/$module_sys" ]]; then + return 0 + fiIs the comment above the if-statement correct?
/sys/module/${module_sys}is also created for built-in kernel drivers that set the .owner field, isn't it? See also the sysfs_create_link(... "module") calls indrivers/base/.
As far as I understand, there is a corner case that the /sys/module/X directories are not created for built-in kernel drivers, such as nvme-fc or nvmet-fc. When the driver does not have any parameter, the directory is not created, probably.
Other than these, I plan to reflect the comments by Bart to the v5 series that I'm preparing now. Thanks.
Re: [PATCH v4 1/2] blktests: replace module removal with patient module removal
Quoted 24 lines · Shinichiro Kawasaki
Quoted 17 lines · Bart Van Assche
Quoted 6 lines · Luis Chamberlain
Long ago a WAIT option for module removal was added... that was then removed as it was deemed not needed as folks couldn't figure out when these races happened. The races are actually pretty easy to trigger, it was just never properly documented. A simpe
blkdev_open()will easily bump a module refcnt, and these days many thing scan do that sort of thing.It would be appreciated if "thing" could be replaced with a more specific description of what actually happens.
Quoted 6 lines
The proper solution is to implement then a patient module removal on kmod and that has been merged now as modprobe --wait=MSEC option. We need a work around to open code a similar solution for users of old versions of kmod. An open coded solution for fstests exists there for over a year now. This now provides the respective blktests implementation.
Quoted 5 lines
How can it be concluded what the proper solution is without explaining the root cause first? Please add an explanation of the root cause. I assume that the root cause is that some references are dropped asynchronously after module removal has been requested for the first time?
Quoted 7 lines
I agree that the clarification on the above points will improve the commit message. Said that, I do not think lack of the clarifications should delay the application of this series.
Swarna, Luis, if there is any URL to the "flaky bug" that Swaran faced, please share. I can add it as a Link tag to show another reason for this series.
I donot have any URL to the bug, but below is the description for it:
I had seen a sporadic kernel hang while running block tests (block/003) when testing scsi driver with sector size 16k which is detailed in the testing description of the cover letter sent for RFC v1 series for scsi fix.
(link here- https://lore.kernel.org/all/[email protected]/ ). The fio task triggerd at test block/003 would race with udevs generated while stressing scsi debug module in test - block/001.
I havent been able to reproduce the hang in the current setup lately, so i pasted below the snippet of the dmesg i had captured when the hang occured with block/003 test:
55.240584] __mutex_lock.constprop.0+0x3fc/0xa60
[10755.241693] ? _raw_spin_unlock+0x15/0x30 [10755.242630] bdev_open+0x2ac/0x3d0 [10755.243446] ? __pfx_blkdev_open+0x10/0x10 [10755.244399] blkdev_open+0xc6/0x120 [10755.245217] do_dentry_open+0x242/0x430 [10755.246128] vfs_open+0x2a/0xe0 [10755.246876] path_openat+0x31e/0x12e0 [10755.247752] ? getname_flags.part.0+0x26/0x1c0 [10755.248777] do_filp_open+0xbf/0x170 [10755.249637] ? _raw_spin_unlock_irqrestore+0x23/0x40 [10755.250778] ? __create_object+0x5e/0x90 [10755.251695] ? preempt_count_add+0x47/0xa0 [10755.252645] ? __virt_addr_valid+0xf5/0x170 [10755.253636] ? __check_object_size+0x249/0x2d0 [10755.254670] ? alloc_fd+0x11a/0x180 [10755.255502] do_sys_openat2+0x70/0xd0 [10755.256360] __x64_sys_openat+0x69/0xa0 [10755.257276] do_syscall_64+0x50/0xb60 [10755.258138] entry_SYSCALL_64_after_hwframe+0x76/0x7e [10755.259294] RIP: 0033:0x7f22ceca49ee [10755.260141] RSP: 002b:00007ffc0a8b18e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000101 [10755.261864] RAX: ffffffffffffffda RBX: 00007f22cc568f00 RCX: 00007f22ceca49ee [10755.263489] RDX: 0000000000040002 RSI: 00007f22cb30d2b0 RDI: ffffffffffffff9c [10755.265102] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 [10755.266727] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000040002 [10755.268304] R13: 00007f22c3efa000 R14: 0000000000000000 R15: 000055b79b6f020a [10755.269724] </TASK> [10755.270209] INFO: task fio:109969 is blocked on a mutex likely owned by task systemd-udevd:404. [10755.271920] task:systemd-udevd state:D stack:0 pid:404 tgid:404 ppid:1 task_flags:0x400100 flags:0x00080003 [10755.273863] Call Trace: [10755.274324] <TASK> [10755.274729] __schedule+0x54c/0xc10 [10755.275372] ? _raw_spin_unlock_irqrestore+0x23/0x40 [10755.276249] schedule+0x26/0xd0 [10755.276782] schedule_timeout+0x71/0xf0 [10755.277432] ? __pfx_process_timeout+0x10/0x10 [10755.278167] io_schedule_timeout+0x4d/0x70 [10755.278845] __wait_for_common+0x99/0x1b0 [10755.279517] ? __pfx_io_schedule_timeout+0x10/0x10 [10755.280281] blk_execute_rq+0xeb/0x160 [10755.280857] scsi_execute_cmd+0x11e/0x430 [scsi_mod] [10755.281653] ? disk_scan_partitions+0x5d/0xf0 [10755.282319] ? __x64_sys_ioctl+0x92/0xe0 [10755.282914] sd_spinup_disk+0x102/0x4b0 [sd_mod] [10755.283624] ? _raw_spin_unlock_irqrestore+0x23/0x40 [10755.284341] sd_revalidate_disk+0xda/0x2710 [sd_mod]
Hope this helps.
Thanks
Swarna
Re: [PATCH v4 1/2] blktests: replace module removal with patient module removal
Quoted 42 lines · Swarna Prabhu
Quoted 24 lines · Shinichiro Kawasaki
Quoted 17 lines · Bart Van Assche
Quoted 6 lines · Luis Chamberlain
Long ago a WAIT option for module removal was added... that was then removed as it was deemed not needed as folks couldn't figure out when these races happened. The races are actually pretty easy to trigger, it was just never properly documented. A simpe
blkdev_open()will easily bump a module refcnt, and these days many thing scan do that sort of thing.It would be appreciated if "thing" could be replaced with a more specific description of what actually happens.
Quoted 6 lines
The proper solution is to implement then a patient module removal on kmod and that has been merged now as modprobe --wait=MSEC option. We need a work around to open code a similar solution for users of old versions of kmod. An open coded solution for fstests exists there for over a year now. This now provides the respective blktests implementation.
Quoted 5 lines
How can it be concluded what the proper solution is without explaining the root cause first? Please add an explanation of the root cause. I assume that the root cause is that some references are dropped asynchronously after module removal has been requested for the first time?
Quoted 7 lines
I agree that the clarification on the above points will improve the commit message. Said that, I do not think lack of the clarifications should delay the application of this series.
Swarna, Luis, if there is any URL to the "flaky bug" that Swaran faced, please share. I can add it as a Link tag to show another reason for this series.
I donot have any URL to the bug, but below is the description for it:
I had seen a sporadic kernel hang while running block tests (block/003) when testing scsi driver with sector size 16k which is detailed in the testing description of the cover letter sent for RFC v1 series for scsi fix.
(link here- https://lore.kernel.org/all/[email protected]/ ). The fio task triggerd at test block/003 would race with udevs generated while stressing scsi debug module in test - block/001.
Quoted 3 lines
I havent been able to reproduce the hang in the current setup lately, so i pasted below the snippet of the dmesg i had captured when the hang occured with block/003 test:
Swarna, thanks for the description. The problem you faced looks like the same problem that Luis described. I will amend the commit log to add the link to this e-mail thread for recording.
As Luis described, the problem looks having multiple aspects, and this patient module removal solution addresses one of the aspects. Another aspect I noticed was that lots of udev events influence following test cases. Similar problem was observed for loop/010 [*], and solution for loop/010 was disabling udev events during the testcase run. At this moment, I think and hope this patient module removal solution is enough for block/001, and there is no need to disable udev events in block/001, probably.
Re: [PATCH v4 1/2] blktests: replace module removal with patient module removal
In reply to Luis ChamberlainQuoted 29 lines · Luis Chamberlain
A long time ago, in a galaxy far, far away...
I ran into some odd
scsi_debugfalse positives with fstests. This prompted me to look into them given these false positives prevents me from moving forward with establishing a test baseline with high number of cycles. That is, this stupid issue was prevening creating high confidence in testing.I reported it [0] and exchanged some ideas with Doug. However, in the end, despite efforts to help things with
scsi_debugthere were still issues lingering which seemed to defy our expectations upstream. One of the last hanging fruit issues is and always has been that userspace expectations for proper module removal has been broken, so in the end I have demonstrated this is a generic issue [1].Long ago a WAIT option for module removal was added... that was then removed as it was deemed not needed as folks couldn't figure out when these races happened. The races are actually pretty easy to trigger, it was just never properly documented. A simpe
blkdev_open()will easily bump a module refcnt, and these days many thing scan do that sort of thing.The proper solution is to implement then a patient module removal on kmod and that has been merged now as modprobe --wait=MSEC option. We need a work around to open code a similar solution for users of old versions of kmod. An open coded solution for fstests exists there for over a year now. This now provides the respective blktests implementation.
Quoted 2 lines
[0] https://bugzilla.kernel.org/show_bug.cgi?id=212337
[1] https://bugzilla.kernel.org/show_bug.cgi?id=214015
Luis, thanks for the v4 series and sorry for this slow response. Please find my comments in line.
Quoted 4 lines
Reviewed-by Chaitanya Kulkarni <[email protected]>Signed-off-by Luis Chamberlain <[email protected]>Rebased-by Claude AI
This "Rebased-by" tag is interesting. But it does not look widely used and may not be so valuable to leave in the commit log.
Quoted 14 lines
--- common/multipath-over-rdma | 11 +--- common/null_blk | 6 +- common/nvme | 9 +-- common/rc | 126 +++++++++++++++++++++++++++++++++++++ common/scsi_debug | 14 ++--- tests/srp/rc | 4 +- 6 files changed, 145 insertions(+), 25 deletions(-)common/multipath-over-rdma +0
-0
common/multipath-over-rdma line 7
Quoted 4 lines
common/multipath-over-rdma +1
-0@@ -4,6 +4,7 @@ # # Functions and global variables used by the srp tests. +. common/rc
This commit created a number of dependencies between common/rc and other common/X. The dependency added to common/nvme triggered a shellcheck warning SC2178. This warning itself is not due to this commit, (actually, it is shellcheck bug), but now I think this shows less dependency is the better.
I suggest to add the new helper functions to not common/rc but the "check" script. This avoids the new dependencies. It also provide the chance to refactor _unload_module() in the "check" script.
Quoted 2 lines
common/multipath-over-rdma +0
-0. common/shellcheck . common/null_blk
[...]
common/rc _min() { · line 743
Quoted 13 lines
common/rc +5
-0@@ -736,3 +736,129 @@ _min() { done echo "$ret" } + +_has_modprobe_patient() +{ + modprobe --help >& /dev/null || return 1 + modprobe --help | grep -q "\-\-wait" || return 1
During test runs, the line above prints out the error message,
"grep: warning: stray \ before -"
repeatedly. The suggested change by Bart in his response will avoid the message.
common/rc _min() { · line 745
Quoted 2 lines
common/rc +2
-0+ return 0 +}
Including this part, I tried to move this common/rc changes into the "check" script, and did a few trial runs. It looks working good.
I'm now revising this series by myself to repost as v5 to reflect may comments as well as the comments by Bart. In case you want to respin this by yourself, please let me know. Thanks!
Re: [PATCH v4 1/2] blktests: replace module removal with patient module removal
Quoted 2 lines · Shinichiro Kawasaki
I'm now revising this series by myself to repost as v5 to reflect may comments as well as the comments by Bart.
Oh not at all, this series just collects dust in my queue so I get to it when I can, which is never lately so please feel free to take it on.
Luis
Re: [PATCH v4 1/2] blktests: replace module removal with patient module removal
In reply to Luis ChamberlainOn Nov 26, 2025 / 09:11, Luis Chamberlain wrote:
[...]
Quoted 4 lines
common/rc +0
-0
[...]
Quoted 24 lines
common/rc +24
-0+# Tries to wait patiently to remove a module by ensuring first +# the refcnt is 0 and then trying to remove the module over and over +# again within the time allowed. The timeout is configurable per test, just set +# MODPROBE_PATIENT_RM_TIMEOUT_SECONDS prior to calling this function. +# This applies to both cases where kmod supports the patient module remover +# (modprobe --wait) and where it does not. +# +# If your version of kmod supports modprobe --wait, we use that instead. +# Otherwise we have to implement a patient module remover ourselves. +_patient_rmmod() +{ + local module=$1 + local max_tries_max=$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS + 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//-/_} + + # Check if module is built-in or not loaded + if [[ ! -d "/sys/module/$module_sys" ]]; then + return 0
common/rc _min() { · line 809
Quoted 10 lines
common/rc +10
-0+ fi + + # Check if this is a built-in module (no refcnt file means built-in) + if [[ ! -f "/sys/module/$module_sys/refcnt" ]]; then + return 0 + fi + + if [[ -n $MODPROBE_HAS_WAIT ]]; then + local timeout_ms=$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000)) + modprobe -r --wait="${timeout_ms}" "$module"
While I evaluated this patch, I noticed that the modprobe command above can return zero status code, even when it fails to remove the target module. I will add a follow-up patch to the v5 series to check the reference count even when the command above success to catch such cases.
Also, I will replace the short option -r with the long option --remove for script readability and stability.
Quoted 6 lines
common/rc +6
-0+ mod_ret=$? + if [[ $mod_ret -ne 0 ]]; then + echo "kmod patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max returned $mod_ret" + fi + return $mod_ret + fi
PATCH v4 2/2 tests/srp/rc: replace module removal with patient module removal
In reply to Luis ChamberlainBart had put the remove_mpath_devs() call inside a loop because multipathd keeps running while the loop is ongoing and hence can modify paths while the loop is running. The races that multipathd can trigger with the module refcnt is precisely the sort of races which patient module removal is supposed to address.
--- tests/srp/rc | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
tests/srp/rc +3 -12
@@ -331,19 +331,10 @@ start_srp_ini() { # 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 $? } # Associate the LIO device with name $1/$2 with file $3 and SCSI serial $4.
Signature · 1 line
2.51.0