* 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
* 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 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 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 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
* [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
* 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 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 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 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 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
* [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 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 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 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).