From 671b77e5cde3a0edd0d7febbdb9019d4d4966f94 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Tue, 31 Jul 2018 13:20:38 -0700 Subject: [PATCH] bdev: remove vbdevs and base_bdevs arrays The intents of these arrays was to keep track in the bdev layer of all base<->virtual bdev relationships - i.e. which member disk bdevs make up a RAID bdev, which logical volume bdevs are associated with a bdev that contains an lvolstore, etc. Currently none of this is used however. And trying to keep track in the bdev layer instead of asking the bdev modules for the relationships has a number of complications. Early one, we tried to do this with TAILQs - but that doesn't work since this can't be done with a single TAILQ_ENTRY in the bdev structures. So we moved to arrays - that works a bit better, but then the pointer arrays have to be realloc'd which isn't ideal. The biggest problem though with these arrays is that they held bdev pointers - not bdev descriptor pointers. It's not really valid to access bdevs without a descriptor - the descriptors are what make sure active references are accounted for when a bdev is hotplugged. Of course the bdev layer knows when a bdev is getting removed and could go and do the updates to these arrays separately - but that just seems very convoluted. So for now just remove these arrays completely. If there is a future need for the bdev layer to understand relationships between bdevs, we can add module APIs so that the generic layer can ask the modules about the relationships. Signed-off-by: Jim Harris Change-Id: I99ef1068240bff1262f64f234260cf2fb44df51d Reviewed-on: https://review.gerrithub.io/420932 Tested-by: SPDK CI Jenkins Chandler-Test-Pool: SPDK Automated Test System Reviewed-by: Changpeng Liu Reviewed-by: Ben Walker Reviewed-by: GangCao --- include/spdk/bdev_module.h | 8 --- lib/bdev/bdev.c | 102 ---------------------------- test/unit/lib/bdev/bdev.c/bdev_ut.c | 75 -------------------- test/unit/lib/bdev/part.c/part_ut.c | 6 -- 4 files changed, 191 deletions(-) diff --git a/include/spdk/bdev_module.h b/include/spdk/bdev_module.h index 074cb6438..2a15e6551 100644 --- a/include/spdk/bdev_module.h +++ b/include/spdk/bdev_module.h @@ -271,10 +271,6 @@ struct spdk_bdev { /** function table for all LUN ops */ const struct spdk_bdev_fn_table *fn_table; - /** The array of virtual block devices built on top of this block device. */ - struct spdk_bdev **vbdevs; - size_t vbdevs_cnt; - /** Fields that are used internally by the bdev subsystem. Bdev modules * must not read or write to these fields. */ @@ -291,10 +287,6 @@ struct spdk_bdev { /** The bdev status */ enum spdk_bdev_status status; - /** The array of block devices that this block device is built on top of (if any). */ - struct spdk_bdev **base_bdevs; - size_t base_bdevs_cnt; - /** * Pointer to the module that has claimed this bdev for purposes of creating virtual * bdevs on top of it. Set to NULL if the bdev has not been claimed. diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index 49153b130..39d336cca 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -2926,94 +2926,6 @@ spdk_bdev_register(struct spdk_bdev *bdev) return rc; } -static void -spdk_vbdev_remove_base_bdevs(struct spdk_bdev *vbdev) -{ - struct spdk_bdev **bdevs; - struct spdk_bdev *base; - size_t i, j, k; - bool found; - - /* Iterate over base bdevs to remove vbdev from them. */ - for (i = 0; i < vbdev->internal.base_bdevs_cnt; i++) { - found = false; - base = vbdev->internal.base_bdevs[i]; - - for (j = 0; j < base->vbdevs_cnt; j++) { - if (base->vbdevs[j] != vbdev) { - continue; - } - - for (k = j; k + 1 < base->vbdevs_cnt; k++) { - base->vbdevs[k] = base->vbdevs[k + 1]; - } - - base->vbdevs_cnt--; - if (base->vbdevs_cnt > 0) { - bdevs = realloc(base->vbdevs, base->vbdevs_cnt * sizeof(bdevs[0])); - /* It would be odd if shrinking memory block fail. */ - assert(bdevs); - base->vbdevs = bdevs; - } else { - free(base->vbdevs); - base->vbdevs = NULL; - } - - found = true; - break; - } - - if (!found) { - SPDK_WARNLOG("Bdev '%s' is not base bdev of '%s'.\n", base->name, vbdev->name); - } - } - - free(vbdev->internal.base_bdevs); - vbdev->internal.base_bdevs = NULL; - vbdev->internal.base_bdevs_cnt = 0; -} - -static int -spdk_vbdev_set_base_bdevs(struct spdk_bdev *vbdev, struct spdk_bdev **base_bdevs, size_t cnt) -{ - struct spdk_bdev **vbdevs; - struct spdk_bdev *base; - size_t i; - - /* Adding base bdevs isn't supported (yet?). */ - assert(vbdev->internal.base_bdevs_cnt == 0); - - vbdev->internal.base_bdevs = malloc(cnt * sizeof(vbdev->internal.base_bdevs[0])); - if (!vbdev->internal.base_bdevs) { - SPDK_ERRLOG("%s - realloc() failed\n", vbdev->name); - return -ENOMEM; - } - - memcpy(vbdev->internal.base_bdevs, base_bdevs, cnt * sizeof(vbdev->internal.base_bdevs[0])); - vbdev->internal.base_bdevs_cnt = cnt; - - /* Iterate over base bdevs to add this vbdev to them. */ - for (i = 0; i < cnt; i++) { - base = vbdev->internal.base_bdevs[i]; - - assert(base != NULL); - assert(base->internal.claim_module != NULL); - - vbdevs = realloc(base->vbdevs, (base->vbdevs_cnt + 1) * sizeof(vbdevs[0])); - if (!vbdevs) { - SPDK_ERRLOG("%s - realloc() failed\n", base->name); - spdk_vbdev_remove_base_bdevs(vbdev); - return -ENOMEM; - } - - vbdevs[base->vbdevs_cnt] = vbdev; - base->vbdevs = vbdevs; - base->vbdevs_cnt++; - } - - return 0; -} - int spdk_vbdev_register(struct spdk_bdev *vbdev, struct spdk_bdev **base_bdevs, int base_bdev_count) { @@ -3024,20 +2936,8 @@ spdk_vbdev_register(struct spdk_bdev *vbdev, struct spdk_bdev **base_bdevs, int return rc; } - if (base_bdev_count == 0) { - spdk_bdev_start(vbdev); - return 0; - } - - rc = spdk_vbdev_set_base_bdevs(vbdev, base_bdevs, base_bdev_count); - if (rc) { - spdk_bdev_fini(vbdev); - return rc; - } - spdk_bdev_start(vbdev); return 0; - } void @@ -3076,8 +2976,6 @@ spdk_bdev_unregister(struct spdk_bdev *bdev, spdk_bdev_unregister_cb cb_fn, void pthread_mutex_lock(&bdev->internal.mutex); - spdk_vbdev_remove_base_bdevs(bdev); - bdev->internal.status = SPDK_BDEV_STATUS_REMOVING; bdev->internal.unregister_cb = cb_fn; bdev->internal.unregister_ctx = cb_arg; diff --git a/test/unit/lib/bdev/bdev.c/bdev_ut.c b/test/unit/lib/bdev/bdev.c/bdev_ut.c index 97ee20242..422fb25d3 100644 --- a/test/unit/lib/bdev/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/bdev.c/bdev_ut.c @@ -195,45 +195,6 @@ vbdev_ut_examine(struct spdk_bdev *bdev) spdk_bdev_module_examine_done(&vbdev_ut_if); } -static bool -is_vbdev(struct spdk_bdev *base, struct spdk_bdev *vbdev) -{ - size_t i; - int found = 0; - - for (i = 0; i < base->vbdevs_cnt; i++) { - found += base->vbdevs[i] == vbdev; - } - - CU_ASSERT(found <= 1); - return !!found; -} - -static bool -is_base_bdev(struct spdk_bdev *base, struct spdk_bdev *vbdev) -{ - size_t i; - int found = 0; - - for (i = 0; i < vbdev->internal.base_bdevs_cnt; i++) { - found += vbdev->internal.base_bdevs[i] == base; - } - - CU_ASSERT(found <= 1); - return !!found; -} - -static bool -check_base_and_vbdev(struct spdk_bdev *base, struct spdk_bdev *vbdev) -{ - bool _is_vbdev = is_vbdev(base, vbdev); - bool _is_base = is_base_bdev(base, vbdev); - - CU_ASSERT(_is_vbdev == _is_base); - - return _is_base && _is_vbdev; -} - static struct spdk_bdev * allocate_bdev(char *name) { @@ -250,8 +211,6 @@ allocate_bdev(char *name) rc = spdk_bdev_register(bdev); CU_ASSERT(rc == 0); - CU_ASSERT(bdev->internal.base_bdevs_cnt == 0); - CU_ASSERT(bdev->vbdevs_cnt == 0); return bdev; } @@ -278,14 +237,6 @@ allocate_vbdev(char *name, struct spdk_bdev *base1, struct spdk_bdev *base2) rc = spdk_vbdev_register(bdev, array, base2 == NULL ? 1 : 2); CU_ASSERT(rc == 0); - CU_ASSERT(bdev->internal.base_bdevs_cnt > 0); - CU_ASSERT(bdev->vbdevs_cnt == 0); - - CU_ASSERT(check_base_and_vbdev(base1, bdev) == true); - - if (base2) { - CU_ASSERT(check_base_and_vbdev(base2, bdev) == true); - } return bdev; } @@ -301,7 +252,6 @@ free_bdev(struct spdk_bdev *bdev) static void free_vbdev(struct spdk_bdev *bdev) { - CU_ASSERT(bdev->internal.base_bdevs_cnt != 0); spdk_bdev_unregister(bdev, NULL, NULL); memset(bdev, 0xFF, sizeof(*bdev)); free(bdev); @@ -404,31 +354,6 @@ open_write_test(void) bdev[8] = allocate_vbdev("bdev8", bdev[4], bdev[5]); - /* Check tree */ - CU_ASSERT(check_base_and_vbdev(bdev[0], bdev[4]) == true); - CU_ASSERT(check_base_and_vbdev(bdev[0], bdev[5]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[0], bdev[6]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[0], bdev[7]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[0], bdev[8]) == false); - - CU_ASSERT(check_base_and_vbdev(bdev[1], bdev[4]) == true); - CU_ASSERT(check_base_and_vbdev(bdev[1], bdev[5]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[1], bdev[6]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[1], bdev[7]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[1], bdev[8]) == false); - - CU_ASSERT(check_base_and_vbdev(bdev[2], bdev[4]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[2], bdev[5]) == true); - CU_ASSERT(check_base_and_vbdev(bdev[2], bdev[6]) == true); - CU_ASSERT(check_base_and_vbdev(bdev[2], bdev[7]) == true); - CU_ASSERT(check_base_and_vbdev(bdev[2], bdev[8]) == false); - - CU_ASSERT(check_base_and_vbdev(bdev[3], bdev[4]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[3], bdev[5]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[3], bdev[6]) == false); - CU_ASSERT(check_base_and_vbdev(bdev[3], bdev[7]) == true); - CU_ASSERT(check_base_and_vbdev(bdev[3], bdev[8]) == false); - /* Open bdev0 read-only. This should succeed. */ rc = spdk_bdev_open(bdev[0], false, NULL, NULL, &desc[0]); CU_ASSERT(rc == 0); diff --git a/test/unit/lib/bdev/part.c/part_ut.c b/test/unit/lib/bdev/part.c/part_ut.c index 1f9a2b4d1..789e1dbdc 100644 --- a/test/unit/lib/bdev/part.c/part_ut.c +++ b/test/unit/lib/bdev/part.c/part_ut.c @@ -119,12 +119,6 @@ part_test(void) spdk_bdev_part_base_hotremove(&bdev_base, &tailq); - /* - * The base device was removed - ensure that the partition vbdevs were - * removed from the base's vbdev list. - */ - CU_ASSERT(bdev_base.vbdevs_cnt == 0); - spdk_bdev_part_base_free(base); spdk_bdev_unregister(&bdev_base, NULL, NULL); }