nvme: Use metadata for passthrough commands

PATCH New Keith Busch 2 participants 4 messages Aug 28, 2017 – Aug 29, 2017 NVMe

Share & tools

Keith Busch

author

2017-08-28 22:03 UTC #0 lore raw

PATCH 1/1 nvme: Use metadata for passthrough commands

The ioctls' struct allows the user to provide a metadata address and length for a passthrough command. This patch uses these values that were previously ignored and deletes the now unused wrapper function.

Note the implementation currently requires a gendisk so will not work for admin commands.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/core.c | 21 +++++++--------------
 drivers/nvme/host/nvme.h |  7 -------
 2 files changed, 7 insertions(+), 21 deletions(-)

drivers/nvme/host/core.c +7 ~~-14~~

index 0f3d4bf..df282e4 100644
@@ -704,10 +704,10 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 }
 EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
 
-int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
-		void __user *ubuffer, unsigned bufflen,
-		void __user *meta_buffer, unsigned meta_len, u32 meta_seed,
-		u32 *result, unsigned timeout)
+static int nvme_submit_user_cmd(struct request_queue *q,
+		struct nvme_command *cmd, void __user *ubuffer,
+		unsigned bufflen, void __user *meta_buffer, unsigned meta_len,
+		u32 meta_seed, u32 *result, unsigned timeout)
 {
 	bool write = nvme_is_write(cmd);
 	struct nvme_ns *ns = q->queuedata;

@@ -790,14 +790,6 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 	return ret;
 }
 
-int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
-		void __user *ubuffer, unsigned bufflen, u32 *result,
-		unsigned timeout)
-{
-	return __nvme_submit_user_cmd(q, cmd, ubuffer, bufflen, NULL, 0, 0,
-			result, timeout);
-}
-
 static void nvme_keep_alive_end_io(struct request *rq, blk_status_t status)
 {
 	struct nvme_ctrl *ctrl = rq->end_io_data;

@@ -1087,7 +1079,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct nvme_user_io __user *uio)
 	c.rw.apptag = cpu_to_le16(io.apptag);
 	c.rw.appmask = cpu_to_le16(io.appmask);
 
-	return __nvme_submit_user_cmd(ns->queue, &c,
+	return nvme_submit_user_cmd(ns->queue, &c,
 			(void __user *)(uintptr_t)io.addr, length,
 			metadata, meta_len, io.slba, NULL, 0);
 }

@@ -1125,7 +1117,8 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 
 	status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
 			(void __user *)(uintptr_t)cmd.addr, cmd.data_len,
-			&cmd.result, timeout);
+			(void __user *)(uintptr_t)cmd.metadata, cmd.metadata,
+			0, &cmd.result, timeout);
 	if (status >= 0) {
 		if (put_user(cmd.result, &ucmd->result))
 			return -EFAULT;

drivers/nvme/host/nvme.h +0 -7

index 1fc35de..194b4c8 100644
@@ -342,13 +342,6 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 		union nvme_result *result, void *buffer, unsigned bufflen,
 		unsigned timeout, int qid, int at_head, int flags);
-int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
-		void __user *ubuffer, unsigned bufflen, u32 *result,
-		unsigned timeout);
-int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
-		void __user *ubuffer, unsigned bufflen,
-		void __user *meta_buffer, unsigned meta_len, u32 meta_seed,
-		u32 *result, unsigned timeout);
 int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count);
 void nvme_start_keep_alive(struct nvme_ctrl *ctrl);
 void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);

Signature · 1 line

2.5.5

Christoph Hellwig

maintainer

2017-08-29 08:43 UTC #1 lore raw

PATCH 1/1 nvme: Use metadata for passthrough commands

Quoted 6 lines · Keith Busch

The ioctls' struct allows the user to provide a metadata address and length for a passthrough command. This patch uses these values that were previously ignored and deletes the now unused wrapper function.
Note the implementation currently requires a gendisk so will not work for admin commands.

This looks generally ok. I thought NVME_IOCTL_SUBMIT_IO was added because the other ioctls don't supported metadata, but history tells me it was the other way around, and NVME_IOCTL_SUBMIT_IO came before the other ioctls.

But that begs the question: is it time to deprecate NVME_IOCTL_SUBMIT_IO slowly? nvme-cli unfortunately still uses it for read/write/compare though.

Do we need this sniplet from nvme_submit_io in nvme_user_cmd or nvme_submit_user_cmd as well:

	if (ns->ext) {
		length += meta_len;
		meta_len = 0;
	} else if (meta_len) {
		if ((io.metadata & 3) || !io.metadata)
			return -EINVAL;
	}

Also last but not least I'd be tempted to say that the removal of __nvme_submit_user_cmd shold be a separate prep patch.

Keith Busch

author

2017-08-29 19:43 UTC #2 lore raw

PATCH 1/1 nvme: Use metadata for passthrough commands

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Keith Busch
The ioctls' struct allows the user to provide a metadata address and length for a passthrough command. This patch uses these values that were previously ignored and deletes the now unused wrapper function.

Quoted 7 lines

Quoted 2 lines
Note the implementation currently requires a gendisk so will not work for admin commands.
This looks generally ok. I thought NVME_IOCTL_SUBMIT_IO was added because the other ioctls don't supported metadata, but history tells me it was the other way around, and NVME_IOCTL_SUBMIT_IO came before the other ioctls.

Yep, NVME_IOCTL_SUBMIT_IO was conceived when read/write/compare were the only commands in the IO set. The structure is unusable for any other IO commands, so we extended the admin passthrough only after it was too late to undo SUBMIT_IO.

Quoted 3 lines

But that begs the question: is it time to deprecate NVME_IOCTL_SUBMIT_IO slowly? nvme-cli unfortunately still uses it for read/write/compare though.

I can certainly discourage new usage of SUBMIT_IO. I'll look into having nvme-cli use the passthrough, but it'll break some usage for existing kernels, so the transition may be slow.

Quoted 11 lines

Do we need this sniplet from nvme_submit_io in nvme_user_cmd or nvme_submit_user_cmd as well:
	if (ns->ext) {
		length += meta_len;
		meta_len = 0;
	} else if (meta_len) {
		if ((io.metadata & 3) || !io.metadata)
			return -EINVAL;
	}

Quoted 1 line

?

That shouldn't be necessary with passthrough ioctl since it takes transfer lengths directly from the user. The SUBMIT_IO ioctl had to infer these based on the LBA.

Quoted 2 lines

Also last but not least I'd be tempted to say that the removal of __nvme_submit_user_cmd shold be a separate prep patch.

Sounds good, I'll resend with that split, along with your patch separating metadata setup from the rest of the user command.

I also noticed we need to skip nvme_dif_remap for REQ_OP_DRV_IN/OUT requests for this to really work. I'll add that to the series.

Christoph Hellwig

maintainer

2017-08-29 09:30 UTC #3 lore raw

PATCH 1/1 nvme: Use metadata for passthrough commands

In reply to Keith Busch

Btw, what do you think about throwing in a patch liks this if we're touching the area anyway?

---

Quoted 1 line

From 5f7f9b6c2f14be938d178cd39ccc92eea61044ab Mon Sep 17 00:00:00 2001

From: Christoph Hellwig <[email protected]>
Date: Tue, 29 Aug 2017 11:24:52 +0200
Subject: nvme: factor metadata handling out of __nvme_submit_user_cmd

Keep the metadata code in a separate helper instead of making the main function more complicated.

Signed-off-by Christoph Hellwig <hch at lst.de>

---
 drivers/nvme/host/core.c | 78 +++++++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 37 deletions(-)

drivers/nvme/host/core.c +41 ~~-37~~

index b0dd58db110e..8e0c041cdc78 100644
@@ -600,6 +600,40 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 }
 EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
 
+static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
+		unsigned len, u32 seed, bool write)
+{
+	struct bio_integrity_payload *bip;
+	int ret = -ENOMEM;
+	void *buf;
+
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		goto out;
+
+	ret = -EFAULT;
+	if (write && copy_from_user(buf, buf, len))
+		goto out_free_meta;
+
+	bip = bio_integrity_alloc(bio, GFP_KERNEL, 1);
+	if (IS_ERR(bip)) {
+		ret = PTR_ERR(bip);
+		goto out_free_meta;
+	}
+
+	bip->bip_iter.bi_size = len;
+	bip->bip_iter.bi_sector = seed;
+	ret = bio_integrity_add_page(bio, virt_to_page(buf), len,
+			offset_in_page(buf));
+	if (ret == len)
+		return buf;
+	ret = -ENOMEM;
+out_free_meta:
+	kfree(buf);
+out:
+	return ERR_PTR(ret);
+}
+
 int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void __user *ubuffer, unsigned bufflen,
 		void __user *meta_buffer, unsigned meta_len, u32 meta_seed,

@@ -625,46 +659,17 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		if (ret)
 			goto out;
 		bio = req->bio;
-
-		if (!disk)
-			goto submit;
 		bio->bi_disk = disk;
-
-		if (meta_buffer && meta_len) {
-			struct bio_integrity_payload *bip;
-
-			meta = kmalloc(meta_len, GFP_KERNEL);
-			if (!meta) {
-				ret = -ENOMEM;
+		if (disk && meta_buffer && meta_len) {
+			meta = nvme_add_user_metadata(bio, meta_buffer, meta_len,
+					meta_seed, write);
+			if (IS_ERR(meta)) {
+				ret = PTR_ERR(meta);
 				goto out_unmap;
 			}
-
-			if (write) {
-				if (copy_from_user(meta, meta_buffer,
-						meta_len)) {
-					ret = -EFAULT;
-					goto out_free_meta;
-				}
-			}
-
-			bip = bio_integrity_alloc(bio, GFP_KERNEL, 1);
-			if (IS_ERR(bip)) {
-				ret = PTR_ERR(bip);
-				goto out_free_meta;
-			}
-
-			bip->bip_iter.bi_size = meta_len;
-			bip->bip_iter.bi_sector = meta_seed;
-
-			ret = bio_integrity_add_page(bio, virt_to_page(meta),
-					meta_len, offset_in_page(meta));
-			if (ret != meta_len) {
-				ret = -ENOMEM;
-				goto out_free_meta;
-			}
 		}
 	}
- submit:
+
 	blk_execute_rq(req->q, disk, req, 0);
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
 		ret = -EINTR;

@@ -675,9 +680,8 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 	if (meta && !ret && !write) {
 		if (copy_to_user(meta_buffer, meta, meta_len))
 			ret = -EFAULT;
+		kfree(meta);
 	}
- out_free_meta:
-	kfree(meta);
  out_unmap:
 	if (bio)
 		blk_rq_unmap_user(bio);

Signature · 1 line

2.11.0