From 3959e397d4c3aacc14b3a571463e3c5dc66e8b8b Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Fri, 25 Jun 2021 05:59:00 +0900 Subject: [PATCH] nvme: Add new detach to a detach context while it is being polled This update will allow us to use spdk_nvme_detach_async() and spdk_nvme_detach_poll_async() easier to aggregate multiple detachments. Previously, we could do: spdk_nvme_detach_async() spdk_nvme_detach_async() spdk_nvme_detach_async() and then started doing spdk_nvme_detach_poll_async(). Hence aggregating multiple detachments is already supported. After this patch, the following sequence is possible: spdk_nvme_detach_async() = 0 spdk_nvme_detach_async() = 0 spdk_nvme_detach_async() = 0 spdk_nvme_detach_poll_async() = -EAGAIN spdk_nvme_detach_async() = 0 spdk_nvme_detach_async() = 0 spdk_nvme_detach_poll_async() = -EAGAIN spdk_nvme_detach_poll_async() = -EAGAIN spdk_nvme_detach_poll_async() = -EAGAIN spdk_nvme_detach_poll_async() = 0 The actual changes is to remove the variable polling_started from struct spdk_nvme_detach_ctx because it is not necessary anymore. Clarify this change via updating the header file and CHANGELOG. Verify this change by unit test. Signed-off-by: Shuhei Matsumoto Change-Id: Iebdf6c27c5304a2097b7084c315ccc99634ffa1e Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8468 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Reviewed-by: Jim Harris Reviewed-by: Aleksey Marchuk --- CHANGELOG.md | 3 ++ lib/nvme/nvme.c | 5 --- lib/nvme/nvme_internal.h | 1 - test/unit/lib/nvme/nvme.c/nvme_ut.c | 50 +++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e0b0273fb..ba0e89c772 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,9 @@ the spdk_nvme_ctrlr_opts structure prior to controller attach. Add a new function `spdk_nvme_detach_poll` to simplify a common use case to continue polling until all detachments complete. +An existing function `spdk_nvme_detach_async` was updated to add one or more detachments +to an active context while it is being polled. + ### rpc New RPC `bdev_rbd_register_cluster` and `bdev_rbd_unregister_cluster` was added, it allows to create diff --git a/lib/nvme/nvme.c b/lib/nvme/nvme.c index b9b74d4d69..1cd8753fd5 100644 --- a/lib/nvme/nvme.c +++ b/lib/nvme/nvme.c @@ -180,9 +180,6 @@ spdk_nvme_detach_async(struct spdk_nvme_ctrlr *ctrlr, return -ENOMEM; } TAILQ_INIT(&detach_ctx->head); - } else if (detach_ctx->polling_started) { - SPDK_ERRLOG("Busy at polling detachment now.\n"); - return -EBUSY; } rc = nvme_ctrlr_detach_async(ctrlr, &ctx); @@ -214,8 +211,6 @@ spdk_nvme_detach_poll_async(struct spdk_nvme_detach_ctx *detach_ctx) return -EINVAL; } - detach_ctx->polling_started = true; - TAILQ_FOREACH_SAFE(ctx, &detach_ctx->head, link, tmp_ctx) { TAILQ_REMOVE(&detach_ctx->head, ctx, link); diff --git a/lib/nvme/nvme_internal.h b/lib/nvme/nvme_internal.h index ebedac38f2..49fd789dbd 100644 --- a/lib/nvme/nvme_internal.h +++ b/lib/nvme/nvme_internal.h @@ -914,7 +914,6 @@ struct nvme_ctrlr_detach_ctx { struct spdk_nvme_detach_ctx { TAILQ_HEAD(, nvme_ctrlr_detach_ctx) head; - bool polling_started; }; struct nvme_driver { diff --git a/test/unit/lib/nvme/nvme.c/nvme_ut.c b/test/unit/lib/nvme/nvme.c/nvme_ut.c index 0c6fbfe7da..44077a7471 100644 --- a/test/unit/lib/nvme/nvme.c/nvme_ut.c +++ b/test/unit/lib/nvme/nvme.c/nvme_ut.c @@ -79,12 +79,18 @@ nvme_ctrlr_destruct_async(struct spdk_nvme_ctrlr *ctrlr, struct nvme_ctrlr_detac { ut_destruct_called = true; ctrlr->is_destructed = true; + + ctx->shutdown_complete = true; } int nvme_ctrlr_destruct_poll_async(struct spdk_nvme_ctrlr *ctrlr, struct nvme_ctrlr_detach_ctx *ctx) { + if (!ctx->shutdown_complete) { + return -EAGAIN; + } + if (ctx->cb_fn) { ctx->cb_fn(ctrlr); } @@ -1496,6 +1502,7 @@ test_spdk_nvme_detach_async(void) struct spdk_nvme_ctrlr ctrlr1, ctrlr2; struct nvme_driver test_driver; struct spdk_nvme_detach_ctx *detach_ctx; + struct nvme_ctrlr_detach_ctx *ctx; detach_ctx = NULL; memset(&ctrlr1, 0, sizeof(ctrlr1)); @@ -1556,6 +1563,49 @@ test_spdk_nvme_detach_async(void) CU_ASSERT(TAILQ_EMPTY(&g_nvme_attached_ctrlrs) == true); CU_ASSERT(TAILQ_EMPTY(&test_driver.shared_attached_ctrlrs) == true); + /* Test if ctrlr2 can be detached by using the same context that + * ctrlr1 uses while ctrlr1 is being detached. + */ + detach_ctx = NULL; + memset(&ctrlr1, 0, sizeof(ctrlr1)); + ctrlr1.trid.trtype = SPDK_NVME_TRANSPORT_PCIE; + memset(&ctrlr2, 0, sizeof(ctrlr2)); + ctrlr2.trid.trtype = SPDK_NVME_TRANSPORT_PCIE; + TAILQ_INSERT_TAIL(&test_driver.shared_attached_ctrlrs, &ctrlr1, tailq); + TAILQ_INSERT_TAIL(&test_driver.shared_attached_ctrlrs, &ctrlr2, tailq); + + rc = spdk_nvme_detach_async(&ctrlr1, &detach_ctx); + CU_ASSERT(rc == 0); + CU_ASSERT(ctrlr1.is_destructed == true); + SPDK_CU_ASSERT_FATAL(detach_ctx != NULL); + + ctx = TAILQ_FIRST(&detach_ctx->head); + SPDK_CU_ASSERT_FATAL(ctx != NULL); + CU_ASSERT(ctx->ctrlr == &ctrlr1); + CU_ASSERT(ctx->shutdown_complete == true); + + /* Set ctx->shutdown_complete for ctrlr1 to false to allow ctrlr2 to + * add to detach_ctx while spdk_nvme_detach_poll_async() is being + * executed. + */ + ctx->shutdown_complete = false; + + rc = spdk_nvme_detach_poll_async(detach_ctx); + CU_ASSERT(rc == -EAGAIN); + + rc = spdk_nvme_detach_async(&ctrlr2, &detach_ctx); + CU_ASSERT(rc == 0); + CU_ASSERT(ctrlr2.is_destructed == true); + + /* After ctrlr2 is added to detach_ctx, set ctx->shutdown_complete for + * ctrlr1 to true to complete spdk_nvme_detach_poll_async(). + */ + ctx->shutdown_complete = true; + + rc = spdk_nvme_detach_poll_async(detach_ctx); + CU_ASSERT(rc == 0); + CU_ASSERT(TAILQ_EMPTY(&test_driver.shared_attached_ctrlrs) == true); + g_spdk_nvme_driver = NULL; pthread_mutex_destroy(&test_driver.lock); MOCK_CLEAR(nvme_ctrlr_get_ref_count);