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 <shuhei.matsumoto.xt@hitachi.com>
Change-Id: I2db027a159559c403cdfbd71800afba590b0f328
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/4576
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>

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.

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.
  *

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

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;