From 04c6347b4e23753e7f5a6c96841193904795f119 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Mon, 29 Jan 2018 16:51:51 +0900 Subject: [PATCH] iscsi: Consolidate checking uniqueness of PG into register/unregister Checking uniqueness of portal group is done without mutex and before register/unregister. This is not thread-safe. Hence this patch is added to ensure PG uniqueness. A little related refactoring is also done. Change-Id: Iaa3b5e380f2be5cfdaa2d69f9f2763c98954b0c3 Signed-off-by: Shuhei Matsumoto Reviewed-on: https://review.gerrithub.io/396847 Tested-by: SPDK Automated Test System Reviewed-by: Jim Harris Reviewed-by: Daniel Verkamp --- lib/iscsi/iscsi_rpc.c | 2 +- lib/iscsi/portal_grp.c | 64 +++++++++-------- lib/iscsi/portal_grp.h | 1 + .../lib/iscsi/portal_grp.c/portal_grp_ut.c | 72 +++++++++++++++++++ 4 files changed, 108 insertions(+), 31 deletions(-) diff --git a/lib/iscsi/iscsi_rpc.c b/lib/iscsi/iscsi_rpc.c index 18e9f51ae..a02d8e98f 100644 --- a/lib/iscsi/iscsi_rpc.c +++ b/lib/iscsi/iscsi_rpc.c @@ -896,7 +896,7 @@ spdk_rpc_delete_portal_group(struct spdk_jsonrpc_request *request, goto invalid; } - pg = spdk_iscsi_portal_grp_find_by_tag(req.tag); + pg = spdk_iscsi_portal_grp_unregister(req.tag); if (!pg) { goto invalid; } diff --git a/lib/iscsi/portal_grp.c b/lib/iscsi/portal_grp.c index e4f8aefdd..761905598 100644 --- a/lib/iscsi/portal_grp.c +++ b/lib/iscsi/portal_grp.c @@ -336,13 +336,6 @@ spdk_iscsi_portal_grp_create(int tag) return NULL; } - /* Make sure there are no duplicate portal group tags */ - if (spdk_iscsi_portal_grp_find_by_tag(tag)) { - SPDK_ERRLOG("portal group creation failed. duplicate portal group tag (%d)\n", tag); - free(pg); - return NULL; - } - pg->ref = 0; pg->tag = tag; @@ -367,15 +360,23 @@ spdk_iscsi_portal_grp_destroy(struct spdk_iscsi_portal_grp *pg) free(pg); } -static void +static int spdk_iscsi_portal_grp_register(struct spdk_iscsi_portal_grp *pg) { + int rc = -1; + struct spdk_iscsi_portal_grp *tmp; + assert(pg != NULL); assert(!TAILQ_EMPTY(&pg->head)); pthread_mutex_lock(&g_spdk_iscsi.mutex); - TAILQ_INSERT_TAIL(&g_spdk_iscsi.pg_head, pg, tailq); + tmp = spdk_iscsi_portal_grp_find_by_tag(pg->tag); + if (tmp == NULL) { + TAILQ_INSERT_TAIL(&g_spdk_iscsi.pg_head, pg, tailq); + rc = 0; + } pthread_mutex_unlock(&g_spdk_iscsi.mutex); + return rc; } static void @@ -440,7 +441,10 @@ spdk_iscsi_portal_grp_create_from_portal_list(int tag, } /* Add portal group to the end of the pg list */ - spdk_iscsi_portal_grp_register(pg); + rc = spdk_iscsi_portal_grp_register(pg); + if (rc != 0) { + spdk_iscsi_portal_grp_destroy(pg); + } } return rc; @@ -478,20 +482,20 @@ spdk_iscsi_portal_grp_create_from_configfile(struct spdk_conf_section *sp) rc = spdk_iscsi_portal_create_from_configline(portal, &p, 1); if (rc < 0) { SPDK_ERRLOG("parse portal error (%s)\n", portal); - goto error_out; + return -1; } } portals = i; if (portals > MAX_PORTAL) { SPDK_ERRLOG("%d > MAX_PORTAL\n", portals); - goto error_out; + return -1; } pg = spdk_iscsi_portal_grp_create(spdk_conf_section_get_num(sp)); if (!pg) { SPDK_ERRLOG("portal group malloc error (%s)\n", spdk_conf_section_get_name(sp)); - goto error_out; + return -1; } for (i = 0; i < portals; i++) { @@ -500,14 +504,14 @@ spdk_iscsi_portal_grp_create_from_configfile(struct spdk_conf_section *sp) if (label == NULL || portal == NULL) { spdk_iscsi_portal_grp_destroy(pg); SPDK_ERRLOG("portal error\n"); - goto error_out; + return -1; } rc = spdk_iscsi_portal_create_from_configline(portal, &p, 0); if (rc < 0) { spdk_iscsi_portal_grp_destroy(pg); SPDK_ERRLOG("parse portal error (%s)\n", portal); - goto error_out; + return -1; } SPDK_DEBUGLOG(SPDK_LOG_ISCSI, @@ -518,12 +522,14 @@ spdk_iscsi_portal_grp_create_from_configfile(struct spdk_conf_section *sp) } /* Add portal group to the end of the pg list */ - spdk_iscsi_portal_grp_register(pg); + rc = spdk_iscsi_portal_grp_register(pg); + if (rc != 0) { + SPDK_ERRLOG("register portal failed\n"); + spdk_iscsi_portal_grp_destroy(pg); + return -1; + } return 0; - -error_out: - return -1; } struct spdk_iscsi_portal_grp * @@ -642,28 +648,26 @@ spdk_iscsi_portal_grp_close_all(void) pthread_mutex_unlock(&g_spdk_iscsi.mutex); } -static inline void -spdk_iscsi_portal_grp_unregister(struct spdk_iscsi_portal_grp *pg) +struct spdk_iscsi_portal_grp * +spdk_iscsi_portal_grp_unregister(int tag) { - struct spdk_iscsi_portal_grp *portal_group; - struct spdk_iscsi_portal_grp *portal_group_tmp; - - assert(pg != NULL); - assert(!TAILQ_EMPTY(&pg->head)); + struct spdk_iscsi_portal_grp *pg; pthread_mutex_lock(&g_spdk_iscsi.mutex); - TAILQ_FOREACH_SAFE(portal_group, &g_spdk_iscsi.pg_head, tailq, portal_group_tmp) { - if (portal_group->tag == pg->tag) { - TAILQ_REMOVE(&g_spdk_iscsi.pg_head, portal_group, tailq); + TAILQ_FOREACH(pg, &g_spdk_iscsi.pg_head, tailq) { + if (pg->tag == tag) { + TAILQ_REMOVE(&g_spdk_iscsi.pg_head, pg, tailq); + pthread_mutex_unlock(&g_spdk_iscsi.mutex); + return pg; } } pthread_mutex_unlock(&g_spdk_iscsi.mutex); + return NULL; } void spdk_iscsi_portal_grp_release(struct spdk_iscsi_portal_grp *pg) { spdk_iscsi_portal_grp_close(pg); - spdk_iscsi_portal_grp_unregister(pg); spdk_iscsi_portal_grp_destroy(pg); } diff --git a/lib/iscsi/portal_grp.h b/lib/iscsi/portal_grp.h index 52286668f..ce030d86b 100644 --- a/lib/iscsi/portal_grp.h +++ b/lib/iscsi/portal_grp.h @@ -68,6 +68,7 @@ int spdk_iscsi_portal_grp_create_from_portal_list(int tag, void spdk_iscsi_portal_grp_release(struct spdk_iscsi_portal_grp *pg); int spdk_iscsi_portal_grp_array_create(void); void spdk_iscsi_portal_grp_array_destroy(void); +struct spdk_iscsi_portal_grp *spdk_iscsi_portal_grp_unregister(int tag); struct spdk_iscsi_portal_grp *spdk_iscsi_portal_grp_find_by_tag(int tag); int spdk_iscsi_portal_grp_open_all(void); diff --git a/test/unit/lib/iscsi/portal_grp.c/portal_grp_ut.c b/test/unit/lib/iscsi/portal_grp.c/portal_grp_ut.c index 2a0454a9a..8e401198d 100644 --- a/test/unit/lib/iscsi/portal_grp.c/portal_grp_ut.c +++ b/test/unit/lib/iscsi/portal_grp.c/portal_grp_ut.c @@ -45,6 +45,7 @@ static int test_setup(void) { TAILQ_INIT(&g_spdk_iscsi.portal_head); + TAILQ_INIT(&g_spdk_iscsi.pg_head); pthread_mutex_init(&g_spdk_iscsi.mutex, NULL); return 0; } @@ -303,6 +304,73 @@ portal_create_from_configline_ipv6_skip_port_and_cpumask_case(void) CU_ASSERT(TAILQ_EMPTY(&g_spdk_iscsi.portal_head)); } +static void +portal_grp_register_unregister_case(void) +{ + struct spdk_iscsi_portal *p; + struct spdk_iscsi_portal_grp *pg1, *pg2; + int rc; + const char *host = "192.168.2.0"; + const char *port = "3260"; + const char *cpumask = "1"; + + pg1 = spdk_iscsi_portal_grp_create(1); + CU_ASSERT(pg1 != NULL); + + p = spdk_iscsi_portal_create(host, port, cpumask); + CU_ASSERT(p != NULL); + + spdk_iscsi_portal_grp_add_portal(pg1, p); + + rc = spdk_iscsi_portal_grp_register(pg1); + CU_ASSERT(rc == 0); + + pg2 = spdk_iscsi_portal_grp_unregister(1); + CU_ASSERT(pg2 != NULL); + CU_ASSERT(pg1 == pg2); + + CU_ASSERT(TAILQ_EMPTY(&g_spdk_iscsi.pg_head)); + + spdk_iscsi_portal_grp_destroy(pg1); + + CU_ASSERT(TAILQ_EMPTY(&g_spdk_iscsi.portal_head)); +} + +static void +portal_grp_register_twice_case(void) +{ + struct spdk_iscsi_portal *p; + struct spdk_iscsi_portal_grp *pg1, *pg2; + int rc; + const char *host = "192.168.2.0"; + const char *port = "3260"; + const char *cpumask = "1"; + + pg1 = spdk_iscsi_portal_grp_create(1); + CU_ASSERT(pg1 != NULL); + + p = spdk_iscsi_portal_create(host, port, cpumask); + CU_ASSERT(p != NULL); + + spdk_iscsi_portal_grp_add_portal(pg1, p); + + rc = spdk_iscsi_portal_grp_register(pg1); + CU_ASSERT(rc == 0); + + rc = spdk_iscsi_portal_grp_register(pg1); + CU_ASSERT(rc != 0); + + pg2 = spdk_iscsi_portal_grp_unregister(1); + CU_ASSERT(pg2 != NULL); + CU_ASSERT(pg1 == pg2); + + CU_ASSERT(TAILQ_EMPTY(&g_spdk_iscsi.pg_head)); + + spdk_iscsi_portal_grp_destroy(pg1); + + CU_ASSERT(TAILQ_EMPTY(&g_spdk_iscsi.portal_head)); +} + int main(int argc, char **argv) { @@ -346,6 +414,10 @@ main(int argc, char **argv) portal_create_from_configline_ipv4_skip_port_and_cpumask_case) == NULL || CU_add_test(suite, "portal create from configline ipv6 skip port and cpumask case", portal_create_from_configline_ipv6_skip_port_and_cpumask_case) == NULL + || CU_add_test(suite, "portal group register/unregister case", + portal_grp_register_unregister_case) == NULL + || CU_add_test(suite, "portal group register twice case", + portal_grp_register_twice_case) == NULL ) { CU_cleanup_registry(); return CU_get_error();