[PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Daniel Wagner]

When nvme_tcp_try_send fails to send a request is usually defers the
error handling to upper layers but it ignores the REQ_FAILFAST_DRIVER
flag.

This can lead to very long shutdown delays when the users issues a
'nvme disconnect-all':

 [44888.710527] nvme nvme0: Removing ctrl: NQN "xxx"
 [44898.981684] nvme nvme0: failed to send request -32
 [44960.982977] nvme nvme0: queue 0: timeout request 0x18 type 4
 [44960.983099] nvme nvme0: Property Set error: 881, offset 0x14

At timestamp [44898.981684] nvme_tcp_try_send fails to send but we have
to wait for the timeout handler to expires at [44960.982977] before we
make any progress.

With honoring the REQ_FAILFAST_DRIVER flag the log looks like this:

 [ 8999.227495] nvme nvme1: Removing ctrl: NQN "xxx"
 [ 9016.680447] nvme nvme1: failed to send request -32
 [ 9016.680477] nvme nvme1: Property Set error: 880, offset 0x14

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/host/tcp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 10fc45d95b86..f714d7ad38c0 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1145,9 +1145,11 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
 	if (ret == -EAGAIN) {
 		ret = 0;
 	} else if (ret < 0) {
+		struct request *rq = blk_mq_rq_from_pdu(queue->request);
 		dev_err(queue->ctrl->ctrl.device,
 			"failed to send request %d\n", ret);
-		if (ret != -EPIPE && ret != -ECONNRESET)
+		if ((ret != -EPIPE && ret != -ECONNRESET) ||
+		    rq->cmd_flags & REQ_FAILFAST_DRIVER)
 			nvme_tcp_fail_request(queue->request);
 		nvme_tcp_done_send_req(queue);
 	}
-- 
2.29.2

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Sagi Grimberg]

> When nvme_tcp_try_send fails to send a request is usually defers the
> error handling to upper layers but it ignores the REQ_FAILFAST_DRIVER
> flag.
> 
> This can lead to very long shutdown delays when the users issues a
> 'nvme disconnect-all':
> 
>   [44888.710527] nvme nvme0: Removing ctrl: NQN "xxx"
>   [44898.981684] nvme nvme0: failed to send request -32
>   [44960.982977] nvme nvme0: queue 0: timeout request 0x18 type 4
>   [44960.983099] nvme nvme0: Property Set error: 881, offset 0x14
> 
> At timestamp [44898.981684] nvme_tcp_try_send fails to send but we have
> to wait for the timeout handler to expires at [44960.982977] before we
> make any progress.
> 
> With honoring the REQ_FAILFAST_DRIVER flag the log looks like this:
> 
>   [ 8999.227495] nvme nvme1: Removing ctrl: NQN "xxx"
>   [ 9016.680447] nvme nvme1: failed to send request -32
>   [ 9016.680477] nvme nvme1: Property Set error: 880, offset 0x14
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>   drivers/nvme/host/tcp.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 10fc45d95b86..f714d7ad38c0 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1145,9 +1145,11 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
>   	if (ret == -EAGAIN) {
>   		ret = 0;
>   	} else if (ret < 0) {
> +		struct request *rq = blk_mq_rq_from_pdu(queue->request);
>   		dev_err(queue->ctrl->ctrl.device,
>   			"failed to send request %d\n", ret);
> -		if (ret != -EPIPE && ret != -ECONNRESET)
> +		if ((ret != -EPIPE && ret != -ECONNRESET) ||
> +		    rq->cmd_flags & REQ_FAILFAST_DRIVER)

I'm wandering if this will race with nvme_cancel_admin_tagset which
can also complete the command... The whole assumption is that the
cancel iterator will be running as the socket is shutdown by the peer
and error_recovery was triggered.

I think these shouldn't race because we shutdown the queue (and flush
the io_work) and only then cancel. But I've seen races of that
sort in the past...

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Daniel Wagner]

On Thu, Apr 07, 2022 at 03:04:42PM +0300, Sagi Grimberg wrote:
> > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > index 10fc45d95b86..f714d7ad38c0 100644
> > --- a/drivers/nvme/host/tcp.c
> > +++ b/drivers/nvme/host/tcp.c
> > @@ -1145,9 +1145,11 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
> >   	if (ret == -EAGAIN) {
> >   		ret = 0;
> >   	} else if (ret < 0) {
> > +		struct request *rq = blk_mq_rq_from_pdu(queue->request);
> >   		dev_err(queue->ctrl->ctrl.device,
> >   			"failed to send request %d\n", ret);
> > -		if (ret != -EPIPE && ret != -ECONNRESET)
> > +		if ((ret != -EPIPE && ret != -ECONNRESET) ||
> > +		    rq->cmd_flags & REQ_FAILFAST_DRIVER)
> 
> I'm wandering if this will race with nvme_cancel_admin_tagset which
> can also complete the command... The whole assumption is that the
> cancel iterator will be running as the socket is shutdown by the peer
> and error_recovery was triggered.
> 
> I think these shouldn't race because we shutdown the queue (and flush
> the io_work) and only then cancel. But I've seen races of that
> sort in the past...

This particular request is issued by nvme_shutdown_ctrl() so we know
exactly where we are. But this might not be the case for any other
REQ_FAILFAST_DRIVER flagged request. So this might introduce the race
you are pointing out.

The option is see is what Margin suggested in

http://lists.infradead.org/pipermail/linux-nvme/2021-September/027867.html

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Sagi Grimberg]

>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>> index 10fc45d95b86..f714d7ad38c0 100644
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>> @@ -1145,9 +1145,11 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
>>>    	if (ret == -EAGAIN) {
>>>    		ret = 0;
>>>    	} else if (ret < 0) {
>>> +		struct request *rq = blk_mq_rq_from_pdu(queue->request);
>>>    		dev_err(queue->ctrl->ctrl.device,
>>>    			"failed to send request %d\n", ret);
>>> -		if (ret != -EPIPE && ret != -ECONNRESET)
>>> +		if ((ret != -EPIPE && ret != -ECONNRESET) ||
>>> +		    rq->cmd_flags & REQ_FAILFAST_DRIVER)
>>
>> I'm wandering if this will race with nvme_cancel_admin_tagset which
>> can also complete the command... The whole assumption is that the
>> cancel iterator will be running as the socket is shutdown by the peer
>> and error_recovery was triggered.
>>
>> I think these shouldn't race because we shutdown the queue (and flush
>> the io_work) and only then cancel. But I've seen races of that
>> sort in the past...
> 
> This particular request is issued by nvme_shutdown_ctrl() so we know
> exactly where we are. But this might not be the case for any other
> REQ_FAILFAST_DRIVER flagged request. So this might introduce the race
> you are pointing out.
> 
> The option is see is what Margin suggested in
> 
> http://lists.infradead.org/pipermail/linux-nvme/2021-September/027867.html

Don't know if I like the state check. Why this does not apply in a ctrl
reset?

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Daniel Wagner]

On Thu, Apr 07, 2022 at 04:04:44PM +0300, Sagi Grimberg wrote:
>
> > > > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> > > > index 10fc45d95b86..f714d7ad38c0 100644
> > > > --- a/drivers/nvme/host/tcp.c
> > > > +++ b/drivers/nvme/host/tcp.c
> > > > @@ -1145,9 +1145,11 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue *queue)
> > > >         if (ret == -EAGAIN) {
> > > >                 ret = 0;
> > > >         } else if (ret < 0) {
> > > > +		struct request *rq = blk_mq_rq_from_pdu(queue->request);
> > > >                 dev_err(queue->ctrl->ctrl.device,
> > > >                         "failed to send request %d\n", ret);
> > > > -		if (ret != -EPIPE && ret != -ECONNRESET)
> > > > +		if ((ret != -EPIPE && ret != -ECONNRESET) ||
> > > > +		    rq->cmd_flags & REQ_FAILFAST_DRIVER)
> > >
> > > I'm wandering if this will race with nvme_cancel_admin_tagset which
> > > can also complete the command... The whole assumption is that the
> > > cancel iterator will be running as the socket is shutdown by the peer
> > > and error_recovery was triggered.
> > >
> > > I think these shouldn't race because we shutdown the queue (and flush
> > > the io_work) and only then cancel. But I've seen races of that
> > > sort in the past...
> >
> > This particular request is issued by nvme_shutdown_ctrl() so we know
> > exactly where we are. But this might not be the case for any other
> > REQ_FAILFAST_DRIVER flagged request. So this might introduce the race
> > you are pointing out.
> >
> > The option is see is what Margin suggested in
> >
> > http://lists.infradead.org/pipermail/linux-nvme/2021-September/027867.html
>
> Don't know if I like the state check. Why this does not apply in a ctrl
> reset?

Understood. Would something like this work

      struct request *rq = blk_mq_rq_from_pdu(queue->request);
      bool queue_ready = test_bit(NVME_TCP_Q_LIVE, &queue->flags);

      dev_err(queue->ctrl->ctrl.device,
              "failed to send request %d\n", ret);
      if ((ret != -EPIPE && ret != -ECONNRESET) ||
          (!queue_ready && rq->cmd_flags & REQ_FAILFAST_DRIVER))
              nvme_tcp_fail_request(queue->request);

?

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Sagi Grimberg]

>>>> I'm wandering if this will race with nvme_cancel_admin_tagset which
>>>> can also complete the command... The whole assumption is that the
>>>> cancel iterator will be running as the socket is shutdown by the peer
>>>> and error_recovery was triggered.
>>>>
>>>> I think these shouldn't race because we shutdown the queue (and flush
>>>> the io_work) and only then cancel. But I've seen races of that
>>>> sort in the past...
>>>
>>> This particular request is issued by nvme_shutdown_ctrl() so we know
>>> exactly where we are. But this might not be the case for any other
>>> REQ_FAILFAST_DRIVER flagged request. So this might introduce the race
>>> you are pointing out.
>>>
>>> The option is see is what Margin suggested in
>>>
>>> http://lists.infradead.org/pipermail/linux-nvme/2021-September/027867.html
>>
>> Don't know if I like the state check. Why this does not apply in a ctrl
>> reset?
> 
> Understood. Would something like this work
> 
>        struct request *rq = blk_mq_rq_from_pdu(queue->request);
>        bool queue_ready = test_bit(NVME_TCP_Q_LIVE, &queue->flags);
> 
>        dev_err(queue->ctrl->ctrl.device,
>                "failed to send request %d\n", ret);
>        if ((ret != -EPIPE && ret != -ECONNRESET) ||
>            (!queue_ready && rq->cmd_flags & REQ_FAILFAST_DRIVER))
>                nvme_tcp_fail_request(queue->request);
> 
> ?

But in your case the queue is live...

I'm thinking that we maybe need a register access timeout value for
fabrics...

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Daniel Wagner]

On Thu, Apr 07, 2022 at 04:42:55PM +0300, Sagi Grimberg wrote:
> > Understood. Would something like this work
> > 
> >        struct request *rq = blk_mq_rq_from_pdu(queue->request);
> >        bool queue_ready = test_bit(NVME_TCP_Q_LIVE, &queue->flags);
> > 
> >        dev_err(queue->ctrl->ctrl.device,
> >                "failed to send request %d\n", ret);
> >        if ((ret != -EPIPE && ret != -ECONNRESET) ||
> >            (!queue_ready && rq->cmd_flags & REQ_FAILFAST_DRIVER))
> >                nvme_tcp_fail_request(queue->request);
> > 
> > ?
> 
> But in your case the queue is live...

Could we not just test for NVME_CTRL_ADMIN_Q_STOPPED instead as it is
set in nvme_stop_admin_queue:

nvme_tcp_teardown_ctrl
  nvme_stop_admin_queue
  nvme_shutdown_ctrl

instead?

> I'm thinking that we maybe need a register access timeout value for
> fabrics...

I see there is a shutdown_timeout which is 5 seconds on default. Seems
not to trigger though.

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Daniel Wagner]

On Thu, Apr 07, 2022 at 03:53:54PM +0200, Daniel Wagner wrote:
> On Thu, Apr 07, 2022 at 04:42:55PM +0300, Sagi Grimberg wrote:
> > > Understood. Would something like this work
> > > 
> > >        struct request *rq = blk_mq_rq_from_pdu(queue->request);
> > >        bool queue_ready = test_bit(NVME_TCP_Q_LIVE, &queue->flags);
> > > 
> > >        dev_err(queue->ctrl->ctrl.device,
> > >                "failed to send request %d\n", ret);
> > >        if ((ret != -EPIPE && ret != -ECONNRESET) ||
> > >            (!queue_ready && rq->cmd_flags & REQ_FAILFAST_DRIVER))
> > >                nvme_tcp_fail_request(queue->request);
> > > 
> > > ?
> > 
> > But in your case the queue is live...
> 
> Could we not just test for NVME_CTRL_ADMIN_Q_STOPPED instead as it is
> set in nvme_stop_admin_queue:
> 
> nvme_tcp_teardown_ctrl
>   nvme_stop_admin_queue
>   nvme_shutdown_ctrl
> 
> instead?
> 
> > I'm thinking that we maybe need a register access timeout value for
> > fabrics...
> 
> I see there is a shutdown_timeout which is 5 seconds on default. Seems
> not to trigger though.

Obviously, the shutdown_timeout is not connected to the register
writes. So we would need something like:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f204c6f78b5b..a146ea25f386 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2133,7 +2133,8 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u64 cap, bool enabled)
 	u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
 	int ret;
 
-	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
+	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS,
+					    &csts, timeout)) == 0) {
 		if (csts == ~0)
 			return -ENODEV;
 		if ((csts & NVME_CSTS_RDY) == bit)
@@ -2166,7 +2167,8 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
 	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
 	ctrl->ctrl_config &= ~NVME_CC_ENABLE;
 
-	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
+	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
+				     NVME_REG_ACCESS_TIMEOUT);
 	if (ret)
 		return ret;
 
@@ -2182,7 +2184,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	unsigned dev_page_min;
 	int ret;
 
-	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
+	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap,
+				    NVME_REG_ACCESS_TIMEOUT);
 	if (ret) {
 		dev_err(ctrl->device, "Reading CAP failed (%d)\n", ret);
 		return ret;
@@ -2205,7 +2208,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
 	ctrl->ctrl_config |= NVME_CC_ENABLE;
 
-	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
+	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
+				     NVME_REG_ACCESS_TIMEOUT);
 	if (ret)
 		return ret;
 	return nvme_wait_ready(ctrl, ctrl->cap, true);
@@ -2221,11 +2225,13 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
 	ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
 
-	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
+	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
+				     NVME_REG_ACCESS_TIMEOUT);
 	if (ret)
 		return ret;
 
-	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
+	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts,
+					    NVME_REG_ACCESS_TIMEOUT)) == 0) {
 		if ((csts & NVME_CSTS_SHST_MASK) == NVME_CSTS_SHST_CMPLT)
 			break;
 
@@ -3084,7 +3090,8 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
 {
 	int ret;
 
-	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
+	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs,
+				    NVME_REG_ACCESS_TIMEOUT);
 	if (ret) {
 		dev_err(ctrl->device, "Reading VS failed (%d)\n", ret);
 		return ret;
@@ -4386,7 +4393,8 @@ static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
 
 	u32 csts;
 
-	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
+	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts,
+				  NVME_REG_ACCESS_TIMEOUT))
 		return false;
 
 	if (csts == ~0)
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index ee79a6d639b4..7882559e89bc 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -142,7 +142,7 @@ EXPORT_SYMBOL_GPL(nvmf_get_address);
  *	> 0: NVMe error status code
  *	< 0: Linux errno error code
  */
-int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
+int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val, int timeout)
 {
 	struct nvme_command cmd = { };
 	union nvme_result res;
@@ -152,8 +152,8 @@ int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
 	cmd.prop_get.fctype = nvme_fabrics_type_property_get;
 	cmd.prop_get.offset = cpu_to_le32(off);
 
-	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
-			NVME_QID_ANY, 0, 0);
+	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0,
+				     timeout, NVME_QID_ANY, 0, 0);
 
 	if (ret >= 0)
 		*val = le64_to_cpu(res.u64);
@@ -187,7 +187,7 @@ EXPORT_SYMBOL_GPL(nvmf_reg_read32);
  *	> 0: NVMe error status code
  *	< 0: Linux errno error code
  */
-int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
+int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val, int timeout)
 {
 	struct nvme_command cmd = { };
 	union nvme_result res;
@@ -198,8 +198,8 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
 	cmd.prop_get.attrib = 1;
 	cmd.prop_get.offset = cpu_to_le32(off);
 
-	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
-			NVME_QID_ANY, 0, 0);
+	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0,
+				     timeout, NVME_QID_ANY, 0, 0);
 
 	if (ret >= 0)
 		*val = le64_to_cpu(res.u64);
@@ -232,7 +232,7 @@ EXPORT_SYMBOL_GPL(nvmf_reg_read64);
  *	> 0: NVMe error status code
  *	< 0: Linux errno error code
  */
-int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
+int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val, int timeout)
 {
 	struct nvme_command cmd = { };
 	int ret;
@@ -243,8 +243,8 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
 	cmd.prop_set.offset = cpu_to_le32(off);
 	cmd.prop_set.value = cpu_to_le64(val);
 
-	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
-			NVME_QID_ANY, 0, 0);
+	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0,
+				     timeout, NVME_QID_ANY, 0, 0);
 	if (unlikely(ret))
 		dev_err(ctrl->device,
 			"Property Set error: %d, offset %#x\n",
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index c3203ff1c654..c2ae0c04cd4a 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -186,9 +186,9 @@ static inline char *nvmf_ctrl_subsysnqn(struct nvme_ctrl *ctrl)
 	return ctrl->subsys->subnqn;
 }
 
-int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val);
-int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val);
-int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val);
+int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val, int timeout);
+int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val, int timeout);
+int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val, int timeout);
 int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl);
 int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid);
 int nvmf_register_transport(struct nvmf_transport_ops *ops);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a1f8403ffd78..f17a4df17e11 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -27,6 +27,8 @@ extern unsigned int admin_timeout;
 
 #define NVME_DEFAULT_KATO	5
 
+#define NVME_REG_ACCESS_TIMEOUT NVME_DEFAULT_KATO
+
 #ifdef CONFIG_ARCH_NO_SG_CHAIN
 #define  NVME_INLINE_SG_CNT  0
 #define  NVME_INLINE_METADATA_SG_CNT  0
@@ -489,9 +491,12 @@ struct nvme_ctrl_ops {
 #define NVME_F_FABRICS			(1 << 0)
 #define NVME_F_METADATA_SUPPORTED	(1 << 1)
 #define NVME_F_PCI_P2PDMA		(1 << 2)
-	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
-	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
-	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
+	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val,
+			  int timeout);
+	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val,
+			   int timoeut);
+	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val,
+			  int timeout);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
 	void (*submit_async_event)(struct nvme_ctrl *ctrl);
 	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
@@ -561,7 +566,8 @@ static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
 {
 	if (!ctrl->subsystem)
 		return -ENOTTY;
-	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 0x4E564D65);
+	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 0x4E564D65,
+				      NVME_REG_ACCESS_TIMEOUT);
 }
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 66f1eee2509a..b7246fee99ef 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2943,19 +2943,22 @@ static void nvme_remove_dead_ctrl_work(struct work_struct *work)
 	nvme_put_ctrl(&dev->ctrl);
 }
 
-static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
+static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val,
+			       int timeout)
 {
 	*val = readl(to_nvme_dev(ctrl)->bar + off);
 	return 0;
 }
 
-static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
+static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val,
+				int timeout)
 {
 	writel(val, to_nvme_dev(ctrl)->bar + off);
 	return 0;
 }
 
-static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
+static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val,
+			       int timeout)
 {
 	*val = lo_hi_readq(to_nvme_dev(ctrl)->bar + off);
 	return 0;

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Sagi Grimberg]

>>> But in your case the queue is live...
>>
>> Could we not just test for NVME_CTRL_ADMIN_Q_STOPPED instead as it is
>> set in nvme_stop_admin_queue:
>>
>> nvme_tcp_teardown_ctrl
>>    nvme_stop_admin_queue
>>    nvme_shutdown_ctrl
>>
>> instead?
>>
>>> I'm thinking that we maybe need a register access timeout value for
>>> fabrics...
>>
>> I see there is a shutdown_timeout which is 5 seconds on default. Seems
>> not to trigger though.
> 
> Obviously, the shutdown_timeout is not connected to the register
> writes. So we would need something like:

Looking at this, its probably not what we want...

I've inspected the code and I don't think that we should check
the return code at all, we should simply fail the request. If
the io_work context is running in parallel with the the error
recovery handler we have a bug that we need to fix.

How about this?
--
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 10fc45d95b86..6d59867bb275 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1147,8 +1147,7 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue 
*queue)
         } else if (ret < 0) {
                 dev_err(queue->ctrl->ctrl.device,
                         "failed to send request %d\n", ret);
-               if (ret != -EPIPE && ret != -ECONNRESET)
-                       nvme_tcp_fail_request(queue->request);
+               nvme_tcp_fail_request(req);
                 nvme_tcp_done_send_req(queue);
         }
         return ret;
--

Adding Anton,

Anton, do you see an issue with this change? io_work is safely
fenced from the cancellation iter so there is no reason for
us not to fail the request here when the socket closes (or
any other reason for that matter).


> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f204c6f78b5b..a146ea25f386 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2133,7 +2133,8 @@ static int nvme_wait_ready(struct nvme_ctrl *ctrl, u64 cap, bool enabled)
>   	u32 csts, bit = enabled ? NVME_CSTS_RDY : 0;
>   	int ret;
>   
> -	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
> +	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS,
> +					    &csts, timeout)) == 0) {
>   		if (csts == ~0)
>   			return -ENODEV;
>   		if ((csts & NVME_CSTS_RDY) == bit)
> @@ -2166,7 +2167,8 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl)
>   	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
>   	ctrl->ctrl_config &= ~NVME_CC_ENABLE;
>   
> -	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> +	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
> +				     NVME_REG_ACCESS_TIMEOUT);
>   	if (ret)
>   		return ret;
>   
> @@ -2182,7 +2184,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>   	unsigned dev_page_min;
>   	int ret;
>   
> -	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
> +	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap,
> +				    NVME_REG_ACCESS_TIMEOUT);
>   	if (ret) {
>   		dev_err(ctrl->device, "Reading CAP failed (%d)\n", ret);
>   		return ret;
> @@ -2205,7 +2208,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>   	ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
>   	ctrl->ctrl_config |= NVME_CC_ENABLE;
>   
> -	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> +	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
> +				     NVME_REG_ACCESS_TIMEOUT);
>   	if (ret)
>   		return ret;
>   	return nvme_wait_ready(ctrl, ctrl->cap, true);
> @@ -2221,11 +2225,13 @@ int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
>   	ctrl->ctrl_config &= ~NVME_CC_SHN_MASK;
>   	ctrl->ctrl_config |= NVME_CC_SHN_NORMAL;
>   
> -	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> +	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config,
> +				     NVME_REG_ACCESS_TIMEOUT);
>   	if (ret)
>   		return ret;
>   
> -	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
> +	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts,
> +					    NVME_REG_ACCESS_TIMEOUT)) == 0) {
>   		if ((csts & NVME_CSTS_SHST_MASK) == NVME_CSTS_SHST_CMPLT)
>   			break;
>   
> @@ -3084,7 +3090,8 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl)
>   {
>   	int ret;
>   
> -	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
> +	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs,
> +				    NVME_REG_ACCESS_TIMEOUT);
>   	if (ret) {
>   		dev_err(ctrl->device, "Reading VS failed (%d)\n", ret);
>   		return ret;
> @@ -4386,7 +4393,8 @@ static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
>   
>   	u32 csts;
>   
> -	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
> +	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts,
> +				  NVME_REG_ACCESS_TIMEOUT))
>   		return false;
>   
>   	if (csts == ~0)
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index ee79a6d639b4..7882559e89bc 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -142,7 +142,7 @@ EXPORT_SYMBOL_GPL(nvmf_get_address);
>    *	> 0: NVMe error status code
>    *	< 0: Linux errno error code
>    */
> -int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
> +int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val, int timeout)
>   {
>   	struct nvme_command cmd = { };
>   	union nvme_result res;
> @@ -152,8 +152,8 @@ int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
>   	cmd.prop_get.fctype = nvme_fabrics_type_property_get;
>   	cmd.prop_get.offset = cpu_to_le32(off);
>   
> -	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
> -			NVME_QID_ANY, 0, 0);
> +	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0,
> +				     timeout, NVME_QID_ANY, 0, 0);
>   
>   	if (ret >= 0)
>   		*val = le64_to_cpu(res.u64);
> @@ -187,7 +187,7 @@ EXPORT_SYMBOL_GPL(nvmf_reg_read32);
>    *	> 0: NVMe error status code
>    *	< 0: Linux errno error code
>    */
> -int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
> +int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val, int timeout)
>   {
>   	struct nvme_command cmd = { };
>   	union nvme_result res;
> @@ -198,8 +198,8 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
>   	cmd.prop_get.attrib = 1;
>   	cmd.prop_get.offset = cpu_to_le32(off);
>   
> -	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
> -			NVME_QID_ANY, 0, 0);
> +	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0,
> +				     timeout, NVME_QID_ANY, 0, 0);
>   
>   	if (ret >= 0)
>   		*val = le64_to_cpu(res.u64);
> @@ -232,7 +232,7 @@ EXPORT_SYMBOL_GPL(nvmf_reg_read64);
>    *	> 0: NVMe error status code
>    *	< 0: Linux errno error code
>    */
> -int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
> +int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val, int timeout)
>   {
>   	struct nvme_command cmd = { };
>   	int ret;
> @@ -243,8 +243,8 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
>   	cmd.prop_set.offset = cpu_to_le32(off);
>   	cmd.prop_set.value = cpu_to_le64(val);
>   
> -	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
> -			NVME_QID_ANY, 0, 0);
> +	ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0,
> +				     timeout, NVME_QID_ANY, 0, 0);
>   	if (unlikely(ret))
>   		dev_err(ctrl->device,
>   			"Property Set error: %d, offset %#x\n",
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index c3203ff1c654..c2ae0c04cd4a 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -186,9 +186,9 @@ static inline char *nvmf_ctrl_subsysnqn(struct nvme_ctrl *ctrl)
>   	return ctrl->subsys->subnqn;
>   }
>   
> -int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val);
> -int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val);
> -int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val);
> +int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val, int timeout);
> +int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val, int timeout);
> +int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val, int timeout);
>   int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl);
>   int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid);
>   int nvmf_register_transport(struct nvmf_transport_ops *ops);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index a1f8403ffd78..f17a4df17e11 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -27,6 +27,8 @@ extern unsigned int admin_timeout;
>   
>   #define NVME_DEFAULT_KATO	5
>   
> +#define NVME_REG_ACCESS_TIMEOUT NVME_DEFAULT_KATO
> +
>   #ifdef CONFIG_ARCH_NO_SG_CHAIN
>   #define  NVME_INLINE_SG_CNT  0
>   #define  NVME_INLINE_METADATA_SG_CNT  0
> @@ -489,9 +491,12 @@ struct nvme_ctrl_ops {
>   #define NVME_F_FABRICS			(1 << 0)
>   #define NVME_F_METADATA_SUPPORTED	(1 << 1)
>   #define NVME_F_PCI_P2PDMA		(1 << 2)
> -	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
> -	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
> -	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
> +	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val,
> +			  int timeout);
> +	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val,
> +			   int timoeut);
> +	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val,
> +			  int timeout);
>   	void (*free_ctrl)(struct nvme_ctrl *ctrl);
>   	void (*submit_async_event)(struct nvme_ctrl *ctrl);
>   	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
> @@ -561,7 +566,8 @@ static inline int nvme_reset_subsystem(struct nvme_ctrl *ctrl)
>   {
>   	if (!ctrl->subsystem)
>   		return -ENOTTY;
> -	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 0x4E564D65);
> +	return ctrl->ops->reg_write32(ctrl, NVME_REG_NSSR, 0x4E564D65,
> +				      NVME_REG_ACCESS_TIMEOUT);
>   }
>   
>   /*
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 66f1eee2509a..b7246fee99ef 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2943,19 +2943,22 @@ static void nvme_remove_dead_ctrl_work(struct work_struct *work)
>   	nvme_put_ctrl(&dev->ctrl);
>   }
>   
> -static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
> +static int nvme_pci_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val,
> +			       int timeout)
>   {
>   	*val = readl(to_nvme_dev(ctrl)->bar + off);
>   	return 0;
>   }
>   
> -static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
> +static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val,
> +				int timeout)
>   {
>   	writel(val, to_nvme_dev(ctrl)->bar + off);
>   	return 0;
>   }
>   
> -static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
> +static int nvme_pci_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val,
> +			       int timeout)
>   {
>   	*val = lo_hi_readq(to_nvme_dev(ctrl)->bar + off);
>   	return 0;

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Daniel Wagner]

On Mon, Apr 11, 2022 at 12:58:09PM +0300, Sagi Grimberg wrote:
> > > I see there is a shutdown_timeout which is 5 seconds on default. Seems
> > > not to trigger though.
> > 
> > Obviously, the shutdown_timeout is not connected to the register
> > writes. So we would need something like:
> 
> Looking at this, its probably not what we want...

I agree. We just have to make sure that we fail these register
read/writes on transport layer and don't requeue them.

> I've inspected the code and I don't think that we should check
> the return code at all, we should simply fail the request. If
> the io_work context is running in parallel with the the error
> recovery handler we have a bug that we need to fix.
> 
> How about this?

Yes this would work fine for my case.

> --
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 10fc45d95b86..6d59867bb275 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1147,8 +1147,7 @@ static int nvme_tcp_try_send(struct nvme_tcp_queue
> *queue)
>         } else if (ret < 0) {
>                 dev_err(queue->ctrl->ctrl.device,
>                         "failed to send request %d\n", ret);
> -               if (ret != -EPIPE && ret != -ECONNRESET)
> -                       nvme_tcp_fail_request(queue->request);
> +               nvme_tcp_fail_request(req);
>                 nvme_tcp_done_send_req(queue);
>         }
>         return ret;
> --
> 
> Adding Anton,
> 
> Anton, do you see an issue with this change? io_work is safely
> fenced from the cancellation iter so there is no reason for
> us not to fail the request here when the socket closes (or
> any other reason for that matter).

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Daniel Wagner]

Hi Sagi,

On Mon, Apr 11, 2022 at 01:22:18PM +0200, Daniel Wagner wrote:
> On Mon, Apr 11, 2022 at 12:58:09PM +0300, Sagi Grimberg wrote:
> > > > I see there is a shutdown_timeout which is 5 seconds on default. Seems
> > > > not to trigger though.
> > > 
> > > Obviously, the shutdown_timeout is not connected to the register
> > > writes. So we would need something like:
> > 
> > Looking at this, its probably not what we want...
> 
> I agree. We just have to make sure that we fail these register
> read/writes on transport layer and don't requeue them.
> 
> > I've inspected the code and I don't think that we should check
> > the return code at all, we should simply fail the request. If
> > the io_work context is running in parallel with the the error
> > recovery handler we have a bug that we need to fix.
> > 
> > How about this?
> 
> Yes this would work fine for my case.

Do you want me to post this patch or are you going to post it?

Thanks,
Daniel

Re: [PATCH] nvme-tcp: handle failing req immediately for REQ_FAILFAST_DRIVER

[Sagi Grimberg]

>>>>> I see there is a shutdown_timeout which is 5 seconds on default. Seems
>>>>> not to trigger though.
>>>>
>>>> Obviously, the shutdown_timeout is not connected to the register
>>>> writes. So we would need something like:
>>>
>>> Looking at this, its probably not what we want...
>>
>> I agree. We just have to make sure that we fail these register
>> read/writes on transport layer and don't requeue them.
>>
>>> I've inspected the code and I don't think that we should check
>>> the return code at all, we should simply fail the request. If
>>> the io_work context is running in parallel with the the error
>>> recovery handler we have a bug that we need to fix.
>>>
>>> How about this?
>>
>> Yes this would work fine for my case.
> 
> Do you want me to post this patch or are you going to post it?

Would like to get Anton's feedback on it, will ping him to reply
soon.