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

[Hannes Reinecke <hare@suse.de>]

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