Re: [PATCH 5/6] nvme-rdma: fix timeout handler

[Chao Leng <lengchao@huawei.com>]

On 2020/8/4 23:36, Sagi Grimberg wrote:
> 
>>>>> +static void nvme_rdma_complete_timed_out(struct request *rq)
>>>>> +{
>>>>> +    struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
>>>>> +    struct nvme_rdma_queue *queue = req->queue;
>>>>> +    struct nvme_rdma_ctrl *ctrl = queue->ctrl;
>>>>> +
>>>>> +    /* fence other contexts that may complete the command */
>>>>> +    flush_work(&ctrl->err_work);
>>>>> +    nvme_rdma_stop_queue(queue);
>>>> There maybe concurrent with error recovery, may cause abnormal because
>>>> nvme_rdma_stop_queue will return but the queue is not stoped,
>>>> maybe is stopping by the error recovery.
>>>
>>> err_work flush used to fence, once we did queue stop, it should be safe
>>> to complete the command from the timeout handler.
>>
>> Flush work just can avoid trigger error recovery by nvme_rdma_timeout or
>> reduce concurrent probalibity trigger error recovery by other progress,
>> but can not avoid.
> 
> The point is that we can complete the command because err_work
> was flushed and the queue was stopped, which means we shouldn't have
> any context completing the request.
> 
>> if nvme_rdma_cm_handler or other progress call
>> nvme_rdma_error_recovery, between change state to queue_work may
>> interrupt by hard interrupt, and then timeout happen, thus flush work
>> can not avoid concurrent.
>> Like this:
>>
>> static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
>> {
>>      if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
>>          return;
>> --------------------------------
> 
> If we are in RESETTING/CONNECTING state already, this won't do anything.
> 
>> may interrupt by hard interrupt, and then timeout progress flush work
>> at this time. Thus error recovery and nvme_rdma_complete_timed_out may
>> concurrent to stop queue. will cause: error recovery may cancel request
>> or nvme_rdma_complete_timed_out may complete request, but the queue may
>> not be stoped. Thus will cause abnormal.
> 
> We should be fine and safe to complete the I/O.

Complete request in nvme_rdma_timeout or cancel request in
nvme_rdma_error_recovery_work or nvme_rdma_reset_ctrl_work is not safe.
Because the queue may be not really stoped, it may just cleard the flag:
NVME_RDMA_Q_ALLOCATED for the queue. Thus one request may concurrent
treat by two progress, it is not allowed.

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