[PATCH v4 0/5] avoid double request completion and IO error

[PATCH v4 0/5] avoid double request completion and IO error

[Chao Leng]

Add blk_mq_set_request_complete and nvme_complete_failed_req for two bug
fixs.
First avoid double request completion for nvmf_fail_nonready_command.
Second avoid IO error for nvme native multipath.

V4:
	- add status parameter for nvme_complete_failed_req.
	- export nvme_complete_failed_req instead of inline function.
V3:
	- complete the request just for HBA driver path related error.
V2:
	- use "switch" instead "if" to check return status.

Chao Leng (5):
  blk-mq: introduce blk_mq_set_request_complete
  nvme-core: introduce complete failed request
  nvme-fabrics: avoid double request completion for
    nvmf_fail_nonready_command
  nvme-rdma: avoid IO error for nvme native multipath
  nvme-fc: avoid IO error for nvme native multipath

 drivers/nvme/host/core.c    | 8 ++++++++
 drivers/nvme/host/fabrics.c | 4 +---
 drivers/nvme/host/fc.c      | 7 ++++++-
 drivers/nvme/host/nvme.h    | 1 +
 drivers/nvme/host/rdma.c    | 9 ++++++++-
 include/linux/blk-mq.h      | 5 +++++
 6 files changed, 29 insertions(+), 5 deletions(-)

-- 
2.16.4

[PATCH v4 0/5] avoid double request completion and IO error

[Chao Leng]

Add blk_mq_set_request_complete and nvme_complete_failed_req for two bug
fixs.
First avoid double request completion for nvmf_fail_nonready_command.
Second avoid IO error for nvme native multipath.

V4:
	- add status parameter for nvme_complete_failed_req.
	- export nvme_complete_failed_req instead of inline function.
V3:
	- complete the request just for HBA driver path related error.
V2:
	- use "switch" instead "if" to check return status.

Chao Leng (5):
  blk-mq: introduce blk_mq_set_request_complete
  nvme-core: introduce complete failed request
  nvme-fabrics: avoid double request completion for
    nvmf_fail_nonready_command
  nvme-rdma: avoid IO error for nvme native multipath
  nvme-fc: avoid IO error for nvme native multipath

 drivers/nvme/host/core.c    | 8 ++++++++
 drivers/nvme/host/fabrics.c | 4 +---
 drivers/nvme/host/fc.c      | 7 ++++++-
 drivers/nvme/host/nvme.h    | 1 +
 drivers/nvme/host/rdma.c    | 9 ++++++++-
 include/linux/blk-mq.h      | 5 +++++
 6 files changed, 29 insertions(+), 5 deletions(-)

-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 1/5] blk-mq: introduce blk_mq_set_request_complete

[Chao Leng]

nvme drivers need to set the state of request to MQ_RQ_COMPLETE when
directly complete request in queue_rq.
So add blk_mq_set_request_complete.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 include/linux/blk-mq.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 47b021952ac7..fc096b04bb7a 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -494,6 +494,11 @@ static inline int blk_mq_request_completed(struct request *rq)
 	return blk_mq_rq_state(rq) == MQ_RQ_COMPLETE;
 }
 
+static inline void blk_mq_set_request_complete(struct request *rq)
+{
+	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
+}
+
 void blk_mq_start_request(struct request *rq);
 void blk_mq_end_request(struct request *rq, blk_status_t error);
 void __blk_mq_end_request(struct request *rq, blk_status_t error);
-- 
2.16.4

[PATCH v4 1/5] blk-mq: introduce blk_mq_set_request_complete

[Chao Leng]

nvme drivers need to set the state of request to MQ_RQ_COMPLETE when
directly complete request in queue_rq.
So add blk_mq_set_request_complete.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 include/linux/blk-mq.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 47b021952ac7..fc096b04bb7a 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -494,6 +494,11 @@ static inline int blk_mq_request_completed(struct request *rq)
 	return blk_mq_rq_state(rq) == MQ_RQ_COMPLETE;
 }
 
+static inline void blk_mq_set_request_complete(struct request *rq)
+{
+	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
+}
+
 void blk_mq_start_request(struct request *rq);
 void blk_mq_end_request(struct request *rq, blk_status_t error);
 void __blk_mq_end_request(struct request *rq, blk_status_t error);
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 1/5] blk-mq: introduce blk_mq_set_request_complete

[Christoph Hellwig]

> +static inline void blk_mq_set_request_complete(struct request *rq)
> +{
> +	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
> +}

This function needs a detailed comment explaining the use case.
Otherwise we'll get lots of driver abuses.

Re: [PATCH v4 1/5] blk-mq: introduce blk_mq_set_request_complete

[Christoph Hellwig]

> +static inline void blk_mq_set_request_complete(struct request *rq)
> +{
> +	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
> +}

This function needs a detailed comment explaining the use case.
Otherwise we'll get lots of driver abuses.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 1/5] blk-mq: introduce blk_mq_set_request_complete

[Chao Leng]

On 2021/1/28 0:40, Christoph Hellwig wrote:
>> +static inline void blk_mq_set_request_complete(struct request *rq)
>> +{
>> +	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
>> +}
> 
> This function needs a detailed comment explaining the use case.
> Otherwise we'll get lots of driver abuses.
ok, thanks.
> .
> 

Re: [PATCH v4 1/5] blk-mq: introduce blk_mq_set_request_complete

[Chao Leng]

On 2021/1/28 0:40, Christoph Hellwig wrote:
>> +static inline void blk_mq_set_request_complete(struct request *rq)
>> +{
>> +	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
>> +}
> 
> This function needs a detailed comment explaining the use case.
> Otherwise we'll get lots of driver abuses.
ok, thanks.
> .
> 

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 2/5] nvme-core: introduce complete failed request

[Chao Leng]

Work with nvme native multipath, if a path related error occurs when
queue_rq call HBA drive to send request, queue_rq need complete the
request with NVME_SC_HOST_PATH_ERROR, the request will fail over to
retry if needed.
So introduce nvme_complete_failed_req for queue_rq and
nvmf_fail_nonready_command.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/core.c | 8 ++++++++
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8caf9b34734d..9aad4111d9cd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -356,6 +356,14 @@ void nvme_complete_rq(struct request *req)
 }
 EXPORT_SYMBOL_GPL(nvme_complete_rq);
 
+void nvme_complete_failed_rq(struct request *req, u16 status)
+{
+	nvme_req(req)->status = status;
+	blk_mq_set_request_complete(req);
+	nvme_complete_rq(req);
+}
+EXPORT_SYMBOL_GPL(nvme_complete_failed_rq);
+
 bool nvme_cancel_request(struct request *req, void *data, bool reserved)
 {
 	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 88a6b97247f5..f0f609644309 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -575,6 +575,7 @@ static inline bool nvme_is_aen_req(u16 qid, __u16 command_id)
 }
 
 void nvme_complete_rq(struct request *req);
+void nvme_complete_failed_rq(struct request *req, u16 status);
 bool nvme_cancel_request(struct request *req, void *data, bool reserved);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
-- 
2.16.4

[PATCH v4 2/5] nvme-core: introduce complete failed request

[Chao Leng]

Work with nvme native multipath, if a path related error occurs when
queue_rq call HBA drive to send request, queue_rq need complete the
request with NVME_SC_HOST_PATH_ERROR, the request will fail over to
retry if needed.
So introduce nvme_complete_failed_req for queue_rq and
nvmf_fail_nonready_command.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/core.c | 8 ++++++++
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8caf9b34734d..9aad4111d9cd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -356,6 +356,14 @@ void nvme_complete_rq(struct request *req)
 }
 EXPORT_SYMBOL_GPL(nvme_complete_rq);
 
+void nvme_complete_failed_rq(struct request *req, u16 status)
+{
+	nvme_req(req)->status = status;
+	blk_mq_set_request_complete(req);
+	nvme_complete_rq(req);
+}
+EXPORT_SYMBOL_GPL(nvme_complete_failed_rq);
+
 bool nvme_cancel_request(struct request *req, void *data, bool reserved)
 {
 	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 88a6b97247f5..f0f609644309 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -575,6 +575,7 @@ static inline bool nvme_is_aen_req(u16 qid, __u16 command_id)
 }
 
 void nvme_complete_rq(struct request *req);
+void nvme_complete_failed_rq(struct request *req, u16 status);
 bool nvme_cancel_request(struct request *req, void *data, bool reserved);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 3/5] nvme-fabrics: avoid double request completion for nvmf_fail_nonready_command

[Chao Leng]

When reconnect, the request may be completed with NVME_SC_HOST_PATH_ERROR
in nvmf_fail_nonready_command. The state of request will be changed to
MQ_RQ_IN_FLIGHT before call nvme_complete_rq. If free the request
asynchronously such as in nvme_submit_user_cmd, in extreme scenario
the request may be completed again in tear down process.
nvmf_fail_nonready_command do not need calling blk_mq_start_request
before complete the request. nvmf_fail_nonready_command should set
the state of request to MQ_RQ_COMPLETE before complete the request.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/fabrics.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 72ac00173500..40f40c0debc1 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -553,9 +553,7 @@ blk_status_t nvmf_fail_nonready_command(struct nvme_ctrl *ctrl,
 	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
 		return BLK_STS_RESOURCE;
 
-	nvme_req(rq)->status = NVME_SC_HOST_PATH_ERROR;
-	blk_mq_start_request(rq);
-	nvme_complete_rq(rq);
+	nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
 	return BLK_STS_OK;
 }
 EXPORT_SYMBOL_GPL(nvmf_fail_nonready_command);
-- 
2.16.4

[PATCH v4 3/5] nvme-fabrics: avoid double request completion for nvmf_fail_nonready_command

[Chao Leng]

When reconnect, the request may be completed with NVME_SC_HOST_PATH_ERROR
in nvmf_fail_nonready_command. The state of request will be changed to
MQ_RQ_IN_FLIGHT before call nvme_complete_rq. If free the request
asynchronously such as in nvme_submit_user_cmd, in extreme scenario
the request may be completed again in tear down process.
nvmf_fail_nonready_command do not need calling blk_mq_start_request
before complete the request. nvmf_fail_nonready_command should set
the state of request to MQ_RQ_COMPLETE before complete the request.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/fabrics.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 72ac00173500..40f40c0debc1 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -553,9 +553,7 @@ blk_status_t nvmf_fail_nonready_command(struct nvme_ctrl *ctrl,
 	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
 		return BLK_STS_RESOURCE;
 
-	nvme_req(rq)->status = NVME_SC_HOST_PATH_ERROR;
-	blk_mq_start_request(rq);
-	nvme_complete_rq(rq);
+	nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
 	return BLK_STS_OK;
 }
 EXPORT_SYMBOL_GPL(nvmf_fail_nonready_command);
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

Work with nvme native multipath, if a path related error occurs when
queue_rq call HBA drive to send request, queue_rq will return
BLK_STS_IOERR to blk-mq. The request is completed with BLK_STS_IOERR
instead of fail over to retry.
queue_rq need call nvme_complete_rq to complete the request with
NVME_SC_HOST_PATH_ERROR, the request will fail over to retry if needed.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/rdma.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index b7ce4f221d99..7c801132d5ed 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2037,6 +2037,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	bool queue_ready = test_bit(NVME_RDMA_Q_LIVE, &queue->flags);
 	blk_status_t ret;
 	int err;
+	bool driver_error = false;
 
 	WARN_ON_ONCE(rq->tag < 0);
 
@@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
 			req->mr ? &req->reg_wr.wr : NULL);
-	if (unlikely(err))
+	if (unlikely(err)) {
+		driver_error = true;
 		goto err_unmap;
+	}
 
 	return BLK_STS_OK;
 
@@ -2100,6 +2103,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 unmap_qe:
 	ib_dma_unmap_single(dev, req->sqe.dma, sizeof(struct nvme_command),
 			    DMA_TO_DEVICE);
+	if (driver_error && ret == BLK_STS_IOERR) {
+		nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
+		ret = BLK_STS_OK;
+	}
 	return ret;
 }
 
-- 
2.16.4

[PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

Work with nvme native multipath, if a path related error occurs when
queue_rq call HBA drive to send request, queue_rq will return
BLK_STS_IOERR to blk-mq. The request is completed with BLK_STS_IOERR
instead of fail over to retry.
queue_rq need call nvme_complete_rq to complete the request with
NVME_SC_HOST_PATH_ERROR, the request will fail over to retry if needed.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/rdma.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index b7ce4f221d99..7c801132d5ed 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2037,6 +2037,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 	bool queue_ready = test_bit(NVME_RDMA_Q_LIVE, &queue->flags);
 	blk_status_t ret;
 	int err;
+	bool driver_error = false;
 
 	WARN_ON_ONCE(rq->tag < 0);
 
@@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
 			req->mr ? &req->reg_wr.wr : NULL);
-	if (unlikely(err))
+	if (unlikely(err)) {
+		driver_error = true;
 		goto err_unmap;
+	}
 
 	return BLK_STS_OK;
 
@@ -2100,6 +2103,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
 unmap_qe:
 	ib_dma_unmap_single(dev, req->sqe.dma, sizeof(struct nvme_command),
 			    DMA_TO_DEVICE);
+	if (driver_error && ret == BLK_STS_IOERR) {
+		nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
+		ret = BLK_STS_OK;
+	}
 	return ret;
 }
 
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>   
>   	err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>   			req->mr ? &req->reg_wr.wr : NULL);
> -	if (unlikely(err))
> +	if (unlikely(err)) {
> +		driver_error = true;
>   		goto err_unmap;

Why not just call set the status and call nvme_rdma_complete_rq and
return here?

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>   
>   	err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>   			req->mr ? &req->reg_wr.wr : NULL);
> -	if (unlikely(err))
> +	if (unlikely(err)) {
> +		driver_error = true;
>   		goto err_unmap;

Why not just call set the status and call nvme_rdma_complete_rq and
return here?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/28 16:39, Sagi Grimberg wrote:
> 
>> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>       err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>               req->mr ? &req->reg_wr.wr : NULL);
>> -    if (unlikely(err))
>> +    if (unlikely(err)) {
>> +        driver_error = true;
>>           goto err_unmap;
> 
> Why not just call set the status and call nvme_rdma_complete_rq and
> return here?
If the err is ENOMEM or EAGAIN, I am not sure the err must be a
path-related error for all HBA drivers. So reused the error check code.
I think it would be more reasonable to assume any errors returned by HBA
driver as path-related errors.
If you think so, I will modify it in next patch version.

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/28 16:39, Sagi Grimberg wrote:
> 
>> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>       err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>               req->mr ? &req->reg_wr.wr : NULL);
>> -    if (unlikely(err))
>> +    if (unlikely(err)) {
>> +        driver_error = true;
>>           goto err_unmap;
> 
> Why not just call set the status and call nvme_rdma_complete_rq and
> return here?
If the err is ENOMEM or EAGAIN, I am not sure the err must be a
path-related error for all HBA drivers. So reused the error check code.
I think it would be more reasonable to assume any errors returned by HBA
driver as path-related errors.
If you think so, I will modify it in next patch version.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

>>> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct 
>>> blk_mq_hw_ctx *hctx,
>>>       err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>               req->mr ? &req->reg_wr.wr : NULL);
>>> -    if (unlikely(err))
>>> +    if (unlikely(err)) {
>>> +        driver_error = true;
>>>           goto err_unmap;
>>
>> Why not just call set the status and call nvme_rdma_complete_rq and
>> return here?
> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
> path-related error for all HBA drivers. So reused the error check code.
> I think it would be more reasonable to assume any errors returned by HBA
> driver as path-related errors.
> If you think so, I will modify it in next patch version.

Meant to do that only for -EIO. We should absolutely not do any of this
for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
return due to a bug or anything like that.

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

>>> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct 
>>> blk_mq_hw_ctx *hctx,
>>>       err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>               req->mr ? &req->reg_wr.wr : NULL);
>>> -    if (unlikely(err))
>>> +    if (unlikely(err)) {
>>> +        driver_error = true;
>>>           goto err_unmap;
>>
>> Why not just call set the status and call nvme_rdma_complete_rq and
>> return here?
> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
> path-related error for all HBA drivers. So reused the error check code.
> I think it would be more reasonable to assume any errors returned by HBA
> driver as path-related errors.
> If you think so, I will modify it in next patch version.

Meant to do that only for -EIO. We should absolutely not do any of this
for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
return due to a bug or anything like that.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 9:35, Sagi Grimberg wrote:
> 
>>>> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>       err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>>               req->mr ? &req->reg_wr.wr : NULL);
>>>> -    if (unlikely(err))
>>>> +    if (unlikely(err)) {
>>>> +        driver_error = true;
>>>>           goto err_unmap;
>>>
>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>> return here?
>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>> path-related error for all HBA drivers. So reused the error check code.
>> I think it would be more reasonable to assume any errors returned by HBA
>> driver as path-related errors.
>> If you think so, I will modify it in next patch version.
> 
> Meant to do that only for -EIO. We should absolutely not do any of this
> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
> return due to a bug or anything like that.
ok, please review again, thank you.
---
  drivers/nvme/host/rdma.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index b7ce4f221d99..66b697461bd9 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,

         err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
                         req->mr ? &req->reg_wr.wr : NULL);
-       if (unlikely(err))
+       if (unlikely(err)) {
+               if (err == -EIO) {
+                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
+                       err = 0;
+               }
                 goto err_unmap;
+       }

         return BLK_STS_OK;

@@ -2094,7 +2099,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
  err:
         if (err == -ENOMEM || err == -EAGAIN)
                 ret = BLK_STS_RESOURCE;
-       else
+       else if (err)
                 ret = BLK_STS_IOERR;
         nvme_cleanup_cmd(rq);
  unmap_qe:
-- 
> .

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 9:35, Sagi Grimberg wrote:
> 
>>>> @@ -2084,8 +2085,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>       err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>>               req->mr ? &req->reg_wr.wr : NULL);
>>>> -    if (unlikely(err))
>>>> +    if (unlikely(err)) {
>>>> +        driver_error = true;
>>>>           goto err_unmap;
>>>
>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>> return here?
>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>> path-related error for all HBA drivers. So reused the error check code.
>> I think it would be more reasonable to assume any errors returned by HBA
>> driver as path-related errors.
>> If you think so, I will modify it in next patch version.
> 
> Meant to do that only for -EIO. We should absolutely not do any of this
> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
> return due to a bug or anything like that.
ok, please review again, thank you.
---
  drivers/nvme/host/rdma.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index b7ce4f221d99..66b697461bd9 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,

         err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
                         req->mr ? &req->reg_wr.wr : NULL);
-       if (unlikely(err))
+       if (unlikely(err)) {
+               if (err == -EIO) {
+                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
+                       err = 0;
+               }
                 goto err_unmap;
+       }

         return BLK_STS_OK;

@@ -2094,7 +2099,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
  err:
         if (err == -ENOMEM || err == -EAGAIN)
                 ret = BLK_STS_RESOURCE;
-       else
+       else if (err)
                 ret = BLK_STS_IOERR;
         nvme_cleanup_cmd(rq);
  unmap_qe:
-- 
> .

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

>>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>>> return here?
>>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>>> path-related error for all HBA drivers. So reused the error check code.
>>> I think it would be more reasonable to assume any errors returned by HBA
>>> driver as path-related errors.
>>> If you think so, I will modify it in next patch version.
>>
>> Meant to do that only for -EIO. We should absolutely not do any of this
>> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
>> return due to a bug or anything like that.
> ok, please review again, thank you.
> ---
>   drivers/nvme/host/rdma.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index b7ce4f221d99..66b697461bd9 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct 
> blk_mq_hw_ctx *hctx,
> 
>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>                          req->mr ? &req->reg_wr.wr : NULL);
> -       if (unlikely(err))
> +       if (unlikely(err)) {
> +               if (err == -EIO) {
> +                       nvme_complete_failed_rq(rq, 
> NVME_SC_HOST_PATH_ERROR);

I was thinking about:
--
         err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
                         req->mr ? &req->reg_wr.wr : NULL);
         if (unlikely(err)) {
                 if (err == -EIO) {
                         /*
                          * Fail the reuqest so upper layer can failover I/O
                          * if another path is available
                          */
                         req->status = NVME_SC_HOST_PATH_ERROR;
                         nvme_rdma_complete_rq(rq);
                         return BLK_STS_OK;

                 }
                 goto err_unmap;
         }
--

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

>>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>>> return here?
>>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>>> path-related error for all HBA drivers. So reused the error check code.
>>> I think it would be more reasonable to assume any errors returned by HBA
>>> driver as path-related errors.
>>> If you think so, I will modify it in next patch version.
>>
>> Meant to do that only for -EIO. We should absolutely not do any of this
>> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
>> return due to a bug or anything like that.
> ok, please review again, thank you.
> ---
>   drivers/nvme/host/rdma.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index b7ce4f221d99..66b697461bd9 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct 
> blk_mq_hw_ctx *hctx,
> 
>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>                          req->mr ? &req->reg_wr.wr : NULL);
> -       if (unlikely(err))
> +       if (unlikely(err)) {
> +               if (err == -EIO) {
> +                       nvme_complete_failed_rq(rq, 
> NVME_SC_HOST_PATH_ERROR);

I was thinking about:
--
         err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
                         req->mr ? &req->reg_wr.wr : NULL);
         if (unlikely(err)) {
                 if (err == -EIO) {
                         /*
                          * Fail the reuqest so upper layer can failover I/O
                          * if another path is available
                          */
                         req->status = NVME_SC_HOST_PATH_ERROR;
                         nvme_rdma_complete_rq(rq);
                         return BLK_STS_OK;

                 }
                 goto err_unmap;
         }
--

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 11:24, Sagi Grimberg wrote:
> 
>>>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>>>> return here?
>>>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>>>> path-related error for all HBA drivers. So reused the error check code.
>>>> I think it would be more reasonable to assume any errors returned by HBA
>>>> driver as path-related errors.
>>>> If you think so, I will modify it in next patch version.
>>>
>>> Meant to do that only for -EIO. We should absolutely not do any of this
>>> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
>>> return due to a bug or anything like that.
>> ok, please review again, thank you.
>> ---
>>   drivers/nvme/host/rdma.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index b7ce4f221d99..66b697461bd9 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>
>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>                          req->mr ? &req->reg_wr.wr : NULL);
>> -       if (unlikely(err))
>> +       if (unlikely(err)) {
>> +               if (err == -EIO) {
>> +                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
> 
> I was thinking about:
> -- 
>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>                          req->mr ? &req->reg_wr.wr : NULL);
>          if (unlikely(err)) {
>                  if (err == -EIO) {
>                          /*
>                           * Fail the reuqest so upper layer can failover I/O
>                           * if another path is available
>                           */
>                          req->status = NVME_SC_HOST_PATH_ERROR;
>                          nvme_rdma_complete_rq(rq);
>                          return BLK_STS_OK;Need to do clean. so can not directly return.
> 
>                  }
>                  goto err_unmap;
>          }
> -- 
> .

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 11:24, Sagi Grimberg wrote:
> 
>>>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>>>> return here?
>>>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>>>> path-related error for all HBA drivers. So reused the error check code.
>>>> I think it would be more reasonable to assume any errors returned by HBA
>>>> driver as path-related errors.
>>>> If you think so, I will modify it in next patch version.
>>>
>>> Meant to do that only for -EIO. We should absolutely not do any of this
>>> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
>>> return due to a bug or anything like that.
>> ok, please review again, thank you.
>> ---
>>   drivers/nvme/host/rdma.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index b7ce4f221d99..66b697461bd9 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>
>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>                          req->mr ? &req->reg_wr.wr : NULL);
>> -       if (unlikely(err))
>> +       if (unlikely(err)) {
>> +               if (err == -EIO) {
>> +                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
> 
> I was thinking about:
> -- 
>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>                          req->mr ? &req->reg_wr.wr : NULL);
>          if (unlikely(err)) {
>                  if (err == -EIO) {
>                          /*
>                           * Fail the reuqest so upper layer can failover I/O
>                           * if another path is available
>                           */
>                          req->status = NVME_SC_HOST_PATH_ERROR;
>                          nvme_rdma_complete_rq(rq);
>                          return BLK_STS_OK;Need to do clean. so can not directly return.
> 
>                  }
>                  goto err_unmap;
>          }
> -- 
> .

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>> index b7ce4f221d99..66b697461bd9 100644
>>> --- a/drivers/nvme/host/rdma.c
>>> +++ b/drivers/nvme/host/rdma.c
>>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct 
>>> blk_mq_hw_ctx *hctx,
>>>
>>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>                          req->mr ? &req->reg_wr.wr : NULL);
>>> -       if (unlikely(err))
>>> +       if (unlikely(err)) {
>>> +               if (err == -EIO) {
>>> +                       nvme_complete_failed_rq(rq, 
>>> NVME_SC_HOST_PATH_ERROR);
>>
>> I was thinking about:
>> -- 
>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>                          req->mr ? &req->reg_wr.wr : NULL);
>>          if (unlikely(err)) {
>>                  if (err == -EIO) {
>>                          /*
>>                           * Fail the reuqest so upper layer can 
>> failover I/O
>>                           * if another path is available
>>                           */
>>                          req->status = NVME_SC_HOST_PATH_ERROR;
>>                          nvme_rdma_complete_rq(rq);
>>                          return BLK_STS_OK;Need to do clean. so can 
>> not directly return.

The completion path cleans up though

>>
>>                  }
>>                  goto err_unmap;
>>          }
>> -- 
>> .
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Sagi Grimberg]

>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>> index b7ce4f221d99..66b697461bd9 100644
>>> --- a/drivers/nvme/host/rdma.c
>>> +++ b/drivers/nvme/host/rdma.c
>>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct 
>>> blk_mq_hw_ctx *hctx,
>>>
>>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>                          req->mr ? &req->reg_wr.wr : NULL);
>>> -       if (unlikely(err))
>>> +       if (unlikely(err)) {
>>> +               if (err == -EIO) {
>>> +                       nvme_complete_failed_rq(rq, 
>>> NVME_SC_HOST_PATH_ERROR);
>>
>> I was thinking about:
>> -- 
>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>                          req->mr ? &req->reg_wr.wr : NULL);
>>          if (unlikely(err)) {
>>                  if (err == -EIO) {
>>                          /*
>>                           * Fail the reuqest so upper layer can 
>> failover I/O
>>                           * if another path is available
>>                           */
>>                          req->status = NVME_SC_HOST_PATH_ERROR;
>>                          nvme_rdma_complete_rq(rq);
>>                          return BLK_STS_OK;Need to do clean. so can 
>> not directly return.

The completion path cleans up though

>>
>>                  }
>>                  goto err_unmap;
>>          }
>> -- 
>> .
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 11:37, Sagi Grimberg wrote:
> 
>>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>>> index b7ce4f221d99..66b697461bd9 100644
>>>> --- a/drivers/nvme/host/rdma.c
>>>> +++ b/drivers/nvme/host/rdma.c
>>>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>
>>>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>>                          req->mr ? &req->reg_wr.wr : NULL);
>>>> -       if (unlikely(err))
>>>> +       if (unlikely(err)) {
>>>> +               if (err == -EIO) {
>>>> +                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
>>>
>>> I was thinking about:
>>> -- 
>>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>                          req->mr ? &req->reg_wr.wr : NULL);
>>>          if (unlikely(err)) {
>>>                  if (err == -EIO) {
>>>                          /*
>>>                           * Fail the reuqest so upper layer can failover I/O
>>>                           * if another path is available
>>>                           */
>>>                          req->status = NVME_SC_HOST_PATH_ERROR;
>>>                          nvme_rdma_complete_rq(rq);
>>>                          return BLK_STS_OK;Need to do clean. so can not directly return.
> 
> The completion path cleans up though
ok, i see.
But we need to use the new helper nvme_complete_failed_rq to avoid double request completion.
Or we can do like this:
req->status = NVME_SC_HOST_PATH_ERROR;
blk_mq_set_request_complete(req);
nvme_rdma_complete_rq(rq);
> 
>>>
>>>                  }
>>>                  goto err_unmap;
>>>          }
>>> -- 
>>> .
>>
>> _______________________________________________
>> Linux-nvme mailing list
>> Linux-nvme@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-nvme
> .

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 11:37, Sagi Grimberg wrote:
> 
>>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>>> index b7ce4f221d99..66b697461bd9 100644
>>>> --- a/drivers/nvme/host/rdma.c
>>>> +++ b/drivers/nvme/host/rdma.c
>>>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>
>>>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>>                          req->mr ? &req->reg_wr.wr : NULL);
>>>> -       if (unlikely(err))
>>>> +       if (unlikely(err)) {
>>>> +               if (err == -EIO) {
>>>> +                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
>>>
>>> I was thinking about:
>>> -- 
>>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>>                          req->mr ? &req->reg_wr.wr : NULL);
>>>          if (unlikely(err)) {
>>>                  if (err == -EIO) {
>>>                          /*
>>>                           * Fail the reuqest so upper layer can failover I/O
>>>                           * if another path is available
>>>                           */
>>>                          req->status = NVME_SC_HOST_PATH_ERROR;
>>>                          nvme_rdma_complete_rq(rq);
>>>                          return BLK_STS_OK;Need to do clean. so can not directly return.
> 
> The completion path cleans up though
ok, i see.
But we need to use the new helper nvme_complete_failed_rq to avoid double request completion.
Or we can do like this:
req->status = NVME_SC_HOST_PATH_ERROR;
blk_mq_set_request_complete(req);
nvme_rdma_complete_rq(rq);
> 
>>>
>>>                  }
>>>                  goto err_unmap;
>>>          }
>>> -- 
>>> .
>>
>> _______________________________________________
>> Linux-nvme mailing list
>> Linux-nvme@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-nvme
> .

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 11:24, Sagi Grimberg wrote:
> 
>>>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>>>> return here?
>>>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>>>> path-related error for all HBA drivers. So reused the error check code.
>>>> I think it would be more reasonable to assume any errors returned by HBA
>>>> driver as path-related errors.
>>>> If you think so, I will modify it in next patch version.
>>>
>>> Meant to do that only for -EIO. We should absolutely not do any of this
>>> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
>>> return due to a bug or anything like that.
>> ok, please review again, thank you.
>> ---
>>   drivers/nvme/host/rdma.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index b7ce4f221d99..66b697461bd9 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>
>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>                          req->mr ? &req->reg_wr.wr : NULL);
>> -       if (unlikely(err))
>> +       if (unlikely(err)) {
>> +               if (err == -EIO) {
>> +                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
> 
> I was thinking about:
> -- 
>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>                          req->mr ? &req->reg_wr.wr : NULL);
>          if (unlikely(err)) {
>                  if (err == -EIO) {
>                          /*
>                           * Fail the reuqest so upper layer can failover I/O
>                           * if another path is available
>                           */
>                          req->status = NVME_SC_HOST_PATH_ERROR;
>                          nvme_rdma_complete_rq(rq);
>                          return BLK_STS_OK;
Need to do clean. so can not directly return.
> 
>                  }
>                  goto err_unmap;
>          }
> -- 
> .

Re: [PATCH v4 4/5] nvme-rdma: avoid IO error for nvme native multipath

[Chao Leng]

On 2021/1/29 11:24, Sagi Grimberg wrote:
> 
>>>>> Why not just call set the status and call nvme_rdma_complete_rq and
>>>>> return here?
>>>> If the err is ENOMEM or EAGAIN, I am not sure the err must be a
>>>> path-related error for all HBA drivers. So reused the error check code.
>>>> I think it would be more reasonable to assume any errors returned by HBA
>>>> driver as path-related errors.
>>>> If you think so, I will modify it in next patch version.
>>>
>>> Meant to do that only for -EIO. We should absolutely not do any of this
>>> for stuff like EINVAL, EOPNOTSUPP, EPERM or any strange error that may
>>> return due to a bug or anything like that.
>> ok, please review again, thank you.
>> ---
>>   drivers/nvme/host/rdma.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index b7ce4f221d99..66b697461bd9 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -2084,8 +2084,13 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>>
>>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>>                          req->mr ? &req->reg_wr.wr : NULL);
>> -       if (unlikely(err))
>> +       if (unlikely(err)) {
>> +               if (err == -EIO) {
>> +                       nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
> 
> I was thinking about:
> -- 
>          err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
>                          req->mr ? &req->reg_wr.wr : NULL);
>          if (unlikely(err)) {
>                  if (err == -EIO) {
>                          /*
>                           * Fail the reuqest so upper layer can failover I/O
>                           * if another path is available
>                           */
>                          req->status = NVME_SC_HOST_PATH_ERROR;
>                          nvme_rdma_complete_rq(rq);
>                          return BLK_STS_OK;
Need to do clean. so can not directly return.
> 
>                  }
>                  goto err_unmap;
>          }
> -- 
> .

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH v4 5/5] nvme-fc: avoid IO error for nvme native multipath

[Chao Leng]

Work with nvme native multipath, if a path related error occurs when
queue_rq call HBA drive to send request, queue_rq will return
BLK_STS_IOERR to blk-mq. The request is completed with BLK_STS_IOERR
instead of fail over to retry.
queue_rq need call nvme_complete_rq to complete the request with
NVME_SC_HOST_PATH_ERROR, the request will fail over to retry if needed.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/fc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 5f36cfa8136c..400a5638d68a 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2791,7 +2791,12 @@ nvme_fc_queue_rq(struct blk_mq_hw_ctx *hctx,
 	}
 
 
-	return nvme_fc_start_fcp_op(ctrl, queue, op, data_len, io_dir);
+	ret = nvme_fc_start_fcp_op(ctrl, queue, op, data_len, io_dir);
+	if (ret == BLK_STS_IOERR) {
+		nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
+		ret = BLK_STS_OK;
+	}
+	return ret;
 }
 
 static void
-- 
2.16.4

[PATCH v4 5/5] nvme-fc: avoid IO error for nvme native multipath

[Chao Leng]

Work with nvme native multipath, if a path related error occurs when
queue_rq call HBA drive to send request, queue_rq will return
BLK_STS_IOERR to blk-mq. The request is completed with BLK_STS_IOERR
instead of fail over to retry.
queue_rq need call nvme_complete_rq to complete the request with
NVME_SC_HOST_PATH_ERROR, the request will fail over to retry if needed.

Signed-off-by: Chao Leng <lengchao@huawei.com>
---
 drivers/nvme/host/fc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 5f36cfa8136c..400a5638d68a 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2791,7 +2791,12 @@ nvme_fc_queue_rq(struct blk_mq_hw_ctx *hctx,
 	}
 
 
-	return nvme_fc_start_fcp_op(ctrl, queue, op, data_len, io_dir);
+	ret = nvme_fc_start_fcp_op(ctrl, queue, op, data_len, io_dir);
+	if (ret == BLK_STS_IOERR) {
+		nvme_complete_failed_rq(rq, NVME_SC_HOST_PATH_ERROR);
+		ret = BLK_STS_OK;
+	}
+	return ret;
 }
 
 static void
-- 
2.16.4


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme