scsi: Serialize LUN reset execution

Task management functions don't require performance. Serializing
execution of task managment functions will sipmlify and stabilize
the logic and will be helpful for upcoming patches to support
other task management functions.

This patch introduces two queues, pending and submitted queue,
and serializes LUN reset exection and makes LUN hot removal
wait for LUN reset.

Besides, checking if LUN is NULL is moved to the upper layer as
same as IO task submission.

Change-Id: Ia0cf3f437a745ee70fc9b17744cc63c833690dda
Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-on: https://review.gerrithub.io/434764
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>

lib/iscsi/iscsi.c

@@ -3221,6 +3221,13 @@ spdk_iscsi_op_task(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu)
 	task->tag = task_tag;
 	task->scsi.lun = spdk_scsi_dev_get_lun(dev, lun_i);
 
+	if (task->scsi.lun == NULL) {
+		task->scsi.response = SPDK_SCSI_TASK_MGMT_RESP_INVALID_LUN;
+		spdk_iscsi_task_mgmt_response(conn, task);
+		spdk_iscsi_task_put(task);
+		return 0;
+	}
+
 	switch (function) {
 	/* abort task identified by Referenced Task Tag field */
 	case ISCSI_TASK_FUNC_ABORT_TASK:

lib/scsi/dev.c

@@ -252,7 +252,8 @@ spdk_scsi_dev_queue_mgmt_task(struct spdk_scsi_dev *dev,
 	assert(task != NULL);
 
 	task->function = func;
-	spdk_scsi_lun_task_mgmt_execute(task);
+	spdk_scsi_lun_append_mgmt_task(task->lun, task);
+	spdk_scsi_lun_execute_mgmt_task(task->lun);
 }
 
 void
@@ -406,7 +407,9 @@ spdk_scsi_dev_has_pending_tasks(const struct spdk_scsi_dev *dev)
 	int i;
 
 	for (i = 0; i < SPDK_SCSI_DEV_MAX_LUN; ++i) {
-		if (dev->lun[i] && spdk_scsi_lun_has_pending_tasks(dev->lun[i])) {
+		if (dev->lun[i] &&
+		    (spdk_scsi_lun_has_pending_tasks(dev->lun[i]) ||
+		     spdk_scsi_lun_has_pending_mgmt_tasks(dev->lun[i]))) {
 			return true;
 		}
 	}

lib/scsi/lun.c

@@ -49,6 +49,18 @@ spdk_scsi_lun_complete_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *ta
 	task->cpl_fn(task);
 }
 
+static void
+spdk_scsi_lun_complete_mgmt_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task)
+{
+	TAILQ_REMOVE(&lun->mgmt_tasks, task, scsi_link);
+
+	task->cpl_fn(task);
+
+	/* Try to execute the first pending mgmt task if it exists. */
+	spdk_scsi_lun_execute_mgmt_task(lun);
+}
+
+/* This will be called in the callback to the bdev reset function. */
 void
 spdk_scsi_lun_complete_reset_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task)
 {
@@ -64,30 +76,14 @@ spdk_scsi_lun_complete_reset_task(struct spdk_scsi_lun *lun, struct spdk_scsi_ta
 		}
 	}
 
-	task->cpl_fn(task);
+	spdk_scsi_lun_complete_mgmt_task(lun, task);
 }
 
 static void
-spdk_scsi_lun_complete_mgmt_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task)
+_spdk_scsi_lun_execute_mgmt_task(struct spdk_scsi_lun *lun,
+				 struct spdk_scsi_task *task)
 {
-	assert(task->function != SPDK_SCSI_TASK_FUNC_LUN_RESET);
+	TAILQ_INSERT_TAIL(&lun->mgmt_tasks, task, scsi_link);
-
-	task->cpl_fn(task);
-}
-
-int
-spdk_scsi_lun_task_mgmt_execute(struct spdk_scsi_task *task)
-{
-	if (!task) {
-		return -1;
-	}
-
-	if (!task->lun) {
-		/* LUN does not exist */
-		task->response = SPDK_SCSI_TASK_MGMT_RESP_INVALID_LUN;
-		task->cpl_fn(task);
-		return -1;
-	}
 
 	switch (task->function) {
 	case SPDK_SCSI_TASK_FUNC_ABORT_TASK:
@@ -102,7 +98,7 @@ spdk_scsi_lun_task_mgmt_execute(struct spdk_scsi_task *task)
 
 	case SPDK_SCSI_TASK_FUNC_LUN_RESET:
 		spdk_bdev_scsi_reset(task);
-		return 0;
+		return;
 
 	default:
 		SPDK_ERRLOG("Unknown Task Management Function!\n");
@@ -115,9 +111,32 @@ spdk_scsi_lun_task_mgmt_execute(struct spdk_scsi_task *task)
 		break;
 	}
 
-	spdk_scsi_lun_complete_mgmt_task(task->lun, task);
+	spdk_scsi_lun_complete_mgmt_task(lun, task);
+}
 
-	return -1;
+void
+spdk_scsi_lun_append_mgmt_task(struct spdk_scsi_lun *lun,
+			       struct spdk_scsi_task *task)
+{
+	TAILQ_INSERT_TAIL(&lun->pending_mgmt_tasks, task, scsi_link);
+}
+
+void
+spdk_scsi_lun_execute_mgmt_task(struct spdk_scsi_lun *lun)
+{
+	struct spdk_scsi_task *task;
+
+	if (!TAILQ_EMPTY(&lun->mgmt_tasks)) {
+		return;
+	}
+
+	task = TAILQ_FIRST(&lun->pending_mgmt_tasks);
+	if (task == NULL) {
+		return;
+	}
+	TAILQ_REMOVE(&lun->pending_mgmt_tasks, task, scsi_link);
+
+	_spdk_scsi_lun_execute_mgmt_task(lun, task);
 }
 
 void
@@ -241,7 +260,8 @@ spdk_scsi_lun_check_pending_tasks(void *arg)
 {
 	struct spdk_scsi_lun *lun = (struct spdk_scsi_lun *)arg;
 
-	if (spdk_scsi_lun_has_pending_tasks(lun)) {
+	if (spdk_scsi_lun_has_pending_tasks(lun) ||
+	    spdk_scsi_lun_has_pending_mgmt_tasks(lun)) {
 		return -1;
 	}
 	spdk_poller_unregister(&lun->hotremove_poller);
@@ -255,7 +275,8 @@ _spdk_scsi_lun_hot_remove(void *arg1)
 {
 	struct spdk_scsi_lun *lun = arg1;
 
-	if (spdk_scsi_lun_has_pending_tasks(lun)) {
+	if (spdk_scsi_lun_has_pending_tasks(lun) ||
+	    spdk_scsi_lun_has_pending_mgmt_tasks(lun)) {
 		lun->hotremove_poller = spdk_poller_register(spdk_scsi_lun_check_pending_tasks,
 					lun, 10);
 	} else {
@@ -323,6 +344,8 @@ spdk_scsi_lun_construct(struct spdk_bdev *bdev,
 	}
 
 	TAILQ_INIT(&lun->tasks);
+	TAILQ_INIT(&lun->mgmt_tasks);
+	TAILQ_INIT(&lun->pending_mgmt_tasks);
 
 	lun->bdev = bdev;
 	lun->io_channel = NULL;
@@ -446,6 +469,13 @@ spdk_scsi_lun_get_dev(const struct spdk_scsi_lun *lun)
 	return lun->dev;
 }
 
+bool
+spdk_scsi_lun_has_pending_mgmt_tasks(const struct spdk_scsi_lun *lun)
+{
+	return !TAILQ_EMPTY(&lun->pending_mgmt_tasks) ||
+	       !TAILQ_EMPTY(&lun->mgmt_tasks);
+}
+
 bool
 spdk_scsi_lun_has_pending_tasks(const struct spdk_scsi_lun *lun)
 {

lib/scsi/scsi_internal.h

@@ -117,6 +117,13 @@ struct spdk_scsi_lun {
 
 	/** pending tasks */
 	TAILQ_HEAD(tasks, spdk_scsi_task) tasks;
+
+	/** submitted management tasks */
+	TAILQ_HEAD(mgmt_tasks, spdk_scsi_task) mgmt_tasks;
+
+	/** pending management tasks */
+	TAILQ_HEAD(pending_mgmt_tasks, spdk_scsi_task) pending_mgmt_tasks;
+
 };
 
 struct spdk_lun_db_entry {
@@ -137,7 +144,9 @@ _spdk_scsi_lun *spdk_scsi_lun_construct(struct spdk_bdev *bdev,
 void spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun);
 
 void spdk_scsi_lun_execute_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task);
-int spdk_scsi_lun_task_mgmt_execute(struct spdk_scsi_task *task);
+void spdk_scsi_lun_append_mgmt_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task);
+void spdk_scsi_lun_execute_mgmt_task(struct spdk_scsi_lun *lun);
+bool spdk_scsi_lun_has_pending_mgmt_tasks(const struct spdk_scsi_lun *lun);
 void spdk_scsi_lun_complete_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task);
 void spdk_scsi_lun_complete_reset_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task);
 bool spdk_scsi_lun_has_pending_tasks(const struct spdk_scsi_lun *lun);

test/unit/lib/scsi/dev.c/dev_ut.c

@@ -111,10 +111,20 @@ spdk_bdev_get_by_name(const char *bdev_name)
 	return NULL;
 }
 
-int
+void
-spdk_scsi_lun_task_mgmt_execute(struct spdk_scsi_task *task)
+spdk_scsi_lun_append_mgmt_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task)
 {
-	return 0;
+}
+
+void
+spdk_scsi_lun_execute_mgmt_task(struct spdk_scsi_lun *lun)
+{
+}
+
+bool
+spdk_scsi_lun_has_pending_mgmt_tasks(const struct spdk_scsi_lun *lun)
+{
+	return false;
 }
 
 void

test/unit/lib/scsi/lun.c/lun_ut.c

@@ -163,7 +163,10 @@ void spdk_scsi_dev_delete_lun(struct spdk_scsi_dev *dev,
 void
 spdk_bdev_scsi_reset(struct spdk_scsi_task *task)
 {
-	return;
+	task->status = SPDK_SCSI_STATUS_GOOD;
+	task->response = SPDK_SCSI_TASK_MGMT_RESP_SUCCESS;
+
+	spdk_scsi_lun_complete_reset_task(task->lun, task);
 }
 
 int
@@ -219,37 +222,6 @@ lun_destruct(struct spdk_scsi_lun *lun)
 	spdk_scsi_lun_destruct(lun);
 }
 
-static void
-lun_task_mgmt_execute_null_task(void)
-{
-	int rc;
-
-	rc = spdk_scsi_lun_task_mgmt_execute(NULL);
-
-	/* returns -1 since we passed NULL for the task */
-	CU_ASSERT_TRUE(rc < 0);
-	CU_ASSERT_EQUAL(g_task_count, 0);
-}
-
-static void
-lun_task_mgmt_execute_abort_task_null_lun_failure(void)
-{
-	struct spdk_scsi_task mgmt_task = { 0 };
-	struct spdk_scsi_port initiator_port = { 0 };
-	int rc;
-
-	ut_init_task(&mgmt_task);
-	mgmt_task.lun = NULL;
-	mgmt_task.initiator_port = &initiator_port;
-	mgmt_task.function = SPDK_SCSI_TASK_FUNC_ABORT_TASK;
-
-	rc = spdk_scsi_lun_task_mgmt_execute(&mgmt_task);
-
-	/* returns -1 since we passed NULL for LUN */
-	CU_ASSERT_TRUE(rc < 0);
-	CU_ASSERT_EQUAL(g_task_count, 0);
-}
-
 static void
 lun_task_mgmt_execute_abort_task_not_supported(void)
 {
@@ -259,7 +231,6 @@ lun_task_mgmt_execute_abort_task_not_supported(void)
 	struct spdk_scsi_port initiator_port = { 0 };
 	struct spdk_scsi_dev dev = { 0 };
 	uint8_t cdb[6] = { 0 };
-	int rc;
 
 	lun = lun_construct();
 	lun->dev = &dev;
@@ -279,10 +250,10 @@ lun_task_mgmt_execute_abort_task_not_supported(void)
 	/* task should now be on the tasks list */
 	CU_ASSERT(!TAILQ_EMPTY(&lun->tasks));
 
-	rc = spdk_scsi_lun_task_mgmt_execute(&mgmt_task);
+	spdk_scsi_lun_append_mgmt_task(lun, &mgmt_task);
+	spdk_scsi_lun_execute_mgmt_task(lun);
 
-	/* returns -1 since task abort is not supported */
+	/* task abort is not supported */
-	CU_ASSERT_TRUE(rc < 0);
 	CU_ASSERT(mgmt_task.response == SPDK_SCSI_TASK_MGMT_RESP_REJECT_FUNC_NOT_SUPPORTED);
 
 	/* task is still on the tasks list */
@@ -294,26 +265,6 @@ lun_task_mgmt_execute_abort_task_not_supported(void)
 	lun_destruct(lun);
 }
 
-static void
-lun_task_mgmt_execute_abort_task_all_null_lun_failure(void)
-{
-	struct spdk_scsi_task mgmt_task = { 0 };
-	struct spdk_scsi_port initiator_port = { 0 };
-	int rc;
-
-	ut_init_task(&mgmt_task);
-	mgmt_task.lun = NULL;
-	mgmt_task.initiator_port = &initiator_port;
-	mgmt_task.function = SPDK_SCSI_TASK_FUNC_ABORT_TASK_SET;
-
-	rc = spdk_scsi_lun_task_mgmt_execute(&mgmt_task);
-
-	/* Returns -1 since we passed NULL for lun */
-	CU_ASSERT_TRUE(rc < 0);
-
-	CU_ASSERT_EQUAL(g_task_count, 0);
-}
-
 static void
 lun_task_mgmt_execute_abort_task_all_not_supported(void)
 {
@@ -322,7 +273,6 @@ lun_task_mgmt_execute_abort_task_all_not_supported(void)
 	struct spdk_scsi_task mgmt_task = { 0 };
 	struct spdk_scsi_port initiator_port = { 0 };
 	struct spdk_scsi_dev dev = { 0 };
-	int rc;
 	uint8_t cdb[6] = { 0 };
 
 	lun = lun_construct();
@@ -344,10 +294,10 @@ lun_task_mgmt_execute_abort_task_all_not_supported(void)
 	/* task should now be on the tasks list */
 	CU_ASSERT(!TAILQ_EMPTY(&lun->tasks));
 
-	rc = spdk_scsi_lun_task_mgmt_execute(&mgmt_task);
+	spdk_scsi_lun_append_mgmt_task(lun, &mgmt_task);
+	spdk_scsi_lun_execute_mgmt_task(lun);
 
-	/* returns -1 since task abort is not supported */
+	/* task abort is not supported */
-	CU_ASSERT_TRUE(rc < 0);
 	CU_ASSERT(mgmt_task.response == SPDK_SCSI_TASK_MGMT_RESP_REJECT_FUNC_NOT_SUPPORTED);
 
 	/* task is still on the tasks list */
@@ -360,31 +310,12 @@ lun_task_mgmt_execute_abort_task_all_not_supported(void)
 	lun_destruct(lun);
 }
 
-static void
-lun_task_mgmt_execute_lun_reset_failure(void)
-{
-	struct spdk_scsi_task mgmt_task = { 0 };
-	int rc;
-
-	ut_init_task(&mgmt_task);
-	mgmt_task.lun = NULL;
-	mgmt_task.function = SPDK_SCSI_TASK_FUNC_LUN_RESET;
-
-	rc = spdk_scsi_lun_task_mgmt_execute(&mgmt_task);
-
-	/* Returns -1 since we passed NULL for lun */
-	CU_ASSERT_TRUE(rc < 0);
-
-	CU_ASSERT_EQUAL(g_task_count, 0);
-}
-
 static void
 lun_task_mgmt_execute_lun_reset(void)
 {
 	struct spdk_scsi_lun *lun;
 	struct spdk_scsi_task mgmt_task = { 0 };
 	struct spdk_scsi_dev dev = { 0 };
-	int rc;
 
 	lun = lun_construct();
 	lun->dev = &dev;
@@ -393,16 +324,16 @@ lun_task_mgmt_execute_lun_reset(void)
 	mgmt_task.lun = lun;
 	mgmt_task.function = SPDK_SCSI_TASK_FUNC_LUN_RESET;
 
-	rc = spdk_scsi_lun_task_mgmt_execute(&mgmt_task);
+	spdk_scsi_lun_append_mgmt_task(lun, &mgmt_task);
+	spdk_scsi_lun_execute_mgmt_task(lun);
 
 	/* Returns success */
-	CU_ASSERT_EQUAL(rc, 0);
+	CU_ASSERT_EQUAL(mgmt_task.status, SPDK_SCSI_STATUS_GOOD);
+	CU_ASSERT_EQUAL(mgmt_task.response, SPDK_SCSI_TASK_MGMT_RESP_SUCCESS);
 
 	lun_destruct(lun);
 
-	/* task is still on the tasks list */
+	CU_ASSERT_EQUAL(g_task_count, 0);
-	CU_ASSERT_EQUAL(g_task_count, 1);
-	g_task_count = 0;
 }
 
 static void
@@ -411,7 +342,6 @@ lun_task_mgmt_execute_invalid_case(void)
 	struct spdk_scsi_lun *lun;
 	struct spdk_scsi_task mgmt_task = { 0 };
 	struct spdk_scsi_dev dev = { 0 };
-	int rc;
 
 	lun = lun_construct();
 	lun->dev = &dev;
@@ -420,10 +350,11 @@ lun_task_mgmt_execute_invalid_case(void)
 	mgmt_task.function = 5;
 
 	/* Pass an invalid value to the switch statement */
-	rc = spdk_scsi_lun_task_mgmt_execute(&mgmt_task);
+	spdk_scsi_lun_append_mgmt_task(lun, &mgmt_task);
+	spdk_scsi_lun_execute_mgmt_task(lun);
 
-	/* Returns -1 on passing an invalid value to the switch case */
+	/* function code is invalid */
-	CU_ASSERT_TRUE(rc < 0);
+	CU_ASSERT_EQUAL(mgmt_task.response, SPDK_SCSI_TASK_MGMT_RESP_REJECT_FUNC_NOT_SUPPORTED);
 
 	lun_destruct(lun);
 
@@ -620,18 +551,10 @@ main(int argc, char **argv)
 	}
 
 	if (
-		CU_add_test(suite, "task management - null task failure",
+		CU_add_test(suite, "task management abort task - not supported",
-			    lun_task_mgmt_execute_null_task) == NULL
+			    lun_task_mgmt_execute_abort_task_not_supported) == NULL
-		|| CU_add_test(suite, "task management abort task - null lun failure",
-			       lun_task_mgmt_execute_abort_task_null_lun_failure) == NULL
-		|| CU_add_test(suite, "task management abort task - not supported",
-			       lun_task_mgmt_execute_abort_task_not_supported) == NULL
-		|| CU_add_test(suite, "task management abort task set - null lun failure",
-			       lun_task_mgmt_execute_abort_task_all_null_lun_failure) == NULL
 		|| CU_add_test(suite, "task management abort task set - success",
 			       lun_task_mgmt_execute_abort_task_all_not_supported) == NULL
-		|| CU_add_test(suite, "task management - lun reset failure",
-			       lun_task_mgmt_execute_lun_reset_failure) == NULL
 		|| CU_add_test(suite, "task management - lun reset success",
 			       lun_task_mgmt_execute_lun_reset) == NULL
 		|| CU_add_test(suite, "task management - invalid option",