[v2,6/7] common/multipath-over-rdma: don't retry module unload
diff mbox series

Message ID 20200806191518.593880-7-sagi@grimberg.me
State New
Headers show
Series
  • blktests: Add support to run nvme tests with tcp/rdma transports
Related show

Commit Message

Sagi Grimberg Aug. 6, 2020, 7:15 p.m. UTC
There is no need to retry module unload for rdma_rxe
and siw. This also creates a dependency on
tests/nvmeof/rc which prevents it from using in
other test subsystems.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 common/multipath-over-rdma | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chaitanya Kulkarni Aug. 7, 2020, 3:18 a.m. UTC | #1
On 8/6/20 12:15, Sagi Grimberg wrote:
> There is no need to retry module unload for rdma_rxe
> and siw. This also creates a dependency on
> tests/nvmeof/rc which prevents it from using in
> other test subsystems.
> 
> Signed-off-by: Sagi Grimberg<sagi@grimberg.me>

You might want to CC Bart for this patch.
Bart Van Assche Aug. 7, 2020, 2:03 p.m. UTC | #2
On 2020-08-06 12:15, Sagi Grimberg wrote:
> There is no need to retry module unload for rdma_rxe
> and siw. This also creates a dependency on
> tests/nvmeof/rc which prevents it from using in
> other test subsystems.

If it wouldn't be necessary to retry module unload I wouldn't have
introduced a loop.

How about moving the unload_module() function definitions from tests/srp/rc
and tests/nvmeof-mp/rc into common/rc instead?

Thanks,

Bart.
Sagi Grimberg Aug. 7, 2020, 5:23 p.m. UTC | #3
>> There is no need to retry module unload for rdma_rxe
>> and siw. This also creates a dependency on
>> tests/nvmeof/rc which prevents it from using in
>> other test subsystems.
> 
> If it wouldn't be necessary to retry module unload I wouldn't have
> introduced a loop.

I thought it was to work-around a driver issue as these drivers
traditionally had plenty of stability issues.

To be honest this retry loop to me indicated that either the
driver has a bug or the test. But maybe there is a need I
am not seeing.

> How about moving the unload_module() function definitions from tests/srp/rc
> and tests/nvmeof-mp/rc into common/rc instead?

Don't have an issue with that.
Bart Van Assche Aug. 7, 2020, 5:34 p.m. UTC | #4
On 2020-08-07 10:23, Sagi Grimberg wrote:
> 
>>> There is no need to retry module unload for rdma_rxe
>>> and siw. This also creates a dependency on
>>> tests/nvmeof/rc which prevents it from using in
>>> other test subsystems.
>>
>> If it wouldn't be necessary to retry module unload I wouldn't have
>> introduced a loop.
> 
> I thought it was to work-around a driver issue as these drivers
> traditionally had plenty of stability issues.
> 
> To be honest this retry loop to me indicated that either the
> driver has a bug or the test. But maybe there is a need I
> am not seeing.

That loop was introduced a long time ago. I haven't tried to root-cause
it but my guess is that the loop is necessary because the module refcount
only drops to zero a short time after the first attempt to unload these
kernel modules.

Bart.
Sagi Grimberg Aug. 7, 2020, 5:50 p.m. UTC | #5
>>>> There is no need to retry module unload for rdma_rxe
>>>> and siw. This also creates a dependency on
>>>> tests/nvmeof/rc which prevents it from using in
>>>> other test subsystems.
>>>
>>> If it wouldn't be necessary to retry module unload I wouldn't have
>>> introduced a loop.
>>
>> I thought it was to work-around a driver issue as these drivers
>> traditionally had plenty of stability issues.
>>
>> To be honest this retry loop to me indicated that either the
>> driver has a bug or the test. But maybe there is a need I
>> am not seeing.
> 
> That loop was introduced a long time ago. I haven't tried to root-cause
> it but my guess is that the loop is necessary because the module refcount
> only drops to zero a short time after the first attempt to unload these
> kernel modules.

Didn't see any of that in the nvme testing...

Patch
diff mbox series

diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
index 676d2837fb06..e54b2a96153c 100644
--- a/common/multipath-over-rdma
+++ b/common/multipath-over-rdma
@@ -457,7 +457,7 @@  stop_soft_rdma() {
 				fi
 			done
 	)
-	if ! unload_module rdma_rxe 10; then
+	if ! modprobe -r rdma_rxe; then
 		echo "Unloading rdma_rxe failed"
 		return 1
 	fi
@@ -469,7 +469,7 @@  stop_soft_rdma() {
 					echo "Failed to unbind the siw driver from ${i%_siw}"
 			done
 	)
-	if ! unload_module siw 10; then
+	if ! modprobe -r siw; then
 		echo "Unloading siw failed"
 		return 1
 	fi