Skip to contentSkip to conversation

NFS/RDMA client stall patches

PATCH Peter Leckie 2 participants 2 messages May 20, 2008 3 messages not in the archive

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Peter Leckie

author
#3 lore raw
In reply to a message not in the archive
Quoted 7 lines · Thomas
Quoted 4 lines · Peter Leckie

This patch changes rpcrdma_conn_func() to directly call xprt_disconnect() instead of directly waking the pending task queue.

Quoted 7 lines

Does this fix some issue, or is it simply more efficient to initiate the disconnect immediately? Because the conn_func is called directly from the RDMA provider's connection upcall, it may be entered in an arbitrary context, so xprt_disconnect() could be a bit heavyweight and lead to deadlock. Have you satisified yourself this is not the case?

Well xprt_disconnect_done() is no more heavy weight then the rpcrdma_conn_func() equivalent it simply clears the connected bit and labels the queues as not connected. The reason for calling xprt_disconnect_done() is to make a single disconnect function.

The reason this change is needed is to allow the send and resend queues to be drained on disconnect, patch 02 could have been changed to also drain the these queues from rpcrdma_conn_func() however I think this is a cleaner fix.

Thanks
Pete

Quoted 36 lines
Quoted 33 lines
Signed-off-by Peter Leckie <pleckie-cP1dWloDopni96+mSzHFpQC/[email protected]>
Reviewed-by Greg Banks <gnb-cP1dWloDopni96+mSzHFpQC/[email protected]>

X-Sgi-Pv: 970244
<http://bugworks/query.cgi/970244>---
Index: linux-2.6.25.3/net/sunrpc/xprtrdma/rpc_rdma.c
===================================================================

net/sunrpc/xprtrdma/rpc_rdma.c +4 -6
@@ -680,15 +680,13 @@ rpcrdma_conn_func(struct rpcrdma_ep *ep)
 {
       struct rpc_xprt *xprt = ep->rep_xprt;
 
-       spin_lock_bh(&xprt->transport_lock);
       if (ep->rep_connected > 0) {
+               spin_lock_bh(&xprt->transport_lock);
               if (!xprt_test_and_set_connected(xprt))
                       xprt_wake_pending_tasks(xprt, 0);
-       } else {
-               if (xprt_test_and_clear_connected(xprt))
-                       xprt_wake_pending_tasks(xprt, ep->rep_connected);
-       }
-       spin_unlock_bh(&xprt->transport_lock);
+               spin_unlock_bh(&xprt->transport_lock);
+       } else
+               xprt_disconnect(xprt);
 }
 
 /*
#4 lore raw
Quoted 6 lines · Peter Leckie

Well xprt_disconnect_done() is no more heavy weight then the rpcrdma_conn_func()
equivalent it simply clears the connected bit and labels the queues as not connected.
The reason for calling xprt_disconnect_done() is to make a single disconnect function.

Aha - I agree and now realize that I misunderstood because your patch calls xprt_disconnect(), which (IIRC) is an older heavier function that's no longer in sunrpc. Is this the right patch then?

Quoted 4 lines

The reason this change is needed is to allow the send and resend queues to be drained on disconnect, patch 02 could have been changed to also drain the these queues from rpcrdma_conn_func() however I think this is a cleaner fix.

Sounds good - pending better understanding of which fix is right. :-)

...

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

Index: linux-2.6.25.3/net/sunrpc/xprtrdma/rpc_rdma.c
===================================================================

net/sunrpc/xprtrdma/rpc_rdma.c +0 -0
@@ -680,15 +680,13 @@ rpcrdma_conn_func(struct rpcrdma_ep *ep)

...

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
net/sunrpc/xprtrdma/rpc_rdma.c +3 -0
+               spin_unlock_bh(&xprt->transport_lock);
+       } else
+               xprt_disconnect(xprt);

}

Tom.