From 2e1dbc458758ba3d2709299913bc199fa3926bda Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Tue, 10 Apr 2018 14:58:10 -0700 Subject: [PATCH] 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: Ben Walker Reviewed-on: https://review.gerrithub.io/407354 Reviewed-by: Daniel Verkamp Reviewed-by: Jim Harris Reviewed-by: GangCao Tested-by: SPDK Automated Test System --- lib/bdev/bdev.c | 16 +++++++++------- test/unit/lib/bdev/mt/bdev.c/bdev_ut.c | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index 9d7c0fdf1..0258dde8e 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -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,12 +1139,15 @@ 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 (spdk_bdev_qos_channel_create(bdev) != 0) { - _spdk_bdev_channel_destroy_resource(ch); - pthread_mutex_unlock(&bdev->mutex); - return -1; + 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++; diff --git a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c b/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c index 1dab7dc21..2578497cd 100644 --- a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c @@ -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;