From db8bd99588cf7121af155a5f0f0330320961713c Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Fri, 9 Oct 2020 14:14:24 +0900 Subject: [PATCH] bdev/part: Add spdk_bdev_part_base_construct_ext() to pass bdev_name Add an new API spdk_bdev_part_base_construct_ext() to pass not bdev but bdev_name to fix the race condition due to the time gap between spdk_bdev_get_by_name() and spdk_bdev_open(). A pointer to a bdev is valid only while the bdev is opened. In the new API, spdk_bdev_get_by_name() is included in spdk_bdev_part_base_construct_ext() and the caller has to know if the bdev exists or not. Hence spdk_bdev_part_base_construct_ext() returns return code and returns the created part object by the double pointer. Another critical change is that base is just freed if spdk_bdev_open_ext() failed with -ENODEV. The reason is that if we call spdk_bdev_part_base_free() for that case, the configuration is removed by the registered callback and so bdev_examine() will not work. The following patches will replace spdk_bdev_part_base_construct() by spdk_bdev_part_base_construct_ext() for the corresponding bdev modules. Signed-off-by: Shuhei Matsumoto Change-Id: I2db027a159559c403cdfbd71800afba590b0f328 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4576 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Tomasz Zawadzki Reviewed-by: Aleksey Marchuk --- CHANGELOG.md | 8 +++++ include/spdk/bdev_module.h | 32 ++++++++++++++++++- lib/bdev/part.c | 63 ++++++++++++++++++++++++++++---------- lib/bdev/spdk_bdev.map | 1 + 4 files changed, 87 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 265382091..da5c43292 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## v20.10: (Upcoming Release) +### bdev + +A new `spdk_bdev_part_base_construct_ext` function has been added and the +`spdk_bdev_part_base_construct` has been deprecated. The +`spdk_bdev_part_base_construct_ext` function takes bdev name as an argument instead +of bdev structure to avoid a race condition that can happen when the bdev is being +removed between a call to get its structure based on a name and actually openning it. + ### dpdk Updated DPDK submodule to DPDK 20.08. diff --git a/include/spdk/bdev_module.h b/include/spdk/bdev_module.h index 9afc29390..899470283 100644 --- a/include/spdk/bdev_module.h +++ b/include/spdk/bdev_module.h @@ -1087,7 +1087,8 @@ void spdk_bdev_part_base_hotremove(struct spdk_bdev_part_base *part_base, struct bdev_part_tailq *tailq); /** - * Construct a new spdk_bdev_part_base on top of the provided bdev. + * Construct a new spdk_bdev_part_base on top of the provided bdev + * (deprecated. please use spdk_bdev_part_base_construct_ext). * * \param bdev The spdk_bdev upon which this base will be built. * \param remove_cb Function to be called upon hotremove of the bdev. @@ -1114,6 +1115,35 @@ struct spdk_bdev_part_base *spdk_bdev_part_base_construct(struct spdk_bdev *bdev spdk_io_channel_create_cb ch_create_cb, spdk_io_channel_destroy_cb ch_destroy_cb); +/** + * Construct a new spdk_bdev_part_base on top of the provided bdev. + * + * \param bdev_name Name of the bdev upon which this base will be built. + * \param remove_cb Function to be called upon hotremove of the bdev. + * \param module The module to which this bdev base belongs. + * \param fn_table Function table for communicating with the bdev backend. + * \param tailq The head of the list of all spdk_bdev_part structures registered to this base's module. + * \param free_fn User provided function to free base related context upon bdev removal or shutdown. + * \param ctx Module specific context for this bdev part base. + * \param channel_size Channel size in bytes. + * \param ch_create_cb Called after a new channel is allocated. + * \param ch_destroy_cb Called upon channel deletion. + * \param base output parameter for the part object when operation is succssful. + * + * \return 0 if operation is successful, or suitable errno value otherwise. + */ +int spdk_bdev_part_base_construct_ext(const char *bdev_name, + spdk_bdev_remove_cb_t remove_cb, + struct spdk_bdev_module *module, + struct spdk_bdev_fn_table *fn_table, + struct bdev_part_tailq *tailq, + spdk_bdev_part_base_free_fn free_fn, + void *ctx, + uint32_t channel_size, + spdk_io_channel_create_cb ch_create_cb, + spdk_io_channel_destroy_cb ch_destroy_cb, + struct spdk_bdev_part_base **base); + /** * Create a logical spdk_bdev_part on top of a base. * diff --git a/lib/bdev/part.c b/lib/bdev/part.c index aeb64c03a..4b0e6fc1d 100644 --- a/lib/bdev/part.c +++ b/lib/bdev/part.c @@ -433,26 +433,30 @@ bdev_part_base_event_cb(enum spdk_bdev_event_type type, struct spdk_bdev *bdev, } } -struct spdk_bdev_part_base * - spdk_bdev_part_base_construct(struct spdk_bdev *bdev, - spdk_bdev_remove_cb_t remove_cb, struct spdk_bdev_module *module, - struct spdk_bdev_fn_table *fn_table, struct bdev_part_tailq *tailq, - spdk_bdev_part_base_free_fn free_fn, void *ctx, - uint32_t channel_size, spdk_io_channel_create_cb ch_create_cb, - spdk_io_channel_destroy_cb ch_destroy_cb) +int +spdk_bdev_part_base_construct_ext(const char *bdev_name, + spdk_bdev_remove_cb_t remove_cb, struct spdk_bdev_module *module, + struct spdk_bdev_fn_table *fn_table, struct bdev_part_tailq *tailq, + spdk_bdev_part_base_free_fn free_fn, void *ctx, + uint32_t channel_size, spdk_io_channel_create_cb ch_create_cb, + spdk_io_channel_destroy_cb ch_destroy_cb, + struct spdk_bdev_part_base **_base) { int rc; struct spdk_bdev_part_base *base; + if (_base == NULL) { + return -EINVAL; + } + base = calloc(1, sizeof(*base)); if (!base) { SPDK_ERRLOG("Memory allocation failure\n"); - return NULL; + return -ENOMEM; } fn_table->get_io_channel = bdev_part_get_io_channel; fn_table->io_type_supported = bdev_part_io_type_supported; - base->bdev = bdev; base->desc = NULL; base->ref = 0; base->module = module; @@ -466,19 +470,46 @@ struct spdk_bdev_part_base * base->ch_destroy_cb = ch_destroy_cb; base->remove_cb = remove_cb; - rc = spdk_bdev_open_ext(spdk_bdev_get_name(bdev), false, bdev_part_base_event_cb, - base, &base->desc); + rc = spdk_bdev_open_ext(bdev_name, false, bdev_part_base_event_cb, base, &base->desc); if (rc) { - spdk_bdev_part_base_free(base); - SPDK_ERRLOG("could not open bdev %s: %s\n", spdk_bdev_get_name(bdev), - spdk_strerror(-rc)); - return NULL; + if (rc == -ENODEV) { + free(base); + } else { + SPDK_ERRLOG("could not open bdev %s: %s\n", bdev_name, spdk_strerror(-rc)); + spdk_bdev_part_base_free(base); + } + return rc; } + base->bdev = spdk_bdev_desc_get_bdev(base->desc); + /* Save the thread where the base device is opened */ base->thread = spdk_get_thread(); - return base; + *_base = base; + + return 0; +} + +struct spdk_bdev_part_base * + spdk_bdev_part_base_construct(struct spdk_bdev *bdev, + spdk_bdev_remove_cb_t remove_cb, struct spdk_bdev_module *module, + struct spdk_bdev_fn_table *fn_table, struct bdev_part_tailq *tailq, + spdk_bdev_part_base_free_fn free_fn, void *ctx, + uint32_t channel_size, spdk_io_channel_create_cb ch_create_cb, + spdk_io_channel_destroy_cb ch_destroy_cb) +{ + struct spdk_bdev_part_base *base = NULL; + int rc; + + rc = spdk_bdev_part_base_construct_ext(spdk_bdev_get_name(bdev), remove_cb, module, + fn_table, tailq, free_fn, ctx, + channel_size, ch_create_cb, ch_destroy_cb, &base); + if (rc == 0) { + return base; + } else { + return NULL; + } } int diff --git a/lib/bdev/spdk_bdev.map b/lib/bdev/spdk_bdev.map index 80c3bb555..36a592a36 100644 --- a/lib/bdev/spdk_bdev.map +++ b/lib/bdev/spdk_bdev.map @@ -129,6 +129,7 @@ spdk_bdev_part_free; spdk_bdev_part_base_hotremove; spdk_bdev_part_base_construct; + spdk_bdev_part_base_construct_ext; spdk_bdev_part_construct; spdk_bdev_part_submit_request; spdk_bdev_part_get_bdev;