[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