linux-kernel - Re: RAID1 might_sleep() warning on 3.19-rc7

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150210135017.7659e49c@notabene.brown>
Date:	Tue, 10 Feb 2015 13:50:17 +1100
From:	NeilBrown <neilb@...e.de>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Tony Battersby <tonyb@...ernetics.com>, linux-raid@...r.kernel.org,
	lkml <linux-kernel@...r.kernel.org>, axboe@...nel.dk,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: RAID1 might_sleep() warning on 3.19-rc7

On Mon, 9 Feb 2015 10:10:00 +0100 Peter Zijlstra <peterz@...radead.org> wrote:

> On Mon, Feb 09, 2015 at 12:13:57PM +1100, NeilBrown wrote:
> > I had to re-read the code (And your analysis) a couple of times to be sure ...
> 
> Sorry :-)

My point was that actually reading it (rather than assuming I knew what it
said) actually helped!

> 
> > 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.
> 
> Unless, something along the way stuck something back on, right? So
> should we stick an:
> 
> 	WARN_ON(current->in_iowait);
> 
> somewhere near where things are added to this plug list? (and move the
> blk_flush_plug() call inside of where that's actually true of course).

No, I don't think so.

It is certainly possible that some request on plug->cb_list could add
something to plug->list - which is processed after ->cb_list.

I think the best way to think about this is that the *problem* was that a
wait_event loop could spin without making any progress.   So any time that
clear forward progress is made it is safe sleep without necessitating the
warning.  Hence sched_annotate_sleep() is reasonable.
blk_flush_plug() with definitely have dispatched some requests if it
might_sleep(), so the sleep is OK.


> 
> The only thing that really goes wrong then is if people 'forget' to put
> a loop around io_schedule().

You mean like in congestion_wait() ??

Though that is mostly called inside a loop...


NeilBrown

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 92f4b4b288dd..7334be27823d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1111,6 +1111,14 @@ static inline void blk_flush_plug(struct task_struct *tsk)
 {
 	struct blk_plug *plug = tsk->plug;
 
+	/*
+	 * Any sleeping in blk_flush_plug() should not
+	 * trigger the "do not call blocking ops" warning
+	 * as it makes clear forward process (requests are
+	 * dispatched) and so it will not cause indefinite
+	 * looping in a higher level wait loop.
+	 */
+	sched_annotate_sleep();
 	if (plug)
 		blk_flush_plug_list(plug, false);
 }

Content of type "application/pgp-signature" skipped