From 01f769c62aa784ef7f57c10abe5c55b0a1fdf514 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Thu, 22 Oct 2020 06:10:57 +0900 Subject: [PATCH] bdev/nvme: Ensure ctrlr->destruct is set only once Checking if destruct is false and setting destruct to true are separated by mutex in remove_cb() and bdev_nvme_library_fini(). It was possible that multiple threads called nvme_bdev_ctrlr_destruct() because the caller could call nvme_bdev_ctrlr_destruct() before setting destruct to true after knowing it was false. This patch ensures that nvme_bdev_ctrlr_destruct() is called only once. Set ctrlr->destruct to true without releasing mutex after checking if ctrlr->destruct is false. If destruct is set to true before calling nvme_ctrlr_depopulate_namespaces(), nvme_ctrlr_depopulate_namespace_done() may call nvme_bdev_ctrlr_destruct() because it is likely that reference count is zero and destruct is true. In this case, remove_cb() or bdev_nvme_library_fini() cannot call nvme_bdev_destruct() after returning from nvme_ctrlr_depopulate_namespaces(). On the other hand, if a controller has no namespace, nvme_ctrlr_depopulate_namespaces() does nothing and nvme_ctrlr_depopulate_namespace_done() is not called. Hence remove_cb() or bdev_nvme_library_fini() has to call nvme_bdev_destruct() after returning nvme_ctrlr_depopulate_namespaces(). To unify both cases, initialize reference count to one as a sentinel value and remove_cb() and bdev_nvme_library_fini() decrement reference count and then calls nvme_bdev_ctrlr_destruct() if reference count is zero after returning from nvme_ctrlr_depopulate_namespaces(). Additionally, add assert to check if reference count is not negative to find bug in future. Signed-off-by: Shuhei Matsumoto Change-Id: I8a617b5aa4d0a9faff832e63c2ed4b353341dd6b Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4817 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Reviewed-by: Changpeng Liu Reviewed-by: Jim Harris Reviewed-by: Aleksey Marchuk Tested-by: SPDK CI Jenkins --- module/bdev/nvme/bdev_nvme.c | 11 ++++++++--- module/bdev/nvme/common.c | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/module/bdev/nvme/bdev_nvme.c b/module/bdev/nvme/bdev_nvme.c index 53b6f940a..24bf9a264 100644 --- a/module/bdev/nvme/bdev_nvme.c +++ b/module/bdev/nvme/bdev_nvme.c @@ -1200,6 +1200,7 @@ void nvme_ctrlr_depopulate_namespace_done(struct nvme_bdev_ctrlr *nvme_bdev_ctrlr) { pthread_mutex_lock(&g_bdev_nvme_mutex); + assert(nvme_bdev_ctrlr->ref > 0); nvme_bdev_ctrlr->ref--; if (nvme_bdev_ctrlr->ref == 0 && nvme_bdev_ctrlr->destruct) { @@ -1426,7 +1427,7 @@ nvme_bdev_ctrlr_create(struct spdk_nvme_ctrlr *ctrlr, nvme_bdev_ctrlr->thread = spdk_get_thread(); nvme_bdev_ctrlr->adminq_timer_poller = NULL; nvme_bdev_ctrlr->ctrlr = ctrlr; - nvme_bdev_ctrlr->ref = 0; + nvme_bdev_ctrlr->ref = 1; nvme_bdev_ctrlr->connected_trid = &trid_entry->trid; nvme_bdev_ctrlr->name = strdup(name); if (nvme_bdev_ctrlr->name == NULL) { @@ -1541,12 +1542,14 @@ remove_cb(void *cb_ctx, struct spdk_nvme_ctrlr *ctrlr) pthread_mutex_unlock(&g_bdev_nvme_mutex); return; } + nvme_bdev_ctrlr->destruct = true; pthread_mutex_unlock(&g_bdev_nvme_mutex); nvme_ctrlr_depopulate_namespaces(nvme_bdev_ctrlr); pthread_mutex_lock(&g_bdev_nvme_mutex); - nvme_bdev_ctrlr->destruct = true; + assert(nvme_bdev_ctrlr->ref > 0); + nvme_bdev_ctrlr->ref--; if (nvme_bdev_ctrlr->ref == 0) { pthread_mutex_unlock(&g_bdev_nvme_mutex); nvme_bdev_ctrlr_destruct(nvme_bdev_ctrlr); @@ -2020,14 +2023,16 @@ bdev_nvme_library_fini(void) */ continue; } + nvme_bdev_ctrlr->destruct = true; pthread_mutex_unlock(&g_bdev_nvme_mutex); nvme_ctrlr_depopulate_namespaces(nvme_bdev_ctrlr); pthread_mutex_lock(&g_bdev_nvme_mutex); - nvme_bdev_ctrlr->destruct = true; + assert(nvme_bdev_ctrlr->ref > 0); + nvme_bdev_ctrlr->ref--; if (nvme_bdev_ctrlr->ref == 0) { pthread_mutex_unlock(&g_bdev_nvme_mutex); nvme_bdev_ctrlr_destruct(nvme_bdev_ctrlr); diff --git a/module/bdev/nvme/common.c b/module/bdev/nvme/common.c index 571f2f8a7..75ffaf469 100644 --- a/module/bdev/nvme/common.c +++ b/module/bdev/nvme/common.c @@ -196,6 +196,7 @@ nvme_bdev_detach_bdev_from_ns(struct nvme_bdev *nvme_disk) struct nvme_bdev_ctrlr *ctrlr = nvme_disk->nvme_ns->ctrlr; pthread_mutex_lock(&g_bdev_nvme_mutex); + assert(ctrlr->ref > 0); ctrlr->ref--; TAILQ_REMOVE(&nvme_disk->nvme_ns->bdevs, nvme_disk, tailq);