[PATCH] nvme-core: mark passthru requests RQF_QUIET flag

[Alan Adamson]

I tried the patch with my tests.  It resolves the error logging with nvme/012.  My verbose errors tests (nvme/039) now passes,
but we still see the error when the system boots.


root at localhost blktests]# ./check nvme/012
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  39.180s  ...  41.432s
[root at localhost blktests]#


[   77.946505] run blktests nvme/012 at 2022-04-06 19:22:23
[   78.013092] loop: module loaded
[   78.014344] loop0: detected capacity change from 0 to 2097152
[   78.024875] nvmet: adding nsid 1 to subsystem blktests-subsystem-1
[   78.052874] nvmet: creating nvm controller 1 for subsystem blktests-subsystem-1 for NQN nqn.2014-08.org.nvmexpress:uuid:00000000-0000-0000-0000-000000000000.
[   78.053107] nvme nvme1: creating 1 I/O queues.
[   78.053182] nvme nvme1: new ctrl: "blktests-subsystem-1"
[   79.125929] XFS (nvme1n1): Mounting V5 Filesystem
[   79.128321] XFS (nvme1n1): Ending clean mount
[   79.128480] xfs filesystem being mounted at /mnt/blktests supports timestamps until 2038 (0x7fffffff)
[  119.253246] XFS (nvme1n1): Unmounting Filesystem
[  119.264005] nvme nvme1: Removing ctrl: NQN "blktests-subsystem-1"


Verbose Errors blktests
=======================
[root at localhost blktests]# ./check nvme/039
nvme/039 => nvme0n1 (test error logging)                     [passed]
    runtime  0.060s  ...  0.058s
[root at localhost blktests]#

Boot
====
[    3.244056] nvme0: Identify(0x6), Invalid Field in Command (sct 0x0 / sc 0x2) DNR


Alan



~                 

> On Apr 6, 2022, at 3:16 PM, Chaitanya Kulkarni <chaitanyak at nvidia.com> wrote:
> 
> On 4/6/22 15:06, Alan Adamson wrote:
>> 
>> 
>>> On Apr 6, 2022, at 10:21 AM, Keith Busch <kbusch at kernel.org> wrote:
>>> 
>>> On Wed, Apr 06, 2022 at 05:01:46PM +0000, Chaitanya Kulkarni wrote:
>>>> On 4/6/22 09:52, Keith Busch wrote:
>>>>> On Wed, Apr 06, 2022 at 09:41:09AM -0700, Chaitanya Kulkarni wrote:
>>>>>> @@ -370,7 +370,8 @@ static inline void nvme_end_req(struct request *req)
>>>>>>  {
>>>>>>  	blk_status_t status = nvme_error_status(nvme_req(req)->status);
>>>>>> 
>>>>>> -	if (unlikely(nvme_req(req)->status != NVME_SC_SUCCESS))
>>>>>> +	if (unlikely(nvme_req(req)->status != NVME_SC_SUCCESS &&
>>>>>> +		    !(req->rq_flags & RQF_QUIET)))
>>>>>>  		nvme_log_error(req);
>>>>>>  	nvme_end_req_zoned(req);
>>>>>>  	nvme_trace_bio_complete(req);
>>>>>> @@ -651,6 +652,7 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
>>>>>>  	cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>>>>>> 
>>>>>>  	req->cmd_flags |= REQ_FAILFAST_DRIVER;
>>>>>> +	req->rq_flags |= RQF_QUIET;
>>>>> 
>>>>> This defeats the admin error logging logic since every admin command comes
>>>>> through here. If you're sure we should do this, then I suppose you can remove
>>>>> that unreachable code.
>>>> 
>>>> If you point out the unreachable code that will be great,
>>>> I'll keep looking meanwhile...
>>> 
>>> The second half of nvme_log_error(), plus nvme_get_admin_opcode_str() and the
>>> array it defines are unreachable since all admin commands don't log errors with
>>> this change.
>>> 
>>> You could skip the RQF_QUIET setting and check blk_rq_is_passthrough() instead.
>> 
>> Using RQF_QUIET or blk_rq_is_passrhrough() will mean no nvme admin-passthru command will log an error.
>> I ran into this using the blktests I’m coding up for verbose errors.  Is this the behavior we want?
>> 
>> Alan
>> 
> 
> Sagi/Christoph/Hannes/Keith,
> 
> After debugging the issue following patch [1] makes errors disappear.
> 
> But I'm not sure if this behavior aligns with protocol or not.
> 
> I'll keep digging, meanwhile if anyone has an idea please provide a 
> review on patch in [1] if make sense or not.
> 
> -ck
> 
> [1] mask invalid NVME_ID_CNS_CS_CTRL errors.
> 
> diff --git a/drivers/nvme/target/admin-cmd.c 
> b/drivers/nvme/target/admin-cmd.c
> index 397daaf51f1b..e5eea2f0ac9c 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -718,9 +718,16 @@ static void nvmet_execute_identify(struct nvmet_req 
> *req)
>                         switch (req->cmd->identify.csi) {
>                         case NVME_CSI_ZNS:
>                                 return 
> nvmet_execute_identify_cns_cs_ctrl(req);
> +                       case NVME_CSI_NVM:
> +                               return nvmet_execute_identify_ctrl(req);
>                         default:
>                                 break;
>                         }
> +               } else {
> +                       switch (req->cmd->identify.csi) {
> +                       case NVME_CSI_NVM:
> +                               return nvmet_execute_identify_ctrl(req);
> +                       }
>                 }
>                 break;
>         case NVME_ID_CNS_NS_ACTIVE_LIST:
> diff --git a/drivers/nvme/target/discovery.c 
> b/drivers/nvme/target/discovery.c
> index c2162eef8ce1..34c7ed055674 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -254,7 +254,11 @@ static void nvmet_execute_disc_identify(struct 
> nvmet_req *req)
>         if (!nvmet_check_transfer_len(req, NVME_IDENTIFY_DATA_SIZE))
>                 return;
> 
> -       if (req->cmd->identify.cns != NVME_ID_CNS_CTRL) {
> +       switch (req->cmd->identify.cns) {
> +       case NVME_ID_CNS_CTRL:
> +       case NVME_ID_CNS_CS_CTRL:
> +               break;
> +       default:
>                 req->error_loc = offsetof(struct nvme_identify, cns);
>                 status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
>                 goto out;
>