Re: [PATCHv2 4/5] nvme: use return value from blk_execute_rq()
From: Christoph Hellwig <hch@lst.de>
Date: 2021-04-26 14:42:25
Also in:
linux-nvme
On Fri, Apr 23, 2021 at 03:05:57PM -0700, Keith Busch wrote:
quoted hunk ↗ jump to hunk
We don't have an nvme status to report if the driver's .queue_rq() returns an error without dispatching the requested nvme command. Use 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. Signed-off-by: Keith Busch <kbusch@kernel.org> --- drivers/nvme/host/core.c | 16 ++++++++++++---- drivers/nvme/host/ioctl.c | 6 +----- drivers/nvme/host/nvme.h | 2 +- drivers/nvme/target/passthru.c | 8 ++++---- 4 files changed, 18 insertions(+), 14 deletions(-)diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 10bb8406e067..62af5fe7a0ce 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c@@ -972,12 +972,12 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd, goto out; } - blk_execute_rq(NULL, req, at_head); + ret = blk_execute_rq(NULL, req, at_head); if (result) *result = nvme_req(req)->result; if (nvme_req(req)->flags & NVME_REQ_CANCELLED) ret = -EINTR; - else + else if (nvme_req(req)->status) ret = nvme_req(req)->status;
Just cosmetic, and already in the existing code, but I'd prefer if we could keep the ret assignments together, something like: status = blk_execute_rq(NULL, req, at_head); if (nvme_req(req)->flags & NVME_REQ_CANCELLED) ret = -EINTR; else if (nvme_req(req)->status) ret = nvme_req(req)->status; else ret = blk_status_to_errno(status); if (result) *result = nvme_req(req)->result;
+ ret = blk_execute_rq(disk, rq, 0); if (effects) /* nothing to be done for zero cmd effects */ nvme_passthru_end(ctrl, effects); + + if (nvme_req(rq)->flags & NVME_REQ_CANCELLED) + ret = -EINTR; + else if (nvme_req(rq)->status) + ret = nvme_req(rq)->status; +
I think we want a helper for all this return value magic instead of duplicating it.