Re: [PATCH 2/2] scsi: core: Fix stall if two threads request budget at the same time

[Paolo Valente]

> Il giorno 1 apr 2020, alle ore 03:21, Jens Axboe <axboe@xxxxxxxxx> ha scritto:
> 
> On 3/31/20 5:51 PM, Doug Anderson wrote:
>> Hi,
>> 
>> On Tue, Mar 31, 2020 at 11:26 AM Jens Axboe <axboe@xxxxxxxxx> wrote:
>>> 
>>> On 3/31/20 12:07 PM, Paolo Valente wrote:
>>>>> Il giorno 31 mar 2020, alle ore 03:41, Ming Lei <ming.lei@xxxxxxxxxx> ha scritto:
>>>>> 
>>>>> On Mon, Mar 30, 2020 at 07:49:06AM -0700, Douglas Anderson wrote:
>>>>>> It is possible for two threads to be running
>>>>>> blk_mq_do_dispatch_sched() at the same time with the same "hctx".
>>>>>> This is because there can be more than one caller to
>>>>>> __blk_mq_run_hw_queue() with the same "hctx" and hctx_lock() doesn't
>>>>>> prevent more than one thread from entering.
>>>>>> 
>>>>>> If more than one thread is running blk_mq_do_dispatch_sched() at the
>>>>>> same time with the same "hctx", they may have contention acquiring
>>>>>> budget.  The blk_mq_get_dispatch_budget() can eventually translate
>>>>>> into scsi_mq_get_budget().  If the device's "queue_depth" is 1 (not
>>>>>> uncommon) then only one of the two threads will be the one to
>>>>>> increment "device_busy" to 1 and get the budget.
>>>>>> 
>>>>>> The losing thread will break out of blk_mq_do_dispatch_sched() and
>>>>>> will stop dispatching requests.  The assumption is that when more
>>>>>> budget is available later (when existing transactions finish) the
>>>>>> queue will be kicked again, perhaps in scsi_end_request().
>>>>>> 
>>>>>> The winning thread now has budget and can go on to call
>>>>>> dispatch_request().  If dispatch_request() returns NULL here then we
>>>>>> have a potential problem.  Specifically we'll now call
>>>>> 
>>>>> I guess this problem should be BFQ specific. Now there is definitely
>>>>> requests in BFQ queue wrt. this hctx. However, looks this request is
>>>>> only available from another loser thread, and it won't be retrieved in
>>>>> the winning thread via e->type->ops.dispatch_request().
>>>>> 
>>>>> Just wondering why BFQ is implemented in this way?
>>>>> 
>>>> 
>>>> BFQ inherited this powerful non-working scheme from CFQ, some age ago.
>>>> 
>>>> In more detail: if BFQ has at least one non-empty internal queue, then
>>>> is says of course that there is work to do.  But if the currently
>>>> in-service queue is empty, and is expected to receive new I/O, then
>>>> BFQ plugs I/O dispatch to enforce service guarantees for the
>>>> in-service queue, i.e., BFQ responds NULL to a dispatch request.
>>> 
>>> What BFQ is doing is fine, IFF it always ensures that the queue is run
>>> at some later time, if it returns "yep I have work" yet returns NULL
>>> when attempting to retrieve that work. Generally this should happen from
>>> subsequent IO completion, or whatever else condition will resolve the
>>> issue that is currently preventing dispatch of that request. Last resort
>>> would be a timer, but that can happen if you're slicing your scheduling
>>> somehow.
>> 
>> I've been poking more at this today trying to understand why the idle
>> timer that Paolo says is in BFQ isn't doing what it should be doing.
>> I've been continuing to put most of my stream-of-consciousness at
>> <https://crbug.com/1061950> to avoid so much spamming of this thread.
>> In the trace I looked at most recently it looks like BFQ does try to
>> ensure that the queue is run at a later time, but at least in this
>> trace the later time is not late enough.  Specifically the quick
>> summary of my recent trace:
>> 
>> 28977309us - PID 2167 got the budget.
>> 28977518us - BFQ told PID 2167 that there was nothing to dispatch.
>> 28977702us - BFQ idle timer fires.
>> 28977725us - We start to try to dispatch as a result of BFQ's idle timer.
>> 28977732us - Dispatching that was a result of BFQ's idle timer can't get
>>             budget and thus does nothing.
>> 28977780us - PID 2167 put the budget and exits since there was nothing
>>             to dispatch.
>> 
>> This is only one particular trace, but in this case BFQ did attempt to
>> rerun the queue after it returned NULL, but that ran almost
>> immediately after it returned NULL and thus ran into the race.  :(
> 
> OK, and then it doesn't trigger again? It's key that it keeps doing this
> timeout and re-dispatch if it fails, not just once.
> 

The goal of BFQ's timer is to make BFQ switch from non-work-conserving
to work-conserving mode, just because not doing so would cause a
stall.  In contrast, it sounds a little weird that an I/O scheduler
systematically kicks I/O periodically (how can BFQ know when no more
kicking is needed?).  IOW, it doesn't seem very robust that blk-mq may
need a series of periodic kicks to finally restart, like a flooded
engine.

Compared with this solution, I'd still prefer one where BFQ doesn't
trigger this blk-mq stall at all.

Paolo

> But BFQ really should be smarter here. It's the same caller etc that
> asks whether it has work and whether it can dispatch, yet the answer is
> different. That's just kind of silly, and it'd make more sense if BFQ
> actually implemented the ->has_work() as a "would I actually dispatch
> for this guy, now".
> 
>>>> It would be very easy to change bfq_has_work so that it returns false
>>>> in case the in-service queue is empty, even if there is I/O
>>>> backlogged.  My only concern is: since everything has worked with the
>>>> current scheme for probably 15 years, are we sure that everything is
>>>> still ok after we change this scheme?
>>> 
>>> You're comparing apples to oranges, CFQ never worked within the blk-mq
>>> scheduling framework.
>>> 
>>> That said, I don't think such a change is needed. If we currently have a
>>> hang due to this discrepancy between has_work and gets_work, then it
>>> sounds like we're not always re-running the queue as we should. From the
>>> original patch, the budget putting is not something the scheduler is
>>> involved with. Do we just need to ensure that if we put budget without
>>> having dispatched a request, we need to kick off dispatching again?
>> 
>> By this you mean a change like this in blk_mq_do_dispatch_sched()?
>> 
>>  if (!rq) {
>>    blk_mq_put_dispatch_budget(hctx);
>> +    ret = true;
>>    break;
>>  }
>> 
>> I'm pretty sure that would fix the problems and I'd be happy to test,
>> but it feels like a heavy hammer.  IIUC we're essentially going to go
>> into a polling loop and keep calling has_work() and dispatch_request()
>> over and over again until has_work() returns false or we manage to
>> dispatch something...
> 
> We obviously have to be careful not to introduce a busy-loop, where we
> just keep scheduling dispatch, only to fail.
> 
> -- 
> Jens Axboe