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.
...
if (smc->use_fallback) {
sock->state = rc ? SS_CONNECTING : SS_CONNECTED;
if (!rc)
sk->sk_state = SMC_ACTIVE; /* synchronize sk_state from SMC_INIT to SMC_ACTIVE */
goto out;
}
...
>> + break;
>> + case SS_UNCONNECTED:
>> + sock->state = SS_CONNECTING;
>> + break;
>> + }
>> +
>> switch (sk->sk_state) {
>> default:
>> goto out;
>> + case SMC_CLOSED:
>> + rc = sock_error(sk) ? : -ECONNABORTED;
>> + sock->state = SS_UNCONNECTED;
>> + goto out;
>> case SMC_ACTIVE:
>> rc = -EISCONN;
>> goto out;
>> @@ -1565,20 +1585,24 @@ static int smc_connect(struct socket *sock, struct sockaddr *addr,
>> goto out;
>>
>> sock_hold(&smc->sk); /* sock put in passive closing */
>> - if (smc->use_fallback)
>> + if (smc->use_fallback) {
>> + sock->state = rc ? SS_CONNECTING : SS_CONNECTED;
>> goto out;
>> + }
>> if (flags & O_NONBLOCK) {
>> if (queue_work(smc_hs_wq, &smc->connect_work))
>> smc->connect_nonblock = 1;
>> rc = -EINPROGRESS;
>> + goto out;
>> } else {
>> rc = __smc_connect(smc);
>> if (rc < 0)
>> goto out;
>> - else
>> - rc = 0; /* success cases including fallback */
>> }
>>
>> +connected:
>> + rc = 0;
>> + sock->state = SS_CONNECTED;
>> out:
>> release_sock(sk);
>> out_err:
