From 5863f95ae4154eeaa4d599bd8cb94d7306ad22eb Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Mon, 31 May 2021 16:11:13 +0900 Subject: [PATCH] bdev/nvme: Submit abort command for admin command on the current thread Previously only a single thread could submit abort commands for admin commands and it was the thread of the corresponding controller. When we support multipath, we need to traverse the list of controllers to which the target admin command is submitted. Threads of controllers may be different. On the other hand, the previous implementation made the I/O flow very clean, but the I/O flow will not be clean if there are many controllers and the subsystem does not have its thread. This patch changes the policy so that any SPDK thread can submit abort commands for admin commands. Then when multipath is supported, we will be able to traverse the list of controllers simply on the current thread to abort either I/O command or admin command. We already are able to submit any admin command on any thread anytime including abort command. Hence this will not cause any issue. Signed-off-by: Shuhei Matsumoto Change-Id: Ib69de33f2e84b03861c7d95ce060035bdb589e4b Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8121 Reviewed-by: Aleksey Marchuk Reviewed-by: Ben Walker Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot --- module/bdev/nvme/bdev_nvme.c | 47 +++++--------- .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 61 ++++++++++++------- 2 files changed, 53 insertions(+), 55 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 8194efd3f..7d3c0dbd8 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -3326,34 +3326,6 @@ bdev_nvme_io_passthru_md(struct spdk_nvme_ns *ns, struct spdk_nvme_qpair *qpair, (uint32_t)nbytes, md_buf, bdev_nvme_queued_done, bio); } -static void -bdev_nvme_abort_admin_cmd(void *ctx) -{ - struct nvme_bdev_io *bio = ctx; - struct spdk_bdev_io *bdev_io = spdk_bdev_io_from_ctx(bio); - struct nvme_io_channel *nvme_ch; - struct nvme_bdev_io *bio_to_abort; - int rc; - - nvme_ch = spdk_io_channel_get_ctx(spdk_bdev_io_get_io_channel(bdev_io)); - bio_to_abort = (struct nvme_bdev_io *)bdev_io->u.abort.bio_to_abort->driver_ctx; - - rc = spdk_nvme_ctrlr_cmd_abort_ext(nvme_ch->ctrlr->ctrlr, - NULL, - bio_to_abort, - bdev_nvme_abort_done, bio); - if (rc == -ENOENT) { - /* If no admin command was found in admin qpair, complete the abort - * request with failure. - */ - bio->cpl.cdw0 |= 1U; - bio->cpl.status.sc = SPDK_NVME_SC_SUCCESS; - bio->cpl.status.sct = SPDK_NVME_SCT_GENERIC; - - spdk_thread_send_msg(bio->orig_thread, bdev_nvme_abort_completion, bio); - } -} - static int bdev_nvme_abort(struct nvme_io_channel *nvme_ch, struct nvme_bdev_io *bio, struct nvme_bdev_io *bio_to_abort) @@ -3368,11 +3340,22 @@ bdev_nvme_abort(struct nvme_io_channel *nvme_ch, struct nvme_bdev_io *bio, bdev_nvme_abort_done, bio); if (rc == -ENOENT) { /* If no command was found in I/O qpair, the target command may be - * admin command. Only a single thread tries aborting admin command - * to clean I/O flow. + * admin command. */ - spdk_thread_send_msg(nvme_ch->ctrlr->thread, - bdev_nvme_abort_admin_cmd, bio); + rc = spdk_nvme_ctrlr_cmd_abort_ext(nvme_ch->ctrlr->ctrlr, + NULL, + bio_to_abort, + bdev_nvme_abort_done, bio); + } + + if (rc == -ENOENT) { + /* If no command was found, complete the abort request with failure. */ + bio->cpl.cdw0 |= 1U; + bio->cpl.status.sc = SPDK_NVME_SC_SUCCESS; + bio->cpl.status.sct = SPDK_NVME_SCT_GENERIC; + + bdev_nvme_abort_completion(bio); + rc = 0; } 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 32f2c5942..9ee358b17 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 @@ -1995,13 +1995,13 @@ test_abort(void) const char *attached_names[STRING_SIZE]; struct nvme_bdev *bdev; struct spdk_bdev_io *write_io, *admin_io, *abort_io; - struct spdk_io_channel *ch; - struct nvme_io_channel *nvme_ch; + struct spdk_io_channel *ch1, *ch2; + struct nvme_io_channel *nvme_ch1; int rc; /* Create ctrlr on thread 1 and submit I/O and admin requests to be aborted on - * thread 0. Abort requests are submitted on thread 0. Aborting I/O requests are - * done on thread 0 but aborting admin requests are done on thread 1. + * thread 0. Aborting I/O requests are submitted on thread 0. Aborting admin requests + * are submitted on thread 1. Both should succeed. */ ut_init_trid(&trid); @@ -2027,8 +2027,6 @@ test_abort(void) bdev = nvme_bdev_ctrlr->namespaces[0]->bdev; SPDK_CU_ASSERT_FATAL(bdev != NULL); - set_thread(0); - write_io = calloc(1, sizeof(struct spdk_bdev_io) + sizeof(struct nvme_bdev_io)); SPDK_CU_ASSERT_FATAL(write_io != NULL); write_io->bdev = &bdev->disk; @@ -2046,17 +2044,23 @@ test_abort(void) abort_io->bdev = &bdev->disk; abort_io->type = SPDK_BDEV_IO_TYPE_ABORT; - ch = spdk_get_io_channel(nvme_bdev_ctrlr); - SPDK_CU_ASSERT_FATAL(ch != NULL); - nvme_ch = spdk_io_channel_get_ctx(ch); + set_thread(0); - write_io->internal.ch = (struct spdk_bdev_channel *)ch; - admin_io->internal.ch = (struct spdk_bdev_channel *)ch; - abort_io->internal.ch = (struct spdk_bdev_channel *)ch; + ch1 = spdk_get_io_channel(nvme_bdev_ctrlr); + SPDK_CU_ASSERT_FATAL(ch1 != NULL); + nvme_ch1 = spdk_io_channel_get_ctx(ch1); + + set_thread(1); + + ch2 = spdk_get_io_channel(nvme_bdev_ctrlr); + SPDK_CU_ASSERT_FATAL(ch2 != NULL); + + write_io->internal.ch = (struct spdk_bdev_channel *)ch1; + abort_io->internal.ch = (struct spdk_bdev_channel *)ch1; /* Aborting the already completed request should fail. */ write_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, write_io); + bdev_nvme_submit_request(ch1, write_io); poll_threads(); CU_ASSERT(write_io->internal.in_submit_request == false); @@ -2064,7 +2068,7 @@ test_abort(void) abort_io->u.abort.bio_to_abort = write_io; abort_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, abort_io); + bdev_nvme_submit_request(ch1, abort_io); poll_threads(); @@ -2072,8 +2076,11 @@ test_abort(void) CU_ASSERT(abort_io->internal.status == SPDK_BDEV_IO_STATUS_FAILED); CU_ASSERT(ctrlr->adminq.num_outstanding_reqs == 0); + admin_io->internal.ch = (struct spdk_bdev_channel *)ch1; + abort_io->internal.ch = (struct spdk_bdev_channel *)ch2; + admin_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, admin_io); + bdev_nvme_submit_request(ch1, admin_io); spdk_delay_us(10000); poll_threads(); @@ -2082,7 +2089,7 @@ test_abort(void) abort_io->u.abort.bio_to_abort = admin_io; abort_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, abort_io); + bdev_nvme_submit_request(ch2, abort_io); poll_threads(); @@ -2092,15 +2099,16 @@ test_abort(void) /* Aborting the write request should succeed. */ write_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, write_io); + bdev_nvme_submit_request(ch1, write_io); CU_ASSERT(write_io->internal.in_submit_request == true); - CU_ASSERT(nvme_ch->qpair->num_outstanding_reqs == 1); + CU_ASSERT(nvme_ch1->qpair->num_outstanding_reqs == 1); + abort_io->internal.ch = (struct spdk_bdev_channel *)ch1; abort_io->u.abort.bio_to_abort = write_io; abort_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, abort_io); + bdev_nvme_submit_request(ch1, abort_io); spdk_delay_us(10000); poll_threads(); @@ -2110,19 +2118,20 @@ test_abort(void) CU_ASSERT(ctrlr->adminq.num_outstanding_reqs == 0); CU_ASSERT(write_io->internal.in_submit_request == false); CU_ASSERT(write_io->internal.status == SPDK_BDEV_IO_STATUS_ABORTED); - CU_ASSERT(nvme_ch->qpair->num_outstanding_reqs == 0); + CU_ASSERT(nvme_ch1->qpair->num_outstanding_reqs == 0); /* Aborting the admin request should succeed. */ admin_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, admin_io); + bdev_nvme_submit_request(ch1, admin_io); CU_ASSERT(admin_io->internal.in_submit_request == true); CU_ASSERT(ctrlr->adminq.num_outstanding_reqs == 1); + abort_io->internal.ch = (struct spdk_bdev_channel *)ch2; abort_io->u.abort.bio_to_abort = admin_io; abort_io->internal.in_submit_request = true; - bdev_nvme_submit_request(ch, abort_io); + bdev_nvme_submit_request(ch2, abort_io); spdk_delay_us(10000); poll_threads(); @@ -2134,7 +2143,13 @@ test_abort(void) CU_ASSERT(admin_io->internal.status == SPDK_BDEV_IO_STATUS_ABORTED); CU_ASSERT(ctrlr->adminq.num_outstanding_reqs == 0); - spdk_put_io_channel(ch); + set_thread(0); + + spdk_put_io_channel(ch1); + + set_thread(1); + + spdk_put_io_channel(ch2); poll_threads();