Re: [PATCH] block: re-introduce blk_mq_complete_request_sync

[Chao Leng <lengchao@huawei.com>]

On 2020/10/15 15:50, Ming Lei wrote:
> On Thu, Oct 15, 2020 at 02:05:01PM +0800, Chao Leng wrote:
>>
>>
>> On 2020/10/14 17:56, Ming Lei wrote:
>>> On Wed, Oct 14, 2020 at 05:39:12PM +0800, Chao Leng wrote:
>>>>
>>>>
>>>> On 2020/10/14 11:34, Ming Lei wrote:
>>>>> On Wed, Oct 14, 2020 at 09:08:28AM +0800, Ming Lei wrote:
>>>>>> On Tue, Oct 13, 2020 at 03:36:08PM -0700, Sagi Grimberg wrote:
>>>>>>>
>>>>>>>>>> This may just reduce the probability. The concurrency of timeout
>>>>>>>>>> and teardown will cause the same request
>>>>>>>>>> be treated repeatly, this is not we expected.
>>>>>>>>>
>>>>>>>>> That is right, not like SCSI, NVME doesn't apply atomic request
>>>>>>>>> completion, so
>>>>>>>>> request may be completed/freed from both timeout & nvme_cancel_request().
>>>>>>>>>
>>>>>>>>> .teardown_lock still may cover the race with Sagi's patch because
>>>>>>>>> teardown
>>>>>>>>> actually cancels requests in sync style.
>>>>>>>> In extreme scenarios, the request may be already retry success(rq state
>>>>>>>> change to inflight).
>>>>>>>> Timeout processing may wrongly stop the queue and abort the request.
>>>>>>>> teardown_lock serialize the process of timeout and teardown, but do not
>>>>>>>> avoid the race.
>>>>>>>> It might not be safe.
>>>>>>>
>>>>>>> Not sure I understand the scenario you are describing.
>>>>>>>
>>>>>>> what do you mean by "In extreme scenarios, the request may be already retry
>>>>>>> success(rq state change to inflight)"?
>>>>>>>
>>>>>>> What will retry the request? only when the host will reconnect
>>>>>>> the request will be retried.
>>>>>>>
>>>>>>> We can call nvme_sync_queues in the last part of the teardown, but
>>>>>>> I still don't understand the race here.
>>>>>>
>>>>>> Not like SCSI, NVME doesn't complete request atomically, so double
>>>>>> completion/free can be done from both timeout & nvme_cancel_request()(via teardown).
>>>>>>
>>>>>> Given request is completed remotely or asynchronously in the two code paths,
>>>>>> the teardown_lock can't protect the case.
>>>>>
>>>>> Thinking of the issue further, the race shouldn't be between timeout and
>>>>> teardown.
>>>>>
>>>>> Both nvme_cancel_request() and nvme_tcp_complete_timed_out() are called
>>>>> with .teardown_lock, and both check if the request is completed before
>>>>> calling blk_mq_complete_request() which marks the request as COMPLETE state.
>>>>> So the request shouldn't be double-freed in the two code paths.
>>>>>
>>>>> Another possible reason is that between timeout and normal completion(fail
>>>>> fast pending requests after ctrl state is updated to CONNECTING).
>>>>>
>>>>> Yi, can you try the following patch and see if the issue is fixed?
>>>>>
>>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>>> index d6a3e1487354..fab9220196bd 100644
>>>>> --- a/drivers/nvme/host/tcp.c
>>>>> +++ b/drivers/nvme/host/tcp.c
>>>>> @@ -1886,7 +1886,6 @@ static int nvme_tcp_configure_admin_queue(struct nvme_ctrl *ctrl, bool new)
>>>>>     static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
>>>>>     		bool remove)
>>>>>     {
>>>>> -	mutex_lock(&to_tcp_ctrl(ctrl)->teardown_lock);
>>>>>     	blk_mq_quiesce_queue(ctrl->admin_q);
>>>>>     	nvme_tcp_stop_queue(ctrl, 0);
>>>>>     	if (ctrl->admin_tagset) {
>>>>> @@ -1897,15 +1896,13 @@ static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
>>>>>     	if (remove)
>>>>>     		blk_mq_unquiesce_queue(ctrl->admin_q);
>>>>>     	nvme_tcp_destroy_admin_queue(ctrl, remove);
>>>>> -	mutex_unlock(&to_tcp_ctrl(ctrl)->teardown_lock);
>>>>>     }
>>>>>     static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
>>>>>     		bool remove)
>>>>>     {
>>>>> -	mutex_lock(&to_tcp_ctrl(ctrl)->teardown_lock);
>>>>>     	if (ctrl->queue_count <= 1)
>>>>> -		goto out;
>>>>> +		return;
>>>>>     	blk_mq_quiesce_queue(ctrl->admin_q);
>>>>>     	nvme_start_freeze(ctrl);
>>>>>     	nvme_stop_queues(ctrl);
>>>>> @@ -1918,8 +1915,6 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
>>>>>     	if (remove)
>>>>>     		nvme_start_queues(ctrl);
>>>>>     	nvme_tcp_destroy_io_queues(ctrl, remove);
>>>>> -out:
>>>>> -	mutex_unlock(&to_tcp_ctrl(ctrl)->teardown_lock);
>>>>>     }
>>>>>     static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>>> @@ -2030,11 +2025,11 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
>>>>>     	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
>>>>>     	nvme_stop_keep_alive(ctrl);
>>>>> +
>>>>> +	mutex_lock(&tcp_ctrl->teardown_lock);
>>>>>     	nvme_tcp_teardown_io_queues(ctrl, false);
>>>>> -	/* unquiesce to fail fast pending requests */
>>>>> -	nvme_start_queues(ctrl);
>>>>>     	nvme_tcp_teardown_admin_queue(ctrl, false);
>>>>> -	blk_mq_unquiesce_queue(ctrl->admin_q);
>>>> Delete blk_mq_unquiesce_queue will cause a bug which may cause reconnect failed.
>>>> Delete nvme_start_queues may cause another bug.
>>>
>>> nvme_tcp_setup_ctrl() will re-start io and admin queue, and only .connect_q
>>> and .fabrics_q are required during reconnect.I check the code. Unquiesce the admin queue in nvme_tcp_configure_admin_queue, so reconnect can work well.
>>>
>>> So can you explain in detail about the bug?
>> First if reconnect failed, quiesce the io queue and admin queue will cause IO pause long time.
> 
> Any normal IO can't make progress until reconnect is successful, so this
> change won't increase IO pause. This way is exactly what NVMe PCI takes,
> see nvme_start_queues() called from nvme_reset_work().
now is ok. Now the patch which fix the long pause time is discussing.
> 
>> Second if reconnect failed more than max_reconnects, delete ctrl will hang.
> 
> No, delete ctrl won't hang, because 'shutdown' parameter is true in case
> of deleting ctrl, which will unquiesce both admin_q and io queues in
> nvme_tcp_teardown_io_queues() and nvme_tcp_teardown_admin_queue().
No, now nvme_remove_namespaces is before tear down queues.
tear down queues is in ctrl->ops->delete_ctrl.
static void nvme_do_delete_ctrl(struct nvme_ctrl *ctrl)
{
	dev_info(ctrl->device,
		 "Removing ctrl: NQN \"%s\"\n", ctrl->opts->subsysnqn);

	flush_work(&ctrl->reset_work);
	nvme_stop_ctrl(ctrl);
	nvme_remove_namespaces(ctrl);
	ctrl->ops->delete_ctrl(ctrl);
	nvme_uninit_ctrl(ctrl);
}
> 
> 
> Thanks,
> Ming
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
> .
>