From 140eaaa0844159875e3a543ee634494494b66406 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Thu, 5 Jul 2018 08:57:33 -0700 Subject: [PATCH] bdev: submit queued IO after disabling QoS There's no reason to abort IO that have been queued due to QoS limits, when QoS is switched from enabled to disabled. Submit them to the bdev instead. Fixes issue #357. Signed-off-by: Jim Harris Change-Id: If5eafc53418ac686120e1d6a1da884b42cef845e Reviewed-on: https://review.gerrithub.io/418128 Tested-by: SPDK Automated Test System Reviewed-by: Seth Howell Reviewed-by: Daniel Verkamp Reviewed-by: Ben Walker --- lib/bdev/bdev.c | 20 +++++++++- test/unit/lib/bdev/mt/bdev.c/bdev_ut.c | 51 +++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index a6644dddc8..8973edced0 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -3238,6 +3238,7 @@ _spdk_bdev_disable_qos_done(void *cb_arg) { struct set_qos_limit_ctx *ctx = cb_arg; struct spdk_bdev *bdev = ctx->bdev; + struct spdk_bdev_io *bdev_io; struct spdk_bdev_qos *qos; pthread_mutex_lock(&bdev->internal.mutex); @@ -3245,7 +3246,24 @@ _spdk_bdev_disable_qos_done(void *cb_arg) bdev->internal.qos = NULL; pthread_mutex_unlock(&bdev->internal.mutex); - _spdk_bdev_abort_queued_io(&qos->queued, qos->ch); + while (!TAILQ_EMPTY(&qos->queued)) { + /* Send queued I/O back to their original thread for resubmission. */ + bdev_io = TAILQ_FIRST(&qos->queued); + TAILQ_REMOVE(&qos->queued, bdev_io, internal.link); + + if (bdev_io->internal.io_submit_ch) { + /* + * Channel was changed when sending it to the QoS thread - change it back + * before sending it back to the original thread. + */ + bdev_io->internal.ch = bdev_io->internal.io_submit_ch; + bdev_io->internal.io_submit_ch = NULL; + } + + spdk_thread_send_msg(spdk_io_channel_get_thread(bdev_io->internal.ch->channel), + _spdk_bdev_io_submit, bdev_io); + } + spdk_put_io_channel(spdk_io_channel_from_ctx(qos->ch)); spdk_poller_unregister(&qos->poller); 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 cf583f0cdb..81c996e4c8 100644 --- a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c @@ -1061,7 +1061,8 @@ qos_dynamic_enable(void) struct spdk_io_channel *io_ch[2]; struct spdk_bdev_channel *bdev_ch[2]; struct spdk_bdev *bdev; - int status, second_status; + enum spdk_bdev_io_status bdev_io_status[2]; + int status, second_status, rc, i; setup_test(); reset_time(); @@ -1091,6 +1092,38 @@ qos_dynamic_enable(void) CU_ASSERT((bdev_ch[0]->flags & BDEV_CH_QOS_ENABLED) != 0); CU_ASSERT((bdev_ch[1]->flags & BDEV_CH_QOS_ENABLED) != 0); + /* + * Submit and complete 10 I/O to fill the QoS allotment for this timeslice. + * Additional I/O will then be queued. + */ + set_thread(0); + for (i = 0; i < 10; i++) { + bdev_io_status[0] = SPDK_BDEV_IO_STATUS_PENDING; + rc = spdk_bdev_read_blocks(g_desc, io_ch[0], NULL, 0, 1, io_during_io_done, &bdev_io_status[0]); + CU_ASSERT(rc == 0); + CU_ASSERT(bdev_io_status[0] == SPDK_BDEV_IO_STATUS_PENDING); + poll_thread(0); + stub_complete_io(g_bdev.io_target, 0); + CU_ASSERT(bdev_io_status[0] == SPDK_BDEV_IO_STATUS_SUCCESS); + } + + /* + * Send two more I/O. These I/O will be queued since the current timeslice allotment has been + * filled already. We want to test that when QoS is disabled that these two I/O: + * 1) are not aborted + * 2) are sent back to their original thread for resubmission + */ + bdev_io_status[0] = SPDK_BDEV_IO_STATUS_PENDING; + rc = spdk_bdev_read_blocks(g_desc, io_ch[0], NULL, 0, 1, io_during_io_done, &bdev_io_status[0]); + CU_ASSERT(rc == 0); + CU_ASSERT(bdev_io_status[0] == SPDK_BDEV_IO_STATUS_PENDING); + set_thread(1); + bdev_io_status[1] = SPDK_BDEV_IO_STATUS_PENDING; + rc = spdk_bdev_read_blocks(g_desc, io_ch[1], NULL, 0, 1, io_during_io_done, &bdev_io_status[1]); + CU_ASSERT(rc == 0); + CU_ASSERT(bdev_io_status[1] == SPDK_BDEV_IO_STATUS_PENDING); + poll_threads(); + /* Disable QoS */ status = -1; spdk_bdev_set_qos_limit_iops(bdev, 0, qos_dynamic_enable_done, &status); @@ -1099,6 +1132,22 @@ qos_dynamic_enable(void) CU_ASSERT((bdev_ch[0]->flags & BDEV_CH_QOS_ENABLED) == 0); CU_ASSERT((bdev_ch[1]->flags & BDEV_CH_QOS_ENABLED) == 0); + /* + * All I/O should have been resubmitted back on their original thread. Complete + * all I/O on thread 0, and ensure that only the thread 0 I/O was completed. + */ + set_thread(0); + stub_complete_io(g_bdev.io_target, 0); + poll_threads(); + CU_ASSERT(bdev_io_status[0] == SPDK_BDEV_IO_STATUS_SUCCESS); + CU_ASSERT(bdev_io_status[1] == SPDK_BDEV_IO_STATUS_PENDING); + + /* Now complete all I/O on thread 1 and ensure the thread 1 I/O was completed. */ + set_thread(1); + stub_complete_io(g_bdev.io_target, 0); + poll_threads(); + CU_ASSERT(bdev_io_status[1] == SPDK_BDEV_IO_STATUS_SUCCESS); + /* Disable QoS again */ status = -1; spdk_bdev_set_qos_limit_iops(bdev, 0, qos_dynamic_enable_done, &status);