Commit 2e1dbc45 authored by Ben Walker's avatar Ben Walker Committed by Daniel Verkamp
Browse files

bdev: Fix race condition when testing whether QoS is enabled 



When testing whether QoS is enabled, the code previously
checked mutable values in the bdev itself. Instead, it needs
to check the flag in the channel.

Right now, QoS can only be configured statically when the
bdev is created. This means that no channels will exist
prior to QoS being turned on, which simplifies setting
the per-channel flag (only need to set it when a channel
is created).

Change-Id: I59e56c64c18c262cc2a7f71a6dde8329edb35db7
Signed-off-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/407354


Reviewed-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarGangCao <gang.cao@intel.com>
Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
parent d859e3cc

lib/bdev/bdev.c

+9 −7
Original line number Diff line number Diff line
@@ -938,8 +938,7 @@ spdk_bdev_io_submit(struct spdk_bdev_io *bdev_io) 


	assert(bdev_io->status == SPDK_BDEV_IO_STATUS_PENDING);



	/* QoS channel and thread have been properly configured */

	if (bdev->ios_per_sec > 0 && bdev->qos_channel && bdev->qos_thread) {

	if (bdev_io->ch->flags & BDEV_CH_QOS_ENABLED) {

		bdev_io->io_submit_ch = bdev_io->ch;

		bdev_io->ch = bdev->qos_channel;

		spdk_thread_send_msg(bdev->qos_thread, _spdk_bdev_io_submit, bdev_io);
@@ -1140,13 +1139,16 @@ spdk_bdev_channel_create(void *io_device, void *ctx_buf) 
	pthread_mutex_lock(&bdev->mutex);



	/* Rate limiting on this bdev enabled */

	if (bdev->ios_per_sec > 0 && bdev->qos_channel == NULL) {

	if (bdev->ios_per_sec) {

		if (bdev->qos_channel == NULL) {

			if (spdk_bdev_qos_channel_create(bdev) != 0) {

				_spdk_bdev_channel_destroy_resource(ch);

				pthread_mutex_unlock(&bdev->mutex);

				return -1;

			}

		}

		ch->flags |= BDEV_CH_QOS_ENABLED;

	}



	bdev->channel_count++;

test/unit/lib/bdev/mt/bdev.c/bdev_ut.c

+19 −0
Original line number Diff line number Diff line
@@ -617,6 +617,7 @@ static void 
basic_qos(void)

{

	struct spdk_io_channel *io_ch[2];

	struct spdk_bdev_channel *bdev_ch[2];

	struct spdk_bdev *bdev;

	enum spdk_bdev_io_status status;

	int rc;
@@ -631,9 +632,13 @@ basic_qos(void) 


	set_thread(0);

	io_ch[0] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[0] = spdk_io_channel_get_ctx(io_ch[0]);

	CU_ASSERT(bdev_ch[0]->flags == BDEV_CH_QOS_ENABLED);



	set_thread(1);

	io_ch[1] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[1] = spdk_io_channel_get_ctx(io_ch[1]);

	CU_ASSERT(bdev_ch[1]->flags == BDEV_CH_QOS_ENABLED);



	/*

	 * Send an I/O on thread 0, which is where the QoS thread is running.
@@ -683,9 +688,13 @@ basic_qos(void) 
	/* Create the channels in reverse order. */

	set_thread(1);

	io_ch[1] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[1] = spdk_io_channel_get_ctx(io_ch[1]);

	CU_ASSERT(bdev_ch[1]->flags == BDEV_CH_QOS_ENABLED);



	set_thread(0);

	io_ch[0] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[0] = spdk_io_channel_get_ctx(io_ch[0]);

	CU_ASSERT(bdev_ch[0]->flags == BDEV_CH_QOS_ENABLED);



	/* Confirm that the qos tracking was re-enabled */

	CU_ASSERT(bdev->qos_channel != NULL);
@@ -706,6 +715,7 @@ static void 
io_during_qos_queue(void)

{

	struct spdk_io_channel *io_ch[2];

	struct spdk_bdev_channel *bdev_ch[2];

	struct spdk_bdev *bdev;

	enum spdk_bdev_io_status status0, status1;

	int rc;
@@ -722,9 +732,13 @@ io_during_qos_queue(void) 
	/* Create channels */

	set_thread(0);

	io_ch[0] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[0] = spdk_io_channel_get_ctx(io_ch[0]);

	CU_ASSERT(bdev_ch[0]->flags == BDEV_CH_QOS_ENABLED);



	set_thread(1);

	io_ch[1] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[1] = spdk_io_channel_get_ctx(io_ch[1]);

	CU_ASSERT(bdev_ch[1]->flags == BDEV_CH_QOS_ENABLED);



	/* Send two I/O */

	status1 = SPDK_BDEV_IO_STATUS_PENDING;
@@ -781,6 +795,7 @@ static void 
io_during_qos_reset(void)

{

	struct spdk_io_channel *io_ch[2];

	struct spdk_bdev_channel *bdev_ch[2];

	struct spdk_bdev *bdev;

	enum spdk_bdev_io_status status0, status1, reset_status;

	int rc;
@@ -797,9 +812,13 @@ io_during_qos_reset(void) 
	/* Create channels */

	set_thread(0);

	io_ch[0] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[0] = spdk_io_channel_get_ctx(io_ch[0]);

	CU_ASSERT(bdev_ch[0]->flags == BDEV_CH_QOS_ENABLED);



	set_thread(1);

	io_ch[1] = spdk_bdev_get_io_channel(g_desc);

	bdev_ch[1] = spdk_io_channel_get_ctx(io_ch[1]);

	CU_ASSERT(bdev_ch[1]->flags == BDEV_CH_QOS_ENABLED);



	/* Send two I/O. One of these gets queued by QoS. The other is sitting at the disk. */

	status1 = SPDK_BDEV_IO_STATUS_PENDING;