bdev: Add spdk_bdev_unregister_by_name() to handle race condtions

To unregister a bdev more correctly, we had to call
spdk_bdev_open_ext(), spdk_bdev_desc_get_bdev(), spdk_bdev_unregister(),
and then spdk_bdev_close(). This was correct but complicated.

Hence add a new public API spdk_bdev_unregister_by_name() which does
the whole correct sequence of bdev unregistration.

Signed-off-by: Shuhei Matsumoto <smatsumoto@nvidia.com>
Change-Id: I9068d4ac49dca944436e0ba587308fd356dfef75
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12065
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>

CHANGELOG.md

@@ -12,6 +12,8 @@ fragment copies.
 
 Removed deprecated spdk_bdev_module_finish_done(). Use spdk_bdev_module_fini_done() instead.
 
+A new API `spdk_bdev_unregister_by_name` was added to handle race conditions correctly.
+
 ### idxd
 
 A new parameter `flags` was added to all low level submission and preparation

include/spdk/bdev_module.h

@@ -803,16 +803,35 @@ int spdk_bdev_register(struct spdk_bdev *bdev);
 
 /**
  * Start unregistering a bdev. This will notify each currently open descriptor
- * on this bdev about the hotremoval in hopes that the upper layers will stop
+ * on this bdev of the hotremoval to request the upper layers to stop using this bdev
- * using this bdev and manually close all the descriptors with spdk_bdev_close().
+ * and manually close all the descriptors with spdk_bdev_close().
  * The actual bdev unregistration may be deferred until all descriptors are closed.
  *
+ * Note: spdk_bdev_unregister() can be unsafe unless the bdev is not opened before and
+ * closed after unregistration. It is recommended to use spdk_bdev_unregister_by_name().
+ *
  * \param bdev Block device to unregister.
  * \param cb_fn Callback function to be called when the unregister is complete.
  * \param cb_arg Argument to be supplied to cb_fn
  */
 void spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg);
 
+/**
+ * Start unregistering a bdev. This will notify each currently open descriptor
+ * on this bdev of the hotremoval to request the upper layer to stop using this bdev
+ * and manually close all the descriptors with spdk_bdev_close().
+ * The actual bdev unregistration may be deferred until all descriptors are closed.
+ *
+ * \param bdev_name Block device name to unregister.
+ * \param module Module by which the block device was registered.
+ * \param cb_fn Callback function to be called when the unregister is complete.
+ * \param cb_arg Argument to be supplied to cb_fn
+ *
+ * \return 0 on success, or suitable errno value otherwise
+ */
+int spdk_bdev_unregister_by_name(const char *bdev_name, struct spdk_bdev_module *module,
+				 spdk_bdev_unregister_cb cb_fn, void *cb_arg);
+
 /**
  * Invokes the unregister callback of a bdev backing a virtual bdev.
  *

lib/bdev/bdev.c

@@ -6145,6 +6145,42 @@ spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void
 	}
 }
 
+static void
+_tmp_bdev_event_cb(enum spdk_bdev_event_type type, struct spdk_bdev *bdev, void *ctx)
+{
+	SPDK_NOTICELOG("Unexpected event type: %d\n", type);
+}
+
+int
+spdk_bdev_unregister_by_name(const char *bdev_name, struct spdk_bdev_module *module,
+			     spdk_bdev_unregister_cb cb_fn, void *cb_arg)
+{
+	struct spdk_bdev_desc *desc;
+	struct spdk_bdev *bdev;
+	int rc;
+
+	rc = spdk_bdev_open_ext(bdev_name, false, _tmp_bdev_event_cb, NULL, &desc);
+	if (rc != 0) {
+		SPDK_ERRLOG("Failed to open bdev with name: %s\n", bdev_name);
+		return rc;
+	}
+
+	bdev = spdk_bdev_desc_get_bdev(desc);
+
+	if (bdev->module != module) {
+		spdk_bdev_close(desc);
+		SPDK_ERRLOG("Bdev %s was not registered by the specified module.\n",
+			    bdev_name);
+		return -ENODEV;
+	}
+
+	spdk_bdev_unregister(bdev, cb_fn, cb_arg);
+
+	spdk_bdev_close(desc);
+
+	return 0;
+}
+
 static int
 bdev_start_qos(struct spdk_bdev *bdev)
 {

lib/bdev/spdk_bdev.map

@@ -101,6 +101,7 @@
 	# Public functions in bdev_module.h
 	spdk_bdev_register;
 	spdk_bdev_unregister;
+	spdk_bdev_unregister_by_name;
 	spdk_bdev_destruct_done;
 	spdk_bdev_module_examine_done;
 	spdk_bdev_module_init_done;

test/unit/lib/bdev/bdev.c/bdev_ut.c

@@ -5153,6 +5153,42 @@ bdev_register_uuid_alias(void)
 	poll_threads();
 }
 
+static void
+bdev_unregister_by_name(void)
+{
+	struct spdk_bdev *bdev;
+	int rc;
+
+	bdev = allocate_bdev("bdev");
+
+	g_event_type1 = 0xFF;
+	g_unregister_arg = NULL;
+	g_unregister_rc = -1;
+
+	rc = spdk_bdev_unregister_by_name("bdev1", &bdev_ut_if, bdev_unregister_cb, (void *)0x12345678);
+	CU_ASSERT(rc == -ENODEV);
+
+	rc = spdk_bdev_unregister_by_name("bdev", &vbdev_ut_if, bdev_unregister_cb, (void *)0x12345678);
+	CU_ASSERT(rc == -ENODEV);
+
+	rc = spdk_bdev_unregister_by_name("bdev", &bdev_ut_if, bdev_unregister_cb, (void *)0x12345678);
+	CU_ASSERT(rc == 0);
+
+	/* Check that unregister callback is delayed */
+	CU_ASSERT(g_unregister_arg == NULL);
+	CU_ASSERT(g_unregister_rc == -1);
+
+	poll_threads();
+
+	/* Event callback shall not be issued because device was closed */
+	CU_ASSERT(g_event_type1 == 0xFF);
+	/* Unregister callback is issued */
+	CU_ASSERT(g_unregister_arg == (void *)0x12345678);
+	CU_ASSERT(g_unregister_rc == 0);
+
+	free_bdev(bdev);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -5202,6 +5238,7 @@ main(int argc, char **argv)
 	CU_ADD_TEST(suite, bdev_get_memory_domains);
 	CU_ADD_TEST(suite, bdev_writev_readv_ext);
 	CU_ADD_TEST(suite, bdev_register_uuid_alias);
+	CU_ADD_TEST(suite, bdev_unregister_by_name);
 
 	allocate_cores(1);
 	allocate_threads(1);