* reserved tag allocation fixups
@ 2021-03-03 12:53 Christoph Hellwig
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-03-03 12:53 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
Hi all,
this series is based on the report form Hannes, and first reduces
the reserved tags on the admin queue from 2 to 1 and then uses a
nowait allocation for the keep alive request to avoid a deadlock
during unlikely error scenarios.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] nvme-fabrics: only reserve a single tag
2021-03-03 12:53 reserved tag allocation fixups Christoph Hellwig
@ 2021-03-03 12:53 ` Christoph Hellwig
2021-03-03 21:03 ` Daniel Wagner
` (3 more replies)
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
2021-03-03 12:53 ` [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT Christoph Hellwig
2 siblings, 4 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-03-03 12:53 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
Fabrics drivers currently reserve two tags on the admin queue. But
given that the connect command is only run on a freshly created queue
or after all commands have been force aborted we only need to reserve
a single tag.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/fabrics.h | 7 +++++++
drivers/nvme/host/fc.c | 4 ++--
drivers/nvme/host/rdma.c | 4 ++--
drivers/nvme/host/tcp.c | 4 ++--
drivers/nvme/target/loop.c | 4 ++--
5 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 733010d2eafd80..888b108d87a400 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -18,6 +18,13 @@
/* default is -1: the fail fast mechanism is disabled */
#define NVMF_DEF_FAIL_FAST_TMO -1
+/*
+ * Reserved one command for internal usage. This command is used for sending
+ * the connect command, as well as for the keep alive command on the admin
+ * queue once live.
+ */
+#define NVMF_RESERVED_TAGS 1
+
/*
* Define a host as seen by the target. We allocate one at boot, but also
* allow the override it when creating controllers. This is both to provide
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 20dadd86e98121..0f8b9784cbcd30 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2862,7 +2862,7 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl)
memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
ctrl->tag_set.ops = &nvme_fc_mq_ops;
ctrl->tag_set.queue_depth = ctrl->ctrl.opts->queue_size;
- ctrl->tag_set.reserved_tags = 1; /* fabric connect */
+ ctrl->tag_set.reserved_tags = NVMF_RESERVED_TAGS;
ctrl->tag_set.numa_node = ctrl->ctrl.numa_node;
ctrl->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
ctrl->tag_set.cmd_size =
@@ -3484,7 +3484,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
memset(&ctrl->admin_tag_set, 0, sizeof(ctrl->admin_tag_set));
ctrl->admin_tag_set.ops = &nvme_fc_admin_mq_ops;
ctrl->admin_tag_set.queue_depth = NVME_AQ_MQ_TAG_DEPTH;
- ctrl->admin_tag_set.reserved_tags = 2; /* fabric connect + Keep-Alive */
+ ctrl->admin_tag_set.reserved_tags = NVMF_RESERVED_TAGS;
ctrl->admin_tag_set.numa_node = ctrl->ctrl.numa_node;
ctrl->admin_tag_set.cmd_size =
struct_size((struct nvme_fcp_op_w_sgl *)NULL, priv,
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 53ac4d7442ba9c..7855103e05d25f 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -798,7 +798,7 @@ static struct blk_mq_tag_set *nvme_rdma_alloc_tagset(struct nvme_ctrl *nctrl,
memset(set, 0, sizeof(*set));
set->ops = &nvme_rdma_admin_mq_ops;
set->queue_depth = NVME_AQ_MQ_TAG_DEPTH;
- set->reserved_tags = 2; /* connect + keep-alive */
+ set->reserved_tags = NVMF_RESERVED_TAGS;
set->numa_node = nctrl->numa_node;
set->cmd_size = sizeof(struct nvme_rdma_request) +
NVME_RDMA_DATA_SGL_SIZE;
@@ -811,7 +811,7 @@ static struct blk_mq_tag_set *nvme_rdma_alloc_tagset(struct nvme_ctrl *nctrl,
memset(set, 0, sizeof(*set));
set->ops = &nvme_rdma_mq_ops;
set->queue_depth = nctrl->sqsize + 1;
- set->reserved_tags = 1; /* fabric connect */
+ set->reserved_tags = NVMF_RESERVED_TAGS;
set->numa_node = nctrl->numa_node;
set->flags = BLK_MQ_F_SHOULD_MERGE;
set->cmd_size = sizeof(struct nvme_rdma_request) +
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 69f59d2c5799b8..c535836e7065b9 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1575,7 +1575,7 @@ static struct blk_mq_tag_set *nvme_tcp_alloc_tagset(struct nvme_ctrl *nctrl,
memset(set, 0, sizeof(*set));
set->ops = &nvme_tcp_admin_mq_ops;
set->queue_depth = NVME_AQ_MQ_TAG_DEPTH;
- set->reserved_tags = 2; /* connect + keep-alive */
+ set->reserved_tags = NVMF_RESERVED_TAGS;
set->numa_node = nctrl->numa_node;
set->flags = BLK_MQ_F_BLOCKING;
set->cmd_size = sizeof(struct nvme_tcp_request);
@@ -1587,7 +1587,7 @@ static struct blk_mq_tag_set *nvme_tcp_alloc_tagset(struct nvme_ctrl *nctrl,
memset(set, 0, sizeof(*set));
set->ops = &nvme_tcp_mq_ops;
set->queue_depth = nctrl->sqsize + 1;
- set->reserved_tags = 1; /* fabric connect */
+ set->reserved_tags = NVMF_RESERVED_TAGS;
set->numa_node = nctrl->numa_node;
set->flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_BLOCKING;
set->cmd_size = sizeof(struct nvme_tcp_request);
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index cb6f86572b24ad..3e189e753bcf5a 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -349,7 +349,7 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
memset(&ctrl->admin_tag_set, 0, sizeof(ctrl->admin_tag_set));
ctrl->admin_tag_set.ops = &nvme_loop_admin_mq_ops;
ctrl->admin_tag_set.queue_depth = NVME_AQ_MQ_TAG_DEPTH;
- ctrl->admin_tag_set.reserved_tags = 2; /* connect + keep-alive */
+ ctrl->admin_tag_set.reserved_tags = NVMF_RESERVED_TAGS;
ctrl->admin_tag_set.numa_node = ctrl->ctrl.numa_node;
ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_loop_iod) +
NVME_INLINE_SG_CNT * sizeof(struct scatterlist);
@@ -520,7 +520,7 @@ static int nvme_loop_create_io_queues(struct nvme_loop_ctrl *ctrl)
memset(&ctrl->tag_set, 0, sizeof(ctrl->tag_set));
ctrl->tag_set.ops = &nvme_loop_mq_ops;
ctrl->tag_set.queue_depth = ctrl->ctrl.opts->queue_size;
- ctrl->tag_set.reserved_tags = 1; /* fabric connect */
+ ctrl->tag_set.reserved_tags = NVMF_RESERVED_TAGS;
ctrl->tag_set.numa_node = ctrl->ctrl.numa_node;
ctrl->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
ctrl->tag_set.cmd_size = sizeof(struct nvme_loop_iod) +
--
2.29.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work
2021-03-03 12:53 reserved tag allocation fixups Christoph Hellwig
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
@ 2021-03-03 12:53 ` Christoph Hellwig
2021-03-03 21:01 ` Daniel Wagner
` (4 more replies)
2021-03-03 12:53 ` [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT Christoph Hellwig
2 siblings, 5 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-03-03 12:53 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
Merge nvme_keep_alive into its only caller to prepare for additional
changes to this code.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/core.c | 26 ++++++++------------------
1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e68a8c4ac5a6ea..bdeb3feae61899 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1225,28 +1225,12 @@ static void nvme_keep_alive_end_io(struct request *rq, blk_status_t status)
queue_delayed_work(nvme_wq, &ctrl->ka_work, ctrl->kato * HZ);
}
-static int nvme_keep_alive(struct nvme_ctrl *ctrl)
-{
- struct request *rq;
-
- rq = nvme_alloc_request(ctrl->admin_q, &ctrl->ka_cmd,
- BLK_MQ_REQ_RESERVED);
- if (IS_ERR(rq))
- return PTR_ERR(rq);
-
- rq->timeout = ctrl->kato * HZ;
- rq->end_io_data = ctrl;
-
- blk_execute_rq_nowait(NULL, rq, 0, nvme_keep_alive_end_io);
-
- return 0;
-}
-
static void nvme_keep_alive_work(struct work_struct *work)
{
struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
struct nvme_ctrl, ka_work);
bool comp_seen = ctrl->comp_seen;
+ struct request *rq;
if ((ctrl->ctratt & NVME_CTRL_ATTR_TBKAS) && comp_seen) {
dev_dbg(ctrl->device,
@@ -1256,12 +1240,18 @@ static void nvme_keep_alive_work(struct work_struct *work)
return;
}
- if (nvme_keep_alive(ctrl)) {
+ rq = nvme_alloc_request(ctrl->admin_q, &ctrl->ka_cmd,
+ BLK_MQ_REQ_RESERVED);
+ if (IS_ERR(rq)) {
/* allocation failure, reset the controller */
dev_err(ctrl->device, "keep-alive failed\n");
nvme_reset_ctrl(ctrl);
return;
}
+
+ rq->timeout = ctrl->kato * HZ;
+ rq->end_io_data = ctrl;
+ blk_execute_rq_nowait(NULL, rq, 0, nvme_keep_alive_end_io);
}
static void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
--
2.29.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT
2021-03-03 12:53 reserved tag allocation fixups Christoph Hellwig
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
@ 2021-03-03 12:53 ` Christoph Hellwig
2021-03-03 21:00 ` Daniel Wagner
` (3 more replies)
2 siblings, 4 replies; 19+ messages in thread
From: Christoph Hellwig @ 2021-03-03 12:53 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
To avoid an error recovery deadlock where the keep alive work is waiting
for a request and thus can't be flushed to make progress for tearing down
the controller. Also print the error code returned from
blk_mq_alloc_request to help debugging any future issues in this code.
Based on an earlier patch from Hannes Reinecke <hare@suse.de>.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index bdeb3feae61899..8a4a9fe96cdf83 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1241,10 +1241,10 @@ static void nvme_keep_alive_work(struct work_struct *work)
}
rq = nvme_alloc_request(ctrl->admin_q, &ctrl->ka_cmd,
- BLK_MQ_REQ_RESERVED);
+ BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
if (IS_ERR(rq)) {
/* allocation failure, reset the controller */
- dev_err(ctrl->device, "keep-alive failed\n");
+ dev_err(ctrl->device, "keep-alive failed: %ld\n", PTR_ERR(rq));
nvme_reset_ctrl(ctrl);
return;
}
--
2.29.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT
2021-03-03 12:53 ` [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT Christoph Hellwig
@ 2021-03-03 21:00 ` Daniel Wagner
2021-03-03 21:22 ` Chaitanya Kulkarni
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Daniel Wagner @ 2021-03-03 21:00 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, James Smart, linux-nvme
On Wed, Mar 03, 2021 at 01:53:04PM +0100, Christoph Hellwig wrote:
> To avoid an error recovery deadlock where the keep alive work is waiting
> for a request and thus can't be flushed to make progress for tearing down
> the controller. Also print the error code returned from
> blk_mq_alloc_request to help debugging any future issues in this code.
>
> Based on an earlier patch from Hannes Reinecke <hare@suse.de>.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
And for what's worth, our customer tested this. I've done the same patch
during debugging. It fixes the deadlock as Hannes reported.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
@ 2021-03-03 21:01 ` Daniel Wagner
2021-03-03 21:24 ` Chaitanya Kulkarni
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Daniel Wagner @ 2021-03-03 21:01 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, James Smart, linux-nvme
On Wed, Mar 03, 2021 at 01:53:03PM +0100, Christoph Hellwig wrote:
> Merge nvme_keep_alive into its only caller to prepare for additional
> changes to this code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] nvme-fabrics: only reserve a single tag
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
@ 2021-03-03 21:03 ` Daniel Wagner
2021-03-03 21:26 ` Chaitanya Kulkarni
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Daniel Wagner @ 2021-03-03 21:03 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, James Smart, linux-nvme
On Wed, Mar 03, 2021 at 01:53:02PM +0100, Christoph Hellwig wrote:
> Fabrics drivers currently reserve two tags on the admin queue. But
> given that the connect command is only run on a freshly created queue
> or after all commands have been force aborted we only need to reserve
> a single tag.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT
2021-03-03 12:53 ` [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT Christoph Hellwig
2021-03-03 21:00 ` Daniel Wagner
@ 2021-03-03 21:22 ` Chaitanya Kulkarni
2021-03-04 7:03 ` Hannes Reinecke
2021-03-15 17:13 ` Sagi Grimberg
3 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-03-03 21:22 UTC (permalink / raw)
To: Christoph Hellwig, Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
On 3/3/21 07:25, Christoph Hellwig wrote:
> To avoid an error recovery deadlock where the keep alive work is waiting
> for a request and thus can't be flushed to make progress for tearing down
> the controller. Also print the error code returned from
> blk_mq_alloc_request to help debugging any future issues in this code.
>
> Based on an earlier patch from Hannes Reinecke <hare@suse.de>.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
2021-03-03 21:01 ` Daniel Wagner
@ 2021-03-03 21:24 ` Chaitanya Kulkarni
2021-03-03 21:53 ` Chaitanya Kulkarni
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-03-03 21:24 UTC (permalink / raw)
To: Christoph Hellwig, Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
On 3/3/21 11:15, Christoph Hellwig wrote:
> Merge nvme_keep_alive into its only caller to prepare for additional
> changes to this code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] nvme-fabrics: only reserve a single tag
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
2021-03-03 21:03 ` Daniel Wagner
@ 2021-03-03 21:26 ` Chaitanya Kulkarni
2021-03-04 7:02 ` Hannes Reinecke
2021-03-05 20:51 ` Sagi Grimberg
3 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-03-03 21:26 UTC (permalink / raw)
To: Christoph Hellwig, Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
On 3/3/21 13:18, Christoph Hellwig wrote:
> Fabrics drivers currently reserve two tags on the admin queue. But
> given that the connect command is only run on a freshly created queue
> or after all commands have been force aborted we only need to reserve
> a single tag.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Thanks a lot for creating a macro with nice comment, looks good.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
2021-03-03 21:01 ` Daniel Wagner
2021-03-03 21:24 ` Chaitanya Kulkarni
@ 2021-03-03 21:53 ` Chaitanya Kulkarni
2021-03-04 7:02 ` Hannes Reinecke
2021-03-15 17:13 ` Sagi Grimberg
4 siblings, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2021-03-03 21:53 UTC (permalink / raw)
To: Christoph Hellwig, Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
On 3/3/21 11:15, Christoph Hellwig wrote:
> Merge nvme_keep_alive into its only caller to prepare for additional
> changes to this code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] nvme-fabrics: only reserve a single tag
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
2021-03-03 21:03 ` Daniel Wagner
2021-03-03 21:26 ` Chaitanya Kulkarni
@ 2021-03-04 7:02 ` Hannes Reinecke
2021-03-05 20:51 ` Sagi Grimberg
3 siblings, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2021-03-04 7:02 UTC (permalink / raw)
To: Christoph Hellwig, Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
On 3/3/21 1:53 PM, Christoph Hellwig wrote:
> Fabrics drivers currently reserve two tags on the admin queue. But
> given that the connect command is only run on a freshly created queue
> or after all commands have been force aborted we only need to reserve
> a single tag.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/fabrics.h | 7 +++++++
> drivers/nvme/host/fc.c | 4 ++--
> drivers/nvme/host/rdma.c | 4 ++--
> drivers/nvme/host/tcp.c | 4 ++--
> drivers/nvme/target/loop.c | 4 ++--
> 5 files changed, 15 insertions(+), 8 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
` (2 preceding siblings ...)
2021-03-03 21:53 ` Chaitanya Kulkarni
@ 2021-03-04 7:02 ` Hannes Reinecke
2021-03-15 17:13 ` Sagi Grimberg
4 siblings, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2021-03-04 7:02 UTC (permalink / raw)
To: Christoph Hellwig, Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
On 3/3/21 1:53 PM, Christoph Hellwig wrote:
> Merge nvme_keep_alive into its only caller to prepare for additional
> changes to this code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/core.c | 26 ++++++++------------------
> 1 file changed, 8 insertions(+), 18 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT
2021-03-03 12:53 ` [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT Christoph Hellwig
2021-03-03 21:00 ` Daniel Wagner
2021-03-03 21:22 ` Chaitanya Kulkarni
@ 2021-03-04 7:03 ` Hannes Reinecke
2021-03-15 17:13 ` Sagi Grimberg
3 siblings, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2021-03-04 7:03 UTC (permalink / raw)
To: Christoph Hellwig, Sagi Grimberg; +Cc: Keith Busch, James Smart, linux-nvme
On 3/3/21 1:53 PM, Christoph Hellwig wrote:
> To avoid an error recovery deadlock where the keep alive work is waiting
> for a request and thus can't be flushed to make progress for tearing down
> the controller. Also print the error code returned from
> blk_mq_alloc_request to help debugging any future issues in this code.
>
> Based on an earlier patch from Hannes Reinecke <hare@suse.de>.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index bdeb3feae61899..8a4a9fe96cdf83 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1241,10 +1241,10 @@ static void nvme_keep_alive_work(struct work_struct *work)
> }
>
> rq = nvme_alloc_request(ctrl->admin_q, &ctrl->ka_cmd,
> - BLK_MQ_REQ_RESERVED);
> + BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
> if (IS_ERR(rq)) {
> /* allocation failure, reset the controller */
> - dev_err(ctrl->device, "keep-alive failed\n");
> + dev_err(ctrl->device, "keep-alive failed: %ld\n", PTR_ERR(rq));
> nvme_reset_ctrl(ctrl);
> return;
> }
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] nvme-fabrics: only reserve a single tag
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
` (2 preceding siblings ...)
2021-03-04 7:02 ` Hannes Reinecke
@ 2021-03-05 20:51 ` Sagi Grimberg
2021-03-06 7:09 ` Christoph Hellwig
3 siblings, 1 reply; 19+ messages in thread
From: Sagi Grimberg @ 2021-03-05 20:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, James Smart, linux-nvme
> Fabrics drivers currently reserve two tags on the admin queue. But
> given that the connect command is only run on a freshly created queue
> or after all commands have been force aborted we only need to reserve
> a single tag.
Umm, I think this would be an issue for non-mpath fabrics devices...
When we teardown the controller, we iterate over all tags
and cancel them, however for non-mpath devices these actually
stick around until they exhaust the retrys counter (with the
hope that the controller will reconnect again) unlike the mpath
case where the requests are failed-over.
Now if the admin queue is absolutely full during a reset, we won't
have a keep-alive command.
One possible solution is to make sure that
nvme_decide_disposition to complete a request for admin
commands. However note that this means that admin commands
would fail immediately when the controller resets, which is
a regression from the existing behavior and we may piss off
users with that...
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] nvme-fabrics: only reserve a single tag
2021-03-05 20:51 ` Sagi Grimberg
@ 2021-03-06 7:09 ` Christoph Hellwig
2021-03-15 17:12 ` Sagi Grimberg
0 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2021-03-06 7:09 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, James Smart, linux-nvme
On Fri, Mar 05, 2021 at 12:51:15PM -0800, Sagi Grimberg wrote:
>
>> Fabrics drivers currently reserve two tags on the admin queue. But
>> given that the connect command is only run on a freshly created queue
>> or after all commands have been force aborted we only need to reserve
>> a single tag.
>
> Umm, I think this would be an issue for non-mpath fabrics devices...
>
> When we teardown the controller, we iterate over all tags
> and cancel them, however for non-mpath devices these actually
> stick around until they exhaust the retrys counter (with the
> hope that the controller will reconnect again) unlike the mpath
> case where the requests are failed-over.
>
> Now if the admin queue is absolutely full during a reset, we won't
> have a keep-alive command.
>
> One possible solution is to make sure that
> nvme_decide_disposition to complete a request for admin
> commands. However note that this means that admin commands
> would fail immediately when the controller resets, which is
> a regression from the existing behavior and we may piss off
> users with that...
We never retry admin commands! And the reserved tags are set aside
and not relevant to the "normal" commands.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] nvme-fabrics: only reserve a single tag
2021-03-06 7:09 ` Christoph Hellwig
@ 2021-03-15 17:12 ` Sagi Grimberg
0 siblings, 0 replies; 19+ messages in thread
From: Sagi Grimberg @ 2021-03-15 17:12 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, James Smart, linux-nvme
>>> Fabrics drivers currently reserve two tags on the admin queue. But
>>> given that the connect command is only run on a freshly created queue
>>> or after all commands have been force aborted we only need to reserve
>>> a single tag.
>>
>> Umm, I think this would be an issue for non-mpath fabrics devices...
>>
>> When we teardown the controller, we iterate over all tags
>> and cancel them, however for non-mpath devices these actually
>> stick around until they exhaust the retrys counter (with the
>> hope that the controller will reconnect again) unlike the mpath
>> case where the requests are failed-over.
>>
>> Now if the admin queue is absolutely full during a reset, we won't
>> have a keep-alive command.
>>
>> One possible solution is to make sure that
>> nvme_decide_disposition to complete a request for admin
>> commands. However note that this means that admin commands
>> would fail immediately when the controller resets, which is
>> a regression from the existing behavior and we may piss off
>> users with that...
>
> We never retry admin commands! And the reserved tags are set aside
> and not relevant to the "normal" commands.
You're right, admin commands will be noretry commands. this looks
good to me.
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
` (3 preceding siblings ...)
2021-03-04 7:02 ` Hannes Reinecke
@ 2021-03-15 17:13 ` Sagi Grimberg
4 siblings, 0 replies; 19+ messages in thread
From: Sagi Grimberg @ 2021-03-15 17:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, James Smart, linux-nvme
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT
2021-03-03 12:53 ` [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT Christoph Hellwig
` (2 preceding siblings ...)
2021-03-04 7:03 ` Hannes Reinecke
@ 2021-03-15 17:13 ` Sagi Grimberg
3 siblings, 0 replies; 19+ messages in thread
From: Sagi Grimberg @ 2021-03-15 17:13 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, James Smart, linux-nvme
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-03-15 17:13 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 12:53 reserved tag allocation fixups Christoph Hellwig
2021-03-03 12:53 ` [PATCH 1/3] nvme-fabrics: only reserve a single tag Christoph Hellwig
2021-03-03 21:03 ` Daniel Wagner
2021-03-03 21:26 ` Chaitanya Kulkarni
2021-03-04 7:02 ` Hannes Reinecke
2021-03-05 20:51 ` Sagi Grimberg
2021-03-06 7:09 ` Christoph Hellwig
2021-03-15 17:12 ` Sagi Grimberg
2021-03-03 12:53 ` [PATCH 2/3] nvme: merge nvme_keep_alive into nvme_keep_alive_work Christoph Hellwig
2021-03-03 21:01 ` Daniel Wagner
2021-03-03 21:24 ` Chaitanya Kulkarni
2021-03-03 21:53 ` Chaitanya Kulkarni
2021-03-04 7:02 ` Hannes Reinecke
2021-03-15 17:13 ` Sagi Grimberg
2021-03-03 12:53 ` [PATCH 3/3] nvme: allocate the keep alive request using BLK_MQ_REQ_NOWAIT Christoph Hellwig
2021-03-03 21:00 ` Daniel Wagner
2021-03-03 21:22 ` Chaitanya Kulkarni
2021-03-04 7:03 ` Hannes Reinecke
2021-03-15 17:13 ` Sagi Grimberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).