* [PATCH 1/3] nvme: introduce nvme_finish_cmd function
@ 2019-09-02 15:01 Max Gurtovoy
2019-09-02 15:01 ` [PATCH 2/3] nvmet-loop: use nvme_finish_cmd during complete callback Max Gurtovoy
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Max Gurtovoy @ 2019-09-02 15:01 UTC (permalink / raw)
To: linux-nvme, keith.busch, hch, james.smart, sagi; +Cc: israelr, Max Gurtovoy
This function should be called by every transport during complete callback
upon successful submission of the request. Thus, in case of a failure in
command submission and in case nvme_setup_cmd was called, the transport
should call nvme_cleanup_cmd in the error flow to avoid resource leakage.
Add this logic to all the underlaying transports.
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
drivers/nvme/host/core.c | 20 +++++++++++++-------
drivers/nvme/host/fc.c | 4 ++--
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/pci.c | 2 +-
drivers/nvme/host/rdma.c | 12 ++++++------
drivers/nvme/host/tcp.c | 11 +++++++++--
6 files changed, 32 insertions(+), 18 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d3d6b7b..9409427 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -683,13 +683,6 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
void nvme_cleanup_cmd(struct request *req)
{
- if (blk_integrity_rq(req) && req_op(req) == REQ_OP_READ &&
- nvme_req(req)->status == 0) {
- struct nvme_ns *ns = req->rq_disk->private_data;
-
- t10_pi_complete(req, ns->pi_type,
- blk_rq_bytes(req) >> ns->lba_shift);
- }
if (req->rq_flags & RQF_SPECIAL_PAYLOAD) {
struct nvme_ns *ns = req->rq_disk->private_data;
struct page *page = req->special_vec.bv_page;
@@ -702,6 +695,19 @@ void nvme_cleanup_cmd(struct request *req)
}
EXPORT_SYMBOL_GPL(nvme_cleanup_cmd);
+void nvme_finish_cmd(struct request *req)
+{
+ if (blk_integrity_rq(req) && req_op(req) == REQ_OP_READ &&
+ nvme_req(req)->status == 0) {
+ struct nvme_ns *ns = req->rq_disk->private_data;
+
+ t10_pi_complete(req, ns->pi_type,
+ blk_rq_bytes(req) >> ns->lba_shift);
+ }
+ nvme_cleanup_cmd(req);
+}
+EXPORT_SYMBOL_GPL(nvme_finish_cmd);
+
blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
struct nvme_command *cmd)
{
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 232d8094..dd7fbe2 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2152,8 +2152,6 @@ enum {
((rq_data_dir(rq) == WRITE) ?
DMA_TO_DEVICE : DMA_FROM_DEVICE));
- nvme_cleanup_cmd(rq);
-
sg_free_table_chained(&freq->sg_table, SG_CHUNK_SIZE);
freq->sg_cnt = 0;
@@ -2284,6 +2282,7 @@ enum {
if (!(op->flags & FCOP_FLAGS_AEN))
nvme_fc_unmap_data(ctrl, op->rq, op);
+ nvme_cleanup_cmd(op->rq);
nvme_fc_ctrl_put(ctrl);
if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE &&
@@ -2376,6 +2375,7 @@ enum {
atomic_set(&op->state, FCPOP_STATE_IDLE);
nvme_fc_unmap_data(ctrl, rq, op);
+ nvme_finish_cmd(rq);
nvme_complete_rq(rq);
nvme_fc_ctrl_put(ctrl);
}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 2d678fb..11b2f52 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -463,6 +463,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
struct request *nvme_alloc_request(struct request_queue *q,
struct nvme_command *cmd, blk_mq_req_flags_t flags, int qid);
void nvme_cleanup_cmd(struct request *req);
+void nvme_finish_cmd(struct request *req);
blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
struct nvme_command *cmd);
int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 732d5b6..ab3b1c9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -920,7 +920,7 @@ static void nvme_pci_complete_rq(struct request *req)
struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
struct nvme_dev *dev = iod->nvmeq->dev;
- nvme_cleanup_cmd(req);
+ nvme_finish_cmd(req);
if (blk_integrity_rq(req))
dma_unmap_page(dev->dev, iod->meta_dma,
rq_integrity_vec(req)->bv_len, rq_data_dir(req));
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 1a6449b..cd94f8a 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1149,7 +1149,6 @@ static void nvme_rdma_unmap_data(struct nvme_rdma_queue *queue,
req->nents, rq_data_dir(rq) ==
WRITE ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
- nvme_cleanup_cmd(rq);
sg_free_table_chained(&req->sg_table, SG_CHUNK_SIZE);
}
@@ -1748,7 +1747,6 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
if (unlikely(err < 0)) {
dev_err(queue->ctrl->ctrl.device,
"Failed to map data (%d)\n", err);
- nvme_cleanup_cmd(rq);
goto err;
}
@@ -1759,18 +1757,19 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
err = nvme_rdma_post_send(queue, sqe, req->sge, req->num_sge,
req->mr ? &req->reg_wr.wr : NULL);
- if (unlikely(err)) {
- nvme_rdma_unmap_data(queue, rq);
- goto err;
- }
+ if (unlikely(err))
+ goto err_unmap;
return BLK_STS_OK;
+err_unmap:
+ nvme_rdma_unmap_data(queue, rq);
err:
if (err == -ENOMEM || err == -EAGAIN)
ret = BLK_STS_RESOURCE;
else
ret = BLK_STS_IOERR;
+ nvme_cleanup_cmd(rq);
unmap_qe:
ib_dma_unmap_single(dev, req->sqe.dma, sizeof(struct nvme_command),
DMA_TO_DEVICE);
@@ -1791,6 +1790,7 @@ static void nvme_rdma_complete_rq(struct request *rq)
struct ib_device *ibdev = queue->device->dev;
nvme_rdma_unmap_data(queue, rq);
+ nvme_finish_cmd(rq);
ib_dma_unmap_single(ibdev, req->sqe.dma, sizeof(struct nvme_command),
DMA_TO_DEVICE);
nvme_complete_rq(rq);
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 606b13d..bd00128 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2093,6 +2093,7 @@ static blk_status_t nvme_tcp_setup_cmd_pdu(struct nvme_ns *ns,
ret = nvme_tcp_map_data(queue, rq);
if (unlikely(ret)) {
+ nvme_cleanup_cmd(rq);
dev_err(queue->ctrl->ctrl.device,
"Failed to map data (%d)\n", ret);
return ret;
@@ -2101,6 +2102,12 @@ static blk_status_t nvme_tcp_setup_cmd_pdu(struct nvme_ns *ns,
return 0;
}
+static void nvme_tcp_complete_rq(struct request *rq)
+{
+ nvme_finish_cmd(rq);
+ nvme_complete_rq(rq);
+}
+
static blk_status_t nvme_tcp_queue_rq(struct blk_mq_hw_ctx *hctx,
const struct blk_mq_queue_data *bd)
{
@@ -2161,7 +2168,7 @@ static int nvme_tcp_map_queues(struct blk_mq_tag_set *set)
static struct blk_mq_ops nvme_tcp_mq_ops = {
.queue_rq = nvme_tcp_queue_rq,
- .complete = nvme_complete_rq,
+ .complete = nvme_tcp_complete_rq,
.init_request = nvme_tcp_init_request,
.exit_request = nvme_tcp_exit_request,
.init_hctx = nvme_tcp_init_hctx,
@@ -2171,7 +2178,7 @@ static int nvme_tcp_map_queues(struct blk_mq_tag_set *set)
static struct blk_mq_ops nvme_tcp_admin_mq_ops = {
.queue_rq = nvme_tcp_queue_rq,
- .complete = nvme_complete_rq,
+ .complete = nvme_tcp_complete_rq,
.init_request = nvme_tcp_init_request,
.exit_request = nvme_tcp_exit_request,
.init_hctx = nvme_tcp_init_admin_hctx,
--
1.8.3.1
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] nvmet-loop: use nvme_finish_cmd during complete callback
2019-09-02 15:01 [PATCH 1/3] nvme: introduce nvme_finish_cmd function Max Gurtovoy
@ 2019-09-02 15:01 ` Max Gurtovoy
2019-09-02 15:14 ` Christoph Hellwig
2019-09-02 15:01 ` [PATCH 3/3] nvme-rdma: avoid un-needed dereference of request sqe Max Gurtovoy
2019-09-02 15:13 ` [PATCH 1/3] nvme: introduce nvme_finish_cmd function Christoph Hellwig
2 siblings, 1 reply; 8+ messages in thread
From: Max Gurtovoy @ 2019-09-02 15:01 UTC (permalink / raw)
To: linux-nvme, keith.busch, hch, james.smart, sagi; +Cc: israelr, Max Gurtovoy
Also fix possible leakage during error flow in nvme_loop_queue_rq callback.
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
drivers/nvme/target/loop.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 0940c50..33bc6c3 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -76,7 +76,7 @@ static void nvme_loop_complete_rq(struct request *req)
{
struct nvme_loop_iod *iod = blk_mq_rq_to_pdu(req);
- nvme_cleanup_cmd(req);
+ nvme_finish_cmd(req);
sg_free_table_chained(&iod->sg_table, SG_CHUNK_SIZE);
nvme_complete_rq(req);
}
@@ -157,8 +157,10 @@ static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
iod->sg_table.sgl = iod->first_sgl;
if (sg_alloc_table_chained(&iod->sg_table,
blk_rq_nr_phys_segments(req),
- iod->sg_table.sgl, SG_CHUNK_SIZE))
+ iod->sg_table.sgl, SG_CHUNK_SIZE)) {
+ nvme_cleanup_cmd(req);
return BLK_STS_RESOURCE;
+ }
iod->req.sg = iod->sg_table.sgl;
iod->req.sg_cnt = blk_rq_map_sg(req->q, req, iod->sg_table.sgl);
--
1.8.3.1
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] nvme-rdma: avoid un-needed dereference of request sqe
2019-09-02 15:01 [PATCH 1/3] nvme: introduce nvme_finish_cmd function Max Gurtovoy
2019-09-02 15:01 ` [PATCH 2/3] nvmet-loop: use nvme_finish_cmd during complete callback Max Gurtovoy
@ 2019-09-02 15:01 ` Max Gurtovoy
2019-09-02 15:14 ` Christoph Hellwig
2019-09-02 15:13 ` [PATCH 1/3] nvme: introduce nvme_finish_cmd function Christoph Hellwig
2 siblings, 1 reply; 8+ messages in thread
From: Max Gurtovoy @ 2019-09-02 15:01 UTC (permalink / raw)
To: linux-nvme, keith.busch, hch, james.smart, sagi; +Cc: israelr, Max Gurtovoy
Instead, use the local variable that was set previously.
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
drivers/nvme/host/rdma.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index cd94f8a..458b5f4 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1727,10 +1727,10 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
dev = queue->device->dev;
- req->sqe.dma = ib_dma_map_single(dev, req->sqe.data,
- sizeof(struct nvme_command),
- DMA_TO_DEVICE);
- err = ib_dma_mapping_error(dev, req->sqe.dma);
+ sqe->dma = ib_dma_map_single(dev, sqe->data,
+ sizeof(struct nvme_command),
+ DMA_TO_DEVICE);
+ err = ib_dma_mapping_error(dev, sqe->dma);
if (unlikely(err))
return BLK_STS_RESOURCE;
@@ -1771,7 +1771,7 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
ret = BLK_STS_IOERR;
nvme_cleanup_cmd(rq);
unmap_qe:
- ib_dma_unmap_single(dev, req->sqe.dma, sizeof(struct nvme_command),
+ ib_dma_unmap_single(dev, sqe->dma, sizeof(struct nvme_command),
DMA_TO_DEVICE);
return ret;
}
--
1.8.3.1
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] nvme: introduce nvme_finish_cmd function
2019-09-02 15:01 [PATCH 1/3] nvme: introduce nvme_finish_cmd function Max Gurtovoy
2019-09-02 15:01 ` [PATCH 2/3] nvmet-loop: use nvme_finish_cmd during complete callback Max Gurtovoy
2019-09-02 15:01 ` [PATCH 3/3] nvme-rdma: avoid un-needed dereference of request sqe Max Gurtovoy
@ 2019-09-02 15:13 ` Christoph Hellwig
2019-09-02 15:42 ` Max Gurtovoy
2 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2019-09-02 15:13 UTC (permalink / raw)
To: Max Gurtovoy
Cc: sagi, martin.petersen, israelr, james.smart, linux-nvme,
keith.busch, hch
On Mon, Sep 02, 2019 at 06:01:00PM +0300, Max Gurtovoy wrote:
> This function should be called by every transport during complete callback
> upon successful submission of the request. Thus, in case of a failure in
> command submission and in case nvme_setup_cmd was called, the transport
> should call nvme_cleanup_cmd in the error flow to avoid resource leakage.
> Add this logic to all the underlaying transports.
The idea looks good, but I'm a little worried about the confusion
the two functions create. I remember we once had the idea of lifting
the pi_type into the core block layer, in which case we could do
the t10_pi_complete call inside blk_mq_end_request instead of having
drivers deal with it. Same for the t10_pi_prepare call on the
submissions side.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] nvmet-loop: use nvme_finish_cmd during complete callback
2019-09-02 15:01 ` [PATCH 2/3] nvmet-loop: use nvme_finish_cmd during complete callback Max Gurtovoy
@ 2019-09-02 15:14 ` Christoph Hellwig
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2019-09-02 15:14 UTC (permalink / raw)
To: Max Gurtovoy; +Cc: sagi, israelr, james.smart, linux-nvme, keith.busch, hch
> @@ -157,8 +157,10 @@ static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
> iod->sg_table.sgl = iod->first_sgl;
> if (sg_alloc_table_chained(&iod->sg_table,
> blk_rq_nr_phys_segments(req),
> - iod->sg_table.sgl, SG_CHUNK_SIZE))
> + iod->sg_table.sgl, SG_CHUNK_SIZE)) {
> + nvme_cleanup_cmd(req);
> return BLK_STS_RESOURCE;
> + }
Please submit this as a standalone backportable fix.
The other hunk should go into your v1 if we stick to the separate
functions.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] nvme-rdma: avoid un-needed dereference of request sqe
2019-09-02 15:01 ` [PATCH 3/3] nvme-rdma: avoid un-needed dereference of request sqe Max Gurtovoy
@ 2019-09-02 15:14 ` Christoph Hellwig
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2019-09-02 15:14 UTC (permalink / raw)
To: Max Gurtovoy; +Cc: sagi, israelr, james.smart, linux-nvme, keith.busch, hch
On Mon, Sep 02, 2019 at 06:01:02PM +0300, Max Gurtovoy wrote:
> Instead, use the local variable that was set previously.
>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] nvme: introduce nvme_finish_cmd function
2019-09-02 15:13 ` [PATCH 1/3] nvme: introduce nvme_finish_cmd function Christoph Hellwig
@ 2019-09-02 15:42 ` Max Gurtovoy
2019-09-02 15:47 ` Christoph Hellwig
0 siblings, 1 reply; 8+ messages in thread
From: Max Gurtovoy @ 2019-09-02 15:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: sagi, martin.petersen, israelr, james.smart, linux-nvme, keith.busch
On 9/2/2019 6:13 PM, Christoph Hellwig wrote:
> On Mon, Sep 02, 2019 at 06:01:00PM +0300, Max Gurtovoy wrote:
>> This function should be called by every transport during complete callback
>> upon successful submission of the request. Thus, in case of a failure in
>> command submission and in case nvme_setup_cmd was called, the transport
>> should call nvme_cleanup_cmd in the error flow to avoid resource leakage.
>> Add this logic to all the underlaying transports.
> The idea looks good, but I'm a little worried about the confusion
> the two functions create. I remember we once had the idea of lifting
> the pi_type into the core block layer, in which case we could do
> the t10_pi_complete call inside blk_mq_end_request instead of having
> drivers deal with it. Same for the t10_pi_prepare call on the
> submissions side.
I can check this out.
So scsi layer always use blk_mq now ? no legacy IO path possible ?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] nvme: introduce nvme_finish_cmd function
2019-09-02 15:42 ` Max Gurtovoy
@ 2019-09-02 15:47 ` Christoph Hellwig
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2019-09-02 15:47 UTC (permalink / raw)
To: Max Gurtovoy
Cc: sagi, martin.petersen, israelr, james.smart, linux-nvme,
keith.busch, Christoph Hellwig
On Mon, Sep 02, 2019 at 06:42:43PM +0300, Max Gurtovoy wrote:
> I can check this out.
>
> So scsi layer always use blk_mq now ? no legacy IO path possible ?
Yes, the legacy I/O path has been gone for a few releases.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-02 15:47 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-02 15:01 [PATCH 1/3] nvme: introduce nvme_finish_cmd function Max Gurtovoy
2019-09-02 15:01 ` [PATCH 2/3] nvmet-loop: use nvme_finish_cmd during complete callback Max Gurtovoy
2019-09-02 15:14 ` Christoph Hellwig
2019-09-02 15:01 ` [PATCH 3/3] nvme-rdma: avoid un-needed dereference of request sqe Max Gurtovoy
2019-09-02 15:14 ` Christoph Hellwig
2019-09-02 15:13 ` [PATCH 1/3] nvme: introduce nvme_finish_cmd function Christoph Hellwig
2019-09-02 15:42 ` Max Gurtovoy
2019-09-02 15:47 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).