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

[Chaitanya Kulkarni]

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