Re: [PATCH v2 7/8] nvme-rdma: fix timeout handler

[Sagi Grimberg <sagi@grimberg.me>]

>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1185,6 +1185,7 @@ static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
>>   	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
>>   		return;
>>   
>> +	dev_warn(ctrl->ctrl.device, "starting error recovery\n");
> 
> Should this really be a warning?  I'd turn this down to _info.

I can do that. but given that this is a fix, I'll to do that
as a separate patch.

>> +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 */
>> +	mutex_lock(&ctrl->teardown_lock);
>> +	nvme_rdma_stop_queue(queue);
>> +	if (blk_mq_request_completed(rq))
>> +		goto out;
>> +	nvme_req(rq)->status = NVME_SC_HOST_ABORTED_CMD;
>> +	blk_mq_complete_request(rq);
>> +out:
> 
> Nit: I'd probably avoid the goto here for a slightly simpler flow.

OK

>> +			nvme_rdma_complete_timed_out(rq);
>> +			return BLK_EH_DONE;
>> +		}
>> +		/*
>> +		 * Restart the timer if a controller reset is already scheduled.
>> +		 * Any timed out commands would be handled before entering the
>> +		 * connecting state.
>> +		 */
>>   		return BLK_EH_RESET_TIMER;
>> +	case NVME_CTRL_CONNECTING:
>> +		if (reserved || !nvme_rdma_queue_idx(queue)) {
>> +			/*
>> +			 * if we are connecting we must complete immediately
>> +			 * connect (reserved) or admin requests because we may
>> +			 * block controller setup sequence.
>> +			 */
>> +			nvme_rdma_complete_timed_out(rq);
>> +			return BLK_EH_DONE;
> 
> A goto to share the immediate completion branch would be nice.  I wonder
> if we should also do it for the reserved case during shutdown even if
> that should never happen and entirely share the code, though:
> 
> 	switch (ctrl->ctrl.state) {
> 	case NVME_CTRL_RESETTING:
> 	case NVME_CTRL_CONNECTING:
> 		/*
> 		 * If we are connecting or connecting, we must complete
> 		 * connect (reserved) or admin requests immediately, because
> 		 * they may block the controller setup or teardown sequence.
> 		 */
> 		if (reserved || !nvme_rdma_queue_idx(queue)) {
> 			nvme_rdma_complete_timed_out(rq);
> 			return BLK_EH_DONE;
> 		}
> 		break;
> 	default:
> 		break;
> 	}
> 
> 	nvme_rdma_error_recovery(ctrl);
>   	return BLK_EH_RESET_TIMER;
> }

Maybe that can work, I'll look into that. thanks.

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