Bdev ext api #36

AlekseyMarchuk · 2021-01-14T07:25:05Z

No description provided.

EugeneKochetov · 2021-01-14T07:29:23Z

include/spdk/bdev.h

+struct spdk_bdev_io_memory_key_ctx {
+	void *ctx;
+	enum spdk_bdev_io_memory_key_ctx_type ctx_type;
+	/** Memory key to be filled by the user of \b spdk_bdev_readv_with_md_ext of \b spdk_bdev_writev_with_md_ext


2nd "of" -> "or"

EugeneKochetov · 2021-01-14T07:59:59Z

lib/bdev/bdev.c

+			return -EINVAL;
+		}
+
+		bdev_copy_ext_io_opts(opts, &opts_local, opts->opts_size);


We have two copies of opts: here and in bdev_readv_blocks_with_md. Can we optimize somehow and keep just one?

Will rework to pass user's struct and check if it is NULL or not

EugeneKochetov · 2021-01-14T08:00:15Z

lib/bdev/bdev.c

+	} \
+
+	SET_FIELD(get_mkey_cb);
+	SET_FIELD(get_mkey_cb_arg);


Why not just memcpy?

EugeneKochetov · 2021-01-14T08:00:17Z

lib/bdev/bdev.c

+	dst->opts_size = opts_size;
+
+#define SET_FIELD(field) \
+	if (offsetof(struct spdk_bdev_ext_io_opts, field) + sizeof(src->field) <= opts_size) { \


Can it be that src and dst have different size?

EugeneKochetov · 2021-01-14T08:00:20Z

lib/bdev/bdev.c

+
+static void
+bdev_copy_ext_io_opts(const struct spdk_bdev_ext_io_opts *src, struct spdk_bdev_ext_io_opts *dst,
+		      size_t opts_size)


Why do we need opts_size argument? We have one in src.

EugeneKochetov · 2021-01-14T08:06:16Z

lib/bdev/bdev.c

+	}
+
+	memset(&caps_local, 0, sizeof(caps_local));
+	caps_local.size = caps->size;


If user was built with newer version, caps->size can be larger than caps_local structure size

EugeneKochetov · 2021-01-14T10:44:12Z

include/spdk/bdev.h

+	enum spdk_bdev_io_memory_key_ctx_type ctx_type;
+	/** Memory key to be filled by the user of \b spdk_bdev_readv_with_md_ext of \b spdk_bdev_writev_with_md_ext
+	 * when bdev module calls \b spdk_bdev_io_get_mkey */
+	uint32_t mkey;


Need more description here. It is not very clear that ctx and ctx_typre are input parameters and mkey is out parameter.

EugeneKochetov · 2021-01-14T10:48:26Z

include/spdk/bdev.h

@@ -1733,6 +1733,28 @@ void spdk_bdev_histogram_get(struct spdk_bdev *bdev, struct spdk_histogram_data
 size_t spdk_bdev_get_media_events(struct spdk_bdev_desc *bdev_desc,
 				  struct spdk_bdev_media_event *events, size_t max_events);

+enum spdk_bdev_capability_type {
+	/** Bdev module doesn't read or modify data buffer passed in IO request */
+	SPDK_BDEV_CAP_DATA_PASSTHRU = 1u << 0u,


The description is not correct here. We use it to determine whether bdev supports IOs with mkey extended opt.
And the name should be changed.
Now I don't see any relation between this bdev capability and extended IO opts introduced in the next commits

AlekseyMarchuk · 2021-01-14T11:12:48Z

include/spdk/nvme.h

+					spdk_nvme_cmd_cb cb_fn, void *cb_arg, uint32_t io_flags,
+					spdk_nvme_req_reset_sgl_cb reset_sgl_fn,
+					spdk_nvme_req_next_sge_cb next_sge_fn, void *metadata,
+					uint16_t apptag_mask, uint16_t apptag,


Move apptag_mask, apptag and io_flags to ext_opts structure

The new API spdk_bdev_get_caps returns capabilities specific for bdev module. Change-Id: Ic9f42eff59bdc4c8c6e73deb76b3eecfc04f80a8 Signed-off-by: Alexey Marchuk <[email protected]>

New functions accept extedable structure of IO options Change-Id: If6864df151a3c0ad722785cb26d1f5d4309cd733 Signed-off-by: Alexey Marchuk <[email protected]>

These functions accept extendable structure with IO request options. The options structure contains a callback to get a memory key per Io request. This callback is used in RDMA transport. Change-Id: I65bfba279904e77539348520c3dfac7aadbe80d9 Signed-off-by: Alexey Marchuk <[email protected]>

The new API is used if flags is not zero. Change-Id: I414b5d19bff54114d6708efed89ba19b5955f56a Signed-off-by: Alexey Marchuk <[email protected]>

AlekseyMarchuk · 2021-01-15T09:14:19Z

include/spdk/bdev.h

+ * \param caps Capabilities of Block device to be filled by this function
+ * \return 0 on success, negated errno on failure.
+ */
+int spdk_bdev_get_caps(struct spdk_bdev *bdev, struct spdk_bdev_capability *caps);


rename to
spdk_bdev_get_ext_caps

AlekseyMarchuk · 2021-01-15T09:38:52Z

include/spdk/bdev.h

+	/** Bdev supports indirect memory access using Memory Key.
+	 * That means that the user of ext bdev API can fill spdk_bdev_ext_io_opts_mem_type
+	 * structure and set SPDK_BDEV_EXT_IO_OPTS_MEM_TYPE flag in spdk_bdev_ext_io_opts structure */
+	SPDK_BDEV_CAP_EXT_MEMORY_TYPE_MKEY = 1u << 0u,


/** Bdev supports indirect memory access using Memory Key.

That means that the user of ext bdev API can fill spdk_bdev_ext_io_opts_mem_type

structure and set SPDK_BDEV_EXT_IO_OPTS_MEM_TYPE flag in spdk_bdev_ext_io_opts structure

That also means that bdev can work with regular memory buffers */

AlekseyMarchuk · 2021-01-15T09:39:18Z

include/spdk/bdev_module.h

@@ -222,6 +222,9 @@ struct spdk_bdev_fn_table {

 	/** Get bdev module context. */
 	void *(*get_module_ctx)(void *ctx);
+
+	/** Get block device capabilities */


extended capabilities

AlekseyMarchuk · 2021-01-15T09:39:59Z

module/bdev/delay/vbdev_delay.c

+	struct vbdev_delay *delay_node = (struct vbdev_delay *)ctx;
+
+	if (delay_node->base_bdev->fn_table->get_caps) {
+		delay_node->base_bdev->fn_table->get_caps(delay_node->base_bdev, caps);


uint64_t local_flags = 0;

if (caps->flags & SPDK_BDEV_CAP_EXT_MEMORY_TYPE_MKEY) {
/* Delay bdev doesn't modify data so it can work with indirect memory if it is supported by underlying bdev */
local_flags |= SPDK_BDEV_CAP_EXT_MEMORY_TYPE_MKEY
}
caps->flags = local_flags;

compress/crypto:
uint64_t local_flags = 0;

AlekseyMarchuk · 2021-01-15T10:08:12Z

bdev in spdk can work with mkey - bdev_nvme over RDMA
return caps with MKEY bit
bdev can work with mkey but this bdev is not part of spdk (e.g. alibaba pangu)
aware of SNAP - set MKEY bit
otherise - do not set bit
vbdev in spdk modifies data (vbdev_compress)
default - get_caps cb is not implemented
we can update all vbdevs and set correct bits
vbdev out of spdk that touches data
default - get_caps cb is not implemented
vbdev raid0 - doesn't modify data
lvol management (proprietary)
proprietary vbdev that doesn't modify data but it is desgined for snap (aware of cross gvmi)

AlekseyMarchuk · 2021-01-18T08:05:37Z

include/spdk/bdev.h

+ */
+struct spdk_bdev_ext_io_opts {
+	/** Combination of bits defined in \b enum spdk_bdev_ext_io_opts_flags */
+	uint64_t flags;


flags -> comp_mask

AlekseyMarchuk · 2021-01-18T08:13:17Z

include/spdk/bdev.h

+ * Callback used to get a Memory Key per IO request
+ *
+ * pd is input parameter and should point to a memory domain
+ * mkey is an output value


Add comments for other arguments

AlekseyMarchuk · 2021-01-18T08:13:44Z

include/spdk/bdev.h

+ * pd is input parameter and should point to a memory domain
+ * mkey is an output value
+ */
+typedef int (*spdk_bdev_io_get_mkey)(void *cb_arg, void *address, size_t length, void *pd,


Use forward declaration of ibv_pd structure

AlekseyMarchuk · 2021-01-18T08:17:18Z

include/spdk/bdev.h

+ * mkey is an output value
+ */
+typedef int (*spdk_bdev_io_get_mkey)(void *cb_arg, void *address, size_t length, void *pd,
+				     uint32_t *mkey);


return 2 keys - local and remote?
Ask if we need 2 keys

AlekseyMarchuk · 2021-01-18T08:21:12Z

include/spdk/nvme.h

+	enum spdk_nvme_ns_cmd_ext_io_opts_mem_types type;
+	union {
+		struct {
+			spdk_nvme_ns_cmd_io_get_mkey get_mkey_cb;


add void *cb_arg

AlekseyMarchuk

1

EugeneKochetov reviewed Jan 14, 2021

View reviewed changes

AlekseyMarchuk commented Jan 14, 2021

View reviewed changes

AlekseyMarchuk added 4 commits January 14, 2021 16:04

[RFC] bdev: Add API to get bdev capabilities

b681709

The new API spdk_bdev_get_caps returns capabilities specific for bdev module. Change-Id: Ic9f42eff59bdc4c8c6e73deb76b3eecfc04f80a8 Signed-off-by: Alexey Marchuk <[email protected]>

[RFC] bdev: Add extended versions of readv/writev_with_md

7e1c1f0

New functions accept extedable structure of IO options Change-Id: If6864df151a3c0ad722785cb26d1f5d4309cd733 Signed-off-by: Alexey Marchuk <[email protected]>

[RFC] bdev_nvme: Use new extended API

130167c

The new API is used if flags is not zero. Change-Id: I414b5d19bff54114d6708efed89ba19b5955f56a Signed-off-by: Alexey Marchuk <[email protected]>

AlekseyMarchuk force-pushed the bdev_ext_api branch from 9c31a76 to 130167c Compare January 14, 2021 16:34

AlekseyMarchuk commented Jan 15, 2021

View reviewed changes

AlekseyMarchuk commented Jan 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bdev ext api #36

Bdev ext api #36

AlekseyMarchuk commented Jan 14, 2021

EugeneKochetov Jan 14, 2021

EugeneKochetov Jan 14, 2021

AlekseyMarchuk Jan 14, 2021

EugeneKochetov Jan 14, 2021

EugeneKochetov Jan 14, 2021

EugeneKochetov Jan 14, 2021

EugeneKochetov Jan 14, 2021

EugeneKochetov Jan 14, 2021

EugeneKochetov Jan 14, 2021

AlekseyMarchuk Jan 14, 2021

AlekseyMarchuk Jan 15, 2021

AlekseyMarchuk Jan 15, 2021

AlekseyMarchuk Jan 15, 2021

AlekseyMarchuk Jan 15, 2021

AlekseyMarchuk commented Jan 15, 2021

AlekseyMarchuk Jan 18, 2021

AlekseyMarchuk Jan 18, 2021

AlekseyMarchuk Jan 18, 2021

AlekseyMarchuk Jan 18, 2021

AlekseyMarchuk Jan 18, 2021

AlekseyMarchuk left a comment

Bdev ext api #36

Are you sure you want to change the base?

Bdev ext api #36

Conversation

AlekseyMarchuk commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyMarchuk commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyMarchuk left a comment

Choose a reason for hiding this comment