* [PATCH] nvme: move nvme_stop_keep_alive() back to original position
@ 2023-11-21 8:01 Hannes Reinecke
2023-11-21 9:23 ` Sagi Grimberg
0 siblings, 1 reply; 6+ messages in thread
From: Hannes Reinecke @ 2023-11-21 8:01 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke
Stopping keep-alive not only stops the keep-alive workqueue,
but also needs to be synchronized with I/O termination as we
must not send a keep-alive command after all I/O had been
terminated.
So to avoid any regressions move the call to stop_keep_alive()
back to its original position and ensure that keep-alive is
correctly stopped failing to setup the admin queue.
Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
Suggested-by: Sagi Grimberg <sagi@grimberg.me>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/core.c | 2 +-
drivers/nvme/host/fc.c | 19 ++++++++-----------
drivers/nvme/host/rdma.c | 1 +
drivers/nvme/host/tcp.c | 1 +
4 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fd28e6b6574c..46a4c9c5ea96 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -482,7 +482,6 @@ EXPORT_SYMBOL_GPL(nvme_cancel_tagset);
void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
{
- nvme_stop_keep_alive(ctrl);
if (ctrl->admin_tagset) {
blk_mq_tagset_busy_iter(ctrl->admin_tagset,
nvme_cancel_request, ctrl);
@@ -4355,6 +4354,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
{
nvme_mpath_stop(ctrl);
nvme_auth_stop(ctrl);
+ nvme_stop_keep_alive(ctrl);
nvme_stop_failfast_work(ctrl);
flush_work(&ctrl->async_event_work);
cancel_work_sync(&ctrl->fw_act_work);
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 49c3e46eaa1e..9f9a3b35dc64 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2530,12 +2530,6 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
* clean up the admin queue. Same thing as above.
*/
nvme_quiesce_admin_queue(&ctrl->ctrl);
-
- /*
- * Open-coding nvme_cancel_admin_tagset() as fc
- * is not using nvme_cancel_request().
- */
- nvme_stop_keep_alive(&ctrl->ctrl);
blk_sync_queue(ctrl->ctrl.admin_q);
blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
nvme_fc_terminate_exchange, &ctrl->ctrl);
@@ -3138,11 +3132,12 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
nvme_unquiesce_admin_queue(&ctrl->ctrl);
ret = nvme_init_ctrl_finish(&ctrl->ctrl, false);
- if (!ret && test_bit(ASSOC_FAILED, &ctrl->flags))
- ret = -EIO;
if (ret)
goto out_disconnect_admin_queue;
-
+ if (test_bit(ASSOC_FAILED, &ctrl->flags)) {
+ ret = -EIO;
+ goto out_stop_keep_alive;
+ }
/* sanity checks */
/* FC-NVME does not have other data in the capsule */
@@ -3150,7 +3145,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
dev_err(ctrl->ctrl.device, "icdoff %d is not supported!\n",
ctrl->ctrl.icdoff);
ret = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
- goto out_disconnect_admin_queue;
+ goto out_stop_keep_alive;
}
/* FC-NVME supports normal SGL Data Block Descriptors */
@@ -3158,7 +3153,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
dev_err(ctrl->ctrl.device,
"Mandatory sgls are not supported!\n");
ret = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
- goto out_disconnect_admin_queue;
+ goto out_stop_keep_alive;
}
if (opts->queue_size > ctrl->ctrl.maxcmd) {
@@ -3205,6 +3200,8 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
out_term_aen_ops:
nvme_fc_term_aen_ops(ctrl);
+out_stop_keep_alive:
+ nvme_stop_keep_alive(&ctrl->ctrl);
out_disconnect_admin_queue:
dev_warn(ctrl->ctrl.device,
"NVME-FC{%d}: create_assoc failed, assoc_id %llx ret %d\n",
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index a7fea4cbacd7..6d178d555920 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1080,6 +1080,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
nvme_rdma_free_io_queues(ctrl);
}
destroy_admin:
+ nvme_stop_keep_alive(&ctrl->ctrl);
nvme_quiesce_admin_queue(&ctrl->ctrl);
blk_sync_queue(ctrl->ctrl.admin_q);
nvme_rdma_stop_queue(&ctrl->queues[0]);
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 6ed794815517..ddcd23fb8b75 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2237,6 +2237,7 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
nvme_tcp_destroy_io_queues(ctrl, new);
}
destroy_admin:
+ nvme_stop_keep_alive(ctrl);
nvme_tcp_teardown_admin_queue(ctrl, false);
return ret;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme: move nvme_stop_keep_alive() back to original position
2023-11-21 8:01 [PATCH] nvme: move nvme_stop_keep_alive() back to original position Hannes Reinecke
@ 2023-11-21 9:23 ` Sagi Grimberg
2023-11-21 14:41 ` Hannes Reinecke
0 siblings, 1 reply; 6+ messages in thread
From: Sagi Grimberg @ 2023-11-21 9:23 UTC (permalink / raw)
To: Hannes Reinecke, Christoph Hellwig; +Cc: Keith Busch, linux-nvme
> Stopping keep-alive not only stops the keep-alive workqueue,
> but also needs to be synchronized with I/O termination as we
> must not send a keep-alive command after all I/O had been
> terminated.
> So to avoid any regressions move the call to stop_keep_alive()
> back to its original position and ensure that keep-alive is
> correctly stopped failing to setup the admin queue.
>
> Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
It should be folded in I guess. I suggest to either suggest an
alternative phrasing of the patch or resend an alternative patch
set.
> Suggested-by: Sagi Grimberg <sagi@grimberg.me>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/nvme/host/core.c | 2 +-
> drivers/nvme/host/fc.c | 19 ++++++++-----------
> drivers/nvme/host/rdma.c | 1 +
> drivers/nvme/host/tcp.c | 1 +
> 4 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index fd28e6b6574c..46a4c9c5ea96 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -482,7 +482,6 @@ EXPORT_SYMBOL_GPL(nvme_cancel_tagset);
>
> void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
> {
> - nvme_stop_keep_alive(ctrl);
> if (ctrl->admin_tagset) {
> blk_mq_tagset_busy_iter(ctrl->admin_tagset,
> nvme_cancel_request, ctrl);
> @@ -4355,6 +4354,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
> {
> nvme_mpath_stop(ctrl);
> nvme_auth_stop(ctrl);
> + nvme_stop_keep_alive(ctrl);
> nvme_stop_failfast_work(ctrl);
> flush_work(&ctrl->async_event_work);
> cancel_work_sync(&ctrl->fw_act_work);
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 49c3e46eaa1e..9f9a3b35dc64 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2530,12 +2530,6 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
> * clean up the admin queue. Same thing as above.
> */
> nvme_quiesce_admin_queue(&ctrl->ctrl);
> -
> - /*
> - * Open-coding nvme_cancel_admin_tagset() as fc
> - * is not using nvme_cancel_request().
> - */
> - nvme_stop_keep_alive(&ctrl->ctrl);
> blk_sync_queue(ctrl->ctrl.admin_q);
> blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
> nvme_fc_terminate_exchange, &ctrl->ctrl);
> @@ -3138,11 +3132,12 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> nvme_unquiesce_admin_queue(&ctrl->ctrl);
>
> ret = nvme_init_ctrl_finish(&ctrl->ctrl, false);
> - if (!ret && test_bit(ASSOC_FAILED, &ctrl->flags))
> - ret = -EIO;
> if (ret)
> goto out_disconnect_admin_queue;
> -
> + if (test_bit(ASSOC_FAILED, &ctrl->flags)) {
> + ret = -EIO;
> + goto out_stop_keep_alive;
> + }
> /* sanity checks */
>
> /* FC-NVME does not have other data in the capsule */
> @@ -3150,7 +3145,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> dev_err(ctrl->ctrl.device, "icdoff %d is not supported!\n",
> ctrl->ctrl.icdoff);
> ret = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
> - goto out_disconnect_admin_queue;
> + goto out_stop_keep_alive;
> }
>
> /* FC-NVME supports normal SGL Data Block Descriptors */
> @@ -3158,7 +3153,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
> dev_err(ctrl->ctrl.device,
> "Mandatory sgls are not supported!\n");
> ret = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
> - goto out_disconnect_admin_queue;
> + goto out_stop_keep_alive;
> }
>
> if (opts->queue_size > ctrl->ctrl.maxcmd) {
> @@ -3205,6 +3200,8 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
>
> out_term_aen_ops:
> nvme_fc_term_aen_ops(ctrl);
> +out_stop_keep_alive:
> + nvme_stop_keep_alive(&ctrl->ctrl);
> out_disconnect_admin_queue:
> dev_warn(ctrl->ctrl.device,
> "NVME-FC{%d}: create_assoc failed, assoc_id %llx ret %d\n",
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index a7fea4cbacd7..6d178d555920 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1080,6 +1080,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
> nvme_rdma_free_io_queues(ctrl);
> }
> destroy_admin:
> + nvme_stop_keep_alive(&ctrl->ctrl);
> nvme_quiesce_admin_queue(&ctrl->ctrl);
> blk_sync_queue(ctrl->ctrl.admin_q);
> nvme_rdma_stop_queue(&ctrl->queues[0]);
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 6ed794815517..ddcd23fb8b75 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2237,6 +2237,7 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
> nvme_tcp_destroy_io_queues(ctrl, new);
> }
> destroy_admin:
> + nvme_stop_keep_alive(ctrl);
> nvme_tcp_teardown_admin_queue(ctrl, false);
> return ret;
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme: move nvme_stop_keep_alive() back to original position
2023-11-21 9:23 ` Sagi Grimberg
@ 2023-11-21 14:41 ` Hannes Reinecke
2023-11-21 17:51 ` Keith Busch
0 siblings, 1 reply; 6+ messages in thread
From: Hannes Reinecke @ 2023-11-21 14:41 UTC (permalink / raw)
To: Sagi Grimberg, Christoph Hellwig; +Cc: Keith Busch, linux-nvme
On 11/21/23 10:23, Sagi Grimberg wrote:
>
>> Stopping keep-alive not only stops the keep-alive workqueue,
>> but also needs to be synchronized with I/O termination as we
>> must not send a keep-alive command after all I/O had been
>> terminated.
>> So to avoid any regressions move the call to stop_keep_alive()
>> back to its original position and ensure that keep-alive is
>> correctly stopped failing to setup the admin queue.
>>
>> Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
>
> It should be folded in I guess. I suggest to either suggest an
> alternative phrasing of the patch or resend an alternative patch
> set.
>
Well, the original patch has already been merged to the nvme-6.7 tree,
so I've attached the 'Fixes' tag.
But let me know how you'd like to play it.
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), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme: move nvme_stop_keep_alive() back to original position
2023-11-21 14:41 ` Hannes Reinecke
@ 2023-11-21 17:51 ` Keith Busch
2023-11-22 8:16 ` Sagi Grimberg
0 siblings, 1 reply; 6+ messages in thread
From: Keith Busch @ 2023-11-21 17:51 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Sagi Grimberg, Christoph Hellwig, linux-nvme
On Tue, Nov 21, 2023 at 03:41:52PM +0100, Hannes Reinecke wrote:
> On 11/21/23 10:23, Sagi Grimberg wrote:
> >
> > > Stopping keep-alive not only stops the keep-alive workqueue,
> > > but also needs to be synchronized with I/O termination as we
> > > must not send a keep-alive command after all I/O had been
> > > terminated.
> > > So to avoid any regressions move the call to stop_keep_alive()
> > > back to its original position and ensure that keep-alive is
> > > correctly stopped failing to setup the admin queue.
> > >
> > > Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> >
> > It should be folded in I guess. I suggest to either suggest an
> > alternative phrasing of the patch or resend an alternative patch
> > set.
> >
> Well, the original patch has already been merged to the nvme-6.7 tree,
> so I've attached the 'Fixes' tag.
> But let me know how you'd like to play it.
It's not just in nvme-6.7; it's merged upstream too. We can't squash it
now, so is it okay to apply this update as-is for the next pull request?
I'm going to send the next one out tomorrow before US holiday.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme: move nvme_stop_keep_alive() back to original position
2023-11-21 17:51 ` Keith Busch
@ 2023-11-22 8:16 ` Sagi Grimberg
2023-11-22 16:07 ` Keith Busch
0 siblings, 1 reply; 6+ messages in thread
From: Sagi Grimberg @ 2023-11-22 8:16 UTC (permalink / raw)
To: Keith Busch, Hannes Reinecke; +Cc: Christoph Hellwig, linux-nvme
>>>> Stopping keep-alive not only stops the keep-alive workqueue,
>>>> but also needs to be synchronized with I/O termination as we
>>>> must not send a keep-alive command after all I/O had been
>>>> terminated.
>>>> So to avoid any regressions move the call to stop_keep_alive()
>>>> back to its original position and ensure that keep-alive is
>>>> correctly stopped failing to setup the admin queue.
>>>>
>>>> Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
>>>
>>> It should be folded in I guess. I suggest to either suggest an
>>> alternative phrasing of the patch or resend an alternative patch
>>> set.
>>>
>> Well, the original patch has already been merged to the nvme-6.7 tree,
>> so I've attached the 'Fixes' tag.
>> But let me know how you'd like to play it.
>
> It's not just in nvme-6.7; it's merged upstream too. We can't squash it
> now, so is it okay to apply this update as-is for the next pull request?
> I'm going to send the next one out tomorrow before US holiday.
Right, I didn't reach the PR email that was sent. Lets send the patch
as is. You can add my:
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme: move nvme_stop_keep_alive() back to original position
2023-11-22 8:16 ` Sagi Grimberg
@ 2023-11-22 16:07 ` Keith Busch
0 siblings, 0 replies; 6+ messages in thread
From: Keith Busch @ 2023-11-22 16:07 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Hannes Reinecke, Christoph Hellwig, linux-nvme
On Wed, Nov 22, 2023 at 10:16:09AM +0200, Sagi Grimberg wrote:
>
> > > > > Stopping keep-alive not only stops the keep-alive workqueue,
> > > > > but also needs to be synchronized with I/O termination as we
> > > > > must not send a keep-alive command after all I/O had been
> > > > > terminated.
> > > > > So to avoid any regressions move the call to stop_keep_alive()
> > > > > back to its original position and ensure that keep-alive is
> > > > > correctly stopped failing to setup the admin queue.
> > > > >
> > > > > Fixes: 4733b65d82bd ("nvme: start keep-alive after admin queue setup")
> > > >
> > > > It should be folded in I guess. I suggest to either suggest an
> > > > alternative phrasing of the patch or resend an alternative patch
> > > > set.
> > > >
> > > Well, the original patch has already been merged to the nvme-6.7 tree,
> > > so I've attached the 'Fixes' tag.
> > > But let me know how you'd like to play it.
> >
> > It's not just in nvme-6.7; it's merged upstream too. We can't squash it
> > now, so is it okay to apply this update as-is for the next pull request?
> > I'm going to send the next one out tomorrow before US holiday.
>
> Right, I didn't reach the PR email that was sent. Lets send the patch
> as is. You can add my:
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Thanks, queued up now for nvme-6.7.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-22 16:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-21 8:01 [PATCH] nvme: move nvme_stop_keep_alive() back to original position Hannes Reinecke
2023-11-21 9:23 ` Sagi Grimberg
2023-11-21 14:41 ` Hannes Reinecke
2023-11-21 17:51 ` Keith Busch
2023-11-22 8:16 ` Sagi Grimberg
2023-11-22 16:07 ` Keith Busch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.