Commit 04c6347b authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Jim Harris
Browse files

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: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-on: https://review.gerrithub.io/396847


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
parent 95006504

lib/iscsi/iscsi_rpc.c

+1 −1
Original line number Diff line number Diff line
@@ -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;

	}

lib/iscsi/portal_grp.c

+34 −30
Original line number Diff line number Diff line
@@ -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);

	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);

}

lib/iscsi/portal_grp.h

+1 −0
Original line number Diff line number Diff line
@@ -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);

test/unit/lib/iscsi/portal_grp.c/portal_grp_ut.c

+72 −0
Original line number Diff line number Diff line
@@ -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();