Re: [PATCH] nvme-tcp: fix io_work priority inversion when send_mutex is held

[Seiji Nishikawa <snishika@redhat.com>]

Thank you for the review.

On 10/21/26 12:10 PM, Hannes Reinecke wrote:
> On 10/14/26 8:45 AM, Seiji Nishikawa wrote:
>> We hit this on a 6 CPU VM running a database on nvme-tcp storage. In
>> the vmcore, 2 tasks held send_mutex of 2 queues and were runnable in
>> lock_sock_nested() for 103 seconds, while the kworkers for those queues
>> kept running io_work on their CPUs. All database tasks were in one
>> cgroup, which got only about 0.3% of the CPU against the nice -20
>> kworker. Requests timed out, and blk_mq_timeout_work() waited in
>> synchronize_srcu() for the tasks still in .queue_rq.
>>
> How very interesting. I don't think you could craft a blktest for this?
>

I do not have a reliable reproducer in my lab. Reproducing the hang
requires a particular sequence of scheduling events. The task holding
send_mutex is preempted inside lock_sock_nested(), a higher priority
(nice -20) kworker looping in io_work, waiting for send_mutex to be
released, takes that CPU, and then starves the task holding send_mutex
of CPU time indefinitely. The kworker keeps looping because
mutex_trylock() keeps failing, since the preempted task still holds
send_mutex, and the pending check (f42d4796ee10) keeps the kworker
running instead of letting it return. It happened on a 6 CPU VM with a
database under heavy load. So far I have only seen it on the customer
system, but there it reproduced every time.

>> Only do the pending check if io_work could take send_mutex. If it could
>> not, the task holding send_mutex sends what it can in
>> nvme_tcp_send_all(), and queues io_work again after it releases the
>> mutex if requests are still pending, so no request is left behind.
>>
>
> The direct send path again. I really wonder if we need it at all, or
> whether we cannot simply delete it.
>
> Can you try if you see a regression with this patch?
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index ba5c7b3e2a7c..2c59b550e13f 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -405,17 +405,6 @@ static inline void nvme_tcp_queue_request(struct
> nvme_tcp_request *req,
>          empty = llist_add(&req->lentry, &queue->req_list) &&
>                  list_empty(&queue->send_list) && !queue->request;
>
> -       /*
> -        * if we're the first on the send_list and we can try to send
> -        * directly, otherwise queue io_work. Also, only do that if we
> -        * are on the same cpu, so we don't introduce contention.
> -        */
> -       if (queue->io_cpu == raw_smp_processor_id() &&
> -           empty && mutex_trylock(&queue->send_mutex)) {
> -               nvme_tcp_send_all(queue);
> -               mutex_unlock(&queue->send_mutex);
> -       }
> -
>          if (last && nvme_tcp_queue_has_pending(queue))
>                  queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
>   }
>

I cannot try it where the hang reproduced. That is a customer production
system, and I have no reproducer outside of it. On the kernel with the
regression, that system hung within 12 to about 50 minutes after boot, 5
times in 2 days, under its normal database load. It has been running a
kernel with my patch since October 15 under the same load, and the hang
has not come back. With your patch this hang cannot happen, since
io_work becomes the only sender and the trylock in io_work never fails.

What I could do is a sanity check. On a 4 CPU VM with an nvmet tcp
loopback target I ran

  # NVMET_TRTYPES=tcp ./check nvme

on 3 kernels: v7.3 (base), v7.3 with my patch (seiji), and v7.3 with
your patch (hannes).

base    passed=49 failed=0 not_run=12
seiji   passed=49 failed=0 not_run=12
hannes  passed=49 failed=0 not_run=12
per-test results identical

The 12 not run are the same on all 3 kernels, skipped for reasons
unrelated to the patches:

      6     nvme_trtype=tcp is not supported in this test
      1     command tlshd is not available
      1     driver scsi_debug is not available
      1     kernel option KASAN has not been enabled
      1     kernel option NVME_TARGET_DEBUGFS has not been enabled
      1     libnvme version is older than 1.11.0
      1     Remote target required but NVME_TARGET_CONTROL is not set

This is only a functional check. It says nothing about latency.

To apply your patch on v7.3 I had to drop the !nvme_tcp_queue_tls()
line as well, since 08acb54b063a ("nvme-tcp: defer TLS inline send to
io_work") is in v7.3. That commit already skips the direct send for
TLS queues, so the io_work only path is already in use there. Two
small things in the patch. empty is set but no longer used, which
warns with W=1, and nvme_tcp_send_all() loses its only caller. I
dropped both in the version I built.

I agree the direct send path is the source of this family of bugs,
5c11f7d9f843, a0fdd1418007, 70f437fb4395 and this one, and I am fine
with removing it. But I think the two should be kept apart. This
regression is in v6.16 and later, so it is in every current stable tree
and in the distro kernels based on them, and people are hitting it in
production now. What stable and the distros need is a small fix that
only changes behavior for those who hit the problem, and that is what
this patch is. Removing the direct send path changes performance for
everyone using nvme-tcp and requires a performance review before it is
introduced. Also, it belongs in the next merge window rather than in
stable.

So I would prefer to take this minimal fix now for v7.4 and stable, and
do the removal as a separate patch for-next. If you agree, I can write
and send that patch with your Suggested-by. For the performance side I
can only test on VMs, so latency numbers from a real nvme-tcp setup
would be needed from someone else. If later performance measurements
reveal that the direct send is unnecessary, the check added here goes
away with it, which is fine.

Thanks,
Seiji