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

[Christoph Hellwig <hch@lst.de>]

On Thu, Aug 06, 2020 at 12:11:26PM -0700, Sagi Grimberg wrote:
> Currently we check if the controller state != LIVE, and
> we directly fail the command under the assumption that this
> is the connect command or an admin command within the
> controller initialization sequence.
> 
> This is wrong, we need to check if the request risking
> controller setup/teardown blocking if not completed and
> only then fail.

FYI: you can use up to 73 characters in the commit log..

> +++ 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.

> +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.

>  {
> @@ -1961,29 +1979,43 @@ nvme_rdma_timeout(struct request *rq, bool reserved)
>  	dev_warn(ctrl->ctrl.device, "I/O %d QID %d timeout\n",
>  		 rq->tag, nvme_rdma_queue_idx(queue));
>  
> +	switch (ctrl->ctrl.state) {
> +	case NVME_CTRL_RESETTING:
> +		if (!nvme_rdma_queue_idx(queue)) {
> +			/*
> +			 * if we are in teardown we must complete immediately
> +			 * because we may block the teardown sequence (e.g.
> +			 * nvme_disable_ctrl timed out).
> +			 */

Please start the setence with an upper case character.

> +			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;
}

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