From 3debc9d89abdc825c423dccb8e723d5dde1e4fb5 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Thu, 25 Nov 2021 18:43:17 +0900 Subject: [PATCH] bdev/nvme: Remove the failover_in_progress flag from struct nvme_ctrlr The failover_in_progress flag is used to decide the return value of bdev_nvme_failover(). bdev_nvme_delete() calls bdev_nvme_failover() with remove=true to remove nvme_ctrlr->active_path_id. However bdev_nvme_failover() returns zero if nvme_ctrlr->failover_in_progress is true. bdev_nvme_failover() may return zero even if it does not remove nvme_ctrlr->active_path_id. The following will be better. bdev_nvme_failover() returns -EBUSY if nvme_ctrlr->resetting is true, and the caller repeats calling bdev_nvme_failover() until the target trid becomes alternative path or bdev_nvme_failover() returns zero. To do that, the failover_in_progress flag is not necessary any more. Removing the failover_in_progress will also simplify the following patches to unify ctrlr reset and failover. Signed-off-by: Shuhei Matsumoto Change-Id: I57ab944beb1d06ea4def144c81c69705860de35f Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/10441 Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Aleksey Marchuk --- module/bdev/nvme/bdev_nvme.c | 15 +++-------- module/bdev/nvme/bdev_nvme.h | 1 - .../lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c | 25 +++---------------- 3 files changed, 7 insertions(+), 34 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 539e3ad84..c10ac8001 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -1283,7 +1283,6 @@ _bdev_nvme_reset_complete(struct spdk_io_channel_iter *i, int status) pthread_mutex_lock(&nvme_ctrlr->mutex); nvme_ctrlr->resetting = false; - nvme_ctrlr->failover_in_progress = false; path_id = TAILQ_FIRST(&nvme_ctrlr->trids); assert(path_id != NULL); @@ -1580,7 +1579,7 @@ static int bdev_nvme_failover_start(struct nvme_ctrlr *nvme_ctrlr, bool remove) { struct nvme_path_id *path_id = NULL, *next_path = NULL; - int rc; + int rc __attribute__((unused)); pthread_mutex_lock(&nvme_ctrlr->mutex); if (nvme_ctrlr->destruct) { @@ -1595,14 +1594,9 @@ bdev_nvme_failover_start(struct nvme_ctrlr *nvme_ctrlr, bool remove) next_path = TAILQ_NEXT(path_id, link); if (nvme_ctrlr->resetting) { - if (next_path && !nvme_ctrlr->failover_in_progress) { - rc = -EBUSY; - } else { - rc = -EALREADY; - } pthread_mutex_unlock(&nvme_ctrlr->mutex); SPDK_NOTICELOG("Unable to perform reset, already in progress.\n"); - return rc; + return -EBUSY; } nvme_ctrlr->resetting = true; @@ -1614,7 +1608,6 @@ bdev_nvme_failover_start(struct nvme_ctrlr *nvme_ctrlr, bool remove) SPDK_NOTICELOG("Start failover from %s:%s to %s:%s\n", path_id->trid.traddr, path_id->trid.trsvcid, next_path->trid.traddr, next_path->trid.trsvcid); - nvme_ctrlr->failover_in_progress = true; spdk_nvme_ctrlr_fail(nvme_ctrlr->ctrlr); nvme_ctrlr->active_path_id = next_path; rc = spdk_nvme_ctrlr_set_trid(nvme_ctrlr->ctrlr, &next_path->trid); @@ -1642,11 +1635,9 @@ bdev_nvme_failover(struct nvme_ctrlr *nvme_ctrlr, bool remove) rc = bdev_nvme_failover_start(nvme_ctrlr, remove); if (rc == 0) { spdk_thread_send_msg(nvme_ctrlr->thread, _bdev_nvme_reset, nvme_ctrlr); - } else if (rc != -EALREADY) { - return rc; } - return 0; + return rc; } static int bdev_nvme_unmap(struct nvme_bdev_io *bio, uint64_t offset_blocks, diff --git a/module/bdev/nvme/bdev_nvme.h b/module/bdev/nvme/bdev_nvme.h index 61bf17430..2f377c32b 100644 --- a/module/bdev/nvme/bdev_nvme.h +++ b/module/bdev/nvme/bdev_nvme.h @@ -104,7 +104,6 @@ struct nvme_ctrlr { int ref; uint32_t resetting : 1; - uint32_t failover_in_progress : 1; uint32_t destruct : 1; uint32_t ana_log_page_updating : 1; /** 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 0b1a47b16..bc67770fa 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 @@ -1451,18 +1451,10 @@ test_failover_ctrlr(void) nvme_ctrlr->resetting = true; rc = bdev_nvme_failover(nvme_ctrlr, false); - CU_ASSERT(rc == 0); + CU_ASSERT(rc == -EBUSY); - /* Case 3: failover is in progress. */ - nvme_ctrlr->failover_in_progress = true; - - rc = bdev_nvme_failover(nvme_ctrlr, false); - CU_ASSERT(rc == 0); - CU_ASSERT(curr_trid->is_failed == false); - - /* Case 4: reset completes successfully. */ + /* Case 3: reset completes successfully. */ nvme_ctrlr->resetting = false; - nvme_ctrlr->failover_in_progress = false; rc = bdev_nvme_failover(nvme_ctrlr, false); CU_ASSERT(rc == 0); @@ -1492,27 +1484,19 @@ test_failover_ctrlr(void) /* Failover starts from thread 1. */ set_thread(1); - /* Case 5: reset is in progress. */ + /* Case 4: reset is in progress. */ nvme_ctrlr->resetting = true; rc = bdev_nvme_failover(nvme_ctrlr, false); CU_ASSERT(rc == -EBUSY); - /* Case 5: failover is in progress. */ - nvme_ctrlr->failover_in_progress = true; - - rc = bdev_nvme_failover(nvme_ctrlr, false); - CU_ASSERT(rc == 0); - - /* Case 6: failover completes successfully. */ + /* Case 5: failover completes successfully. */ nvme_ctrlr->resetting = false; - nvme_ctrlr->failover_in_progress = false; rc = bdev_nvme_failover(nvme_ctrlr, false); CU_ASSERT(rc == 0); CU_ASSERT(nvme_ctrlr->resetting == true); - CU_ASSERT(nvme_ctrlr->failover_in_progress == true); next_trid = TAILQ_FIRST(&nvme_ctrlr->trids); SPDK_CU_ASSERT_FATAL(next_trid != NULL); @@ -1523,7 +1507,6 @@ test_failover_ctrlr(void) poll_threads(); CU_ASSERT(nvme_ctrlr->resetting == false); - CU_ASSERT(nvme_ctrlr->failover_in_progress == false); spdk_put_io_channel(ch2);