On Mon, 9 Feb 2015 10:10:00 +0100 Peter Zijlstra <pet...@infradead.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);
}
pgphUr1YcsQR8.pgp
Description: OpenPGP digital signature
