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

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 4/6/22 15:06, Alan Adamson wrote:
> 
> 
>> On Apr 6, 2022, at 10:21 AM, Keith Busch <kbusch@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;