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

[Jens Axboe <axboe@kernel.dk>]

On 06/09/2016 03:47 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
>> On 06/09/2016 03:28 PM, Jeff Moyer wrote:
>>> 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.
>>>>
>>>> This patch adds code to CFQ that bumps the priority class and data for
>>>> an idle task, if is doing IO marked as PRIO or META. With this, we no
>>>> longer see the softlockups.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@fb.com>
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 32a283eb7274..3cfd67d006fb 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -1781,6 +1781,11 @@ get_rq:
>>>>    		rw_flags |= REQ_SYNC;
>>>>
>>>>    	/*
>>>> +	 * Add in META/PRIO flags, if set, before we get to the IO scheduler
>>>> +	 */
>>>> +	rw_flags |= (bio->bi_rw & (REQ_META | REQ_PRIO));
>>>> +
>>>> +	/*
>>>
>>> This needs a docbook update.  It now reads:
>>>
>>>    * @rw_flags: RW and SYNC flags
>>>
>>> so whatever flags we're adding should be specified, I guess.
>>>
>>> Speaking of which, after much waffling, I think I've decided it would be
>>> cleaner to limit the priority boost to REQ_PRIO requests only.
>>
>> I went and checked, but I don't see it. Where is this?
>
> Oops, sorry.  I meant that get_request and __get_request need updates to
> their documentation.

See followup email, that no longer applies!

> On the second part (in case there was confusion on what I meant there),
> what I meant was only do the prio boost for REQ_PRIO requests instead
> of also doing it for REQ_META.  The way I arrived at that conclusion was
> when I was going to ask you to update the documentation for REQ_META to
> state that it implied REQ_PRIO, at which point, one has to wonder why we
> need two flags.
>
> There are cases where REQ_PRIO is used without REQ_META.
> There are cases where REQ_META is used withoug REQ_PRIO.
> And of course, there are cases where they're both sent down.
>
> REQ_META itself is useful for tracing, and also makes the code
> self-documenting.
>
> REQ_PRIO pretty clearly means that we should boost priority for this
> request.  And I think Christoph was making a case for REQ_META that
> doesn't require a priority boost (if I read what he said correctly).
>
> So, I think they serve different purposes.  Have I convinced you?  It'll
> make your patch smaller!  ;-)

I got what you meant, the updated patch in that same followup email has 
it cut down to just REQ_PRIO and drops RQ_PRIO_MASK as a result.

-- 
Jens Axboe