From cb8aa8abc51955d298e2dfab18858f535323a89d Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Wed, 24 Mar 2021 19:31:43 +0900 Subject: [PATCH] bdev/nvme: Not try freeing qpair when it is NULL, and add test scenario The API spdk_nvme_ctrlr_free_io_qpair() returns immediately if the passed qpair is NULL, but calling spdk_nvme_ctrlr_free_io_qpair() with NULL should be avoided. This patch cleans up the code to ensure that nvme_ch->qpair is NULL if disconnected and spdk_nvme_ctrlr_free_io_qpair() is called only if nvme_ch->qpair is not NULL. Then add a test scenario that two reset requests were submitted simultaneously and the first reset request failed and then the second reset request also failed. This verifies the refactoring done in the next patch. Signed-off-by: Shuhei Matsumoto Change-Id: Iae461f7f826b0e1a4607a17e528c04a642242d6e Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/7041 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Reviewed-by: Paul Luse Reviewed-by: Jim Harris Reviewed-by: Aleksey Marchuk Tested-by: SPDK CI Jenkins --- module/bdev/nvme/bdev_nvme.c | 17 ++++++---- .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 34 ++++++++++++++++++- 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index df6151b4e..97f134656 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -340,6 +340,7 @@ bdev_nvme_create_qpair(struct nvme_io_channel *nvme_ch) { struct spdk_nvme_ctrlr *ctrlr = nvme_ch->ctrlr->ctrlr; struct spdk_nvme_io_qpair_opts opts; + struct spdk_nvme_qpair *qpair; int rc; spdk_nvme_ctrlr_get_default_io_qpair_opts(ctrlr, &opts, sizeof(opts)); @@ -348,29 +349,31 @@ bdev_nvme_create_qpair(struct nvme_io_channel *nvme_ch) opts.io_queue_requests = spdk_max(g_opts.io_queue_requests, opts.io_queue_requests); g_opts.io_queue_requests = opts.io_queue_requests; - nvme_ch->qpair = spdk_nvme_ctrlr_alloc_io_qpair(ctrlr, &opts, sizeof(opts)); - if (nvme_ch->qpair == NULL) { + qpair = spdk_nvme_ctrlr_alloc_io_qpair(ctrlr, &opts, sizeof(opts)); + if (qpair == NULL) { return -1; } assert(nvme_ch->group != NULL); - rc = spdk_nvme_poll_group_add(nvme_ch->group->group, nvme_ch->qpair); + rc = spdk_nvme_poll_group_add(nvme_ch->group->group, qpair); if (rc != 0) { SPDK_ERRLOG("Unable to begin polling on NVMe Channel.\n"); goto err; } - rc = spdk_nvme_ctrlr_connect_io_qpair(ctrlr, nvme_ch->qpair); + rc = spdk_nvme_ctrlr_connect_io_qpair(ctrlr, qpair); if (rc != 0) { SPDK_ERRLOG("Unable to connect I/O qpair.\n"); goto err; } + nvme_ch->qpair = qpair; + return 0; err: - spdk_nvme_ctrlr_free_io_qpair(nvme_ch->qpair); + spdk_nvme_ctrlr_free_io_qpair(qpair); return rc; } @@ -1020,7 +1023,9 @@ bdev_nvme_destroy_cb(void *io_device, void *ctx_buf) bdev_ocssd_destroy_io_channel(nvme_ch); } - spdk_nvme_ctrlr_free_io_qpair(nvme_ch->qpair); + if (nvme_ch->qpair != NULL) { + spdk_nvme_ctrlr_free_io_qpair(nvme_ch->qpair); + } spdk_put_io_channel(spdk_io_channel_from_ctx(nvme_ch->group)); } 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 cddec2474..3d2a7386a 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 @@ -252,6 +252,7 @@ struct spdk_nvme_ctrlr { struct spdk_nvme_qpair adminq; struct spdk_nvme_ctrlr_data cdata; bool is_failed; + bool fail_reset; struct spdk_nvme_transport_id trid; TAILQ_HEAD(, spdk_nvme_qpair) active_io_qpairs; TAILQ_ENTRY(spdk_nvme_ctrlr) tailq; @@ -615,6 +616,10 @@ spdk_nvme_ctrlr_free_io_qpair(struct spdk_nvme_qpair *qpair) int spdk_nvme_ctrlr_reset(struct spdk_nvme_ctrlr *ctrlr) { + if (ctrlr->fail_reset) { + return -EIO; + } + ctrlr->is_failed = false; return 0; @@ -1331,7 +1336,7 @@ test_pending_reset(void) nvme_ch2 = spdk_io_channel_get_ctx(ch2); - /* The first abort request is submitted on thread 1, and the second abort request + /* The first reset request is submitted on thread 1, and the second reset request * is submitted on thread 0 while processing the first request. */ rc = bdev_nvme_reset(nvme_ch2, first_bio); @@ -1351,6 +1356,33 @@ test_pending_reset(void) CU_ASSERT(first_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_SUCCESS); CU_ASSERT(second_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_SUCCESS); + /* The first reset request is submitted on thread 1, and the second reset request + * is submitted on thread 0 while processing the first request. + * + * The difference from the above scenario is that the controller is removed while + * processing the first request. Hence both reset requests should fail. + */ + set_thread(1); + + rc = bdev_nvme_reset(nvme_ch2, first_bio); + CU_ASSERT(rc == 0); + CU_ASSERT(nvme_bdev_ctrlr->resetting == true); + CU_ASSERT(TAILQ_EMPTY(&nvme_ch2->pending_resets)); + + set_thread(0); + + rc = bdev_nvme_reset(nvme_ch1, second_bio); + CU_ASSERT(rc == 0); + CU_ASSERT(TAILQ_FIRST(&nvme_ch1->pending_resets) == second_bdev_io); + + ctrlr.fail_reset = true; + + poll_threads(); + + CU_ASSERT(nvme_bdev_ctrlr->resetting == false); + CU_ASSERT(first_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_FAILED); + CU_ASSERT(second_bdev_io->internal.status == SPDK_BDEV_IO_STATUS_FAILED); + spdk_put_io_channel(ch1); set_thread(1);