On 24.05.22 15:05, Karsten Graul wrote:
On 24/05/2022 14:57, liuyacan@xxxxxxxxxxxxxxxx wrote:
On 2022/5/23 20:24, Karsten Graul wrote:
On 13/05/2022 04:24, Guangguan Wang wrote:
Connect with O_NONBLOCK will not be completed immediately
and returns -EINPROGRESS. It is possible to use selector/poll
for completion by selecting the socket for writing. After select
indicates writability, a second connect function call will return
0 to indicate connected successfully as TCP does, but smc returns
-EISCONN. Use socket state for smc to indicate connect state, which
can help smc aligning the connect behaviour with TCP.
Signed-off-by: Guangguan Wang <guangguan.wang@xxxxxxxxxxxxxxxxx>
Acked-by: Karsten Graul <kgraul@xxxxxxxxxxxxx>
---
net/smc/af_smc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 46 insertions(+), 4 deletions(-)
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index fce16b9d6e1a..5f70642a8044 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1544,9 +1544,29 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
goto out_err;
lock_sock(sk);
+ switch (sock->state) {
+ default:
+ rc = -EINVAL;
+ goto out;
+ case SS_CONNECTED:
+ rc = sk->sk_state == SMC_ACTIVE ? -EISCONN : -EINVAL;
+ goto out;
+ case SS_CONNECTING:
+ if (sk->sk_state == SMC_ACTIVE)
+ goto connected;
I stumbled over this when thinking about the fallback processing. If for whatever reason
fallback==true during smc_connect(), the "if (smc->use_fallback)" below would set sock->state
to e.g. SS_CONNECTED. But in the fallback case sk_state keeps SMC_INIT. So during the next call
the SS_CONNECTING case above would break because sk_state in NOT SMC_ACTIVE, and we would end
up calling kernel_connect() again. Which seems to be no problem when kernel_connect() returns
-EISCONN and we return this to the caller. But is this how it should work, or does it work by chance?
Since the sk_state keeps SMC_INIT and does not correctly indicate the state of clcsock, it should end
up calling kernel_connect() again to get the actual connection state of clcsock.
And I'm sorry there is a problem that if sock->state==SS_CONNECTED and sk_state==SMC_INIT, further call
of smc_connect will return -EINVAL where -EISCONN is preferred.
The steps to reproduce:
1）switch fallback before connect, such as setsockopt TCP_FASTOPEN
2）connect with noblocking and returns -EINPROGRESS. (sock->state changes to SS_CONNECTING)
3) end up calling connect with noblocking again and returns 0. (kernel_connect() returns 0 and sock->state changes to
SS_CONNECTED but sk->sk_state stays SMC_INIT)
4) call connect again, maybe by mistake, will return -EINVAL, but -EISCONN is preferred.
What do you think about if we synchronize the sk_state to SMC_ACTIVE instead of keeping SMC_INIT when clcsock
connected successfully in fallback case described above.
...
I start thinking that the fix in 86434744 introduced a problem. Before that fix a connect with
fallback always reached __smc_connect() and on top of that function in case of fallback
smc_connect_fallback() is called, which itself sets sk_state to SMC_ACTIVE.
86434744 removed that code path and I wonder what it actually fixed, because at this time the
fallback check in __smc_connect() was already present.
Without that "goto out;" the state would be set correctly in smc_connect_fallback(), and the
socket close processing would work as expected.
I think it is OK without that "goto out;". And I guess the purpose of "goto out;" is to avoid calling __smc_connect(),
because it is impossible to establish an rdma channel at this time.
Yes that was the purpose, but this disabled all the extra processing that should be done
for fallback sockets during connect().
Since Karsten's suggestion, we didn't hear from you any more. We just
want to know:
- What do you think about the commit (86434744)? Could it be the trigger
of the problem you met?
- Have you ever tried to just remove the following lines from
smc_connection(), and check if your scenario could run correctly?
if (smc->use_fallback)
goto out;
In our opinion, we don't see the necessity of the patch, if partly
reverting the commit (86434744) could solve the problem.
