On Mon, Apr 26, 2021 at 10:15 AM Keith Busch [off-list ref] wrote:

On Mon, Apr 26, 2021 at 10:10:09AM -0700, Yuanyuan Zhong wrote:

quoted

On Fri, Apr 23, 2021 at 3:06 PM Keith Busch [off-list ref] wrote:

quoted

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)

Since nvme_req(req)->status is uninitialized for a command failed to dispatch,
it's valid only if ret from blk_execute_rq() is 0.

That's not quite right. If queue_rq() succeeds, but the SSD returns an
error, blk_execute_rq() returns a non-zero value with a valid nvme_req
status.

Agreed. But after that, freeing the req let nvme_req(req)->status from SSD stay.
If the same req get re-allocated and get dispatching failure,
shouldn't the dispatching
error take precedence over the stale nvme_req(req)->status?