[PATCH v3 2/9] nvme-fabrics: allow to queue requests for live queues

[James Smart]

On 8/20/2020 11:22 PM, Christoph Hellwig wrote:
> On Thu, Aug 20, 2020 at 01:45:50PM -0700, James Smart wrote:
>> I still dislike any random ioctls coming in while we're still initializing
>> the controller.
> Agreed.
>
>>    Looking at the flow - I wouldn't want them to be allowed
>> until after nvme_init_identify() is complete. Especially if the ioctls are
>> doing subsystem or controller dumping or using commands that should be
>> capped by values set by nvme_queue_limits().   But, if we're going to
>> allow nvme_init_identify the admin_q needs to be unquiesced.
>>
>> So I'm still voting for the admin queue exception.
> And I really don't like the admin queue special case.  What is the
> advantage of letting user space passthrough I/O commands in at this
> point in time?

You somewhat lost me... the special case/exception is what prevents the 
commands in at this point in time.  I don't know of an advantage to 
letting them through.

In some respects, this goes back to the ioctl issue from the cli that we 
had 1.5+ yrs ago... where we backed out retry patches.  The desire was 
to have the ioctl rejected if the state wasn't live. Granted, there's 
still the race of it passing this check but controller changing state by 
the time the request comes off the request queue (which is ultimately 
what this exception prevents). The biggest issue I've seen is that in 
between the 1st connect and the read of the discovery log, something 
happened such that the controller is still connected yet has to go 
through a reconnect. The cli saw the error on the read log ioctl, gives 
up, then proceeds with it's exit path that deletes the controller - 
leaving nothing discovered and no event to cause it to retry.  When we 
last discussed this - we were looking for killable ioctls and also 
wanted to add retries to nvme-cli but using a standard method.   I've 
had someone working with me on putting at least the retry part into the 
cli so I'll post them soon.

Regardless - given there's the window between check and request being 
executed by the handler - I don't see it worthwhile to remove the 
special case.

-- james