Re: RAID1 might_sleep() warning on 3.19-rc7

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3876 bytes --]

On Fri, 13 Feb 2015 09:32:50 +0100 Peter Zijlstra <peterz@infradead.org>
wrote:

> On Fri, Feb 13, 2015 at 04:26:00PM +1100, NeilBrown wrote:
> > I choose ... Buzz Lightyear !!!
> 
> Great choice!
> 
> > From: NeilBrown <neilb@suse.de>
> > Date: Fri, 13 Feb 2015 15:49:17 +1100
> > Subject: [PATCH] sched: prevent recursion in io_schedule()
> > 
> > io_schedule() calls blk_flush_plug() which, depending on the
> > contents of current->plug, can initiate arbitrary blk-io requests.
> > 
> > Note that this contrasts with blk_schedule_flush_plug() which requires
> > all non-trivial work to be handed off to a separate thread.
> > 
> > This makes it possible for io_schedule() to recurse, and initiating
> > block requests could possibly call mempool_alloc() which, in times of
> > memory pressure, uses io_schedule().
> > 
> > Apart from any stack usage issues, io_schedule() will not behave
> > correctly when called recursively as delayacct_blkio_start() does
> > not allow for repeated calls.
> 
> Which seems to still be an issue with this patch.
> 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 1f37fe7f77a4..90f3de8bc7ca 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -4420,30 +4420,27 @@ EXPORT_SYMBOL_GPL(yield_to);
> >   */
> >  void __sched io_schedule(void)
> >  {
> > +	io_schedule_timeout(MAX_SCHEDULE_TIMEOUT);
> >  }
> >  EXPORT_SYMBOL(io_schedule);
> 
> Might as well move it to sched.h as an inline or so..
> 
> >  long __sched io_schedule_timeout(long timeout)
> >  {
> > +	struct rq *rq;
> >  	long ret;
> > +	int old_iowait = current->in_iowait;
> > +
> > +	current->in_iowait = 1;
> > +	if (old_iowait)
> > +		blk_schedule_flush_plug(current);
> > +	else
> > +		blk_flush_plug(current);
> >  
> >  	delayacct_blkio_start();
> > +	rq = raw_rq();
> >  	atomic_inc(&rq->nr_iowait);
> >  	ret = schedule_timeout(timeout);
> > +	current->in_iowait = old_iowait;
> >  	atomic_dec(&rq->nr_iowait);
> >  	delayacct_blkio_end();
> >  	return ret;
> 
> Like said, that will still recursive call delayacct_blkio_*() and would
> increase nr_iowait for a second time; while arguably its still the same
> one io-wait instance.

No it doesn't.  There is no "blk_flush_plug" call between the
delayacct_blkio_*() calls.

I've moved blk_flush_plug to the beginning of the function.

> 
> So would a little something like:
> 
> long __sched io_schedule_timeout(long timeout)
> {
> 	struct rq *rq;
> 	long ret;
> 
> 	/*
> 	 * Recursive io_schedule() call; make sure to not recurse
> 	 * on the blk_flush_plug() stuff again.
> 	 */
> 	if (unlikely(current->in_iowait)) {
> 		/*
> 		 * Our parent io_schedule() call will already have done
> 		 * all the required io-wait accounting.
> 		 */
> 		blk_schedule_flush_plug(current);
> 		return schedule_timeout(timeout);
> 	}
> 
> 	current->in_iowait = 1;
> 	delayacct_blkio_start();
> 	rq = raw_rq();
> 	atomic_inc(&rq->nr_iowait);
> 	blk_flush_plug(current);
> 	ret = schedule_timeout(timeout);
> 	atomic_dec(&rq->nr_iowait);
> 	delayacct_blkio_end();
> 	current->in_iowait = 0;
> 
> 	return ret;
> }
> 
> not make more sense? 

That does make a similar amount of sense at least....

I wondered if it really make sense to call blk_flush_plug with nr_iowait
elevated and delayacct_blkio active.  blk_flush_plug() could call schedule()
for non-"io" reasons and maybe that could upset stuff???

I don't really know.  I'm happy with your version.  I don't suppose anyone
else is paying attention and could give a third opinion....

Thanks,
NeilBrown



> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]