Re: [PATCH v1] nvme: do not log errors for user commands

From: Alan Adamson <alan.adamson@oracle.com>
To: Daniel Wagner <dwagner@suse.de>
Cc: Pankaj Raghav <pankydev8@gmail.com>,
	"open list:NVM EXPRESS DRIVER" <linux-nvme@lists.infradead.org>,
	Keith Busch <kbusch@kernel.org>, Yi Zhang <yi.zhang@redhat.com>,
	Tomas Bzatek <tbzatek@redhat.com>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	Martin Petersen <martin.petersen@oracle.com>,
	"gost.dev@samsung.com" <gost.dev@samsung.com>,
	Pankaj Raghav <p.raghav@samsung.com>
Subject: Re: [PATCH v1] nvme: do not log errors for user commands
Date: Fri, 7 Oct 2022 21:19:08 +0000	[thread overview]
Message-ID: <35A47E59-90E0-4F6C-80EA-A98F59213FA0@oracle.com> (raw)
In-Reply-To: <20221006133053.mmzw7m7wbfsapqbx@carbon.lan>

> On Oct 6, 2022, at 6:30 AM, Daniel Wagner <dwagner@suse.de> wrote:
> 
> On Thu, Oct 06, 2022 at 02:16:35PM +0200, Pankaj Raghav wrote:
>>> +	req->rq_flags |= RQF_QUIET;
>> 
>> With this, we are disabling logging for ADMIN and IO commands via the
>> ioctl. IMO, the error logging for IO commands could be useful for
>> debugging. But I do understand your regression argument here.
> 
> I do get the debugging argument, but this clearly regresses production
> code. Ideally, this code should be opt-in. The only way I see how to make
> this work is by introducing a new API which allows enable this
> feature.

Looks like a new API may need to be created.  I’ll investigate that.  With your change, there
will need to be a blktests change.

nvme/039 => nvme0n1 (test error logging)                     [failed]
    runtime  0.120s  ...  0.118s
    --- tests/nvme/039.out	2022-09-21 17:14:12.760890663 -0400
    +++ /root/blktests/results/nvme0n1/nvme/039.out.bad	2022-10-06 19:11:45.846879996 -0400
    @@ -2,6 +2,4 @@
      Read(0x2) @ LBA 0, 1 blocks, Unrecovered Read Error (sct 0x2 / sc 0x81) DNR 
      Read(0x2) @ LBA 0, 1 blocks, Unknown (sct 0x3 / sc 0x75) DNR 
      Write(0x1) @ LBA 0, 1 blocks, Write Fault (sct 0x2 / sc 0x80) DNR 
    - Identify(0x6), Access Denied (sct 0x2 / sc 0x86) DNR 
    - Unknown(0x96), Invalid Command Opcode (sct 0x0 / sc 0x1) DNR 
     Test complete
[root@localhost blktests]#

Alan

> 
>>> 	ret = nvme_execute_passthru_rq(req, &effects);
>>> 
>>> 	if (result)
>> 
>> We can also send admin commands via the uring_cmd interface:
>> nvme_dev_uring_cmd(). Should we also enable RQF_QUIET flags for them?
> 
> Good point, this path is likely to have the same problem.
> 
>> P.S: I sent a patch today that touches the error logging in NVMe that
>> sets the starting LBA as zero for IO passthrough requests.
>> https://lore.kernel.org/linux-nvme/20221006091053.36611-1-p.raghav@samsung.com/
> 
> Yes, saw your patch, but with this patch it wont do match afterwards :)
> 