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

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: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
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: default avatarPaul Luse <paul.e.luse@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 059dcf1d

module/bdev/nvme/bdev_nvme.c

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

	}



	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));

}

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

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