RE: [PATCH] nvme-tcp: proper handling of tcp socket closing flows
From: Grupi, Elad <hidden>
Date: 2021-01-28 15:28:21
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@grimberg.me> Sent: Thursday, 28 January 2021 9:55 To: Grupi, Elad; linux-nvme@lists.infradead.org Subject: Re: [PATCH] nvme-tcp: proper handling of tcp socket closing flows [EXTERNAL EMAIL]
From: Elad Grupi <redacted> 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?
quoted hunk ↗ jump to hunk
Signed-off-by: Elad Grupi <redacted> --- 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);
_______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme