[PATCH] nvmet-tcp: remove a redundant line in nvmet_tcp_release_queue_work

[PATCH] nvmet-tcp: remove a redundant line in nvmet_tcp_release_queue_work

[Tomas Henzl]

cancel_work_sync(&queue->io_work) is called twice, remove the
second instance.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/nvme/target/tcp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 470bf37e5a63..a1d0f6b18b2c 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1577,7 +1577,6 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
 	nvmet_tcp_uninit_data_in_cmds(queue);
 	nvmet_sq_destroy(&queue->nvme_sq);
 	nvmet_cq_put(&queue->nvme_cq);
-	cancel_work_sync(&queue->io_work);
 	nvmet_tcp_free_cmd_data_in_buffers(queue);
 	/* ->sock will be released by fput() */
 	fput(queue->sock->file);
-- 
2.51.0

Re: [PATCH] nvmet-tcp: remove a redundant line in nvmet_tcp_release_queue_work

[Maurizio Lombardi]

On Mon Oct 6, 2025 at 7:59 PM CEST, Tomas Henzl wrote:
> cancel_work_sync(&queue->io_work) is called twice, remove the
> second instance.
>
> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> ---
>  drivers/nvme/target/tcp.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> index 470bf37e5a63..a1d0f6b18b2c 100644
> --- a/drivers/nvme/target/tcp.c
> +++ b/drivers/nvme/target/tcp.c
> @@ -1577,7 +1577,6 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
>  	nvmet_tcp_uninit_data_in_cmds(queue);
>  	nvmet_sq_destroy(&queue->nvme_sq);
>  	nvmet_cq_put(&queue->nvme_cq);
> -	cancel_work_sync(&queue->io_work);
>  	nvmet_tcp_free_cmd_data_in_buffers(queue);
>  	/* ->sock will be released by fput() */
>  	fput(queue->sock->file);

I am not sure it's safe to remove it because io_work can potentially
re-enqueue itself.

The second call to cancel_work_sync(), after rcv_state is set to
NVMET_TCP_RECV_ERR, should ensure that no more "io_work"s are pending
on the workqueue.

Maurizio

Re: [PATCH] nvmet-tcp: remove a redundant line in nvmet_tcp_release_queue_work

[Maurizio Lombardi]

On Tue Oct 7, 2025 at 8:26 AM CEST, Maurizio Lombardi wrote:
> On Mon Oct 6, 2025 at 7:59 PM CEST, Tomas Henzl wrote:
>> cancel_work_sync(&queue->io_work) is called twice, remove the
>> second instance.
>>
>> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
>> ---
>>  drivers/nvme/target/tcp.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>> index 470bf37e5a63..a1d0f6b18b2c 100644
>> --- a/drivers/nvme/target/tcp.c
>> +++ b/drivers/nvme/target/tcp.c
>> @@ -1577,7 +1577,6 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
>>  	nvmet_tcp_uninit_data_in_cmds(queue);
>>  	nvmet_sq_destroy(&queue->nvme_sq);
>>  	nvmet_cq_put(&queue->nvme_cq);
>> -	cancel_work_sync(&queue->io_work);
>>  	nvmet_tcp_free_cmd_data_in_buffers(queue);
>>  	/* ->sock will be released by fput() */
>>  	fput(queue->sock->file);
>
> I am not sure it's safe to remove it because io_work can potentially
> re-enqueue itself.
>

Small correction, the problem isn't io_work re-enqueing itself
(the first cancel_work_sync() prevents that); I think the real problem
is that nvmet_sq_destroy() could end up calling
nvmet_tcp_queue_response() which enqueues io_work again.

Maurizio

Re: [PATCH] nvmet-tcp: remove a redundant line in nvmet_tcp_release_queue_work

[Tomas Henzl]

On 10/7/25 8:46 AM, Maurizio Lombardi wrote:
> On Tue Oct 7, 2025 at 8:26 AM CEST, Maurizio Lombardi wrote:
>> On Mon Oct 6, 2025 at 7:59 PM CEST, Tomas Henzl wrote:
>>> cancel_work_sync(&queue->io_work) is called twice, remove the
>>> second instance.
>>>
>>> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
>>> ---
>>>  drivers/nvme/target/tcp.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>>> index 470bf37e5a63..a1d0f6b18b2c 100644
>>> --- a/drivers/nvme/target/tcp.c
>>> +++ b/drivers/nvme/target/tcp.c
>>> @@ -1577,7 +1577,6 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
>>>  	nvmet_tcp_uninit_data_in_cmds(queue);
>>>  	nvmet_sq_destroy(&queue->nvme_sq);
>>>  	nvmet_cq_put(&queue->nvme_cq);
>>> -	cancel_work_sync(&queue->io_work);
>>>  	nvmet_tcp_free_cmd_data_in_buffers(queue);
>>>  	/* ->sock will be released by fput() */
>>>  	fput(queue->sock->file);
>>
>> I am not sure it's safe to remove it because io_work can potentially
>> re-enqueue itself.
>>
> 
> Small correction, the problem isn't io_work re-enqueing itself
> (the first cancel_work_sync() prevents that); I think the real problem
> is that nvmet_sq_destroy() could end up calling
> nvmet_tcp_queue_response() which enqueues io_work again.

The NVMET_TCP_RECV_ERR sate would make the workqueue finish quickly but
sure we don't want a running workqueue.

Let's drop this patch.

tomash

> 
> Maurizio
>