[PATCH] nvme-tcp: Do not terminate commands when in RESETTING
Sagi Grimberg
sagi at grimberg.me
Thu Jan 11 04:30:36 PST 2024
> From: Hannes Reinecke <hare at 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 at 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?
More information about the Linux-nvme
mailing list