NVMF/RDMA 8K Inline Support

drivers/nvme/target/configfs.c +5 -0

+}
+
+static ssize_t nvmet_inline_data_size_store(struct config_item *item,
+		const char *page, size_t count)
+{

+CONFIGFS_ATTR(nvmet_, inline_data_size);

+#define NVMET_DEFAULT_INLINE_DATA_SIZE -1

drivers/nvme/target/configfs.c static ssize_t nvmet_addr_trsvcid_store(struct config_item *item, · line 223

Quoted 7 lines

drivers/nvme/target/configfs.c +7 -0

+static ssize_t nvmet_inline_data_size_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+
+	return snprintf(page, PAGE_SIZE, "%u\n",
+			port->inline_data_size);

Please fir the whole sprintf statement onto a single line.

Quoted 5 lines

drivers/nvme/target/configfs.c +5 -0

+}
+
+static ssize_t nvmet_inline_data_size_store(struct config_item *item,
+		const char *page, size_t count)
+{

drivers/nvme/target/configfs.c static ssize_t nvmet_addr_trsvcid_store(struct config_item *item, · line 238

Quoted 10 lines

drivers/nvme/target/configfs.c +10 -0

+	struct nvmet_port *port = to_nvmet_port(item);
+	unsigned int size;
+	int ret;
+
+	if (port->enabled) {
+		pr_err("Cannot modify inline_data_size enabled\n");
+		pr_err("Disable the port before modifying\n");
+		return -EACCES;
+	}
+	ret = kstrtouint((const char *)page, 0, &size);

This cast looks bogus.

Also inline_data_size shoul be and u32 as that is closest to what is on the wire, and you thus should use kstrtou32 and pass the inline_data_size straight to kstrtou32 instead of bouncing it through a local variable.

Quoted 1 line

+CONFIGFS_ATTR(nvmet_, inline_data_size);

The characters before the first _ in the name are used as a group by nvmetcli. So I think this should get a param_ or so prefix before the inline_data_size. Also currently this attribute only makes sense for rdma, so I think we still need a flag in nvmet_fabrics_ops that enables/disables this attribute.

Last but not least please also send a nvmetcli patch to support this new attribute.

Quoted 1 line

+#define NVMET_DEFAULT_INLINE_DATA_SIZE -1

0 makes much more sense as the default, and then we don't even need a name for it.

drivers/nvme/target/rdma.c line 39

Quoted 2 lines

drivers/nvme/target/rdma.c +2 -0

+#define NVMET_RDMA_DEFAULT_INLINE_DATA_SIZE	PAGE_SIZE
+#define NVMET_RDMA_MAX_INLINE_DATA_SIZE		max_t(int, SZ_16K, PAGE_SIZE)

So for 64k pages the minimum is bigger than the maximum? :)

Quoted 7 lines

drivers/nvme/target/configfs.c static ssize_t nvmet_addr_trsvcid_store(struct config_item *item, · line 238

Quoted 1 line

drivers/nvme/target/configfs.c +1 -0

+	ret = kstrtouint((const char *)page, 0, &size);

This cast looks bogus.

Also inline_data_size shoul be and u32 as that is closest to what is on the wire, and you thus should use kstrtou32 and pass the inline_data_size straight to kstrtou32 instead of bouncing it through a local variable.

I made it an int so it could be initialized to -1 indicating it is not set by the config.? This allows the rdma transport to use its default value if the config does not specify any value.? I did this so the admin could totally disable inline by specifying 0.?? So I needed a value that indicates "unspecified".

Quoted 3 lines

Quoted 1 line

+#define NVMET_DEFAULT_INLINE_DATA_SIZE -1

0 makes much more sense as the default, and then we don't even need a name for it.

I wanted the user to be able to disable inline by setting it to 0.? Is that not needed?? Maybe by adding back the nvmet_fabrics_ops field will alleviate this issue.? Perhaps a default_inline_size field that rdma sets to PAGE_SIZE.? Then configfs can default it to that.?

Quoted 4 lines

drivers/nvme/target/rdma.c line 39

Quoted 3 lines

drivers/nvme/target/rdma.c +2 -0

+#define NVMET_RDMA_DEFAULT_INLINE_DATA_SIZE	PAGE_SIZE
+#define NVMET_RDMA_MAX_INLINE_DATA_SIZE		max_t(int, SZ_16K, PAGE_SIZE)

So for 64k pages the minimum is bigger than the maximum? :)

For 64k pages, the default is 64K and the max is 64K.

Quoted 8 lines

Quoted 7 lines

drivers/nvme/target/configfs.c static ssize_t nvmet_addr_trsvcid_store(struct config_item *item, · line 238

Quoted 1 line

drivers/nvme/target/configfs.c +1 -0

+	ret = kstrtouint((const char *)page, 0, &size);

This cast looks bogus.

Also inline_data_size shoul be and u32 as that is closest to what is on the wire, and you thus should use kstrtou32 and pass the inline_data_size straight to kstrtou32 instead of bouncing it through a local variable.

Quoted 5 lines

I made it an int so it could be initialized to -1 indicating it is not set by the config.? This allows the rdma transport to use its default value if the config does not specify any value.? I did this so the admin could totally disable inline by specifying 0.?? So I needed a value that indicates "unspecified".

Ok, make sense. So lets keep that behavior, and let every negative value mean default so that we don't need another error check here. Rest of the comments above still stands.

For small nvmf write IO over the rdma transport, it is advantagous to make use of inline mode to avoid the latency of the target issuing an rdma read to fetch the data. Currently inline is used for <= 4K writes. 8K, though, requires the rdma read. For iWARP transports additional latency is incurred because the target mr of the read must be registered with remote write access. By allowing 2 pages worth of inline payload, I see a reduction in 8K nvmf write latency of anywhere from 2-7 usecs depending on the RDMA transport..

This series is a respin of a series floated last year by Parav and Max [1]. I'm continuing it now and trying to address the comments from their submission.

A few of the comments have been addressed:

- nvme-rdma: Support up to 4 segments of inline data.

- nvme-rdma: Cap the number of inline segments to not exceed device limitations.

- nvmet-rdma: Make the inline data size configurable in nvmet-rdma via configfs.

Other issues I haven't addressed:

- nvme-rdma: make the sge array for inline segments dynamic based on the target's advertised inline_data_size. Since we're limiting the max count to 4, I'm not sure this is worth the complexity of allocating the sge array vs just embedding the max.

- nvmet-rdma: concern about high order page allocations. Is 4 pages too high? One possibility is that, if the device max_sge allows, use a few more sges. IE 16K could be 2 8K sges, or 4 4K. This probably makes passing the inline data to bio more complex. I haven't looked into this yet.

- nvmet-rdma: reduce the qp depth if the inline size greatly increases the memory footprint. I'm not sure how to do this in a reasonable mannor. Since the inline data size is now configurable, do we still need this?

- nvmet-rdma: make the qp depth configurable so the admin can reduce it manually to lower the memory footprint.

Please comment!

Thanks,

Steve.

[1] Original submissions:
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008057.html
http://lists.infradead.org/pipermail/linux-nvme/2017-February/008059.html

Steve Wise (2):
nvme-rdma: support up to 4 segments of inline data
nvmet-rdma: support 16K inline data

 drivers/nvme/host/rdma.c        | 34 +++++++++++++++++++++++-----------
 drivers/nvme/target/admin-cmd.c |  4 ++--
 drivers/nvme/target/configfs.c  | 34 ++++++++++++++++++++++++++++++++++
 drivers/nvme/target/discovery.c |  2 +-
 drivers/nvme/target/nvmet.h     |  4 +++-
 drivers/nvme/target/rdma.c      | 41 +++++++++++++++++++++++++++++------------
 6 files changed, 92 insertions(+), 27 deletions(-)