Re: [PATCH v2 1/8] nvme-fabrics: allow to queue requests for live queues

[Sagi Grimberg <sagi@grimberg.me>]

>> 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.
> 
> Which will still happen with the admin queue user passthrough
> commands with this patch, so I don't think it actually solves anything,
> it just reduces the exposure a bit.

The original version of the patch removed that as well, but james
indicated that it's still needed because we have no way to make sure
the admin (re)connect will be the first request when we unquiesce.

So I kept that one around and will fix it later, and yes, this
is niche corner case compared to user I/O.

>> While we are at it, remove the redundant NVME_CTRL_NEW case, which
>> should never see any I/O as it must first transition to
>> NVME_CTRL_CONNECTING.
> 
> That probablyly should be a separate patch.

OK.

>> -		if (nvme_is_fabrics(req->cmd) &&
>> +		if (blk_rq_is_passthrough(rq) && nvme_is_fabrics(req->cmd) &&
> 
> And this (make sure we don't access garbage in ->cmd for non-passthrough)
> should probably be a separate fix as well.

No, the check was in the upper condition and this reference relied on
it so if I separate this part there is no justification for the
change.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme