sched/wait: Fix add_wait_queue() behavioral change

sched/wait: Fix add_wait_queue() behavioral change

[Tetsuo Handa]

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?

Re: sched/wait: Fix add_wait_queue() behavioral change

[Jens Axboe]

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

Re: sched/wait: Fix add_wait_queue() behavioral change

[Ingo Molnar]

* 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.git.osandov@fb.com
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);

Re: sched/wait: Fix add_wait_queue() behavioral change

[Linus Torvalds]

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

Re: sched/wait: Fix add_wait_queue() behavioral change

[Greg Kroah-Hartman]

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

Re: sched/wait: Fix add_wait_queue() behavioral change

[Jens Axboe]

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.

-- 
Jens Axboe