[PATCH] nvme: Use metadata for passthrough commands

[PATCH] nvme: Use metadata for passthrough commands

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

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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0f3d4bf..df282e4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1fc35de..194b4c8 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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);
-- 
2.5.5

[PATCH] nvme: Use metadata for passthrough commands

[Christoph Hellwig]

On Mon, Aug 28, 2017@06:03:00PM -0400, Keith Busch wrote:
> 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.

[PATCH] nvme: Use metadata for passthrough commands

[Christoph Hellwig]

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

---
>From 5f7f9b6c2f14be938d178cd39ccc92eea61044ab Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b0dd58db110e..8e0c041cdc78 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
-- 
2.11.0

[PATCH] nvme: Use metadata for passthrough commands

[Keith Busch]

On Tue, Aug 29, 2017@10:43:50AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 28, 2017@06:03:00PM -0400, Keith Busch wrote:
> > 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.

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

> 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;
> 	}
> 
> ?

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.


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