Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug()

From: Peter Zijlstra <peterz@infradead.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jens Axboe <axboe@fb.com>, Ingo Molnar <mingo@kernel.org>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Chris Mason <clm@fb.com>, Dave Chinner <david@fromorbit.com>,
	Jan Kara <jack@suse.cz>, Josef Bacik <jbacik@fb.com>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Neil Brown <neilb@suse.de>, Christoph Hellwig <hch@lst.de>,
	Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug()
Date: Fri, 18 Sep 2015 17:59:56 +0200	[thread overview]
Message-ID: <20150918155956.GZ3816@twins.programming.kicks-ass.net> (raw)
In-Reply-To: <CA+55aFznZFMHzGZr1ybY9wu_FuRJ-ZiJipuHM3brvHO7GybUpQ@mail.gmail.com>

On Fri, Sep 18, 2015 at 08:32:15AM -0700, Linus Torvalds wrote:

> Actually, even a *full* schedule doesn't unplug, unless the process is
> going to sleep. See sched_submit_work(), which  will only call the
> unplugging if the process is actually going to sleep (ok, so it's a
> bit subtle if you don't know the state rules, but it's the
> "!tsk->state" check there)
> 
> So preemption and cond_resched() isn't _that_ odd. We've basically
> treated a non-sleeping schedule as a no-op for the task work.
> 
> The thinking was probably that it might be better to delay starting
> the IO in case we get scheduled back quickly, and we're obviously not
> actually _sleeping_, so it's likely not too bad.
> 
> Now, that's probably bogus, and I think that we should perhaps just
> make the rule be that "if we actually switch to another task, we run
> blk_schedule_flush_plug()".

That gets to be a tad tricky. We must either do it while holding
scheduler locks (and I would really want to avoid calling block layer
code with those held) or when the new task is already scheduled (and
that would suck too, because then we account stuff to the wrong task).

> But it should be noted that that really *does* introduce a lot of new
> potential races. Traditionally, our block layer plugging has been
> entirely thread-synchronous, and would never happen asynchronously.
> But with preemption, that "switch to another thread" really *does*
> happen asynchronously.
> 
> So making things always happen on task switch is actually fairly
> dangerous, and potentially adds the need for much more synchronization
> for the IO submission.

Can't say I'm a fan of it :/

> What we possibly *could* make the scheduler rule be:
> 
>  - if it's not an actual PREEMPT_ACTIVE (ie in a random place)

PREEMPT_ACTIVE is actually a recursion flag, 'random place' does not
factor into it. That is, most 'random places' will not have
PREEMPT_ACTIVE set.

>  - _and_ we actually switch to another thread

This is 'hard', as per the above.

>  - _then_ do the whole blk_schedule_flush_plug(tsk) thing.
> 
> adding some scheduler people to the explicit cc list.
> 
> That said, the "cond_resched[_lock]()" functions currently always set
> PREEMPT_ACTIVE (indirectly - they use preempt_schedule_common()), so
> even though those are synchronous, right now they *look* asynchronous
> to the scheduler, so we'd still have to sort that out.

See PREEMPT_ACTIVE being a recursion flag, we set it there so we won't
preempt while we're already scheduling.