Re: [PATCH] Don't wait if queue already has requests.

[Justin TerAvest]

On Tue, Feb 8, 2011 at 11:48 AM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Tue, Feb 08, 2011 at 11:18:17AM -0800, Justin TerAvest wrote:
>> Commit 7667aa0630407bc07dc38dcc79d29cc0a65553c1 added logic to wait for
>> the last queue of the group to become busy (have at least one request),
>> so that the group does not lose out for not being continuously
>> backlogged. The commit did not check for the condition that the last
>> queue already has some requests. As a result, if the queue already has
>> requests, wait_busy is set. Later on, cfq_select_queue() checks the
>> flag, and decides that since the queue has a request now and wait_busy
>> is set, the queue is expired.  This results in early expiration of the
>> queue.
>
> Hi Justin,
>
> wait_busy will be set only if slice has expired or about to be expired. So
> even if we are setting wait_busy flag, it is not a huge deal even if
> select_queue() expires it? Anyway queue has consumed or almost consumed
> its allocated slice?

Correct, the queue will have consumed or almost consumed its allocated
slice. However, this must be happening often enough because there is a
measurable impact in our testing.

>
> Having said that, it does not make sense to set wait_busy flag if
> cfqq has requests. So I would be fine with the patch. I am just
> curious that how did you see a difference in practice.

In practice,
  - We see some better isolation between tasks with this patch alone
(Possibly those writes are getting marked SYNC somehow, or it's
another timing change)
  - With other patches that introduce isolation between buffered
writes, we see that isolation works better, especially for cgroups
with small weights.

>
>>
>> This patch fixes the problem by adding a check to see if queue already
>> has requests. If it does, wait_busy is not set. As a result, time slices
>> do not expire early.
>>
>> The queues with more than one request are usually buffered writers.
>> Testing shows improvement in isolation between buffered writers.
>
> Upstream code puts all the buffered WRITES in root cgroup. So there
> is no isolation between buffered WRITES?

This was a mistake on my part, we have other patches that add
isolation between buffered writes; I just took this one out of order.
If you'd like, we can hold on this patch for now and I can resend this
later, but I think the actual patch itself is still good.

My apologies for the confusion.

Thanks,
Justin

>
> Thanks
> Vivek
>
>>
>> Signed-off-by: Justin TerAvest <teravest@xxxxxxxxxx>
>> ---
>>  block/cfq-iosched.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> index 501ffdf..5dcc353 100644
>> --- a/block/cfq-iosched.c
>> +++ b/block/cfq-iosched.c
>> @@ -3432,6 +3432,10 @@ static bool cfq_should_wait_busy(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>>  {
>>       struct cfq_io_context *cic = cfqd->active_cic;
>>
>> +     /* If the queue already has requests, don't wait */
>> +     if (!RB_EMPTY_ROOT(&cfqq->sort_list))
>> +             return false;
>> +
>>       /* If there are other queues in the group, don't wait */
>>       if (cfqq->cfqg->nr_cfqq > 1)
>>               return false;
>> --
>> 1.7.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/