Commit 48454bb2 authored Nov 28, 2024 by Shuhei Matsumoto Committed by Ben Walker Dec 04, 2024

bdev/nvme: Add lock to unprotected operations around detach controller



When a NVMe bdev has a multipath configuration made of two NVMe-oF controllers
and the two NVMe-oF controllers are detached, the NVMe bdev is deleted but sometimes
one of NVMe-oF controllers failed to delete.

The root cause analysis showed that the failed NVMe-oF controller was not tried
to delete at all.

It is very likely that some complex race condition occurred.

Then, it was found that a few critical operations were not protected by lock.

Checking nvme_ns->bdev was protected by lock but clearing nvme_ns->bdev was not protected
by the same lock.

Removing nvme_ns from bdev->nvme_ns_list was protected by lock but traversing
bdev->nvme_ns_list was not protected by the lock.

Hence, add these missing locks.

The lock ordering should be nvme_bdev->mutex is first and nvme_ctrlr->mutex is second.

nvme_ctrlr_depopulate_namespaces() does not hold lock while traversing nvme_ns_tree.
However, nvme_ns_tree is deleted at the final controller deletion. Hence, this will not be
necessary for now

Signed-off-by: Shuhei Matsumoto <smatsumoto@nvidia.com>
Change-Id: I974adbf87ca02e1b2f167dd3eab485934ff03774
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/25483


Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Community CI Samsung <spdk.community.ci.samsung@gmail.com>
Reviewed-by: GangCao <gang.cao@intel.com>
Reviewed-by: Jim Harris <jim.harris@nvidia.com>
Reviewed-by: Ben Walker <ben@nvidia.com>
Community-CI: Mellanox Build Bot

parent 4b59d789

module/bdev/nvme/bdev_nvme.c

+7 −0

Original line number	Diff line number	Diff line
		@@ -1905,6 +1905,8 @@ bdev_nvme_destruct(void *ctx)

		SPDK_DTRACE_PROBE2(bdev_nvme_destruct, nbdev->nbdev_ctrlr->name, nbdev->nsid);

		pthread_mutex_lock(&nbdev->mutex);

		TAILQ_FOREACH_SAFE(nvme_ns, &nbdev->nvme_ns_list, tailq, tmp_nvme_ns) {
		pthread_mutex_lock(&nvme_ns->ctrlr->mutex);

		@@ -1922,6 +1924,8 @@ bdev_nvme_destruct(void *ctx)
		}
		}

		pthread_mutex_unlock(&nbdev->mutex);

		pthread_mutex_lock(&g_bdev_nvme_mutex);
		TAILQ_REMOVE(&nbdev->nbdev_ctrlr->bdevs, nbdev, tailq);
		pthread_mutex_unlock(&g_bdev_nvme_mutex);
		@@ -5016,7 +5020,10 @@ nvme_ctrlr_depopulate_namespace(struct nvme_ctrlr nvme_ctrlr, struct nvme_ns n
		* and clear nvme_ns->bdev here.
		*/
		TAILQ_REMOVE(&nbdev->nvme_ns_list, nvme_ns, tailq);

		pthread_mutex_lock(&nvme_ns->ctrlr->mutex);
		nvme_ns->bdev = NULL;
		pthread_mutex_unlock(&nvme_ns->ctrlr->mutex);

		pthread_mutex_unlock(&nbdev->mutex);