Bdev ext api by AlekseyMarchuk · Pull Request #36 · Mellanox/spdk

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


+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 <alexeymar@mellanox.com>

New functions accept extedable structure of IO options Change-Id: If6864df151a3c0ad722785cb26d1f5d4309cd733 Signed-off-by: Alexey Marchuk <alexeymar@mellanox.com>

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 <alexeymar@mellanox.com>

The new API is used if flags is not zero. Change-Id: I414b5d19bff54114d6708efed89ba19b5955f56a Signed-off-by: Alexey Marchuk <alexeymar@mellanox.com>

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

 	/** 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 <alexeymar@mellanox.com>

[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 <alexeymar@mellanox.com>

[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 <alexeymar@mellanox.com>

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

Conversation

AlekseyMarchuk commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyMarchuk commented Jan 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyMarchuk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants