All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-next 0/4] iSER initiator updates for 3.16
@ 2014-05-22  8:00 Or Gerlitz
       [not found] ` <1400745621-9978-1-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Or Gerlitz @ 2014-05-22  8:00 UTC (permalink / raw)
  To: roland-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, roid-VPRAkNaXOzVWk0Htik3J/w,
	sagig-H+wXaHxf7aLQT0dZR+AlfA, oren-VPRAkNaXOzVWk0Htik3J/w,
	arieln-VPRAkNaXOzVWk0Htik3J/w, Or Gerlitz

From: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This patchset address several resiliency issues in the area of
connection establishment and teardown (especially in error flows).

Patch #1 introduces a re-design of iSER connection management done
by Ariel and Sagi. It comes to solve few error-flow hangs/oops's 
we've seen in regression testing under various failure schemes, and
make things simpler.

Patch #2 Addresses a racy state transition.

Following that comes a patch that fixes some prints and a bump to the 
driver version.

Or.

Ariel Nahum (2):
  IB/iser: Simplify connection management
  IB/iser: Fix a possible race in iser connection states transition

Or Gerlitz (1):
  IB/iser: Bump version to 1.4

Roi Dayan (1):
  IB/iser: Add missing newlines to logging messages

 drivers/infiniband/ulp/iser/iscsi_iser.c |  105 +++++++++++++++++------------
 drivers/infiniband/ulp/iser/iscsi_iser.h |   10 ++-
 drivers/infiniband/ulp/iser/iser_verbs.c |   89 +++++++++++--------------
 3 files changed, 106 insertions(+), 98 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH for-next 1/4] IB/iser: Simplify connection management
       [not found] ` <1400745621-9978-1-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
@ 2014-05-22  8:00   ` Or Gerlitz
       [not found]     ` <1400745621-9978-2-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
  2014-05-22  8:00   ` [PATCH for-next 2/4] IB/iser: Fix a possible race in iser connection states transition Or Gerlitz
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Or Gerlitz @ 2014-05-22  8:00 UTC (permalink / raw)
  To: roland-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, roid-VPRAkNaXOzVWk0Htik3J/w,
	sagig-H+wXaHxf7aLQT0dZR+AlfA, oren-VPRAkNaXOzVWk0Htik3J/w,
	arieln-VPRAkNaXOzVWk0Htik3J/w, Sagi Grimberg

From: Ariel Nahum <arieln-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

iSER relies on refcounting to manage iser connections
establishment and teardown.

Following commit 39ff05d "IB/iser: Enhance disconnection logic
for multi-pathing", iser connection maintain 3 references:

- iscsi_endpoint (at creation stage)
- cma_id (at connection request stage)
- iscsi_conn (at bind stage)

We can avoid taking explicit refcounts by correctly serializing
iser teardown flows (graceful and non-graceful).

Our approach is to trigger a scheduled work to handle ordered
teardown by gracefully waiting for 2 cleanup stages to complete:

1. Cleanup of live pending tasks indicated by iscsi_conn_stop completion
2. Flush errors processing

Each completed stage will notify a waiting worker thread when it is
done to allow teardwon continuation.

Since iSCSI connection establishment may trigger endpoint disconnect without
a successful endpoint connect, we rely on the iscsi <-> iser binding (.conn_bind)
to learn about the teardown policy we should take wrt cleanup stages.

Since all cleanup worker threads are scheduled (release_wq) in .ep_disconnect
it is safe to assume that when module_exit is called, all cleanup workers are
already scheduled. Thus proper module unload shall flush all scheduled works
before allowing safe exit, to guarantee no resources got left behind.

Signed-off-by: Ariel Nahum <arieln-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |   97 +++++++++++++++++------------
 drivers/infiniband/ulp/iser/iscsi_iser.h |    8 ++-
 drivers/infiniband/ulp/iser/iser_verbs.c |   85 +++++++++++---------------
 3 files changed, 99 insertions(+), 91 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 25f195e..f217488 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -99,6 +99,7 @@ MODULE_PARM_DESC(pi_enable, "Enable T10-PI offload support (default:disabled)");
 module_param_named(pi_guard, iser_pi_guard, int, 0644);
 MODULE_PARM_DESC(pi_guard, "T10-PI guard_type, 0:CRC|1:IP_CSUM (default:CRC)");
 
+static struct workqueue_struct *release_wq;
 struct iser_global ig;
 
 void
@@ -337,24 +338,6 @@ iscsi_iser_conn_create(struct iscsi_cls_session *cls_session, uint32_t conn_idx)
 	return cls_conn;
 }
 
-static void
-iscsi_iser_conn_destroy(struct iscsi_cls_conn *cls_conn)
-{
-	struct iscsi_conn *conn = cls_conn->dd_data;
-	struct iser_conn *ib_conn = conn->dd_data;
-
-	iscsi_conn_teardown(cls_conn);
-	/*
-	 * Userspace will normally call the stop callback and
-	 * already have freed the ib_conn, but if it goofed up then
-	 * we free it here.
-	 */
-	if (ib_conn) {
-		ib_conn->iscsi_conn = NULL;
-		iser_conn_put(ib_conn, 1); /* deref iscsi/ib conn unbinding */
-	}
-}
-
 static int
 iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
 		     struct iscsi_cls_conn *cls_conn, uint64_t transport_eph,
@@ -392,29 +375,39 @@ iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
 	conn->dd_data = ib_conn;
 	ib_conn->iscsi_conn = conn;
 
-	iser_conn_get(ib_conn); /* ref iscsi/ib conn binding */
 	return 0;
 }
 
+static int
+iscsi_iser_conn_start(struct iscsi_cls_conn *cls_conn)
+{
+	struct iscsi_conn *iscsi_conn;
+	struct iser_conn *ib_conn;
+
+	iscsi_conn = cls_conn->dd_data;
+	ib_conn = iscsi_conn->dd_data;
+	reinit_completion(&ib_conn->stop_completion);
+
+	return iscsi_conn_start(cls_conn);
+}
+
 static void
 iscsi_iser_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
 {
 	struct iscsi_conn *conn = cls_conn->dd_data;
 	struct iser_conn *ib_conn = conn->dd_data;
 
+	iser_dbg("stopping iscsi_conn: %p, ib_conn: %p\n", conn, ib_conn);
+	iscsi_conn_stop(cls_conn, flag);
+
 	/*
 	 * Userspace may have goofed up and not bound the connection or
 	 * might have only partially setup the connection.
 	 */
 	if (ib_conn) {
-		iscsi_conn_stop(cls_conn, flag);
-		/*
-		 * There is no unbind event so the stop callback
-		 * must release the ref from the bind.
-		 */
-		iser_conn_put(ib_conn, 1); /* deref iscsi/ib conn unbinding */
+		conn->dd_data = NULL;
+		complete(&ib_conn->stop_completion);
 	}
-	conn->dd_data = NULL;
 }
 
 static void iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
@@ -652,19 +645,20 @@ iscsi_iser_ep_disconnect(struct iscsi_endpoint *ep)
 	struct iser_conn *ib_conn;
 
 	ib_conn = ep->dd_data;
-	if (ib_conn->iscsi_conn)
-		/*
-		 * Must suspend xmit path if the ep is bound to the
-		 * iscsi_conn, so we know we are not accessing the ib_conn
-		 * when we free it.
-		 *
-		 * This may not be bound if the ep poll failed.
-		 */
-		iscsi_suspend_tx(ib_conn->iscsi_conn);
-
-
-	iser_info("ib conn %p state %d\n", ib_conn, ib_conn->state);
+	iser_info("ep %p ib conn %p state %d\n", ep, ib_conn, ib_conn->state);
 	iser_conn_terminate(ib_conn);
+
+	/*
+	 * if iser_conn and iscsi_conn are bound, we must wait iscsi_conn_stop
+	 * call and ISER_CONN_DOWN state before freeing the iser resources.
+	 * otherwise we are safe to free resources immediately.
+	 */
+	if (ib_conn->iscsi_conn) {
+		INIT_WORK(&ib_conn->release_work, iser_release_work);
+		queue_work(release_wq, &ib_conn->release_work);
+	} else {
+		iser_conn_release(ib_conn);
+	}
 }
 
 static umode_t iser_attr_is_visible(int param_type, int param)
@@ -748,13 +742,13 @@ static struct iscsi_transport iscsi_iser_transport = {
 	/* connection management */
 	.create_conn            = iscsi_iser_conn_create,
 	.bind_conn              = iscsi_iser_conn_bind,
-	.destroy_conn           = iscsi_iser_conn_destroy,
+	.destroy_conn           = iscsi_conn_teardown,
 	.attr_is_visible	= iser_attr_is_visible,
 	.set_param              = iscsi_iser_set_param,
 	.get_conn_param		= iscsi_conn_get_param,
 	.get_ep_param		= iscsi_iser_get_ep_param,
 	.get_session_param	= iscsi_session_get_param,
-	.start_conn             = iscsi_conn_start,
+	.start_conn             = iscsi_iser_conn_start,
 	.stop_conn              = iscsi_iser_conn_stop,
 	/* iscsi host params */
 	.get_host_param		= iscsi_host_get_param,
@@ -801,6 +795,12 @@ static int __init iser_init(void)
 	mutex_init(&ig.connlist_mutex);
 	INIT_LIST_HEAD(&ig.connlist);
 
+	release_wq = alloc_workqueue("release workqueue", 0, 0);
+	if (!release_wq) {
+		iser_err("failed to allocate release workqueue\n");
+		return -ENOMEM;
+	}
+
 	iscsi_iser_scsi_transport = iscsi_register_transport(
 							&iscsi_iser_transport);
 	if (!iscsi_iser_scsi_transport) {
@@ -819,7 +819,24 @@ register_transport_failure:
 
 static void __exit iser_exit(void)
 {
+	struct iser_conn *ib_conn, *n;
+	int connlist_empty;
+
 	iser_dbg("Removing iSER datamover...\n");
+	destroy_workqueue(release_wq);
+
+	mutex_lock(&ig.connlist_mutex);
+	connlist_empty = list_empty(&ig.connlist);
+	mutex_unlock(&ig.connlist_mutex);
+
+	if (!connlist_empty) {
+		iser_err("Error cleanup stage completed but we still have iser "
+			 "connections, destroying them anyway.\n");
+		list_for_each_entry_safe(ib_conn, n, &ig.connlist, conn_list) {
+			iser_conn_release(ib_conn);
+		}
+	}
+
 	iscsi_unregister_transport(&iscsi_iser_transport);
 	kmem_cache_destroy(ig.desc_cache);
 }
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 324129f..d309620 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -333,6 +333,8 @@ struct iser_conn {
 	int                          post_recv_buf_count; /* posted rx count  */
 	atomic_t                     post_send_buf_count; /* posted tx count   */
 	char 			     name[ISER_OBJECT_NAME_SIZE];
+	struct work_struct	     release_work;
+	struct completion	     stop_completion;
 	struct list_head	     conn_list;       /* entry in ig conn list */
 
 	char  			     *login_buf;
@@ -417,12 +419,12 @@ void iscsi_iser_recv(struct iscsi_conn *conn,
 
 void iser_conn_init(struct iser_conn *ib_conn);
 
-void iser_conn_get(struct iser_conn *ib_conn);
-
-int iser_conn_put(struct iser_conn *ib_conn, int destroy_cma_id_allowed);
+void iser_conn_release(struct iser_conn *ib_conn);
 
 void iser_conn_terminate(struct iser_conn *ib_conn);
 
+void iser_release_work(struct work_struct *work);
+
 void iser_rcv_completion(struct iser_rx_desc *desc,
 			 unsigned long    dto_xfer_len,
 			struct iser_conn *ib_conn);
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 32849f2..4c698e5 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -581,14 +581,30 @@ static int iser_conn_state_comp_exch(struct iser_conn *ib_conn,
 	return ret;
 }
 
+void iser_release_work(struct work_struct *work)
+{
+	struct iser_conn *ib_conn;
+
+	ib_conn = container_of(work, struct iser_conn, release_work);
+
+	/* wait for .conn_stop callback */
+	wait_for_completion(&ib_conn->stop_completion);
+
+	/* wait for the qp`s post send and post receive buffers to empty */
+	wait_event_interruptible(ib_conn->wait,
+				 ib_conn->state == ISER_CONN_DOWN);
+
+	iser_conn_release(ib_conn);
+}
+
 /**
  * Frees all conn objects and deallocs conn descriptor
  */
-static void iser_conn_release(struct iser_conn *ib_conn, int can_destroy_id)
+void iser_conn_release(struct iser_conn *ib_conn)
 {
 	struct iser_device  *device = ib_conn->device;
 
-	BUG_ON(ib_conn->state != ISER_CONN_DOWN);
+	BUG_ON(ib_conn->state == ISER_CONN_UP);
 
 	mutex_lock(&ig.connlist_mutex);
 	list_del(&ib_conn->conn_list);
@@ -600,27 +616,13 @@ static void iser_conn_release(struct iser_conn *ib_conn, int can_destroy_id)
 	if (device != NULL)
 		iser_device_try_release(device);
 	/* if cma handler context, the caller actually destroy the id */
-	if (ib_conn->cma_id != NULL && can_destroy_id) {
+	if (ib_conn->cma_id != NULL) {
 		rdma_destroy_id(ib_conn->cma_id);
 		ib_conn->cma_id = NULL;
 	}
 	iscsi_destroy_endpoint(ib_conn->ep);
 }
 
-void iser_conn_get(struct iser_conn *ib_conn)
-{
-	atomic_inc(&ib_conn->refcount);
-}
-
-int iser_conn_put(struct iser_conn *ib_conn, int can_destroy_id)
-{
-	if (atomic_dec_and_test(&ib_conn->refcount)) {
-		iser_conn_release(ib_conn, can_destroy_id);
-		return 1;
-	}
-	return 0;
-}
-
 /**
  * triggers start of the disconnect procedures and wait for them to be done
  */
@@ -638,24 +640,19 @@ void iser_conn_terminate(struct iser_conn *ib_conn)
 	if (err)
 		iser_err("Failed to disconnect, conn: 0x%p err %d\n",
 			 ib_conn,err);
-
-	wait_event_interruptible(ib_conn->wait,
-				 ib_conn->state == ISER_CONN_DOWN);
-
-	iser_conn_put(ib_conn, 1); /* deref ib conn deallocate */
 }
 
-static int iser_connect_error(struct rdma_cm_id *cma_id)
+static void iser_connect_error(struct rdma_cm_id *cma_id)
 {
 	struct iser_conn *ib_conn;
+
 	ib_conn = (struct iser_conn *)cma_id->context;
 
 	ib_conn->state = ISER_CONN_DOWN;
 	wake_up_interruptible(&ib_conn->wait);
-	return iser_conn_put(ib_conn, 0); /* deref ib conn's cma id */
 }
 
-static int iser_addr_handler(struct rdma_cm_id *cma_id)
+static void iser_addr_handler(struct rdma_cm_id *cma_id)
 {
 	struct iser_device *device;
 	struct iser_conn   *ib_conn;
@@ -664,7 +661,8 @@ static int iser_addr_handler(struct rdma_cm_id *cma_id)
 	device = iser_device_find_by_ib_device(cma_id);
 	if (!device) {
 		iser_err("device lookup/creation failed\n");
-		return iser_connect_error(cma_id);
+		iser_connect_error(cma_id);
+		return;
 	}
 
 	ib_conn = (struct iser_conn *)cma_id->context;
@@ -686,13 +684,12 @@ static int iser_addr_handler(struct rdma_cm_id *cma_id)
 	ret = rdma_resolve_route(cma_id, 1000);
 	if (ret) {
 		iser_err("resolve route failed: %d\n", ret);
-		return iser_connect_error(cma_id);
+		iser_connect_error(cma_id);
+		return;
 	}
-
-	return 0;
 }
 
-static int iser_route_handler(struct rdma_cm_id *cma_id)
+static void iser_route_handler(struct rdma_cm_id *cma_id)
 {
 	struct rdma_conn_param conn_param;
 	int    ret;
@@ -720,9 +717,9 @@ static int iser_route_handler(struct rdma_cm_id *cma_id)
 		goto failure;
 	}
 
-	return 0;
+	return;
 failure:
-	return iser_connect_error(cma_id);
+	iser_connect_error(cma_id);
 }
 
 static void iser_connected_handler(struct rdma_cm_id *cma_id)
@@ -739,10 +736,9 @@ static void iser_connected_handler(struct rdma_cm_id *cma_id)
 	wake_up_interruptible(&ib_conn->wait);
 }
 
-static int iser_disconnected_handler(struct rdma_cm_id *cma_id)
+static void iser_disconnected_handler(struct rdma_cm_id *cma_id)
 {
 	struct iser_conn *ib_conn;
-	int ret;
 
 	ib_conn = (struct iser_conn *)cma_id->context;
 
@@ -762,24 +758,19 @@ static int iser_disconnected_handler(struct rdma_cm_id *cma_id)
 		ib_conn->state = ISER_CONN_DOWN;
 		wake_up_interruptible(&ib_conn->wait);
 	}
-
-	ret = iser_conn_put(ib_conn, 0); /* deref ib conn's cma id */
-	return ret;
 }
 
 static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 {
-	int ret = 0;
-
 	iser_info("event %d status %d conn %p id %p\n",
 		  event->event, event->status, cma_id->context, cma_id);
 
 	switch (event->event) {
 	case RDMA_CM_EVENT_ADDR_RESOLVED:
-		ret = iser_addr_handler(cma_id);
+		iser_addr_handler(cma_id);
 		break;
 	case RDMA_CM_EVENT_ROUTE_RESOLVED:
-		ret = iser_route_handler(cma_id);
+		iser_route_handler(cma_id);
 		break;
 	case RDMA_CM_EVENT_ESTABLISHED:
 		iser_connected_handler(cma_id);
@@ -789,18 +780,18 @@ static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve
 	case RDMA_CM_EVENT_CONNECT_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
 	case RDMA_CM_EVENT_REJECTED:
-		ret = iser_connect_error(cma_id);
+		iser_connect_error(cma_id);
 		break;
 	case RDMA_CM_EVENT_DISCONNECTED:
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 	case RDMA_CM_EVENT_ADDR_CHANGE:
-		ret = iser_disconnected_handler(cma_id);
+		iser_disconnected_handler(cma_id);
 		break;
 	default:
 		iser_err("Unexpected RDMA CM event (%d)\n", event->event);
 		break;
 	}
-	return ret;
+	return 0;
 }
 
 void iser_conn_init(struct iser_conn *ib_conn)
@@ -809,7 +800,7 @@ void iser_conn_init(struct iser_conn *ib_conn)
 	init_waitqueue_head(&ib_conn->wait);
 	ib_conn->post_recv_buf_count = 0;
 	atomic_set(&ib_conn->post_send_buf_count, 0);
-	atomic_set(&ib_conn->refcount, 1); /* ref ib conn allocation */
+	init_completion(&ib_conn->stop_completion);
 	INIT_LIST_HEAD(&ib_conn->conn_list);
 	spin_lock_init(&ib_conn->lock);
 }
@@ -837,7 +828,6 @@ int iser_connect(struct iser_conn   *ib_conn,
 
 	ib_conn->state = ISER_CONN_PENDING;
 
-	iser_conn_get(ib_conn); /* ref ib conn's cma id */
 	ib_conn->cma_id = rdma_create_id(iser_cma_handler,
 					     (void *)ib_conn,
 					     RDMA_PS_TCP, IB_QPT_RC);
@@ -874,9 +864,8 @@ id_failure:
 	ib_conn->cma_id = NULL;
 addr_failure:
 	ib_conn->state = ISER_CONN_DOWN;
-	iser_conn_put(ib_conn, 1); /* deref ib conn's cma id */
 connect_failure:
-	iser_conn_put(ib_conn, 1); /* deref ib conn deallocate */
+	iser_conn_release(ib_conn);
 	return err;
 }
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH for-next 2/4] IB/iser: Fix a possible race in iser connection states transition
       [not found] ` <1400745621-9978-1-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
  2014-05-22  8:00   ` [PATCH for-next 1/4] IB/iser: Simplify connection management Or Gerlitz
@ 2014-05-22  8:00   ` Or Gerlitz
  2014-05-22  8:00   ` [PATCH for-next 3/4] IB/iser: Add missing newlines to logging messages Or Gerlitz
  2014-05-22  8:00   ` [PATCH for-next 4/4] IB/iser: Bump version to 1.4 Or Gerlitz
  3 siblings, 0 replies; 7+ messages in thread
From: Or Gerlitz @ 2014-05-22  8:00 UTC (permalink / raw)
  To: roland-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, roid-VPRAkNaXOzVWk0Htik3J/w,
	sagig-H+wXaHxf7aLQT0dZR+AlfA, oren-VPRAkNaXOzVWk0Htik3J/w,
	arieln-VPRAkNaXOzVWk0Htik3J/w, Sagi Grimberg

From: Ariel Nahum <arieln-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

In some circumstances (multiple targets), RDMA_CM ESTABLISHED event and
ep_disconnect may race. In this case, the iser connection state may
transition to UP (after ep_disconnect transitioned it to TERMINATING),
while the connection is being teared down.

Upon RDMA_CM event ESTABLISHED we allow iser connection state to transition
to UP only from PENDING. We also make sure to protect this state change
(done under the connection lock).

Signed-off-by: Ariel Nahum <arieln-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iser_verbs.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 4c698e5..ea01075 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -732,8 +732,8 @@ static void iser_connected_handler(struct rdma_cm_id *cma_id)
 	iser_info("remote qpn:%x my qpn:%x\n", attr.dest_qp_num, cma_id->qp->qp_num);
 
 	ib_conn = (struct iser_conn *)cma_id->context;
-	ib_conn->state = ISER_CONN_UP;
-	wake_up_interruptible(&ib_conn->wait);
+	if (iser_conn_state_comp_exch(ib_conn, ISER_CONN_PENDING, ISER_CONN_UP))
+		wake_up_interruptible(&ib_conn->wait);
 }
 
 static void iser_disconnected_handler(struct rdma_cm_id *cma_id)
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH for-next 3/4] IB/iser: Add missing newlines to logging messages
       [not found] ` <1400745621-9978-1-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
  2014-05-22  8:00   ` [PATCH for-next 1/4] IB/iser: Simplify connection management Or Gerlitz
  2014-05-22  8:00   ` [PATCH for-next 2/4] IB/iser: Fix a possible race in iser connection states transition Or Gerlitz
@ 2014-05-22  8:00   ` Or Gerlitz
  2014-05-22  8:00   ` [PATCH for-next 4/4] IB/iser: Bump version to 1.4 Or Gerlitz
  3 siblings, 0 replies; 7+ messages in thread
From: Or Gerlitz @ 2014-05-22  8:00 UTC (permalink / raw)
  To: roland-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, roid-VPRAkNaXOzVWk0Htik3J/w,
	sagig-H+wXaHxf7aLQT0dZR+AlfA, oren-VPRAkNaXOzVWk0Htik3J/w,
	arieln-VPRAkNaXOzVWk0Htik3J/w, Joe Perches, Or Gerlitz

From: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Logging messages need terminating newlines to avoid
possible message interleaving.  Add them.

Signed-off-by: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index f217488..eb79739 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -508,28 +508,28 @@ iscsi_iser_set_param(struct iscsi_cls_conn *cls_conn,
 	case ISCSI_PARAM_HDRDGST_EN:
 		sscanf(buf, "%d", &value);
 		if (value) {
-			iser_err("DataDigest wasn't negotiated to None");
+			iser_err("DataDigest wasn't negotiated to None\n");
 			return -EPROTO;
 		}
 		break;
 	case ISCSI_PARAM_DATADGST_EN:
 		sscanf(buf, "%d", &value);
 		if (value) {
-			iser_err("DataDigest wasn't negotiated to None");
+			iser_err("DataDigest wasn't negotiated to None\n");
 			return -EPROTO;
 		}
 		break;
 	case ISCSI_PARAM_IFMARKER_EN:
 		sscanf(buf, "%d", &value);
 		if (value) {
-			iser_err("IFMarker wasn't negotiated to No");
+			iser_err("IFMarker wasn't negotiated to No\n");
 			return -EPROTO;
 		}
 		break;
 	case ISCSI_PARAM_OFMARKER_EN:
 		sscanf(buf, "%d", &value);
 		if (value) {
-			iser_err("OFMarker wasn't negotiated to No");
+			iser_err("OFMarker wasn't negotiated to No\n");
 			return -EPROTO;
 		}
 		break;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH for-next 4/4] IB/iser: Bump version to 1.4
       [not found] ` <1400745621-9978-1-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
                     ` (2 preceding siblings ...)
  2014-05-22  8:00   ` [PATCH for-next 3/4] IB/iser: Add missing newlines to logging messages Or Gerlitz
@ 2014-05-22  8:00   ` Or Gerlitz
  3 siblings, 0 replies; 7+ messages in thread
From: Or Gerlitz @ 2014-05-22  8:00 UTC (permalink / raw)
  To: roland-DgEjT+Ai2ygdnm+yROfE0A
  Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, roid-VPRAkNaXOzVWk0Htik3J/w,
	sagig-H+wXaHxf7aLQT0dZR+AlfA, oren-VPRAkNaXOzVWk0Htik3J/w,
	arieln-VPRAkNaXOzVWk0Htik3J/w, Or Gerlitz, Sagi Grimberg

From: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index d309620..97cd385 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -69,7 +69,7 @@
 
 #define DRV_NAME	"iser"
 #define PFX		DRV_NAME ": "
-#define DRV_VER		"1.3"
+#define DRV_VER		"1.4"
 
 #define iser_dbg(fmt, arg...)				\
 	do {						\
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH for-next 1/4] IB/iser: Simplify connection management
       [not found]     ` <1400745621-9978-2-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
@ 2014-05-22  8:09       ` Or Gerlitz
       [not found]         ` <537DB0C8.4070805-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Or Gerlitz @ 2014-05-22  8:09 UTC (permalink / raw)
  To: Mike Christie
  Cc: roland-DgEjT+Ai2ygdnm+yROfE0A, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	roid-VPRAkNaXOzVWk0Htik3J/w, sagig-H+wXaHxf7aLQT0dZR+AlfA,
	oren-VPRAkNaXOzVWk0Htik3J/w, arieln-VPRAkNaXOzVWk0Htik3J/w,
	Sagi Grimberg

On 22/05/2014 11:00, Or Gerlitz wrote:

sorry for the spam,  I forgot to add Mike Christie, the iscsi 
maintainer, so here you are CC-ed Mike,
I preferred doing it with a single reply vs. a whole new post, Mike if 
you need the actual patch
for the sake of review/looking it's here 
http://marc.info/?l=linux-rdma&m=140074563408534&q=raw

Or.
> From: Ariel Nahum <arieln-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> iSER relies on refcounting to manage iser connections
> establishment and teardown.
>
> Following commit 39ff05d "IB/iser: Enhance disconnection logic
> for multi-pathing", iser connection maintain 3 references:
>
> - iscsi_endpoint (at creation stage)
> - cma_id (at connection request stage)
> - iscsi_conn (at bind stage)
>
> We can avoid taking explicit refcounts by correctly serializing
> iser teardown flows (graceful and non-graceful).
>
> Our approach is to trigger a scheduled work to handle ordered
> teardown by gracefully waiting for 2 cleanup stages to complete:
>
> 1. Cleanup of live pending tasks indicated by iscsi_conn_stop completion
> 2. Flush errors processing
>
> Each completed stage will notify a waiting worker thread when it is
> done to allow teardwon continuation.
>
> Since iSCSI connection establishment may trigger endpoint disconnect without
> a successful endpoint connect, we rely on the iscsi <-> iser binding (.conn_bind)
> to learn about the teardown policy we should take wrt cleanup stages.
>
> Since all cleanup worker threads are scheduled (release_wq) in .ep_disconnect
> it is safe to assume that when module_exit is called, all cleanup workers are
> already scheduled. Thus proper module unload shall flush all scheduled works
> before allowing safe exit, to guarantee no resources got left behind.
>
> Signed-off-by: Ariel Nahum <arieln-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Roi Dayan <roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>   drivers/infiniband/ulp/iser/iscsi_iser.c |   97 +++++++++++++++++------------
>   drivers/infiniband/ulp/iser/iscsi_iser.h |    8 ++-
>   drivers/infiniband/ulp/iser/iser_verbs.c |   85 +++++++++++---------------
>   3 files changed, 99 insertions(+), 91 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index 25f195e..f217488 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -99,6 +99,7 @@ MODULE_PARM_DESC(pi_enable, "Enable T10-PI offload support (default:disabled)");
>   module_param_named(pi_guard, iser_pi_guard, int, 0644);
>   MODULE_PARM_DESC(pi_guard, "T10-PI guard_type, 0:CRC|1:IP_CSUM (default:CRC)");
>   
> +static struct workqueue_struct *release_wq;
>   struct iser_global ig;
>   
>   void
> @@ -337,24 +338,6 @@ iscsi_iser_conn_create(struct iscsi_cls_session *cls_session, uint32_t conn_idx)
>   	return cls_conn;
>   }
>   
> -static void
> -iscsi_iser_conn_destroy(struct iscsi_cls_conn *cls_conn)
> -{
> -	struct iscsi_conn *conn = cls_conn->dd_data;
> -	struct iser_conn *ib_conn = conn->dd_data;
> -
> -	iscsi_conn_teardown(cls_conn);
> -	/*
> -	 * Userspace will normally call the stop callback and
> -	 * already have freed the ib_conn, but if it goofed up then
> -	 * we free it here.
> -	 */
> -	if (ib_conn) {
> -		ib_conn->iscsi_conn = NULL;
> -		iser_conn_put(ib_conn, 1); /* deref iscsi/ib conn unbinding */
> -	}
> -}
> -
>   static int
>   iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
>   		     struct iscsi_cls_conn *cls_conn, uint64_t transport_eph,
> @@ -392,29 +375,39 @@ iscsi_iser_conn_bind(struct iscsi_cls_session *cls_session,
>   	conn->dd_data = ib_conn;
>   	ib_conn->iscsi_conn = conn;
>   
> -	iser_conn_get(ib_conn); /* ref iscsi/ib conn binding */
>   	return 0;
>   }
>   
> +static int
> +iscsi_iser_conn_start(struct iscsi_cls_conn *cls_conn)
> +{
> +	struct iscsi_conn *iscsi_conn;
> +	struct iser_conn *ib_conn;
> +
> +	iscsi_conn = cls_conn->dd_data;
> +	ib_conn = iscsi_conn->dd_data;
> +	reinit_completion(&ib_conn->stop_completion);
> +
> +	return iscsi_conn_start(cls_conn);
> +}
> +
>   static void
>   iscsi_iser_conn_stop(struct iscsi_cls_conn *cls_conn, int flag)
>   {
>   	struct iscsi_conn *conn = cls_conn->dd_data;
>   	struct iser_conn *ib_conn = conn->dd_data;
>   
> +	iser_dbg("stopping iscsi_conn: %p, ib_conn: %p\n", conn, ib_conn);
> +	iscsi_conn_stop(cls_conn, flag);
> +
>   	/*
>   	 * Userspace may have goofed up and not bound the connection or
>   	 * might have only partially setup the connection.
>   	 */
>   	if (ib_conn) {
> -		iscsi_conn_stop(cls_conn, flag);
> -		/*
> -		 * There is no unbind event so the stop callback
> -		 * must release the ref from the bind.
> -		 */
> -		iser_conn_put(ib_conn, 1); /* deref iscsi/ib conn unbinding */
> +		conn->dd_data = NULL;
> +		complete(&ib_conn->stop_completion);
>   	}
> -	conn->dd_data = NULL;
>   }
>   
>   static void iscsi_iser_session_destroy(struct iscsi_cls_session *cls_session)
> @@ -652,19 +645,20 @@ iscsi_iser_ep_disconnect(struct iscsi_endpoint *ep)
>   	struct iser_conn *ib_conn;
>   
>   	ib_conn = ep->dd_data;
> -	if (ib_conn->iscsi_conn)
> -		/*
> -		 * Must suspend xmit path if the ep is bound to the
> -		 * iscsi_conn, so we know we are not accessing the ib_conn
> -		 * when we free it.
> -		 *
> -		 * This may not be bound if the ep poll failed.
> -		 */
> -		iscsi_suspend_tx(ib_conn->iscsi_conn);
> -
> -
> -	iser_info("ib conn %p state %d\n", ib_conn, ib_conn->state);
> +	iser_info("ep %p ib conn %p state %d\n", ep, ib_conn, ib_conn->state);
>   	iser_conn_terminate(ib_conn);
> +
> +	/*
> +	 * if iser_conn and iscsi_conn are bound, we must wait iscsi_conn_stop
> +	 * call and ISER_CONN_DOWN state before freeing the iser resources.
> +	 * otherwise we are safe to free resources immediately.
> +	 */
> +	if (ib_conn->iscsi_conn) {
> +		INIT_WORK(&ib_conn->release_work, iser_release_work);
> +		queue_work(release_wq, &ib_conn->release_work);
> +	} else {
> +		iser_conn_release(ib_conn);
> +	}
>   }
>   
>   static umode_t iser_attr_is_visible(int param_type, int param)
> @@ -748,13 +742,13 @@ static struct iscsi_transport iscsi_iser_transport = {
>   	/* connection management */
>   	.create_conn            = iscsi_iser_conn_create,
>   	.bind_conn              = iscsi_iser_conn_bind,
> -	.destroy_conn           = iscsi_iser_conn_destroy,
> +	.destroy_conn           = iscsi_conn_teardown,
>   	.attr_is_visible	= iser_attr_is_visible,
>   	.set_param              = iscsi_iser_set_param,
>   	.get_conn_param		= iscsi_conn_get_param,
>   	.get_ep_param		= iscsi_iser_get_ep_param,
>   	.get_session_param	= iscsi_session_get_param,
> -	.start_conn             = iscsi_conn_start,
> +	.start_conn             = iscsi_iser_conn_start,
>   	.stop_conn              = iscsi_iser_conn_stop,
>   	/* iscsi host params */
>   	.get_host_param		= iscsi_host_get_param,
> @@ -801,6 +795,12 @@ static int __init iser_init(void)
>   	mutex_init(&ig.connlist_mutex);
>   	INIT_LIST_HEAD(&ig.connlist);
>   
> +	release_wq = alloc_workqueue("release workqueue", 0, 0);
> +	if (!release_wq) {
> +		iser_err("failed to allocate release workqueue\n");
> +		return -ENOMEM;
> +	}
> +
>   	iscsi_iser_scsi_transport = iscsi_register_transport(
>   							&iscsi_iser_transport);
>   	if (!iscsi_iser_scsi_transport) {
> @@ -819,7 +819,24 @@ register_transport_failure:
>   
>   static void __exit iser_exit(void)
>   {
> +	struct iser_conn *ib_conn, *n;
> +	int connlist_empty;
> +
>   	iser_dbg("Removing iSER datamover...\n");
> +	destroy_workqueue(release_wq);
> +
> +	mutex_lock(&ig.connlist_mutex);
> +	connlist_empty = list_empty(&ig.connlist);
> +	mutex_unlock(&ig.connlist_mutex);
> +
> +	if (!connlist_empty) {
> +		iser_err("Error cleanup stage completed but we still have iser "
> +			 "connections, destroying them anyway.\n");
> +		list_for_each_entry_safe(ib_conn, n, &ig.connlist, conn_list) {
> +			iser_conn_release(ib_conn);
> +		}
> +	}
> +
>   	iscsi_unregister_transport(&iscsi_iser_transport);
>   	kmem_cache_destroy(ig.desc_cache);
>   }
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
> index 324129f..d309620 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.h
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
> @@ -333,6 +333,8 @@ struct iser_conn {
>   	int                          post_recv_buf_count; /* posted rx count  */
>   	atomic_t                     post_send_buf_count; /* posted tx count   */
>   	char 			     name[ISER_OBJECT_NAME_SIZE];
> +	struct work_struct	     release_work;
> +	struct completion	     stop_completion;
>   	struct list_head	     conn_list;       /* entry in ig conn list */
>   
>   	char  			     *login_buf;
> @@ -417,12 +419,12 @@ void iscsi_iser_recv(struct iscsi_conn *conn,
>   
>   void iser_conn_init(struct iser_conn *ib_conn);
>   
> -void iser_conn_get(struct iser_conn *ib_conn);
> -
> -int iser_conn_put(struct iser_conn *ib_conn, int destroy_cma_id_allowed);
> +void iser_conn_release(struct iser_conn *ib_conn);
>   
>   void iser_conn_terminate(struct iser_conn *ib_conn);
>   
> +void iser_release_work(struct work_struct *work);
> +
>   void iser_rcv_completion(struct iser_rx_desc *desc,
>   			 unsigned long    dto_xfer_len,
>   			struct iser_conn *ib_conn);
> diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
> index 32849f2..4c698e5 100644
> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
> @@ -581,14 +581,30 @@ static int iser_conn_state_comp_exch(struct iser_conn *ib_conn,
>   	return ret;
>   }
>   
> +void iser_release_work(struct work_struct *work)
> +{
> +	struct iser_conn *ib_conn;
> +
> +	ib_conn = container_of(work, struct iser_conn, release_work);
> +
> +	/* wait for .conn_stop callback */
> +	wait_for_completion(&ib_conn->stop_completion);
> +
> +	/* wait for the qp`s post send and post receive buffers to empty */
> +	wait_event_interruptible(ib_conn->wait,
> +				 ib_conn->state == ISER_CONN_DOWN);
> +
> +	iser_conn_release(ib_conn);
> +}
> +
>   /**
>    * Frees all conn objects and deallocs conn descriptor
>    */
> -static void iser_conn_release(struct iser_conn *ib_conn, int can_destroy_id)
> +void iser_conn_release(struct iser_conn *ib_conn)
>   {
>   	struct iser_device  *device = ib_conn->device;
>   
> -	BUG_ON(ib_conn->state != ISER_CONN_DOWN);
> +	BUG_ON(ib_conn->state == ISER_CONN_UP);
>   
>   	mutex_lock(&ig.connlist_mutex);
>   	list_del(&ib_conn->conn_list);
> @@ -600,27 +616,13 @@ static void iser_conn_release(struct iser_conn *ib_conn, int can_destroy_id)
>   	if (device != NULL)
>   		iser_device_try_release(device);
>   	/* if cma handler context, the caller actually destroy the id */
> -	if (ib_conn->cma_id != NULL && can_destroy_id) {
> +	if (ib_conn->cma_id != NULL) {
>   		rdma_destroy_id(ib_conn->cma_id);
>   		ib_conn->cma_id = NULL;
>   	}
>   	iscsi_destroy_endpoint(ib_conn->ep);
>   }
>   
> -void iser_conn_get(struct iser_conn *ib_conn)
> -{
> -	atomic_inc(&ib_conn->refcount);
> -}
> -
> -int iser_conn_put(struct iser_conn *ib_conn, int can_destroy_id)
> -{
> -	if (atomic_dec_and_test(&ib_conn->refcount)) {
> -		iser_conn_release(ib_conn, can_destroy_id);
> -		return 1;
> -	}
> -	return 0;
> -}
> -
>   /**
>    * triggers start of the disconnect procedures and wait for them to be done
>    */
> @@ -638,24 +640,19 @@ void iser_conn_terminate(struct iser_conn *ib_conn)
>   	if (err)
>   		iser_err("Failed to disconnect, conn: 0x%p err %d\n",
>   			 ib_conn,err);
> -
> -	wait_event_interruptible(ib_conn->wait,
> -				 ib_conn->state == ISER_CONN_DOWN);
> -
> -	iser_conn_put(ib_conn, 1); /* deref ib conn deallocate */
>   }
>   
> -static int iser_connect_error(struct rdma_cm_id *cma_id)
> +static void iser_connect_error(struct rdma_cm_id *cma_id)
>   {
>   	struct iser_conn *ib_conn;
> +
>   	ib_conn = (struct iser_conn *)cma_id->context;
>   
>   	ib_conn->state = ISER_CONN_DOWN;
>   	wake_up_interruptible(&ib_conn->wait);
> -	return iser_conn_put(ib_conn, 0); /* deref ib conn's cma id */
>   }
>   
> -static int iser_addr_handler(struct rdma_cm_id *cma_id)
> +static void iser_addr_handler(struct rdma_cm_id *cma_id)
>   {
>   	struct iser_device *device;
>   	struct iser_conn   *ib_conn;
> @@ -664,7 +661,8 @@ static int iser_addr_handler(struct rdma_cm_id *cma_id)
>   	device = iser_device_find_by_ib_device(cma_id);
>   	if (!device) {
>   		iser_err("device lookup/creation failed\n");
> -		return iser_connect_error(cma_id);
> +		iser_connect_error(cma_id);
> +		return;
>   	}
>   
>   	ib_conn = (struct iser_conn *)cma_id->context;
> @@ -686,13 +684,12 @@ static int iser_addr_handler(struct rdma_cm_id *cma_id)
>   	ret = rdma_resolve_route(cma_id, 1000);
>   	if (ret) {
>   		iser_err("resolve route failed: %d\n", ret);
> -		return iser_connect_error(cma_id);
> +		iser_connect_error(cma_id);
> +		return;
>   	}
> -
> -	return 0;
>   }
>   
> -static int iser_route_handler(struct rdma_cm_id *cma_id)
> +static void iser_route_handler(struct rdma_cm_id *cma_id)
>   {
>   	struct rdma_conn_param conn_param;
>   	int    ret;
> @@ -720,9 +717,9 @@ static int iser_route_handler(struct rdma_cm_id *cma_id)
>   		goto failure;
>   	}
>   
> -	return 0;
> +	return;
>   failure:
> -	return iser_connect_error(cma_id);
> +	iser_connect_error(cma_id);
>   }
>   
>   static void iser_connected_handler(struct rdma_cm_id *cma_id)
> @@ -739,10 +736,9 @@ static void iser_connected_handler(struct rdma_cm_id *cma_id)
>   	wake_up_interruptible(&ib_conn->wait);
>   }
>   
> -static int iser_disconnected_handler(struct rdma_cm_id *cma_id)
> +static void iser_disconnected_handler(struct rdma_cm_id *cma_id)
>   {
>   	struct iser_conn *ib_conn;
> -	int ret;
>   
>   	ib_conn = (struct iser_conn *)cma_id->context;
>   
> @@ -762,24 +758,19 @@ static int iser_disconnected_handler(struct rdma_cm_id *cma_id)
>   		ib_conn->state = ISER_CONN_DOWN;
>   		wake_up_interruptible(&ib_conn->wait);
>   	}
> -
> -	ret = iser_conn_put(ib_conn, 0); /* deref ib conn's cma id */
> -	return ret;
>   }
>   
>   static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
>   {
> -	int ret = 0;
> -
>   	iser_info("event %d status %d conn %p id %p\n",
>   		  event->event, event->status, cma_id->context, cma_id);
>   
>   	switch (event->event) {
>   	case RDMA_CM_EVENT_ADDR_RESOLVED:
> -		ret = iser_addr_handler(cma_id);
> +		iser_addr_handler(cma_id);
>   		break;
>   	case RDMA_CM_EVENT_ROUTE_RESOLVED:
> -		ret = iser_route_handler(cma_id);
> +		iser_route_handler(cma_id);
>   		break;
>   	case RDMA_CM_EVENT_ESTABLISHED:
>   		iser_connected_handler(cma_id);
> @@ -789,18 +780,18 @@ static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve
>   	case RDMA_CM_EVENT_CONNECT_ERROR:
>   	case RDMA_CM_EVENT_UNREACHABLE:
>   	case RDMA_CM_EVENT_REJECTED:
> -		ret = iser_connect_error(cma_id);
> +		iser_connect_error(cma_id);
>   		break;
>   	case RDMA_CM_EVENT_DISCONNECTED:
>   	case RDMA_CM_EVENT_DEVICE_REMOVAL:
>   	case RDMA_CM_EVENT_ADDR_CHANGE:
> -		ret = iser_disconnected_handler(cma_id);
> +		iser_disconnected_handler(cma_id);
>   		break;
>   	default:
>   		iser_err("Unexpected RDMA CM event (%d)\n", event->event);
>   		break;
>   	}
> -	return ret;
> +	return 0;
>   }
>   
>   void iser_conn_init(struct iser_conn *ib_conn)
> @@ -809,7 +800,7 @@ void iser_conn_init(struct iser_conn *ib_conn)
>   	init_waitqueue_head(&ib_conn->wait);
>   	ib_conn->post_recv_buf_count = 0;
>   	atomic_set(&ib_conn->post_send_buf_count, 0);
> -	atomic_set(&ib_conn->refcount, 1); /* ref ib conn allocation */
> +	init_completion(&ib_conn->stop_completion);
>   	INIT_LIST_HEAD(&ib_conn->conn_list);
>   	spin_lock_init(&ib_conn->lock);
>   }
> @@ -837,7 +828,6 @@ int iser_connect(struct iser_conn   *ib_conn,
>   
>   	ib_conn->state = ISER_CONN_PENDING;
>   
> -	iser_conn_get(ib_conn); /* ref ib conn's cma id */
>   	ib_conn->cma_id = rdma_create_id(iser_cma_handler,
>   					     (void *)ib_conn,
>   					     RDMA_PS_TCP, IB_QPT_RC);
> @@ -874,9 +864,8 @@ id_failure:
>   	ib_conn->cma_id = NULL;
>   addr_failure:
>   	ib_conn->state = ISER_CONN_DOWN;
> -	iser_conn_put(ib_conn, 1); /* deref ib conn's cma id */
>   connect_failure:
> -	iser_conn_put(ib_conn, 1); /* deref ib conn deallocate */
> +	iser_conn_release(ib_conn);
>   	return err;
>   }
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH for-next 1/4] IB/iser: Simplify connection management
       [not found]         ` <537DB0C8.4070805-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2014-05-28 23:47           ` Mike Christie
  0 siblings, 0 replies; 7+ messages in thread
From: Mike Christie @ 2014-05-28 23:47 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: roland-DgEjT+Ai2ygdnm+yROfE0A, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	roid-VPRAkNaXOzVWk0Htik3J/w, oren-VPRAkNaXOzVWk0Htik3J/w,
	arieln-VPRAkNaXOzVWk0Htik3J/w, Sagi Grimberg

On 05/22/2014 03:09 AM, Or Gerlitz wrote:
> On 22/05/2014 11:00, Or Gerlitz wrote:
> 
> sorry for the spam,  I forgot to add Mike Christie, the iscsi
> maintainer, so here you are CC-ed Mike,
> I preferred doing it with a single reply vs. a whole new post, Mike if
> you need the actual patch
> for the sake of review/looking it's here
> http://marc.info/?l=linux-rdma&m=140074563408534&q=raw
> 

The patch looks ok to me.

Reviewed-by: Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-05-28 23:47 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-22  8:00 [PATCH for-next 0/4] iSER initiator updates for 3.16 Or Gerlitz
     [not found] ` <1400745621-9978-1-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
2014-05-22  8:00   ` [PATCH for-next 1/4] IB/iser: Simplify connection management Or Gerlitz
     [not found]     ` <1400745621-9978-2-git-send-email-ogerlitz-lZu6o6FoHf8DyJZ7l8Lk7nI+JuX82XLG@public.gmane.org>
2014-05-22  8:09       ` Or Gerlitz
     [not found]         ` <537DB0C8.4070805-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2014-05-28 23:47           ` Mike Christie
2014-05-22  8:00   ` [PATCH for-next 2/4] IB/iser: Fix a possible race in iser connection states transition Or Gerlitz
2014-05-22  8:00   ` [PATCH for-next 3/4] IB/iser: Add missing newlines to logging messages Or Gerlitz
2014-05-22  8:00   ` [PATCH for-next 4/4] IB/iser: Bump version to 1.4 Or Gerlitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.