bdev: examine and register on app thread

This introduces a deprecation for calling spdk_bdev_register() and
spdk_bdev_examine() on a thread other than the app thread. The
deprecation period starts in SPDK 23.01 and removal is expected in SPDK
23.05.

The intent of this deprecation is to ensure that bdev modules'
examine_config() and examine_disk() callbacks are only ever called on
the app thread. This largely a formalization of what has long happened
due to the RPC poller running on the first thread started by
spdk_app_start().

Signed-off-by: Mike Gerdts <mgerdts@nvidia.com>
Change-Id: Ic9d7b87b6522be20357d2eab2d0c77cd5753452f
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15690
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Mellanox Build Bot
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Aleksey Marchuk <alexeymar@nvidia.com>
Reviewed-by: Shuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: Mateusz Kozlowski <mateusz.kozlowski@intel.com>

deprecation.md

@@ -21,7 +21,21 @@ The tags can be matched with the level 4 headers below.
 
 ### nvme
 
-#### `nvme_ctrlr_prepare_for_reset``
+#### `nvme_ctrlr_prepare_for_reset`
 
 Deprecated `spdk_nvme_ctrlr_prepare_for_reset` API, which will be removed in SPDK 22.01.
 For PCIe transport, `spdk_nvme_ctrlr_disconnect` should be used before freeing I/O qpairs.
+
+### bdev
+
+#### `bdev_register_examine_thread`
+
+Deprecated calling `spdk_bdev_register()` and `spdk_bdev_examine()` from a thread other than the
+app thread. See `spdk_thread_get_app_thread()`. Starting in SPDK 23.05, calling
+`spdk_bdev_register()` or `spdk_bdev_examine()` from a thread other than the app thread will return
+an error.
+
+With the removal of this deprecation, calls to vbdev modules' `examine_disk()` and
+`examine_config()` callbacks will happen only on the app thread. This means that vbdev module
+maintainers will not need to make any changes to examine callbacks that call `spdk_bdev_register()`
+on the same thread as the examine callback uses.

include/spdk/bdev.h

@@ -252,6 +252,8 @@ int spdk_bdev_wait_for_examine(spdk_bdev_wait_for_examine_cb cb_fn, void *cb_arg
 /**
  * Examine a block device explicitly
  *
+ * This function must be called from the SPDK app thread.
+ *
  * \param name the name or alias of the block device
  * \return 0 if block device was examined successfully, suitable errno value otherwise
  */

include/spdk/bdev_module.h

@@ -849,6 +849,8 @@ struct spdk_bdev_io {
 /**
  * Register a new bdev.
  *
+ * This function must be called from the SPDK app thread.
+ *
  * \param bdev Block device to register.
  *
  * \return 0 on success.

lib/bdev/bdev.c

@@ -61,6 +61,9 @@ int __itt_init_ittlib(const char *, __itt_group_id);
  */
 #define SPDK_BDEV_MAX_CHILDREN_COPY_REQS (8)
 
+SPDK_LOG_DEPRECATION_REGISTER(bdev_register_examine_thread,
+			      "bdev register and examine on non-app thread", "SPDK 23.05", 0);
+
 static const char *qos_rpc_type[] = {"rw_ios_per_sec",
 				     "rw_mbytes_per_sec", "r_mbytes_per_sec", "w_mbytes_per_sec"
 				    };
@@ -677,6 +680,10 @@ spdk_bdev_examine(const char *name)
 	struct spdk_bdev *bdev;
 	struct spdk_bdev_examine_item *item;
 
+	if (spdk_unlikely(spdk_thread_get_app_thread() != spdk_get_thread())) {
+		SPDK_LOG_DEPRECATED(bdev_register_examine_thread);
+	}
+
 	if (g_bdev_opts.bdev_auto_examine) {
 		SPDK_ERRLOG("Manual examine is not allowed if auto examine is enabled");
 		return -EINVAL;
@@ -7214,6 +7221,10 @@ spdk_bdev_register(struct spdk_bdev *bdev)
 	struct spdk_bdev_desc *desc;
 	int rc;
 
+	if (spdk_unlikely(spdk_thread_get_app_thread() != spdk_get_thread())) {
+		SPDK_LOG_DEPRECATED(bdev_register_examine_thread);
+	}
+
 	rc = bdev_register(bdev);
 	if (rc != 0) {
 		return rc;

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

@@ -1,6 +1,7 @@
 /*   SPDX-License-Identifier: BSD-3-Clause
  *   Copyright (C) 2017 Intel Corporation.
  *   All rights reserved.
+ *   Copyright (c) 2022 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
  */
 
 #include "spdk_cunit.h"
@@ -2293,16 +2294,182 @@ unregister_during_reset(void)
 	teardown_test();
 }
 
+static void
+bdev_init_wt_cb(void *done, int rc)
+{
+}
+
+
+static uint64_t
+get_wrong_thread_hits(void)
+{
+	static uint64_t previous = 0;
+	uint64_t ret, current;
+
+	current = spdk_deprecation_get_hits(_deprecated_bdev_register_examine_thread);
+	ret = current - previous;
+	previous = current;
+
+	return ret;
+}
+
+static int
+wrong_thread_setup(void)
+{
+	allocate_cores(1);
+	allocate_threads(2);
+	set_thread(0);
+
+	spdk_bdev_initialize(bdev_init_wt_cb, NULL);
+	spdk_io_device_register(&g_io_device, stub_create_ch, stub_destroy_ch,
+				sizeof(struct ut_bdev_channel), NULL);
+
+	set_thread(1);
+
+	/* Ignore return, just setting the base for the next time it is called. */
+	get_wrong_thread_hits();
+
+	return 0;
+}
+
+static int
+wrong_thread_teardown(void)
+{
+	int rc;
+
+	rc = get_wrong_thread_hits();
+
+	set_thread(0);
+
+	g_teardown_done = false;
+	spdk_io_device_unregister(&g_io_device, NULL);
+	spdk_bdev_finish(finish_cb, NULL);
+	poll_threads();
+	memset(&g_bdev, 0, sizeof(g_bdev));
+	if (!g_teardown_done) {
+		fprintf(stderr, "%s:%d %s: teardown not done\n", __FILE__, __LINE__, __func__);
+		rc = -1;
+	}
+	g_teardown_done = false;
+
+	free_threads();
+	free_cores();
+
+	return rc;
+}
+
+static void
+spdk_bdev_register_wt(void)
+{
+	struct spdk_bdev bdev = { 0 };
+	int rc;
+	bool done;
+
+	bdev.name = "wt_bdev";
+	bdev.fn_table = &fn_table;
+	bdev.module = &bdev_ut_if;
+	bdev.blocklen = 4096;
+	bdev.blockcnt = 1024;
+
+	/* Can register only on app thread */
+	rc = spdk_bdev_register(&bdev);
+	CU_ASSERT(rc == 0);
+	CU_ASSERT(get_wrong_thread_hits() == 1);
+
+	/* Can unregister on any thread */
+	done = false;
+	spdk_bdev_unregister(&bdev, _bdev_unregistered, &done);
+	poll_threads();
+	CU_ASSERT(get_wrong_thread_hits() == 0);
+	CU_ASSERT(done);
+
+	/* Can unregister by name on any thread */
+	rc = spdk_bdev_register(&bdev);
+	CU_ASSERT(rc == 0);
+	CU_ASSERT(get_wrong_thread_hits() == 1);
+	done = false;
+	rc = spdk_bdev_unregister_by_name(bdev.name, bdev.module, _bdev_unregistered, &done);
+	CU_ASSERT(rc == 0);
+	poll_threads();
+	CU_ASSERT(get_wrong_thread_hits() == 0);
+	CU_ASSERT(done);
+}
+
+static void
+wait_for_examine_cb(void *arg)
+{
+	struct spdk_thread **thread = arg;
+
+	*thread = spdk_get_thread();
+}
+
+static void
+spdk_bdev_examine_wt(void)
+{
+	int rc;
+	bool save_auto_examine = g_bdev_opts.bdev_auto_examine;
+	struct spdk_thread *thread;
+
+	g_bdev_opts.bdev_auto_examine = false;
+
+	set_thread(0);
+	register_bdev(&g_bdev, "ut_bdev_wt", &g_io_device);
+	CU_ASSERT(spdk_bdev_get_by_name("ut_bdev_wt") != NULL);
+	set_thread(1);
+
+	/* Can examine only on the app thread */
+	rc = spdk_bdev_examine("ut_bdev_wt");
+	CU_ASSERT(rc == 0);
+	poll_threads();
+	CU_ASSERT(get_wrong_thread_hits() == 1);
+	unregister_bdev(&g_bdev);
+	CU_ASSERT(spdk_bdev_get_by_name("ut_bdev_wt") == NULL);
+	CU_ASSERT(get_wrong_thread_hits() == 0);
+
+	/* Can wait for examine on app thread, callback called on app thread. */
+	set_thread(0);
+	register_bdev(&g_bdev, "ut_bdev_wt", &g_io_device);
+	CU_ASSERT(spdk_bdev_get_by_name("ut_bdev_wt") != NULL);
+	thread = NULL;
+	rc = spdk_bdev_wait_for_examine(wait_for_examine_cb, &thread);
+	CU_ASSERT(rc == 0);
+	poll_threads();
+	CU_ASSERT(get_wrong_thread_hits() == 0);
+	CU_ASSERT(thread == spdk_get_thread());
+	unregister_bdev(&g_bdev);
+	CU_ASSERT(spdk_bdev_get_by_name("ut_bdev_wt") == NULL);
+	CU_ASSERT(get_wrong_thread_hits() == 0);
+
+	/* Can wait for examine on non-app thread, callback called on same thread. */
+	set_thread(0);
+	register_bdev(&g_bdev, "ut_bdev_wt", &g_io_device);
+	CU_ASSERT(spdk_bdev_get_by_name("ut_bdev_wt") != NULL);
+	thread = NULL;
+	rc = spdk_bdev_wait_for_examine(wait_for_examine_cb, &thread);
+	CU_ASSERT(rc == 0);
+	poll_threads();
+	CU_ASSERT(get_wrong_thread_hits() == 0);
+	CU_ASSERT(thread == spdk_get_thread());
+	unregister_bdev(&g_bdev);
+	CU_ASSERT(spdk_bdev_get_by_name("ut_bdev_wt") == NULL);
+	CU_ASSERT(get_wrong_thread_hits() == 0);
+
+	unregister_bdev(&g_bdev);
+	g_bdev_opts.bdev_auto_examine = save_auto_examine;
+}
+
 int
 main(int argc, char **argv)
 {
 	CU_pSuite	suite = NULL;
+	CU_pSuite	suite_wt = NULL;
 	unsigned int	num_failures;
 
 	CU_set_error_action(CUEA_ABORT);
 	CU_initialize_registry();
 
 	suite = CU_add_suite("bdev", NULL, NULL);
+	suite_wt = CU_add_suite("bdev_wrong_thread", wrong_thread_setup, wrong_thread_teardown);
 
 	CU_ADD_TEST(suite, basic);
 	CU_ADD_TEST(suite, unregister_and_close);
@@ -2323,6 +2490,8 @@ main(int argc, char **argv)
 	CU_ADD_TEST(suite, bdev_set_io_timeout_mt);
 	CU_ADD_TEST(suite, lock_lba_range_then_submit_io);
 	CU_ADD_TEST(suite, unregister_during_reset);
+	CU_ADD_TEST(suite_wt, spdk_bdev_register_wt);
+	CU_ADD_TEST(suite_wt, spdk_bdev_examine_wt);
 
 	CU_basic_set_mode(CU_BRM_VERBOSE);
 	CU_basic_run_tests();