Re: [PATCH] cfq: priority boost on meta/prio marked IO

[Jens Axboe <axboe@kernel.dk>]

On 06/09/2016 12:31 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
>> On 06/09/2016 09:55 AM, Jeff Moyer wrote:
>>> Hi, Jens,
>>>
>>> Jens Axboe <axboe@fb.com> writes:
>>>
>>>> At Facebook, we have a number of cases where people use ionice to set a
>>>> lower priority, then end up having tasks stuck for a long time because
>>>> eg meta data updates from an idle priority tasks is blocking out higher
>>>> priority processes. It's bad enough that it will trigger the softlockup
>>>> warning.
>>>
>>> I expect a higher prio process could be blocked on a lower prio process
>>> reading the same metadata, too.  I had a hard time tracking down where
>>> REQ_META WRITE I/O was issued outside of the journal or writeback paths
>>> (and I hope you're not ionice-ing those!).  Eventually, with the help of
>>> sandeen, I found some oddball cases that I doubt you're running into.
>>> Can you enlighten me as to where this (REQ_META write I/O) is happening?
>>> I don't disagree that it's a problem, I just would like to understand
>>> your problem case better.
>>>
>>> Anyway, it seems to me you could just set REQ_PRIO in the code paths you
>>> care about instead of modifying CFQ to treat REQ_META and REQ_PRIO as
>>> the same thing, which essentially undoes commit 65299a3b788bd ("block:
>>> separate priority boosting from REQ_META") from Christoph.
>>
>> I personally don't care too much about boosting for just PRIO, or for
>> both PRIO and META. For the important paths, we basically have both set
>> anyway.
>
> Yeah, I had considered that.  I like that REQ_META is separated out for
> logging purposes.  I'm not too concerned about whether we imply boosted
> priority from that or not.

Not a huge deal to me either, and most of the interesting REQ_META sites 
already set REQ_PRIO as well.

>> These are reads.
>
> I was curious about writes.  ;-)  Anyway, it's good to validate that the
> read case is also relevant.

You mean O_DIRECT writes? Most of the buffered writes will come out of 
the associated threads, so I don't think it's a big of an issue there. 
We've only seen it for reads.

>> It's a classic case of starting some operation that ends up holding a
>> file system resource, then making very slow progress (due to task
>> being scheduled as idle IO), and hence blocking out potentially much
>> important tasks.
>>
>> The IO is marked META|PRIO, so if folks don't [care] for boosting on meta,
>> we can just kill that. "Normal" meta data could be scheduled as idle,
>> but anything scheduled under a fs lock or similar should be marked PRIO
>> and get boosted.
>
> Interesting.  I would have thought that the cfqd->active_queue would
> have been preempted by a request marked with REQ_PRIO.  But you're
> suggesting that did not happen?
>
> Specifically, cfq_insert_request would call cfq_rq_enqueued, and that
> would call cfq_preempt_queue if cfq_should_preempt (and I think it
> should, since the new request has REQ_PRIO set).

We seem to handily mostly ignore prio_pending for the idle class. If the 
new queue is idle, then we don't look at prio pending. I'd rather make 
this more explicit, the patch is pretty similar to what we had in the 
past. It's somewhat of a regression caused by commit 4aede84b33d, except 
I like using the request flags for this a lot more than the old 
current->fs_excl. REQ_PRIO should always be set for cases where we hold 
fs (or even directory) specific resources.


-- 
Jens Axboe