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

[Alan Adamson]

> On Apr 7, 2022, at 2:00 PM, Chaitanya Kulkarni <chaitanyak at nvidia.com> wrote:
> 
> On 4/7/22 13:35, Alan Adamson wrote:
>> 
>> 
>>> On Apr 7, 2022, at 1:10 PM, Chaitanya Kulkarni <chaitanyak at nvidia.com> wrote:
>>> 
>>> On 4/7/22 04:52, Sagi Grimberg wrote:
>>>> 
>>>>>> 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?
>>>>> 
>>>>> Well, you submitted the logging so we're curious about your use case.
>>>>> SCSI skips logging errors for internally submitted commands, but not for
>>>>> userspace passthrough.  So we could move the RQF_QUIET into
>>>>> __nvme_submit_sync_cmd / nvme_keep_alive_work / nvme_timeout / etc.
>>>>> 
>>>> 
>>>> Makes sense to me
>>> 
>>> This was my initial proposal to fix the call sites with internal
>>> passthru commands [1], so this should work.
>>> 
>>> -ck
>>> 
>>> [1]
>>> http://lists.infradead.org/pipermail/linux-nvme/2022-April/031160.html
>>> 
>>> 
>> 
>> It works (suppresses logging) for driver initiated admin cmds which is good, but it also suppresses logging
>> when running "nvme admin-passthru” which I don’t think we want.
>> 
>> Alan
>> 
> 
> I think nvme admin-passthru and io-passthru uses different
> interface than internal passthru commnds i.e.
> 
> nvme admin-passthru :- NVME_IOCTL_ADMIN
> 
>  nvme_submit_user_cmd()
>   req allocation and submission no internal passthru cmds.
> 
> nvme io-passthru:- NVME_IOCTL_IO_CMD
> 
>  nvme_user_cmd()
>   nvme_submit_user_cmd()
>    req allocation and submission, no internal passthru cmds.
> 
> The internal passthru interface uses it's own request allocation
> and submission, so if we change that it should not affect the
> nvme admin-io passthru interface... unless we are using something
> else for internal passthru which is not going through 
> __nvme_submit_sync_cmd() ultimately.
> 
> In short what I meant is following patch, this should help
> us remove the boot time messages you have reported and
> blktest warnings also, can you please confirm ?
> 
> if not then I'll keep digging..
> 
> nvme (nvme-5.18) # git diff
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f204c6f78b5b..a1ea2f736d42 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -370,7 +370,7 @@ 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 && !(req->rq_flags & RQF_QUIET)))
>                 nvme_log_error(req);
>         nvme_end_req_zoned(req);
>         nvme_trace_bio_complete(req);
> @@ -1086,9 +1086,11 @@ int __nvme_submit_sync_cmd(struct request_queue 
> *q, struct nvme_command *cmd,
>         else
>                 req = blk_mq_alloc_request_hctx(q, nvme_req_op(cmd), flags,
>                                                 qid ? qid - 1 : 0);
> -
>         if (IS_ERR(req))
>                 return PTR_ERR(req);
> +
> +       req->rq_flags |= RQF_QUIET;
> +
>         nvme_init_request(req, cmd);
> 
>         if (timeout)
> 
> 
> if not then I'll keep digging..
> 
> -ck

I just tried the patch on my config, it properly suppresses the bootup message, but it also supresses messages from "nvme admin-passthru”.

Alan