From 3ef479ab163d96d6fd7f28b256d2a93ab42afd8e Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Wed, 6 Dec 2017 14:52:20 -0700 Subject: [PATCH] bdev: Correctly defer completion of resets until channels are unlocked Change-Id: I23f71ff38b805723d74aca639489e0079ecdb993 Signed-off-by: Ben Walker Reviewed-on: https://review.gerrithub.io/390341 Reviewed-by: Jim Harris Reviewed-by: Daniel Verkamp Tested-by: SPDK Automated Test System --- lib/bdev/bdev.c | 58 +++++++++++++++++--------- test/unit/lib/bdev/mt/bdev.c/bdev_ut.c | 20 ++++++++- 2 files changed, 56 insertions(+), 22 deletions(-) diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index b41388445..10accd0a5 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -1321,8 +1321,8 @@ _spdk_bdev_reset_dev(void *io_device, void *ctx, int status) } static int -_spdk_bdev_reset_abort_channel(void *io_device, struct spdk_io_channel *ch, - void *ctx) +_spdk_bdev_reset_freeze_channel(void *io_device, struct spdk_io_channel *ch, + void *ctx) { struct spdk_bdev_channel *channel; struct spdk_bdev_mgmt_channel *mgmt_channel; @@ -1344,7 +1344,7 @@ _spdk_bdev_start_reset(void *ctx) { struct spdk_bdev_channel *ch = ctx; - spdk_for_each_channel(ch->bdev, _spdk_bdev_reset_abort_channel, + spdk_for_each_channel(ch->bdev, _spdk_bdev_reset_freeze_channel, ch, _spdk_bdev_reset_dev); } @@ -1371,19 +1371,6 @@ _spdk_bdev_channel_start_reset(struct spdk_bdev_channel *ch) pthread_mutex_unlock(&bdev->mutex); } -static int -_spdk_bdev_complete_reset_channel(void *io_device, struct spdk_io_channel *_ch, void *ctx) -{ - struct spdk_bdev_channel *ch = spdk_io_channel_get_ctx(_ch); - - ch->flags &= ~BDEV_CH_RESET_IN_PROGRESS; - if (!TAILQ_EMPTY(&ch->queued_resets)) { - _spdk_bdev_channel_start_reset(ch); - } - - return 0; -} - int spdk_bdev_reset(struct spdk_bdev_desc *desc, struct spdk_io_channel *ch, spdk_bdev_io_completion_cb cb, void *cb_arg) @@ -1595,6 +1582,32 @@ _spdk_bdev_io_complete(void *ctx) bdev_io->cb(bdev_io, bdev_io->status == SPDK_BDEV_IO_STATUS_SUCCESS, bdev_io->caller_ctx); } +static void +_spdk_bdev_reset_complete(void *io_device, void *ctx, int status) +{ + struct spdk_bdev_io *bdev_io = ctx; + + if (bdev_io->u.reset.ch_ref != NULL) { + spdk_put_io_channel(bdev_io->u.reset.ch_ref); + bdev_io->u.reset.ch_ref = NULL; + } + + _spdk_bdev_io_complete(bdev_io); +} + +static int +_spdk_bdev_unfreeze_channel(void *io_device, struct spdk_io_channel *_ch, void *ctx) +{ + struct spdk_bdev_channel *ch = spdk_io_channel_get_ctx(_ch); + + ch->flags &= ~BDEV_CH_RESET_IN_PROGRESS; + if (!TAILQ_EMPTY(&ch->queued_resets)) { + _spdk_bdev_channel_start_reset(ch); + } + + return 0; +} + void spdk_bdev_io_complete(struct spdk_bdev_io *bdev_io, enum spdk_bdev_io_status status) { @@ -1604,18 +1617,23 @@ spdk_bdev_io_complete(struct spdk_bdev_io *bdev_io, enum spdk_bdev_io_status sta bdev_io->status = status; if (spdk_unlikely(bdev_io->type == SPDK_BDEV_IO_TYPE_RESET)) { + bool unlock_channels = false; + if (status == SPDK_BDEV_IO_STATUS_NOMEM) { SPDK_ERRLOG("NOMEM returned for reset\n"); } pthread_mutex_lock(&bdev->mutex); if (bdev_io == bdev->reset_in_progress) { bdev->reset_in_progress = NULL; + unlock_channels = true; } pthread_mutex_unlock(&bdev->mutex); - if (bdev_io->u.reset.ch_ref != NULL) { - spdk_put_io_channel(bdev_io->u.reset.ch_ref); + + if (unlock_channels) { + spdk_for_each_channel(bdev, _spdk_bdev_unfreeze_channel, bdev_io, + _spdk_bdev_reset_complete); + return; } - spdk_for_each_channel(bdev, _spdk_bdev_complete_reset_channel, NULL, NULL); } else { assert(bdev_ch->io_outstanding > 0); bdev_ch->io_outstanding--; @@ -1672,7 +1690,7 @@ spdk_bdev_io_complete(struct spdk_bdev_io *bdev_io, enum spdk_bdev_io_status sta } #endif - if (bdev_io->in_submit_request || bdev_io->type == SPDK_BDEV_IO_TYPE_RESET) { + if (bdev_io->in_submit_request) { /* * Defer completion to avoid potential infinite recursion if the * user's completion callback issues a new I/O. 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 5211cd7f8..5d42a35eb 100644 --- a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c @@ -391,7 +391,7 @@ io_during_reset(void) CU_ASSERT(status1 == SPDK_BDEV_IO_STATUS_SUCCESS); /* - * Now submit a reset, and leave it pending while we submit I?O on two different + * Now submit a reset, and leave it pending while we submit I/O on two different * channels. These I/O should be failed by the bdev layer since the reset is in * progress. */ @@ -425,13 +425,29 @@ io_during_reset(void) CU_ASSERT(status0 == SPDK_BDEV_IO_STATUS_FAILED); CU_ASSERT(status1 == SPDK_BDEV_IO_STATUS_FAILED); + /* + * Complete the reset + */ set_thread(0); stub_complete_io(0); + + /* + * Only poll thread 0. We should not get a completion. + */ + poll_thread(0); + CU_ASSERT(status_reset == SPDK_BDEV_IO_STATUS_PENDING); + + /* + * Poll both thread 0 and 1 so the messages can propagate and we + * get a completion. + */ + poll_threads(); + CU_ASSERT(status_reset == SPDK_BDEV_IO_STATUS_SUCCESS); + spdk_put_io_channel(io_ch[0]); set_thread(1); spdk_put_io_channel(io_ch[1]); poll_threads(); - CU_ASSERT(status_reset == SPDK_BDEV_IO_STATUS_SUCCESS); teardown_test(); }