nvme: fix stuck discovery
cover nvme: fix stuck discovery
Hi all,
when the user manages to abort a 'nvme discover' the resulting discovery controller is never removed, as it will try to reconnect for ever. And as the 'duplicate_connect' option is not given any further attempt will be returned with -EALREADY.
This patchset fixes this issue by a) disabling reconnection attempts and b) allowing for duplicate connections on the discovery controller.
As usual, comments and reviews are welcome.
Hannes Reinecke (3):
nvme: centralize discovery controller defaults
nvme: avoid reconnection for the discovery controller
nvme: allow duplicate connections to the discovery controller
drivers/nvme/host/core.c | 3 ++- drivers/nvme/host/fabrics.c | 12 ++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-)
Signature · 1 line
2.12.3
PATCH 1/3 nvme: centralize discovery controller defaults
When connecting to the discovery controller we have certain defaults to observe, so centralize them to avoid inconsistencies due to argument ordering.
--- drivers/nvme/host/fabrics.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvme/host/fabrics.c +4 -4
@@ -698,10 +698,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts, opts->discovery_nqn = !(strcmp (opts->subsysnqn,NVME_DISC_SUBSYS_NAME )); -if (opts->discovery_nqn) { - opts->kato =0 ; - opts->nr_io_queues =0 ; - }break ;case NVMF_OPT_TRADDR : p =match_strdup (args);
@@ -856,6 +852,10 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts, } } +if (opts->discovery_nqn) { + opts->kato =0 ; + opts->nr_io_queues =0 ; + }if (ctrl_loss_tmo <0 ) opts->max_reconnects =-1 ;else
Signature · 1 line
2.12.3
PATCH 1/3 nvme: centralize discovery controller defaults
Quoted 25 lines · Hannes Reinecke
When connecting to the discovery controller we have certain defaults to observe, so centralize them to avoid inconsistencies due to argument ordering.
Signed-off-by Hannes Reinecke <hare at suse.com>--- drivers/nvme/host/fabrics.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/nvme/host/fabrics.c +0
-4@@ -698,10 +698,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts, opts->discovery_nqn = !(strcmp (opts->subsysnqn,NVME_DISC_SUBSYS_NAME )); -if (opts->discovery_nqn) { - opts->kato =0 ; - opts->nr_io_queues =0 ; - }break ;case NVMF_OPT_TRADDR : p =match_strdup (args);
drivers/nvme/host/fabrics.c static int nvmf_parse_options(struct nvmf_ctrl_options *opts, · line 861
Quoted 10 lines
drivers/nvme/host/fabrics.c +4
-0@@ -856,6 +852,10 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts, } } +if (opts->discovery_nqn) { + opts->kato =0 ; + opts->nr_io_queues =0 ; + }if (ctrl_loss_tmo <0 ) opts->max_reconnects =-1 ;else
looks fine
-- james
PATCH 1/3 nvme: centralize discovery controller defaults
In reply to Hannes ReineckeQuoted 3 lines · Hannes Reinecke
When connecting to the discovery controller we have certain defaults to observe, so centralize them to avoid inconsistencies due to argument ordering.
Thanks, applied to nvme-4.18-2.
PATCH 2/3 nvme: avoid reconnection for the discovery controller
In reply to Hannes ReineckeIf the connection to the discovery controller fails for whatever reason we should not attempt to reconnect; rather the user should retry the discovery altogether.
--- drivers/nvme/host/fabrics.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/nvme/host/fabrics.c +3 -0
@@ -474,6 +474,9 @@ EXPORT_SYMBOL_GPL(nvmf_connect_io_queue);bool nvmf_should_reconnect (struct nvme_ctrl *ctrl) { +if (ctrl->opts->discovery_nqn) +return false; +if (ctrl->opts->max_reconnects !=-1 && ctrl->nr_reconnects < ctrl->opts->max_reconnects)return true;
Signature · 1 line
2.12.3
PATCH 2/3 nvme: avoid reconnection for the discovery controller
Quoted 14 lines · Hannes Reinecke
If the connection to the discovery controller fails for whatever reason we should not attempt to reconnect; rather the user should retry the discovery altogether.
Signed-off-by Hannes Reinecke <hare at suse.com>--- drivers/nvme/host/fabrics.c | 3 +++ 1 file changed, 3 insertions(+)drivers/nvme/host/fabrics.c +0
-0
drivers/nvme/host/fabrics.c EXPORT_SYMBOL_GPL(nvmf_connect_io_queue); · line 482
Quoted 9 lines
drivers/nvme/host/fabrics.c +3
-0@@ -474,6 +474,9 @@ EXPORT_SYMBOL_GPL(nvmf_connect_io_queue);bool nvmf_should_reconnect (struct nvme_ctrl *ctrl) { +if (ctrl->opts->discovery_nqn) +return false; +if (ctrl->opts->max_reconnects !=-1 && ctrl->nr_reconnects < ctrl->opts->max_reconnects)return true;
um no.? It should follow the same reconnect policies as anything other controller.?? There is no guarantee the event that triggered the discovery controller will do so again. Not all discovery controller connects are manually initiated by an admin.?? So failing the reconnect may mean you will never reconnect with the subsystems described by the discovery controller.
-- james
PATCH 2/3 nvme: avoid reconnection for the discovery controller
Quoted 24 lines · James Smart
Quoted 21 lines · Hannes Reinecke
If the connection to the discovery controller fails for whatever reason we should not attempt to reconnect; rather the user should retry the discovery altogether.
Signed-off-by Hannes Reinecke <hare at suse.com>---
?drivers/nvme/host/fabrics.c| 3 +++
? 1 file changed, 3 insertions(+)drivers/nvme/host/fabrics.c +3
-0@@ -474,6 +474,9 @@ EXPORT_SYMBOL_GPL(nvmf_connect_io_queue); ? bool nvmf_should_reconnect(struct nvme_ctrl *ctrl) ? { +??? if (ctrl->opts->discovery_nqn) +??????? return false; +????? if (
ctrl->opts->max_reconnects!= -1 && ?????????ctrl->nr_reconnects<ctrl->opts->max_reconnects)
Quoted 9 lines
Quoted 1 line
????????? return true;
um no.? It should follow the same reconnect policies as anything other controller.?? There is no guarantee the event that triggered the discovery controller will do so again. Not all discovery controller connects are manually initiated by an admin.?? So failing the reconnect may mean you will never reconnect with the subsystems described by the discovery controller.
Ah. Fair enough.
So we should be limiting the number of reconnects only when triggered by the admin. Should be doable; I'll be looking into it.
Cheers,
Hannes
PATCH 3/3 nvme: allow duplicate connections to the discovery controller
In reply to Hannes ReineckeThe whole point of the discovery controller is that it can accept multiple connections. Additionally the cmic field is not even defined for the discovery controller identify page.
--- drivers/nvme/host/core.c | 3 ++- drivers/nvme/host/fabrics.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
drivers/nvme/host/core.c +2 -1
@@ -2494,7 +2494,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)* Verify that the subsystem actually supports multiple * controllers, else bail out. */ -if (nvme_active_ctrls (found) && !(id->cmic & (1 <<1 ))) { +if (!ctrl->opts->discovery_nqn && +nvme_active_ctrls (found) && !(id->cmic & (1 <<1 ))) {dev_err (ctrl->device,"ignoring ctrl due to duplicate subnqn (%s).\n" , found->subnqn);
drivers/nvme/host/fabrics.c +1 -0
@@ -858,6 +858,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,if (opts->discovery_nqn) { opts->kato =0 ; opts->nr_io_queues =0 ; + opts->duplicate_connect = true; }if (ctrl_loss_tmo <0 ) opts->max_reconnects =-1 ;
Signature · 1 line
2.12.3
PATCH 3/3 nvme: allow duplicate connections to the discovery controller
Quoted 29 lines · Hannes Reinecke
The whole point of the discovery controller is that it can accept multiple connections. Additionally the cmic field is not even defined for the discovery controller identify page.
Signed-off-by Hannes Reinecke <hare at suse.com>--- drivers/nvme/host/core.c | 3 ++- drivers/nvme/host/fabrics.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +2
-1@@ -2494,7 +2494,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)* Verify that the subsystem actually supports multiple * controllers, else bail out. */ -if (nvme_active_ctrls (found) && !(id->cmic & (1 <<1 ))) { +if (!ctrl->opts->discovery_nqn && +nvme_active_ctrls (found) && !(id->cmic & (1 <<1 ))) {dev_err (ctrl->device,"ignoring ctrl due to duplicate subnqn (%s).\n" , found->subnqn);drivers/nvme/host/fabrics.c +0
-0
drivers/nvme/host/fabrics.c static int nvmf_parse_options(struct nvmf_ctrl_options *opts, · line 864
Quoted 7 lines
drivers/nvme/host/fabrics.c +1
-0@@ -858,6 +858,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,if (opts->discovery_nqn) { opts->kato =0 ; opts->nr_io_queues =0 ; + opts->duplicate_connect = true; }if (ctrl_loss_tmo <0 ) opts->max_reconnects =-1 ;
this is reasonable.
-- james
PATCH 3/3 nvme: allow duplicate connections to the discovery controller
In reply to Hannes ReineckeThanks, applied to nvme-4.18-2.
PATCH 3/3 nvme: allow duplicate connections to the discovery controller
In reply to Hannes ReineckeQuoted 16 lines · Hannes Reinecke
The whole point of the discovery controller is that it can accept multiple connections. Additionally the cmic field is not even defined for the discovery controller identify page.
Signed-off-by Hannes Reinecke <hare at suse.com>--- drivers/nvme/host/core.c | 3 ++- drivers/nvme/host/fabrics.c | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +0
-0
drivers/nvme/host/core.c static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id) · line 2501
Quoted 9 lines
drivers/nvme/host/core.c +2
-1@@ -2494,7 +2494,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)* Verify that the subsystem actually supports multiple * controllers, else bail out. */ -if (nvme_active_ctrls (found) && !(id->cmic & (1 <<1 ))) { +if (!ctrl->opts->discovery_nqn && +nvme_active_ctrls (found) && !(id->cmic & (1 <<1 ))) {dev_err (ctrl->device,"ignoring ctrl due to duplicate subnqn (%s).\n" , found->subnqn);
Question: this snippet excludes the discovery controller from failing the init call. But in doing so, it allows the discovery controller to be seen as "single unique device" based on its subnqn with another path added.? I assume, the read of the discovery log command will be done against the /dev/nvme? controller instance, so it will still go to the right "path" that corresponds to where the "nvme connect or connect-all" was issued to.? Correct ??? and other commands against the discovery controller should also use ioctls to the /dev/nvme? name so the proper device/path is used/deleted - Correct ?
Are there any side effects of the fact that we allow the discovery controller to have multiple paths ?
-- james
PATCH 3/3 nvme: allow duplicate connections to the discovery controller
Quoted 39 lines · James Smart
Quoted 17 lines · Hannes Reinecke
The whole point of the discovery controller is that it can accept multiple connections. Additionally the cmic field is not even defined for the discovery controller identify page.
Signed-off-by Hannes Reinecke <hare at suse.com>---
?drivers/nvme/host/core.c??? | 3 ++-
?drivers/nvme/host/fabrics.c| 1 +
? 2 files changed, 3 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +0
-0@@ -2494,7 +2494,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)?????????? * Verify that the subsystem actually supports multiple
Quoted 9 lines
?????????? * controllers, else bail out.
?????????? */
-??????? if (nvme_active_ctrls(found)&& !(id->cmic & (1 << 1))) { +??????? if (!ctrl->opts->discovery_nqn &&
+???????????nvme_active_ctrls(found)&& !(id->cmic & (1 << 1))) { ????????????? dev_err(ctrl->device,
????????????????? "ignoring ctrl due to duplicate subnqn (%s).\n", ?????????????????found->subnqn);Question: this snippet excludes the discovery controller from failing the init call. But in doing so, it allows the discovery controller to be seen as "single unique device" based on its subnqn with another path added.? I assume, the read of the discovery log command will be done against the
/dev/nvme? controller instance, so it will still go to the right "path" that corresponds to where the "nvme connect or connect-all" was issued to.? Correct ??? and other commands against the discovery controller should also use ioctls to the/dev/nvme? name so the proper device/path is used/deleted - Correct ?
Quoted 3 lines
Are there any side effects of the fact that we allow the discovery controller to have multiple paths ?
That would only be relevant if the discovery log page entry could be different for the individual paths.
It's somewhat hard to see how that could happen; it's not that one path can set the ANA CMIC bit, and the other path doesn't ...
So even if we somehow fail to get the path right we still would be getting the correct information, no?
Cheers,
Hannes
PATCH 3/3 nvme: allow duplicate connections to the discovery controller
Quoted 44 lines · Hannes Reinecke
Quoted 40 lines · James Smart
Quoted 18 lines · Hannes Reinecke
The whole point of the discovery controller is that it can accept multiple connections. Additionally the cmic field is not even defined for
the discovery controller identify page.Signed-off-by Hannes Reinecke <hare at suse.com>---
?drivers/nvme/host/core.c??? | 3 ++-
?drivers/nvme/host/fabrics.c| 1 +
? 2 files changed, 3 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +0
-0@@ -2494,7 +2494,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)?????????? * Verify that the subsystem actually supports multiple
Quoted 9 lines
?????????? * controllers, else bail out.
?????????? */
-??????? if (nvme_active_ctrls(found)&& !(id->cmic & (1 << 1))) { +??????? if (!ctrl->opts->discovery_nqn &&
+???????????nvme_active_ctrls(found)&& !(id->cmic & (1 << 1))) { ????????????? dev_err(ctrl->device,
????????????????? "ignoring ctrl due to duplicate subnqn (%s).\n", ?????????????????found->subnqn);Question: this snippet excludes the discovery controller from failing the init call. But in doing so, it allows the discovery controller to be seen as "single unique device" based on its subnqn with another path added.? I assume, the read of the discovery log command will be done against the
/dev/nvme? controller instance, so it will still go to the right "path" that corresponds to where the "nvme connect or connect-all" was issued to.? Correct ??? and other commands against the discovery controller should also use ioctls to the/dev/nvme? name so the proper device/path is used/deleted - Correct ?Quoted 3 lines
Are there any side effects of the fact that we allow the discovery controller to have multiple paths ?
Quoted 6 lines
That would only be relevant if the discovery log page entry could be different for the individual paths.
It's somewhat hard to see how that could happen; it's not that one path can set the ANA CMIC bit, and the other path doesn't ... So even if we somehow fail to get the path right we still would be getting the correct information, no?
This has little/none to do with pathing - but rather that all discovery controllers use a single well-known NQN - and they don't support multipathing.
There can be any number of discovery controllers, each with a different "view" of the storage subsystems available. Thus it matters very much which path you use. E.g. using a more extreme example: the discovery controller on a FC link could be very different from a discovery controller at an ip address.
-- james
PATCH 3/3 nvme: allow duplicate connections to the discovery controller
Quoted 58 lines · James Smart
Quoted 44 lines · Hannes Reinecke
Quoted 40 lines · James Smart
Quoted 18 lines · Hannes Reinecke
The whole point of the discovery controller is that it can accept multiple connections. Additionally the cmic field is not even defined for
the discovery controller identify page.Signed-off-by Hannes Reinecke <hare at suse.com>---
?drivers/nvme/host/core.c??? | 3 ++-
?drivers/nvme/host/fabrics.c| 1 +
? 2 files changed, 3 insertions(+), 1 deletion(-)drivers/nvme/host/core.c +0
-0@@ -2494,7 +2494,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)?????????? * Verify that the subsystem actually supports multiple
Quoted 9 lines
?????????? * controllers, else bail out.
?????????? */
-??????? if (nvme_active_ctrls(found)&& !(id->cmic & (1 << 1))) { +??????? if (!ctrl->opts->discovery_nqn &&
+???????????nvme_active_ctrls(found)&& !(id->cmic & (1 << 1))) { ????????????? dev_err(ctrl->device,
????????????????? "ignoring ctrl due to duplicate subnqn (%s).\n", ?????????????????found->subnqn);Question: this snippet excludes the discovery controller from failing the init call. But in doing so, it allows the discovery controller to be seen as "single unique device" based on its subnqn with another path added.? I assume, the read of the discovery log command will be done against the
/dev/nvme? controller instance, so it will still go to the right "path" that corresponds to where the "nvme connect or connect-all" was issued to.? Correct ??? and other commands against the discovery controller should also use ioctls to the/dev/nvme? name so the proper device/path is used/deleted - Correct ?Quoted 3 lines
Are there any side effects of the fact that we allow the discovery controller to have multiple paths ?
Quoted 6 lines
That would only be relevant if the discovery log page entry could be different for the individual paths.
It's somewhat hard to see how that could happen; it's not that one path can set the ANA CMIC bit, and the other path doesn't ... So even if we somehow fail to get the path right we still would be getting the correct information, no?This has little/none to do with pathing - but rather that all discovery controllers use a single well-known NQN - and they don't support multipathing.
Quoted 6 lines
There can be any number of discovery controllers, each with a different "view" of the storage subsystems available. Thus it matters very much which path you use. E.g. using a more extreme example: the discovery controller on a FC link could be very different from a discovery controller at an ip address.
Oh, indeed.
I was under the impression that we'd be indexing the discovery entries per transport/traaddr and subsysnqn.
But now I see that we'd be indexing them just by subsysnqn, ie we will only ever have one discovery subsysnqn entry.
Which really is wrong if different paths/transport can return different discovery log pages.
And locking access to the discovery subsysnqn has scalability impacts as we'ver just seen.
So we really should index the discovery subsysnqn by transport and traddr, not by subsysnqn.
Will be preparing a patch.
Cheers,
Hannes