RE: [PATCH v1 05/13] xprtrdma: Don't drain CQs on transport disconnect

Devesh Sharma
2014-07-03T05:33:58Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5782 bytes --]

> -----Original Message-----
> From: Chuck Lever [mailto:chuck.lever@oracle.com]
> Sent: Thursday, July 03, 2014 1:30 AM
> To: Devesh Sharma
> Cc: Steve Wise; linux-rdma@vger.kernel.org; Linux NFS Mailing List
> Subject: Re: [PATCH v1 05/13] xprtrdma: Don't drain CQs on transport
> disconnect
>
>
> On Jul 2, 2014, at 3:48 PM, Devesh Sharma <Devesh.Sharma@Emulex.Com>
> wrote:
>
> >
> >
> >> -----Original Message-----
> >> From: Steve Wise [mailto:swise@opengridcomputing.com]
> >> Sent: Thursday, July 03, 2014 1:16 AM
> >> To: 'Chuck Lever'; Devesh Sharma
> >> Cc: linux-rdma@vger.kernel.org; 'Linux NFS Mailing List'
> >> Subject: RE: [PATCH v1 05/13] xprtrdma: Don't drain CQs on transport
> >> disconnect
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Chuck Lever [mailto:chuck.lever@oracle.com]
> >>> Sent: Wednesday, July 02, 2014 2:40 PM
> >>> To: Steve Wise; Devesh Sharma
> >>> Cc: linux-rdma@vger.kernel.org; Linux NFS Mailing List
> >>> Subject: Re: [PATCH v1 05/13] xprtrdma: Don't drain CQs on transport
> >>> disconnect
> >>>
> >>>
> >>> On Jul 2, 2014, at 3:28 PM, Steve Wise
> <swise@opengridcomputing.com>
> >> wrote:
> >>>
> >>>> On 7/2/2014 2:06 PM, Devesh Sharma wrote:
> >>>>> This change is very much prone to generate poll_cq errors because
> >>>>> of un-cleaned
> >>> completions which still
> >>>>> point to the non-existent QPs. On the new connection when these
> >>>>> completions are polled,
> >>> the poll_cq will
> >>>>> fail because old QP pointer is already NULL.
> >>>>> Did anyone hit this situation during their testing?
> >>>
> >>> I tested this aggressively with a fault injector that triggers
> >>> regular connection disruption.
> >>>
> >>>> Hey Devesh,
> >>>>
> >>>> iw_cxgb4 will silently toss CQEs if the QP is not active.
> >>>
> >>> xprtrdma relies on getting a completion (either successful or in
> >>> error) for every WR it has posted. The goal of this patch is to
> >>> avoid throwing away queued completions after a transport disconnect
> >>> so we don't lose track of FRMR rkey updates (FAST_REG_MR and
> >>> LOCAL_INV
> >>> completions) and we can capture all RPC replies posted before the
> >> connection was lost.
> >>>
> >>> Sounds like we also need to keep the QP around, even in error state,
> >>> until all known WRs on that QP have completed?
> >>>
> >
> > Why not poll and process every completion during
> rpcrdma_cq_cleanup()….
>
> Yes, I have a patch in the next version of this series that does that.
> It just calls rpcrdma_sendcq_upcall() from the connect worker. I will squash
> that change into this patch.

Cool.

>
> Maybe it needs to invoke rpcrdma_recvcq_upcall() there as well.

Yes, that would be needed.

>
>
> >
> >>
> >> Perhaps.
> >>
> >>>
> >>>>
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> >>>>>> owner@vger.kernel.org] On Behalf Of Chuck Lever
> >>>>>> Sent: Tuesday, June 24, 2014 4:10 AM
> >>>>>> To: linux-rdma@vger.kernel.org; linux-nfs@vger.kernel.org
> >>>>>> Subject: [PATCH v1 05/13] xprtrdma: Don't drain CQs on transport
> >>>>>> disconnect
> >>>>>>
> >>>>>> CQs are not destroyed until unmount. By draining CQs on transport
> >>>>>> disconnect, successful completions that can change the
> >>>>>> r.frmr.state field can be missed.
> >>>>>>
> >>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>>> ---
> >>>>>> net/sunrpc/xprtrdma/verbs.c | 5 -----
> >>>>>> 1 file changed, 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/net/sunrpc/xprtrdma/verbs.c
> >>>>>> b/net/sunrpc/xprtrdma/verbs.c index 3c7f904..451e100 100644
> >>>>>> --- a/net/sunrpc/xprtrdma/verbs.c
> >>>>>> +++ b/net/sunrpc/xprtrdma/verbs.c
> >>>>>> @@ -873,9 +873,6 @@ retry:
> >>>>>> dprintk("RPC: %s:
> rpcrdma_ep_disconnect"
> >>>>>> " status %i\n", __func__, rc);
> >>>>>>
> >>>>>> - rpcrdma_clean_cq(ep->rep_attr.recv_cq);
> >>>>>> - rpcrdma_clean_cq(ep->rep_attr.send_cq);
> >>>>>> -
> >>>>>> xprt = container_of(ia, struct rpcrdma_xprt, rx_ia);
> >>>>>> id = rpcrdma_create_id(xprt, ia,
> >>>>>> (struct sockaddr *)&xprt-
> >rx_data.addr);
> >> @@ -985,8 +982,6 @@
> >>>>>> rpcrdma_ep_disconnect(struct rpcrdma_ep *ep, struct rpcrdma_ia
> >>>>>> *ia) {
> >>>>>> int rc;
> >>>>>>
> >>>>>> - rpcrdma_clean_cq(ep->rep_attr.recv_cq);
> >>>>>> - rpcrdma_clean_cq(ep->rep_attr.send_cq);
> >>>>>> rc = rdma_disconnect(ia->ri_id);
> >>>>>> if (!rc) {
> >>>>>> /* returns without wait if not connected */
> >>>>>>
> >>>>>> --
> >>>>>> To unsubscribe from this list: send the line "unsubscribe
> >>>>>> linux-rdma" in the body of a message to
> majordomo@vger.kernel.org
> >>>>>> More majordomo info at http://vger.kernel.org/majordomo-
> info.html
> >>>>> N r y b X Ç§v ^ )Þº{.n + { " ^n r z \x1a h & \x1e G
> >>>>> h \x03( éšŽ
> >>> Ý¢j" \x1a ^[m z Þ– f h ~ mml==
> >>>>
> >>>> --
> >>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> >>>> in the body of a message to majordomo@vger.kernel.org More
> >> majordomo
> >>>> info at http://vger.kernel.org/majordomo-info.html
> >>>
> >>> --
> >>> Chuck Lever
> >>> chuck[dot]lever[at]oracle[dot]com
> >>>
> >>>
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> > in the body of a message to majordomo@vger.kernel.org More
> majordomo
> > info at http://vger.kernel.org/majordomo-info.html
> >
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dÊ‡Ú™ë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš