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

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

[Seiji Nishikawa]

Commit f42d4796ee10 ("nvme-tcp: fix I/O stalls on congested sockets")
added a check in nvme_tcp_io_work() that sets pending to true when the
queue has pending requests and the socket is writeable. The check is
placed after the mutex_trylock() block, so it also runs when
mutex_trylock(&queue->send_mutex) failed and io_work did not send
anything.

When .queue_rq sends a request directly from its own context, it holds
send_mutex and can give up the CPU in lock_sock_nested() when another
task needs the CPU, for example the io_work kworker of the same queue,
which runs on the same CPU. Then io_work cannot take send_mutex, but
the requests are still on req_list and the socket is writeable, so
pending is set to true. io_work loops until the deadline and queues
itself again on io_cpu, without sleeping.

This is a priority inversion. io_work runs on the high priority
nvme_tcp_wq and cannot make progress until send_mutex is released, but
it keeps running. The task holding send_mutex runs at normal priority
and needs the CPU to release it. The scheduler keeps giving the CPU to
io_work, so the task holding send_mutex may not run again, and the
requests on the queue are not sent and time out.

This is the same problem that commit 70f437fb4395 ("nvme-tcp: fix
io_work priority inversion") fixed. That commit removed
"pending = !llist_empty(&queue->req_list)" from the path where
mutex_trylock() fails, and let the task holding send_mutex queue
io_work again in nvme_tcp_queue_request() after it releases the mutex.
The new check brings back the same behavior in a different form.

Two things make this easier to hit. nvme_tcp_queue_request() sends
directly only when it runs on io_cpu, so the task that takes send_mutex
is on the same CPU as the io_work kworker of that queue when it takes
the mutex. Also, when the tasks are in a cgroup with the cpu controller
enabled, the whole group competes with the nice -20 kworker as one
entity, so each task in the group gets even less CPU time.

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.

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.

Fixes: f42d4796ee10 ("nvme-tcp: fix I/O stalls on congested sockets")
Cc: stable@vger.kernel.org # v6.16+
Signed-off-by: Seiji Nishikawa <snishika@redhat.com>
---
 drivers/nvme/host/tcp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 7eec983c0ead..4b775e22d3cc 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1433,9 +1433,11 @@ static void nvme_tcp_io_work(struct work_struct *w)
 
 	do {
 		bool pending = false;
+		bool got_send_mutex = false;
 		int result;
 
 		if (mutex_trylock(&queue->send_mutex)) {
+			got_send_mutex = true;
 			result = nvme_tcp_try_send(queue);
 			mutex_unlock(&queue->send_mutex);
 			if (result > 0)
@@ -1450,8 +1452,13 @@ static void nvme_tcp_io_work(struct work_struct *w)
 		else if (unlikely(result < 0))
 			return;
 
-		/* did we get some space after spending time in recv? */
-		if (nvme_tcp_queue_has_pending(queue) &&
+		/*
+		 * did we get some space after spending time in recv?
+		 * Only check this if we could take send_mutex. Otherwise
+		 * the task holding it sends the requests and queues io_work
+		 * again after it releases the mutex.
+		 */
+		if (got_send_mutex && nvme_tcp_queue_has_pending(queue) &&
 		    sk_stream_is_writeable(queue->sock->sk))
 			pending = true;
 

base-commit: 5755f1bd0217ffe3c5cf85467bf6b0680f398265
-- 
2.55.0

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

[Hannes Reinecke]

On 10/14/26 8:45 AM, Seiji Nishikawa wrote:
> Commit f42d4796ee10 ("nvme-tcp: fix I/O stalls on congested sockets")
> added a check in nvme_tcp_io_work() that sets pending to true when the
> queue has pending requests and the socket is writeable. The check is
> placed after the mutex_trylock() block, so it also runs when
> mutex_trylock(&queue->send_mutex) failed and io_work did not send
> anything.
> 
> When .queue_rq sends a request directly from its own context, it holds
> send_mutex and can give up the CPU in lock_sock_nested() when another
> task needs the CPU, for example the io_work kworker of the same queue,
> which runs on the same CPU. Then io_work cannot take send_mutex, but
> the requests are still on req_list and the socket is writeable, so
> pending is set to true. io_work loops until the deadline and queues
> itself again on io_cpu, without sleeping.
> 
> This is a priority inversion. io_work runs on the high priority
> nvme_tcp_wq and cannot make progress until send_mutex is released, but
> it keeps running. The task holding send_mutex runs at normal priority
> and needs the CPU to release it. The scheduler keeps giving the CPU to
> io_work, so the task holding send_mutex may not run again, and the
> requests on the queue are not sent and time out.
> 
> This is the same problem that commit 70f437fb4395 ("nvme-tcp: fix
> io_work priority inversion") fixed. That commit removed
> "pending = !llist_empty(&queue->req_list)" from the path where
> mutex_trylock() fails, and let the task holding send_mutex queue
> io_work again in nvme_tcp_queue_request() after it releases the mutex.
> The new check brings back the same behavior in a different form.
> 
> Two things make this easier to hit. nvme_tcp_queue_request() sends
> directly only when it runs on io_cpu, so the task that takes send_mutex
> is on the same CPU as the io_work kworker of that queue when it takes
> the mutex. Also, when the tasks are in a cgroup with the cpu controller
> enabled, the whole group competes with the nice -20 kworker as one
> entity, so each task in the group gets even less CPU time.
> 
> 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?

> 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);
  }

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Seiji Nishikawa]

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