From d7f0a1820eb52bfce7ca511df55ddc0ca9dffab8 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Wed, 9 Mar 2022 07:24:35 +0900 Subject: [PATCH] bdev/nvme: Inline bdev_nvme_destroy_qpair() In the following patches, spdk_nvme_ctrlr_disconnect_io_qpair() will be changed to be asynchronous, spdk_nvme_ctrlr_disconnect_io_qpair() will be called first and then spdk_nvme_ctrlr_free_io_qpair() after the qpair is actually disconnected. We will not be able to keep the current bdev_nvme_destroy_qpair() function. As a preparation, inline bdev_nvme_destroy_qpair() and remove it. Additionally, this patch has the following changes. Previously I/O qpair was freed and then I/O path caches were cleared. Both are SPDK thread local. So there is no dependency for the ordering of these two operations. However, it will reduce the size of the following patches if we clear I/O path caches before freeing I/O qpair when the qpair is disconnected. Hence we clear I/O path caches and then free I/O qpair. Remove DTRACE for bdev_nvme_destroy_qpair() for now. It will be restored in the following patches. Furthermore, fix potential NULL pointer acces in bdev_nvme_create_qpair(). Change-Id: I0ab78ccb0d240e56b95b53179341afcd909a31f6 Signed-off-by: Shuhei Matsumoto Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10746 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Reviewed-by: Ben Walker Reviewed-by: Jim Harris Tested-by: SPDK CI Jenkins --- module/bdev/nvme/bdev_nvme.c | 40 +++++++++---------- .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 5 +-- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 13c19448f..ec2abbd27 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -1080,22 +1080,6 @@ nvme_poll_group_get_ctrlr_channel(struct nvme_poll_group *group, return ctrlr_ch; } -static void -bdev_nvme_destroy_qpair(struct nvme_ctrlr_channel *ctrlr_ch) -{ - struct nvme_ctrlr *nvme_ctrlr __attribute__((unused)); - - if (ctrlr_ch->qpair != NULL) { - nvme_ctrlr = nvme_ctrlr_channel_get_ctrlr(ctrlr_ch); - SPDK_DTRACE_PROBE2(bdev_nvme_destroy_qpair, nvme_ctrlr->nbdev_ctrlr->name, - spdk_nvme_qpair_get_id(ctrlr_ch->qpair)); - spdk_nvme_ctrlr_free_io_qpair(ctrlr_ch->qpair); - ctrlr_ch->qpair = NULL; - } - - _bdev_nvme_clear_io_path_cache(ctrlr_ch); -} - static void bdev_nvme_disconnected_qpair_cb(struct spdk_nvme_qpair *qpair, void *poll_group_ctx) { @@ -1104,12 +1088,18 @@ bdev_nvme_disconnected_qpair_cb(struct spdk_nvme_qpair *qpair, void *poll_group_ struct nvme_ctrlr *nvme_ctrlr; SPDK_NOTICELOG("qpair %p is disconnected, free the qpair and reset controller.\n", qpair); + /* * Free the I/O qpair and reset the nvme_ctrlr. */ ctrlr_ch = nvme_poll_group_get_ctrlr_channel(group, qpair); if (ctrlr_ch != NULL) { - bdev_nvme_destroy_qpair(ctrlr_ch); + if (ctrlr_ch->qpair != NULL) { + spdk_nvme_ctrlr_free_io_qpair(ctrlr_ch->qpair); + ctrlr_ch->qpair = NULL; + } + + _bdev_nvme_clear_io_path_cache(ctrlr_ch); nvme_ctrlr = nvme_ctrlr_channel_get_ctrlr(ctrlr_ch); bdev_nvme_reset(nvme_ctrlr); @@ -1233,7 +1223,7 @@ bdev_nvme_create_qpair(struct nvme_ctrlr_channel *ctrlr_ch) } SPDK_DTRACE_PROBE3(bdev_nvme_create_qpair, nvme_ctrlr->nbdev_ctrlr->name, - spdk_nvme_qpair_get_id(ctrlr_ch->qpair), spdk_thread_get_id(nvme_ctrlr->thread)); + spdk_nvme_qpair_get_id(qpair), spdk_thread_get_id(nvme_ctrlr->thread)); assert(ctrlr_ch->group != NULL); @@ -1504,7 +1494,12 @@ bdev_nvme_reset_destroy_qpair(struct spdk_io_channel_iter *i) struct spdk_io_channel *ch = spdk_io_channel_iter_get_channel(i); struct nvme_ctrlr_channel *ctrlr_ch = spdk_io_channel_get_ctx(ch); - bdev_nvme_destroy_qpair(ctrlr_ch); + _bdev_nvme_clear_io_path_cache(ctrlr_ch); + + if (ctrlr_ch->qpair != NULL) { + spdk_nvme_ctrlr_free_io_qpair(ctrlr_ch->qpair); + ctrlr_ch->qpair = NULL; + } spdk_for_each_channel_continue(i, 0); } @@ -2096,7 +2091,12 @@ bdev_nvme_destroy_ctrlr_channel_cb(void *io_device, void *ctx_buf) assert(ctrlr_ch->group != NULL); - bdev_nvme_destroy_qpair(ctrlr_ch); + _bdev_nvme_clear_io_path_cache(ctrlr_ch); + + if (ctrlr_ch->qpair != NULL) { + spdk_nvme_ctrlr_free_io_qpair(ctrlr_ch->qpair); + ctrlr_ch->qpair = NULL; + } TAILQ_REMOVE(&ctrlr_ch->group->ctrlr_ch_list, ctrlr_ch, tailq); 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 48b2e2a8a..a81730add 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 @@ -4332,9 +4332,8 @@ test_retry_io_for_io_path_error(void) CU_ASSERT(bdev_io->internal.in_submit_request == true); CU_ASSERT(bdev_io == TAILQ_FIRST(&nbdev_ch->retry_io_list)); - bdev_nvme_destroy_qpair(ctrlr_ch1); - - CU_ASSERT(ctrlr_ch1->qpair == NULL); + spdk_nvme_ctrlr_free_io_qpair(ctrlr_ch1->qpair); + ctrlr_ch1->qpair = NULL; poll_threads();