From 7733ce346a1d5319ef4009537e190af2098e96c4 Mon Sep 17 00:00:00 2001 From: Yan Burman Date: Tue, 18 Nov 2014 12:07:35 +0000 Subject: [PATCH] Merged revisions 5859, 5861-5864, 5866-5868, 5871 from iser branch [r5871] by yanb123 isert: Fix page leak if alloc_page fails [r5868] by yanb123 isert: Do not leak workqueues if ib_create_cq failed [r5867] by yanb123 isert: Document performance considerations for iSER [r5866] by yanb123 isert: Fix use-after-free when killing iscsi-scstd Our portal may be destroyed while there are connections alive. This means we are doing list_del() from list_head that no longer exists [r5864] by yanb123 isert: Fix crash on device removal when iscsi-scstd is started with explicit address [r5863] by yanb123 isert: Make sure we decrement CQ QP counters if QP creation fails [r5862] by yanb123 isert: Fix closing devices when connection allocation fails We need to dereference isert_device structures whether we created them or not if connection creation fails [r5861] by yanb123 isert: Warn user of potential compilation issue [r5859] by yanb123 isert: Fix resource leak upon unreachable event Unreachable event caused by crash of initiator while in connection establishment flow, would cause leak of connections. git-svn-id: http://svn.code.sf.net/p/scst/svn/branches/3.0.x-iser@5877 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- iscsi-scst/README.iser | 6 ++ iscsi-scst/kernel/isert-scst/iser.h | 7 ++ iscsi-scst/kernel/isert-scst/iser_buf.c | 2 +- iscsi-scst/kernel/isert-scst/iser_rdma.c | 112 ++++++++++++++++++----- 4 files changed, 102 insertions(+), 25 deletions(-) diff --git a/iscsi-scst/README.iser b/iscsi-scst/README.iser index 360f31155..ea5a57511 100644 --- a/iscsi-scst/README.iser +++ b/iscsi-scst/README.iser @@ -15,6 +15,12 @@ In order to achieve better performance, it is recommended to specify "QueuedCommands 128" parameter per iSER target, since the transport is very fast and you usually want to connect it to fast backstorage. +For performance tuning of initiator and target machines, see +http://community.mellanox.com/docs/DOC-1483 + +Note that if you have an SSD controller that is close to a particular +NUMA node, you want the HCA to be close to the same node. + Limitations: ------------- * Bidirectional commands are not supported diff --git a/iscsi-scst/kernel/isert-scst/iser.h b/iscsi-scst/kernel/isert-scst/iser.h index d4cde7d98..95cdc4ec5 100644 --- a/iscsi-scst/kernel/isert-scst/iser.h +++ b/iscsi-scst/kernel/isert-scst/iser.h @@ -9,12 +9,18 @@ #include "iser_hdr.h" +enum isert_portal_state { + ISERT_PORTAL_ACTIVE, + ISERT_PORTAL_INACTIVE +}; + struct isert_portal { struct rdma_cm_id *cm_id; struct sockaddr_storage addr; struct list_head list_node; /* in portals list */ /* protected by dev_list_mutex */ struct list_head conn_list; /* head of conns list */ + enum isert_portal_state state; }; struct isert_buf { @@ -165,6 +171,7 @@ struct isert_connection { struct isert_wr drain_wr; struct kref kref; + struct isert_portal *portal; void *priv_data; /* for connection tracking */ }; diff --git a/iscsi-scst/kernel/isert-scst/iser_buf.c b/iscsi-scst/kernel/isert-scst/iser_buf.c index 8d74f1bff..32daeb377 100644 --- a/iscsi-scst/kernel/isert-scst/iser_buf.c +++ b/iscsi-scst/kernel/isert-scst/iser_buf.c @@ -84,7 +84,7 @@ static int isert_buf_alloc_pg(struct ib_device *ib_dev, goto out; out_map_failed: - for (; i > 0; --i) + for (; i >= 0; --i) __free_page(sg_page(&isert_buf->sg[i])); kfree(isert_buf->sg); isert_buf->sg = NULL; diff --git a/iscsi-scst/kernel/isert-scst/iser_rdma.c b/iscsi-scst/kernel/isert-scst/iser_rdma.c index 95e93332f..0c669e611 100644 --- a/iscsi-scst/kernel/isert-scst/iser_rdma.c +++ b/iscsi-scst/kernel/isert-scst/iser_rdma.c @@ -45,6 +45,8 @@ static DEFINE_MUTEX(dev_list_mutex); +void isert_portal_free(struct isert_portal *portal); + static int isert_num_recv_posted_on_err(struct ib_recv_wr *first_ib_wr, struct ib_recv_wr *bad_wr) { @@ -813,6 +815,11 @@ static struct isert_device *isert_device_create(struct ib_device *ib_dev) cqe_num = min(isert_dev->device_attr.max_cqe, ISER_CQ_ENTRIES); cqe_num = cqe_num / isert_dev->num_cqs; +#ifdef CONFIG_SCST_EXTRACHECKS + if (isert_dev->device_attr.max_cqe == 0) + pr_err("Zero max_cqe encountered: you may have a compilation problem\n"); +#endif + for (i = 0; i < isert_dev->num_cqs; ++i) { struct isert_cq *cq_desc = &isert_dev->cq_desc[i]; @@ -852,6 +859,7 @@ static struct isert_device *isert_device_create(struct ib_device *ib_dev) cqe_num, i); /* completion vector */ if (IS_ERR(cq)) { + cq_desc->cq = NULL; err = PTR_ERR(cq); pr_err("Failed to create iser dev cq, err:%d\n", err); goto fail_cq; @@ -879,7 +887,7 @@ static struct isert_device *isert_device_create(struct ib_device *ib_dev) return isert_dev; fail_cq: - for (j = 0; j < i; ++j) { + for (j = 0; j <= i; ++j) { if (isert_dev->cq_desc[j].cq) ib_destroy_cq(isert_dev->cq_desc[j].cq); if (isert_dev->cq_desc[j].cq_workqueue) @@ -1009,7 +1017,7 @@ static int isert_conn_qp_create(struct isert_connection *isert_conn) err = rdma_create_qp(cm_id, isert_dev->pd, &qp_attr); if (unlikely(err)) { pr_err("Failed to create qp, err:%d\n", err); - goto out; + goto fail_create_qp; } isert_conn->qp = cm_id->qp; @@ -1018,6 +1026,12 @@ static int isert_conn_qp_create(struct isert_connection *isert_conn) out: TRACE_EXIT_RES(err); return err; + +fail_create_qp: + mutex_lock(&dev_list_mutex); + isert_dev->cq_qps[cq_idx]--; + mutex_unlock(&dev_list_mutex); + goto out; } static void isert_conn_qp_destroy(struct isert_connection *isert_conn) @@ -1031,6 +1045,7 @@ static struct isert_connection *isert_conn_create(struct rdma_cm_id *cm_id, { struct isert_connection *isert_conn; int err; + struct isert_cq *cq; TRACE_ENTRY(); @@ -1082,12 +1097,15 @@ static struct isert_connection *isert_conn_create(struct rdma_cm_id *cm_id, } kref_init(&isert_conn->kref); - kref_get(&isert_conn->kref); TRACE_EXIT(); return isert_conn; fail_post_recv: + cq = isert_conn->qp->recv_cq->cq_context; + mutex_lock(&dev_list_mutex); + isert_dev->cq_qps[cq->idx]--; + mutex_unlock(&dev_list_mutex); isert_conn_qp_destroy(isert_conn); fail_qp: isert_pdu_free(isert_conn->login_rsp_pdu); @@ -1102,6 +1120,13 @@ fail_get: return ERR_PTR(err); } +static void isert_deref_device(struct isert_device *isert_dev) +{ + isert_dev->refcnt--; + if (isert_dev->refcnt == 0) + isert_device_release(isert_dev); +} + static void isert_kref_free(struct kref *kref) { struct isert_connection *isert_conn = container_of(kref, @@ -1122,9 +1147,9 @@ static void isert_kref_free(struct kref *kref) isert_dev->cq_qps[cq->idx]--; list_del(&isert_conn->portal_node); list_del(&isert_conn->dev_node); - isert_dev->refcnt--; - if (isert_dev->refcnt == 0) - isert_device_release(isert_dev); + isert_deref_device(isert_dev); + if (unlikely(isert_conn->portal->state == ISERT_PORTAL_INACTIVE)) + isert_portal_free(isert_conn->portal); mutex_unlock(&dev_list_mutex); rdma_destroy_id(isert_conn->cm_id); @@ -1186,7 +1211,6 @@ static int isert_cm_conn_req_handler(struct rdma_cm_id *cm_id, /* passed in rdma_create_id */ struct isert_portal *portal = cm_id->context; struct ib_device *ib_dev = cm_id->device; - struct isert_device *new_isert_dev = NULL; struct isert_device *isert_dev; struct isert_connection *isert_conn; struct rdma_conn_param *ini_conn_param; @@ -1198,13 +1222,12 @@ static int isert_cm_conn_req_handler(struct rdma_cm_id *cm_id, mutex_lock(&dev_list_mutex); isert_dev = isert_device_find(ib_dev); if (!isert_dev) { - new_isert_dev = isert_device_create(ib_dev); - if (unlikely(IS_ERR(new_isert_dev))) { - err = PTR_ERR(new_isert_dev); + isert_dev = isert_device_create(ib_dev); + if (unlikely(IS_ERR(isert_dev))) { + err = PTR_ERR(isert_dev); mutex_unlock(&dev_list_mutex); goto fail_dev_create; } - isert_dev = new_isert_dev; } isert_dev->refcnt++; mutex_unlock(&dev_list_mutex); @@ -1216,6 +1239,7 @@ static int isert_cm_conn_req_handler(struct rdma_cm_id *cm_id, } isert_conn->state = ISER_CONN_HANDSHAKE; + isert_conn->portal = portal; mutex_lock(&dev_list_mutex); list_add_tail(&isert_conn->portal_node, &portal->conn_list); @@ -1287,13 +1311,9 @@ fail_accept: goto out; fail_conn_create: - if (new_isert_dev) { - mutex_lock(&dev_list_mutex); - new_isert_dev->refcnt--; - if (new_isert_dev->refcnt == 0) - isert_device_release(new_isert_dev); - mutex_unlock(&dev_list_mutex); - } + mutex_lock(&dev_list_mutex); + isert_deref_device(isert_dev); + mutex_unlock(&dev_list_mutex); fail_dev_create: rdma_reject(cm_id, NULL, 0); goto out; @@ -1318,6 +1338,7 @@ static int isert_cm_connect_handler(struct rdma_cm_id *cm_id, if (unlikely(ret)) goto out; + kref_get(&isert_conn->kref); kref_get(&isert_conn->kref); /* notify upper layer */ ret = isert_conn_established(&isert_conn->iscsi, @@ -1396,6 +1417,31 @@ static int isert_handle_failure(struct isert_connection *conn) return 0; } +static int isert_cm_evt_listener_handler(struct rdma_cm_id *cm_id, + struct rdma_cm_event *cm_ev) +{ + enum rdma_cm_event_type ev_type; + struct isert_portal *portal; + int err = 0; + + ev_type = cm_ev->event; + portal = cm_id->context; + + switch (ev_type) { + case RDMA_CM_EVENT_DEVICE_REMOVAL: + portal->cm_id = NULL; + err = -EINVAL; + break; + + default: + pr_info("Listener event:%s(%d), ignored\n", + cm_event_type_str(ev_type), ev_type); + break; + } + + return err; +} + static int isert_cm_evt_handler(struct rdma_cm_id *cm_id, struct rdma_cm_event *cm_ev) { @@ -1415,6 +1461,11 @@ static int isert_cm_evt_handler(struct rdma_cm_id *cm_id, cm_event_type_str(ev_type), ev_type, cm_ev->status, portal, cm_id); + if (portal->cm_id == cm_id) { + err = isert_cm_evt_listener_handler(cm_id, cm_ev); + goto out; + } + switch (ev_type) { case RDMA_CM_EVENT_CONNECT_REQUEST: err = isert_cm_conn_req_handler(cm_id, cm_ev); @@ -1591,23 +1642,36 @@ out: return err; } +void isert_portal_free(struct isert_portal *portal) +{ + lockdep_assert_held(&dev_list_mutex); + + if (!list_empty(&portal->conn_list)) + return; + + kfree(portal); + module_put(THIS_MODULE); +} + void isert_portal_release(struct isert_portal *portal) { struct isert_connection *conn; pr_info("iser portal cm_id:%p releasing\n", portal->cm_id); - rdma_destroy_id(portal->cm_id); + if (portal->cm_id) { + rdma_destroy_id(portal->cm_id); + portal->cm_id = NULL; + } + + isert_portal_list_remove(portal); mutex_lock(&dev_list_mutex); list_for_each_entry(conn, &portal->conn_list, portal_node) isert_conn_disconnect(conn); + portal->state = ISERT_PORTAL_INACTIVE; + isert_portal_free(portal); mutex_unlock(&dev_list_mutex); - - isert_portal_list_remove(portal); - - kfree(portal); - module_put(THIS_MODULE); } struct isert_portal *isert_portal_start(struct sockaddr *sa, size_t addr_len)