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

[swise@opengridcomputing.com (Steve Wise)]

> > > > Hey Sagi, here is some lite reading for you. :)
> > > >
> > > > Prelude:  As part of disconnecting an iwarp connection, the iwarp
provider
> > needs
> > > > to post an IW_CM_EVENT_CLOSE event to iw_cm, which is scheduled onto the
> > > > singlethread workq thread for iw_cm.
> > > >
> > > > Here is what happens with Sagi's patch:
> > > >
> > > > nvme_rdma_device_unplug() calls nvme_rdma_stop_queue() which calls
> > > > rdma_disconnect().  This triggers the disconnect.  iw_cxgb4 posts the
> > > > IW_CM_EVENT_CLOSE to iw_cm, which ends up calling cm_close_handler() in
> > the
> > > > iw_cm workq thread context.  cm_close_handler() calls the rdma_cm event
> > > handler
> > > > for this cm_id, function cm_iw_handler(), which blocks until any
currently
> > > > running event handler for this cm_id finishes.  It does this by calling
> > > > cm_disable_callback().  However since this whole unplug process is
running
> > in
> > > > the event handler function for this same cm_id, the iw_cm workq thread
is
> > now
> > > > stuck in a deadlock.   nvme_rdma_device_unplug() however, continues on
and
> > > > schedules the controller delete worker thread and waits for it to
> complete.
> > The
> > > > delete controller worker thread tries to disconnect and destroy all the
> > > > remaining IO queues, but gets stuck in the destroy() path on the first
IO
> > queue
> > > > because the iw_cm workq thread is already stuck, and processing the
CLOSE
> > > event
> > > > is required to release a reference the iw_cm has on the iwarp providers
> qp.
> > So
> > > > everything comes to a grinding halt....
> > > >
> > > > Now: Ming's 2 patches avoid this deadlock because the cm_id that
received
> > the
> > > > device removal event is disconnected/destroyed _only after_ all the
> > controller
> > > > queues are disconnected/destroyed.  So nvme_rdma_device_unplug() doesn't
> > get
> > > > stuck waiting for the controller to delete the io queues, and only after
> > that
> > > > completes, does it delete the cm_id/qp that got the device removal
event.
> > It
> > > > then returns thus causing the rdma_cm to release the cm_id's callback
> mutex.
> > > > This causes the iw_cm workq thread to now unblock and we continue on.
> (can
> > > you
> > > > say house of cards?)
> > > >
> > > > So the net is:  the cm_id that received the device remove event _must_
be
> > > > disconnect/destroyed _last_.
> > >
> > > Hey Steve, thanks for the detailed description.
> > >
> > > IMHO, this really looks like a buggy design in iWARP connection
> > > management implementation. The fact that a rdma_disconnect forward
> > > progress is dependent on other cm_id execution looks wrong and
> > > backwards to me.
> > >
> >
> > Yea.  I'm not sure how to fix it at this point...
> >
> > > Given that the device is being removed altogether I don't think that
> > > calling rdma_disconnect is important at all. Given the fact that
> > > ib_drain_qp makes sure that the queue-pair is in error state does
> > > this incremental patch resolve the iwarp-deadlock you are seeing?
> > >
> >
> > I'll need to change c4iw_drain_sq/rq() to move the QP to ERROR explicitly.
> When
> > I implemented them I assumed the QP would be disconnected.  Let me try this
> and
> > report back.
> 
> Just doing the drain has the same issue because moving the QP to ERROR does
the
> same process as a disconnect wrt the provider/IWCM interactions.
> 
> The reason for this close provider/iwcm interaction is needed is due to the
> IWARP relationship between QPs and the TCP connection (and thus the cm_id).
The
> iwarp driver stack manipulates the QP state based on the connection events
> directly. For example, the transition from IDLE -> RTS is done _only_ by the
> iwarp driver stack.  Similarly, if the peer closes the TCP connection (or
> aborts), then the local iwarp driver stack will move the QP out of RTS into
> CLOSING or ERROR.  I think this is different from IB and the IBCM.  This in
> connection with the IWCM maintaining a reference on the QP until the iwarp
> driver posts a CLOSED event for its iw_cm_id is causing the problem here.  I'm
> not justifying the existing logic, just trying to explain.
> 
> Perhaps we can explore changing the iw_cm and the iWARP parts of the rdma_cm
> to
> fix this problem...  I guess the main problem is that qp disconnect and
destroy
> basically cannot be done in the event handler.  Granted it works if you only
> destroy the qp associated with the cm_id handling the event, but that still
> causes a deadlock, its just that the event handler can return and thus unblock
> the deadlock.

I think I can fix this in iw_cxgb4.  Stay tuned.