Commit cf64422a authored by Jim Harris's avatar Jim Harris Committed by Tomasz Zawadzki
Browse files

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: default avatarJim Harris <james.r.harris@intel.com>
Change-Id: Ib3d89368aa358bc7a8db46a8a8cb6339340469d9

Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/16554


Reviewed-by: default avatarMike Gerdts <mgerdts@nvidia.com>
Reviewed-by: default avatarXiaodong Liu <xiaodong.liu@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 651c558d

include/spdk/bdev_module.h

+9 −0
Original line number Diff line number Diff line
@@ -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.

lib/bdev/bdev.c

+7 −0
Original line number Diff line number Diff line
@@ -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);

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

+50 −0
Original line number Diff line number Diff line
@@ -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);