On Fri, 13 Feb 2015 09:32:50 +0100 Peter Zijlstra <peterz@xxxxxxxxxxxxx>
wrote:
> On Fri, Feb 13, 2015 at 04:26:00PM +1100, NeilBrown wrote:
> > I choose ... Buzz Lightyear !!!
>
> Great choice!
>
> > From: NeilBrown <neilb@xxxxxxx>
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
pgpundLZLKFlT.pgp
Description: OpenPGP digital signature
