[PATCH net 0/2] Fixes for closing process and minor cleanup

[PATCH net 0/2] Fixes for closing process and minor cleanup

[Tony Lu]

Patch 1 is a minor cleanup for local struct sock variables.

Patch 2 ensures the active closing side enters TIME_WAIT.

Tony Lu (2):
  net/smc: Clean up local struct sock variables
  net/smc: Ensure the active closing peer first closes clcsock

 net/smc/smc_close.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.32.0.3.g01195cf9f

[PATCH net 1/2] net/smc: Clean up local struct sock variables

[Tony Lu]

There remains some variables to replace with local struct sock. So clean
them up all.

Fixes: 3163c5071f25 ("net/smc: use local struct sock variables consistently")
Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_close.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index 0f9ffba07d26..9b235fbb089a 100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -354,9 +354,9 @@ static void smc_close_passive_work(struct work_struct *work)
 	if (rxflags->peer_conn_abort) {
 		/* peer has not received all data */
 		smc_close_passive_abort_received(smc);
-		release_sock(&smc->sk);
+		release_sock(sk);
 		cancel_delayed_work_sync(&conn->tx_work);
-		lock_sock(&smc->sk);
+		lock_sock(sk);
 		goto wakeup;
 	}
 
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net 1/2] net/smc: Clean up local struct sock variables

[Karsten Graul]

On 23/11/2021 09:25, Tony Lu wrote:
> There remains some variables to replace with local struct sock. So clean
> them up all.
> 
> Fixes: 3163c5071f25 ("net/smc: use local struct sock variables consistently")
> Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
> Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
> ---

This is a rather cosmetic change, I will pick it up for our next submission to the 
net-next tree.

Thank you.

[PATCH net 2/2] net/smc: Ensure the active closing peer first closes clcsock

[Tony Lu]

The side that actively closed socket, it's clcsock doesn't enter
TIME_WAIT state, but the passive side does it. It should show the same
behavior as TCP sockets.

Consider this, when client actively closes the socket, the clcsock in
server enters TIME_WAIT state, which means the address is occupied and
won't be reused before TIME_WAIT dismissing. If we restarted server, the
service would be unavailable for a long time.

To solve this issue, shutdown the clcsock in [A], perform the TCP active
close progress first, before the passive closed side closing it. So that
the actively closed side enters TIME_WAIT, not the passive one.

Client                                            |  Server
close() // client actively close                  |
  smc_release()                                   |
      smc_close_active() // PEERCLOSEWAIT1        |
          smc_close_final() // abort or closed = 1|
              smc_cdc_get_slot_and_msg_send()     |
          [A]                                     |
                                                  |smc_cdc_msg_recv_action() // ACTIVE
                                                  |  queue_work(smc_close_wq, &conn->close_work)
                                                  |    smc_close_passive_work() // PROCESSABORT or APPCLOSEWAIT1
                                                  |      smc_close_passive_abort_received() // only in abort
                                                  |
                                                  |close() // server recv zero, close
                                                  |  smc_release() // PROCESSABORT or APPCLOSEWAIT1
                                                  |    smc_close_active()
                                                  |      smc_close_abort() or smc_close_final() // CLOSED
                                                  |        smc_cdc_get_slot_and_msg_send() // abort or closed = 1
smc_cdc_msg_recv_action()                         |    smc_clcsock_release()
  queue_work(smc_close_wq, &conn->close_work)     |      sock_release(tcp) // actively close clc, enter TIME_WAIT
    smc_close_passive_work() // PEERCLOSEWAIT1    |    smc_conn_free()
      smc_close_passive_abort_received() // CLOSED|
      smc_conn_free()                             |
      smc_clcsock_release()                       |
        sock_release(tcp) // passive close clc    |

Link: https://www.spinics.net/lists/netdev/msg780407.html
Fixes: b38d732477e4 ("smc: socket closing and linkgroup cleanup")
Signed-off-by: Tony Lu <tonylu@linux.alibaba.com>
Reviewed-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_close.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
index 9b235fbb089a..3715d2f5ad55 100644
--- a/net/smc/smc_close.c
+++ b/net/smc/smc_close.c
@@ -228,6 +228,12 @@ int smc_close_active(struct smc_sock *smc)
 			/* send close request */
 			rc = smc_close_final(conn);
 			sk->sk_state = SMC_PEERCLOSEWAIT1;
+
+			/* actively shutdown clcsock before peer close it,
+			 * prevent peer from entering TIME_WAIT state.
+			 */
+			if (smc->clcsock && smc->clcsock->sk)
+				rc = kernel_sock_shutdown(smc->clcsock, SHUT_RDWR);
 		} else {
 			/* peer event has changed the state */
 			goto again;
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net 2/2] net/smc: Ensure the active closing peer first closes clcsock

[Karsten Graul]

On 23/11/2021 09:25, Tony Lu wrote:
> The side that actively closed socket, it's clcsock doesn't enter
> TIME_WAIT state, but the passive side does it. It should show the same
> behavior as TCP sockets.
> 
> Consider this, when client actively closes the socket, the clcsock in
> server enters TIME_WAIT state, which means the address is occupied and
> won't be reused before TIME_WAIT dismissing. If we restarted server, the
> service would be unavailable for a long time.
> 
> To solve this issue, shutdown the clcsock in [A], perform the TCP active
> close progress first, before the passive closed side closing it. So that
> the actively closed side enters TIME_WAIT, not the passive one.
> 

Thank you, I will pick this up for our next submission to the net tree.

Re: [PATCH net 0/2] Fixes for closing process and minor cleanup

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 23 Nov 2021 16:25:14 +0800 you wrote:
> Patch 1 is a minor cleanup for local struct sock variables.
> 
> Patch 2 ensures the active closing side enters TIME_WAIT.
> 
> Tony Lu (2):
>   net/smc: Clean up local struct sock variables
>   net/smc: Ensure the active closing peer first closes clcsock
> 
> [...]

Here is the summary with links:
  - [net,1/2] net/smc: Clean up local struct sock variables
    https://git.kernel.org/netdev/net/c/45c3ff7a9ac1
  - [net,2/2] net/smc: Ensure the active closing peer first closes clcsock
    https://git.kernel.org/netdev/net/c/606a63c9783a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html