From 5d62af41a3f5ef1e75b3ecdb2c8dcafe112d9597 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Fri, 8 Oct 2021 13:15:59 +0900 Subject: [PATCH] bdev/nvme: Complete outstanding reset after canceling pending resets Previously the NVMe bdev module had completed the outstanding reset and then canceled pending resets. This was complex. On the other hand, the generic bdev layer cancels pending resets and then completes the outstanding reset. Following the generic bdev layer simplifies the code and makes us easier to control retry reset, delay retry reset by a few seconds, or stop retry after repeated failures and then delete ctrlr. Update unit tests accordingly. Signed-off-by: Shuhei Matsumoto Change-Id: I9a68422918ebcb052b3a281316ffba9b3450ecd4 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9816 Reviewed-by: Aleksey Marchuk Reviewed-by: Ben Walker Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins --- module/bdev/nvme/bdev_nvme.c | 43 +++++++------------ module/bdev/nvme/bdev_nvme.h | 1 - .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 12 ++++++ 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 4df0e6b669..3bef6bb0a5 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -883,29 +883,6 @@ err: return rc; } -static void -_bdev_nvme_check_pending_destruct(struct nvme_ctrlr *nvme_ctrlr) -{ - pthread_mutex_lock(&nvme_ctrlr->mutex); - if (nvme_ctrlr->destruct_after_reset) { - assert(nvme_ctrlr->ref == 0 && nvme_ctrlr->destruct); - pthread_mutex_unlock(&nvme_ctrlr->mutex); - - spdk_thread_send_msg(nvme_ctrlr->thread, nvme_ctrlr_unregister, - nvme_ctrlr); - } else { - pthread_mutex_unlock(&nvme_ctrlr->mutex); - } -} - -static void -bdev_nvme_check_pending_destruct(struct spdk_io_channel_iter *i, int status) -{ - struct nvme_ctrlr *nvme_ctrlr = spdk_io_channel_iter_get_io_device(i); - - _bdev_nvme_check_pending_destruct(nvme_ctrlr); -} - static void bdev_nvme_complete_pending_resets(struct spdk_io_channel_iter *i) { @@ -928,11 +905,14 @@ bdev_nvme_complete_pending_resets(struct spdk_io_channel_iter *i) } static void -bdev_nvme_reset_complete(struct nvme_ctrlr *nvme_ctrlr, bool success) +_bdev_nvme_reset_complete(struct spdk_io_channel_iter *i, int status) { + struct nvme_ctrlr *nvme_ctrlr = spdk_io_channel_iter_get_io_device(i); + bool success = spdk_io_channel_iter_get_ctx(i) == NULL; struct nvme_path_id *path_id; bdev_nvme_reset_cb reset_cb_fn = nvme_ctrlr->reset_cb_fn; void *reset_cb_arg = nvme_ctrlr->reset_cb_arg; + bool complete_pending_destruct = false; nvme_ctrlr->reset_cb_fn = NULL; nvme_ctrlr->reset_cb_arg = NULL; @@ -954,8 +934,8 @@ bdev_nvme_reset_complete(struct nvme_ctrlr *nvme_ctrlr, bool success) path_id->is_failed = !success; if (nvme_ctrlr->ref == 0 && nvme_ctrlr->destruct) { - /* Destruct ctrlr after clearing pending resets. */ - nvme_ctrlr->destruct_after_reset = true; + /* Complete pending destruct after reset completes. */ + complete_pending_destruct = true; } pthread_mutex_unlock(&nvme_ctrlr->mutex); @@ -964,11 +944,20 @@ bdev_nvme_reset_complete(struct nvme_ctrlr *nvme_ctrlr, bool success) reset_cb_fn(reset_cb_arg, success); } + if (complete_pending_destruct) { + spdk_thread_send_msg(nvme_ctrlr->thread, nvme_ctrlr_unregister, + nvme_ctrlr); + } +} + +static void +bdev_nvme_reset_complete(struct nvme_ctrlr *nvme_ctrlr, bool success) +{ /* Make sure we clear any pending resets before returning. */ spdk_for_each_channel(nvme_ctrlr, bdev_nvme_complete_pending_resets, success ? NULL : (void *)0x1, - bdev_nvme_check_pending_destruct); + _bdev_nvme_reset_complete); } static void diff --git a/module/bdev/nvme/bdev_nvme.h b/module/bdev/nvme/bdev_nvme.h index b20026e92b..a8d3313eea 100644 --- a/module/bdev/nvme/bdev_nvme.h +++ b/module/bdev/nvme/bdev_nvme.h @@ -101,7 +101,6 @@ struct nvme_ctrlr { bool resetting; bool failover_in_progress; bool destruct; - bool destruct_after_reset; /** * PI check flags. This flags is set to NVMe controllers created only * through bdev_nvme_attach_controller RPC or .INI config file. Hot added diff --git a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c index 831f453541..4b24052765 100644 --- a/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c +++ b/test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c @@ -1302,6 +1302,12 @@ test_reset_ctrlr(void) CU_ASSERT(nvme_ctrlr->resetting == true); CU_ASSERT(curr_trid->is_failed == true); + poll_thread_times(1, 1); + CU_ASSERT(nvme_ctrlr->resetting == true); + poll_thread_times(0, 1); + CU_ASSERT(nvme_ctrlr->resetting == true); + poll_thread_times(1, 1); + CU_ASSERT(nvme_ctrlr->resetting == true); poll_thread_times(1, 1); CU_ASSERT(nvme_ctrlr->resetting == false); CU_ASSERT(curr_trid->is_failed == false); @@ -2802,6 +2808,9 @@ test_reconnect_qpair(void) CU_ASSERT(ctrlr_ch2->qpair != NULL); CU_ASSERT(nvme_ctrlr->resetting == true); + poll_thread_times(1, 1); + poll_thread_times(0, 1); + poll_thread_times(1, 1); poll_thread_times(1, 1); CU_ASSERT(nvme_ctrlr->resetting == false); @@ -2825,6 +2834,9 @@ test_reconnect_qpair(void) CU_ASSERT(ctrlr_ch2->qpair == NULL); CU_ASSERT(ctrlr->is_failed == true); + poll_thread_times(1, 1); + poll_thread_times(0, 1); + poll_thread_times(1, 1); poll_thread_times(1, 1); CU_ASSERT(ctrlr->is_failed == true); CU_ASSERT(nvme_ctrlr->resetting == false);