Re: [PATCH] nvme-tcp: Do not terminate commands when in RESETTING

[Sagi Grimberg <sagi@grimberg.me>]

> From: Hannes Reinecke <hare@suse.de>
> 
> Terminating commands from the timeout handler might lead
> to a data corruption as the timeout might trigger before
> KATO expired.
> This is the case when several commands have been started
> before the keep-alive command and the command timeouts
> trigger just after the keep-alive command has been sent.
> Then the first command will trigger an error recovery,
> but all the other commands will be aborted directly
> and immediately retried.
> So return BLK_EH_RESET_TIMER for I/O commands when
> error recovery has been started to ensure that the
> commands will be retried only after the KATO interval.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/tcp.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index b234f0674aeb..b9ec121b3fc6 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2429,6 +2429,18 @@ static enum blk_eh_timer_return nvme_tcp_timeout(struct request *rq)
>   		 rq->tag, nvme_cid(rq), pdu->hdr.type, opc,
>   		 nvme_opcode_str(qid, opc, fctype), qid);
>   
> +	/*
> +	 * If the error recovery is started we should ignore all
> +	 * I/O commands as they'll be aborted once error recovery starts.
> +	 * Otherwise they'll be failed over immediately and might
> +	 * cause data corruption.

I think that we can settle with a simpler comment:
	/*
	 * If the controller is in state RESETTING, the error
	 * recovery handler will cancel all inflight commands soon
	 * which in turn may failover to a different path.
	 */

The error handler will start promptly anyways.

> +	 */
> +	if (ctrl->state == NVME_CTRL_RESETTING && qid > 0) {

What would be the issue doing this for admin commands as well?

> +		/* Avoid interfering with firmware download */
> +		if (!WARN_ON(work_pending(&ctrl->fw_act_work)))

Let's ignore the fw_act_work, it's not a thing in fabrics.

> +			return BLK_EH_RESET_TIMER;
> +	}
> +
>   	if (ctrl->state != NVME_CTRL_LIVE) {
>   		/*
>   		 * If we are resetting, connecting or deleting we should

I think that the comment here needs an update as well.

BTW, what happens when deleting a controller? Won't the state be
DELETING and the situation you are trying to prevent happen as well?