From eabe783cc4d79cf17a4cf22fa123a3d3a7e8c095 Mon Sep 17 00:00:00 2001 From: Jiewei Ke Date: Tue, 11 May 2021 03:16:14 -0400 Subject: [PATCH] bdev: speed up bdev name lookup by using rbtree Use the macros for red black tree provided by Free BSD to speed up bdev name lookup in spdk_bdev_get_by_name(). In the bdev_multi_allocation test, we can get 3x ~ 5x speed up when creating multiple bdevs for various bdev nums. Signed-off-by: Jiewei Ke Change-Id: I49a2fbcccf06d4c36cbd445ce59e0b0dd4ada31d Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/7837 Reviewed-by: Shuhei Matsumoto Reviewed-by: Changpeng Liu Reviewed-by: Aleksey Marchuk Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot --- include/spdk/bdev_module.h | 12 ++- lib/bdev/bdev.c | 77 +++++++++++++------ lib/bdev/bdev_rpc.c | 2 +- module/bdev/compress/vbdev_compress.c | 2 +- module/bdev/lvol/vbdev_lvol.c | 4 +- test/unit/lib/bdev/bdev.c/bdev_ut.c | 67 ++++++++++++++++ .../lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c | 20 +++-- 7 files changed, 150 insertions(+), 34 deletions(-) diff --git a/include/spdk/bdev_module.h b/include/spdk/bdev_module.h index 86876a4cc..cfc212d32 100644 --- a/include/spdk/bdev_module.h +++ b/include/spdk/bdev_module.h @@ -47,6 +47,7 @@ #include "spdk/queue.h" #include "spdk/scsi_spec.h" #include "spdk/thread.h" +#include "spdk/tree.h" #include "spdk/util.h" #include "spdk/uuid.h" @@ -267,8 +268,14 @@ enum spdk_bdev_io_status { SPDK_BDEV_IO_STATUS_SUCCESS = 1, }; +struct spdk_bdev_name { + char *name; + struct spdk_bdev *bdev; + RB_ENTRY(spdk_bdev_name) node; +}; + struct spdk_bdev_alias { - char *alias; + struct spdk_bdev_name alias; TAILQ_ENTRY(spdk_bdev_alias) tailq; }; @@ -501,6 +508,9 @@ struct spdk_bdev { * locked due to overlapping with another locked range. */ lba_range_tailq_t pending_locked_ranges; + + /** Bdev name used for quick lookup */ + struct spdk_bdev_name bdev_name; } internal; }; diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index f48f03ba7..42d82c681 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -95,6 +95,16 @@ static const char *qos_rpc_type[] = {"rw_ios_per_sec", TAILQ_HEAD(spdk_bdev_list, spdk_bdev); +RB_HEAD(bdev_name_tree, spdk_bdev_name); + +static int +bdev_name_cmp(struct spdk_bdev_name *name1, struct spdk_bdev_name *name2) +{ + return strcmp(name1->name, name2->name); +} + +RB_GENERATE_STATIC(bdev_name_tree, spdk_bdev_name, node, bdev_name_cmp); + struct spdk_bdev_mgr { struct spdk_mempool *bdev_io_pool; @@ -106,6 +116,7 @@ struct spdk_bdev_mgr { TAILQ_HEAD(bdev_module_list, spdk_bdev_module) bdev_modules; struct spdk_bdev_list bdevs; + struct bdev_name_tree bdev_names; bool init_complete; bool module_init_complete; @@ -120,6 +131,7 @@ struct spdk_bdev_mgr { static struct spdk_bdev_mgr g_bdev_mgr = { .bdev_modules = TAILQ_HEAD_INITIALIZER(g_bdev_mgr.bdev_modules), .bdevs = TAILQ_HEAD_INITIALIZER(g_bdev_mgr.bdevs), + .bdev_names = RB_INITIALIZER(g_bdev_mgr.bdev_names), .init_complete = false, .module_init_complete = false, .mutex = PTHREAD_MUTEX_INITIALIZER, @@ -546,7 +558,7 @@ bdev_in_examine_allowlist(struct spdk_bdev *bdev) return true; } TAILQ_FOREACH(tmp, &bdev->aliases, tailq) { - if (bdev_examine_allowlist_check(tmp->alias)) { + if (bdev_examine_allowlist_check(tmp->alias.name)) { return true; } } @@ -716,21 +728,13 @@ spdk_bdev_next_leaf(struct spdk_bdev *prev) struct spdk_bdev * spdk_bdev_get_by_name(const char *bdev_name) { - struct spdk_bdev_alias *tmp; - struct spdk_bdev *bdev = spdk_bdev_first(); + struct spdk_bdev_name find; + struct spdk_bdev_name *res; - while (bdev != NULL) { - if (strcmp(bdev_name, bdev->name) == 0) { - return bdev; - } - - TAILQ_FOREACH(tmp, &bdev->aliases, tailq) { - if (strcmp(bdev_name, tmp->alias) == 0) { - return bdev; - } - } - - bdev = spdk_bdev_next(bdev); + find.name = (char *)bdev_name; + res = RB_FIND(bdev_name_tree, &g_bdev_mgr.bdev_names, &find); + if (res != NULL) { + return res->bdev; } return NULL; @@ -3176,10 +3180,32 @@ bdev_channel_destroy(void *io_device, void *ctx_buf) bdev_channel_destroy_resource(ch); } +static int +bdev_name_add(struct spdk_bdev_name *bdev_name, struct spdk_bdev *bdev, const char *name) +{ + bdev_name->name = strdup(name); + if (bdev_name->name == NULL) { + SPDK_ERRLOG("Unable to allocate bdev name\n"); + return -ENOMEM; + } + + bdev_name->bdev = bdev; + RB_INSERT(bdev_name_tree, &g_bdev_mgr.bdev_names, bdev_name); + return 0; +} + +static void +bdev_name_del(struct spdk_bdev_name *bdev_name) +{ + RB_REMOVE(bdev_name_tree, &g_bdev_mgr.bdev_names, bdev_name); + free(bdev_name->name); +} + int spdk_bdev_alias_add(struct spdk_bdev *bdev, const char *alias) { struct spdk_bdev_alias *tmp; + int ret; if (alias == NULL) { SPDK_ERRLOG("Empty alias passed\n"); @@ -3197,11 +3223,10 @@ spdk_bdev_alias_add(struct spdk_bdev *bdev, const char *alias) return -ENOMEM; } - tmp->alias = strdup(alias); - if (tmp->alias == NULL) { + ret = bdev_name_add(&tmp->alias, bdev, alias); + if (ret != 0) { free(tmp); - SPDK_ERRLOG("Unable to allocate alias\n"); - return -ENOMEM; + return ret; } TAILQ_INSERT_TAIL(&bdev->aliases, tmp, tailq); @@ -3215,9 +3240,9 @@ spdk_bdev_alias_del(struct spdk_bdev *bdev, const char *alias) struct spdk_bdev_alias *tmp; TAILQ_FOREACH(tmp, &bdev->aliases, tailq) { - if (strcmp(alias, tmp->alias) == 0) { + if (strcmp(alias, tmp->alias.name) == 0) { TAILQ_REMOVE(&bdev->aliases, tmp, tailq); - free(tmp->alias); + bdev_name_del(&tmp->alias); free(tmp); return 0; } @@ -3235,7 +3260,7 @@ spdk_bdev_alias_del_all(struct spdk_bdev *bdev) TAILQ_FOREACH_SAFE(p, &bdev->aliases, tailq, tmp) { TAILQ_REMOVE(&bdev->aliases, p, tailq); - free(p->alias); + bdev_name_del(&p->alias); free(p); } } @@ -5516,6 +5541,7 @@ static int bdev_register(struct spdk_bdev *bdev) { char *bdev_name; + int ret; assert(bdev->module != NULL); @@ -5548,6 +5574,12 @@ bdev_register(struct spdk_bdev *bdev) bdev->internal.qd_poller = NULL; bdev->internal.qos = NULL; + ret = bdev_name_add(&bdev->internal.bdev_name, bdev, bdev->name); + if (ret != 0) { + free(bdev_name); + return ret; + } + /* If the user didn't specify a uuid, generate one. */ if (spdk_mem_all_zero(&bdev->uuid, sizeof(bdev->uuid))) { spdk_uuid_generate(&bdev->uuid); @@ -5707,6 +5739,7 @@ bdev_unregister_unsafe(struct spdk_bdev *bdev) if (rc == 0) { TAILQ_REMOVE(&g_bdev_mgr.bdevs, bdev, internal.link); SPDK_DEBUGLOG(bdev, "Removing bdev %s from list done\n", bdev->name); + bdev_name_del(&bdev->internal.bdev_name); spdk_notify_send("bdev_unregister", spdk_bdev_get_name(bdev)); } diff --git a/lib/bdev/bdev_rpc.c b/lib/bdev/bdev_rpc.c index 9055c77dd..40985b2a8 100644 --- a/lib/bdev/bdev_rpc.c +++ b/lib/bdev/bdev_rpc.c @@ -363,7 +363,7 @@ rpc_dump_bdev_info(struct spdk_json_write_ctx *w, spdk_json_write_named_array_begin(w, "aliases"); TAILQ_FOREACH(tmp, spdk_bdev_get_aliases(bdev), tailq) { - spdk_json_write_string(w, tmp->alias); + spdk_json_write_string(w, tmp->alias.name); } spdk_json_write_array_end(w); diff --git a/module/bdev/compress/vbdev_compress.c b/module/bdev/compress/vbdev_compress.c index 708e67b9b..929af824e 100644 --- a/module/bdev/compress/vbdev_compress.c +++ b/module/bdev/compress/vbdev_compress.c @@ -1591,7 +1591,7 @@ static int _set_compbdev_name(struct vbdev_compress *comp_bdev) if (!TAILQ_EMPTY(spdk_bdev_get_aliases(comp_bdev->base_bdev))) { aliases = TAILQ_FIRST(spdk_bdev_get_aliases(comp_bdev->base_bdev)); - comp_bdev->comp_bdev.name = spdk_sprintf_alloc("COMP_%s", aliases->alias); + comp_bdev->comp_bdev.name = spdk_sprintf_alloc("COMP_%s", aliases->alias.name); if (!comp_bdev->comp_bdev.name) { SPDK_ERRLOG("could not allocate comp_bdev name for alias\n"); return -ENOMEM; diff --git a/module/bdev/lvol/vbdev_lvol.c b/module/bdev/lvol/vbdev_lvol.c index 58e035a72..6f2c0c976 100644 --- a/module/bdev/lvol/vbdev_lvol.c +++ b/module/bdev/lvol/vbdev_lvol.c @@ -92,13 +92,13 @@ _vbdev_lvol_change_bdev_alias(struct spdk_lvol *lvol, const char *new_lvol_name) * while we changed lvs name earlier, we have to iterate alias list to get one, * and check if there is only one alias */ - TAILQ_FOREACH(tmp, &lvol->bdev->aliases, tailq) { + TAILQ_FOREACH(tmp, spdk_bdev_get_aliases(lvol->bdev), tailq) { if (++alias_number > 1) { SPDK_ERRLOG("There is more than 1 alias in bdev %s\n", lvol->bdev->name); return -EINVAL; } - old_alias = tmp->alias; + old_alias = tmp->alias.name; } if (alias_number == 0) { diff --git a/test/unit/lib/bdev/bdev.c/bdev_ut.c b/test/unit/lib/bdev/bdev.c/bdev_ut.c index 16656dae6..54c555f7b 100644 --- a/test/unit/lib/bdev/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/bdev.c/bdev_ut.c @@ -4399,6 +4399,72 @@ bdev_set_options_test(void) CU_ASSERT(rc == 0); } +static uint64_t +get_ns_time(void) +{ + int rc; + struct timespec ts; + + rc = clock_gettime(CLOCK_MONOTONIC, &ts); + CU_ASSERT(rc == 0); + return ts.tv_sec * 1000 * 1000 * 1000 + ts.tv_nsec; +} + +static int +rb_tree_get_height(struct spdk_bdev_name *bdev_name) +{ + int h1, h2; + + if (bdev_name == NULL) { + return -1; + } else { + h1 = rb_tree_get_height(RB_LEFT(bdev_name, node)); + h2 = rb_tree_get_height(RB_RIGHT(bdev_name, node)); + + return spdk_max(h1, h2) + 1; + } +} + +static void +bdev_multi_allocation(void) +{ + const int max_bdev_num = 1024 * 16; + char name[max_bdev_num][10]; + char noexist_name[] = "invalid_bdev"; + struct spdk_bdev *bdev[max_bdev_num]; + int i, j; + uint64_t last_time; + int bdev_num; + int height; + + for (j = 0; j < max_bdev_num; j++) { + snprintf(name[j], sizeof(name[j]), "bdev%d", j); + } + + for (i = 0; i < 16; i++) { + last_time = get_ns_time(); + bdev_num = 1024 * (i + 1); + for (j = 0; j < bdev_num; j++) { + bdev[j] = allocate_bdev(name[j]); + height = rb_tree_get_height(&bdev[j]->internal.bdev_name); + CU_ASSERT(height <= (int)(spdk_u32log2(j + 1))); + } + SPDK_NOTICELOG("alloc bdev num %d takes %lu ms\n", bdev_num, + (get_ns_time() - last_time) / 1000 / 1000); + for (j = 0; j < bdev_num; j++) { + CU_ASSERT(spdk_bdev_get_by_name(name[j]) != NULL); + } + CU_ASSERT(spdk_bdev_get_by_name(noexist_name) == NULL); + + for (j = 0; j < bdev_num; j++) { + free_bdev(bdev[j]); + } + for (j = 0; j < bdev_num; j++) { + CU_ASSERT(spdk_bdev_get_by_name(name[j]) == NULL); + } + } +} + int main(int argc, char **argv) { @@ -4440,6 +4506,7 @@ main(int argc, char **argv) CU_ADD_TEST(suite, bdev_io_abort); CU_ADD_TEST(suite, bdev_unmap); CU_ADD_TEST(suite, bdev_set_options_test); + CU_ADD_TEST(suite, bdev_multi_allocation); allocate_cores(1); allocate_threads(1); 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 c66234736..80bb2403f 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 @@ -61,6 +61,12 @@ bool g_examine_done = false; bool g_bdev_alias_already_exists = false; bool g_lvs_with_name_already_exists = false; +const struct spdk_bdev_aliases_list * +spdk_bdev_get_aliases(const struct spdk_bdev *bdev) +{ + return &bdev->aliases; +} + int spdk_bdev_alias_add(struct spdk_bdev *bdev, const char *alias) { @@ -75,8 +81,8 @@ spdk_bdev_alias_add(struct spdk_bdev *bdev, const char *alias) tmp = calloc(1, sizeof(*tmp)); SPDK_CU_ASSERT_FATAL(tmp != NULL); - tmp->alias = strdup(alias); - SPDK_CU_ASSERT_FATAL(tmp->alias != NULL); + tmp->alias.name = strdup(alias); + SPDK_CU_ASSERT_FATAL(tmp->alias.name != NULL); TAILQ_INSERT_TAIL(&bdev->aliases, tmp, tailq); @@ -92,9 +98,9 @@ spdk_bdev_alias_del(struct spdk_bdev *bdev, const char *alias) TAILQ_FOREACH(tmp, &bdev->aliases, tailq) { SPDK_CU_ASSERT_FATAL(alias != NULL); - if (strncmp(alias, tmp->alias, SPDK_LVOL_NAME_MAX) == 0) { + if (strncmp(alias, tmp->alias.name, SPDK_LVOL_NAME_MAX) == 0) { TAILQ_REMOVE(&bdev->aliases, tmp, tailq); - free(tmp->alias); + free(tmp->alias.name); free(tmp); return 0; } @@ -110,7 +116,7 @@ spdk_bdev_alias_del_all(struct spdk_bdev *bdev) TAILQ_FOREACH_SAFE(p, &bdev->aliases, tailq, tmp) { TAILQ_REMOVE(&bdev->aliases, p, tailq); - free(p->alias); + free(p->alias.name); free(p); } } @@ -1393,7 +1399,7 @@ ut_lvs_rename(void) vbdev_lvs_rename(lvs, "new_lvs_name", lvol_store_op_complete, NULL); CU_ASSERT(g_lvserrno == 0); CU_ASSERT_STRING_EQUAL(lvs->name, "new_lvs_name"); - CU_ASSERT_STRING_EQUAL(TAILQ_FIRST(&g_lvol->bdev->aliases)->alias, "new_lvs_name/lvol"); + CU_ASSERT_STRING_EQUAL(TAILQ_FIRST(&g_lvol->bdev->aliases)->alias.name, "new_lvs_name/lvol"); /* Trying to rename lvs with name already used by another lvs */ /* This is a bdev_lvol test, so g_lvs_with_name_already_exists simulates @@ -1402,7 +1408,7 @@ ut_lvs_rename(void) vbdev_lvs_rename(lvs, "another_new_lvs_name", lvol_store_op_complete, NULL); CU_ASSERT(g_lvserrno == -EEXIST); CU_ASSERT_STRING_EQUAL(lvs->name, "new_lvs_name"); - CU_ASSERT_STRING_EQUAL(TAILQ_FIRST(&g_lvol->bdev->aliases)->alias, "new_lvs_name/lvol"); + CU_ASSERT_STRING_EQUAL(TAILQ_FIRST(&g_lvol->bdev->aliases)->alias.name, "new_lvs_name/lvol"); g_lvs_with_name_already_exists = false; /* Unload lvol store */