[PATCH V7 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

Christoph,

On 12/5/23 02:01, Sagi Grimberg wrote:
>
>> Sagi/Christoph,
>>
>> On 9/19/23 09:02, alan.adamson at oracle.com wrote:
>>>
>>> On 9/12/23 4:39 AM, Sagi Grimberg wrote:
>>>>
>>>>>>>        if (req->q->queuedata)
>>>>>>>            req->timeout = NVME_IO_TIMEOUT;
>>>>>>> -    else /* no queuedata implies admin queue */
>>>>>>> +    else { /* no queuedata implies admin queue */
>>>>>>>            req->timeout = NVME_ADMIN_TIMEOUT;
>>>>>>> +        if (!nr->ctrl->passthru_err_log_enabled)
>>>>>>> +            req->rq_flags |= RQF_QUIET;
>>>>>>> +    }
>>>>>>>        /* passthru commands should let the driver set the SGL 
>>>>>>> flags */
>>>>>>>        cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>>>>>>> @@ -687,8 +720,7 @@ void nvme_init_request(struct request *req,
>>>>>>> struct nvme_command *cmd)
>>>>>>>        if (req->mq_hctx->type == HCTX_TYPE_POLL)
>>>>>>>            req->cmd_flags |= REQ_POLLED;
>>>>>>>        nvme_clear_nvme_request(req);
>>>>>>> -    req->rq_flags |= RQF_QUIET;
>>>>>> Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
>>>>>> previously set?  While we're at it, why do we only allow passthrough
>>>>>> error logging for admin commands anyway?
>>>>>
>>>>> While working through this, we decided that for IO commands
>>>>> (passthrough or not) error logging would always be enabled. For
>>>>> passthrough admin commands, error logging could be enabled or 
>>>>> disabled.
>>>>
>>>> Why shouldn't it apply to both?
>>>
>>> Currently, we have /sys/class/nvme/nvme0/passthru_err_log_enabled that
>>> enables/disables logging for admin command only.  Are you saying it
>>> should also enable/disable for IO commands across all namespaces or
>>> should there also be:
>>>
>>> /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
>>>
>>> /sys/class/nvme/nvme0/nvme0n2/passthru_err_log_enabled
>>>
>>> .
>>>
>>> .
>>>
>>> Alan
>>>
>>>
>>>
>>
>> Can we please have resolution on this patch so we can merge it ?
>
> I'm in favor of the latter, IO commands enabled per ns, and admin
> commands in the controller level.

are you okay with above approach ?

-ck