[PATCH] nvme: free pre-allocated queue if create ioq goes wrong

[PATCH] nvme: free pre-allocated queue if create ioq goes wrong

[Minwoo Im]

If either create-sq or create-cq is failed with an NVMe status,
nvme_init_queue() is invoked which was not expected to be called in
nvme_create_queue().
To prevent this inconsistency of queue_count, free queue(s) already
allocated in nvme_create_io_queues() when it goes wrong in
nvme_create_queue().

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 drivers/nvme/host/pci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d53550e..a58282d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1456,11 +1456,11 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 
 	nvmeq->cq_vector = qid - 1;
 	result = adapter_alloc_cq(dev, qid, nvmeq);
-	if (result < 0)
-		return result;
+	if (result)
+		goto free_queue;
 
 	result = adapter_alloc_sq(dev, qid, nvmeq);
-	if (result < 0)
+	if (result)
 		goto release_cq;
 
 	nvme_init_queue(nvmeq, qid);
@@ -1474,9 +1474,10 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	adapter_delete_sq(dev, qid);
  release_cq:
 	adapter_delete_cq(dev, qid);
+ free_queue:
+	nvme_free_queues(dev, dev->online_queues);
 	return result;
 }
-
 static const struct blk_mq_ops nvme_mq_admin_ops = {
 	.queue_rq	= nvme_queue_rq,
 	.complete	= nvme_pci_complete_rq,
-- 
2.7.4

[PATCH] nvme: free pre-allocated queue if create ioq goes wrong

[Sagi Grimberg]

Minwoo,

> If either create-sq or create-cq is failed with an NVMe status,
> nvme_init_queue() is invoked which was not expected to be called in
> nvme_create_queue().
> To prevent this inconsistency of queue_count, free queue(s) already
> allocated in nvme_create_io_queues() when it goes wrong in
> nvme_create_queue().

The fix looks good, but I think it would be better to cleanup the queue
in the call site where nvme_alloc_queue is called, having it here is a
bit awkward IMO.

[PATCH] nvme: free pre-allocated queue if create ioq goes wrong

[Minwoo Im]

Sagi,

On Sun, Jan 14, 2018@6:18 PM, Sagi Grimberg <sagi@grimberg.me> wrote:
> Minwoo,
>
>> If either create-sq or create-cq is failed with an NVMe status,
>> nvme_init_queue() is invoked which was not expected to be called in
>> nvme_create_queue().
>> To prevent this inconsistency of queue_count, free queue(s) already
>> allocated in nvme_create_io_queues() when it goes wrong in
>> nvme_create_queue().
>
>
> The fix looks good, but I think it would be better to cleanup the queue
> in the call site where nvme_alloc_queue is called, having it here is a
> bit awkward IMO.

Thanks for your kind review.

I agree with that allocation and freeing should be in the same place
because it looks much more consistent in code.

What about below code?

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d53550e..ecef45a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1456,11 +1456,11 @@ static int nvme_create_queue(struct nvme_queue
*nvmeq, int qid)

        nvmeq->cq_vector = qid - 1;
        result = adapter_alloc_cq(dev, qid, nvmeq);
-       if (result < 0)
+       if (result)
                return result;

        result = adapter_alloc_sq(dev, qid, nvmeq);
-       if (result < 0)
+       if (result)
                goto release_cq;

        nvme_init_queue(nvmeq, qid);
@@ -1476,7 +1476,6 @@ static int nvme_create_queue(struct nvme_queue
*nvmeq, int qid)
        adapter_delete_cq(dev, qid);
        return result;
 }
-
 static const struct blk_mq_ops nvme_mq_admin_ops = {
        .queue_rq       = nvme_queue_rq,
        .complete       = nvme_pci_complete_rq,
@@ -1637,8 +1636,10 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
        max = min(dev->max_qid, dev->ctrl.queue_count - 1);
        for (i = dev->online_queues; i <= max; i++) {
                ret = nvme_create_queue(dev->queues[i], i);
-               if (ret)
+               if (ret) {
+                       nvme_free_queues(dev, dev->online_queues);
                        break;
+               }
        }

        /*

[PATCH] nvme: free pre-allocated queue if create ioq goes wrong

[Sagi Grimberg]

On 01/14/2018 11:41 AM, Minwoo Im wrote:
> Sagi,
> 
> On Sun, Jan 14, 2018@6:18 PM, Sagi Grimberg <sagi@grimberg.me> wrote:
>> Minwoo,
>>
>>> If either create-sq or create-cq is failed with an NVMe status,
>>> nvme_init_queue() is invoked which was not expected to be called in
>>> nvme_create_queue().
>>> To prevent this inconsistency of queue_count, free queue(s) already
>>> allocated in nvme_create_io_queues() when it goes wrong in
>>> nvme_create_queue().
>>
>>
>> The fix looks good, but I think it would be better to cleanup the queue
>> in the call site where nvme_alloc_queue is called, having it here is a
>> bit awkward IMO.
> 
> Thanks for your kind review.
> 
> I agree with that allocation and freeing should be in the same place
> because it looks much more consistent in code.
> 
> What about below code?

Looks better, please send a proper patch, thanks.