Re: [PATCH 05/14] blk-mq-sched: don't dequeue request until all in ->dispatch are flushed

[Ming Lei <ming.lei@redhat.com>]

On Tue, Aug 01, 2017 at 04:14:07PM +0000, Bart Van Assche wrote:
> On Tue, 2017-08-01 at 18:44 +0800, Ming Lei wrote:
> > On Mon, Jul 31, 2017 at 11:42:21PM +0000, Bart Van Assche wrote:
> > > Since setting, clearing and testing of BLK_MQ_S_BUSY can happen concurrently
> > > and since clearing and testing happens without any locks held I'm afraid this
> > > patch introduces the following race conditions:
> > > [ ... ]
> > > * Checking BLK_MQ_S_BUSY after requests have been removed from the dispatch list
> > >   but before that bit is cleared, resulting in test_bit(BLK_MQ_S_BUSY, &hctx->state)
> > >   reporting that the BLK_MQ_S_BUSY
> > > has been set although there are no requests
> > >   on the dispatch list.
> > 
> > That won't be a problem, because dispatch will be started in the
> > context in which dispatch list is flushed, since the BUSY bit
> > is cleared after blk_mq_dispatch_rq_list() returns. So no I/O
> > hang.
> 
> Hello Ming,
> 
> Please consider changing the name of the BLK_MQ_S_BUSY constant. That bit
> is used to serialize dispatching requests from the hctx dispatch list but
> that's not clear from the name of that constant.

Actually what we want to do is to stop taking request from sw/scheduler
queue when ->dispatch aren't flushed completely, I think BUSY isn't
a bad name for this case, or how about DISPATCH_BUSY? or
FLUSHING_DISPATCH?

After thinking about the handling further, we can set the
BUSY bit just when adding requests to ->dispatch, and clear the
bit after returning from blk_mq_dispatch_rq_list() when the
current local list(from ->dispatch) is flushed completely and
->dispatch is empty. This way can minimize the race window, and
still safe, because we always move on to dispatch either new
request is added to ->dispatch or ->dispatch is flushed completely.

But anyway comment should be added for clarifying the fact.

-- 
Ming