Re: [PATCH] nvme: enable retries for authentication commands

[Jens Axboe <axboe@kernel.dk>]

On 1/25/24 10:09 AM, Hannes Reinecke wrote:
> On 1/25/24 17:50, Jens Axboe wrote:
>> On 1/25/24 9:20 AM, Hannes Reinecke wrote:
>>> On 1/25/24 16:48, Christoph Hellwig wrote:
>>>> On Thu, Jan 25, 2024 at 08:42:10AM -0700, Jens Axboe wrote:
>>>>> On 1/25/24 6:09 AM, hare@kernel.org wrote:
>>>>>> From: Hannes Reinecke <hare@suse.de>
>>>>>>
>>>>>> Authentication commands are normal NVMe commands, and as such
>>>>>> can return a status where NVME_SC_DNR is not set, indicating
>>>>>> that the command should be retried.
>>>>>> Rather than checking NVME_SC_DNR manually for each command completion
>>>>>> this patch adds a flag 'retry' to __nvme_submit_sync_cmd(), causing
>>>>>> the REQ_FAILFAST_DRIVER option to be cleared and the command to
>>>>>> be retried via the normal mechanism.
>>>>>
>>>>> Can we please do this without adding Yet Another parameter to that
>>>>> function?
>>>>
>>>> Heh, I wrote about the same reply a few minutes ago.  I'll try to think
>>>> what we can do instead.
>>>
>>> We could do something like this:
>>
>> That won't really do much, and yeah wouldn't be the way even if
>> complete. How about just not setting the flag in the first place?
>> nvme_init_request() defaults to doing it, so either pass in the init
>> flags to that, or just clear it in there and have the callers decide if
>> they want to set it.
>>
> But that's the thing; the request is never visible to the caller.
> So moving things out of the nvme_init_request() doesn't really help.
> 
> And open-coding __nvme_submit_sync_cmd() also doesn't look very
> appealing.

__nvme_submit_sync_cmd() already takes another bool (at_head) and blk-mq
flags. You could have it take some nvme specific flags instead, and fold
those two parameters into a single flags. Once that is done, adding the
retry flag on top would be trivial.

And as a side benefit, it gets RID of a bool flag rather than add
another random one.

-- 
Jens Axboe