[PATCH] nvme: quiet user passthrough command errors

[PATCH] nvme: quiet user passthrough command errors

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The driver is spamming the kernel logs for entirely harmless errors from
user space submitting unsupported commmands. Just silence the errors.
The application has direct access to command status, so there's no need
to log these.

Cc: Daniel Wagner <dwagner@suse.de>
Cc: Alan Adamson <alan.adamson@oracle.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 686c55cb5d1a..da874172a31d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1156,6 +1156,7 @@ int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
 	struct nvme_ns *ns = rq->q->queuedata;
 
 	*effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
+	rq->rq_flags |= RQF_QUIET;
 	return nvme_execute_rq(rq, false);
 }
 EXPORT_SYMBOL_NS_GPL(nvme_execute_passthru_rq, NVME_TARGET_PASSTHRU);
-- 
2.30.2

Re: [PATCH] nvme: quiet user passthrough command errors

[Chaitanya Kulkarni]

On 10/26/2022 10:07 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The driver is spamming the kernel logs for entirely harmless errors from
> user space submitting unsupported commmands. Just silence the errors.
> The application has direct access to command status, so there's no need
> to log these.
> 
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---


This should do the trick, however it will be great if reporter
can provide tested-by.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] nvme: quiet user passthrough command errors

[Alan Adamson]

> On Oct 26, 2022, at 11:04 AM, Chaitanya Kulkarni <chaitanyak@nvidia.com> wrote:
> 
> On 10/26/2022 10:07 AM, Keith Busch wrote:
>> From: Keith Busch <kbusch@kernel.org>
>> 
>> The driver is spamming the kernel logs for entirely harmless errors from
>> user space submitting unsupported commmands. Just silence the errors.
>> The application has direct access to command status, so there's no need
>> to log these.
>> 
>> Cc: Daniel Wagner <dwagner@suse.de>
>> Cc: Alan Adamson <alan.adamson@oracle.com>
>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>> ---
> 
> 
> This should do the trick, however it will be great if reporter
> can provide tested-by.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> 
> -ck
> 
> 


I’ll be testing it today.

Alan

Re: [PATCH] nvme: quiet user passthrough command errors

[Keith Busch]

On Wed, Oct 26, 2022 at 06:04:32PM +0000, Chaitanya Kulkarni wrote:
> On 10/26/2022 10:07 AM, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The driver is spamming the kernel logs for entirely harmless errors from
> > user space submitting unsupported commmands. Just silence the errors.
> > The application has direct access to command status, so there's no need
> > to log these.
> > 
> > Cc: Daniel Wagner <dwagner@suse.de>
> > Cc: Alan Adamson <alan.adamson@oracle.com>
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> 
> 
> This should do the trick, however it will be great if reporter
> can provide tested-by.

Pretty easy to test. Just send an invalid command from user space:

  # nvme admin-passthru /dev/nvme0 --opcode=0xff
  nvme0: Admin Cmd(0xff), I/O Error (sct 0x0 / sc 0x1) DNR

With the patch applied, the kernel message is suppressed.

Re: [PATCH] nvme: quiet user passthrough command errors

[Alan Adamson]

> On Oct 26, 2022, at 10:07 AM, Keith Busch <kbusch@meta.com> wrote:
> 
> From: Keith Busch <kbusch@kernel.org>
> 
> The driver is spamming the kernel logs for entirely harmless errors from
> user space submitting unsupported commmands. Just silence the errors.
> The application has direct access to command status, so there's no need
> to log these.
> 
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> drivers/nvme/host/core.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 686c55cb5d1a..da874172a31d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1156,6 +1156,7 @@ int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
> 	struct nvme_ns *ns = rq->q->queuedata;
> 
> 	*effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
> +	rq->rq_flags |= RQF_QUIET;
> 	return nvme_execute_rq(rq, false);
> }
> EXPORT_SYMBOL_NS_GPL(nvme_execute_passthru_rq, NVME_TARGET_PASSTHRU);
> -- 
> 2.30.2
> 

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>

I tested the change with blktests which uses user passthrough:

Before patch:
[   69.041515] nvme0n1: Read(0x2) @ LBA 0, 1 blocks, Unrecovered Read Error (sct 0x2 / sc 0x81) DNR
[   69.041557] critical medium error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x800 phys_seg 1 prio class 2
[   69.055721] nvme0n1: Read(0x2) @ LBA 0, 1 blocks, Unknown (sct 0x3 / sc 0x75) DNR
[   69.055754] I/O error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x800 phys_seg 1 prio class 2
[   69.067335] nvme0n1: Write(0x1) @ LBA 0, 1 blocks, Write Fault (sct 0x2 / sc 0x80) DNR
[   69.067385] critical medium error, dev nvme0n1, sector 0 op 0x1:(WRITE) flags 0x8800 phys_seg 1 prio class 2
[   69.085449] nvme0: Identify(0x6), Access Denied (sct 0x2 / sc 0x86) DNR
[   69.103265] nvme0: Unknown(0x96), Invalid Command Opcode (sct 0x0 / sc 0x1) DNR

After patch:
[  167.375311] nvme0n1: Read(0x2) @ LBA 0, 1 blocks, Unrecovered Read Error (sct 0x2 / sc 0x81) DNR
[  167.375362] critical medium error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x800 phys_seg 1 prio class 2
[  167.388812] nvme0n1: Read(0x2) @ LBA 0, 1 blocks, Unknown (sct 0x3 / sc 0x75) DNR
[  167.388839] I/O error, dev nvme0n1, sector 0 op 0x0:(READ) flags 0x800 phys_seg 1 prio class 2
[  167.399907] nvme0n1: Write(0x1) @ LBA 0, 1 blocks, Write Fault (sct 0x2 / sc 0x80) DNR
[  167.399928] critical medium error, dev nvme0n1, sector 0 op 0x1:(WRITE) flags 0x8800 phys_seg 1 prio class 2


The messages that are due to passthrough are no longer present as expcted.

Tested-by: Alan Adamson <alan.adamson@oracle.com>


blktests will need to updated, specifically blktests/tests/nvme/039.out.

Alan

Re: [PATCH] nvme: quiet user passthrough command errors

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Wed, Oct 26, 2022 at 10:07:15AM -0700, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>The driver is spamming the kernel logs for entirely harmless errors from
>user space submitting unsupported commmands. Just silence the errors.
>The application has direct access to command status, so there's no need
>to log these.
>
>Cc: Daniel Wagner <dwagner@suse.de>
>Cc: Alan Adamson <alan.adamson@oracle.com>
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>index 686c55cb5d1a..da874172a31d 100644
>--- a/drivers/nvme/host/core.c
>+++ b/drivers/nvme/host/core.c
>@@ -1156,6 +1156,7 @@ int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
> 	struct nvme_ns *ns = rq->q->queuedata;
>
> 	*effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
>+	rq->rq_flags |= RQF_QUIET;
> 	return nvme_execute_rq(rq, false);
> }

Can we do this for uring-passthrough path too? Like below patch -

diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 81f5550b670d..a91cefc38506 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -556,6 +556,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
        } else {
                req->end_io = nvme_uring_cmd_end_io;
        }
+       req->rq_flags |= RQF_QUIET;
        blk_execute_rq_nowait(req, false);
        return -EIOCBQUEUED;
 }

Looks good otherwise.
Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] nvme: quiet user passthrough command errors

[Pankaj Raghav]

On Wed, Oct 26, 2022 at 10:07:15AM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The driver is spamming the kernel logs for entirely harmless errors from
> user space submitting unsupported commmands. Just silence the errors.
> The application has direct access to command status, so there's no need
> to log these.
> 
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> 
Shouldn't we also add a Fixes tag here?

Re: [PATCH] nvme: quiet user passthrough command errors

[Jens Axboe]

On 10/26/22 11:41 PM, Kanchan Joshi wrote:
> On Wed, Oct 26, 2022 at 10:07:15AM -0700, Keith Busch wrote:
>> From: Keith Busch <kbusch@kernel.org>
>>
>> The driver is spamming the kernel logs for entirely harmless errors from
>> user space submitting unsupported commmands. Just silence the errors.
>> The application has direct access to command status, so there's no need
>> to log these.
>>
>> Cc: Daniel Wagner <dwagner@suse.de>
>> Cc: Alan Adamson <alan.adamson@oracle.com>
>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>> ---
>> drivers/nvme/host/core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 686c55cb5d1a..da874172a31d 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -1156,6 +1156,7 @@ int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
>>     struct nvme_ns *ns = rq->q->queuedata;
>>
>>     *effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
>> +    rq->rq_flags |= RQF_QUIET;
>>     return nvme_execute_rq(rq, false);
>> }
> 
> Can we do this for uring-passthrough path too? Like below patch -
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 81f5550b670d..a91cefc38506 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -556,6 +556,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>        } else {
>                req->end_io = nvme_uring_cmd_end_io;
>        }
> +       req->rq_flags |= RQF_QUIET;
>        blk_execute_rq_nowait(req, false);
>        return -EIOCBQUEUED;
> }
> 
> Looks good otherwise.
> Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Agree, we should do it for this path too.

-- 
Jens Axboe

Re: [PATCH] nvme: quiet user passthrough command errors

[Keith Busch]

On Thu, Oct 27, 2022 at 07:17:48AM -0600, Jens Axboe wrote:
> On 10/26/22 11:41 PM, Kanchan Joshi wrote:
> > On Wed, Oct 26, 2022 at 10:07:15AM -0700, Keith Busch wrote:
> >> From: Keith Busch <kbusch@kernel.org>
> >>
> >> The driver is spamming the kernel logs for entirely harmless errors from
> >> user space submitting unsupported commmands. Just silence the errors.
> >> The application has direct access to command status, so there's no need
> >> to log these.
> >>
> >> Cc: Daniel Wagner <dwagner@suse.de>
> >> Cc: Alan Adamson <alan.adamson@oracle.com>
> >> Signed-off-by: Keith Busch <kbusch@kernel.org>
> >> ---
> >> drivers/nvme/host/core.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >> index 686c55cb5d1a..da874172a31d 100644
> >> --- a/drivers/nvme/host/core.c
> >> +++ b/drivers/nvme/host/core.c
> >> @@ -1156,6 +1156,7 @@ int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
> >>     struct nvme_ns *ns = rq->q->queuedata;
> >>
> >>     *effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
> >> +    rq->rq_flags |= RQF_QUIET;
> >>     return nvme_execute_rq(rq, false);
> >> }
> > 
> > Can we do this for uring-passthrough path too? Like below patch -
> > 
> > diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> > index 81f5550b670d..a91cefc38506 100644
> > --- a/drivers/nvme/host/ioctl.c
> > +++ b/drivers/nvme/host/ioctl.c
> > @@ -556,6 +556,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> >        } else {
> >                req->end_io = nvme_uring_cmd_end_io;
> >        }
> > +       req->rq_flags |= RQF_QUIET;
> >        blk_execute_rq_nowait(req, false);
> >        return -EIOCBQUEUED;
> > }
> > 
> > Looks good otherwise.
> > Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
> 
> Agree, we should do it for this path too.

Okay, that would make 4 places the driver is setting the QUIET flag,
which is all of the passthrough requests. Instead of doing them
individually, I'll just move the setting to their common helper:
nvme_init_command().

Re: [PATCH] nvme: quiet user passthrough command errors

[Chaitanya Kulkarni]

> 
> On Oct 27, 2022, at 8:02 AM, Keith Busch <kbusch@kernel.org> wrote:
> 
> On Thu, Oct 27, 2022 at 07:17:48AM -0600, Jens Axboe wrote:
>>> On 10/26/22 11:41 PM, Kanchan Joshi wrote:
>>> On Wed, Oct 26, 2022 at 10:07:15AM -0700, Keith Busch wrote:
>>>> From: Keith Busch <kbusch@kernel.org>
>>>> 
>>>> The driver is spamming the kernel logs for entirely harmless errors from
>>>> user space submitting unsupported commmands. Just silence the errors.
>>>> The application has direct access to command status, so there's no need
>>>> to log these.
>>>> 
>>>> Cc: Daniel Wagner <dwagner@suse.de>
>>>> Cc: Alan Adamson <alan.adamson@oracle.com>
>>>> Signed-off-by: Keith Busch <kbusch@kernel.org>
>>>> ---
>>>> drivers/nvme/host/core.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>> 
>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>> index 686c55cb5d1a..da874172a31d 100644
>>>> --- a/drivers/nvme/host/core.c
>>>> +++ b/drivers/nvme/host/core.c
>>>> @@ -1156,6 +1156,7 @@ int nvme_execute_passthru_rq(struct request *rq, u32 *effects)
>>>>     struct nvme_ns *ns = rq->q->queuedata;
>>>> 
>>>>     *effects = nvme_passthru_start(ctrl, ns, cmd->common.opcode);
>>>> +    rq->rq_flags |= RQF_QUIET;
>>>>     return nvme_execute_rq(rq, false);
>>>> }
>>> 
>>> Can we do this for uring-passthrough path too? Like below patch -
>>> 
>>> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>>> index 81f5550b670d..a91cefc38506 100644
>>> --- a/drivers/nvme/host/ioctl.c
>>> +++ b/drivers/nvme/host/ioctl.c
>>> @@ -556,6 +556,7 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>>>        } else {
>>>                req->end_io = nvme_uring_cmd_end_io;
>>>        }
>>> +       req->rq_flags |= RQF_QUIET;
>>>        blk_execute_rq_nowait(req, false);
>>>        return -EIOCBQUEUED;
>>> }
>>> 
>>> Looks good otherwise.
>>> Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>
>> 
>> Agree, we should do it for this path too.
> 
> Okay, that would make 4 places the driver is setting the QUIET flag,
> which is all of the passthrough requests. Instead of doing them
> individually, I'll just move the setting to their common helper:
> nvme_init_command().
> 

Yes please.  

-ck