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

[Sagi Grimberg]

> 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.