[PATCH 2/2] nvme-rdma: move admin queue cleanup to nvme_rdma_free_ctrl

[J Freyensee]

On Wed, 2016-07-13 at 16:36 -0700, Ming Lin wrote:
> On Wed, Jul 13, 2016 at 4:19 PM, J Freyensee
> <james_p_freyensee at linux.intel.com> wrote:
> > 
> > >  static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
> > > @@ -687,6 +684,10 @@ static void nvme_rdma_free_ctrl(struct
> > > nvme_ctrl
> > > *nctrl)
> > >       list_del(&ctrl->list);
> > >       mutex_unlock(&nvme_rdma_ctrl_mutex);
> > > 
> > > +     blk_cleanup_queue(ctrl->ctrl.admin_q);
> > > +     blk_mq_free_tag_set(&ctrl->admin_tag_set);
> > > +     nvme_rdma_dev_put(ctrl->device);
> > > +
> > >       if (ctrl->ctrl.tagset) {
> > >               blk_cleanup_queue(ctrl->ctrl.connect_q);
> > >               blk_mq_free_tag_set(&ctrl->tag_set);
> > 
> > This patch does not remove the second
> > 
> > nvme_rdma_dev_put(ctrl->device);
> > 
> > call that happens within the if() statement above if it evaluates
> > to
> > TRUE.  Should that have been removed or moved elsewhere?
> 
> Not sure I understand your question.
> Did you mean line 694?

Yes I mean line 694.

> 
> For discovery controller, there is no IO queues. So ctrl->ctrl.tagset
> is NULL.
> 
> The first bulk of
> "blk_cleanup_queue/blk_mq_free_tag_set/nvme_rdma_dev_put" is for
> admin
> queue.
> And the second is for IO queues.

I'm just confused when nvme_free_ctrl() in core.c calls:

ctrl->ops->free_ctrl(ctrl);

which looks like would be the only call that would free both the admin
and I/O rdma queues, why there would be the potential to do a _put()
twice in nvme_rdma_free_ctrl() via:

nvme_rdma_dev_put(ctrl->device);

one for the admin section:

687         blk_cleanup_queue(ctrl>ctrl.admin_q);
688         blk_mq_free_tag_set(&ctrl->admin_tag_set);
689         nvme_rdma_dev_put(ctrl->device);

and one for the I/O section (assuming "if (ctrl->ctrl.tagset)" evaluate
s to TRUE in that call):

691         if (ctrl->ctrl.tagset) {
692                 blk_cleanup_queue(ctrl->ctrl.connect_q);
693                 blk_mq_free_tag_set(&ctrl->tag_set);
694                 nvme_rdma_dev_put(ctrl->device);
695         }


My assumption would be that the correct path for this case would be
such that 

nvme_rdma_dev_put(ctrl->device); 

would only be called one time, for a single device.