Recently, we are seeing I/O hungup reports.
I don't know whether a regression introduced by commit 50816c48997af857 ("sched/wait: Standardize internal naming of wait-queue entries") is relevant. But shouldn't we backport commit c6b9d9a330290144 ("sched/wait: Fix add_wait_queue() behavioral change") to 4.13+ kernels anyway?
On 2/10/18 7:14 AM, Tetsuo Handa wrote:
Recently, we are seeing I/O hungup reports.
I don't know whether a regression introduced by commit 50816c48997af857 ("sched/wait: Standardize internal naming of wait-queue entries") is relevant. But shouldn't we backport commit c6b9d9a330290144 ("sched/wait: Fix add_wait_queue() behavioral change") to 4.13+ kernels anyway?
Yes, it most certainly should!
* Jens Axboe axboe@kernel.dk wrote:
On 2/10/18 7:14 AM, Tetsuo Handa wrote:
Recently, we are seeing I/O hungup reports.
I don't know whether a regression introduced by commit 50816c48997af857 ("sched/wait: Standardize internal naming of wait-queue entries") is relevant. But shouldn't we backport commit c6b9d9a330290144 ("sched/wait: Fix add_wait_queue() behavioral change") to 4.13+ kernels anyway?
Yes, it most certainly should!
Indeed, the bug was introduced in v4.13 and the fix was included in v4.15, but it's missing from v4.13 and v4.14 - not sure how I missed that.
The fix (also attached below) applies cleanly to both v4.13 and v4.14:
Acked-by: Ingo Molnar mingo@kernel.org
Thanks,
Ingo
===================>
From c6b9d9a33029014446bd9ed84c1688f6d3d4eab9 Mon Sep 17 00:00:00 2001
From: Omar Sandoval osandov@fb.com Date: Tue, 5 Dec 2017 23:15:31 -0800 Subject: [PATCH] sched/wait: Fix add_wait_queue() behavioral change
The following cleanup commit:
50816c48997a ("sched/wait: Standardize internal naming of wait-queue entries")
... unintentionally changed the behavior of add_wait_queue() from inserting the wait entry at the head of the wait queue to the tail of the wait queue.
Beyond a negative performance impact this change in behavior theoretically also breaks wait queues which mix exclusive and non-exclusive waiters, as non-exclusive waiters will not be woken up if they are queued behind enough exclusive waiters.
Signed-off-by: Omar Sandoval osandov@fb.com Reviewed-by: Jens Axboe axboe@kernel.dk Acked-by: Peter Zijlstra peterz@infradead.org Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Thomas Gleixner tglx@linutronix.de Cc: kernel-team@fb.com Fixes: ("sched/wait: Standardize internal naming of wait-queue entries") Link: http://lkml.kernel.org/r/a16c8ccffd39bd08fdaa45a5192294c784b803a7.1512544324... Signed-off-by: Ingo Molnar mingo@kernel.org --- kernel/sched/wait.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c index 98feab7933c7..929ecb7d6b78 100644 --- a/kernel/sched/wait.c +++ b/kernel/sched/wait.c @@ -27,7 +27,7 @@ void add_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq
wq_entry->flags &= ~WQ_FLAG_EXCLUSIVE; spin_lock_irqsave(&wq_head->lock, flags); - __add_wait_queue_entry_tail(wq_head, wq_entry); + __add_wait_queue(wq_head, wq_entry); spin_unlock_irqrestore(&wq_head->lock, flags); } EXPORT_SYMBOL(add_wait_queue);
On Sun, Feb 11, 2018 at 2:32 AM, Ingo Molnar mingo@kernel.org wrote:
Indeed, the bug was introduced in v4.13 and the fix was included in v4.15, but it's missing from v4.13 and v4.14 - not sure how I missed that.
Sadly, this would actually have been picked up automatically, except for this other problem:
Fixes: ("sched/wait: Standardize internal naming of wait-queue entries")
If that had instead said
Fixes: 50816c48997a ("sched/wait: Standardize internal naming of wait-queue entries")
then I think Greg's stable scripts would have noticed this even without any Cc: stable.
But the commit ID for the thing it fixed was only in the body of the email, not in that Fixes: line, so together with the lack of stable cc it got entirely missed.
That said, the block layer doesn't use exclusive waits very much, so I'm not sure how much this can trigger. So any block IO hangs don't sound likely to be due to this. The only exclusive waits I can find are:
- regular request allocation.
But *all* the waiters are exclusive, so this won't affect it
- blk-mq-tag.c uses
prepare_to_wait_exclusive(&ws->wait, ,,
But all the _wakers_ seem to do "wake_up_all()", so again the order on the wait list shouldn't matter.
but maybe I'm missing something.
Regardless, that commit should be back-ported, but I do get the feeling that it never really ended up being something that people could possibly trigger in practice.
Linus
On Sun, Feb 11, 2018 at 11:32:03AM +0100, Ingo Molnar wrote:
- Jens Axboe axboe@kernel.dk wrote:
On 2/10/18 7:14 AM, Tetsuo Handa wrote:
Recently, we are seeing I/O hungup reports.
I don't know whether a regression introduced by commit 50816c48997af857 ("sched/wait: Standardize internal naming of wait-queue entries") is relevant. But shouldn't we backport commit c6b9d9a330290144 ("sched/wait: Fix add_wait_queue() behavioral change") to 4.13+ kernels anyway?
Yes, it most certainly should!
Indeed, the bug was introduced in v4.13 and the fix was included in v4.15, but it's missing from v4.13 and v4.14 - not sure how I missed that.
The fix (also attached below) applies cleanly to both v4.13 and v4.14:
Now queued up, thanks.
greg k-h
On 2/11/18 11:28 AM, Linus Torvalds wrote:
On Sun, Feb 11, 2018 at 2:32 AM, Ingo Molnar mingo@kernel.org wrote:
Indeed, the bug was introduced in v4.13 and the fix was included in v4.15, but it's missing from v4.13 and v4.14 - not sure how I missed that.
Sadly, this would actually have been picked up automatically, except for this other problem:
Fixes: ("sched/wait: Standardize internal naming of wait-queue entries")
If that had instead said
Fixes: 50816c48997a ("sched/wait: Standardize internal naming of wait-queue entries")
then I think Greg's stable scripts would have noticed this even without any Cc: stable.
But the commit ID for the thing it fixed was only in the body of the email, not in that Fixes: line, so together with the lack of stable cc it got entirely missed.
That said, the block layer doesn't use exclusive waits very much, so I'm not sure how much this can trigger. So any block IO hangs don't sound likely to be due to this. The only exclusive waits I can find are:
regular request allocation.
But *all* the waiters are exclusive, so this won't affect it
blk-mq-tag.c uses
prepare_to_wait_exclusive(&ws->wait, ,,But all the _wakers_ seem to do "wake_up_all()", so again the order
on the wait list shouldn't matter.
but maybe I'm missing something.
Regardless, that commit should be back-ported, but I do get the feeling that it never really ended up being something that people could possibly trigger in practice.
Newer kernels do use them without wake_up_all(), but then he would not be hitting the regression here. It is possible that he's seeing hangs that aren't at the end of the IO stack, just somewhere else. Or it could be that it's something else entirely...
In any case, it's prudent to back port the fix to the kernels that don't have it.
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Ingo Molnar -
Jens Axboe -
Linus Torvalds -
Tetsuo Handa