Commit d7f0a182 authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

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: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10746


Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 486f46e8

module/bdev/nvme/bdev_nvme.c

+20 −20
Original line number Diff line number Diff line
@@ -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);

test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c

+2 −3
Original line number Diff line number Diff line
@@ -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();