Commit 3522d43a authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Tomasz Zawadzki
Browse files

bdev: Unify _resize_notify() and _remove_notify() 



The next patch will improve media mgmt notifications but it will be
almost same as _resize_notify() and _remove_notify().

On the other hand, there are a few differences between _resize_notify()
and _remove_notify(). _remove_notify() will be better.

To avoid duplication, unify _resize_notify() and _remove_notify() by
adding abstraction event_notify() and _event_notify().

Add unit tests for the complex race conditions.

Signed-off-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Signed-off-by: default avatarMike Gerdts <mgerdts@nvidia.com>
Change-Id: Ibe2478479c61459c0da0db8d28c7273f05275e0f
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/16577


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
parent b5497ac2

lib/bdev/bdev.c

+25 −32
Original line number Diff line number Diff line
@@ -4429,20 +4429,20 @@ spdk_bdev_get_current_qd(struct spdk_bdev *bdev, spdk_bdev_get_current_qd_cb cb_ 
}



static void

_resize_notify(void *arg)

_event_notify(struct spdk_bdev_desc *desc, enum spdk_bdev_event_type type)

{

	struct spdk_bdev_desc *desc = arg;

	assert(desc->thread == spdk_get_thread());



	spdk_spin_lock(&desc->spinlock);

	desc->refs--;

	if (!desc->closed) {

		spdk_spin_unlock(&desc->spinlock);

		desc->callback.event_fn(SPDK_BDEV_EVENT_RESIZE,

		desc->callback.event_fn(type,

					desc->bdev,

					desc->callback.ctx);

		return;

	} else if (0 == desc->refs) {

		/* This descriptor was closed after this resize_notify message was sent.

	} else if (desc->refs == 0) {

		/* This descriptor was closed after this event_notify message was sent.

		 * spdk_bdev_close() could not free the descriptor since this message was

		 * in flight, so we free it now using bdev_desc_free().

		 */
@@ -4453,6 +4453,23 @@ _resize_notify(void *arg) 
	spdk_spin_unlock(&desc->spinlock);

}



static void

event_notify(struct spdk_bdev_desc *desc, spdk_msg_fn event_notify_fn)

{

	spdk_spin_lock(&desc->spinlock);

	desc->refs++;

	spdk_thread_send_msg(desc->thread, event_notify_fn, desc);

	spdk_spin_unlock(&desc->spinlock);

}



static void

_resize_notify(void *ctx)

{

	struct spdk_bdev_desc *desc = ctx;



	_event_notify(desc, SPDK_BDEV_EVENT_RESIZE);

}



int

spdk_bdev_notify_blockcnt_change(struct spdk_bdev *bdev, uint64_t size)

{
@@ -4472,12 +4489,7 @@ spdk_bdev_notify_blockcnt_change(struct spdk_bdev *bdev, uint64_t size) 
	} else {

		bdev->blockcnt = size;

		TAILQ_FOREACH(desc, &bdev->internal.open_descs, link) {

			spdk_spin_lock(&desc->spinlock);

			if (!desc->closed) {

				desc->refs++;

				spdk_thread_send_msg(desc->thread, _resize_notify, desc);

			}

			spdk_spin_unlock(&desc->spinlock);

			event_notify(desc, _resize_notify);

		}

		ret = 0;

	}
@@ -6856,23 +6868,7 @@ _remove_notify(void *arg) 
{

	struct spdk_bdev_desc *desc = arg;



	spdk_spin_lock(&desc->spinlock);

	desc->refs--;



	if (!desc->closed) {

		spdk_spin_unlock(&desc->spinlock);

		desc->callback.event_fn(SPDK_BDEV_EVENT_REMOVE, desc->bdev, desc->callback.ctx);

		return;

	} else if (0 == desc->refs) {

		/* This descriptor was closed after this remove_notify message was sent.

		 * spdk_bdev_close() could not free the descriptor since this message was

		 * in flight, so we free it now using bdev_desc_free().

		 */

		spdk_spin_unlock(&desc->spinlock);

		bdev_desc_free(desc);

		return;

	}

	spdk_spin_unlock(&desc->spinlock);

	_event_notify(desc, SPDK_BDEV_EVENT_REMOVE);

}



/* returns: 0 - bdev removed and ready to be destructed.
@@ -6890,16 +6886,13 @@ bdev_unregister_unsafe(struct spdk_bdev *bdev) 
	/* Notify each descriptor about hotremoval */

	TAILQ_FOREACH_SAFE(desc, &bdev->internal.open_descs, link, tmp) {

		rc = -EBUSY;

		spdk_spin_lock(&desc->spinlock);

		/*

		 * Defer invocation of the event_cb to a separate message that will

		 *  run later on its thread.  This ensures this context unwinds and

		 *  we don't recursively unregister this bdev again if the event_cb

		 *  immediately closes its descriptor.

		 */

		desc->refs++;

		spdk_thread_send_msg(desc->thread, _remove_notify, desc);

		spdk_spin_unlock(&desc->spinlock);

		event_notify(desc, _remove_notify);

	}



	/* If there are no descriptors, proceed removing the bdev */

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

+78 −0
Original line number Diff line number Diff line
@@ -274,6 +274,11 @@ _bdev_event_cb(enum spdk_bdev_event_type type, struct spdk_bdev *bdev, 
			*(bool *)event_ctx = true;

		}

		break;

	case SPDK_BDEV_EVENT_RESIZE:

		if (event_ctx != NULL) {

			*(int *)event_ctx += 1;

		}

		break;

	default:

		CU_ASSERT(false);

		break;
@@ -2507,6 +2512,78 @@ spdk_bdev_examine_wt(void) 
	g_bdev_opts.bdev_auto_examine = save_auto_examine;

}



static void

event_notify_and_close(void)

{

	int resize_notify_count = 0;

	struct spdk_bdev_desc *desc = NULL;

	struct spdk_bdev *bdev;

	int rc;



	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);



	rc = spdk_bdev_open_ext("ut_bdev", true, _bdev_event_cb, &resize_notify_count, &desc);

	CU_ASSERT(rc == 0);

	SPDK_CU_ASSERT_FATAL(desc != NULL);



	bdev = spdk_bdev_desc_get_bdev(desc);

	SPDK_CU_ASSERT_FATAL(bdev != NULL);



	/* Test a normal case that a resize event is notified. */

	set_thread(0);



	rc = spdk_bdev_notify_blockcnt_change(bdev, 1024 * 2);

	CU_ASSERT(rc == 0);

	CU_ASSERT(bdev->blockcnt == 1024 * 2);

	CU_ASSERT(desc->refs == 1);

	CU_ASSERT(resize_notify_count == 0);



	poll_threads();



	CU_ASSERT(desc->refs == 0);

	CU_ASSERT(resize_notify_count == 1);



	/* Test a complex case if the bdev is closed after two event_notify messages are sent,

	 * then both event_notify messages are discarded and the desc is freed.

	 */

	rc = spdk_bdev_notify_blockcnt_change(bdev, 1024 * 3);

	CU_ASSERT(rc == 0);

	CU_ASSERT(bdev->blockcnt == 1024 * 3);

	CU_ASSERT(desc->refs == 1);

	CU_ASSERT(resize_notify_count == 1);



	rc = spdk_bdev_notify_blockcnt_change(bdev, 1024 * 4);

	CU_ASSERT(rc == 0);

	CU_ASSERT(bdev->blockcnt == 1024 * 4);

	CU_ASSERT(desc->refs == 2);

	CU_ASSERT(resize_notify_count == 1);



	set_thread(1);



	spdk_bdev_close(desc);

	CU_ASSERT(desc->closed == true);

	CU_ASSERT(desc->refs == 2);

	CU_ASSERT(resize_notify_count == 1);



	poll_threads();



	CU_ASSERT(resize_notify_count == 1);



	set_thread(0);



	/* Restore g_desc. Then, we can execute teardown_test(). */

	spdk_bdev_open_ext("ut_bdev", true, _bdev_event_cb, NULL, &g_desc);

	teardown_test();

}



int

main(int argc, char **argv)

{
@@ -2542,6 +2619,7 @@ main(int argc, char **argv) 
	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_ADD_TEST(suite, event_notify_and_close);



	CU_basic_set_mode(CU_BRM_VERBOSE);

	CU_basic_run_tests();