From d2dd47433b830a39e429fb4b3f27e372676f455b Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Thu, 5 Aug 2021 06:14:54 -0400 Subject: [PATCH] bdev/lvol: remove lvs with lvols during application shutdown Once bdev finish starts, bdev unregister is called on all unclaimed bdevs. This means that for lvs with at least one lvol present, there will be a corresponding bdev unregister. Yet the vbdev_lvol module does not attempt to unload the lvs, once last lvol from that lvs is unregistered. Leaving the base bdev for lvs claimed. This patch fixes that by using fini_start callback from bdev_module to mark when shutdown begins. After that last lvol unregistered on lvs will unload it. Expanded struct lvol_bdev to contain lvol_store_bdev. Closing the lvol will free spdk_lvol, so lvol->lvol_store cannot be accessed. Changed ut_lvol_destroy UT to ut_bdev_finish. Previous UT didn't really test vbdev_lvol_destroy, but 'hotremove' of the lvol bdev. In effect there is no hotremove of the lvol bdevs (only lvs bdev). spdk_bdev_unregister() can only be called from within vbdev_lvol, or during bdev module finish. This UT will now check the bdev module finish. Note that at this point lvs with no lvols will not trigger lvs unload. Next patches in series will introduce async fini_start, to allow for the unload. Signed-off-by: Tomasz Zawadzki Change-Id: I8f51e8c1fcfdc55a5d090a3bc84ccefda813aef8 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9093 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Aleksey Marchuk Reviewed-by: Jim Harris Reviewed-by: Konrad Sztyber --- module/bdev/lvol/vbdev_lvol.c | 33 ++++++++++++++++ module/bdev/lvol/vbdev_lvol.h | 1 + .../lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c | 39 ++++++++++--------- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/module/bdev/lvol/vbdev_lvol.c b/module/bdev/lvol/vbdev_lvol.c index 5be2cb96c..652fce8d8 100644 --- a/module/bdev/lvol/vbdev_lvol.c +++ b/module/bdev/lvol/vbdev_lvol.c @@ -44,12 +44,15 @@ static TAILQ_HEAD(, lvol_store_bdev) g_spdk_lvol_pairs = TAILQ_HEAD_INITIALIZER( g_spdk_lvol_pairs); static int vbdev_lvs_init(void); +static void vbdev_lvs_fini_start(void); static int vbdev_lvs_get_ctx_size(void); static void vbdev_lvs_examine(struct spdk_bdev *bdev); +static bool g_shutdown_started = false; static struct spdk_bdev_module g_lvol_if = { .name = "lvol", .module_init = vbdev_lvs_init, + .fini_start = vbdev_lvs_fini_start, .examine_disk = vbdev_lvs_examine, .get_ctx_size = vbdev_lvs_get_ctx_size, @@ -548,10 +551,33 @@ struct vbdev_lvol_destroy_ctx { void *cb_arg; }; +static void +_vbdev_lvol_unregister_unload_lvs(void *cb_arg, int lvserrno) +{ + struct lvol_bdev *lvol_bdev = cb_arg; + struct lvol_store_bdev *lvs_bdev = lvol_bdev->lvs_bdev; + + if (lvserrno != 0) { + SPDK_INFOLOG(vbdev_lvol, "Lvol store removed with error: %d.\n", lvserrno); + } + + TAILQ_REMOVE(&g_spdk_lvol_pairs, lvs_bdev, lvol_stores); + free(lvs_bdev); + + spdk_bdev_destruct_done(&lvol_bdev->bdev, lvserrno); + free(lvol_bdev); +} + static void _vbdev_lvol_unregister_cb(void *ctx, int lvolerrno) { struct lvol_bdev *lvol_bdev = ctx; + struct lvol_store_bdev *lvs_bdev = lvol_bdev->lvs_bdev; + + if (g_shutdown_started && _vbdev_lvs_are_lvols_closed(lvs_bdev->lvs)) { + spdk_lvs_unload(lvs_bdev->lvs, _vbdev_lvol_unregister_unload_lvs, lvol_bdev); + return; + } spdk_bdev_destruct_done(&lvol_bdev->bdev, lvolerrno); free(lvol_bdev); @@ -944,6 +970,7 @@ _create_lvol_disk(struct spdk_lvol *lvol, bool destroy) } lvol_bdev->lvol = lvol; + lvol_bdev->lvs_bdev = lvs_bdev; bdev = &lvol_bdev->bdev; bdev->name = lvol->unique_id; @@ -1203,6 +1230,12 @@ vbdev_lvs_init(void) return 0; } +static void +vbdev_lvs_fini_start(void) +{ + g_shutdown_started = true; +} + static int vbdev_lvs_get_ctx_size(void) { diff --git a/module/bdev/lvol/vbdev_lvol.h b/module/bdev/lvol/vbdev_lvol.h index d59ba1668..1ccb485cc 100644 --- a/module/bdev/lvol/vbdev_lvol.h +++ b/module/bdev/lvol/vbdev_lvol.h @@ -50,6 +50,7 @@ struct lvol_store_bdev { struct lvol_bdev { struct spdk_bdev bdev; struct spdk_lvol *lvol; + struct lvol_store_bdev *lvs_bdev; }; int vbdev_lvs_create(const char *base_bdev_name, const char *name, uint32_t cluster_sz, diff --git a/test/unit/lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c b/test/unit/lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c index 9feddf77c..80cd1a69e 100644 --- a/test/unit/lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c +++ b/test/unit/lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c @@ -1040,7 +1040,7 @@ ut_lvol_rename(void) } static void -ut_lvol_destroy(void) +ut_bdev_finish(void) { struct spdk_lvol_store *lvs; struct spdk_lvol *lvol; @@ -1048,44 +1048,47 @@ ut_lvol_destroy(void) int sz = 10; int rc; - /* Lvol store is successfully created */ - rc = vbdev_lvs_create("bdev", "lvs", 0, LVS_CLEAR_WITH_UNMAP, lvol_store_op_with_handle_complete, - NULL); + /* Test creating lvs with two lvols. Delete first lvol explicitly, + * then start bdev finish. This should unload the remaining lvol and + * lvol store. */ + + rc = vbdev_lvs_create("bdev", "lvs", 0, LVS_CLEAR_WITH_UNMAP, + lvol_store_op_with_handle_complete, NULL); CU_ASSERT(rc == 0); CU_ASSERT(g_lvserrno == 0); SPDK_CU_ASSERT_FATAL(g_lvol_store != NULL); - CU_ASSERT(g_lvol_store->bs_dev != NULL); lvs = g_lvol_store; - /* Successful lvols create */ - g_lvolerrno = -1; - rc = vbdev_lvol_create(lvs, "lvol", sz, false, LVOL_CLEAR_WITH_DEFAULT, vbdev_lvol_create_complete, - NULL); + rc = vbdev_lvol_create(lvs, "lvol", sz, false, LVOL_CLEAR_WITH_DEFAULT, + vbdev_lvol_create_complete, NULL); SPDK_CU_ASSERT_FATAL(rc == 0); CU_ASSERT(g_lvol != NULL); CU_ASSERT(g_lvolerrno == 0); lvol = g_lvol; - g_lvolerrno = -1; - rc = vbdev_lvol_create(lvs, "lvol2", sz, false, LVOL_CLEAR_WITH_DEFAULT, vbdev_lvol_create_complete, - NULL); + rc = vbdev_lvol_create(lvs, "lvol2", sz, false, LVOL_CLEAR_WITH_DEFAULT, + vbdev_lvol_create_complete, NULL); SPDK_CU_ASSERT_FATAL(rc == 0); CU_ASSERT(g_lvol != NULL); CU_ASSERT(g_lvolerrno == 0); lvol2 = g_lvol; - /* Successful lvols destroy */ + /* Destroy explicitly first lvol */ vbdev_lvol_destroy(lvol, lvol_store_op_complete, NULL); CU_ASSERT(g_lvol == NULL); CU_ASSERT(g_lvolerrno == 0); - /* Hot remove lvol bdev */ + /* Start bdev finish and unregister remaining lvol */ + vbdev_lvs_fini_start(); + CU_ASSERT(g_shutdown_started == true); spdk_bdev_unregister(lvol2->bdev, _spdk_bdev_unregister_cb, NULL); - /* Unload lvol store */ - vbdev_lvs_unload(lvs, lvol_store_op_complete, NULL); - CU_ASSERT(g_lvserrno == 0); + /* During shutdown, removal of last lvol should unload lvs */ CU_ASSERT(g_lvol_store == NULL); + CU_ASSERT(TAILQ_EMPTY(&g_spdk_lvol_pairs)); + + /* Revert module state back to normal */ + g_shutdown_started = false; } static void @@ -1456,7 +1459,7 @@ int main(int argc, char **argv) CU_ADD_TEST(suite, ut_vbdev_lvol_submit_request); CU_ADD_TEST(suite, ut_lvol_examine); CU_ADD_TEST(suite, ut_lvol_rename); - CU_ADD_TEST(suite, ut_lvol_destroy); + CU_ADD_TEST(suite, ut_bdev_finish); CU_ADD_TEST(suite, ut_lvs_rename); CU_basic_set_mode(CU_BRM_VERBOSE);