[PATCHv2 0/4] nvme user metadata updates

[PATCHv2 0/4] nvme user metadata updates

[Keith Busch]

A few minor updates to the nvme user metadata handling, culminating in
actually making use of it for the NVME_IOCTL_IO_CMD.

Christoph Hellwig (1):
  nvme: factor metadata handling out of __nvme_submit_user_cmd

Keith Busch (3):
  nvme/pci: Use req_op to determine DIF remapping
  nvme: Make nvme user functions static
  nvme: Use metadata for passthrough commands

 drivers/nvme/host/core.c | 99 +++++++++++++++++++++++-------------------------
 drivers/nvme/host/nvme.h |  7 ----
 drivers/nvme/host/pci.c  |  4 +-
 3 files changed, 50 insertions(+), 60 deletions(-)

-- 
2.5.5

[PATCHv2 1/4] nvme: factor metadata handling out of __nvme_submit_user_cmd

[Keith Busch]

From: Christoph Hellwig <hch@lst.de>

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 0f3d4bf..a28f181 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -704,6 +704,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,
@@ -729,46 +763,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;
@@ -779,9 +784,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.5.5

[PATCHv2 1/4] nvme: factor metadata handling out of __nvme_submit_user_cmd

[Max Gurtovoy]

On 8/30/2017 12:46 AM, Keith Busch wrote:
> From: Christoph Hellwig <hch at lst.de>
> 
> 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(-)

snip

> 
>   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,
> @@ -729,46 +763,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;
> @@ -779,9 +784,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);

did we move the kfree(meta) here in porpose ?
I think we'll leak in the "write" case.

In general, I prefer allocating and freeing the meta buffer in the same 
function but maybe it make sense to act otherwise here.

>   	}
> - out_free_meta:
> -	kfree(meta);
>    out_unmap:
>   	if (bio)
>   		blk_rq_unmap_user(bio);
> 

[PATCHv2 1/4] nvme: factor metadata handling out of __nvme_submit_user_cmd

[Keith Busch]

On Wed, Aug 30, 2017@01:20:11AM +0300, Max Gurtovoy wrote:
> >   	blk_execute_rq(req->q, disk, req, 0);
> >   	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
> >   		ret = -EINTR;
> > @@ -779,9 +784,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);
> 
> did we move the kfree(meta) here in porpose ?
> I think we'll leak in the "write" case.

Ah, you're right! That should be freed unconditionally outside the 'if'
(it's okay to send NULL to kfree). I'll resend with the fixup.
 
> In general, I prefer allocating and freeing the meta buffer in the same
> function but maybe it make sense to act otherwise here.

Well, the metadata setup was a long enough section with five indent levels
that having its own function should improve readability.

[PATCHv2 1/4] nvme: factor metadata handling out of __nvme_submit_user_cmd

[Christoph Hellwig]

> did we move the kfree(meta) here in porpose ?
> I think we'll leak in the "write" case.

Oops, my fault.

> In general, I prefer allocating and freeing the meta buffer in the same 
> function but maybe it make sense to act otherwise here.

Think of nvme_add_user_metadata as the memory allocator, given
that it returns the buffer :)

[PATCHv2 2/4] nvme/pci: Use req_op to determine DIF remapping

[Keith Busch]

Only read and write commands need DIF remapping. Everything else uses
a passthrough integrity payload.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 925467b..75c0007 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -666,7 +666,7 @@ static blk_status_t nvme_map_data(struct nvme_dev *dev, struct request *req,
 		if (blk_rq_map_integrity_sg(q, req->bio, &iod->meta_sg) != 1)
 			goto out_unmap;
 
-		if (rq_data_dir(req))
+		if (req_op(req) == REQ_OP_WRITE)
 			nvme_dif_remap(req, nvme_dif_prep);
 
 		if (!dma_map_sg(dev->dev, &iod->meta_sg, 1, dma_dir))
@@ -694,7 +694,7 @@ static void nvme_unmap_data(struct nvme_dev *dev, struct request *req)
 	if (iod->nents) {
 		dma_unmap_sg(dev->dev, iod->sg, iod->nents, dma_dir);
 		if (blk_integrity_rq(req)) {
-			if (!rq_data_dir(req))
+			if (req_op(req) == REQ_OP_READ)
 				nvme_dif_remap(req, nvme_dif_complete);
 			dma_unmap_sg(dev->dev, &iod->meta_sg, 1, dma_dir);
 		}
-- 
2.5.5

[PATCHv2 2/4] nvme/pci: Use req_op to determine DIF remapping

[Max Gurtovoy]

On 8/30/2017 12:46 AM, Keith Busch wrote:
> Only read and write commands need DIF remapping. Everything else uses
> a passthrough integrity payload.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>   drivers/nvme/host/pci.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Looks good,

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCHv2 2/4] nvme/pci: Use req_op to determine DIF remapping

[Christoph Hellwig]

On Tue, Aug 29, 2017@05:46:02PM -0400, Keith Busch wrote:
> Only read and write commands need DIF remapping. Everything else uses
> a passthrough integrity payload.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>

Cc to stable because this means metadata passthrough for NVME_SUBMIT_IO
has been broken for a long time?

Also this reminds that that I really want to move the
remapping to the block core.

[PATCHv2 3/4] nvme: Make nvme user functions static

[Keith Busch]

These functions are used only locally in the nvme core.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 10 +++++-----
 drivers/nvme/host/nvme.h |  7 -------
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a28f181..4106e09 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -738,10 +738,10 @@ static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
 	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,
-		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;
@@ -794,7 +794,7 @@ 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,
+static int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 		void __user *ubuffer, unsigned bufflen, u32 *result,
 		unsigned timeout)
 {
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

[PATCHv2 3/4] nvme: Make nvme user functions static

[Max Gurtovoy]

On 8/30/2017 12:46 AM, Keith Busch wrote:
> These functions are used only locally in the nvme core.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>   drivers/nvme/host/core.c | 10 +++++-----
>   drivers/nvme/host/nvme.h |  7 -------
>   2 files changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a28f181..4106e09 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c

looks good,

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCHv2 4/4] 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.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4106e09..05d8545 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -738,7 +738,7 @@ static void *nvme_add_user_metadata(struct bio *bio, void __user *ubuf,
 	return ERR_PTR(ret);
 }
 
-static int __nvme_submit_user_cmd(struct request_queue *q,
+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)
@@ -794,14 +794,6 @@ static int __nvme_submit_user_cmd(struct request_queue *q,
 	return ret;
 }
 
-static 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;
@@ -1091,7 +1083,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);
 }
@@ -1129,7 +1121,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;
-- 
2.5.5

[PATCHv2 4/4] nvme: Use metadata for passthrough commands

[Max Gurtovoy]

On 8/30/2017 12:46 AM, 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.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>   drivers/nvme/host/core.c | 15 ++++-----------
>   1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4106e09..05d8545 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c

looks good,

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCHv2 0/4] nvme user metadata updates

[Christoph Hellwig]

Except for the messup in my patch this looks fine to me:

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCHv2 0/4] nvme user metadata updates

[Sagi Grimberg]

The series looks good to me,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCHv2 0/4] nvme user metadata updates

[Christoph Hellwig]

Applied to the nvme-4.14 branch with the kfree move fixed up.