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

[Chaitanya Kulkarni]

On 4/12/23 02:10, Pankaj Raghav wrote:
> On Sun, Apr 09, 2023 at 02:25:37PM -0700, Chaitanya Kulkarni wrote:
>> -ck
>>
>> Chaitanya Kulkarni (1):
>>    nvme: allow passthru cme error logging
>>
>>   drivers/nvme/host/core.c | 83 +++++++++++++++++++++++++++++++++++++---
>>   drivers/nvme/host/nvme.h |  1 +
>>   2 files changed, 79 insertions(+), 5 deletions(-)
>>
>> v5:
>>
>> - Trim down code in the nvme_log_error_passthrough().
>>    Use following to get the disk name as an arg to
>>     pr_err_ratelimited() :-
>> 	ns ? ns->disk->disk_name : dev_name(nr->ctrl->device),
>>    Use following to get the admin vs I/O opcode string as an arg to
>>    pr_err_ratelimited() :-
>>         	ns ? nvme_get_opcode_str(nr->cmd->common.opcode) :
>>         	     nvme_get_admin_opcode_str(nr->cmd->common.opcode),
>> - Rename nvme_log_error_passthrough() -> nvme_log_err_passthru().
>> - Remove else and return directly in nvme_passthru_err_log_show().
>> - Generate error on invalid values of the passthru_enable variable
>>    in nvme_passthru_log_store().
>> - Rename passthrough -> passthru.
>> - Rename sysfs attr from passthru_admin_err_logging -> passthru_log_err.
> Curious to know why `admin` was removed from the sysfs attr. We enable
> logging for admin commands using this attr but IO errors are always logged.
>
> I tried out the patch and I was surprised to see IO err was logged for
> passthru requests without me enabling it explicitly. Having the sysfs
> attr to be passthru_log_admin_err would have been more clear IMO.

Right now we only use this attr to log admin commands, as decision has
been made to log I/O errors irrespective of the attr value is set.
See nvme_ini_request() admin command timeout else in the patch where
we assign quiet flag.

That is not a surprise it's by design and Alan has explicitly made it
clear to have it this way in his reply, and there are no objections so
far. This keeps ctrl access away from fast path in nvme_init_request().

In future if we decide to change the policy and start I/O logging
based on the sysfs values (which might happen) then we will have to
1. Introduce a new attr (polluting sysfs not a good idea)       OR
2. Rename it to generic name that in the patch (not acceptable) OR
3. Overload existing attr to accommodate I/O command (not acceptable)

So why not use generic to start with ?

I can always send V6 with passthru_admin_err_log name but I'd
avoid using non-generic names unless someone guarantees we will never
ever make passthru I/O commands err logging optional...

-ck