Skip to contentSkip to conversation

xprtrdma: Wake up re_connect_wait on disconnect

PATCH In review Dan Aloni 2 participants 4 messages Jun 20, 2020 – Jun 21, 2020

Subsystems (4)
Networking NFS client NFS server InfiniBand
lore
Share & tools
lore

Dan Aloni

author
#0 lore raw

PATCH 1/1 xprtrdma: Wake up re_connect_wait on disconnect

Given that rpcrdma_xprt_connect() happens from workqueue context, on cases where connections don't succeeds, something needs to wake it up. In my case, this has been observed when the CM callback received RDMA_CM_EVENT_REJECTED, and rpcrdma_xprt_connect() slept forever.

This continues the fix in commit 58bd6656f808 ('xprtrdma: Restore wake-up-all to rpcrdma_cm_event_handler()').

Signed-off-by Dan Aloni <[email protected]>
CC Chuck Lever <[email protected]>

---

Notes:
Hi Chuck,

Maybe I missd something, as it is not clear to me how otherwise (without this patch), re_connect_wait can be woken up in this situation. Please explain?

 net/sunrpc/xprtrdma/verbs.c | 1 +
 1 file changed, 1 insertion(+)
net/sunrpc/xprtrdma/verbs.c +1 -0
index 2ae348377806..8bd76a47a91f 100644
@@ -289,6 +289,7 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		ep->re_connect_status = -ECONNABORTED;
 disconnected:
 		xprt_force_disconnect(xprt);
+		wake_up_all(&ep->re_connect_wait);
 		return rpcrdma_ep_destroy(ep);
 	default:
 		break;
Signature · 1 line
2.25.4
#1 lore raw

Hi Dan-

Quoted 6 lines · Dan Aloni

Given that rpcrdma_xprt_connect() happens from workqueue context, on cases where connections don't succeeds, something needs to wake it up. In my case, this has been observed when the CM callback received RDMA_CM_EVENT_REJECTED, and rpcrdma_xprt_connect() slept forever.

Interesting. My development and testing generates plenty of REJECTED connection requests, but I never saw this particular failure mode.

Quoted 2 lines

This continues the fix in commit 58bd6656f808 ('xprtrdma: Restore wake-up-all to rpcrdma_cm_event_handler()').

The patch looks sensible. I'll pull it into my test harness.

Quoted 22 lines
Signed-off-by Dan Aloni <[email protected]>
CC Chuck Lever <[email protected]>

---

Notes:
Hi Chuck,

Maybe I missd something, as it is not clear to me how otherwise (without this patch), re_connect_wait can be woken up in this situation. Please explain?

net/sunrpc/xprtrdma/verbs.c | 1 +

1 file changed, 1 insertion(+)
net/sunrpc/xprtrdma/verbs.c +0 -0
index 2ae348377806..8bd76a47a91f 100644
@@ -289,6 +289,7 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		ep->re_connect_status = -ECONNABORTED;

disconnected:

		xprt_force_disconnect(xprt);
net/sunrpc/xprtrdma/verbs.c +1 -0
+		wake_up_all(&ep->re_connect_wait);
Quoted 6 lines
		return rpcrdma_ep_destroy(ep);
	default:
		break;
Signature · 1 line
2.25.4

--
Chuck Lever

#2 lore raw

Hi Dan-

Quoted 6 lines · Chuck Lever

Hi Dan-

Quoted 2 lines · Dan Aloni
Quoted 7 lines
Quoted 4 lines

Given that rpcrdma_xprt_connect() happens from workqueue context, on cases where connections don't succeeds, something needs to wake it up. In my case, this has been observed when the CM callback received RDMA_CM_EVENT_REJECTED, and rpcrdma_xprt_connect() slept forever.

Interesting. My development and testing generates plenty of REJECTED connection requests, but I never saw this particular failure mode.

Correction: My testing _used_ _to_ generate REJECTED events regularly. It does not seem to any more, even after client crashes. So that explains why I haven't seen this before.

I haven't reproduced the problem here, but the fix still looks proper to me, and doesn't appear to introduce any regressions. I do have some issues with your proposed patch, though.

The first paragraph of the patch description is incorrect. RDMA_CM_EVENT_DISCONNECTED can occur only once a connection has been established. That guarantees there are no waiters on re_connect_wait in that case. It's connect errors that need to wake-up the connect worker.

Quoted 2 lines
Quoted 2 lines

This continues the fix in commit 58bd6656f808 ('xprtrdma: Restore wake-up-all to rpcrdma_cm_event_handler()').

IMO this paragraph needs to be replaced by:

Fixes e28ce90083f0 ("xprtrdma: kmalloc rpcrdma_ep separate from rpcrdma_xprt")
Quoted 13 lines
Quoted 13 lines
Signed-off-by Dan Aloni <[email protected]>
CC Chuck Lever <[email protected]>

---

Notes:
Hi Chuck,

Maybe I missd something, as it is not clear to me how otherwise (without this patch), re_connect_wait can be woken up in this situation. Please explain?

net/sunrpc/xprtrdma/verbs.c | 1 +

1 file changed, 1 insertion(+)
Quoted 12 lines
Quoted 9 lines
net/sunrpc/xprtrdma/verbs.c +0 -0
index 2ae348377806..8bd76a47a91f 100644
@@ -289,6 +289,7 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		ep->re_connect_status = -ECONNABORTED;

disconnected:

		xprt_force_disconnect(xprt);
net/sunrpc/xprtrdma/verbs.c +1 -0
+		wake_up_all(&ep->re_connect_wait);
Quoted 3 lines
		return rpcrdma_ep_destroy(ep);
	default:
		break;

This hunk does not apply on top of fixes I've already sent to Anna for 5.8-rc1.

So, if you don't object, I'll adjust your patch (this hunk and the description) before sending it along to Anna.

--
Chuck Lever

author
#3 lore raw
Quoted 15 lines · Chuck Lever
Quoted 4 lines
Quoted 4 lines
net/sunrpc/xprtrdma/verbs.c +0 -0
index 2ae348377806..8bd76a47a91f 100644
Quoted 8 lines
Quoted 8 lines
net/sunrpc/xprtrdma/verbs.c +0 -0
@@ -289,6 +289,7 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		ep->re_connect_status = -ECONNABORTED;

disconnected:

		xprt_force_disconnect(xprt);
net/sunrpc/xprtrdma/verbs.c +1 -0
+		wake_up_all(&ep->re_connect_wait);

		return rpcrdma_ep_destroy(ep);
	default:
		break;

This hunk does not apply on top of fixes I've already sent to Anna for 5.8-rc1.

Quoted 2 lines

So, if you don't object, I'll adjust your patch (this hunk and the description) before sending it along to Anna.

Sure, go ahead. Thanks for working on this!

Signature · 1 line
Dan Aloni