| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <91c49412-4251-4e13-80df-9148208aeac7@grimberg.me>
Date: Tue, 9 Apr 2024 23:40:03 +0300
From: Sagi Grimberg <sagi@...mberg.me>
To: Daniel Wagner <dwagner@...e.de>, Christoph Hellwig <hch@....de>
Cc: Keith Busch <kbusch@...nel.org>, James Smart <james.smart@...adcom.com>,
Hannes Reinecke <hare@...e.de>, linux-nvme@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/6] nvme-tcp: short-circuit reconnect retries
On 09/04/2024 12:35, Daniel Wagner wrote:
> From: Hannes Reinecke <hare@...e.de>
>
> Returning an nvme status from nvme_tcp_setup_ctrl() indicates that the
> association was established and we have received a status from the
> controller; consequently we should honour the DNR bit. If not any future
> reconnect attempts will just return the same error, so we can
> short-circuit the reconnect attempts and fail the connection directly.
>
> Signed-off-by: Hannes Reinecke <hare@...e.de>
> [dwagner: add helper to decide to reconnect]
> Signed-off-by: Daniel Wagner <dwagner@...e.de>
> ---
> drivers/nvme/host/nvme.h | 24 ++++++++++++++++++++++++
> drivers/nvme/host/tcp.c | 23 +++++++++++++++--------
> 2 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 9b8904a476b8..dfe103283a3d 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -701,6 +701,30 @@ static inline bool nvme_is_path_error(u16 status)
> return (status & 0x700) == 0x300;
> }
>
> +/*
> + * Evaluate the status information returned by the LLDD
We usually say transport, less so LLDD.
> in order to
> + * decided if a reconnect attempt should be scheduled.
> + *
> + * There are two cases where no reconnect attempt should be attempted:
> + *
> + * 1) The LLDD reports an negative status. There was an error (e.g. no
> + * memory) on the host side and thus abort the operation.
> + * Note, there are exception such as ENOTCONN which is
> + * not an internal driver error, thus we filter these errors
> + * out and retry later.
What is this ENOTCONN here? From what I see its just a random
escape value to bypass the status check. I think that 0 would do the same
wouldn't it?
> + * 2) The DNR bit is set and the specification states no further
> + * connect attempts with the same set of paramenters should be
> + * attempted.
> + */
> +static inline bool nvme_ctrl_reconnect(int status)
> +{
> + if (status < 0 && status != -ENOTCONN)
> + return false;
> + else if (status > 0 && (status & NVME_SC_DNR))
> + return false;
> + return true;
> +}
> +
> /*
> * Fill in the status and result information from the CQE, and then figure out
> * if blk-mq will need to use IPI magic to complete the request, and if yes do
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index fdbcdcedcee9..7e25a96e9870 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2155,9 +2155,11 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
> nvme_tcp_destroy_io_queues(ctrl, remove);
> }
>
> -static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
> +static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl,
> + int status)
> {
> enum nvme_ctrl_state state = nvme_ctrl_state(ctrl);
> + bool recon = nvme_ctrl_reconnect(status);
>
> /* If we are resetting/deleting then do nothing */
> if (state != NVME_CTRL_CONNECTING) {
> @@ -2165,13 +2167,14 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
> return;
> }
>
> - if (nvmf_should_reconnect(ctrl)) {
> + if (recon && nvmf_should_reconnect(ctrl)) {
its weird to have a condition that checks _and_ condition
of two different "should reconnect" conditions.
Why don't we simply pass the status to nvmf_should_reconnect()
and evaluate there?
> dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
> ctrl->opts->reconnect_delay);
> queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work,
> ctrl->opts->reconnect_delay * HZ);
> } else {
> - dev_info(ctrl->device, "Removing controller...\n");
> + dev_info(ctrl->device, "Removing controller (%d)...\n",
> + status);
> nvme_delete_ctrl(ctrl);
> }
> }
> @@ -2252,10 +2255,12 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
> struct nvme_tcp_ctrl *tcp_ctrl = container_of(to_delayed_work(work),
> struct nvme_tcp_ctrl, connect_work);
> struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
> + int ret;
>
> ++ctrl->nr_reconnects;
>
> - if (nvme_tcp_setup_ctrl(ctrl, false))
> + ret = nvme_tcp_setup_ctrl(ctrl, false);
> + if (ret)
> goto requeue;
>
> dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
> @@ -2268,7 +2273,7 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
> requeue:
> dev_info(ctrl->device, "Failed reconnect attempt %d\n",
> ctrl->nr_reconnects);
> - nvme_tcp_reconnect_or_remove(ctrl);
> + nvme_tcp_reconnect_or_remove(ctrl, ret);
> }
>
> static void nvme_tcp_error_recovery_work(struct work_struct *work)
> @@ -2295,7 +2300,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
> return;
> }
>
> - nvme_tcp_reconnect_or_remove(ctrl);
> + nvme_tcp_reconnect_or_remove(ctrl, -ENOTCONN);
> }
>
> static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
> @@ -2315,6 +2320,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
> {
> struct nvme_ctrl *ctrl =
> container_of(work, struct nvme_ctrl, reset_work);
> + int ret;
>
> nvme_stop_ctrl(ctrl);
> nvme_tcp_teardown_ctrl(ctrl, false);
> @@ -2328,14 +2334,15 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
> return;
> }
>
> - if (nvme_tcp_setup_ctrl(ctrl, false))
> + ret = nvme_tcp_setup_ctrl(ctrl, false);
> + if (ret)
> goto out_fail;
>
> return;
>
> out_fail:
> ++ctrl->nr_reconnects;
> - nvme_tcp_reconnect_or_remove(ctrl);
> + nvme_tcp_reconnect_or_remove(ctrl, ret);
> }
>
> static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
Powered by blists - more mailing lists