Re: RAID1 might_sleep() warning on 3.19-rc7

[NeilBrown]

On Mon, 9 Feb 2015 10:10:00 +0100 Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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);
 }

Attachment: pgpZXtrcHYVEI.pgp
Description: OpenPGP digital signature