From 8ed35e417d3f3882db336e6f4dd163fffcb5a7f5 Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Wed, 18 Jul 2018 07:07:33 -0400 Subject: [PATCH] bdev/lvol: fix examine error path Added proper paths for examine: - if lvs loaded, do spdk_lvs_unload - if lvol created, call spdk_lvol_close instead of spdk_blob_close Change-Id: I77a2905417d1c53f36b96da3fbea2679d9a2f56d Signed-off-by: Tomasz Zawadzki Reviewed-on: https://review.gerrithub.io/419558 Tested-by: SPDK CI Jenkins Reviewed-by: Maciej Szwed Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Chandler-Test-Pool: SPDK Automated Test System --- lib/bdev/lvol/vbdev_lvol.c | 17 ++++--- .../lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c | 45 +++++++++++-------- 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/lib/bdev/lvol/vbdev_lvol.c b/lib/bdev/lvol/vbdev_lvol.c index cfe319417..b68986ef3 100644 --- a/lib/bdev/lvol/vbdev_lvol.c +++ b/lib/bdev/lvol/vbdev_lvol.c @@ -1118,6 +1118,12 @@ vbdev_lvs_get_ctx_size(void) return sizeof(struct lvol_task); } +static void +_vbdev_lvs_examine_failed(void *cb_arg, int lvserrno) +{ + spdk_bdev_module_examine_done(&g_lvol_if); +} + static void _vbdev_lvs_examine_finish(void *cb_arg, struct spdk_lvol *lvol, int lvolerrno) { @@ -1138,10 +1144,8 @@ _vbdev_lvs_examine_finish(void *cb_arg, struct spdk_lvol *lvol, int lvolerrno) SPDK_ERRLOG("Cannot create bdev for lvol %s\n", lvol->unique_id); TAILQ_REMOVE(&lvs->lvols, lvol, link); lvs->lvol_count--; - spdk_blob_close(lvol->blob, _vbdev_lvol_close_cb, lvs); + spdk_lvol_close(lvol, _vbdev_lvol_close_cb, lvs); SPDK_INFOLOG(SPDK_LOG_VBDEV_LVOL, "Opening lvol %s failed\n", lvol->unique_id); - free(lvol->unique_id); - free(lvol); return; } @@ -1168,10 +1172,12 @@ _vbdev_lvs_examine_cb(void *arg, struct spdk_lvol_store *lvol_store, int lvserrn SPDK_INFOLOG(SPDK_LOG_VBDEV_LVOL, "Name for lvolstore on device %s conflicts with name for already loaded lvs\n", req->base_bdev->name); + /* On error blobstore destroys bs_dev itself */ spdk_bdev_module_examine_done(&g_lvol_if); goto end; } else if (lvserrno != 0) { SPDK_INFOLOG(SPDK_LOG_VBDEV_LVOL, "Lvol store not found on %s\n", req->base_bdev->name); + /* On error blobstore destroys bs_dev itself */ spdk_bdev_module_examine_done(&g_lvol_if); goto end; } @@ -1179,15 +1185,14 @@ _vbdev_lvs_examine_cb(void *arg, struct spdk_lvol_store *lvol_store, int lvserrn lvserrno = spdk_bs_bdev_claim(lvol_store->bs_dev, &g_lvol_if); if (lvserrno != 0) { SPDK_INFOLOG(SPDK_LOG_VBDEV_LVOL, "Lvol store base bdev already claimed by another bdev\n"); - lvol_store->bs_dev->destroy(lvol_store->bs_dev); - spdk_bdev_module_examine_done(&g_lvol_if); + spdk_lvs_unload(lvol_store, _vbdev_lvs_examine_failed, NULL); goto end; } lvs_bdev = calloc(1, sizeof(*lvs_bdev)); if (!lvs_bdev) { SPDK_ERRLOG("Cannot alloc memory for lvs_bdev\n"); - spdk_bdev_module_examine_done(&g_lvol_if); + spdk_lvs_unload(lvol_store, _vbdev_lvs_examine_failed, NULL); goto end; } 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 002f62b22..54b2c4d48 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 @@ -152,11 +152,6 @@ spdk_lvol_open(struct spdk_lvol *lvol, spdk_lvol_op_with_handle_complete cb_fn, cb_fn(cb_arg, lvol, g_lvolerrno); } -void -spdk_blob_close(struct spdk_blob *b, spdk_blob_op_complete cb_fn, void *cb_arg) -{ -} - uint64_t spdk_blob_get_num_clusters(struct spdk_blob *b) { @@ -211,19 +206,28 @@ spdk_lvs_load(struct spdk_bs_dev *dev, { struct spdk_lvol_store *lvs; int i; + int lvserrno = g_lvserrno; - if (g_lvserrno == 0) { - lvs = calloc(1, sizeof(*lvs)); - SPDK_CU_ASSERT_FATAL(lvs != NULL); - TAILQ_INIT(&lvs->lvols); - TAILQ_INIT(&lvs->pending_lvols); - g_lvol_store = lvs; - for (i = 0; i < g_num_lvols; i++) { - _lvol_create(lvs); - } + if (lvserrno != 0) { + /* On error blobstore destroys bs_dev itself, + * by puttin back io channels. + * This operation is asynchronous, and completed + * after calling the callback for lvol. */ + cb_fn(cb_arg, g_lvol_store, lvserrno); + dev->destroy(dev); + return; } - cb_fn(cb_arg, g_lvol_store, g_lvserrno); + lvs = calloc(1, sizeof(*lvs)); + SPDK_CU_ASSERT_FATAL(lvs != NULL); + TAILQ_INIT(&lvs->lvols); + TAILQ_INIT(&lvs->pending_lvols); + g_lvol_store = lvs; + for (i = 0; i < g_num_lvols; i++) { + _lvol_create(lvs); + } + + cb_fn(cb_arg, g_lvol_store, lvserrno); } int @@ -598,6 +602,7 @@ spdk_vbdev_register(struct spdk_bdev *vbdev, struct spdk_bdev **base_bdevs, int void spdk_bdev_module_examine_done(struct spdk_bdev_module *module) { + SPDK_CU_ASSERT_FATAL(g_examine_done != true); g_examine_done = true; } @@ -959,13 +964,14 @@ ut_lvol_examine(void) g_lvserrno = -1; lvol_already_opened = false; vbdev_lvs_examine(&g_bdev); - CU_ASSERT(g_bs_dev != NULL); + CU_ASSERT(g_bs_dev == NULL); CU_ASSERT(g_lvol_store == NULL); CU_ASSERT(g_examine_done == true); CU_ASSERT(TAILQ_EMPTY(&g_spdk_lvol_pairs)); - free(g_bs_dev); - /* Examine unsuccesfully - fail on lvol load */ + /* Examine succesfully + * - one lvol fails to load + * - lvs is loaded with no lvols present */ g_bs_dev = NULL; g_lvserrno = 0; g_lvolerrno = -1; @@ -981,7 +987,8 @@ ut_lvol_examine(void) CU_ASSERT(!TAILQ_EMPTY(&g_spdk_lvol_pairs)); CU_ASSERT(TAILQ_EMPTY(&g_lvol_store->lvols)); vbdev_lvs_destruct(g_lvol_store, lvol_store_op_complete, NULL); - free(g_bs_dev); + CU_ASSERT(g_lvserrno == 0); + CU_ASSERT(g_lvol_store == NULL); /* Examine succesfully */ g_lvs = calloc(1, sizeof(*g_lvs));