cfq: priority boost on meta/prio marked IO

Commit Message

Jens Axboe June 9, 2016, 9:41 p.m. UTC

On 06/09/2016 03:36 PM, Jens Axboe wrote:
> On 06/09/2016 03:28 PM, Jeff Moyer wrote:
>> Jens Axboe <[e-mail]> 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 <[e-mail]>
>>>
>>> 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?

Ah now I see, you're looking at current -git. The patch is against
for-4.8/core.

Updated version below, dropping REQ_META and changing the naming
s/meta/prio.


@@ -4295,6 +4296,20 @@ static void cfq_completed_request(struct 
request_queue *q, struct request *rq)
  		cfq_schedule_dispatch(cfqd);
  }

+static void cfqq_boost_on_prio(struct cfq_queue *cfqq, int op_flags)
+{
+	if (!(op_flags & REQ_PRIO)) {
+		cfqq->ioprio_class = cfqq->org_ioprio_class;
+		cfqq->ioprio = cfqq->org_ioprio;
+		return;
+	}
+
+	if (cfq_class_idle(cfqq))
+		cfqq->ioprio_class = IOPRIO_CLASS_BE;
+	if (cfqq->ioprio > IOPRIO_NORM)
+		cfqq->ioprio = IOPRIO_NORM;
+}
+
  static inline int __cfq_may_queue(struct cfq_queue *cfqq)
  {
  	if (cfq_cfqq_wait_request(cfqq) && !cfq_cfqq_must_alloc_slice(cfqq)) {
@@ -4325,6 +4340,7 @@ static int cfq_may_queue(struct request_queue *q, 
int op, int op_flags)
  	cfqq = cic_to_cfqq(cic, rw_is_sync(op, op_flags));
  	if (cfqq) {
  		cfq_init_prio_data(cfqq, cic);
+		cfqq_boost_on_prio(cfqq, op_flags);

  		return __cfq_may_queue(cfqq);
  	}

Comments

Jeff Moyer June 9, 2016, 10:04 p.m. UTC | #1

Jens Axboe <[e-mail]> writes:

>> I went and checked, but I don't see it. Where is this?
>
> Ah now I see, you're looking at current -git. The patch is against
> for-4.8/core.

Ah, right, Mike's patches went in.

> Updated version below, dropping REQ_META and changing the naming
> s/meta/prio.
>
> 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));
> +
> +	/*

Do we still need to pass in META here?

-Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jens Axboe June 9, 2016, 10:05 p.m. UTC | #2

On 06/09/2016 04:04 PM, Jeff Moyer wrote:
> Jens Axboe <[e-mail]> writes:
>
>>> I went and checked, but I don't see it. Where is this?
>>
>> Ah now I see, you're looking at current -git. The patch is against
>> for-4.8/core.
>
> Ah, right, Mike's patches went in.
>
>> Updated version below, dropping REQ_META and changing the naming
>> s/meta/prio.
>>
>> 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));
>> +
>> +	/*
>
> Do we still need to pass in META here?

We don't have to, but it doesn't really hurt. Frankly, we should pass in 
the whole damn thing.

Jeff Moyer June 9, 2016, 10:08 p.m. UTC | #3

Jens Axboe <[e-mail]> writes:

> On 06/09/2016 04:04 PM, Jeff Moyer wrote:
>> Jens Axboe <[e-mail]> writes:
>>
>>>> I went and checked, but I don't see it. Where is this?
>>>
>>> Ah now I see, you're looking at current -git. The patch is against
>>> for-4.8/core.
>>
>> Ah, right, Mike's patches went in.
>>
>>> Updated version below, dropping REQ_META and changing the naming
>>> s/meta/prio.
>>>
>>> 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));
>>> +
>>> +	/*
>>
>> Do we still need to pass in META here?
>
> We don't have to, but it doesn't really hurt. Frankly, we should pass
> in the whole damn thing.

Heh, okay.

Fine by me, Jens.  :)

Reviewed-by: Jeff Moyer <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jens Axboe June 9, 2016, 10:15 p.m. UTC | #4

On 06/09/2016 04:08 PM, Jeff Moyer wrote:
> Jens Axboe <[e-mail]> writes:
>
>> On 06/09/2016 04:04 PM, Jeff Moyer wrote:
>>> Jens Axboe <[e-mail]> writes:
>>>
>>>>> I went and checked, but I don't see it. Where is this?
>>>>
>>>> Ah now I see, you're looking at current -git. The patch is against
>>>> for-4.8/core.
>>>
>>> Ah, right, Mike's patches went in.
>>>
>>>> Updated version below, dropping REQ_META and changing the naming
>>>> s/meta/prio.
>>>>
>>>> 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));
>>>> +
>>>> +	/*
>>>
>>> Do we still need to pass in META here?
>>
>> We don't have to, but it doesn't really hurt. Frankly, we should pass
>> in the whole damn thing.
>
> Heh, okay.
>
> Fine by me, Jens.  :)
>
> Reviewed-by: Jeff Moyer <[e-mail]>

Thanks Jeff, added for 4.8.

Patch

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));
+
+	/*
  	 * Grab a free request. This is might sleep but can not fail.
  	 * Returns with the queue unlocked.
  	 */
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 4e5978426ee7..f2955c41a306 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -141,7 +141,7 @@  struct cfq_queue {

  	/* io prio of this group */
  	unsigned short ioprio, org_ioprio;
-	unsigned short ioprio_class;
+	unsigned short ioprio_class, org_ioprio_class;

  	pid_t pid;

@@ -3700,6 +3700,7 @@  static void cfq_init_prio_data(struct cfq_queue 
*cfqq, struct cfq_io_cq *cic)
  	 * elevate the priority of this queue
  	 */
  	cfqq->org_ioprio = cfqq->ioprio;
+	cfqq->org_ioprio_class = cfqq->ioprio_class;
  	cfq_clear_cfqq_prio_changed(cfqq);
  }