[PATCH] nvme: fix error-handling for io_uring nvme-passthrough

[PATCH] nvme: fix error-handling for io_uring nvme-passthrough

[Kanchan Joshi]

From: Anuj Gupta <anuj20.g@samsung.com>

Driver may return an error before submitting the command to the device.
Ensure that such error is propagated up.

Fixes: 456cba386e94 ("nvme: wire-up uring-cmd support for io-passthru on char-device.")

Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 drivers/nvme/host/ioctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index d8ff796fd5f2..53987df6ea7c 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -508,8 +508,11 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
 	req->bio = pdu->bio;
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
 		pdu->nvme_status = -EINTR;
-	else
+	else {
 		pdu->nvme_status = nvme_req(req)->status;
+		if (!pdu->nvme_status)
+			pdu->nvme_status = blk_status_to_errno(err);
+	}
 	pdu->u.result = le64_to_cpu(nvme_req(req)->result.u64);
 
 	/*
-- 
2.25.1

Re: [PATCH] nvme: fix error-handling for io_uring nvme-passthrough

[Christoph Hellwig]

On Wed, Oct 18, 2023 at 07:27:18PM +0530, Kanchan Joshi wrote:
> @@ -508,8 +508,11 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
>  	req->bio = pdu->bio;
>  	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
>  		pdu->nvme_status = -EINTR;
> -	else
> +	else {
>  		pdu->nvme_status = nvme_req(req)->status;
> +		if (!pdu->nvme_status)
> +			pdu->nvme_status = blk_status_to_errno(err);
> +	}

Hmm, shoudn't err take precedence over the nvme status?

Re: [PATCH] nvme: fix error-handling for io_uring nvme-passthrough

[Kanchan Joshi]

On Wed, Oct 18, 2023 at 9:20 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Oct 18, 2023 at 07:27:18PM +0530, Kanchan Joshi wrote:
> > @@ -508,8 +508,11 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
> >       req->bio = pdu->bio;
> >       if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
> >               pdu->nvme_status = -EINTR;
> > -     else
> > +     else {
> >               pdu->nvme_status = nvme_req(req)->status;
> > +             if (!pdu->nvme_status)
> > +                     pdu->nvme_status = blk_status_to_errno(err);
> > +     }
>
> Hmm, shoudn't err take precedence over the nvme status?

Since this is for passthrough interface, nvme status should get the
precedence since that is what we want the users to see.
Same happens for sync passthrough too (in nvme_execute_rq).

Re: [PATCH] nvme: fix error-handling for io_uring nvme-passthrough

[Keith Busch]

On Wed, Oct 18, 2023 at 10:20:10PM +0530, Kanchan Joshi wrote:
> On Wed, Oct 18, 2023 at 9:20 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Wed, Oct 18, 2023 at 07:27:18PM +0530, Kanchan Joshi wrote:
> > > @@ -508,8 +508,11 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
> > >       req->bio = pdu->bio;
> > >       if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
> > >               pdu->nvme_status = -EINTR;
> > > -     else
> > > +     else {
> > >               pdu->nvme_status = nvme_req(req)->status;
> > > +             if (!pdu->nvme_status)
> > > +                     pdu->nvme_status = blk_status_to_errno(err);
> > > +     }
> >
> > Hmm, shoudn't err take precedence over the nvme status?
> 
> Since this is for passthrough interface, nvme status should get the
> precedence since that is what we want the users to see.
> Same happens for sync passthrough too (in nvme_execute_rq).

Right, blk_sts_t 'err' will be non-zero on any nvme error status, but we
want to report nvme error status if we have one.

Re: [PATCH] nvme: fix error-handling for io_uring nvme-passthrough

[Niklas Cassel]

Hello Kanchan,

On Wed, Oct 18, 2023 at 07:27:18PM +0530, Kanchan Joshi wrote:
> From: Anuj Gupta <anuj20.g@samsung.com>
> 
> Driver may return an error before submitting the command to the device.
> Ensure that such error is propagated up.
> 
> Fixes: 456cba386e94 ("nvme: wire-up uring-cmd support for io-passthru on char-device.")
>

Why do you have an empty line between the Fixes and the Signed-off-by ?
(I noticed this on some of your other patches as well.)

I don't think there is supposed to be any empty lines between the tags.

> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
> ---
>  drivers/nvme/host/ioctl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index d8ff796fd5f2..53987df6ea7c 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -508,8 +508,11 @@ static enum rq_end_io_ret nvme_uring_cmd_end_io(struct request *req,
>  	req->bio = pdu->bio;
>  	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
>  		pdu->nvme_status = -EINTR;
> -	else
> +	else {
>  		pdu->nvme_status = nvme_req(req)->status;
> +		if (!pdu->nvme_status)
> +			pdu->nvme_status = blk_status_to_errno(err);
> +	}

The kernel coding standard says that you should have braces on the if,
even if it is a single line statement, when you have brances on the else:
https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces


Kind regards,
Niklas