Commit d4cd02e7 authored by Super User's avatar Super User Committed by Jim Harris
Browse files

bdev/nvme: Fix memory leak at shutdown when I/O path stat is enabled 



At SPDK shutdown, all existing bdevs are unregistered and then all
bdev modules are finished. We had considered such scenario in the unit
tests. However, we had not enabled I/O path statistics or created any
bdev I/O channel. Hence, we overlooked this bug.

nvme_ns is managed not by nvme_bdev but nvme_ctrlr. Hence, it was wrong
that we held lock of nvme_bdev to copy I/O path statistics.

As a bug fix, hold lock of nvme_ctrlr and get nvme_ns from nvme_ctrlr.
To verify the fix, update the corresponding unit test case slightly.

Fixes github issue #3629

Change-Id: Ic47057d5334f2f4044cccacb92d5497d2db1972a
Signed-off-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-on: https://review.spdk.io/c/spdk/spdk/+/25735


Reviewed-by: default avatarKonrad Sztyber <ksztyber@nvidia.com>
Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK Automated Test System <spdkbot@gmail.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
parent f4b3ad77

module/bdev/nvme/bdev_nvme.c

+13 −7
Original line number Diff line number Diff line
@@ -890,15 +890,24 @@ _bdev_nvme_delete_io_path(struct nvme_bdev_channel *nbdev_ch, struct nvme_io_pat 
	struct nvme_qpair *nvme_qpair;

	struct nvme_ctrlr_channel *ctrlr_ch;

	struct nvme_bdev *nbdev;

	struct nvme_ctrlr *nvme_ctrlr;

	struct nvme_ns *nvme_ns;



	nbdev = spdk_io_channel_get_io_device(spdk_io_channel_from_ctx(nbdev_ch));



	nvme_qpair = io_path->qpair;

	assert(nvme_qpair != NULL);



	nvme_ctrlr = nvme_qpair->ctrlr;

	assert(nvme_ctrlr != NULL);



	/* Add the statistics to nvme_ns before this path is destroyed. */

	pthread_mutex_lock(&nbdev->mutex);

	if (nbdev->ref != 0 && io_path->nvme_ns->stat != NULL && io_path->stat != NULL) {

		spdk_bdev_add_io_stat(io_path->nvme_ns->stat, io_path->stat);

	pthread_mutex_lock(&nvme_ctrlr->mutex);

	nvme_ns = nvme_ctrlr_get_ns(nvme_ctrlr, nbdev->nsid);

	if (nvme_ns != NULL && nvme_ns->stat != NULL && io_path->stat != NULL) {

		spdk_bdev_add_io_stat(nvme_ns->stat, io_path->stat);

	}

	pthread_mutex_unlock(&nbdev->mutex);

	pthread_mutex_unlock(&nvme_ctrlr->mutex);



	bdev_nvme_clear_current_io_path(nbdev_ch);

	bdev_nvme_clear_retry_io_path(nbdev_ch, io_path);
@@ -906,9 +915,6 @@ _bdev_nvme_delete_io_path(struct nvme_bdev_channel *nbdev_ch, struct nvme_io_pat 
	STAILQ_REMOVE(&nbdev_ch->io_path_list, io_path, nvme_io_path, stailq);

	io_path->nbdev_ch = NULL;



	nvme_qpair = io_path->qpair;

	assert(nvme_qpair != NULL);



	ctrlr_ch = nvme_qpair->ctrlr_ch;

	assert(ctrlr_ch != NULL);

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

+17 −0
Original line number Diff line number Diff line
@@ -3029,9 +3029,14 @@ test_bdev_unregister(void) 
	const int STRING_SIZE = 32;

	const char *attached_names[STRING_SIZE];

	struct nvme_bdev *nbdev1, *nbdev2;

	struct spdk_io_channel *ch1;

	struct nvme_bdev_channel *nbdev_ch1;

	struct nvme_io_path *io_path1;

	int rc;

	struct spdk_bdev_nvme_ctrlr_opts bdev_opts = {0};



	g_opts.io_path_stat = true;



	spdk_bdev_nvme_get_default_ctrlr_opts(&bdev_opts);

	bdev_opts.multipath = false;
@@ -3066,6 +3071,14 @@ test_bdev_unregister(void) 
	nbdev2 = nvme_ns2->bdev;

	SPDK_CU_ASSERT_FATAL(nbdev2 != NULL);



	ch1 = spdk_get_io_channel(nbdev1);

	SPDK_CU_ASSERT_FATAL(ch1 != NULL);



	nbdev_ch1 = spdk_io_channel_get_ctx(ch1);

	io_path1 = STAILQ_FIRST(&nbdev_ch1->io_path_list);

	SPDK_CU_ASSERT_FATAL(io_path1 != NULL);

	CU_ASSERT(io_path1->nvme_ns == nvme_ns1);



	bdev_nvme_destruct(&nbdev1->disk);

	bdev_nvme_destruct(&nbdev2->disk);
@@ -3077,11 +3090,15 @@ test_bdev_unregister(void) 
	nvme_ctrlr->destruct = true;

	_nvme_ctrlr_destruct(nvme_ctrlr);



	spdk_put_io_channel(ch1);



	poll_threads();

	spdk_delay_us(1000);

	poll_threads();



	CU_ASSERT(nvme_ctrlr_get_by_name("nvme0") == NULL);



	g_opts.io_path_stat = false;

}



static void