Re: RAID1 might_sleep() warning on 3.19-rc7

On Fri, 6 Feb 2015 12:39:30 +0100 Peter Zijlstra [off-list ref] wrote:

On Fri, Feb 06, 2015 at 08:51:33AM +1100, NeilBrown wrote:

quoted

That is exactly what is happening here.  However I don't think that is an
"observed problem" but rather an "observed false-positive".

If nothing inside the outer loop blocks, then in particular
generic_make_request will not be called, so nothing will be added to the
queue that blk_schedule_flush_plug flushes.
So the first time through the loop, a call the 'schedule()' may not actually
block, but every subsequent time it will.
So there is no actual problem here.

So I'd be included to add sched_annotate_sleep() in blk_flush_plug_list().

Peter: what do you think is the best way to silence this warning.

quoted

quoted

Call Trace:

quoted

quoted

 [<ffffffff8027ee62>] __might_sleep+0x82/0x90
 [<ffffffff803bee06>] generic_make_request_checks+0x36/0x2d0
 [<ffffffff803bf0b3>] generic_make_request+0x13/0x100
 [<ffffffff8054983b>] raid1_unplug+0x12b/0x170
 [<ffffffff803c1302>] blk_flush_plug_list+0xa2/0x230
 [<ffffffff80646383>] io_schedule+0x43/0x80
 [<ffffffff80646787>] bit_wait_io+0x27/0x50

Well, I don't know. I don't particularly like the whole blk_flush_plug()
thing scheduling while on its way to schedule. If you ever end up
calling io_schedule() from it there's 'fun'.

Also, how likely is it to actually schedule when doing all that? This
block layer stuff is somewhat impenetrable for me, too many callbacks.

You have some words on how its unlikely, but I can't even find _where_
it would schedule :/ All I see is a loop calling ->make_request_fn() and
god only knows where that ends up.

So there appear to be two blk_flush_plug() variants, one with an
@from_schedule = true, which seems to really try not to schedule, which
seems to suggest the 'false' one (the one above) is meant to schedule?

If scheduling is the rule rather than the exception, the above is
properly broken.

But again, I don't know.

I had to re-read the code (And your analysis) a couple of times to be sure ...

As you say, when schedule() calls blk_schedule_flush_plug(), the
@from_schedule=true variant is used and the unplug code doesn't block.
So there is no problem there.

However, when io_schedule() explicitly calls blk_flush_plug(), then
@from_schedule=false variant is used, and the unplug functions are allowed to
allocate memory and block and maybe even call mempool_alloc() which might
call io_schedule().

This shouldn't be a problem as blk_flush_plug() spliced out the plug list, so
any recursive call will find an empty list and do nothing.

Worst case is that a wait_event loop that calls io_schedule() (i.e.
wait_on_bit_io()) might not block in the first call to io_schedule()
if the unplugging needed to wait.  Every subsequent call will block as
required as there is nothing else to add requests to the plug queue.

So as long as wait_on_bio_io() can cope with a single false wakeup (which it
can), there is no problem here.

If you're confident that scheduling is rare for _ALL_ (current and
future) block device implementations, not just the raid one, then you
can annotate blk_flush_plug_list() I suppose.

Otherwise I would suggest adding them one at a time in whatever blk
device thing likes to go schedule on us. Also, add a comment that
explains why its rare for the future us who need to look at it again.

It isn't that scheduling is "rare" - it is that it can only occur once in a
loop which doesn't expect it.

So I propose the following, though I haven't tested it.

Signed-off-by: NeilBrown <redacted>

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e628cb11b560..b0f12ab3df23 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c

@@ -4374,6 +4374,11 @@ void __sched io_schedule(void)
 
 	delayacct_blkio_start();
 	atomic_inc(&rq->nr_iowait);
+	/* Any sleeping in blk_flush_plug() should not
+	 * trigger the "do not call blocking ops" warning
+	 * as it can only happen once in a wait_event loop.
+	 */
+	sched_annotate_sleep();
 	blk_flush_plug(current);
 	current->in_iowait = 1;
 	schedule();

@@ -4390,6 +4395,11 @@ long __sched io_schedule_timeout(long timeout)
 
 	delayacct_blkio_start();
 	atomic_inc(&rq->nr_iowait);
+	/* Any sleeping in blk_flush_plug() should not
+	 * trigger the "do not call blocking ops" warning
+	 * as it can only happen once in a wait_event loop.
+	 */
+	sched_annotate_sleep();
 	blk_flush_plug(current);
 	current->in_iowait = 1;
 	ret = schedule_timeout(timeout);