From cf64422ad71e4294d5683f6e51bfee7e12832000 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Thu, 26 Jan 2023 17:10:16 -0700 Subject: [PATCH] bdev: call unregister callback on correct thread We should always called the unregister callback on the same thread that spdk_bdev_unregister() was originally called. So save the thread pointer and use an spdk_thread_send_msg() to make sure it gets called on the correct thread when the unregister finishes. Also add unit test that reproduces the original issue. Fixes issue #2883. Signed-off-by: Jim Harris Change-Id: Ib3d89368aa358bc7a8db46a8a8cb6339340469d9 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/16554 Reviewed-by: Mike Gerdts Reviewed-by: Xiaodong Liu Reviewed-by: Shuhei Matsumoto Reviewed-by: Aleksey Marchuk Reviewed-by: Konrad Sztyber Tested-by: SPDK CI Jenkins --- include/spdk/bdev_module.h | 9 +++++ lib/bdev/bdev.c | 7 ++++ test/unit/lib/bdev/mt/bdev.c/bdev_ut.c | 50 ++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/include/spdk/bdev_module.h b/include/spdk/bdev_module.h index d40583e28..8101ebe60 100644 --- a/include/spdk/bdev_module.h +++ b/include/spdk/bdev_module.h @@ -557,6 +557,9 @@ struct spdk_bdev { /** Unregister call context */ void *unregister_ctx; + /** Thread that issued the unregister. The cb must be called on this thread. */ + struct spdk_thread *unregister_td; + /** List of open descriptors for this block device. */ TAILQ_HEAD(, spdk_bdev_desc) open_descs; @@ -900,6 +903,9 @@ int spdk_bdev_register(struct spdk_bdev *bdev); * and manually close all the descriptors with spdk_bdev_close(). * The actual bdev unregistration may be deferred until all descriptors are closed. * + * The cb_fn will be called from the context of the same spdk_thread that called + * spdk_bdev_unregister. + * * 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(). * @@ -915,6 +921,9 @@ void spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, * and manually close all the descriptors with spdk_bdev_close(). * The actual bdev unregistration may be deferred until all descriptors are closed. * + * The cb_fn will be called from the context of the same spdk_thread that called + * spdk_bdev_unregister. + * * \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. diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index 03220c041..a27a90f6c 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -6821,6 +6821,12 @@ bdev_destroy_cb(void *io_device) void *cb_arg; bdev = __bdev_from_io_dev(io_device); + + if (bdev->internal.unregister_td != spdk_get_thread()) { + spdk_thread_send_msg(bdev->internal.unregister_td, bdev_destroy_cb, io_device); + return; + } + cb_fn = bdev->internal.unregister_cb; cb_arg = bdev->internal.unregister_ctx; @@ -6982,6 +6988,7 @@ spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void bdev->internal.status = SPDK_BDEV_STATUS_UNREGISTERING; bdev->internal.unregister_cb = cb_fn; bdev->internal.unregister_ctx = cb_arg; + bdev->internal.unregister_td = thread; spdk_spin_unlock(&bdev->internal.spinlock); spdk_spin_unlock(&g_bdev_mgr.spinlock); 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 65a435f0f..589e3243a 100644 --- a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c @@ -448,6 +448,55 @@ unregister_and_close(void) teardown_test(); } +static void +unregister_and_close_different_threads(void) +{ + bool done; + struct spdk_bdev_desc *desc = NULL; + + setup_test(); + set_thread(0); + + /* setup_test() automatically opens the bdev, + * but this test needs to do that in a different + * way. */ + spdk_bdev_close(g_desc); + poll_threads(); + + set_thread(1); + spdk_bdev_open_ext("ut_bdev", true, _bdev_event_cb, NULL, &desc); + SPDK_CU_ASSERT_FATAL(desc != NULL); + done = false; + + set_thread(0); + spdk_bdev_unregister(&g_bdev.bdev, _bdev_unregistered, &done); + + /* Poll the threads to allow all events to be processed */ + poll_threads(); + + /* Make sure the bdev was not unregistered. We still have a + * descriptor open */ + CU_ASSERT(done == false); + + /* Close the descriptor on thread 1. Poll the thread and confirm the + * unregister did not complete, since it was unregistered on thread 0. + */ + set_thread(1); + spdk_bdev_close(desc); + poll_thread(1); + CU_ASSERT(done == false); + + /* Now poll thread 0 and confirm the unregister completed. */ + set_thread(0); + poll_thread(0); + CU_ASSERT(done == true); + + /* Restore the original g_bdev so that we can use teardown_test(). */ + register_bdev(&g_bdev, "ut_bdev", &g_io_device); + spdk_bdev_open_ext("ut_bdev", true, _bdev_event_cb, NULL, &g_desc); + teardown_test(); +} + static void reset_done(struct spdk_bdev_io *bdev_io, bool success, void *cb_arg) { @@ -2473,6 +2522,7 @@ main(int argc, char **argv) CU_ADD_TEST(suite, basic); CU_ADD_TEST(suite, unregister_and_close); + CU_ADD_TEST(suite, unregister_and_close_different_threads); CU_ADD_TEST(suite, basic_qos); CU_ADD_TEST(suite, put_channel_during_reset); CU_ADD_TEST(suite, aborted_reset);