[PATCH] nvme-tcp: proper handling of tcp socket closing flows

[Grupi, Elad]

> But we need to call the release_queue work, not sure I understand how this works.

If the tcp socket was closed before setting the socket callback,
nvmet_tcp_set_queue_sock will return -ENOTCONN and everything will be cleared by goto out_destroy_sq.
No need a worker to clean resources in that case and everything is protected by sk_callback_lock.

> io_work is going to run with release_queue_work, because it is also coming from the backend completion. The right way to solve this is to correctly fence them.

> Can you describe the exact race you are referring to?

Correct, the fence already exist in the code, cancel_work_sync is called after nvmet_sq_destroy in release_queue_work. I will remove that part form the patch.

-----Original Message-----
From: Sagi Grimberg <sagi at grimberg.me> 
Sent: Thursday, 28 January 2021 9:55
To: Grupi, Elad; linux-nvme at lists.infradead.org
Subject: Re: [PATCH] nvme-tcp: proper handling of tcp socket closing flows


[EXTERNAL EMAIL] 

> From: Elad Grupi <elad.grupi at dell.com>
> 
> avoid calling nvmet_tcp_release_queue_work if tcp socket was closed 
> before setting the sk callbacks.

But we need to call the release_queue work, not sure I understand how this works.

> 
> prevent io_work from enqueuing while closing the tcp queue to avoid 
> race with nvmet_tcp_release_queue_work.

io_work is going to run with release_queue_work, because it is also coming from the backend completion. The right way to solve this is to correctly fence them.

Can you describe the exact race you are referring to?

> 
> Signed-off-by: Elad Grupi <elad.grupi at dell.com>
> ---
>   drivers/nvme/target/tcp.c | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c 
> index d535080b781f..937f2a746d8b 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -647,7 +647,7 @@ static int nvmet_tcp_try_send_one(struct nvmet_tcp_queue *queue,
>   	struct nvmet_tcp_cmd *cmd = queue->snd_cmd;
>   	int ret = 0;
>   
> -	if (!cmd || queue->state == NVMET_TCP_Q_DISCONNECTING) {
> +	if (!cmd) {
>   		cmd = nvmet_tcp_fetch_cmd(queue);
>   		if (unlikely(!cmd))
>   			return 0;
> @@ -1196,7 +1196,7 @@ static void nvmet_tcp_io_work(struct work_struct *w)
>   	/*
>   	 * We exahusted our budget, requeue our selves
>   	 */
> -	if (pending)
> +	if (pending && queue->state != NVMET_TCP_Q_DISCONNECTING)
>   		queue_work_on(queue->cpu, nvmet_tcp_wq, &queue->io_work);
>   }
>   
> @@ -1453,9 +1453,27 @@ static int nvmet_tcp_set_queue_sock(struct nvmet_tcp_queue *queue)
>   	sock->sk->sk_state_change = nvmet_tcp_state_change;
>   	queue->write_space = sock->sk->sk_write_space;
>   	sock->sk->sk_write_space = nvmet_tcp_write_space;
> +
> +	switch (sk->sk_state) {
> +	case TCP_FIN_WAIT1:
> +	case TCP_CLOSE_WAIT:
> +	case TCP_CLOSE:
> +		/* FALLTHRU */
> +		sock->sk->sk_data_ready =  queue->data_ready;
> +		sock->sk->sk_state_change = queue->state_change;
> +		sock->sk->sk_write_space = queue->write_space;
> +		sk->sk_user_data = NULL;
> +		queue->state = NVMET_TCP_Q_DISCONNECTING;
> +		ret = -ENOTCONN;
> +		break;
> +	default:
> +		queue_work_on(queue->cpu, nvmet_tcp_wq, &queue->io_work);
> +		ret = 0;
> +	}
> +
>   	write_unlock_bh(&sock->sk->sk_callback_lock);
>   
> -	return 0;
> +	return ret;
>   }
>   
>   static int nvmet_tcp_alloc_queue(struct nvmet_tcp_port *port, @@ 
> -1506,8 +1524,6 @@ static int nvmet_tcp_alloc_queue(struct nvmet_tcp_port *port,
>   	if (ret)
>   		goto out_destroy_sq;
>   
> -	queue_work_on(queue->cpu, nvmet_tcp_wq, &queue->io_work);
> -
>   	return 0;
>   out_destroy_sq:
>   	mutex_lock(&nvmet_tcp_queue_mutex);
>