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

[James Smart]

On 8/20/2020 9:58 AM, Sagi Grimberg wrote:
>
>>> Right now we are failing requests based on the controller state (which
>>> is checked inline in nvmf_check_ready) however we should definitely
>>> accept requests if the queue is live.
>>>
>>> When entering controller reset, we transition the controller into
>>> NVME_CTRL_RESETTING, and then return BLK_STS_RESOURCE for non-mpath
>>> requests (have blk_noretry_request set).
>>>
>>> This is also the case for NVME_REQ_USER for the wrong reason. There
>>> shouldn't be any reason for us to reject this I/O in a controller 
>>> reset.
>>> We do want to prevent passthru commands on the admin queue because we
>>> need the controller to fully initialize first before we let user 
>>> passthru
>>> admin commands to be issued.
>>>
>>> In a non-mpath setup, this means that the requests will simply be
>>> requeued over and over forever not allowing the q_usage_counter to drop
>>> its final reference, causing controller reset to hang if running
>>> concurrently with heavy I/O.
>>
>> I'm still rather bothered with the admin queue exception.  And given 
>> that
>> the q_usage_counter problem should only really be an issue for file 
>> system
>> requests, as passthrough requests do not automatically get retried why
>> can't we just reject all user command to be symetric and straight 
>> forward?
>> The callers in userspace need to be able to cope with retryable errors
>> anyway.
>
> Looking at the code again, I think we can kill it as well.
>
> The concern is we may issue user generated admin commands before
> the controller is enabled (generating an unforced error just because
> we queued to early). That used to be the case when the admin connect
> used the admin_q which meant we needed to unquiesce before, but
> now that the admin connect uses the fabrics_q, that should no
> longer be an issue.
>
> in nvme-tcp we unquiesce after we enable the ctrl:
> ...
>
> James, can you please have a look if this is still an issue?

I still dislike any random ioctls coming in while we're still 
initializing the controller.  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.

-- james