From b6aaba0852c2efa6ecfc5e4f81090b042bb2b29a Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Mon, 22 Jan 2018 05:19:55 -0500 Subject: [PATCH] bdev: remove vbdevs during spdk_bdev_unregister() spdk_vbdev_unregister() is part of internal bdev API, yet bdev module that uses spdk_vbdev_register() directly will not be removed correctly when using delete_bdev RPC. spdk_vbdev_unregister() is now consolidated with spdk_bdev_unregister(). This comes up when deleting lvol bdev, as it does not use spdk_bdev_part_* functions. base_bdev->vbdevs entry was not removed for bdev that lvs is created on. Additionally patch expands test to create lvol bdev, after removing it using delete_bdev RPC. With ASAN enabled this would report accessing already freed memory previously. Change-Id: I9547e83862e2daa50355d56a1c9f453aaa6cfdb8 Signed-off-by: Tomasz Zawadzki Reviewed-on: https://review.gerrithub.io/395711 Tested-by: SPDK Automated Test System Reviewed-by: Daniel Verkamp Reviewed-by: Jim Harris --- include/spdk_internal/bdev.h | 1 - lib/bdev/bdev.c | 21 +++++++------------ lib/bdev/lvol/vbdev_lvol.c | 2 +- test/lvol/test_cases.py | 19 +++++++++-------- test/lvol/test_plan.md | 1 + test/unit/lib/bdev/bdev.c/bdev_ut.c | 3 ++- .../lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c | 2 +- 7 files changed, 23 insertions(+), 26 deletions(-) diff --git a/include/spdk_internal/bdev.h b/include/spdk_internal/bdev.h index 15fef35a9..60b568e58 100644 --- a/include/spdk_internal/bdev.h +++ b/include/spdk_internal/bdev.h @@ -398,7 +398,6 @@ void spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void spdk_bdev_unregister_done(struct spdk_bdev *bdev, int bdeverrno); int spdk_vbdev_register(struct spdk_bdev *vbdev, struct spdk_bdev **base_bdevs, int base_bdev_count); -void spdk_vbdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg); void spdk_bdev_module_examine_done(struct spdk_bdev_module_if *module); void spdk_bdev_module_init_done(struct spdk_bdev_module_if *module); diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index 1c545e8fc..178555b82 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -2110,11 +2110,18 @@ spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void struct spdk_bdev_desc *desc, *tmp; int rc; bool do_destruct = true; + struct spdk_bdev *base_bdev; SPDK_DEBUGLOG(SPDK_LOG_BDEV, "Removing bdev %s from list\n", bdev->name); pthread_mutex_lock(&bdev->mutex); + if (!TAILQ_EMPTY(&bdev->base_bdevs)) { + TAILQ_FOREACH(base_bdev, &bdev->base_bdevs, base_bdev_link) { + TAILQ_REMOVE(&base_bdev->vbdevs, bdev, vbdev_link); + } + } + bdev->status = SPDK_BDEV_STATUS_REMOVING; bdev->unregister_cb = cb_fn; bdev->unregister_ctx = cb_arg; @@ -2149,18 +2156,6 @@ spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void } } -void -spdk_vbdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg) -{ - struct spdk_bdev *base_bdev; - - assert(!TAILQ_EMPTY(&vbdev->base_bdevs)); - TAILQ_FOREACH(base_bdev, &vbdev->base_bdevs, base_bdev_link) { - TAILQ_REMOVE(&base_bdev->vbdevs, vbdev, vbdev_link); - } - spdk_bdev_unregister(vbdev, cb_fn, cb_arg); -} - int spdk_bdev_open(struct spdk_bdev *bdev, bool write, spdk_bdev_remove_cb_t remove_cb, void *remove_ctx, struct spdk_bdev_desc **_desc) @@ -2332,7 +2327,7 @@ spdk_bdev_part_base_hotremove(struct spdk_bdev *base_bdev, struct bdev_part_tail TAILQ_FOREACH_SAFE(part, tailq, tailq, tmp) { if (part->base->bdev == base_bdev) { - spdk_vbdev_unregister(&part->bdev, NULL, NULL); + spdk_bdev_unregister(&part->bdev, NULL, NULL); } } } diff --git a/lib/bdev/lvol/vbdev_lvol.c b/lib/bdev/lvol/vbdev_lvol.c index ded727ac9..031e35b85 100644 --- a/lib/bdev/lvol/vbdev_lvol.c +++ b/lib/bdev/lvol/vbdev_lvol.c @@ -275,7 +275,7 @@ _vbdev_lvs_remove(struct spdk_lvol_store *lvs, spdk_lvs_op_complete cb_fn, void lvs->destruct = destroy; TAILQ_FOREACH_SAFE(lvol, &lvs->lvols, link, tmp) { lvol->close_only = !destroy; - spdk_vbdev_unregister(lvol->bdev, NULL, NULL); + spdk_bdev_unregister(lvol->bdev, NULL, NULL); } } } diff --git a/test/lvol/test_cases.py b/test/lvol/test_cases.py index 8bbdd21f4..4f0272116 100644 --- a/test/lvol/test_cases.py +++ b/test/lvol/test_cases.py @@ -201,16 +201,17 @@ class TestCases(object): self.cluster_size) size = ((self.total_size - 1) / 4) - uuid_bdevs = [] - for i in range(4): - uuid_bdev = self.c.construct_lvol_bdev(uuid_store, - self.lbd_name + str(i), - size) - uuid_bdevs.append(uuid_bdev) - fail_count += self.c.check_get_bdevs_methods(uuid_bdev, size) + for j in range(2): + uuid_bdevs = [] + for i in range(4): + uuid_bdev = self.c.construct_lvol_bdev(uuid_store, + self.lbd_name + str(i), + size) + uuid_bdevs.append(uuid_bdev) + fail_count += self.c.check_get_bdevs_methods(uuid_bdev, size) - for uuid_bdev in uuid_bdevs: - self.c.delete_bdev(uuid_bdev) + for uuid_bdev in uuid_bdevs: + self.c.delete_bdev(uuid_bdev) self.c.destroy_lvol_store(uuid_store) self.c.delete_bdev(base_name) diff --git a/test/lvol/test_plan.md b/test/lvol/test_plan.md index 2cb83ed5d..a0a4c9932 100644 --- a/test/lvol/test_plan.md +++ b/test/lvol/test_plan.md @@ -69,6 +69,7 @@ Steps: because of lvol metadata) - repeat the previous step three more times - delete lvol bdevs +- create and delete four lvol bdevs again from steps above - destroy lvol store - delete malloc bdev diff --git a/test/unit/lib/bdev/bdev.c/bdev_ut.c b/test/unit/lib/bdev/bdev.c/bdev_ut.c index 98d635e85..7e767ddf2 100644 --- a/test/unit/lib/bdev/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/bdev.c/bdev_ut.c @@ -138,7 +138,8 @@ free_bdev(struct spdk_bdev *bdev) static void free_vbdev(struct spdk_bdev *bdev) { - spdk_vbdev_unregister(bdev, NULL, NULL); + CU_ASSERT(!TAILQ_EMPTY(&bdev->base_bdevs)); + spdk_bdev_unregister(bdev, NULL, NULL); free(bdev); } 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 0ce971222..6b1ed2713 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 @@ -110,7 +110,7 @@ spdk_bs_bdev_claim(struct spdk_bs_dev *bs_dev, struct spdk_bdev_module_if *modul } void -spdk_vbdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg) +spdk_bdev_unregister(struct spdk_bdev *vbdev, spdk_bdev_unregister_cb cb_fn, void *cb_arg) { SPDK_CU_ASSERT_FATAL(vbdev != NULL); vbdev->fn_table->destruct(vbdev->ctxt);