From a6e58cc44c24cc6cbadc4a059eb1ae8cc349fd21 Mon Sep 17 00:00:00 2001 From: Mike Gerdts Date: Tue, 29 Nov 2022 12:58:22 -0600 Subject: [PATCH] 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 Change-Id: Ic9d7b87b6522be20357d2eab2d0c77cd5753452f Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/15690 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Reviewed-by: Jim Harris Reviewed-by: Aleksey Marchuk Reviewed-by: Shuhei Matsumoto Reviewed-by: Mateusz Kozlowski --- deprecation.md | 16 ++- include/spdk/bdev.h | 2 + include/spdk/bdev_module.h | 2 + lib/bdev/bdev.c | 11 ++ test/unit/lib/bdev/mt/bdev.c/bdev_ut.c | 169 +++++++++++++++++++++++++ 5 files changed, 199 insertions(+), 1 deletion(-) diff --git a/deprecation.md b/deprecation.md index 88adb2df5..fe9fbeab0 100644 --- a/deprecation.md +++ b/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. diff --git a/include/spdk/bdev.h b/include/spdk/bdev.h index 7242385b4..5f1559b36 100644 --- a/include/spdk/bdev.h +++ b/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 */ diff --git a/include/spdk/bdev_module.h b/include/spdk/bdev_module.h index 275815a65..58c1b17a5 100644 --- a/include/spdk/bdev_module.h +++ b/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. diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index 908ef8cd1..58f2666cd 100644 --- a/lib/bdev/bdev.c +++ b/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; diff --git a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c b/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c index 3763e7edd..65a435f0f 100644 --- a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c +++ b/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();