[PATCH 1/2] nvme-loop: retrieve iod from the cqe command_id

[PATCH 1/2] nvme-loop: retrieve iod from the cqe command_id

[Sagi Grimberg]

useful to validate that the we didn't mess up
the command_id.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/loop.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index d1f06e7768ff..8e19bf48691c 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -113,11 +113,20 @@ static void nvme_loop_complete_rq(struct request *req)
 	blk_mq_end_request(req, error);
 }
 
+static struct blk_mq_tags *nvme_loop_tagset(struct nvme_loop_queue *queue)
+{
+	u32 queue_idx = nvme_loop_queue_idx(queue);
+
+	if (queue_idx == 0)
+		return queue->ctrl->admin_tag_set.tags[queue_idx];
+	return queue->ctrl->tag_set.tags[queue_idx - 1];
+}
+
 static void nvme_loop_queue_response(struct nvmet_req *req)
 {
-	struct nvme_loop_iod *iod =
-		container_of(req, struct nvme_loop_iod, req);
-	struct nvme_completion *cqe = &iod->rsp;
+	struct nvme_loop_queue *queue =
+		container_of(req->sq, struct nvme_loop_queue, nvme_sq);
+	struct nvme_completion *cqe = req->rsp;
 
 	/*
 	 * AEN requests are special as they don't time out and can
@@ -125,13 +134,23 @@ static void nvme_loop_queue_response(struct nvmet_req *req)
 	 * aborts.  We don't even bother to allocate a struct request
 	 * for them but rather special case them here.
 	 */
-	if (unlikely(nvme_loop_queue_idx(iod->queue) == 0 &&
+	if (unlikely(nvme_loop_queue_idx(queue) == 0 &&
 			cqe->command_id >= NVME_LOOP_AQ_BLKMQ_DEPTH)) {
-		nvme_complete_async_event(&iod->queue->ctrl->ctrl, cqe->status,
+		nvme_complete_async_event(&queue->ctrl->ctrl, cqe->status,
 				&cqe->result);
 	} else {
-		struct request *rq = blk_mq_rq_from_pdu(iod);
+		struct request *rq;
+		struct nvme_loop_iod *iod;
+
+		rq = blk_mq_tag_to_rq(nvme_loop_tagset(queue), cqe->command_id);
+		if (!rq) {
+			dev_err(queue->ctrl->ctrl.device,
+				"tag 0x%x on queue %d not found\n",
+				cqe->command_id, nvme_loop_queue_idx(queue));
+			return;
+		}
 
+		iod = blk_mq_rq_to_pdu(rq);
 		iod->nvme_req.result = cqe->result;
 		blk_mq_complete_request(rq, le16_to_cpu(cqe->status) >> 1);
 	}
-- 
2.7.4

[PATCH 2/2] nvme-loop: fix a possible use-after-free when destroying the admin queue

[Sagi Grimberg]

we need to destroy the nvmet sq and let it finish gracefully
before continue to cleanup the queue.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 8e19bf48691c..2fc750010e93 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -307,9 +307,9 @@ static struct blk_mq_ops nvme_loop_admin_mq_ops = {
 
 static void nvme_loop_destroy_admin_queue(struct nvme_loop_ctrl *ctrl)
 {
+	nvmet_sq_destroy(&ctrl->queues[0].nvme_sq);
 	blk_cleanup_queue(ctrl->ctrl.admin_q);
 	blk_mq_free_tag_set(&ctrl->admin_tag_set);
-	nvmet_sq_destroy(&ctrl->queues[0].nvme_sq);
 }
 
 static void nvme_loop_free_ctrl(struct nvme_ctrl *nctrl)
-- 
2.7.4

[PATCH 1/2] nvme-loop: retrieve iod from the cqe command_id

[Christoph Hellwig]

On Mon, Feb 27, 2017@07:00:19PM +0200, Sagi Grimberg wrote:
> useful to validate that the we didn't mess up
> the command_id.

But also less efficient, so I don't really see a good reason for it.

[PATCH 2/2] nvme-loop: fix a possible use-after-free when destroying the admin queue

[Christoph Hellwig]

Looks fine,

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

[PATCH 1/2] nvme-loop: retrieve iod from the cqe command_id

[Sagi Grimberg]

>> useful to validate that the we didn't mess up
>> the command_id.
>
> But also less efficient, so I don't really see a good reason for it.

Less efficient? I highly doubt this will ever make any noticeable
difference...

I changed that because the blk-mq scheduler facilities broke nvmf and
it took me a while to figure out why loop doesn't break like rdma, but
I really don't see why we do something different from all other
transports, especially since its main usage is testing...

I can drop it if you insist against it but I sorta disagree with
you.