Re: [PATCH 2/2] svcrdma: Handle ADDR_CHANGE CM event properly

[Chuck Lever <chuck.lever@oracle.com>]

On Sat, Jun 01, 2024 at 12:48:40PM +0200, Zhu Yanjun wrote:
> On 31.05.24 15:15, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Sagi tells me that when a bonded device reports an address change,
> > the consumer must destroy its listener IDs and create new ones.
> > 
> > See commit a032e4f6d60d ("nvmet-rdma: fix bonding failover possible
> > NULL deref").
> > 
> > Suggested-by: Sagi Grimberg <sagi@grimberg.me>
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >   net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 +++++++++++++++-
> >   1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > index fa50b7494a0a..a003b62fb7d5 100644
> > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > @@ -284,17 +284,31 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id,
> >    *
> >    * Return values:
> >    *     %0: Do not destroy @cma_id
> > - *     %1: Destroy @cma_id (never returned here)
> > + *     %1: Destroy @cma_id
> >    *
> >    * NB: There is never a DEVICE_REMOVAL event for INADDR_ANY listeners.
> >    */
> >   static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
> >   				   struct rdma_cm_event *event)
> >   {
> > +	struct svcxprt_rdma *cma_xprt = cma_id->context;
> > +	struct svc_xprt *cma_rdma = &cma_xprt->sc_xprt;
> > +	struct sockaddr *sap = (struct sockaddr *)&cma_id->route.addr.src_addr;
> > +	struct rdma_cm_id *listen_id;
> 
> I am not sure whether I need to suggest "Reverse Christmas Tree" or not.
> This is a trivial problem. ^_^
> You can ignore it. And it is not mandatory.

Oops, I missed that @sap can be declared first. It must have gotten
lost while I was trying out different solutions for this issue.

Fixed.


> But if we follow this rule, the source code will become more readable.
> 
> Zhu Yanjun
> 
> > +
> >   	switch (event->event) {
> >   	case RDMA_CM_EVENT_CONNECT_REQUEST:
> >   		handle_connect_req(cma_id, &event->param.conn);
> >   		break;
> > +	case RDMA_CM_EVENT_ADDR_CHANGE:
> > +		listen_id = svc_rdma_create_listen_id(cma_rdma->xpt_net,
> > +						      sap, cma_xprt);
> > +		if (IS_ERR(listen_id)) {
> > +			pr_err("Listener dead, address change failed for device %s\n",
> > +				cma_id->device->name);
> > +		} else
> > +			cma_xprt->sc_cm_id = listen_id;
> > +		return 1;
> >   	default:
> >   		break;
> >   	}
> 

-- 
Chuck Lever