diff --git a/lib/scsi/dev.c b/lib/scsi/dev.c index fa0f35288..1221d84a8 100644 --- a/lib/scsi/dev.c +++ b/lib/scsi/dev.c @@ -64,28 +64,42 @@ allocate_dev(void) static void free_dev(struct spdk_scsi_dev *dev) { + assert(dev->is_allocated == 1); + assert(dev->removed == true); + dev->is_allocated = 0; } void spdk_scsi_dev_destruct(struct spdk_scsi_dev *dev) { + int lun_cnt; int i; - if (dev == NULL) { + if (dev == NULL || dev->removed) { return; } + dev->removed = true; + lun_cnt = 0; + for (i = 0; i < SPDK_SCSI_DEV_MAX_LUN; i++) { if (dev->lun[i] == NULL) { continue; } + /* + * LUN will remove itself from this dev when all outstanding IO + * is done. When no more LUNs, dev will be deleted. + */ spdk_scsi_lun_destruct(dev->lun[i]); - dev->lun[i] = NULL; + lun_cnt++; } - free_dev(dev); + if (lun_cnt == 0) { + free_dev(dev); + return; + } } static void @@ -101,12 +115,21 @@ void spdk_scsi_dev_delete_lun(struct spdk_scsi_dev *dev, struct spdk_scsi_lun *lun) { + int lun_cnt = 0; int i; for (i = 0; i < SPDK_SCSI_DEV_MAX_LUN; i++) { if (dev->lun[i] == lun) { dev->lun[i] = NULL; } + + if (dev->lun[i]) { + lun_cnt++; + } + } + + if (dev->removed == true && lun_cnt == 0) { + free_dev(dev); } } diff --git a/lib/scsi/lun.c b/lib/scsi/lun.c index 0f3549893..a125f4db9 100644 --- a/lib/scsi/lun.c +++ b/lib/scsi/lun.c @@ -188,7 +188,12 @@ spdk_scsi_lun_hotplug(void *arg) if (!spdk_scsi_lun_has_pending_tasks(lun)) { spdk_scsi_lun_free_io_channel(lun); - spdk_scsi_lun_delete(lun); + + spdk_bdev_close(lun->bdev_desc); + spdk_poller_unregister(&lun->hotplug_poller); + + spdk_scsi_dev_delete_lun(lun->dev, lun); + free(lun); } } @@ -197,12 +202,15 @@ _spdk_scsi_lun_hot_remove(void *arg1) { struct spdk_scsi_lun *lun = arg1; - lun->removed = true; if (lun->hotremove_cb) { lun->hotremove_cb(lun, lun->hotremove_ctx); } - lun->hotplug_poller = spdk_poller_register(spdk_scsi_lun_hotplug, lun, 0); + if (spdk_scsi_lun_has_pending_tasks(lun)) { + lun->hotplug_poller = spdk_poller_register(spdk_scsi_lun_hotplug, lun, 10); + } else { + spdk_scsi_lun_hotplug(lun); + } } static void @@ -211,6 +219,11 @@ spdk_scsi_lun_hot_remove(void *remove_ctx) struct spdk_scsi_lun *lun = (struct spdk_scsi_lun *)remove_ctx; struct spdk_thread *thread; + if (lun->removed) { + return; + } + + lun->removed = true; if (lun->io_channel == NULL) { _spdk_scsi_lun_hot_remove(lun); return; @@ -269,35 +282,10 @@ spdk_scsi_lun_construct(struct spdk_bdev *bdev, return lun; } -int +void spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun) { - spdk_bdev_close(lun->bdev_desc); - spdk_poller_unregister(&lun->hotplug_poller); - - free(lun); - - return 0; -} - -int -spdk_scsi_lun_delete(struct spdk_scsi_lun *lun) -{ - struct spdk_scsi_dev *dev; - - pthread_mutex_lock(&g_spdk_scsi.mutex); - - dev = lun->dev; - - /* Remove the LUN from the device */ - if (dev != NULL) { - spdk_scsi_dev_delete_lun(dev, lun); - } - - /* Destroy this lun */ - spdk_scsi_lun_destruct(lun); - pthread_mutex_unlock(&g_spdk_scsi.mutex); - return 0; + spdk_scsi_lun_hot_remove(lun); } int spdk_scsi_lun_allocate_io_channel(struct spdk_scsi_lun *lun) diff --git a/lib/scsi/scsi_internal.h b/lib/scsi/scsi_internal.h index 94d1c3d82..075668bd5 100644 --- a/lib/scsi/scsi_internal.h +++ b/lib/scsi/scsi_internal.h @@ -60,6 +60,7 @@ struct spdk_scsi_port { struct spdk_scsi_dev { int id; int is_allocated; + bool removed; char name[SPDK_SCSI_DEV_MAX_NAME + 1]; @@ -125,13 +126,12 @@ typedef struct spdk_scsi_lun _spdk_scsi_lun; _spdk_scsi_lun *spdk_scsi_lun_construct(struct spdk_bdev *bdev, void (*hotremove_cb)(const struct spdk_scsi_lun *, void *), void *hotremove_ctx); -int spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun); +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, enum spdk_scsi_task_func func); void spdk_scsi_lun_complete_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task); void spdk_scsi_lun_complete_mgmt_task(struct spdk_scsi_lun *lun, struct spdk_scsi_task *task); -int spdk_scsi_lun_delete(struct spdk_scsi_lun *lun); int spdk_scsi_lun_allocate_io_channel(struct spdk_scsi_lun *lun); void spdk_scsi_lun_free_io_channel(struct spdk_scsi_lun *lun); 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 af6f3be29..9a4264876 100644 --- a/test/unit/lib/scsi/dev.c/dev_ut.c +++ b/test/unit/lib/scsi/dev.c/dev_ut.c @@ -91,11 +91,10 @@ spdk_scsi_lun_construct(struct spdk_bdev *bdev, return lun; } -int +void spdk_scsi_lun_destruct(struct spdk_scsi_lun *lun) { free(lun); - return 0; } struct spdk_bdev * @@ -150,7 +149,7 @@ dev_destruct_null_dev(void) static void dev_destruct_zero_luns(void) { - struct spdk_scsi_dev dev = { 0 }; + struct spdk_scsi_dev dev = { .is_allocated = 1 }; /* No luns attached to the dev */ @@ -161,7 +160,7 @@ dev_destruct_zero_luns(void) static void dev_destruct_null_lun(void) { - struct spdk_scsi_dev dev = { 0 }; + struct spdk_scsi_dev dev = { .is_allocated = 1 }; /* pass null for the lun */ dev.lun[0] = NULL; @@ -173,13 +172,13 @@ dev_destruct_null_lun(void) static void dev_destruct_success(void) { - struct spdk_scsi_dev dev = { 0 }; + struct spdk_scsi_dev dev = { .is_allocated = 1 }; struct spdk_scsi_lun *lun; lun = calloc(1, sizeof(struct spdk_scsi_lun)); /* dev with a single lun */ - dev.lun[0] = lun; + spdk_scsi_dev_add_lun(&dev, lun, 0); /* free the dev */ spdk_scsi_dev_destruct(&dev); diff --git a/test/unit/lib/scsi/lun.c/lun_ut.c b/test/unit/lib/scsi/lun.c/lun_ut.c index 2dc81b68b..37ec333c4 100644 --- a/test/unit/lib/scsi/lun.c/lun_ut.c +++ b/test/unit/lib/scsi/lun.c/lun_ut.c @@ -215,6 +215,9 @@ lun_construct(void) static void lun_destruct(struct spdk_scsi_lun *lun) { + /* LUN will defer its removal if there are any unfinished tasks */ + SPDK_CU_ASSERT_FATAL(TAILQ_EMPTY(&lun->tasks)); + spdk_scsi_lun_destruct(lun); } @@ -282,11 +285,13 @@ lun_task_mgmt_execute_abort_task_not_supported(void) CU_ASSERT_TRUE(rc < 0); CU_ASSERT(mgmt_task.response == SPDK_SCSI_TASK_MGMT_RESP_REJECT_FUNC_NOT_SUPPORTED); - lun_destruct(lun); - /* task is still on the tasks list */ CU_ASSERT_EQUAL(g_task_count, 1); - g_task_count = 0; + + spdk_scsi_lun_complete_task(lun, &task); + CU_ASSERT_EQUAL(g_task_count, 0); + + lun_destruct(lun); } static void @@ -343,11 +348,14 @@ lun_task_mgmt_execute_abort_task_all_not_supported(void) CU_ASSERT_TRUE(rc < 0); CU_ASSERT(mgmt_task.response == SPDK_SCSI_TASK_MGMT_RESP_REJECT_FUNC_NOT_SUPPORTED); - lun_destruct(lun); - /* task is still on the tasks list */ CU_ASSERT_EQUAL(g_task_count, 1); - g_task_count = 0; + + spdk_scsi_lun_complete_task(lun, &task); + + CU_ASSERT_EQUAL(g_task_count, 0); + + lun_destruct(lun); } static void @@ -511,11 +519,15 @@ lun_execute_scsi_task_pending(void) /* Assert the task has been successfully added to the tasks queue */ CU_ASSERT(!TAILQ_EMPTY(&lun->tasks)); - lun_destruct(lun); - /* task is still on the tasks list */ CU_ASSERT_EQUAL(g_task_count, 1); - g_task_count = 0; + + /* Need to complete task so LUN might be removed right now */ + spdk_scsi_lun_complete_task(lun, &task); + + CU_ASSERT_EQUAL(g_task_count, 0); + + lun_destruct(lun); } static void @@ -553,13 +565,11 @@ static void lun_destruct_success(void) { struct spdk_scsi_lun *lun; - int rc; lun = lun_construct(); - rc = spdk_scsi_lun_destruct(lun); + spdk_scsi_lun_destruct(lun); - CU_ASSERT_EQUAL(rc, 0); CU_ASSERT_EQUAL(g_task_count, 0); } @@ -585,18 +595,6 @@ lun_construct_success(void) CU_ASSERT_EQUAL(g_task_count, 0); } -static void -lun_delete(void) -{ - struct spdk_scsi_lun *lun; - int rc; - - lun = lun_construct(); - - rc = spdk_scsi_lun_delete(lun); - CU_ASSERT_EQUAL(rc, 0); -} - int main(int argc, char **argv) { @@ -644,7 +642,6 @@ main(int argc, char **argv) || CU_add_test(suite, "destruct task - success", lun_destruct_success) == NULL || CU_add_test(suite, "construct - null ctx", lun_construct_null_ctx) == NULL || CU_add_test(suite, "construct - success", lun_construct_success) == NULL - || CU_add_test(suite, "lun_delete", lun_delete) == NULL ) { CU_cleanup_registry(); return CU_get_error();