block and nvme passthrough error handling
Subsystems (4)
Keith Busch
maintainerv3 cover block and nvme passthrough error handling
This series gets blk_execute_rq() to process polled requests and return queueing errors so the caller may know if their request wasn't dispatched.
Changes since v2:
Dropped patch 5 (ioctl polling flag) since there is currently no real use case.
Move blk_execute_rq's poll handling to a small helper function (patch 1)
Return blk_status_t instead of errno (patch 3)
Removed "extern" header declaration, and named the parameters (patch 3)
Initialize nvme status to 0 (patch 4)
Add helper function for blk_status_t to nvme passthrough return code (patch 4)
Keith Busch (4):
block: support polling through blk_execute_rq
nvme: use blk_execute_rq() for passthrough commands
block: return errors from blk_execute_rq()
nvme: use return value from blk_execute_rq()
block/blk-exec.c | 25 ++++++++++++-- drivers/nvme/host/core.c | 63 ++++++++++++++-------------------- drivers/nvme/host/fabrics.c | 13 ++++--- drivers/nvme/host/fabrics.h | 2 +- drivers/nvme/host/fc.c | 2 +- drivers/nvme/host/ioctl.c | 6 +--- drivers/nvme/host/nvme.h | 4 +-- drivers/nvme/host/rdma.c | 3 +- drivers/nvme/host/tcp.c | 2 +- drivers/nvme/target/loop.c | 2 +- drivers/nvme/target/passthru.c | 8 ++--- include/linux/blkdev.h | 4 ++- 12 files changed, 69 insertions(+), 65 deletions(-)
Signature · 1 line
2.25.4
Keith Busch
maintainerPATCH v3 1/4 block: support polling through blk_execute_rq
Poll for completions if the request's hctx is a polling type.
---v1->v2:
Moved blk_execute_rq's poll handling into a small helper function
block/blk-exec.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
block/blk-exec.c +17 -1
@@ -63,6 +63,19 @@ void blk_execute_rq_nowait(struct gendisk *bd_disk, struct request *rq, }EXPORT_SYMBOL_GPL (blk_execute_rq_nowait); +static bool blk_rq_is_poll (struct request *rq) +{ +return rq->mq_hctx && rq->mq_hctx->type ==HCTX_TYPE_POLL ; +} + +static void blk_rq_poll_completion (struct request *rq,struct completion *wait) +{ +do { +blk_poll (rq->q,request_to_qc_t (rq->mq_hctx, rq), true); +cond_resched (); + }while (!completion_done (wait)); +} +/** * blk_execute_rq - insert a request into queue for execution * @bd_disk: matching gendisk
@@ -83,7 +96,10 @@ void blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head)/* Prevent hang_check timer from firing at us during very long I/O */ hang_check = sysctl_hung_task_timeout_secs; -if (hang_check) + +if (blk_rq_is_poll (rq)) +blk_rq_poll_completion (rq, &wait); +else if (hang_check)while (!wait_for_completion_io_timeout (&wait, hang_check * (HZ /2 )));else wait_for_completion_io (&wait);
Signature · 1 line
2.25.4
Re: [PATCHv3 1/4] block: support polling through blk_execute_rq
Quoted 3 lines · Keith Busch
Poll for completions if the request's hctx is a polling type.
Signed-off-by Keith Busch <[email protected]>
Looks good,
Re: [PATCHv3 1/4] block: support polling through blk_execute_rq
In reply to Keith BuschQuoted 7 lines · Keith Busch
Poll for completions if the request's hctx is a polling type.
Signed-off-by Keith Busch <[email protected]>---
v1->v2:Moved blk_execute_rq's poll handling into a small helper function
Signature · 1 line
Ming
Keith Busch
maintainerPATCH v3 2/4 nvme: use blk_execute_rq() for passthrough commands
In reply to Keith BuschThe generic blk_execute_rq() knows how to handle polled completions. Use that instead of implementing an nvme specific handler.
---
No changes since v2
drivers/nvme/host/core.c | 38 +++++-------------------------------- drivers/nvme/host/fabrics.c | 13 ++++++------- drivers/nvme/host/fabrics.h | 2 +- drivers/nvme/host/fc.c | 2 +- drivers/nvme/host/nvme.h | 2 +- drivers/nvme/host/rdma.c | 3 +-- drivers/nvme/host/tcp.c | 2 +- drivers/nvme/target/loop.c | 2 +- 8 files changed, 17 insertions(+), 47 deletions(-)
drivers/nvme/host/core.c +5 -33
@@ -1012,31 +1012,6 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req) }EXPORT_SYMBOL_GPL (nvme_setup_cmd); -static void nvme_end_sync_rq (struct request *rq,blk_status_t error) -{ -struct completion *waiting = rq->end_io_data; - - rq->end_io_data =NULL ; -complete (waiting); -} - -static void nvme_execute_rq_polled (struct request_queue *q, -struct gendisk *bd_disk,struct request *rq,int at_head) -{ -DECLARE_COMPLETION_ONSTACK (wait); - -WARN_ON_ONCE (!test_bit (QUEUE_FLAG_POLL , &q->queue_flags)); - - rq->cmd_flags |=REQ_HIPRI ; - rq->end_io_data = &wait; -blk_execute_rq_nowait (bd_disk, rq, at_head, nvme_end_sync_rq); - -while (!completion_done (&wait)) { -blk_poll (q,request_to_qc_t (rq->mq_hctx, rq), true); -cond_resched (); - } -} -/* * Returns 0 on success. If the result is negative, it's a Linux error code; * if the result is positive, it's an NVM Express status code
@@ -1044,7 +1019,7 @@ static void nvme_execute_rq_polled(struct request_queue *q,int __nvme_submit_sync_cmd (struct request_queue *q,struct nvme_command *cmd,union nvme_result *result,void *buffer,unsigned bufflen,unsigned timeout,int qid,int at_head, -blk_mq_req_flags_t flags,bool poll) +blk_mq_req_flags_t flags) {struct request *req;int ret;
@@ -1065,10 +1040,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, goto out; } -if (poll) -nvme_execute_rq_polled (req->q,NULL , req, at_head); -else -blk_execute_rq (NULL , req, at_head); +blk_execute_rq (NULL , req, at_head);if (result) *result =nvme_req (req)->result;if (nvme_req (req)->flags &NVME_REQ_CANCELLED )
@@ -1085,7 +1057,7 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, void *buffer, unsigned bufflen) {return __nvme_submit_sync_cmd (q, cmd,NULL , buffer, bufflen,0 , -NVME_QID_ANY ,0 ,0 , false); +NVME_QID_ANY ,0 ,0 ); }EXPORT_SYMBOL_GPL (nvme_submit_sync_cmd);
@@ -1449,7 +1421,7 @@ static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid, c.features.dword11 =cpu_to_le32 (dword11); ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &res, - buffer, buflen,0 ,NVME_QID_ANY ,0 ,0 , false); + buffer, buflen,0 ,NVME_QID_ANY ,0 ,0 );if (ret >=0 && result) *result =le32_to_cpu (res.u32);return ret;
@@ -2048,7 +2020,7 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len, cmd.common.cdw11 =cpu_to_le32 (len);return __nvme_submit_sync_cmd (ctrl->admin_q, &cmd,NULL , buffer, len,0 , - NVME_QID_ANY, 1, 0, false); + NVME_QID_ANY, 1, 0); }EXPORT_SYMBOL_GPL (nvme_sec_submit);#endif /* CONFIG_BLK_SED_OPAL */
drivers/nvme/host/fabrics.c +6 -7
@@ -151,7 +151,7 @@ int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val) 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 , false); +NVME_QID_ANY ,0 ,0 );if (ret >=0 ) *val =le64_to_cpu (res.u64);
@@ -198,7 +198,7 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val) 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 , false); +NVME_QID_ANY ,0 ,0 );if (ret >=0 ) *val =le64_to_cpu (res.u64);
@@ -244,7 +244,7 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val) 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 , false); +NVME_QID_ANY ,0 ,0 );if (unlikely (ret)) dev_err(ctrl->device, "Property Set error: %d, offset %#x\n",
@@ -396,7 +396,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl) ret =__nvme_submit_sync_cmd (ctrl->fabrics_q, &cmd, &res, data,sizeof (*data),0 ,NVME_QID_ANY ,1 , -BLK_MQ_REQ_RESERVED |BLK_MQ_REQ_NOWAIT , false); +BLK_MQ_REQ_RESERVED |BLK_MQ_REQ_NOWAIT );if (ret) {nvmf_log_connect_error (ctrl, ret,le32_to_cpu (res.u32), &cmd, data);
@@ -420,7 +420,6 @@ EXPORT_SYMBOL_GPL(nvmf_connect_admin_queue);* @qid: NVMe I/O queue number for the new I/O connection between * host and target (note qid == 0 is illegal as this is * the Admin queue, per NVMe standard). -* @poll: Whether or not to poll for the completion of the connect cmd. * * This function issues a fabrics-protocol connection * of a NVMe I/O queue (via NVMe Fabrics "Connect" command)
@@ -432,7 +431,7 @@ EXPORT_SYMBOL_GPL(nvmf_connect_admin_queue);* > 0: NVMe error status code * < 0: Linux errno error code */ -int nvmf_connect_io_queue (struct nvme_ctrl *ctrl,u16 qid,bool poll) +int nvmf_connect_io_queue (struct nvme_ctrl *ctrl,u16 qid) {struct nvme_command cmd;struct nvmf_connect_data *data;
@@ -459,7 +458,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid, bool poll) ret =__nvme_submit_sync_cmd (ctrl->connect_q, &cmd, &res, data,sizeof (*data),0 , qid,1 , -BLK_MQ_REQ_RESERVED |BLK_MQ_REQ_NOWAIT , poll); +BLK_MQ_REQ_RESERVED |BLK_MQ_REQ_NOWAIT );if (ret) {nvmf_log_connect_error (ctrl, ret,le32_to_cpu (res.u32), &cmd, data);
drivers/nvme/host/fabrics.h +1 -1
@@ -178,7 +178,7 @@ 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_connect_admin_queue (struct nvme_ctrl *ctrl); -int nvmf_connect_io_queue (struct nvme_ctrl *ctrl,u16 qid,bool poll); +int nvmf_connect_io_queue (struct nvme_ctrl *ctrl,u16 qid);int nvmf_register_transport (struct nvmf_transport_ops *ops);void nvmf_unregister_transport (struct nvmf_transport_ops *ops);void nvmf_free_options (struct nvmf_ctrl_options *opts);
drivers/nvme/host/fc.c +1 -1
@@ -2346,7 +2346,7 @@ nvme_fc_connect_io_queues(struct nvme_fc_ctrl *ctrl, u16 qsize) (qsize / 5));if (ret)break ; - ret =nvmf_connect_io_queue (&ctrl->ctrl, i, false); + ret =nvmf_connect_io_queue (&ctrl->ctrl, i);if (ret)break ;
drivers/nvme/host/nvme.h +1 -1
@@ -658,7 +658,7 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,int __nvme_submit_sync_cmd (struct request_queue *q,struct nvme_command *cmd,union nvme_result *result,void *buffer,unsigned bufflen,unsigned timeout,int qid,int at_head, -blk_mq_req_flags_t flags,bool poll); +blk_mq_req_flags_t flags);int nvme_set_features (struct nvme_ctrl *dev,unsigned int fid,unsigned int dword11,void *buffer,size_t buflen,u32 *result);
drivers/nvme/host/rdma.c +1 -2
@@ -680,11 +680,10 @@ static void nvme_rdma_stop_io_queues(struct nvme_rdma_ctrl *ctrl)static int nvme_rdma_start_queue (struct nvme_rdma_ctrl *ctrl,int idx) {struct nvme_rdma_queue *queue = &ctrl->queues[idx]; -bool poll =nvme_rdma_poll_queue (queue);int ret;if (idx) - ret =nvmf_connect_io_queue (&ctrl->ctrl, idx, poll); + ret =nvmf_connect_io_queue (&ctrl->ctrl, idx);else ret =nvmf_connect_admin_queue (&ctrl->ctrl);
drivers/nvme/host/tcp.c +1 -1
@@ -1559,7 +1559,7 @@ static int nvme_tcp_start_queue(struct nvme_ctrl *nctrl, int idx)int ret;if (idx) - ret =nvmf_connect_io_queue (nctrl, idx, false); + ret =nvmf_connect_io_queue (nctrl, idx);else ret =nvmf_connect_admin_queue (nctrl);
drivers/nvme/target/loop.c +1 -1
@@ -335,7 +335,7 @@ static int nvme_loop_connect_io_queues(struct nvme_loop_ctrl *ctrl)int i, ret;for (i =1 ; i < ctrl->ctrl.queue_count; i++) { - ret =nvmf_connect_io_queue (&ctrl->ctrl, i, false); + ret =nvmf_connect_io_queue (&ctrl->ctrl, i);if (ret)return ret;set_bit (NVME_LOOP_Q_LIVE , &ctrl->queues[i].flags);
Signature · 1 line
2.25.4
Re: [PATCHv3 2/4] nvme: use blk_execute_rq() for passthrough commands
Quoted 2 lines · Keith Busch
The generic
blk_execute_rq()knows how to handle polled completions. Use that instead of implementing an nvme specific handler.
Looks good,
Re: [PATCHv3 2/4] nvme: use blk_execute_rq() for passthrough commands
In reply to Keith BuschQuoted 6 lines · Keith Busch
The generic
blk_execute_rq()knows how to handle polled completions. Use that instead of implementing an nvme specific handler.Signed-off-by Keith Busch <[email protected]>---
No changes since v2
Looks fine, one thing need to pay attention is that this nvme polled request won't be marked as REQ_HIPRI.
Signature · 1 line
Ming
Re: [PATCHv3 2/4] nvme: use blk_execute_rq() for passthrough commands
In reply to Keith BuschQuoted 32 lines · Keith Busch
The generic
blk_execute_rq()knows how to handle polled completions. Use that instead of implementing an nvme specific handler.Signed-off-by Keith Busch <[email protected]>---
No changes since v2drivers/nvme/host/core.c | 38 +++++-------------------------------- drivers/nvme/host/fabrics.c | 13 ++++++------- drivers/nvme/host/fabrics.h | 2 +- drivers/nvme/host/fc.c | 2 +- drivers/nvme/host/nvme.h | 2 +- drivers/nvme/host/rdma.c | 3 +-- drivers/nvme/host/tcp.c | 2 +- drivers/nvme/target/loop.c | 2 +- 8 files changed, 17 insertions(+), 47 deletions(-)drivers/nvme/host/core.c +0
-6@@ -1012,31 +1012,6 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req) }EXPORT_SYMBOL_GPL (nvme_setup_cmd); -static void nvme_end_sync_rq (struct request *rq,blk_status_t error) -{ -struct completion *waiting = rq->end_io_data; - - rq->end_io_data =NULL ; -complete (waiting);
drivers/nvme/host/core.c blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req) · line 1030
Quoted 10 lines
drivers/nvme/host/core.c +0
-10-} - -static void nvme_execute_rq_polled (struct request_queue *q, -struct gendisk *bd_disk,struct request *rq,int at_head) -{ -DECLARE_COMPLETION_ONSTACK (wait); - -WARN_ON_ONCE (!test_bit (QUEUE_FLAG_POLL , &q->queue_flags)); - - rq->cmd_flags |=REQ_HIPRI ;
The new code doesn't retain this flag.
Looks good otherwise.
Signature · 1 line
Kanchan
Keith Busch
maintainerRe: [PATCHv3 2/4] nvme: use blk_execute_rq() for passthrough commands
Quoted 38 lines · Kanchan Joshi
Quoted 37 lines · Keith Busch
The generic
blk_execute_rq()knows how to handle polled completions. Use that instead of implementing an nvme specific handler.Signed-off-by Keith Busch <[email protected]>---
No changes since v2drivers/nvme/host/core.c | 38 +++++-------------------------------- drivers/nvme/host/fabrics.c | 13 ++++++------- drivers/nvme/host/fabrics.h | 2 +- drivers/nvme/host/fc.c | 2 +- drivers/nvme/host/nvme.h | 2 +- drivers/nvme/host/rdma.c | 3 +-- drivers/nvme/host/tcp.c | 2 +- drivers/nvme/target/loop.c | 2 +- 8 files changed, 17 insertions(+), 47 deletions(-)drivers/nvme/host/core.c +0
-11@@ -1012,31 +1012,6 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req) }EXPORT_SYMBOL_GPL (nvme_setup_cmd); -static void nvme_end_sync_rq (struct request *rq,blk_status_t error) -{ -struct completion *waiting = rq->end_io_data; - - rq->end_io_data =NULL ; -complete (waiting); -} - -static void nvme_execute_rq_polled (struct request_queue *q, -struct gendisk *bd_disk,struct request *rq,int at_head) -{
Quoted 8 lines
drivers/nvme/host/core.c blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req) · line 1030
Quoted 5 lines
drivers/nvme/host/core.c +0
-5-DECLARE_COMPLETION_ONSTACK (wait); - -WARN_ON_ONCE (!test_bit (QUEUE_FLAG_POLL , &q->queue_flags)); - - rq->cmd_flags |=REQ_HIPRI ;The new code doesn't retain this flag.
Looks good otherwise.
The flag is only used to select an apporpriate hctx. We've explicitly selected a polling hctx in this path already, so the flag is unnecessary.
Christoph Hellwig
maintainerRe: [PATCHv3 2/4] nvme: use blk_execute_rq() for passthrough commands
Quoted 6 lines · Keith Busch
Quoted 2 lines
The new code doesn't retain this flag.
Looks good otherwise.The flag is only used to select an apporpriate hctx. We've explicitly selected a polling hctx in this path already, so the flag is unnecessary.
I think setting the flag would still be useful, if only to prevent issues with new callers and/or better document what we are doing.
Keith Busch
maintainerPATCH v3 3/4 block: return errors from blk_execute_rq()
In reply to Keith BuschThe synchronous blk_execute_rq() had not provided a way for its callers to know if its request was successful or not. Return the blk_status_t result of the request.
---v2->v3:
Return blk_status_t instead of errno.
Removed "extern" header declaration, and named the parameters.
block/blk-exec.c | 7 +++++-- include/linux/blkdev.h | 4 +++- 2 files changed, 8 insertions(+), 3 deletions(-)
block/blk-exec.c +5 -2
@@ -21,7 +21,7 @@ static void blk_end_sync_rq(struct request *rq, blk_status_t error) {struct completion *waiting = rq->end_io_data; - rq->end_io_data =NULL ; + rq->end_io_data = (void *)(uintptr_t )error;/* * complete last, if this is a stack request the process (and thus
@@ -85,8 +85,9 @@ static void blk_rq_poll_completion(struct request *rq, struct completion *wait)* Description: * Insert a fully prepared request at the back of the I/O scheduler queue * for execution and wait for completion. +* Return: The blk_status_t result provided to blk_mq_end_request(). */ -void blk_execute_rq (struct gendisk *bd_disk,struct request *rq,int at_head) +blk_status_t blk_execute_rq (struct gendisk *bd_disk,struct request *rq,int at_head) {DECLARE_COMPLETION_ONSTACK (wait);unsigned long hang_check;
@@ -103,5 +104,7 @@ void blk_execute_rq(struct gendisk *bd_disk, struct request *rq, int at_head)while (!wait_for_completion_io_timeout (&wait, hang_check * (HZ /2 ))); else wait_for_completion_io(&wait); + +return (blk_status_t )(uintptr_t )rq->end_io_data; }EXPORT_SYMBOL (blk_execute_rq);
include/linux/blkdev.h +3 -1
@@ -936,10 +936,12 @@ extern int blk_rq_map_kern(struct request_queue *, struct request *, void *, unsextern int blk_rq_map_user_iov (struct request_queue *,struct request *,struct rq_map_data *,const struct iov_iter *,gfp_t ); -extern void blk_execute_rq (struct gendisk *,struct request *,int );extern void blk_execute_rq_nowait (struct gendisk *,struct request *,int ,rq_end_io_fn *); +blk_status_t blk_execute_rq (struct gendisk *bd_disk,struct request *rq, +int at_head); +/* Helper to convert REQ_OP_XXX to its string format XXX */ extern const char *blk_op_str (unsigned int op);
Signature · 1 line
2.25.4
Re: [PATCHv3 3/4] block: return errors from blk_execute_rq()
Quoted 5 lines · Keith Busch
The synchronous
blk_execute_rq()had not provided a way for its callers to know if its request was successful or not. Return theblk_status_tresult of the request.Signed-off-by Keith Busch <[email protected]>
Looks good,
Re: [PATCHv3 3/4] block: return errors from blk_execute_rq()
In reply to Keith BuschQuoted 11 lines · Keith Busch
The synchronous
blk_execute_rq()had not provided a way for its callers to know if its request was successful or not. Return theblk_status_tresult of the request.Signed-off-by Keith Busch <[email protected]>---
v2->v3:Return
blk_status_tinstead of errno.Removed "extern" header declaration, and named the parameters.
Signature · 1 line
Ming
Keith Busch
maintainerPATCH v3 4/4 nvme: use return value from blk_execute_rq()
In reply to Keith BuschWe don't have an nvme status to report if the driver's .queue_rq() returns an error without dispatching the requested nvme command. Check the return value from blk_execute_rq() for all passthrough commands so the caller may know their command was not successful.
If the command is from the target passthrough interface and fails to dispatch, synthesize the response back to the host as a internal target error.
---v2->v3:
Initialize nvme status to 0
Add helper function for blk_status_t to nvme passthrough return code
drivers/nvme/host/core.c | 33 +++++++++++++++++++++++++-------- drivers/nvme/host/ioctl.c | 6 +----- drivers/nvme/host/nvme.h | 2 +- drivers/nvme/target/passthru.c | 8 ++++---- 4 files changed, 31 insertions(+), 18 deletions(-)
drivers/nvme/host/core.c +25 -8
@@ -589,6 +589,7 @@ EXPORT_SYMBOL_NS_GPL(nvme_put_ns, NVME_TARGET_PASSTHRU);static inline void nvme_clear_nvme_request (struct request *req) { +nvme_req (req)->status =0 ;nvme_req (req)->retries =0 ;nvme_req (req)->flags =0 ; req->rq_flags |=RQF_DONTPREP ;
@@ -1012,6 +1013,21 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req) }EXPORT_SYMBOL_GPL (nvme_setup_cmd); +/* +* Return values: +* 0: success +* >0: nvme controller's cqe status response +* <0: kernel error in lieu of controller response +*/ +static int nvme_passthrough_status (struct request *rq,blk_status_t status) +{ +if (nvme_req (rq)->flags &NVME_REQ_CANCELLED ) +return -EINTR ; +else if (nvme_req (rq)->status) +return nvme_req (rq)->status; +return blk_status_to_errno (status); +} +/* * Returns 0 on success. If the result is negative, it's a Linux error code; * if the result is positive, it's an NVM Express status code
@@ -1022,6 +1038,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, blk_mq_req_flags_t flags) {struct request *req; +blk_status_t status;int ret;if (qid ==NVME_QID_ANY )
@@ -1040,13 +1057,10 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, goto out; } -blk_execute_rq (NULL , req, at_head); -if (result) + status =blk_execute_rq (NULL , req, at_head); + ret =nvme_passthrough_status (req, status); +if (result && ret >=0 ) *result =nvme_req (req)->result; -if (nvme_req (req)->flags &NVME_REQ_CANCELLED ) - ret = -EINTR ; -else - ret =nvme_req (req)->status; out:blk_mq_free_request (req);return ret;
@@ -1134,18 +1148,21 @@ static void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects) } } -void nvme_execute_passthru_rq (struct request *rq) +int nvme_execute_passthru_rq (struct request *rq) {struct nvme_command *cmd =nvme_req (rq)->cmd;struct nvme_ctrl *ctrl =nvme_req (rq)->ctrl;struct nvme_ns *ns = rq->q->queuedata;struct gendisk *disk = ns ? ns->disk :NULL ; +blk_status_t status;u32 effects; effects =nvme_passthru_start (ctrl, ns, cmd->common.opcode); -blk_execute_rq (disk, rq,0 ); + status =blk_execute_rq (disk, rq,0 );if (effects)/* nothing to be done for zero cmd effects */ nvme_passthru_end (ctrl, effects); + +return nvme_passthrough_status (rq, status); }EXPORT_SYMBOL_NS_GPL (nvme_execute_passthru_rq,NVME_TARGET_PASSTHRU );
drivers/nvme/host/ioctl.c +1 -5
@@ -93,11 +93,7 @@ static int nvme_submit_user_cmd(struct request_queue *q, } } -nvme_execute_passthru_rq (req); -if (nvme_req (req)->flags &NVME_REQ_CANCELLED ) - ret = -EINTR ; -else - ret =nvme_req (req)->status; + ret =nvme_execute_passthru_rq (req);if (result) *result =le64_to_cpu (nvme_req (req)->result.u64);if (meta && !ret && !write) {
drivers/nvme/host/nvme.h +1 -1
@@ -877,7 +877,7 @@ static inline void nvme_hwmon_exit(struct nvme_ctrl *ctrl)u32 nvme_command_effects (struct nvme_ctrl *ctrl,struct nvme_ns *ns,u8 opcode); -void nvme_execute_passthru_rq (struct request *rq); +int nvme_execute_passthru_rq (struct request *rq);struct nvme_ctrl *nvme_ctrl_from_file (struct file *file);struct nvme_ns *nvme_find_get_ns (struct nvme_ctrl *ctrl,unsigned nsid);void nvme_put_ns (struct nvme_ns *ns);
drivers/nvme/target/passthru.c +4 -4
@@ -153,11 +153,10 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w) { struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);struct request *rq = req->p.rq; -u16 status; +int status; -nvme_execute_passthru_rq (rq); + status =nvme_execute_passthru_rq (rq); - status =nvme_req (rq)->status;if (status ==NVME_SC_SUCCESS && req->cmd->common.opcode == nvme_admin_identify) {switch (req->cmd->identify.cns) {
@@ -168,7 +167,8 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)nvmet_passthru_override_id_ns (req);break ; } - } + }else if (status <0 ) + status =NVME_SC_INTERNAL ; req->cqe->result =nvme_req (rq)->result;nvmet_req_complete (req, status);
Signature · 1 line
2.25.4
Christoph Hellwig
maintainerRe: [PATCHv3 4/4] nvme: use return value from blk_execute_rq()
Quoted 6 lines
+/*
+ * Return values:
+ * 0: success
+ * >0: nvme controller's cqe status response
+ * <0: kernel error in lieu of controller response
+ */
For better reading flow I'd reformat this as:
/*
* Return value:
* 0: success
* > 0: nvme controller's cqe status response
* < 0: kernel error in lieu of controller response */
drivers/nvme/host/core.c blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req) · line 1029
Quoted 8 lines
drivers/nvme/host/core.c +8
-0+static int nvme_passthrough_status (struct request *rq,blk_status_t status) +{ +if (nvme_req (rq)->flags &NVME_REQ_CANCELLED ) +return -EINTR ; +else if (nvme_req (rq)->status) +return nvme_req (rq)->status; +return blk_status_to_errno (status); +}
I find this a little odd disconnected from the actual execure call. What about a helper like this instead:
static int nvme_execute_rq(struct gendisk *disk, struct request *rq,
bool at_head)
{
blk_status_t status;
status = blk_execute_rq(disk, req, at_head);
if (nvme_req(rq)->flags & NVME_REQ_CANCELLED)
return -EINTR;
if (nvme_req(rq)->status)
return nvme_req(rq)->status;
return blk_status_to_errno(status);
}Quoted 8 lines
drivers/nvme/target/passthru.c +1
-2-nvme_execute_passthru_rq (rq); + status =nvme_execute_passthru_rq (rq); - status =nvme_req (rq)->status;if (status ==NVME_SC_SUCCESS && req->cmd->common.opcode == nvme_admin_identify) {switch (req->cmd->identify.cns) {
drivers/nvme/target/passthru.c static void nvmet_passthru_execute_cmd_work(struct work_struct *w) · line 171
Quoted 6 lines
drivers/nvme/target/passthru.c +2
-1@@ -168,7 +167,8 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)nvmet_passthru_override_id_ns (req);break ; } - } + }else if (status <0 ) + status =NVME_SC_INTERNAL ;
Don't we need a better translation here?
Keith Busch
maintainerRe: [PATCHv3 4/4] nvme: use return value from blk_execute_rq()
Quoted 9 lines · Christoph Hellwig
drivers/nvme/target/passthru.c static void nvmet_passthru_execute_cmd_work(struct work_struct *w) · line 171
Quoted 7 lines
drivers/nvme/target/passthru.c +2
-1@@ -168,7 +167,8 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)nvmet_passthru_override_id_ns (req);break ; } - } + }else if (status <0 ) + status =NVME_SC_INTERNAL ;Don't we need a better translation here?
Did you have something in mind? I couldn't think of anything more appropriate than the generic internal error. The errno's we get here are -EINTR or -EIO. Both indicate we can't communicate with the back-end device, but these problems are internal to the target from the host's perspective.
Christoph Hellwig
maintainerRe: [PATCHv3 4/4] nvme: use return value from blk_execute_rq()
Quoted 11 lines · Keith Busch
Quoted 9 lines · Christoph Hellwig
drivers/nvme/target/passthru.c static void nvmet_passthru_execute_cmd_work(struct work_struct *w) · line 171
Quoted 7 lines
drivers/nvme/target/passthru.c +2
-1@@ -168,7 +167,8 @@ static void nvmet_passthru_execute_cmd_work(struct work_struct *w)nvmet_passthru_override_id_ns (req);break ; } - } + }else if (status <0 ) + status =NVME_SC_INTERNAL ;Don't we need a better translation here?
Quoted 5 lines
Did you have something in mind? I couldn't think of anything more appropriate than the generic internal error. The errno's we get here are
-EINTRor-EIO. Both indicate we can't communicate with the back-end device, but these problems are internal to the target from the host's perspective.
Ok.