nvme user metadata updates
v2 cover nvme user metadata updates
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(-)
Signature · 1 line
2.5.5
PATCH v2 1/4 nvme: factor metadata handling out of __nvme_submit_user_cmd
From: Christoph Hellwig <[email protected]>
Keep the metadata code in a separate helper instead of making the main function more complicated.
--- drivers/nvme/host/core.c | 78 +++++++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 37 deletions(-)
drivers/nvme/host/core.c +41 -37
@@ -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 &©_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);
Signature · 1 line
2.5.5
PATCH v2 1/4 nvme: factor metadata handling out of __nvme_submit_user_cmd
Quoted 9 lines · Keith Busch
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
Quoted 58 lines
int __nvme_submit_user_cmd(struct
request_queue*q, structnvme_command*cmd,
void__user*ubuffer, unsigned bufflen,
void__user*meta_buffer, unsignedmeta_len, u32meta_seed,drivers/nvme/host/core.c +6
-35@@ -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 ;
drivers/nvme/host/core.c int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd, · line 787
Quoted 4 lines
drivers/nvme/host/core.c +1
-0@@ -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.
Quoted 7 lines
drivers/nvme/host/core.c +0
-2} - out_free_meta: -kfree (meta); out_unmap:if (bio)blk_rq_unmap_user (bio);
PATCH v2 1/4 nvme: factor metadata handling out of __nvme_submit_user_cmd
Quoted 11 lines · Max Gurtovoy
drivers/nvme/host/core.c int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd, · line 787
Quoted 8 lines
blk_execute_rq(req->q, disk, req, 0); if (nvme_req(req)->flags & NVME_REQ_CANCELLED) ret = -EINTR;drivers/nvme/host/core.c +1
-0@@ -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.
Quoted 2 lines
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.
Christoph Hellwig
maintainerPATCH v2 1/4 nvme: factor metadata handling out of __nvme_submit_user_cmd
In reply to Max GurtovoyQuoted 2 lines
did we move the kfree(meta) here in porpose ?
I think we'll leak in the "write" case.
Oops, my fault.
Quoted 2 lines
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 :)
PATCH v2 2/4 nvme/pci: Use req_op to determine DIF remapping
In reply to Keith BuschOnly read and write commands need DIF remapping. Everything else uses a passthrough integrity payload.
--- drivers/nvme/host/pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/nvme/host/pci.c +2 -2
@@ -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); }
Signature · 1 line
2.5.5
PATCH v2 2/4 nvme/pci: Use req_op to determine DIF remapping
Quoted 8 lines · 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(-)
Looks good,
Christoph Hellwig
maintainerPATCH v2 2/4 nvme/pci: Use req_op to determine DIF remapping
In reply to Keith BuschQuoted 4 lines · 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>
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.
PATCH v2 3/4 nvme: Make nvme user functions static
In reply to Keith BuschThese functions are used only locally in the nvme core.
--- drivers/nvme/host/core.c | 10 +++++----- drivers/nvme/host/nvme.h | 7 ------- 2 files changed, 5 insertions(+), 12 deletions(-)
drivers/nvme/host/core.c +5 -5
@@ -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) {
drivers/nvme/host/nvme.h +0 -7
@@ -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
PATCH v2 3/4 nvme: Make nvme user functions static
drivers/nvme/host/core.c
Quoted 12 lines · 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(-)drivers/nvme/host/core.c +0
-0
looks good,
PATCH v2 4/4 nvme: Use metadata for passthrough commands
In reply to Keith BuschThe 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.
--- drivers/nvme/host/core.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
drivers/nvme/host/core.c +4 -11
@@ -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 ;
Signature · 1 line
2.5.5
PATCH v2 4/4 nvme: Use metadata for passthrough commands
drivers/nvme/host/core.c
Quoted 13 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.
Signed-off-by Keith Busch <keith.busch at intel.com>--- drivers/nvme/host/core.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)drivers/nvme/host/core.c +0
-0
looks good,
Christoph Hellwig
maintainerv2 cover nvme user metadata updates
In reply to Keith BuschExcept for the messup in my patch this looks fine to me:
Sagi Grimberg
maintainerv2 cover nvme user metadata updates
In reply to Keith BuschThe series looks good to me,
v2 cover nvme user metadata updates
In reply to Keith BuschApplied to the nvme-4.14 branch with the kfree move fixed up.