Re: RAID1 might_sleep() warning on 3.19-rc7

[Peter Zijlstra <peterz@infradead.org>]

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.

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?