From 6dd09113e51e8c5bd24d9d2c9569507e565ced37 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Wed, 28 Nov 2018 09:47:45 +0900 Subject: [PATCH] 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 Reviewed-on: https://review.gerrithub.io/434764 Reviewed-by: Ben Walker Reviewed-by: Jim Harris Chandler-Test-Pool: SPDK Automated Test System Tested-by: SPDK CI Jenkins --- lib/iscsi/iscsi.c | 7 ++ lib/scsi/dev.c | 7 +- lib/scsi/lun.c | 80 +++++++++++++------- lib/scsi/scsi_internal.h | 11 ++- test/unit/lib/scsi/dev.c/dev_ut.c | 16 +++- test/unit/lib/scsi/lun.c/lun_ut.c | 119 ++++++------------------------ 6 files changed, 111 insertions(+), 129 deletions(-) diff --git a/lib/iscsi/iscsi.c b/lib/iscsi/iscsi.c index 31af178f4..1b4b6402d 100644 --- a/lib/iscsi/iscsi.c +++ b/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: diff --git a/lib/scsi/dev.c b/lib/scsi/dev.c index cdac1b74d..424902f57 100644 --- a/lib/scsi/dev.c +++ b/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; } } diff --git a/lib/scsi/lun.c b/lib/scsi/lun.c index c43f355b0..d88409baf 100644 --- a/lib/scsi/lun.c +++ b/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); - - 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; - } + TAILQ_INSERT_TAIL(&lun->mgmt_tasks, task, scsi_link); 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) { diff --git a/lib/scsi/scsi_internal.h b/lib/scsi/scsi_internal.h index 3afca34f3..eecd7db65 100644 --- a/lib/scsi/scsi_internal.h +++ b/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); diff --git a/test/unit/lib/scsi/dev.c/dev_ut.c b/test/unit/lib/scsi/dev.c/dev_ut.c index 83a453361..21fb50533 100644 --- a/test/unit/lib/scsi/dev.c/dev_ut.c +++ b/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 -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) { - 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 diff --git a/test/unit/lib/scsi/lun.c/lun_ut.c b/test/unit/lib/scsi/lun.c/lun_ut.c index dcd81ac0f..c2698975f 100644 --- a/test/unit/lib/scsi/lun.c/lun_ut.c +++ b/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 */ - CU_ASSERT_TRUE(rc < 0); + /* task abort is not supported */ 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 */ - CU_ASSERT_TRUE(rc < 0); + /* task abort is not supported */ 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, 1); - g_task_count = 0; + CU_ASSERT_EQUAL(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 */ - CU_ASSERT_TRUE(rc < 0); + /* function code is invalid */ + 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", - lun_task_mgmt_execute_null_task) == 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 - not supported", + lun_task_mgmt_execute_abort_task_not_supported) == 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",