Re: [PATCH V3] NVMe: basic conversion to blk-mq

[Jens Axboe <axboe@kernel.dk>]

[-- Attachment #1: Type: text/plain, Size: 1350 bytes --]

On 05/29/2014 08:25 AM, Jens Axboe wrote:
>>> +static int nvme_submit_flush_sync(struct nvme_queue *nvmeq, struct
>>> nvme_ns *ns)
>>> +{
>>> +    struct request *req;
>>> +    struct nvme_command cmnd;
>>> +
>>> +    req = blk_mq_alloc_request(ns->queue, WRITE, GFP_KERNEL, false);
>>> +    if (!req)
>>> +        return -ENOMEM;
>>> +
>>> +    nvme_setup_flush(&cmnd, ns, req->tag);
>>> +    nvme_submit_sync_cmd(req, &cmnd, NULL, NVME_IO_TIMEOUT);
>>>
>>>     return 0;
>>> }
>>
>> It looks like this function above is being called from an interrupt
>> context where we are already holding a spinlock. The sync command will
>> try to take that same lock.
> 
> Yes, that code still looks very buggy. The initial alloc for
> flush_cmd_info should also retry, not fail hard, if that alloc fails.
> For the reinsert part, Matias, you want to look at the flush code in
> blk-mq and how that handles it.

There's an easy fix for this. Once it's managed by blk-mq, blk-mq will
decompose requests for you. This means a flush with data will be turned
into two commands for you, so we can kill this code attempting to handle
flush request with data.

Patch attached. Depending on how the series needs to look, the prep
patch of support bio flush with data should just be dropped however. No
point in adding that, and the removing it again.

-- 
Jens Axboe


[-- Attachment #2: nvme-flush.patch --]
[-- Type: text/x-patch, Size: 2178 bytes --]

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index ac695b336a98..23bd58dfa360 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -513,35 +513,11 @@ static void nvme_submit_flush(struct nvme_queue *nvmeq, struct nvme_ns *ns,
 	writel(nvmeq->sq_tail, nvmeq->q_db);
 }
 
-static int nvme_submit_flush_sync(struct nvme_queue *nvmeq, struct nvme_ns *ns)
-{
-	struct request *req;
-	struct nvme_command cmnd;
-
-	req = blk_mq_alloc_request(ns->queue, WRITE, GFP_KERNEL, false);
-	if (!req)
-		return -ENOMEM;
-
-	nvme_setup_flush(&cmnd, ns, req->tag);
-	nvme_submit_sync_cmd(req, &cmnd, NULL, NVME_IO_TIMEOUT);
-
-	return 0;
-}
-
 struct flush_cmd_info {
 	struct nvme_ns *ns;
 	struct nvme_iod *iod;
 };
 
-static void req_flush_completion(struct nvme_queue *nvmeq, void *ctx,
-						struct nvme_completion *cqe)
-{
-	struct flush_cmd_info *flush_cmd = ctx;
-	nvme_submit_flush_sync(nvmeq, flush_cmd->ns);
-	req_completion(nvmeq, flush_cmd->iod, cqe);
-	kfree(flush_cmd);
-}
-
 static int nvme_submit_iod(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 							struct nvme_ns *ns)
 {
@@ -560,7 +536,7 @@ static int nvme_submit_iod(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 		nvme_submit_discard(nvmeq, ns, req, iod);
 		goto end_submit;
 	}
-	if (req->cmd_flags & REQ_FLUSH && !iod->nents) {
+	if (req->cmd_flags & REQ_FLUSH) {
 		nvme_submit_flush(nvmeq, ns, req->tag);
 		goto end_submit;
 	}
@@ -615,16 +591,6 @@ static int nvme_submit_req_queue(struct nvme_queue *nvmeq, struct nvme_ns *ns,
 
 	nvme_set_info(cmd, iod, req_completion);
 
-	if ((req->cmd_flags & REQ_FLUSH) && psegs) {
-		struct flush_cmd_info *flush_cmd = kmalloc(
-				sizeof(struct flush_cmd_info), GFP_KERNEL);
-		if (!flush_cmd)
-			goto free_iod;
-		flush_cmd->ns = ns;
-		flush_cmd->iod = iod;
-		nvme_set_info(cmd, flush_cmd, req_flush_completion);
-	}
-
 	if (req->cmd_flags & REQ_DISCARD) {
 		void *range;
 		/*
@@ -655,7 +621,6 @@ static int nvme_submit_req_queue(struct nvme_queue *nvmeq, struct nvme_ns *ns,
 
  finish_cmd:
 	nvme_finish_cmd(nvmeq, req->tag, NULL);
- free_iod:
 	nvme_free_iod(nvmeq->dev, iod);
 	return BLK_MQ_RQ_QUEUE_ERROR;
 }