[PATCH v1 0/7] Centrelize common fabrics code to core drivers

[PATCH v1 0/7] Centrelize common fabrics code to core drivers

[Max Gurtovoy]

Hi Christoph, Sagi, Keith and Co,
This series is intended to move common reconnection, device removal and
error recovery logic to common fabrics and core drivers. In this stage,
it mainly deals with the RDMA and TCP drivers which had the same logic
for the above tasks. FC driver can start using this framework in the
future after some preparation work that should be done.

This will ease on code maintenance and reduce the code duplication
between the RDMA and TCP drivers.

Since there are some attributes that were moved to generic nvme_ctrl
structure, I've measured perf impact.

NVMe PCI Perf numbers using NVIDIA's NVMe SNAP Bluefield-2 device
(Fio 16 jobs, 128 iodepth, libaio):

IO size   rand READ (before/after)  rand WRITE (before/after)
-------- -------------------------  --------------------------
 512B          2189k/2197k                2031k/2092k
 1KB           2188k/2201k                2027k/2088k
 2KB           2166k/2181k                2023k/2078k
 4KB           2153k/2169k                2025k/2076k
 8KB           1381k/1381k                1378k/1375k
 16KB          669k/676k                  660k/664k
 32KB          320k/322k                  305k/305k
 64KB          157k/157k                  151k/151k

As we can see, there is no impact on the performance.

Max Gurtovoy (7):
  nvme: add connect_work attribute to nvme ctrl
  nvme-fabrics: introduce nvmf_reconnect_or_remove API
  nvme: add err_work attribute to nvme ctrl
  nvme-fabrics: introduce nvmf_error_recovery API
  nvme/nvme-fabrics: introduce nvmf_error_recovery_work API
  nvme/nvme-fabrics: introduce nvmf_reconnect_ctrl_work API
  nvme-fabrics: add nvmf_init_ctrl/nvmf_teardown_ctrl API

 drivers/nvme/host/fabrics.c |  90 ++++++++++++++++++++++++
 drivers/nvme/host/fabrics.h |   4 ++
 drivers/nvme/host/fc.c      |  23 ++++---
 drivers/nvme/host/nvme.h    |   7 ++
 drivers/nvme/host/rdma.c    | 134 ++++++++++--------------------------
 drivers/nvme/host/tcp.c     | 124 ++++++++-------------------------
 6 files changed, 180 insertions(+), 202 deletions(-)

-- 
2.18.1

[PATCH 1/7] nvme: add connect_work attribute to nvme ctrl

[Max Gurtovoy]

This structure is duplicated for all the fabric controllers. Move it to
common code.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fc.c   | 23 ++++++++++++-----------
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/rdma.c | 10 ++++------
 drivers/nvme/host/tcp.c  |  9 ++++-----
 4 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index aa14ad963d91..4c7dffa8126e 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -167,7 +167,6 @@ struct nvme_fc_ctrl {
 	struct blk_mq_tag_set	tag_set;
 
 	struct work_struct	ioerr_work;
-	struct delayed_work	connect_work;
 
 	struct kref		ref;
 	unsigned long		flags;
@@ -567,7 +566,7 @@ nvme_fc_resume_controller(struct nvme_fc_ctrl *ctrl)
 			"NVME-FC{%d}: connectivity re-established. "
 			"Attempting reconnect\n", ctrl->cnum);
 
-		queue_delayed_work(nvme_wq, &ctrl->connect_work, 0);
+		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work, 0);
 		break;
 
 	case NVME_CTRL_RESETTING:
@@ -3263,7 +3262,7 @@ nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
 	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
 
 	cancel_work_sync(&ctrl->ioerr_work);
-	cancel_delayed_work_sync(&ctrl->connect_work);
+	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
 	/*
 	 * kill the association on the link side.  this will block
 	 * waiting for io to terminate
@@ -3300,7 +3299,8 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
 		else if (time_after(jiffies + recon_delay, rport->dev_loss_end))
 			recon_delay = rport->dev_loss_end - jiffies;
 
-		queue_delayed_work(nvme_wq, &ctrl->connect_work, recon_delay);
+		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
+				   recon_delay);
 	} else {
 		if (portptr->port_state == FC_OBJSTATE_ONLINE) {
 			if (status > 0 && (status & NVME_SC_DNR))
@@ -3340,12 +3340,13 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
 			"to CONNECTING\n", ctrl->cnum);
 
 	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) {
-		if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
+		if (!queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
+					0)) {
 			dev_err(ctrl->ctrl.device,
 				"NVME-FC{%d}: failed to schedule connect "
 				"after reset\n", ctrl->cnum);
 		} else {
-			flush_delayed_work(&ctrl->connect_work);
+			flush_delayed_work(&ctrl->ctrl.connect_work);
 		}
 	} else {
 		nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN);
@@ -3373,7 +3374,7 @@ nvme_fc_connect_ctrl_work(struct work_struct *work)
 
 	struct nvme_fc_ctrl *ctrl =
 			container_of(to_delayed_work(work),
-				struct nvme_fc_ctrl, connect_work);
+				struct nvme_fc_ctrl, ctrl.connect_work);
 
 	ret = nvme_fc_create_association(ctrl);
 	if (ret)
@@ -3485,7 +3486,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	kref_init(&ctrl->ref);
 
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_fc_reset_ctrl_work);
-	INIT_DELAYED_WORK(&ctrl->connect_work, nvme_fc_connect_ctrl_work);
+	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvme_fc_connect_ctrl_work);
 	INIT_WORK(&ctrl->ioerr_work, nvme_fc_ctrl_ioerr_work);
 	spin_lock_init(&ctrl->lock);
 
@@ -3561,14 +3562,14 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 		goto fail_ctrl;
 	}
 
-	if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
+	if (!queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work, 0)) {
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: failed to schedule initial connect\n",
 			ctrl->cnum);
 		goto fail_ctrl;
 	}
 
-	flush_delayed_work(&ctrl->connect_work);
+	flush_delayed_work(&ctrl->ctrl.connect_work);
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d}: new ctrl: NQN \"%s\"\n",
@@ -3580,7 +3581,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
 	cancel_work_sync(&ctrl->ioerr_work);
 	cancel_work_sync(&ctrl->ctrl.reset_work);
-	cancel_delayed_work_sync(&ctrl->connect_work);
+	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
 
 	ctrl->ctrl.opts = NULL;
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ed79a6c7e804..81ca5dd9b7f9 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -343,6 +343,7 @@ struct nvme_ctrl {
 	unsigned long flags;
 #define NVME_CTRL_FAILFAST_EXPIRED	0
 	struct nvmf_ctrl_options *opts;
+	struct delayed_work connect_work;
 
 	struct page *discard_page;
 	unsigned long discard_page_busy;
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 0498801542eb..fbfa18a47bd8 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -110,8 +110,6 @@ struct nvme_rdma_ctrl {
 
 	struct nvme_rdma_qe	async_event_sqe;
 
-	struct delayed_work	reconnect_work;
-
 	struct list_head	list;
 
 	struct blk_mq_tag_set	admin_tag_set;
@@ -1078,7 +1076,7 @@ static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
 	if (nvmf_should_reconnect(&ctrl->ctrl)) {
 		dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n",
 			ctrl->ctrl.opts->reconnect_delay);
-		queue_delayed_work(nvme_wq, &ctrl->reconnect_work,
+		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
 				ctrl->ctrl.opts->reconnect_delay * HZ);
 	} else {
 		nvme_delete_ctrl(&ctrl->ctrl);
@@ -1166,7 +1164,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 {
 	struct nvme_rdma_ctrl *ctrl = container_of(to_delayed_work(work),
-			struct nvme_rdma_ctrl, reconnect_work);
+			struct nvme_rdma_ctrl, ctrl.connect_work);
 
 	++ctrl->ctrl.nr_reconnects;
 
@@ -2230,7 +2228,7 @@ static const struct blk_mq_ops nvme_rdma_admin_mq_ops = {
 static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 {
 	cancel_work_sync(&ctrl->err_work);
-	cancel_delayed_work_sync(&ctrl->reconnect_work);
+	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
 
 	nvme_rdma_teardown_io_queues(ctrl, shutdown);
 	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
@@ -2358,7 +2356,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		goto out_free_ctrl;
 	}
 
-	INIT_DELAYED_WORK(&ctrl->reconnect_work,
+	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
 			nvme_rdma_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 3c1c29dd3020..3ace20e39c86 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -127,7 +127,6 @@ struct nvme_tcp_ctrl {
 	struct nvme_ctrl	ctrl;
 
 	struct work_struct	err_work;
-	struct delayed_work	connect_work;
 	struct nvme_tcp_request async_req;
 	u32			io_queues[HCTX_MAX_TYPES];
 };
@@ -1983,7 +1982,7 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
 	if (nvmf_should_reconnect(ctrl)) {
 		dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
 			ctrl->opts->reconnect_delay);
-		queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work,
+		queue_delayed_work(nvme_wq, &ctrl->connect_work,
 				ctrl->opts->reconnect_delay * HZ);
 	} else {
 		dev_info(ctrl->device, "Removing controller...\n");
@@ -2066,7 +2065,7 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 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_tcp_ctrl, ctrl.connect_work);
 	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
 
 	++ctrl->nr_reconnects;
@@ -2113,7 +2112,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
 	cancel_work_sync(&to_tcp_ctrl(ctrl)->err_work);
-	cancel_delayed_work_sync(&to_tcp_ctrl(ctrl)->connect_work);
+	cancel_delayed_work_sync(&ctrl->connect_work);
 
 	nvme_tcp_teardown_io_queues(ctrl, shutdown);
 	blk_mq_quiesce_queue(ctrl->admin_q);
@@ -2513,7 +2512,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 	ctrl->ctrl.sqsize = opts->queue_size - 1;
 	ctrl->ctrl.kato = opts->kato;
 
-	INIT_DELAYED_WORK(&ctrl->connect_work,
+	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
 			nvme_tcp_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->err_work, nvme_tcp_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
-- 
2.18.1

Re: [PATCH 1/7] nvme: add connect_work attribute to nvme ctrl

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 1/7] nvme: add connect_work attribute to nvme ctrl

[Hannes Reinecke]

On 10/18/21 3:40 PM, Max Gurtovoy wrote:
> This structure is duplicated for all the fabric controllers. Move it to
> common code.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/fc.c   | 23 ++++++++++++-----------
>   drivers/nvme/host/nvme.h |  1 +
>   drivers/nvme/host/rdma.c | 10 ++++------
>   drivers/nvme/host/tcp.c  |  9 ++++-----
>   4 files changed, 21 insertions(+), 22 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/7] nvme: add connect_work attribute to nvme ctrl

[Himanshu Madhani]

> On Oct 18, 2021, at 8:40 AM, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> 
> This structure is duplicated for all the fabric controllers. Move it to
> common code.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
> drivers/nvme/host/fc.c   | 23 ++++++++++++-----------
> drivers/nvme/host/nvme.h |  1 +
> drivers/nvme/host/rdma.c | 10 ++++------
> drivers/nvme/host/tcp.c  |  9 ++++-----
> 4 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index aa14ad963d91..4c7dffa8126e 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -167,7 +167,6 @@ struct nvme_fc_ctrl {
> 	struct blk_mq_tag_set	tag_set;
> 
> 	struct work_struct	ioerr_work;
> -	struct delayed_work	connect_work;
> 
> 	struct kref		ref;
> 	unsigned long		flags;
> @@ -567,7 +566,7 @@ nvme_fc_resume_controller(struct nvme_fc_ctrl *ctrl)
> 			"NVME-FC{%d}: connectivity re-established. "
> 			"Attempting reconnect\n", ctrl->cnum);
> 
> -		queue_delayed_work(nvme_wq, &ctrl->connect_work, 0);
> +		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work, 0);
> 		break;
> 
> 	case NVME_CTRL_RESETTING:
> @@ -3263,7 +3262,7 @@ nvme_fc_delete_ctrl(struct nvme_ctrl *nctrl)
> 	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
> 
> 	cancel_work_sync(&ctrl->ioerr_work);
> -	cancel_delayed_work_sync(&ctrl->connect_work);
> +	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
> 	/*
> 	 * kill the association on the link side.  this will block
> 	 * waiting for io to terminate
> @@ -3300,7 +3299,8 @@ nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status)
> 		else if (time_after(jiffies + recon_delay, rport->dev_loss_end))
> 			recon_delay = rport->dev_loss_end - jiffies;
> 
> -		queue_delayed_work(nvme_wq, &ctrl->connect_work, recon_delay);
> +		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
> +				   recon_delay);
> 	} else {
> 		if (portptr->port_state == FC_OBJSTATE_ONLINE) {
> 			if (status > 0 && (status & NVME_SC_DNR))
> @@ -3340,12 +3340,13 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
> 			"to CONNECTING\n", ctrl->cnum);
> 
> 	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) {
> -		if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
> +		if (!queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
> +					0)) {
> 			dev_err(ctrl->ctrl.device,
> 				"NVME-FC{%d}: failed to schedule connect "
> 				"after reset\n", ctrl->cnum);
> 		} else {
> -			flush_delayed_work(&ctrl->connect_work);
> +			flush_delayed_work(&ctrl->ctrl.connect_work);
> 		}
> 	} else {
> 		nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN);
> @@ -3373,7 +3374,7 @@ nvme_fc_connect_ctrl_work(struct work_struct *work)
> 
> 	struct nvme_fc_ctrl *ctrl =
> 			container_of(to_delayed_work(work),
> -				struct nvme_fc_ctrl, connect_work);
> +				struct nvme_fc_ctrl, ctrl.connect_work);
> 
> 	ret = nvme_fc_create_association(ctrl);
> 	if (ret)
> @@ -3485,7 +3486,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
> 	kref_init(&ctrl->ref);
> 
> 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_fc_reset_ctrl_work);
> -	INIT_DELAYED_WORK(&ctrl->connect_work, nvme_fc_connect_ctrl_work);
> +	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvme_fc_connect_ctrl_work);
> 	INIT_WORK(&ctrl->ioerr_work, nvme_fc_ctrl_ioerr_work);
> 	spin_lock_init(&ctrl->lock);
> 
> @@ -3561,14 +3562,14 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
> 		goto fail_ctrl;
> 	}
> 
> -	if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
> +	if (!queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work, 0)) {
> 		dev_err(ctrl->ctrl.device,
> 			"NVME-FC{%d}: failed to schedule initial connect\n",
> 			ctrl->cnum);
> 		goto fail_ctrl;
> 	}
> 
> -	flush_delayed_work(&ctrl->connect_work);
> +	flush_delayed_work(&ctrl->ctrl.connect_work);
> 
> 	dev_info(ctrl->ctrl.device,
> 		"NVME-FC{%d}: new ctrl: NQN \"%s\"\n",
> @@ -3580,7 +3581,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
> 	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
> 	cancel_work_sync(&ctrl->ioerr_work);
> 	cancel_work_sync(&ctrl->ctrl.reset_work);
> -	cancel_delayed_work_sync(&ctrl->connect_work);
> +	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
> 
> 	ctrl->ctrl.opts = NULL;
> 
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index ed79a6c7e804..81ca5dd9b7f9 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -343,6 +343,7 @@ struct nvme_ctrl {
> 	unsigned long flags;
> #define NVME_CTRL_FAILFAST_EXPIRED	0
> 	struct nvmf_ctrl_options *opts;
> +	struct delayed_work connect_work;
> 
> 	struct page *discard_page;
> 	unsigned long discard_page_busy;
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 0498801542eb..fbfa18a47bd8 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -110,8 +110,6 @@ struct nvme_rdma_ctrl {
> 
> 	struct nvme_rdma_qe	async_event_sqe;
> 
> -	struct delayed_work	reconnect_work;
> -
> 	struct list_head	list;
> 
> 	struct blk_mq_tag_set	admin_tag_set;
> @@ -1078,7 +1076,7 @@ static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
> 	if (nvmf_should_reconnect(&ctrl->ctrl)) {
> 		dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n",
> 			ctrl->ctrl.opts->reconnect_delay);
> -		queue_delayed_work(nvme_wq, &ctrl->reconnect_work,
> +		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
> 				ctrl->ctrl.opts->reconnect_delay * HZ);
> 	} else {
> 		nvme_delete_ctrl(&ctrl->ctrl);
> @@ -1166,7 +1164,7 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
> static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
> {
> 	struct nvme_rdma_ctrl *ctrl = container_of(to_delayed_work(work),
> -			struct nvme_rdma_ctrl, reconnect_work);
> +			struct nvme_rdma_ctrl, ctrl.connect_work);
> 
> 	++ctrl->ctrl.nr_reconnects;
> 
> @@ -2230,7 +2228,7 @@ static const struct blk_mq_ops nvme_rdma_admin_mq_ops = {
> static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
> {
> 	cancel_work_sync(&ctrl->err_work);
> -	cancel_delayed_work_sync(&ctrl->reconnect_work);
> +	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
> 
> 	nvme_rdma_teardown_io_queues(ctrl, shutdown);
> 	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
> @@ -2358,7 +2356,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
> 		goto out_free_ctrl;
> 	}
> 
> -	INIT_DELAYED_WORK(&ctrl->reconnect_work,
> +	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
> 			nvme_rdma_reconnect_ctrl_work);
> 	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
> 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 3c1c29dd3020..3ace20e39c86 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -127,7 +127,6 @@ struct nvme_tcp_ctrl {
> 	struct nvme_ctrl	ctrl;
> 
> 	struct work_struct	err_work;
> -	struct delayed_work	connect_work;
> 	struct nvme_tcp_request async_req;
> 	u32			io_queues[HCTX_MAX_TYPES];
> };
> @@ -1983,7 +1982,7 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
> 	if (nvmf_should_reconnect(ctrl)) {
> 		dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
> 			ctrl->opts->reconnect_delay);
> -		queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work,
> +		queue_delayed_work(nvme_wq, &ctrl->connect_work,
> 				ctrl->opts->reconnect_delay * HZ);
> 	} else {
> 		dev_info(ctrl->device, "Removing controller...\n");
> @@ -2066,7 +2065,7 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
> 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_tcp_ctrl, ctrl.connect_work);
> 	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
> 
> 	++ctrl->nr_reconnects;
> @@ -2113,7 +2112,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
> static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
> {
> 	cancel_work_sync(&to_tcp_ctrl(ctrl)->err_work);
> -	cancel_delayed_work_sync(&to_tcp_ctrl(ctrl)->connect_work);
> +	cancel_delayed_work_sync(&ctrl->connect_work);
> 
> 	nvme_tcp_teardown_io_queues(ctrl, shutdown);
> 	blk_mq_quiesce_queue(ctrl->admin_q);
> @@ -2513,7 +2512,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
> 	ctrl->ctrl.sqsize = opts->queue_size - 1;
> 	ctrl->ctrl.kato = opts->kato;
> 
> -	INIT_DELAYED_WORK(&ctrl->connect_work,
> +	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
> 			nvme_tcp_reconnect_ctrl_work);
> 	INIT_WORK(&ctrl->err_work, nvme_tcp_error_recovery_work);
> 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
> -- 
> 2.18.1
> 
> 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

[PATCH 2/7] nvme-fabrics: introduce nvmf_reconnect_or_remove API

[Max Gurtovoy]

This logic is duplicated today for RDMA and TCP controllers. Move it to
the fabrics driver and export it as a new API.

Also update the RDMA/TCP transport drivers to use this API and remove
the duplicated code.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 21 +++++++++++++++++++++
 drivers/nvme/host/fabrics.h |  1 +
 drivers/nvme/host/rdma.c    | 25 +++----------------------
 drivers/nvme/host/tcp.c     | 26 +++-----------------------
 4 files changed, 28 insertions(+), 45 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 668c6bb7a567..4a1ef67c6fb3 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -472,6 +472,27 @@ bool nvmf_should_reconnect(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvmf_should_reconnect);
 
+void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
+{
+	/* If we are resetting/deleting then do nothing */
+	if (ctrl->state != NVME_CTRL_CONNECTING) {
+		WARN_ON_ONCE(ctrl->state == NVME_CTRL_NEW ||
+			ctrl->state == NVME_CTRL_LIVE);
+		return;
+	}
+
+	if (nvmf_should_reconnect(ctrl)) {
+		dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
+			ctrl->opts->reconnect_delay);
+		queue_delayed_work(nvme_wq, &ctrl->connect_work,
+				ctrl->opts->reconnect_delay * HZ);
+	} else {
+		dev_info(ctrl->device, "Removing controller...\n");
+		nvme_delete_ctrl(ctrl);
+	}
+}
+EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
+
 /**
  * nvmf_register_transport() - NVMe Fabrics Library registration function.
  * @ops:	Transport ops instance to be registered to the
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index a146cb903869..de213ab26977 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -188,6 +188,7 @@ void nvmf_unregister_transport(struct nvmf_transport_ops *ops);
 void nvmf_free_options(struct nvmf_ctrl_options *opts);
 int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
+void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
 bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 		struct nvmf_ctrl_options *opts);
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index fbfa18a47bd8..f31a56d8fd73 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1064,25 +1064,6 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
 	kfree(ctrl);
 }
 
-static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
-{
-	/* If we are resetting/deleting then do nothing */
-	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
-		WARN_ON_ONCE(ctrl->ctrl.state == NVME_CTRL_NEW ||
-			ctrl->ctrl.state == NVME_CTRL_LIVE);
-		return;
-	}
-
-	if (nvmf_should_reconnect(&ctrl->ctrl)) {
-		dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n",
-			ctrl->ctrl.opts->reconnect_delay);
-		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
-				ctrl->ctrl.opts->reconnect_delay * HZ);
-	} else {
-		nvme_delete_ctrl(&ctrl->ctrl);
-	}
-}
-
 static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 {
 	int ret;
@@ -1181,7 +1162,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 requeue:
 	dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
 			ctrl->ctrl.nr_reconnects);
-	nvme_rdma_reconnect_or_remove(ctrl);
+	nvmf_reconnect_or_remove(&ctrl->ctrl);
 }
 
 static void nvme_rdma_error_recovery_work(struct work_struct *work)
@@ -1202,7 +1183,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 		return;
 	}
 
-	nvme_rdma_reconnect_or_remove(ctrl);
+	nvmf_reconnect_or_remove(&ctrl->ctrl);
 }
 
 static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
@@ -2265,7 +2246,7 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 
 out_fail:
 	++ctrl->ctrl.nr_reconnects;
-	nvme_rdma_reconnect_or_remove(ctrl);
+	nvmf_reconnect_or_remove(&ctrl->ctrl);
 }
 
 static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 3ace20e39c86..530ff76d4ac9 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1970,26 +1970,6 @@ 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)
-{
-	/* If we are resetting/deleting then do nothing */
-	if (ctrl->state != NVME_CTRL_CONNECTING) {
-		WARN_ON_ONCE(ctrl->state == NVME_CTRL_NEW ||
-			ctrl->state == NVME_CTRL_LIVE);
-		return;
-	}
-
-	if (nvmf_should_reconnect(ctrl)) {
-		dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
-			ctrl->opts->reconnect_delay);
-		queue_delayed_work(nvme_wq, &ctrl->connect_work,
-				ctrl->opts->reconnect_delay * HZ);
-	} else {
-		dev_info(ctrl->device, "Removing controller...\n");
-		nvme_delete_ctrl(ctrl);
-	}
-}
-
 static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 {
 	struct nvmf_ctrl_options *opts = ctrl->opts;
@@ -2083,7 +2063,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);
+	nvmf_reconnect_or_remove(ctrl);
 }
 
 static void nvme_tcp_error_recovery_work(struct work_struct *work)
@@ -2106,7 +2086,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 		return;
 	}
 
-	nvme_tcp_reconnect_or_remove(ctrl);
+	nvmf_reconnect_or_remove(ctrl);
 }
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
@@ -2150,7 +2130,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
 
 out_fail:
 	++ctrl->nr_reconnects;
-	nvme_tcp_reconnect_or_remove(ctrl);
+	nvmf_reconnect_or_remove(ctrl);
 }
 
 static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
-- 
2.18.1

Re: [PATCH 2/7] nvme-fabrics: introduce nvmf_reconnect_or_remove API

[Chaitanya Kulkarni]

On 10/18/2021 6:40 AM, Max Gurtovoy wrote:
> This logic is duplicated today for RDMA and TCP controllers. Move it to
> the fabrics driver and export it as a new API.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH 2/7] nvme-fabrics: introduce nvmf_reconnect_or_remove API

[Sagi Grimberg]

> This logic is duplicated today for RDMA and TCP controllers. Move it to
> the fabrics driver and export it as a new API.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/fabrics.c | 21 +++++++++++++++++++++
>   drivers/nvme/host/fabrics.h |  1 +
>   drivers/nvme/host/rdma.c    | 25 +++----------------------
>   drivers/nvme/host/tcp.c     | 26 +++-----------------------
>   4 files changed, 28 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 668c6bb7a567..4a1ef67c6fb3 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -472,6 +472,27 @@ bool nvmf_should_reconnect(struct nvme_ctrl *ctrl)
>   }
>   EXPORT_SYMBOL_GPL(nvmf_should_reconnect);
>   
> +void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
> +{
> +	/* If we are resetting/deleting then do nothing */
> +	if (ctrl->state != NVME_CTRL_CONNECTING) {
> +		WARN_ON_ONCE(ctrl->state == NVME_CTRL_NEW ||
> +			ctrl->state == NVME_CTRL_LIVE);
> +		return;
> +	}
> +
> +	if (nvmf_should_reconnect(ctrl)) {
> +		dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
> +			ctrl->opts->reconnect_delay);
> +		queue_delayed_work(nvme_wq, &ctrl->connect_work,
> +				ctrl->opts->reconnect_delay * HZ);
> +	} else {
> +		dev_info(ctrl->device, "Removing controller...\n");
> +		nvme_delete_ctrl(ctrl);
> +	}

We need to look at fc here as it does this differently.
Can it converge somehow? if not we can add a callback for a driver
to override it.

Re: [PATCH 2/7] nvme-fabrics: introduce nvmf_reconnect_or_remove API

[Max Gurtovoy]

On 10/19/2021 3:36 PM, Sagi Grimberg wrote:
>
>> This logic is duplicated today for RDMA and TCP controllers. Move it to
>> the fabrics driver and export it as a new API.
>>
>> Also update the RDMA/TCP transport drivers to use this API and remove
>> the duplicated code.
>>
>> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   drivers/nvme/host/fabrics.c | 21 +++++++++++++++++++++
>>   drivers/nvme/host/fabrics.h |  1 +
>>   drivers/nvme/host/rdma.c    | 25 +++----------------------
>>   drivers/nvme/host/tcp.c     | 26 +++-----------------------
>>   4 files changed, 28 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 668c6bb7a567..4a1ef67c6fb3 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -472,6 +472,27 @@ bool nvmf_should_reconnect(struct nvme_ctrl *ctrl)
>>   }
>>   EXPORT_SYMBOL_GPL(nvmf_should_reconnect);
>>   +void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
>> +{
>> +    /* If we are resetting/deleting then do nothing */
>> +    if (ctrl->state != NVME_CTRL_CONNECTING) {
>> +        WARN_ON_ONCE(ctrl->state == NVME_CTRL_NEW ||
>> +            ctrl->state == NVME_CTRL_LIVE);
>> +        return;
>> +    }
>> +
>> +    if (nvmf_should_reconnect(ctrl)) {
>> +        dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
>> +            ctrl->opts->reconnect_delay);
>> +        queue_delayed_work(nvme_wq, &ctrl->connect_work,
>> +                ctrl->opts->reconnect_delay * HZ);
>> +    } else {
>> +        dev_info(ctrl->device, "Removing controller...\n");
>> +        nvme_delete_ctrl(ctrl);
>> +    }
>
> We need to look at fc here as it does this differently.
> Can it converge somehow? if not we can add a callback for a driver
> to override it.

we need to start somewhere.

Adding a callback to override this logic is not a problem and i thought 
about it but decided not to do so.

James and other FC developers evaluate moving FC code using more core 
functionality to ease on this driver.

This work can be done also after merging this series.

This series is not related to that.

Re: [PATCH 2/7] nvme-fabrics: introduce nvmf_reconnect_or_remove API

[Hannes Reinecke]

On 10/18/21 3:40 PM, Max Gurtovoy wrote:
> This logic is duplicated today for RDMA and TCP controllers. Move it to
> the fabrics driver and export it as a new API.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/fabrics.c | 21 +++++++++++++++++++++
>   drivers/nvme/host/fabrics.h |  1 +
>   drivers/nvme/host/rdma.c    | 25 +++----------------------
>   drivers/nvme/host/tcp.c     | 26 +++-----------------------
>   4 files changed, 28 insertions(+), 45 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/7] nvme-fabrics: introduce nvmf_reconnect_or_remove API

[Himanshu Madhani]

> On Oct 18, 2021, at 8:40 AM, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> 
> This logic is duplicated today for RDMA and TCP controllers. Move it to
> the fabrics driver and export it as a new API.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
> drivers/nvme/host/fabrics.c | 21 +++++++++++++++++++++
> drivers/nvme/host/fabrics.h |  1 +
> drivers/nvme/host/rdma.c    | 25 +++----------------------
> drivers/nvme/host/tcp.c     | 26 +++-----------------------
> 4 files changed, 28 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 668c6bb7a567..4a1ef67c6fb3 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -472,6 +472,27 @@ bool nvmf_should_reconnect(struct nvme_ctrl *ctrl)
> }
> EXPORT_SYMBOL_GPL(nvmf_should_reconnect);
> 
> +void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
> +{
> +	/* If we are resetting/deleting then do nothing */
> +	if (ctrl->state != NVME_CTRL_CONNECTING) {
> +		WARN_ON_ONCE(ctrl->state == NVME_CTRL_NEW ||
> +			ctrl->state == NVME_CTRL_LIVE);
> +		return;
> +	}
> +
> +	if (nvmf_should_reconnect(ctrl)) {
> +		dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
> +			ctrl->opts->reconnect_delay);
> +		queue_delayed_work(nvme_wq, &ctrl->connect_work,
> +				ctrl->opts->reconnect_delay * HZ);
> +	} else {
> +		dev_info(ctrl->device, "Removing controller...\n");
> +		nvme_delete_ctrl(ctrl);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
> +
> /**
>  * nvmf_register_transport() - NVMe Fabrics Library registration function.
>  * @ops:	Transport ops instance to be registered to the
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index a146cb903869..de213ab26977 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -188,6 +188,7 @@ void nvmf_unregister_transport(struct nvmf_transport_ops *ops);
> void nvmf_free_options(struct nvmf_ctrl_options *opts);
> int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
> bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
> +void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
> bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
> 		struct nvmf_ctrl_options *opts);
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index fbfa18a47bd8..f31a56d8fd73 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1064,25 +1064,6 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
> 	kfree(ctrl);
> }
> 
> -static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
> -{
> -	/* If we are resetting/deleting then do nothing */
> -	if (ctrl->ctrl.state != NVME_CTRL_CONNECTING) {
> -		WARN_ON_ONCE(ctrl->ctrl.state == NVME_CTRL_NEW ||
> -			ctrl->ctrl.state == NVME_CTRL_LIVE);
> -		return;
> -	}
> -
> -	if (nvmf_should_reconnect(&ctrl->ctrl)) {
> -		dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n",
> -			ctrl->ctrl.opts->reconnect_delay);
> -		queue_delayed_work(nvme_wq, &ctrl->ctrl.connect_work,
> -				ctrl->ctrl.opts->reconnect_delay * HZ);
> -	} else {
> -		nvme_delete_ctrl(&ctrl->ctrl);
> -	}
> -}
> -
> static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
> {
> 	int ret;
> @@ -1181,7 +1162,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
> requeue:
> 	dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
> 			ctrl->ctrl.nr_reconnects);
> -	nvme_rdma_reconnect_or_remove(ctrl);
> +	nvmf_reconnect_or_remove(&ctrl->ctrl);
> }
> 
> static void nvme_rdma_error_recovery_work(struct work_struct *work)
> @@ -1202,7 +1183,7 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
> 		return;
> 	}
> 
> -	nvme_rdma_reconnect_or_remove(ctrl);
> +	nvmf_reconnect_or_remove(&ctrl->ctrl);
> }
> 
> static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
> @@ -2265,7 +2246,7 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
> 
> out_fail:
> 	++ctrl->ctrl.nr_reconnects;
> -	nvme_rdma_reconnect_or_remove(ctrl);
> +	nvmf_reconnect_or_remove(&ctrl->ctrl);
> }
> 
> static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 3ace20e39c86..530ff76d4ac9 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1970,26 +1970,6 @@ 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)
> -{
> -	/* If we are resetting/deleting then do nothing */
> -	if (ctrl->state != NVME_CTRL_CONNECTING) {
> -		WARN_ON_ONCE(ctrl->state == NVME_CTRL_NEW ||
> -			ctrl->state == NVME_CTRL_LIVE);
> -		return;
> -	}
> -
> -	if (nvmf_should_reconnect(ctrl)) {
> -		dev_info(ctrl->device, "Reconnecting in %d seconds...\n",
> -			ctrl->opts->reconnect_delay);
> -		queue_delayed_work(nvme_wq, &ctrl->connect_work,
> -				ctrl->opts->reconnect_delay * HZ);
> -	} else {
> -		dev_info(ctrl->device, "Removing controller...\n");
> -		nvme_delete_ctrl(ctrl);
> -	}
> -}
> -
> static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
> {
> 	struct nvmf_ctrl_options *opts = ctrl->opts;
> @@ -2083,7 +2063,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);
> +	nvmf_reconnect_or_remove(ctrl);
> }
> 
> static void nvme_tcp_error_recovery_work(struct work_struct *work)
> @@ -2106,7 +2086,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
> 		return;
> 	}
> 
> -	nvme_tcp_reconnect_or_remove(ctrl);
> +	nvmf_reconnect_or_remove(ctrl);
> }
> 
> static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
> @@ -2150,7 +2130,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
> 
> out_fail:
> 	++ctrl->nr_reconnects;
> -	nvme_tcp_reconnect_or_remove(ctrl);
> +	nvmf_reconnect_or_remove(ctrl);
> }
> 
> static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
> -- 
> 2.18.1
> 
> 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

[PATCH 3/7] nvme: add err_work attribute to nvme ctrl

[Max Gurtovoy]

This structure is duplicated for RDMA/TCP fabric controllers. Move it to
common code.

FC controllers might use this attribute in the future instead of a local
ioerr_work attribute.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/rdma.c |  9 ++++-----
 drivers/nvme/host/tcp.c  | 12 +++++-------
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 81ca5dd9b7f9..f9e1ce93d61d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -344,6 +344,7 @@ struct nvme_ctrl {
 #define NVME_CTRL_FAILFAST_EXPIRED	0
 	struct nvmf_ctrl_options *opts;
 	struct delayed_work connect_work;
+	struct work_struct err_work;
 
 	struct page *discard_page;
 	unsigned long discard_page_busy;
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index f31a56d8fd73..da7f61a5fac4 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -106,7 +106,6 @@ struct nvme_rdma_ctrl {
 
 	/* other member variables */
 	struct blk_mq_tag_set	tag_set;
-	struct work_struct	err_work;
 
 	struct nvme_rdma_qe	async_event_sqe;
 
@@ -1168,7 +1167,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 static void nvme_rdma_error_recovery_work(struct work_struct *work)
 {
 	struct nvme_rdma_ctrl *ctrl = container_of(work,
-			struct nvme_rdma_ctrl, err_work);
+			struct nvme_rdma_ctrl, ctrl.err_work);
 
 	nvme_stop_keep_alive(&ctrl->ctrl);
 	nvme_rdma_teardown_io_queues(ctrl, false);
@@ -1192,7 +1191,7 @@ static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
 		return;
 
 	dev_warn(ctrl->ctrl.device, "starting error recovery\n");
-	queue_work(nvme_reset_wq, &ctrl->err_work);
+	queue_work(nvme_reset_wq, &ctrl->ctrl.err_work);
 }
 
 static void nvme_rdma_end_request(struct nvme_rdma_request *req)
@@ -2208,7 +2207,7 @@ static const struct blk_mq_ops nvme_rdma_admin_mq_ops = {
 
 static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 {
-	cancel_work_sync(&ctrl->err_work);
+	cancel_work_sync(&ctrl->ctrl.err_work);
 	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
 
 	nvme_rdma_teardown_io_queues(ctrl, shutdown);
@@ -2339,7 +2338,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 
 	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
 			nvme_rdma_reconnect_ctrl_work);
-	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
+	INIT_WORK(&ctrl->ctrl.err_work, nvme_rdma_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
 
 	ctrl->ctrl.queue_count = opts->nr_io_queues + opts->nr_write_queues +
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 530ff76d4ac9..07a9cc4f2274 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -126,7 +126,6 @@ struct nvme_tcp_ctrl {
 	struct sockaddr_storage src_addr;
 	struct nvme_ctrl	ctrl;
 
-	struct work_struct	err_work;
 	struct nvme_tcp_request async_req;
 	u32			io_queues[HCTX_MAX_TYPES];
 };
@@ -486,7 +485,7 @@ static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)
 		return;
 
 	dev_warn(ctrl->device, "starting error recovery\n");
-	queue_work(nvme_reset_wq, &to_tcp_ctrl(ctrl)->err_work);
+	queue_work(nvme_reset_wq, &ctrl->err_work);
 }
 
 static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
@@ -2068,9 +2067,8 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
 
 static void nvme_tcp_error_recovery_work(struct work_struct *work)
 {
-	struct nvme_tcp_ctrl *tcp_ctrl = container_of(work,
-				struct nvme_tcp_ctrl, err_work);
-	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
+	struct nvme_ctrl *ctrl = container_of(work,
+				struct nvme_ctrl, err_work);
 
 	nvme_stop_keep_alive(ctrl);
 	nvme_tcp_teardown_io_queues(ctrl, false);
@@ -2091,7 +2089,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
-	cancel_work_sync(&to_tcp_ctrl(ctrl)->err_work);
+	cancel_work_sync(&ctrl->err_work);
 	cancel_delayed_work_sync(&ctrl->connect_work);
 
 	nvme_tcp_teardown_io_queues(ctrl, shutdown);
@@ -2494,7 +2492,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 
 	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
 			nvme_tcp_reconnect_ctrl_work);
-	INIT_WORK(&ctrl->err_work, nvme_tcp_error_recovery_work);
+	INIT_WORK(&ctrl->ctrl.err_work, nvme_tcp_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
 
 	if (!(opts->mask & NVMF_OPT_TRSVCID)) {
-- 
2.18.1

Re: [PATCH 3/7] nvme: add err_work attribute to nvme ctrl

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 3/7] nvme: add err_work attribute to nvme ctrl

[Himanshu Madhani]

> On Oct 18, 2021, at 8:40 AM, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> 
> This structure is duplicated for RDMA/TCP fabric controllers. Move it to
> common code.
> 
> FC controllers might use this attribute in the future instead of a local
> ioerr_work attribute.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
> drivers/nvme/host/nvme.h |  1 +
> drivers/nvme/host/rdma.c |  9 ++++-----
> drivers/nvme/host/tcp.c  | 12 +++++-------
> 3 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 81ca5dd9b7f9..f9e1ce93d61d 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -344,6 +344,7 @@ struct nvme_ctrl {
> #define NVME_CTRL_FAILFAST_EXPIRED	0
> 	struct nvmf_ctrl_options *opts;
> 	struct delayed_work connect_work;
> +	struct work_struct err_work;
> 
> 	struct page *discard_page;
> 	unsigned long discard_page_busy;
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index f31a56d8fd73..da7f61a5fac4 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -106,7 +106,6 @@ struct nvme_rdma_ctrl {
> 
> 	/* other member variables */
> 	struct blk_mq_tag_set	tag_set;
> -	struct work_struct	err_work;
> 
> 	struct nvme_rdma_qe	async_event_sqe;
> 
> @@ -1168,7 +1167,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
> static void nvme_rdma_error_recovery_work(struct work_struct *work)
> {
> 	struct nvme_rdma_ctrl *ctrl = container_of(work,
> -			struct nvme_rdma_ctrl, err_work);
> +			struct nvme_rdma_ctrl, ctrl.err_work);
> 
> 	nvme_stop_keep_alive(&ctrl->ctrl);
> 	nvme_rdma_teardown_io_queues(ctrl, false);
> @@ -1192,7 +1191,7 @@ static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
> 		return;
> 
> 	dev_warn(ctrl->ctrl.device, "starting error recovery\n");
> -	queue_work(nvme_reset_wq, &ctrl->err_work);
> +	queue_work(nvme_reset_wq, &ctrl->ctrl.err_work);
> }
> 
> static void nvme_rdma_end_request(struct nvme_rdma_request *req)
> @@ -2208,7 +2207,7 @@ static const struct blk_mq_ops nvme_rdma_admin_mq_ops = {
> 
> static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
> {
> -	cancel_work_sync(&ctrl->err_work);
> +	cancel_work_sync(&ctrl->ctrl.err_work);
> 	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
> 
> 	nvme_rdma_teardown_io_queues(ctrl, shutdown);
> @@ -2339,7 +2338,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
> 
> 	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
> 			nvme_rdma_reconnect_ctrl_work);
> -	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
> +	INIT_WORK(&ctrl->ctrl.err_work, nvme_rdma_error_recovery_work);
> 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
> 
> 	ctrl->ctrl.queue_count = opts->nr_io_queues + opts->nr_write_queues +
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 530ff76d4ac9..07a9cc4f2274 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -126,7 +126,6 @@ struct nvme_tcp_ctrl {
> 	struct sockaddr_storage src_addr;
> 	struct nvme_ctrl	ctrl;
> 
> -	struct work_struct	err_work;
> 	struct nvme_tcp_request async_req;
> 	u32			io_queues[HCTX_MAX_TYPES];
> };
> @@ -486,7 +485,7 @@ static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)
> 		return;
> 
> 	dev_warn(ctrl->device, "starting error recovery\n");
> -	queue_work(nvme_reset_wq, &to_tcp_ctrl(ctrl)->err_work);
> +	queue_work(nvme_reset_wq, &ctrl->err_work);
> }
> 
> static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
> @@ -2068,9 +2067,8 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
> 
> static void nvme_tcp_error_recovery_work(struct work_struct *work)
> {
> -	struct nvme_tcp_ctrl *tcp_ctrl = container_of(work,
> -				struct nvme_tcp_ctrl, err_work);
> -	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
> +	struct nvme_ctrl *ctrl = container_of(work,
> +				struct nvme_ctrl, err_work);
> 
> 	nvme_stop_keep_alive(ctrl);
> 	nvme_tcp_teardown_io_queues(ctrl, false);
> @@ -2091,7 +2089,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
> 
> static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
> {
> -	cancel_work_sync(&to_tcp_ctrl(ctrl)->err_work);
> +	cancel_work_sync(&ctrl->err_work);
> 	cancel_delayed_work_sync(&ctrl->connect_work);
> 
> 	nvme_tcp_teardown_io_queues(ctrl, shutdown);
> @@ -2494,7 +2492,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
> 
> 	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
> 			nvme_tcp_reconnect_ctrl_work);
> -	INIT_WORK(&ctrl->err_work, nvme_tcp_error_recovery_work);
> +	INIT_WORK(&ctrl->ctrl.err_work, nvme_tcp_error_recovery_work);
> 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
> 
> 	if (!(opts->mask & NVMF_OPT_TRSVCID)) {
> -- 
> 2.18.1
> 
> 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

[PATCH 4/7] nvme-fabrics: introduce nvmf_error_recovery API

[Max Gurtovoy]

Error recovery mechanism is duplicated in RDMA and TCP transports. Move
this logic to common code.

Also update the RDMA/TCP transport drivers to use this API and remove
the duplicated code.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 10 ++++++++++
 drivers/nvme/host/fabrics.h |  1 +
 drivers/nvme/host/rdma.c    | 25 ++++++++-----------------
 drivers/nvme/host/tcp.c     | 19 +++++--------------
 4 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 4a1ef67c6fb3..2edd086fa922 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -493,6 +493,16 @@ void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
 
+void nvmf_error_recovery(struct nvme_ctrl *ctrl)
+{
+	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
+		return;
+
+	dev_warn(ctrl->device, "starting error recovery\n");
+	queue_work(nvme_reset_wq, &ctrl->err_work);
+}
+EXPORT_SYMBOL_GPL(nvmf_error_recovery);
+
 /**
  * nvmf_register_transport() - NVMe Fabrics Library registration function.
  * @ops:	Transport ops instance to be registered to the
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index de213ab26977..3d8ec7133fc8 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -189,6 +189,7 @@ void nvmf_free_options(struct nvmf_ctrl_options *opts);
 int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
 void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
+void nvmf_error_recovery(struct nvme_ctrl *ctrl);
 bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 		struct nvmf_ctrl_options *opts);
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index da7f61a5fac4..1c57e371af61 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1185,15 +1185,6 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 	nvmf_reconnect_or_remove(&ctrl->ctrl);
 }
 
-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");
-	queue_work(nvme_reset_wq, &ctrl->ctrl.err_work);
-}
-
 static void nvme_rdma_end_request(struct nvme_rdma_request *req)
 {
 	struct request *rq = blk_mq_rq_from_pdu(req);
@@ -1215,7 +1206,7 @@ static void nvme_rdma_wr_error(struct ib_cq *cq, struct ib_wc *wc,
 			     "%s for CQE 0x%p failed with status %s (%d)\n",
 			     op, wc->wr_cqe,
 			     ib_wc_status_msg(wc->status), wc->status);
-	nvme_rdma_error_recovery(ctrl);
+	nvmf_error_recovery(&ctrl->ctrl);
 }
 
 static void nvme_rdma_memreg_done(struct ib_cq *cq, struct ib_wc *wc)
@@ -1715,7 +1706,7 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue,
 		dev_err(queue->ctrl->ctrl.device,
 			"got bad command_id %#x on QP %#x\n",
 			cqe->command_id, queue->qp->qp_num);
-		nvme_rdma_error_recovery(queue->ctrl);
+		nvmf_error_recovery(&queue->ctrl->ctrl);
 		return;
 	}
 	req = blk_mq_rq_to_pdu(rq);
@@ -1729,7 +1720,7 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue,
 			dev_err(queue->ctrl->ctrl.device,
 				"Bogus remote invalidation for rkey %#x\n",
 				req->mr ? req->mr->rkey : 0);
-			nvme_rdma_error_recovery(queue->ctrl);
+			nvmf_error_recovery(&queue->ctrl->ctrl);
 		}
 	} else if (req->mr) {
 		int ret;
@@ -1739,7 +1730,7 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue,
 			dev_err(queue->ctrl->ctrl.device,
 				"Queueing INV WR for rkey %#x failed (%d)\n",
 				req->mr->rkey, ret);
-			nvme_rdma_error_recovery(queue->ctrl);
+			nvmf_error_recovery(&queue->ctrl->ctrl);
 		}
 		/* the local invalidation completion will end the request */
 		return;
@@ -1766,7 +1757,7 @@ static void nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)
 	if (unlikely(wc->byte_len < len)) {
 		dev_err(queue->ctrl->ctrl.device,
 			"Unexpected nvme completion length(%d)\n", wc->byte_len);
-		nvme_rdma_error_recovery(queue->ctrl);
+		nvmf_error_recovery(&queue->ctrl->ctrl);
 		return;
 	}
 
@@ -1936,7 +1927,7 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 	case RDMA_CM_EVENT_TIMEWAIT_EXIT:
 		dev_dbg(queue->ctrl->ctrl.device,
 			"disconnect received - connection closed\n");
-		nvme_rdma_error_recovery(queue->ctrl);
+		nvmf_error_recovery(&queue->ctrl->ctrl);
 		break;
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 		/* device removal is handled via the ib_client API */
@@ -1944,7 +1935,7 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 	default:
 		dev_err(queue->ctrl->ctrl.device,
 			"Unexpected RDMA CM event (%d)\n", ev->event);
-		nvme_rdma_error_recovery(queue->ctrl);
+		nvmf_error_recovery(&queue->ctrl->ctrl);
 		break;
 	}
 
@@ -2000,7 +1991,7 @@ nvme_rdma_timeout(struct request *rq, bool reserved)
 	 * LIVE state should trigger the normal error recovery which will
 	 * handle completing this request.
 	 */
-	nvme_rdma_error_recovery(ctrl);
+	nvmf_error_recovery(&ctrl->ctrl);
 	return BLK_EH_RESET_TIMER;
 }
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 07a9cc4f2274..fe1f2fec457b 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -479,15 +479,6 @@ static void nvme_tcp_init_recv_ctx(struct nvme_tcp_queue *queue)
 	queue->ddgst_remaining = 0;
 }
 
-static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)
-{
-	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
-		return;
-
-	dev_warn(ctrl->device, "starting error recovery\n");
-	queue_work(nvme_reset_wq, &ctrl->err_work);
-}
-
 static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
 		struct nvme_completion *cqe)
 {
@@ -499,7 +490,7 @@ static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
 		dev_err(queue->ctrl->ctrl.device,
 			"got bad cqe.command_id %#x on queue %d\n",
 			cqe->command_id, nvme_tcp_queue_id(queue));
-		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
+		nvmf_error_recovery(&queue->ctrl->ctrl);
 		return -EINVAL;
 	}
 
@@ -541,7 +532,7 @@ static int nvme_tcp_handle_c2h_data(struct nvme_tcp_queue *queue,
 		dev_err(queue->ctrl->ctrl.device,
 			"queue %d tag %#x SUCCESS set but not last PDU\n",
 			nvme_tcp_queue_id(queue), rq->tag);
-		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
+		nvmf_error_recovery(&queue->ctrl->ctrl);
 		return -EPROTO;
 	}
 
@@ -850,7 +841,7 @@ static int nvme_tcp_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
 			dev_err(queue->ctrl->ctrl.device,
 				"receive failed:  %d\n", result);
 			queue->rd_enabled = false;
-			nvme_tcp_error_recovery(&queue->ctrl->ctrl);
+			nvmf_error_recovery(&queue->ctrl->ctrl);
 			return result;
 		}
 	}
@@ -898,7 +889,7 @@ static void nvme_tcp_state_change(struct sock *sk)
 	case TCP_LAST_ACK:
 	case TCP_FIN_WAIT1:
 	case TCP_FIN_WAIT2:
-		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
+		nvmf_error_recovery(&queue->ctrl->ctrl);
 		break;
 	default:
 		dev_info(queue->ctrl->ctrl.device,
@@ -2252,7 +2243,7 @@ nvme_tcp_timeout(struct request *rq, bool reserved)
 	 * LIVE state should trigger the normal error recovery which will
 	 * handle completing this request.
 	 */
-	nvme_tcp_error_recovery(ctrl);
+	nvmf_error_recovery(ctrl);
 	return BLK_EH_RESET_TIMER;
 }
 
-- 
2.18.1

Re: [PATCH 4/7] nvme-fabrics: introduce nvmf_error_recovery API

[Hannes Reinecke]

On 10/18/21 3:40 PM, Max Gurtovoy wrote:
> Error recovery mechanism is duplicated in RDMA and TCP transports. Move
> this logic to common code.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/fabrics.c | 10 ++++++++++
>   drivers/nvme/host/fabrics.h |  1 +
>   drivers/nvme/host/rdma.c    | 25 ++++++++-----------------
>   drivers/nvme/host/tcp.c     | 19 +++++--------------
>   4 files changed, 24 insertions(+), 31 deletions(-)
> 

_Technically_ we shouldn't start reconnecting for command errors; 
reconnection only should start once KATO expires.

But that might be a topic for a different discussion.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 4/7] nvme-fabrics: introduce nvmf_error_recovery API

[Himanshu Madhani]

> On Oct 18, 2021, at 8:40 AM, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> 
> Error recovery mechanism is duplicated in RDMA and TCP transports. Move
> this logic to common code.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
> drivers/nvme/host/fabrics.c | 10 ++++++++++
> drivers/nvme/host/fabrics.h |  1 +
> drivers/nvme/host/rdma.c    | 25 ++++++++-----------------
> drivers/nvme/host/tcp.c     | 19 +++++--------------
> 4 files changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 4a1ef67c6fb3..2edd086fa922 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -493,6 +493,16 @@ void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
> }
> EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
> 
> +void nvmf_error_recovery(struct nvme_ctrl *ctrl)
> +{
> +	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> +		return;
> +
> +	dev_warn(ctrl->device, "starting error recovery\n");
> +	queue_work(nvme_reset_wq, &ctrl->err_work);
> +}
> +EXPORT_SYMBOL_GPL(nvmf_error_recovery);
> +
> /**
>  * nvmf_register_transport() - NVMe Fabrics Library registration function.
>  * @ops:	Transport ops instance to be registered to the
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index de213ab26977..3d8ec7133fc8 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -189,6 +189,7 @@ void nvmf_free_options(struct nvmf_ctrl_options *opts);
> int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
> bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
> void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
> +void nvmf_error_recovery(struct nvme_ctrl *ctrl);
> bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
> 		struct nvmf_ctrl_options *opts);
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index da7f61a5fac4..1c57e371af61 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1185,15 +1185,6 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
> 	nvmf_reconnect_or_remove(&ctrl->ctrl);
> }
> 
> -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");
> -	queue_work(nvme_reset_wq, &ctrl->ctrl.err_work);
> -}
> -
> static void nvme_rdma_end_request(struct nvme_rdma_request *req)
> {
> 	struct request *rq = blk_mq_rq_from_pdu(req);
> @@ -1215,7 +1206,7 @@ static void nvme_rdma_wr_error(struct ib_cq *cq, struct ib_wc *wc,
> 			     "%s for CQE 0x%p failed with status %s (%d)\n",
> 			     op, wc->wr_cqe,
> 			     ib_wc_status_msg(wc->status), wc->status);
> -	nvme_rdma_error_recovery(ctrl);
> +	nvmf_error_recovery(&ctrl->ctrl);
> }
> 
> static void nvme_rdma_memreg_done(struct ib_cq *cq, struct ib_wc *wc)
> @@ -1715,7 +1706,7 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue,
> 		dev_err(queue->ctrl->ctrl.device,
> 			"got bad command_id %#x on QP %#x\n",
> 			cqe->command_id, queue->qp->qp_num);
> -		nvme_rdma_error_recovery(queue->ctrl);
> +		nvmf_error_recovery(&queue->ctrl->ctrl);
> 		return;
> 	}
> 	req = blk_mq_rq_to_pdu(rq);
> @@ -1729,7 +1720,7 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue,
> 			dev_err(queue->ctrl->ctrl.device,
> 				"Bogus remote invalidation for rkey %#x\n",
> 				req->mr ? req->mr->rkey : 0);
> -			nvme_rdma_error_recovery(queue->ctrl);
> +			nvmf_error_recovery(&queue->ctrl->ctrl);
> 		}
> 	} else if (req->mr) {
> 		int ret;
> @@ -1739,7 +1730,7 @@ static void nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue,
> 			dev_err(queue->ctrl->ctrl.device,
> 				"Queueing INV WR for rkey %#x failed (%d)\n",
> 				req->mr->rkey, ret);
> -			nvme_rdma_error_recovery(queue->ctrl);
> +			nvmf_error_recovery(&queue->ctrl->ctrl);
> 		}
> 		/* the local invalidation completion will end the request */
> 		return;
> @@ -1766,7 +1757,7 @@ static void nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)
> 	if (unlikely(wc->byte_len < len)) {
> 		dev_err(queue->ctrl->ctrl.device,
> 			"Unexpected nvme completion length(%d)\n", wc->byte_len);
> -		nvme_rdma_error_recovery(queue->ctrl);
> +		nvmf_error_recovery(&queue->ctrl->ctrl);
> 		return;
> 	}
> 
> @@ -1936,7 +1927,7 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
> 	case RDMA_CM_EVENT_TIMEWAIT_EXIT:
> 		dev_dbg(queue->ctrl->ctrl.device,
> 			"disconnect received - connection closed\n");
> -		nvme_rdma_error_recovery(queue->ctrl);
> +		nvmf_error_recovery(&queue->ctrl->ctrl);
> 		break;
> 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
> 		/* device removal is handled via the ib_client API */
> @@ -1944,7 +1935,7 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
> 	default:
> 		dev_err(queue->ctrl->ctrl.device,
> 			"Unexpected RDMA CM event (%d)\n", ev->event);
> -		nvme_rdma_error_recovery(queue->ctrl);
> +		nvmf_error_recovery(&queue->ctrl->ctrl);
> 		break;
> 	}
> 
> @@ -2000,7 +1991,7 @@ nvme_rdma_timeout(struct request *rq, bool reserved)
> 	 * LIVE state should trigger the normal error recovery which will
> 	 * handle completing this request.
> 	 */
> -	nvme_rdma_error_recovery(ctrl);
> +	nvmf_error_recovery(&ctrl->ctrl);
> 	return BLK_EH_RESET_TIMER;
> }
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 07a9cc4f2274..fe1f2fec457b 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -479,15 +479,6 @@ static void nvme_tcp_init_recv_ctx(struct nvme_tcp_queue *queue)
> 	queue->ddgst_remaining = 0;
> }
> 
> -static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)
> -{
> -	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> -		return;
> -
> -	dev_warn(ctrl->device, "starting error recovery\n");
> -	queue_work(nvme_reset_wq, &ctrl->err_work);
> -}
> -
> static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
> 		struct nvme_completion *cqe)
> {
> @@ -499,7 +490,7 @@ static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
> 		dev_err(queue->ctrl->ctrl.device,
> 			"got bad cqe.command_id %#x on queue %d\n",
> 			cqe->command_id, nvme_tcp_queue_id(queue));
> -		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
> +		nvmf_error_recovery(&queue->ctrl->ctrl);
> 		return -EINVAL;
> 	}
> 
> @@ -541,7 +532,7 @@ static int nvme_tcp_handle_c2h_data(struct nvme_tcp_queue *queue,
> 		dev_err(queue->ctrl->ctrl.device,
> 			"queue %d tag %#x SUCCESS set but not last PDU\n",
> 			nvme_tcp_queue_id(queue), rq->tag);
> -		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
> +		nvmf_error_recovery(&queue->ctrl->ctrl);
> 		return -EPROTO;
> 	}
> 
> @@ -850,7 +841,7 @@ static int nvme_tcp_recv_skb(read_descriptor_t *desc, struct sk_buff *skb,
> 			dev_err(queue->ctrl->ctrl.device,
> 				"receive failed:  %d\n", result);
> 			queue->rd_enabled = false;
> -			nvme_tcp_error_recovery(&queue->ctrl->ctrl);
> +			nvmf_error_recovery(&queue->ctrl->ctrl);
> 			return result;
> 		}
> 	}
> @@ -898,7 +889,7 @@ static void nvme_tcp_state_change(struct sock *sk)
> 	case TCP_LAST_ACK:
> 	case TCP_FIN_WAIT1:
> 	case TCP_FIN_WAIT2:
> -		nvme_tcp_error_recovery(&queue->ctrl->ctrl);
> +		nvmf_error_recovery(&queue->ctrl->ctrl);
> 		break;
> 	default:
> 		dev_info(queue->ctrl->ctrl.device,
> @@ -2252,7 +2243,7 @@ nvme_tcp_timeout(struct request *rq, bool reserved)
> 	 * LIVE state should trigger the normal error recovery which will
> 	 * handle completing this request.
> 	 */
> -	nvme_tcp_error_recovery(ctrl);
> +	nvmf_error_recovery(ctrl);
> 	return BLK_EH_RESET_TIMER;
> }
> 
> -- 
> 2.18.1
> 
> 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	 Oracle Linux Engineering

[PATCH 5/7] nvme/nvme-fabrics: introduce nvmf_error_recovery_work API

[Max Gurtovoy]

Error recovery work is duplicated in RDMA and TCP transports. Move this
logic to common code. For that, introduce 2 new ctrl ops to teardown IO
and admin queue.

Also update the RDMA/TCP transport drivers to use this API and remove
the duplicated code.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 23 +++++++++++++++
 drivers/nvme/host/fabrics.h |  1 +
 drivers/nvme/host/nvme.h    |  4 +++
 drivers/nvme/host/rdma.c    | 56 ++++++++++++++++---------------------
 drivers/nvme/host/tcp.c     | 56 +++++++++++++++----------------------
 5 files changed, 75 insertions(+), 65 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 2edd086fa922..544195369c97 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -493,6 +493,29 @@ void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
 
+void nvmf_error_recovery_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl = container_of(work,
+				struct nvme_ctrl, err_work);
+
+	nvme_stop_keep_alive(ctrl);
+	ctrl->ops->teardown_ctrl_io_queues(ctrl);
+	/* unquiesce to fail fast pending requests */
+	nvme_start_queues(ctrl);
+	ctrl->ops->teardown_ctrl_admin_queue(ctrl);
+	blk_mq_unquiesce_queue(ctrl->admin_q);
+
+	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
+		/* state change failure is ok if we started ctrl delete */
+		WARN_ON_ONCE(ctrl->state != NVME_CTRL_DELETING &&
+			     ctrl->state != NVME_CTRL_DELETING_NOIO);
+		return;
+	}
+
+	nvmf_reconnect_or_remove(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvmf_error_recovery_work);
+
 void nvmf_error_recovery(struct nvme_ctrl *ctrl)
 {
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 3d8ec7133fc8..8655eff74ed0 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -190,6 +190,7 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
 void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
 void nvmf_error_recovery(struct nvme_ctrl *ctrl);
+void nvmf_error_recovery_work(struct work_struct *work);
 bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 		struct nvmf_ctrl_options *opts);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index f9e1ce93d61d..1573edf6e97f 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -493,6 +493,10 @@ struct nvme_ctrl_ops {
 	void (*submit_async_event)(struct nvme_ctrl *ctrl);
 	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
 	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
+
+	/* Fabrics only */
+	void (*teardown_ctrl_io_queues)(struct nvme_ctrl *ctrl);
+	void (*teardown_ctrl_admin_queue)(struct nvme_ctrl *ctrl);
 };
 
 /*
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 1c57e371af61..f4e4ebf673d2 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1031,6 +1031,11 @@ static void nvme_rdma_teardown_admin_queue(struct nvme_rdma_ctrl *ctrl,
 	nvme_rdma_destroy_admin_queue(ctrl, remove);
 }
 
+static void _nvme_rdma_teardown_admin_queue(struct nvme_ctrl *ctrl)
+{
+	nvme_rdma_teardown_admin_queue(to_rdma_ctrl(ctrl), false);
+}
+
 static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
 		bool remove)
 {
@@ -1046,6 +1051,11 @@ static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
 	}
 }
 
+static void _nvme_rdma_teardown_io_queues(struct nvme_ctrl *ctrl)
+{
+	nvme_rdma_teardown_io_queues(to_rdma_ctrl(ctrl), false);
+}
+
 static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
@@ -1164,27 +1174,6 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	nvmf_reconnect_or_remove(&ctrl->ctrl);
 }
 
-static void nvme_rdma_error_recovery_work(struct work_struct *work)
-{
-	struct nvme_rdma_ctrl *ctrl = container_of(work,
-			struct nvme_rdma_ctrl, ctrl.err_work);
-
-	nvme_stop_keep_alive(&ctrl->ctrl);
-	nvme_rdma_teardown_io_queues(ctrl, false);
-	nvme_start_queues(&ctrl->ctrl);
-	nvme_rdma_teardown_admin_queue(ctrl, false);
-	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
-
-	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
-		/* state change failure is ok if we started ctrl delete */
-		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING &&
-			     ctrl->ctrl.state != NVME_CTRL_DELETING_NOIO);
-		return;
-	}
-
-	nvmf_reconnect_or_remove(&ctrl->ctrl);
-}
-
 static void nvme_rdma_end_request(struct nvme_rdma_request *req)
 {
 	struct request *rq = blk_mq_rq_from_pdu(req);
@@ -2240,16 +2229,19 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 }
 
 static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
-	.name			= "rdma",
-	.module			= THIS_MODULE,
-	.flags			= NVME_F_FABRICS | NVME_F_METADATA_SUPPORTED,
-	.reg_read32		= nvmf_reg_read32,
-	.reg_read64		= nvmf_reg_read64,
-	.reg_write32		= nvmf_reg_write32,
-	.free_ctrl		= nvme_rdma_free_ctrl,
-	.submit_async_event	= nvme_rdma_submit_async_event,
-	.delete_ctrl		= nvme_rdma_delete_ctrl,
-	.get_address		= nvmf_get_address,
+	.name				= "rdma",
+	.module				= THIS_MODULE,
+	.flags				= NVME_F_FABRICS |
+					  NVME_F_METADATA_SUPPORTED,
+	.reg_read32			= nvmf_reg_read32,
+	.reg_read64			= nvmf_reg_read64,
+	.reg_write32			= nvmf_reg_write32,
+	.free_ctrl			= nvme_rdma_free_ctrl,
+	.submit_async_event		= nvme_rdma_submit_async_event,
+	.delete_ctrl			= nvme_rdma_delete_ctrl,
+	.get_address			= nvmf_get_address,
+	.teardown_ctrl_io_queues	= _nvme_rdma_teardown_io_queues,
+	.teardown_ctrl_admin_queue	= _nvme_rdma_teardown_admin_queue,
 };
 
 /*
@@ -2329,7 +2321,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 
 	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
 			nvme_rdma_reconnect_ctrl_work);
-	INIT_WORK(&ctrl->ctrl.err_work, nvme_rdma_error_recovery_work);
+	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
 
 	ctrl->ctrl.queue_count = opts->nr_io_queues + opts->nr_write_queues +
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index fe1f2fec457b..14bd16b8d99f 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1944,6 +1944,11 @@ static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
 	nvme_tcp_destroy_admin_queue(ctrl, remove);
 }
 
+static void _nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl)
+{
+	nvme_tcp_teardown_admin_queue(ctrl, false);
+}
+
 static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
 		bool remove)
 {
@@ -1960,6 +1965,11 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
 	nvme_tcp_destroy_io_queues(ctrl, remove);
 }
 
+static void _nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
+{
+	nvme_tcp_teardown_io_queues(ctrl, false);
+}
+
 static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 {
 	struct nvmf_ctrl_options *opts = ctrl->opts;
@@ -2056,28 +2066,6 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
 	nvmf_reconnect_or_remove(ctrl);
 }
 
-static void nvme_tcp_error_recovery_work(struct work_struct *work)
-{
-	struct nvme_ctrl *ctrl = container_of(work,
-				struct nvme_ctrl, err_work);
-
-	nvme_stop_keep_alive(ctrl);
-	nvme_tcp_teardown_io_queues(ctrl, false);
-	/* unquiesce to fail fast pending requests */
-	nvme_start_queues(ctrl);
-	nvme_tcp_teardown_admin_queue(ctrl, false);
-	blk_mq_unquiesce_queue(ctrl->admin_q);
-
-	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
-		/* state change failure is ok if we started ctrl delete */
-		WARN_ON_ONCE(ctrl->state != NVME_CTRL_DELETING &&
-			     ctrl->state != NVME_CTRL_DELETING_NOIO);
-		return;
-	}
-
-	nvmf_reconnect_or_remove(ctrl);
-}
-
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
 	cancel_work_sync(&ctrl->err_work);
@@ -2435,16 +2423,18 @@ static const struct blk_mq_ops nvme_tcp_admin_mq_ops = {
 };
 
 static const struct nvme_ctrl_ops nvme_tcp_ctrl_ops = {
-	.name			= "tcp",
-	.module			= THIS_MODULE,
-	.flags			= NVME_F_FABRICS,
-	.reg_read32		= nvmf_reg_read32,
-	.reg_read64		= nvmf_reg_read64,
-	.reg_write32		= nvmf_reg_write32,
-	.free_ctrl		= nvme_tcp_free_ctrl,
-	.submit_async_event	= nvme_tcp_submit_async_event,
-	.delete_ctrl		= nvme_tcp_delete_ctrl,
-	.get_address		= nvmf_get_address,
+	.name				= "tcp",
+	.module				= THIS_MODULE,
+	.flags				= NVME_F_FABRICS,
+	.reg_read32			= nvmf_reg_read32,
+	.reg_read64			= nvmf_reg_read64,
+	.reg_write32			= nvmf_reg_write32,
+	.free_ctrl			= nvme_tcp_free_ctrl,
+	.submit_async_event		= nvme_tcp_submit_async_event,
+	.delete_ctrl			= nvme_tcp_delete_ctrl,
+	.get_address			= nvmf_get_address,
+	.teardown_ctrl_io_queues	= _nvme_tcp_teardown_io_queues,
+	.teardown_ctrl_admin_queue	= _nvme_tcp_teardown_admin_queue,
 };
 
 static bool
@@ -2483,7 +2473,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 
 	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
 			nvme_tcp_reconnect_ctrl_work);
-	INIT_WORK(&ctrl->ctrl.err_work, nvme_tcp_error_recovery_work);
+	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
 
 	if (!(opts->mask & NVMF_OPT_TRSVCID)) {
-- 
2.18.1

Re: [PATCH 5/7] nvme/nvme-fabrics: introduce nvmf_error_recovery_work API

[Chaitanya Kulkarni]

On 10/18/2021 6:40 AM, Max Gurtovoy wrote:
> Error recovery work is duplicated in RDMA and TCP transports. Move this
> logic to common code. For that, introduce 2 new ctrl ops to teardown IO
> and admin queue.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---


I didn't like the odd convention"_nvme", since we are under time crunch :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH 5/7] nvme/nvme-fabrics: introduce nvmf_error_recovery_work API

[Sagi Grimberg]

On 10/18/21 4:40 PM, Max Gurtovoy wrote:
> Error recovery work is duplicated in RDMA and TCP transports. Move this
> logic to common code. For that, introduce 2 new ctrl ops to teardown IO
> and admin queue.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/fabrics.c | 23 +++++++++++++++
>   drivers/nvme/host/fabrics.h |  1 +
>   drivers/nvme/host/nvme.h    |  4 +++
>   drivers/nvme/host/rdma.c    | 56 ++++++++++++++++---------------------
>   drivers/nvme/host/tcp.c     | 56 +++++++++++++++----------------------
>   5 files changed, 75 insertions(+), 65 deletions(-)

Diffstat dry stats are not in your favor...

> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 2edd086fa922..544195369c97 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -493,6 +493,29 @@ void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
>   }
>   EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
>   
> +void nvmf_error_recovery_work(struct work_struct *work)
> +{
> +	struct nvme_ctrl *ctrl = container_of(work,
> +				struct nvme_ctrl, err_work);
> +
> +	nvme_stop_keep_alive(ctrl);
> +	ctrl->ops->teardown_ctrl_io_queues(ctrl);
> +	/* unquiesce to fail fast pending requests */
> +	nvme_start_queues(ctrl);
> +	ctrl->ops->teardown_ctrl_admin_queue(ctrl);
> +	blk_mq_unquiesce_queue(ctrl->admin_q);
> +
> +	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
> +		/* state change failure is ok if we started ctrl delete */
> +		WARN_ON_ONCE(ctrl->state != NVME_CTRL_DELETING &&
> +			     ctrl->state != NVME_CTRL_DELETING_NOIO);
> +		return;
> +	}
> +
> +	nvmf_reconnect_or_remove(ctrl);

We need James to provide feedback how can this be useful for FC.

> +}
> +EXPORT_SYMBOL_GPL(nvmf_error_recovery_work);
> +
>   void nvmf_error_recovery(struct nvme_ctrl *ctrl)
>   {
>   	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 3d8ec7133fc8..8655eff74ed0 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -190,6 +190,7 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>   void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
>   void nvmf_error_recovery(struct nvme_ctrl *ctrl);
> +void nvmf_error_recovery_work(struct work_struct *work);
>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>   		struct nvmf_ctrl_options *opts);
>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index f9e1ce93d61d..1573edf6e97f 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -493,6 +493,10 @@ struct nvme_ctrl_ops {
>   	void (*submit_async_event)(struct nvme_ctrl *ctrl);
>   	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
>   	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
> +
> +	/* Fabrics only */
> +	void (*teardown_ctrl_io_queues)(struct nvme_ctrl *ctrl);
> +	void (*teardown_ctrl_admin_queue)(struct nvme_ctrl *ctrl);

This becomes strange that we have teardown without a setup callback...

>   };
>   
>   /*
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 1c57e371af61..f4e4ebf673d2 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1031,6 +1031,11 @@ static void nvme_rdma_teardown_admin_queue(struct nvme_rdma_ctrl *ctrl,
>   	nvme_rdma_destroy_admin_queue(ctrl, remove);
>   }
>   
> +static void _nvme_rdma_teardown_admin_queue(struct nvme_ctrl *ctrl)
> +{
> +	nvme_rdma_teardown_admin_queue(to_rdma_ctrl(ctrl), false);
> +}
> +
>   static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
>   		bool remove)
>   {
> @@ -1046,6 +1051,11 @@ static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
>   	}
>   }
>   
> +static void _nvme_rdma_teardown_io_queues(struct nvme_ctrl *ctrl)
> +{
> +	nvme_rdma_teardown_io_queues(to_rdma_ctrl(ctrl), false);
> +}
> +
>   static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
>   {
>   	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
> @@ -1164,27 +1174,6 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
>   	nvmf_reconnect_or_remove(&ctrl->ctrl);
>   }
>   
> -static void nvme_rdma_error_recovery_work(struct work_struct *work)
> -{
> -	struct nvme_rdma_ctrl *ctrl = container_of(work,
> -			struct nvme_rdma_ctrl, ctrl.err_work);
> -
> -	nvme_stop_keep_alive(&ctrl->ctrl);
> -	nvme_rdma_teardown_io_queues(ctrl, false);
> -	nvme_start_queues(&ctrl->ctrl);
> -	nvme_rdma_teardown_admin_queue(ctrl, false);
> -	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
> -
> -	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
> -		/* state change failure is ok if we started ctrl delete */
> -		WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING &&
> -			     ctrl->ctrl.state != NVME_CTRL_DELETING_NOIO);
> -		return;
> -	}
> -
> -	nvmf_reconnect_or_remove(&ctrl->ctrl);
> -}
> -
>   static void nvme_rdma_end_request(struct nvme_rdma_request *req)
>   {
>   	struct request *rq = blk_mq_rq_from_pdu(req);
> @@ -2240,16 +2229,19 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
>   }
>   
>   static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
> -	.name			= "rdma",
> -	.module			= THIS_MODULE,
> -	.flags			= NVME_F_FABRICS | NVME_F_METADATA_SUPPORTED,
> -	.reg_read32		= nvmf_reg_read32,
> -	.reg_read64		= nvmf_reg_read64,
> -	.reg_write32		= nvmf_reg_write32,
> -	.free_ctrl		= nvme_rdma_free_ctrl,
> -	.submit_async_event	= nvme_rdma_submit_async_event,
> -	.delete_ctrl		= nvme_rdma_delete_ctrl,
> -	.get_address		= nvmf_get_address,
> +	.name				= "rdma",
> +	.module				= THIS_MODULE,
> +	.flags				= NVME_F_FABRICS |
> +					  NVME_F_METADATA_SUPPORTED,
> +	.reg_read32			= nvmf_reg_read32,
> +	.reg_read64			= nvmf_reg_read64,
> +	.reg_write32			= nvmf_reg_write32,
> +	.free_ctrl			= nvme_rdma_free_ctrl,
> +	.submit_async_event		= nvme_rdma_submit_async_event,
> +	.delete_ctrl			= nvme_rdma_delete_ctrl,
> +	.get_address			= nvmf_get_address,
> +	.teardown_ctrl_io_queues	= _nvme_rdma_teardown_io_queues,
> +	.teardown_ctrl_admin_queue	= _nvme_rdma_teardown_admin_queue,
>   };
>   
>   /*
> @@ -2329,7 +2321,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>   
>   	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
>   			nvme_rdma_reconnect_ctrl_work);
> -	INIT_WORK(&ctrl->ctrl.err_work, nvme_rdma_error_recovery_work);
> +	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);

This initialization needs to move to the core or fabrics lib.

Re: [PATCH 5/7] nvme/nvme-fabrics: introduce nvmf_error_recovery_work API

[Max Gurtovoy]

On 10/19/2021 3:43 PM, Sagi Grimberg wrote:
>
>
> On 10/18/21 4:40 PM, Max Gurtovoy wrote:
>> Error recovery work is duplicated in RDMA and TCP transports. Move this
>> logic to common code. For that, introduce 2 new ctrl ops to teardown IO
>> and admin queue.
>>
>> Also update the RDMA/TCP transport drivers to use this API and remove
>> the duplicated code.
>>
>> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   drivers/nvme/host/fabrics.c | 23 +++++++++++++++
>>   drivers/nvme/host/fabrics.h |  1 +
>>   drivers/nvme/host/nvme.h    |  4 +++
>>   drivers/nvme/host/rdma.c    | 56 ++++++++++++++++---------------------
>>   drivers/nvme/host/tcp.c     | 56 +++++++++++++++----------------------
>>   5 files changed, 75 insertions(+), 65 deletions(-)
>
> Diffstat dry stats are not in your favor...
>
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 2edd086fa922..544195369c97 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -493,6 +493,29 @@ void nvmf_reconnect_or_remove(struct nvme_ctrl 
>> *ctrl)
>>   }
>>   EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
>>   +void nvmf_error_recovery_work(struct work_struct *work)
>> +{
>> +    struct nvme_ctrl *ctrl = container_of(work,
>> +                struct nvme_ctrl, err_work);
>> +
>> +    nvme_stop_keep_alive(ctrl);
>> +    ctrl->ops->teardown_ctrl_io_queues(ctrl);
>> +    /* unquiesce to fail fast pending requests */
>> +    nvme_start_queues(ctrl);
>> +    ctrl->ops->teardown_ctrl_admin_queue(ctrl);
>> +    blk_mq_unquiesce_queue(ctrl->admin_q);
>> +
>> +    if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
>> +        /* state change failure is ok if we started ctrl delete */
>> +        WARN_ON_ONCE(ctrl->state != NVME_CTRL_DELETING &&
>> +                 ctrl->state != NVME_CTRL_DELETING_NOIO);
>> +        return;
>> +    }
>> +
>> +    nvmf_reconnect_or_remove(ctrl);
>
> We need James to provide feedback how can this be useful for FC.
>
>> +}
>> +EXPORT_SYMBOL_GPL(nvmf_error_recovery_work);
>> +
>>   void nvmf_error_recovery(struct nvme_ctrl *ctrl)
>>   {
>>       if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index 3d8ec7133fc8..8655eff74ed0 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -190,6 +190,7 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char 
>> *buf, int size);
>>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>>   void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
>>   void nvmf_error_recovery(struct nvme_ctrl *ctrl);
>> +void nvmf_error_recovery_work(struct work_struct *work);
>>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>>           struct nvmf_ctrl_options *opts);
>>   diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index f9e1ce93d61d..1573edf6e97f 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -493,6 +493,10 @@ struct nvme_ctrl_ops {
>>       void (*submit_async_event)(struct nvme_ctrl *ctrl);
>>       void (*delete_ctrl)(struct nvme_ctrl *ctrl);
>>       int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
>> +
>> +    /* Fabrics only */
>> +    void (*teardown_ctrl_io_queues)(struct nvme_ctrl *ctrl);
>> +    void (*teardown_ctrl_admin_queue)(struct nvme_ctrl *ctrl);
>
> This becomes strange that we have teardown without a setup callback...

We can do it incrementally.

It's not the first time we do it :)

>
>>   };
>>     /*
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index 1c57e371af61..f4e4ebf673d2 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1031,6 +1031,11 @@ static void 
>> nvme_rdma_teardown_admin_queue(struct nvme_rdma_ctrl *ctrl,
>>       nvme_rdma_destroy_admin_queue(ctrl, remove);
>>   }
>>   +static void _nvme_rdma_teardown_admin_queue(struct nvme_ctrl *ctrl)
>> +{
>> +    nvme_rdma_teardown_admin_queue(to_rdma_ctrl(ctrl), false);
>> +}
>> +
>>   static void nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
>>           bool remove)
>>   {
>> @@ -1046,6 +1051,11 @@ static void 
>> nvme_rdma_teardown_io_queues(struct nvme_rdma_ctrl *ctrl,
>>       }
>>   }
>>   +static void _nvme_rdma_teardown_io_queues(struct nvme_ctrl *ctrl)
>> +{
>> +    nvme_rdma_teardown_io_queues(to_rdma_ctrl(ctrl), false);
>> +}
>> +
>>   static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
>>   {
>>       struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
>> @@ -1164,27 +1174,6 @@ static void 
>> nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
>>       nvmf_reconnect_or_remove(&ctrl->ctrl);
>>   }
>>   -static void nvme_rdma_error_recovery_work(struct work_struct *work)
>> -{
>> -    struct nvme_rdma_ctrl *ctrl = container_of(work,
>> -            struct nvme_rdma_ctrl, ctrl.err_work);
>> -
>> -    nvme_stop_keep_alive(&ctrl->ctrl);
>> -    nvme_rdma_teardown_io_queues(ctrl, false);
>> -    nvme_start_queues(&ctrl->ctrl);
>> -    nvme_rdma_teardown_admin_queue(ctrl, false);
>> -    blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
>> -
>> -    if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
>> -        /* state change failure is ok if we started ctrl delete */
>> -        WARN_ON_ONCE(ctrl->ctrl.state != NVME_CTRL_DELETING &&
>> -                 ctrl->ctrl.state != NVME_CTRL_DELETING_NOIO);
>> -        return;
>> -    }
>> -
>> -    nvmf_reconnect_or_remove(&ctrl->ctrl);
>> -}
>> -
>>   static void nvme_rdma_end_request(struct nvme_rdma_request *req)
>>   {
>>       struct request *rq = blk_mq_rq_from_pdu(req);
>> @@ -2240,16 +2229,19 @@ static void nvme_rdma_reset_ctrl_work(struct 
>> work_struct *work)
>>   }
>>     static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
>> -    .name            = "rdma",
>> -    .module            = THIS_MODULE,
>> -    .flags            = NVME_F_FABRICS | NVME_F_METADATA_SUPPORTED,
>> -    .reg_read32        = nvmf_reg_read32,
>> -    .reg_read64        = nvmf_reg_read64,
>> -    .reg_write32        = nvmf_reg_write32,
>> -    .free_ctrl        = nvme_rdma_free_ctrl,
>> -    .submit_async_event    = nvme_rdma_submit_async_event,
>> -    .delete_ctrl        = nvme_rdma_delete_ctrl,
>> -    .get_address        = nvmf_get_address,
>> +    .name                = "rdma",
>> +    .module                = THIS_MODULE,
>> +    .flags                = NVME_F_FABRICS |
>> +                      NVME_F_METADATA_SUPPORTED,
>> +    .reg_read32            = nvmf_reg_read32,
>> +    .reg_read64            = nvmf_reg_read64,
>> +    .reg_write32            = nvmf_reg_write32,
>> +    .free_ctrl            = nvme_rdma_free_ctrl,
>> +    .submit_async_event        = nvme_rdma_submit_async_event,
>> +    .delete_ctrl            = nvme_rdma_delete_ctrl,
>> +    .get_address            = nvmf_get_address,
>> +    .teardown_ctrl_io_queues    = _nvme_rdma_teardown_io_queues,
>> +    .teardown_ctrl_admin_queue    = _nvme_rdma_teardown_admin_queue,
>>   };
>>     /*
>> @@ -2329,7 +2321,7 @@ static struct nvme_ctrl 
>> *nvme_rdma_create_ctrl(struct device *dev,
>>         INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
>>               nvme_rdma_reconnect_ctrl_work);
>> -    INIT_WORK(&ctrl->ctrl.err_work, nvme_rdma_error_recovery_work);
>> +    INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
>
> This initialization needs to move to the core or fabrics lib.

It's done in the next patches.

Re: [PATCH 5/7] nvme/nvme-fabrics: introduce nvmf_error_recovery_work API

[Hannes Reinecke]

On 10/18/21 3:40 PM, Max Gurtovoy wrote:
> Error recovery work is duplicated in RDMA and TCP transports. Move this
> logic to common code. For that, introduce 2 new ctrl ops to teardown IO
> and admin queue.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/fabrics.c | 23 +++++++++++++++
>   drivers/nvme/host/fabrics.h |  1 +
>   drivers/nvme/host/nvme.h    |  4 +++
>   drivers/nvme/host/rdma.c    | 56 ++++++++++++++++---------------------
>   drivers/nvme/host/tcp.c     | 56 +++++++++++++++----------------------
>   5 files changed, 75 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 2edd086fa922..544195369c97 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -493,6 +493,29 @@ void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
>   }
>   EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
>   
> +void nvmf_error_recovery_work(struct work_struct *work)
> +{
> +	struct nvme_ctrl *ctrl = container_of(work,
> +				struct nvme_ctrl, err_work);
> +
> +	nvme_stop_keep_alive(ctrl);
> +	ctrl->ops->teardown_ctrl_io_queues(ctrl);
> +	/* unquiesce to fail fast pending requests */
> +	nvme_start_queues(ctrl);
> +	ctrl->ops->teardown_ctrl_admin_queue(ctrl);
> +	blk_mq_unquiesce_queue(ctrl->admin_q);
> +
> +	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
> +		/* state change failure is ok if we started ctrl delete */
> +		WARN_ON_ONCE(ctrl->state != NVME_CTRL_DELETING &&
> +			     ctrl->state != NVME_CTRL_DELETING_NOIO);
> +		return;
> +	}
> +
> +	nvmf_reconnect_or_remove(ctrl);
> +}
> +EXPORT_SYMBOL_GPL(nvmf_error_recovery_work);
> +
>   void nvmf_error_recovery(struct nvme_ctrl *ctrl)
>   {
>   	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 3d8ec7133fc8..8655eff74ed0 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -190,6 +190,7 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
>   bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>   void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
>   void nvmf_error_recovery(struct nvme_ctrl *ctrl);
> +void nvmf_error_recovery_work(struct work_struct *work);
>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>   		struct nvmf_ctrl_options *opts);
>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index f9e1ce93d61d..1573edf6e97f 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -493,6 +493,10 @@ struct nvme_ctrl_ops {
>   	void (*submit_async_event)(struct nvme_ctrl *ctrl);
>   	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
>   	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
> +
> +	/* Fabrics only */
> +	void (*teardown_ctrl_io_queues)(struct nvme_ctrl *ctrl);
> +	void (*teardown_ctrl_admin_queue)(struct nvme_ctrl *ctrl);
>   };
>   
>   /*

As noted by Sagi, this abstraction really is awkward.
Why not have

void (*teardown_ctrl_queues)(struct nvme_ctrl *ctrl, int qid);

and let the callback figure out whether it's for the admin queue
(ie qid == 8) or the normal I/O queue?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 6/7] nvme/nvme-fabrics: introduce nvmf_reconnect_ctrl_work API

[Max Gurtovoy]

Reconnect work is duplicated in RDMA and TCP transports. Move this logic
to common code. For that, introduce a new ctrl op to setup a ctrl.

Also update the RDMA/TCP transport drivers to use this API and remove
the duplicated code.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 24 ++++++++++++++++++++++++
 drivers/nvme/host/fabrics.h |  1 +
 drivers/nvme/host/nvme.h    |  1 +
 drivers/nvme/host/rdma.c    | 26 ++++----------------------
 drivers/nvme/host/tcp.c     | 27 ++++-----------------------
 5 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 544195369c97..7f76b27ce1f2 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -526,6 +526,30 @@ void nvmf_error_recovery(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvmf_error_recovery);
 
+void nvmf_reconnect_ctrl_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
+			struct nvme_ctrl, connect_work);
+
+	++ctrl->nr_reconnects;
+
+	if (ctrl->ops->setup_ctrl(ctrl))
+		goto requeue;
+
+	dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
+			ctrl->nr_reconnects);
+
+	ctrl->nr_reconnects = 0;
+
+	return;
+
+requeue:
+	dev_info(ctrl->device, "Failed reconnect attempt %d\n",
+		 ctrl->nr_reconnects);
+	nvmf_reconnect_or_remove(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvmf_reconnect_ctrl_work);
+
 /**
  * nvmf_register_transport() - NVMe Fabrics Library registration function.
  * @ops:	Transport ops instance to be registered to the
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 8655eff74ed0..49c98b69647f 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -191,6 +191,7 @@ bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
 void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
 void nvmf_error_recovery(struct nvme_ctrl *ctrl);
 void nvmf_error_recovery_work(struct work_struct *work);
+void nvmf_reconnect_ctrl_work(struct work_struct *work);
 bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 		struct nvmf_ctrl_options *opts);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1573edf6e97f..9ae9594998c3 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -497,6 +497,7 @@ struct nvme_ctrl_ops {
 	/* Fabrics only */
 	void (*teardown_ctrl_io_queues)(struct nvme_ctrl *ctrl);
 	void (*teardown_ctrl_admin_queue)(struct nvme_ctrl *ctrl);
+	int (*setup_ctrl)(struct nvme_ctrl *ctrl);
 };
 
 /*
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index f4e4ebf673d2..7fb2f434fe0d 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1151,27 +1151,9 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 	return ret;
 }
 
-static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
+static int _nvme_rdma_setup_ctrl(struct nvme_ctrl *ctrl)
 {
-	struct nvme_rdma_ctrl *ctrl = container_of(to_delayed_work(work),
-			struct nvme_rdma_ctrl, ctrl.connect_work);
-
-	++ctrl->ctrl.nr_reconnects;
-
-	if (nvme_rdma_setup_ctrl(ctrl, false))
-		goto requeue;
-
-	dev_info(ctrl->ctrl.device, "Successfully reconnected (%d attempts)\n",
-			ctrl->ctrl.nr_reconnects);
-
-	ctrl->ctrl.nr_reconnects = 0;
-
-	return;
-
-requeue:
-	dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
-			ctrl->ctrl.nr_reconnects);
-	nvmf_reconnect_or_remove(&ctrl->ctrl);
+	return nvme_rdma_setup_ctrl(to_rdma_ctrl(ctrl), false);
 }
 
 static void nvme_rdma_end_request(struct nvme_rdma_request *req)
@@ -2242,6 +2224,7 @@ static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
 	.get_address			= nvmf_get_address,
 	.teardown_ctrl_io_queues	= _nvme_rdma_teardown_io_queues,
 	.teardown_ctrl_admin_queue	= _nvme_rdma_teardown_admin_queue,
+	.setup_ctrl			= _nvme_rdma_setup_ctrl,
 };
 
 /*
@@ -2319,8 +2302,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		goto out_free_ctrl;
 	}
 
-	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
-			nvme_rdma_reconnect_ctrl_work);
+	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvmf_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 14bd16b8d99f..c0e5bb3949b3 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2042,28 +2042,9 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 	return ret;
 }
 
-static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
+static int _nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl)
 {
-	struct nvme_tcp_ctrl *tcp_ctrl = container_of(to_delayed_work(work),
-			struct nvme_tcp_ctrl, ctrl.connect_work);
-	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
-
-	++ctrl->nr_reconnects;
-
-	if (nvme_tcp_setup_ctrl(ctrl, false))
-		goto requeue;
-
-	dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
-			ctrl->nr_reconnects);
-
-	ctrl->nr_reconnects = 0;
-
-	return;
-
-requeue:
-	dev_info(ctrl->device, "Failed reconnect attempt %d\n",
-			ctrl->nr_reconnects);
-	nvmf_reconnect_or_remove(ctrl);
+	return nvme_tcp_setup_ctrl(ctrl, false);
 }
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
@@ -2435,6 +2416,7 @@ static const struct nvme_ctrl_ops nvme_tcp_ctrl_ops = {
 	.get_address			= nvmf_get_address,
 	.teardown_ctrl_io_queues	= _nvme_tcp_teardown_io_queues,
 	.teardown_ctrl_admin_queue	= _nvme_tcp_teardown_admin_queue,
+	.setup_ctrl			= _nvme_tcp_setup_ctrl,
 };
 
 static bool
@@ -2471,8 +2453,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 	ctrl->ctrl.sqsize = opts->queue_size - 1;
 	ctrl->ctrl.kato = opts->kato;
 
-	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
-			nvme_tcp_reconnect_ctrl_work);
+	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvmf_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
 
-- 
2.18.1

Re: [PATCH 6/7] nvme/nvme-fabrics: introduce nvmf_reconnect_ctrl_work API

[Chaitanya Kulkarni]

On 10/18/2021 6:40 AM, Max Gurtovoy wrote:
> Reconnect work is duplicated in RDMA and TCP transports. Move this logic
> to common code. For that, introduce a new ctrl op to setup a ctrl.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

Same as previous patch :-

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH 6/7] nvme/nvme-fabrics: introduce nvmf_reconnect_ctrl_work API

[Sagi Grimberg]

>   static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
> @@ -2435,6 +2416,7 @@ static const struct nvme_ctrl_ops nvme_tcp_ctrl_ops = {
>   	.get_address			= nvmf_get_address,
>   	.teardown_ctrl_io_queues	= _nvme_tcp_teardown_io_queues,
>   	.teardown_ctrl_admin_queue	= _nvme_tcp_teardown_admin_queue,
> +	.setup_ctrl			= _nvme_tcp_setup_ctrl,

This asymmetry is not great, what made you choose to go this way?

Re: [PATCH 6/7] nvme/nvme-fabrics: introduce nvmf_reconnect_ctrl_work API

[Max Gurtovoy]

On 10/19/2021 3:44 PM, Sagi Grimberg wrote:
>
>>   static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool 
>> shutdown)
>> @@ -2435,6 +2416,7 @@ static const struct nvme_ctrl_ops 
>> nvme_tcp_ctrl_ops = {
>>       .get_address            = nvmf_get_address,
>>       .teardown_ctrl_io_queues    = _nvme_tcp_teardown_io_queues,
>>       .teardown_ctrl_admin_queue    = _nvme_tcp_teardown_admin_queue,
>> +    .setup_ctrl            = _nvme_tcp_setup_ctrl,
>
> This asymmetry is not great, what made you choose to go this way?

There are more duplicated flows we can work on in the future.

Re: [PATCH 6/7] nvme/nvme-fabrics: introduce nvmf_reconnect_ctrl_work API

[Hannes Reinecke]

On 10/18/21 3:40 PM, Max Gurtovoy wrote:
> Reconnect work is duplicated in RDMA and TCP transports. Move this logic
> to common code. For that, introduce a new ctrl op to setup a ctrl.
> 
> Also update the RDMA/TCP transport drivers to use this API and remove
> the duplicated code.
> 
> Reviewed-by: Israel Rukshin <israelr@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>   drivers/nvme/host/fabrics.c | 24 ++++++++++++++++++++++++
>   drivers/nvme/host/fabrics.h |  1 +
>   drivers/nvme/host/nvme.h    |  1 +
>   drivers/nvme/host/rdma.c    | 26 ++++----------------------
>   drivers/nvme/host/tcp.c     | 27 ++++-----------------------
>   5 files changed, 34 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 544195369c97..7f76b27ce1f2 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -526,6 +526,30 @@ void nvmf_error_recovery(struct nvme_ctrl *ctrl)
>   }
>   EXPORT_SYMBOL_GPL(nvmf_error_recovery);
>   
> +void nvmf_reconnect_ctrl_work(struct work_struct *work)
> +{
> +	struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
> +			struct nvme_ctrl, connect_work);
> +
> +	++ctrl->nr_reconnects;
> +
> +	if (ctrl->ops->setup_ctrl(ctrl))
> +		goto requeue;
> +
> +	dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
> +			ctrl->nr_reconnects);
> +
> +	ctrl->nr_reconnects = 0;
> +
> +	return;
> +
> +requeue:
> +	dev_info(ctrl->device, "Failed reconnect attempt %d\n",
> +		 ctrl->nr_reconnects);
> +	nvmf_reconnect_or_remove(ctrl);
> +}
> +EXPORT_SYMBOL_GPL(nvmf_reconnect_ctrl_work);
> +
>   /**
>    * nvmf_register_transport() - NVMe Fabrics Library registration function.
>    * @ops:	Transport ops instance to be registered to the
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 8655eff74ed0..49c98b69647f 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -191,6 +191,7 @@ bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
>   void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
>   void nvmf_error_recovery(struct nvme_ctrl *ctrl);
>   void nvmf_error_recovery_work(struct work_struct *work);
> +void nvmf_reconnect_ctrl_work(struct work_struct *work);
>   bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
>   		struct nvmf_ctrl_options *opts);
>   
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1573edf6e97f..9ae9594998c3 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -497,6 +497,7 @@ struct nvme_ctrl_ops {
>   	/* Fabrics only */
>   	void (*teardown_ctrl_io_queues)(struct nvme_ctrl *ctrl);
>   	void (*teardown_ctrl_admin_queue)(struct nvme_ctrl *ctrl);
> +	int (*setup_ctrl)(struct nvme_ctrl *ctrl);
>   };
>   
>   /*
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index f4e4ebf673d2..7fb2f434fe0d 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1151,27 +1151,9 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>   	return ret;
>   }
>   
> -static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
> +static int _nvme_rdma_setup_ctrl(struct nvme_ctrl *ctrl)
>   {
> -	struct nvme_rdma_ctrl *ctrl = container_of(to_delayed_work(work),
> -			struct nvme_rdma_ctrl, ctrl.connect_work);
> -
> -	++ctrl->ctrl.nr_reconnects;
> -
> -	if (nvme_rdma_setup_ctrl(ctrl, false))
> -		goto requeue;
> -
> -	dev_info(ctrl->ctrl.device, "Successfully reconnected (%d attempts)\n",
> -			ctrl->ctrl.nr_reconnects);
> -
> -	ctrl->ctrl.nr_reconnects = 0;
> -
> -	return;
> -
> -requeue:
> -	dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
> -			ctrl->ctrl.nr_reconnects);
> -	nvmf_reconnect_or_remove(&ctrl->ctrl);
> +	return nvme_rdma_setup_ctrl(to_rdma_ctrl(ctrl), false);
>   }
>   
>   static void nvme_rdma_end_request(struct nvme_rdma_request *req)

Really? Can't we separate nvme_rdma_setup_ctrl() such that we have two 
distinct functions (one for new == true, and one for new == false)?
Or, alternatively, setting up the tagset in nvme_rdma_init_ctrl() and 
kill the 'new' argument altogether?

> @@ -2242,6 +2224,7 @@ static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
>   	.get_address			= nvmf_get_address,
>   	.teardown_ctrl_io_queues	= _nvme_rdma_teardown_io_queues,
>   	.teardown_ctrl_admin_queue	= _nvme_rdma_teardown_admin_queue,
> +	.setup_ctrl			= _nvme_rdma_setup_ctrl,
>   };
>   
>   /*
> @@ -2319,8 +2302,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>   		goto out_free_ctrl;
>   	}
>   
> -	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work,
> -			nvme_rdma_reconnect_ctrl_work);
> +	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvmf_reconnect_ctrl_work);
>   	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
>   	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
>   
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 14bd16b8d99f..c0e5bb3949b3 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2042,28 +2042,9 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>   	return ret;
>   }
>   
> -static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
> +static int _nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl)
>   {
> -	struct nvme_tcp_ctrl *tcp_ctrl = container_of(to_delayed_work(work),
> -			struct nvme_tcp_ctrl, ctrl.connect_work);
> -	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
> -
> -	++ctrl->nr_reconnects;
> -
> -	if (nvme_tcp_setup_ctrl(ctrl, false))
> -		goto requeue;
> -
> -	dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
> -			ctrl->nr_reconnects);
> -
> -	ctrl->nr_reconnects = 0;
> -
> -	return;
> -
> -requeue:
> -	dev_info(ctrl->device, "Failed reconnect attempt %d\n",
> -			ctrl->nr_reconnects);
> -	nvmf_reconnect_or_remove(ctrl);
> +	return nvme_tcp_setup_ctrl(ctrl, false);
>   }
>   Same argument here; I'd rather modify nvme_tcp_setup_ctrl() to drop the 
'new' argument and allowing it to be used as the callback directly.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 7/7] nvme-fabrics: add nvmf_init_ctrl/nvmf_teardown_ctrl API

[Max Gurtovoy]

Centralize the initalization and teardown of fabrics specific settings.
For now, only used by RDMA and TCP fabric transports.

Also, convert the reconnect_work and error_recovery_work to be static
functions since they are not used outside the fabrics driver anymore.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/fabrics.c | 20 ++++++++++++++++----
 drivers/nvme/host/fabrics.h |  4 ++--
 drivers/nvme/host/rdma.c    |  7 ++-----
 drivers/nvme/host/tcp.c     |  7 ++-----
 4 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 7f76b27ce1f2..4a16e5f85d24 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -493,7 +493,7 @@ void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvmf_reconnect_or_remove);
 
-void nvmf_error_recovery_work(struct work_struct *work)
+static void nvmf_error_recovery_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl = container_of(work,
 				struct nvme_ctrl, err_work);
@@ -514,7 +514,6 @@ void nvmf_error_recovery_work(struct work_struct *work)
 
 	nvmf_reconnect_or_remove(ctrl);
 }
-EXPORT_SYMBOL_GPL(nvmf_error_recovery_work);
 
 void nvmf_error_recovery(struct nvme_ctrl *ctrl)
 {
@@ -526,7 +525,7 @@ void nvmf_error_recovery(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvmf_error_recovery);
 
-void nvmf_reconnect_ctrl_work(struct work_struct *work)
+static void nvmf_reconnect_ctrl_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
 			struct nvme_ctrl, connect_work);
@@ -548,7 +547,20 @@ void nvmf_reconnect_ctrl_work(struct work_struct *work)
 		 ctrl->nr_reconnects);
 	nvmf_reconnect_or_remove(ctrl);
 }
-EXPORT_SYMBOL_GPL(nvmf_reconnect_ctrl_work);
+
+void nvmf_init_ctrl(struct nvme_ctrl *ctrl)
+{
+	INIT_DELAYED_WORK(&ctrl->connect_work, nvmf_reconnect_ctrl_work);
+	INIT_WORK(&ctrl->err_work, nvmf_error_recovery_work);
+}
+EXPORT_SYMBOL_GPL(nvmf_init_ctrl);
+
+void nvmf_teardown_ctrl(struct nvme_ctrl *ctrl)
+{
+	cancel_work_sync(&ctrl->err_work);
+	cancel_delayed_work_sync(&ctrl->connect_work);
+}
+EXPORT_SYMBOL_GPL(nvmf_teardown_ctrl);
 
 /**
  * nvmf_register_transport() - NVMe Fabrics Library registration function.
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 49c98b69647f..08b290c2e01a 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -190,8 +190,8 @@ int nvmf_get_address(struct nvme_ctrl *ctrl, char *buf, int size);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl);
 void nvmf_reconnect_or_remove(struct nvme_ctrl *ctrl);
 void nvmf_error_recovery(struct nvme_ctrl *ctrl);
-void nvmf_error_recovery_work(struct work_struct *work);
-void nvmf_reconnect_ctrl_work(struct work_struct *work);
+void nvmf_init_ctrl(struct nvme_ctrl *ctrl);
+void nvmf_teardown_ctrl(struct nvme_ctrl *ctrl);
 bool nvmf_ip_options_match(struct nvme_ctrl *ctrl,
 		struct nvmf_ctrl_options *opts);
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 7fb2f434fe0d..aa3e142047eb 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2169,9 +2169,7 @@ static const struct blk_mq_ops nvme_rdma_admin_mq_ops = {
 
 static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 {
-	cancel_work_sync(&ctrl->ctrl.err_work);
-	cancel_delayed_work_sync(&ctrl->ctrl.connect_work);
-
+	nvmf_teardown_ctrl(&ctrl->ctrl);
 	nvme_rdma_teardown_io_queues(ctrl, shutdown);
 	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
 	if (shutdown)
@@ -2302,8 +2300,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		goto out_free_ctrl;
 	}
 
-	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvmf_reconnect_ctrl_work);
-	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
+	nvmf_init_ctrl(&ctrl->ctrl);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
 
 	ctrl->ctrl.queue_count = opts->nr_io_queues + opts->nr_write_queues +
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index c0e5bb3949b3..26c2b181edb9 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2049,9 +2049,7 @@ static int _nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl)
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
-	cancel_work_sync(&ctrl->err_work);
-	cancel_delayed_work_sync(&ctrl->connect_work);
-
+	nvmf_teardown_ctrl(ctrl);
 	nvme_tcp_teardown_io_queues(ctrl, shutdown);
 	blk_mq_quiesce_queue(ctrl->admin_q);
 	if (shutdown)
@@ -2453,8 +2451,7 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 	ctrl->ctrl.sqsize = opts->queue_size - 1;
 	ctrl->ctrl.kato = opts->kato;
 
-	INIT_DELAYED_WORK(&ctrl->ctrl.connect_work, nvmf_reconnect_ctrl_work);
-	INIT_WORK(&ctrl->ctrl.err_work, nvmf_error_recovery_work);
+	nvmf_init_ctrl(&ctrl->ctrl);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
 
 	if (!(opts->mask & NVMF_OPT_TRSVCID)) {
-- 
2.18.1

Re: [PATCH 7/7] nvme-fabrics: add nvmf_init_ctrl/nvmf_teardown_ctrl API

[Sagi Grimberg]

> +void nvmf_teardown_ctrl(struct nvme_ctrl *ctrl)
> +{
> +	cancel_work_sync(&ctrl->err_work);
> +	cancel_delayed_work_sync(&ctrl->connect_work);

Name doesn't represent what is done in the function at all..
possible names:
nvmf_uninit_ctrl
nvmf_deinit_ctrl
nvmf_fini_ctrl
...

Re: [PATCH 7/7] nvme-fabrics: add nvmf_init_ctrl/nvmf_teardown_ctrl API

[Max Gurtovoy]

On 10/19/2021 3:46 PM, Sagi Grimberg wrote:
>
>> +void nvmf_teardown_ctrl(struct nvme_ctrl *ctrl)
>> +{
>> +    cancel_work_sync(&ctrl->err_work);
>> +    cancel_delayed_work_sync(&ctrl->connect_work);
>
> Name doesn't represent what is done in the function at all..
> possible names:
> nvmf_uninit_ctrl

Ok, i'll change it to nvmf_uninit_ctrl

> nvmf_deinit_ctrl
> nvmf_fini_ctrl
> ...

Re: [PATCH v1 0/7] Centrelize common fabrics code to core drivers

[James Smart]

On 10/18/2021 6:40 AM, Max Gurtovoy wrote:
> Hi Christoph, Sagi, Keith and Co,
> This series is intended to move common reconnection, device removal and
> error recovery logic to common fabrics and core drivers. In this stage,
> it mainly deals with the RDMA and TCP drivers which had the same logic
> for the above tasks. FC driver can start using this framework in the
> future after some preparation work that should be done.
> 
> This will ease on code maintenance and reduce the code duplication
> between the RDMA and TCP drivers.
> 
> Since there are some attributes that were moved to generic nvme_ctrl
> structure, I've measured perf impact.
> 
> NVMe PCI Perf numbers using NVIDIA's NVMe SNAP Bluefield-2 device
> (Fio 16 jobs, 128 iodepth, libaio):
> 
> IO size   rand READ (before/after)  rand WRITE (before/after)
> -------- -------------------------  --------------------------
>   512B          2189k/2197k                2031k/2092k
>   1KB           2188k/2201k                2027k/2088k
>   2KB           2166k/2181k                2023k/2078k
>   4KB           2153k/2169k                2025k/2076k
>   8KB           1381k/1381k                1378k/1375k
>   16KB          669k/676k                  660k/664k
>   32KB          320k/322k                  305k/305k
>   64KB          157k/157k                  151k/151k
> 
> As we can see, there is no impact on the performance.
> 
> Max Gurtovoy (7):
>    nvme: add connect_work attribute to nvme ctrl
>    nvme-fabrics: introduce nvmf_reconnect_or_remove API
>    nvme: add err_work attribute to nvme ctrl
>    nvme-fabrics: introduce nvmf_error_recovery API
>    nvme/nvme-fabrics: introduce nvmf_error_recovery_work API
>    nvme/nvme-fabrics: introduce nvmf_reconnect_ctrl_work API
>    nvme-fabrics: add nvmf_init_ctrl/nvmf_teardown_ctrl API
> 
>   drivers/nvme/host/fabrics.c |  90 ++++++++++++++++++++++++
>   drivers/nvme/host/fabrics.h |   4 ++
>   drivers/nvme/host/fc.c      |  23 ++++---
>   drivers/nvme/host/nvme.h    |   7 ++
>   drivers/nvme/host/rdma.c    | 134 ++++++++++--------------------------
>   drivers/nvme/host/tcp.c     | 124 ++++++++-------------------------
>   6 files changed, 180 insertions(+), 202 deletions(-)
> 

Thanks Max.

I've been thinking we needed to do this for a while as well.  FC should 
be part of this, and up front, so the abstraction is tested properly. 
I'd rather merge now than spin things again.  I'll go through this over 
the next day or so.

-- james

Re: [PATCH v1 0/7] Centrelize common fabrics code to core drivers

[Christoph Hellwig]

On Mon, Oct 18, 2021 at 07:08:06AM -0700, James Smart wrote:
> I've been thinking we needed to do this for a while as well.  FC should be 
> part of this, and up front, so the abstraction is tested properly. I'd 
> rather merge now than spin things again.  I'll go through this over the 
> next day or so.

I'd really love tp pick this up quickly so that it doesn't go stale again
if we could get a few reviews.  The merge window is about to close.

Re: [PATCH v1 0/7] Centrelize common fabrics code to core drivers

[Chaitanya Kulkarni]

> NVMe PCI Perf numbers using NVIDIA's NVMe SNAP Bluefield-2 device
> (Fio 16 jobs, 128 iodepth, libaio):
> 
> IO size   rand READ (before/after)  rand WRITE (before/after)
> -------- -------------------------  --------------------------
>   512B          2189k/2197k                2031k/2092k
>   1KB           2188k/2201k                2027k/2088k
>   2KB           2166k/2181k                2023k/2078k
>   4KB           2153k/2169k                2025k/2076k
>   8KB           1381k/1381k                1378k/1375k
>   16KB          669k/676k                  660k/664k
>   32KB          320k/322k                  305k/305k
>   64KB          157k/157k                  151k/151k
> 

Can you please share latency numbers for the purpose of documentation
on this thread ?

Re: [PATCH v1 0/7] Centrelize common fabrics code to core drivers

[Sagi Grimberg]

On 10/18/21 4:40 PM, Max Gurtovoy wrote:
> Hi Christoph, Sagi, Keith and Co,
> This series is intended to move common reconnection, device removal and
> error recovery logic to common fabrics and core drivers. In this stage,
> it mainly deals with the RDMA and TCP drivers which had the same logic
> for the above tasks. FC driver can start using this framework in the
> future after some preparation work that should be done.

This is a good start, but I think we should also see how we can
fit pci here as well.