Commit 01f769c6 authored Oct 22, 2020 by Shuhei Matsumoto Committed by Tomasz Zawadzki Nov 16, 2020

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 <shuhei.matsumoto.xt@hitachi.com>
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 <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>

parent 844e6bfa

module/bdev/nvme/bdev_nvme.c

+8 −3

Original line number	Diff line number	Diff line
		@@ -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);

module/bdev/nvme/common.c

+1 −0

Original line number	Diff line number	Diff line
		@@ -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);