* [PATCH v3 1/4] nvmet-fcloop: Remove remote port from list when unlinking
2023-04-18 13:01 [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
@ 2023-04-18 13:01 ` Daniel Wagner
2023-04-18 13:01 ` [PATCH v3 2/4] nvmet-fcloop: Do not wait on completion when unregister fails Daniel Wagner
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Wagner @ 2023-04-18 13:01 UTC (permalink / raw)
To: linux-nvme
Cc: linux-kernel, linux-block, Sagi Grimberg, James Smart,
Chaitanya Kulkarni, Martin Belanger, Shinichiro Kawasaki,
Daniel Wagner
The remote port is removed too late from fcloop_nports list. Remove it
when port is unregistered.
This prevents a busy loop in fcloop_exit, because it is possible the
remote port is found in the list and thus we will never progress.
The kernel log will be spammed with
nvme_fcloop: fcloop_exit: Failed deleting remote port
nvme_fcloop: fcloop_exit: Failed deleting target port
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/target/fcloop.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index c780af36c1d4..629a90fe4306 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -997,11 +997,6 @@ fcloop_nport_free(struct kref *ref)
{
struct fcloop_nport *nport =
container_of(ref, struct fcloop_nport, ref);
- unsigned long flags;
-
- spin_lock_irqsave(&fcloop_lock, flags);
- list_del(&nport->nport_list);
- spin_unlock_irqrestore(&fcloop_lock, flags);
kfree(nport);
}
@@ -1358,6 +1353,8 @@ __unlink_remote_port(struct fcloop_nport *nport)
nport->tport->remoteport = NULL;
nport->rport = NULL;
+ list_del(&nport->nport_list);
+
return rport;
}
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 2/4] nvmet-fcloop: Do not wait on completion when unregister fails
2023-04-18 13:01 [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
2023-04-18 13:01 ` [PATCH v3 1/4] nvmet-fcloop: Remove remote port from list when unlinking Daniel Wagner
@ 2023-04-18 13:01 ` Daniel Wagner
2023-04-18 13:01 ` [PATCH v3 3/4] nvmet-fc: Do not wait in vain when unloading module Daniel Wagner
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Wagner @ 2023-04-18 13:01 UTC (permalink / raw)
To: linux-nvme
Cc: linux-kernel, linux-block, Sagi Grimberg, James Smart,
Chaitanya Kulkarni, Martin Belanger, Shinichiro Kawasaki,
Daniel Wagner
The nvme_fc_unregister_localport() returns an error code in case that
the locaport pointer is NULL or has already been unegisterd. localport is
is either in the ONLINE state (all resources allocated) or has already
been put into DELETED state.
In this case we will never receive an wakeup call and thus any caller
will hang, e.g. module unload.
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/target/fcloop.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 629a90fe4306..57b2d47cf640 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -1163,7 +1163,8 @@ __wait_localport_unreg(struct fcloop_lport *lport)
ret = nvme_fc_unregister_localport(lport->localport);
- wait_for_completion(&lport->unreg_done);
+ if (!ret)
+ wait_for_completion(&lport->unreg_done);
kfree(lport);
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 3/4] nvmet-fc: Do not wait in vain when unloading module
2023-04-18 13:01 [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
2023-04-18 13:01 ` [PATCH v3 1/4] nvmet-fcloop: Remove remote port from list when unlinking Daniel Wagner
2023-04-18 13:01 ` [PATCH v3 2/4] nvmet-fcloop: Do not wait on completion when unregister fails Daniel Wagner
@ 2023-04-18 13:01 ` Daniel Wagner
2023-04-18 13:01 ` [PATCH v3 4/4] nvmet-fc: Release reference on target port Daniel Wagner
2023-04-18 13:43 ` [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Wagner @ 2023-04-18 13:01 UTC (permalink / raw)
To: linux-nvme
Cc: linux-kernel, linux-block, Sagi Grimberg, James Smart,
Chaitanya Kulkarni, Martin Belanger, Shinichiro Kawasaki,
Daniel Wagner
When there is no controller to be deleted the module unload path will
still wait on the nvme_fc_unload_proceed completion. Because this will
will never happen the caller will hang forever.
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/host/fc.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 456ee42a6133..df85cf93742b 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3933,10 +3933,11 @@ static int __init nvme_fc_init_module(void)
return ret;
}
-static void
+static bool
nvme_fc_delete_controllers(struct nvme_fc_rport *rport)
{
struct nvme_fc_ctrl *ctrl;
+ bool cleanup = false;
spin_lock(&rport->lock);
list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
@@ -3944,21 +3945,28 @@ nvme_fc_delete_controllers(struct nvme_fc_rport *rport)
"NVME-FC{%d}: transport unloading: deleting ctrl\n",
ctrl->cnum);
nvme_delete_ctrl(&ctrl->ctrl);
+ cleanup = true;
}
spin_unlock(&rport->lock);
+
+ return cleanup;
}
-static void
+static bool
nvme_fc_cleanup_for_unload(void)
{
struct nvme_fc_lport *lport;
struct nvme_fc_rport *rport;
+ bool cleanup = false;
list_for_each_entry(lport, &nvme_fc_lport_list, port_list) {
list_for_each_entry(rport, &lport->endp_list, endp_list) {
- nvme_fc_delete_controllers(rport);
+ if (nvme_fc_delete_controllers(rport))
+ cleanup = true;
}
}
+
+ return cleanup;
}
static void __exit nvme_fc_exit_module(void)
@@ -3968,10 +3976,8 @@ static void __exit nvme_fc_exit_module(void)
spin_lock_irqsave(&nvme_fc_lock, flags);
nvme_fc_waiting_to_unload = true;
- if (!list_empty(&nvme_fc_lport_list)) {
- need_cleanup = true;
- nvme_fc_cleanup_for_unload();
- }
+ if (!list_empty(&nvme_fc_lport_list))
+ need_cleanup = nvme_fc_cleanup_for_unload();
spin_unlock_irqrestore(&nvme_fc_lock, flags);
if (need_cleanup) {
pr_info("%s: waiting for ctlr deletes\n", __func__);
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v3 4/4] nvmet-fc: Release reference on target port
2023-04-18 13:01 [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
` (2 preceding siblings ...)
2023-04-18 13:01 ` [PATCH v3 3/4] nvmet-fc: Do not wait in vain when unloading module Daniel Wagner
@ 2023-04-18 13:01 ` Daniel Wagner
2023-04-18 13:43 ` [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
4 siblings, 0 replies; 9+ messages in thread
From: Daniel Wagner @ 2023-04-18 13:01 UTC (permalink / raw)
To: linux-nvme
Cc: linux-kernel, linux-block, Sagi Grimberg, James Smart,
Chaitanya Kulkarni, Martin Belanger, Shinichiro Kawasaki,
Daniel Wagner
In case we return early out of __nvmet_fc_finish_ls_req() we still have
to release the reference on the target port.
Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
drivers/nvme/target/fc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 1ab6601fdd5c..df7d84aff843 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -359,6 +359,7 @@ __nvmet_fc_finish_ls_req(struct nvmet_fc_ls_req_op *lsop)
if (!lsop->req_queued) {
spin_unlock_irqrestore(&tgtport->lock, flags);
+ nvmet_fc_tgtport_put(tgtport);
return;
}
--
2.40.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 0/4] nvmet-fcloop: unblock module removal
2023-04-18 13:01 [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
` (3 preceding siblings ...)
2023-04-18 13:01 ` [PATCH v3 4/4] nvmet-fc: Release reference on target port Daniel Wagner
@ 2023-04-18 13:43 ` Daniel Wagner
2023-04-18 14:26 ` Daniel Wagner
2023-04-19 9:35 ` Sagi Grimberg
4 siblings, 2 replies; 9+ messages in thread
From: Daniel Wagner @ 2023-04-18 13:43 UTC (permalink / raw)
To: linux-nvme
Cc: linux-kernel, linux-block, Sagi Grimberg, James Smart,
Chaitanya Kulkarni, Martin Belanger, Shinichiro Kawasaki
On Tue, Apr 18, 2023 at 03:01:55PM +0200, Daniel Wagner wrote:
> nvme/041 (Create authenticated connections) [failed]
> nvme/042 (Test dhchap key types for authenticated connections) [failed]
> nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
> nvme/044 (Test bi-directional authentication) [failed]
> nvme/045 (Test re-authentication) [passed]
I suppose these should be disabled for fc as all this is tcp specific.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 0/4] nvmet-fcloop: unblock module removal
2023-04-18 13:43 ` [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
@ 2023-04-18 14:26 ` Daniel Wagner
2023-04-19 9:03 ` Daniel Wagner
2023-04-19 9:35 ` Sagi Grimberg
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Wagner @ 2023-04-18 14:26 UTC (permalink / raw)
To: linux-nvme
Cc: linux-kernel, linux-block, Sagi Grimberg, James Smart,
Chaitanya Kulkarni, Martin Belanger, Shinichiro Kawasaki
On Tue, Apr 18, 2023 at 03:43:22PM +0200, Daniel Wagner wrote:
> On Tue, Apr 18, 2023 at 03:01:55PM +0200, Daniel Wagner wrote:
> > nvme/041 (Create authenticated connections) [failed]
> > nvme/042 (Test dhchap key types for authenticated connections) [failed]
> > nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
> > nvme/044 (Test bi-directional authentication) [failed]
> > nvme/045 (Test re-authentication) [passed]
>
> I suppose these should be disabled for fc as all this is tcp specific.
After a fresh reboot the deleter tport, lport and rport trouble is back...
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
runtime 10.265s ... 10.365s
tests/nvme/rc: line 198: /sys/class/fcloop/ctl/del_target_port: No such file or directory
tests/nvme/rc: line 190: /sys/class/fcloop/ctl/del_local_port: No such file or directory
tests/nvme/rc: line 182: /sys/class/fcloop/ctl/del_remote_port: No such file or directory
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 0/4] nvmet-fcloop: unblock module removal
2023-04-18 14:26 ` Daniel Wagner
@ 2023-04-19 9:03 ` Daniel Wagner
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Wagner @ 2023-04-19 9:03 UTC (permalink / raw)
To: linux-nvme
Cc: linux-kernel, linux-block, Sagi Grimberg, James Smart,
Chaitanya Kulkarni, Martin Belanger, Shinichiro Kawasaki
On Tue, Apr 18, 2023 at 04:26:27PM +0200, Daniel Wagner wrote:
> On Tue, Apr 18, 2023 at 03:43:22PM +0200, Daniel Wagner wrote:
> > On Tue, Apr 18, 2023 at 03:01:55PM +0200, Daniel Wagner wrote:
> > > nvme/041 (Create authenticated connections) [failed]
> > > nvme/042 (Test dhchap key types for authenticated connections) [failed]
> > > nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
> > > nvme/044 (Test bi-directional authentication) [failed]
> > > nvme/045 (Test re-authentication) [passed]
> >
> > I suppose these should be disabled for fc as all this is tcp specific.
>
> After a fresh reboot the deleter tport, lport and rport trouble is back...
>
> nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
> runtime 10.265s ... 10.365s
> tests/nvme/rc: line 198: /sys/class/fcloop/ctl/del_target_port: No such file or directory
> tests/nvme/rc: line 190: /sys/class/fcloop/ctl/del_local_port: No such file or directory
> tests/nvme/rc: line 182: /sys/class/fcloop/ctl/del_remote_port: No such file or directory
Eventually, I figured the out the root problem. The modules got unloaded before
the resource were freed. This explains a lot of the nasty problems I saw.
Anyway, I posted an updated blktests fixes but I think we should still consider
these patches here.
https://lore.kernel.org/linux-nvme/20230419084757.24846-1-dwagner@suse.de/
BTW, the authentication tests fail for fc, but not for the rest. And after
reading up on it, it supposed to work on fc as well. So here we go first real
bugs found.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/4] nvmet-fcloop: unblock module removal
2023-04-18 13:43 ` [PATCH v3 0/4] nvmet-fcloop: unblock module removal Daniel Wagner
2023-04-18 14:26 ` Daniel Wagner
@ 2023-04-19 9:35 ` Sagi Grimberg
1 sibling, 0 replies; 9+ messages in thread
From: Sagi Grimberg @ 2023-04-19 9:35 UTC (permalink / raw)
To: Daniel Wagner, linux-nvme
Cc: linux-kernel, linux-block, James Smart, Chaitanya Kulkarni,
Martin Belanger, Shinichiro Kawasaki
>> nvme/041 (Create authenticated connections) [failed]
>> nvme/042 (Test dhchap key types for authenticated connections) [failed]
>> nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
>> nvme/044 (Test bi-directional authentication) [failed]
>> nvme/045 (Test re-authentication) [passed]
>
> I suppose these should be disabled for fc as all this is tcp specific.
Umm, no their not...
^ permalink raw reply [flat|nested] 9+ messages in thread