On Fri, 6 Feb 2015 12:39:30 +0100 Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > On Fri, Feb 06, 2015 at 08:51:33AM +1100, NeilBrown wrote: > > 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. > > > > Call Trace: > > > > [<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 <neilb@xxxxxxx> 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);
Attachment:
pgpeSusQI2UuP.pgp
Description: OpenPGP digital signature
