From c5ebb7ff99d80e4742148b8e5e54b9f1d3806569 Mon Sep 17 00:00:00 2001 From: Konrad Sztyber Date: Fri, 9 Jul 2021 10:51:32 +0200 Subject: [PATCH] bdev/nvme: use asynchronous ctrlr detach functions This patch replaces the synchronous `spdk_nvme_detach()` calls with its asynchronous counterparts in the controller unregister path. An additional poller is introduced to periodically poll the NVMe driver for detach completion. Once the detach is completed, the poller is unregistered and the nvme_ctrlr is destroyed. The poller uses the same period (1ms) as the async probe poller. Since reset and detach cannot happen at the same time, reset_poller was renamed to reset_detach_poller and it can now store the pointer either to the reset or detach poller, depending on the circumstances. Signed-off-by: Konrad Sztyber Change-Id: I5eb2dd6383d98d25d1f9748af08c1a13d18acb0e Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8729 Tested-by: SPDK CI Jenkins Community-CI: Broadcom CI Reviewed-by: Shuhei Matsumoto Reviewed-by: Aleksey Marchuk --- module/bdev/nvme/bdev_nvme.c | 59 ++++++++++++++++--- module/bdev/nvme/bdev_nvme.h | 4 +- test/nvmf/host/async_init.sh | 3 +- .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 55 +++++++++++++++++ 4 files changed, 112 insertions(+), 9 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 744771b1d..28eb1f916 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -409,7 +409,7 @@ nvme_bdev_ctrlr_delete(struct nvme_bdev_ctrlr *nbdev_ctrlr, } static void -nvme_ctrlr_delete(struct nvme_ctrlr *nvme_ctrlr) +_nvme_ctrlr_delete(struct nvme_ctrlr *nvme_ctrlr) { struct nvme_ctrlr_trid *trid, *tmp_trid; uint32_t i; @@ -426,8 +426,6 @@ nvme_ctrlr_delete(struct nvme_ctrlr *nvme_ctrlr) nvme_bdev_ctrlr_delete(nvme_ctrlr->nbdev_ctrlr, nvme_ctrlr); } - spdk_nvme_detach(nvme_ctrlr->ctrlr); - spdk_poller_unregister(&nvme_ctrlr->adminq_timer_poller); for (i = 0; i < nvme_ctrlr->num_ns; i++) { free(nvme_ctrlr->namespaces[i]); } @@ -452,6 +450,53 @@ nvme_ctrlr_delete(struct nvme_ctrlr *nvme_ctrlr) pthread_mutex_unlock(&g_bdev_nvme_mutex); } +static int +nvme_detach_poller(void *arg) +{ + struct nvme_ctrlr *nvme_ctrlr = arg; + int rc; + + rc = spdk_nvme_detach_poll_async(nvme_ctrlr->detach_ctx); + if (rc != -EAGAIN) { + spdk_poller_unregister(&nvme_ctrlr->reset_detach_poller); + _nvme_ctrlr_delete(nvme_ctrlr); + } + + return SPDK_POLLER_BUSY; +} + +static void +nvme_ctrlr_delete(struct nvme_ctrlr *nvme_ctrlr) +{ + int rc; + + /* First, unregister the adminq poller, as the driver will poll adminq if necessary */ + spdk_poller_unregister(&nvme_ctrlr->adminq_timer_poller); + + /* If we got here, the reset/detach poller cannot be active */ + assert(nvme_ctrlr->reset_detach_poller == NULL); + nvme_ctrlr->reset_detach_poller = SPDK_POLLER_REGISTER(nvme_detach_poller, + nvme_ctrlr, 1000); + if (nvme_ctrlr->reset_detach_poller == NULL) { + SPDK_ERRLOG("Failed to register detach poller\n"); + goto error; + } + + rc = spdk_nvme_detach_async(nvme_ctrlr->ctrlr, &nvme_ctrlr->detach_ctx); + if (rc != 0) { + SPDK_ERRLOG("Failed to detach the NVMe controller\n"); + goto error; + } + + return; +error: + /* We don't have a good way to handle errors here, so just do what we can and delete the + * controller without detaching the underlying NVMe device. + */ + spdk_poller_unregister(&nvme_ctrlr->reset_detach_poller); + _nvme_ctrlr_delete(nvme_ctrlr); +} + static void nvme_ctrlr_unregister_cb(void *io_device) { @@ -868,7 +913,7 @@ bdev_nvme_ctrlr_reset_poll(void *arg) return SPDK_POLLER_BUSY; } - spdk_poller_unregister(&nvme_ctrlr->reset_poller); + spdk_poller_unregister(&nvme_ctrlr->reset_detach_poller); if (rc == 0) { /* Recreate all of the I/O queue pairs */ spdk_for_each_channel(nvme_ctrlr, @@ -897,9 +942,9 @@ bdev_nvme_reset_ctrlr(struct spdk_io_channel_iter *i, int status) SPDK_ERRLOG("Create controller reset context failed\n"); goto err; } - assert(nvme_ctrlr->reset_poller == NULL); - nvme_ctrlr->reset_poller = SPDK_POLLER_REGISTER(bdev_nvme_ctrlr_reset_poll, - nvme_ctrlr, 0); + assert(nvme_ctrlr->reset_detach_poller == NULL); + nvme_ctrlr->reset_detach_poller = SPDK_POLLER_REGISTER(bdev_nvme_ctrlr_reset_poll, + nvme_ctrlr, 0); return; diff --git a/module/bdev/nvme/bdev_nvme.h b/module/bdev/nvme/bdev_nvme.h index f8e37f125..590c80c1d 100644 --- a/module/bdev/nvme/bdev_nvme.h +++ b/module/bdev/nvme/bdev_nvme.h @@ -118,7 +118,9 @@ struct nvme_ctrlr { bdev_nvme_reset_cb reset_cb_fn; void *reset_cb_arg; struct spdk_nvme_ctrlr_reset_ctx *reset_ctx; - struct spdk_poller *reset_poller; + /* Poller used to check for reset/detach completion */ + struct spdk_poller *reset_detach_poller; + struct spdk_nvme_detach_ctx *detach_ctx; /** linked list pointer for device list */ TAILQ_ENTRY(nvme_ctrlr) tailq; diff --git a/test/nvmf/host/async_init.sh b/test/nvmf/host/async_init.sh index 83c6ab777..e2077d82a 100755 --- a/test/nvmf/host/async_init.sh +++ b/test/nvmf/host/async_init.sh @@ -38,7 +38,8 @@ $rpc_py bdev_nvme_attach_controller -b $nvme_bdev -t $TEST_TRANSPORT -a $NVMF_FI # Make sure the reset is also asynchronous $rpc_py bdev_nvme_reset_controller $nvme_bdev -# TODO: Once the async detach path is functional, send a bdev_nvme_detach_controller here +# Finally, detach the controller to verify the detach path +$rpc_py bdev_nvme_detach_controller $nvme_bdev trap - SIGINT SIGTERM EXIT nvmftestfini diff --git a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c index 04daa4be1..c760b4c28 100644 --- a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c +++ b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c @@ -565,6 +565,21 @@ spdk_nvme_detach(struct spdk_nvme_ctrlr *ctrlr) return 0; } +int +spdk_nvme_detach_async(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_detach_ctx **ctx) +{ + SPDK_CU_ASSERT_FATAL(ctx != NULL); + *(struct spdk_nvme_ctrlr **)ctx = ctrlr; + + return 0; +} + +int +spdk_nvme_detach_poll_async(struct spdk_nvme_detach_ctx *ctx) +{ + return spdk_nvme_detach((struct spdk_nvme_ctrlr *)ctx); +} + void spdk_nvme_ctrlr_get_default_ctrlr_opts(struct spdk_nvme_ctrlr_opts *opts, size_t opts_size) { @@ -1186,6 +1201,8 @@ test_create_ctrlr(void) CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") != NULL); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1294,6 +1311,8 @@ test_reset_ctrlr(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1369,6 +1388,8 @@ test_race_between_reset_and_destruct_ctrlr(void) spdk_put_io_channel(ch2); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1508,6 +1529,8 @@ test_failover_ctrlr(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1637,6 +1660,8 @@ test_pending_reset(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1701,6 +1726,8 @@ test_attach_ctrlr(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1735,6 +1762,8 @@ test_attach_ctrlr(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1765,6 +1794,8 @@ test_attach_ctrlr(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -1864,6 +1895,8 @@ test_aer_cb(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2058,6 +2091,8 @@ test_submit_nvme_cmd(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2170,6 +2205,8 @@ test_add_remove_trid(void) CU_ASSERT(rc == 0); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == nvme_ctrlr); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2212,6 +2249,8 @@ test_add_remove_trid(void) CU_ASSERT(rc == 0); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == nvme_ctrlr); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2388,6 +2427,8 @@ test_abort(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2428,6 +2469,8 @@ test_get_io_qpair(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2492,6 +2535,8 @@ test_bdev_unregister(void) nvme_ctrlr->destruct = true; _nvme_ctrlr_destruct(nvme_ctrlr); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2606,6 +2651,8 @@ test_init_ana_log_page(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2767,6 +2814,8 @@ test_reconnect_qpair(void) rc = bdev_nvme_delete("nvme0", &g_any_trid); CU_ASSERT(rc == 0); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL); @@ -2851,6 +2900,8 @@ test_create_bdev_ctrlr(void) CU_ASSERT(nvme_bdev_ctrlr_get_ctrlr(nbdev_ctrlr, &trid1) != NULL); CU_ASSERT(nvme_bdev_ctrlr_get_ctrlr(nbdev_ctrlr, &trid2) != NULL); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_bdev_ctrlr_get("nvme0") == NULL); @@ -2892,6 +2943,8 @@ test_create_bdev_ctrlr(void) CU_ASSERT(nvme_bdev_ctrlr_get_ctrlr(nbdev_ctrlr, &trid1) != NULL); CU_ASSERT(nvme_bdev_ctrlr_get_ctrlr(nbdev_ctrlr, &trid2) != NULL); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_bdev_ctrlr_get("nvme0") == nbdev_ctrlr); @@ -2905,6 +2958,8 @@ test_create_bdev_ctrlr(void) CU_ASSERT(nvme_bdev_ctrlr_get_ctrlr(nbdev_ctrlr, &trid1) == NULL); CU_ASSERT(nvme_bdev_ctrlr_get_ctrlr(nbdev_ctrlr, &trid2) != NULL); + poll_threads(); + spdk_delay_us(1000); poll_threads(); CU_ASSERT(nvme_bdev_ctrlr_get("nvme0") == NULL);