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

[PATCH V2] 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().

Changes to V1:
    - Move calling nvme_free_queues() to nvme_create_io_queues() instead of
      nvme_create_queue(). (Sagi)

Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
Reviewed-by: Sagi Grimberg <sagi at grmberg.me>
---
 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..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;
+		}
 	}
 
 	/*
-- 
2.7.4

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

[Keith Busch]

On Mon, Jan 15, 2018@06:00:50AM +0900, Minwoo Im wrote:
>  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;
> +		}
>  	}

Unless this is the very first pass at initialisation, I don't think we
can free queues until after blk_mq_update_nr_hw_queues since the hctx
could otherwise point to freed memory.

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

[Minwoo Im]

On Mon, Jan 15, 2018@11:00 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Mon, Jan 15, 2018@06:00:50AM +0900, Minwoo Im wrote:
>>  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;
>> +             }
>>       }
>
> Unless this is the very first pass at initialisation, I don't think we
> can free queues until after blk_mq_update_nr_hw_queues since the hctx
> could otherwise point to freed memory.

Keith,

I agree that if not all create io and sq commands are failed,
blk_mq_update_nr_hw_queues()
will update nr_hw_queues and hw ctxs to be freed.

If it's not in the very first initialization step, however, and *all*
create io sq and cq commands
are failed in nvme_reset_work() because of controller reset (e.g. #
nvme reset /dev/nvme0),
blk_mq_update_nr_hw_queues() seems it cannot update nr_hw_queues
because this function
will not update it when it's zero.

Would it be better if request blk-mq to free all hw ctxs instead of trying
blk_mq_update_nr_hw_queues() ?

Thanks,

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

[Minwoo Im]

On Mon, Jan 15, 2018@11:00 AM, Keith Busch <keith.busch@intel.com> wrote:
> Unless this is the very first pass at initialisation, I don't think we
> can free queues until after blk_mq_update_nr_hw_queues since the hctx
> could otherwise point to freed memory.

If not in the first initial step, if (_online_queues_ < 2) driver will
kill queues and
remove namespaces. I thought this "killing queue" will handle what you concerned
about. If I misunderstand what it is, please let me know.

Otherwise if (_online_queues_ >= 2) which means that
at least one or more IO queue is prepared, blk_mq_update_nr_hw_queues()
will be triggered as you said.

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

[jianchao.wang]

Hi Minwoo 

On 01/17/2018 11:00 PM, Minwoo Im wrote:
> On Mon, Jan 15, 2018@11:00 AM, Keith Busch <keith.busch@intel.com> wrote:
>> Unless this is the very first pass at initialisation, I don't think we
>> can free queues until after blk_mq_update_nr_hw_queues since the hctx
>> could otherwise point to freed memory.
> 
> If not in the first initial step, if (_online_queues_ < 2) driver will
> kill queues and
> remove namespaces. I thought this "killing queue" will handle what you concerned
> about. If I misunderstand what it is, please let me know.
> Think of the following scenario:
nvme_reset_work
  -> nvme_setup_io_queues
    -> nvme_create_io_queues
      -> nvme_free_queues
  -> nvme_kill_queues
    -> blk_set_queue_dying   // just freeze the queue here, but will not wait to be drained.
                                not new requests come in, but maybe still residual requests in blk-mq queues.
    -> blk_mq_unquiesce_queue

the queues are _unquiesced_ here, then the residual requests will be queued
and go through nvme_queue_rq. Then the freed nvme_queue structure will be accessed.
:)

Thanks
Jianchao

> Otherwise if (_online_queues_ >= 2) which means that
> at least one or more IO queue is prepared, blk_mq_update_nr_hw_queues()
> will be triggered as you said.
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=DkKtzCStCZ1WNuxsJ2wSR-xMZ6lJWOHwGIdXYLbzPYc&s=BO4fWOEqPAS4YnfcEoj8jFyeEH68XPsseHc6Fc4PpsQ&e=
> 

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

[Minwoo Im]

On Thu, Jan 18, 2018 at 2:27 PM, jianchao.wang
<jianchao.w.wang@oracle.com> wrote:
> Hi Minwoo

>> Think of the following scenario:
> nvme_reset_work
>   -> nvme_setup_io_queues
>     -> nvme_create_io_queues
>       -> nvme_free_queues
>   -> nvme_kill_queues
>     -> blk_set_queue_dying   // just freeze the queue here, but will not wait to be drained.
>                                 not new requests come in, but maybe still residual requests in blk-mq queues.
>     -> blk_mq_unquiesce_queue
>
> the queues are _unquiesced_ here, then the residual requests will be queued
> and go through nvme_queue_rq. Then the freed nvme_queue structure will be accessed.
> :)
>
> Thanks
> Jianchao

Hi Jianchao,

First of all, I really appreciate letting me know the case.
It seems no one updates the actual nr_hw_queues value and frees hctxs
after nvme_kill_queues().
If you don't mind, would you please tell me where hctxs are freed
after nvme_kill_queues()?
I'm still trying to understand what's going on in blk-mq layer, but
I'm hardly getting to it.

Thanks,

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

[Keith Busch]

On Thu, Jan 18, 2018@07:25:06PM +0900, Minwoo Im wrote:
> On Thu, Jan 18, 2018 at 2:27 PM, jianchao.wang
> <jianchao.w.wang@oracle.com> wrote:
> > Hi Minwoo
> 
> >> Think of the following scenario:
> > nvme_reset_work
> >   -> nvme_setup_io_queues
> >     -> nvme_create_io_queues
> >       -> nvme_free_queues
> >   -> nvme_kill_queues
> >     -> blk_set_queue_dying   // just freeze the queue here, but will not wait to be drained.
> >                                 not new requests come in, but maybe still residual requests in blk-mq queues.
> >     -> blk_mq_unquiesce_queue
> >
> > the queues are _unquiesced_ here, then the residual requests will be queued
> > and go through nvme_queue_rq. Then the freed nvme_queue structure will be accessed.
> > :)
> >
> > Thanks
> > Jianchao
> 
> Hi Jianchao,
> 
> First of all, I really appreciate letting me know the case.
> It seems no one updates the actual nr_hw_queues value and frees hctxs
> after nvme_kill_queues().
> If you don't mind, would you please tell me where hctxs are freed
> after nvme_kill_queues()?

The API doesn't let us set the nr_hw_queues to 0. We'd have to free the
tagset at that point, but we don't free it until the last open reference
is dropped. I can't seem to recall why that's necessary but I'll stare
at this a bit longer see if it makes sense. The memory the driver is
holding onto is not really a big deal either way.

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

[Minwoo Im]

On Thu, Jan 18, 2018@8:31 PM, Keith Busch <keith.busch@intel.com> wrote:
>
> The API doesn't let us set the nr_hw_queues to 0. We'd have to free the
> tagset at that point, but we don't free it until the last open reference
> is dropped. I can't seem to recall why that's necessary but I'll stare
> at this a bit longer see if it makes sense. The memory the driver is
> holding onto is not really a big deal either way.

Thank you for your time, Keith.

I was wondering if I would have to clear the whole tagset or not.
But as you said, If the memory driver holds is not really important
and no one is
referring it then we can skip it.