From 3522d43a95691476d2be20ae76c3872d2166dec0 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Fri, 27 Jan 2023 12:48:43 +0900 Subject: [PATCH] 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: Shuhei Matsumoto Signed-off-by: Mike Gerdts Change-Id: Ibe2478479c61459c0da0db8d28c7273f05275e0f Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/16577 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Aleksey Marchuk --- lib/bdev/bdev.c | 57 +++++++++---------- test/unit/lib/bdev/mt/bdev.c/bdev_ut.c | 78 ++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 32 deletions(-) diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index a27a90f6c..a789da488 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -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 */ 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 589e3243a..56ef58488 100644 --- a/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/mt/bdev.c/bdev_ut.c @@ -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();