Re: [PATCH] Avoid preferential treatment of groups that aren't backlogged

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Feb 10, 2011 at 11:06:37AM -0800, Chad Talbott wrote:
> On Wed, Feb 9, 2011 at 8:02 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > - What happens when cfqd->active_generation wraps over? Should we use
> >  functions which take care of wrapping.
> 
> To be absolutely correct, you're right.  But even if the service tree
> becomes completely idle every microsecond, we still have 1/2 million
> years before the first wrap.

Ok, so I guess it fine for time being untile and unless we encounter a
really fast device. 

> 
> > - So when generation number changes you want to put the newly backlogged
> >  group at the front of tree and not at the end of it? Though it kind
> >  of make sense as any continuously backlogged groups will be on service
> >  tree for long time and newly backlogged groups are either new IO
> >  starting or some application which high think time and which does IO
> >  one in a while and does not keep disk occupied for very long time. In
> >  such cases it probably is not a bad idea to put newly backlogged
> >  groups at the beginning of the tree.
> 
> Yes, that's it exactly.
> 
> >> @@ -906,6 +905,9 @@ cfq_group_service_tree_del(struct cfq_data *cfqd, struct cfq_group *cfqg)
> >>       if (!RB_EMPTY_NODE(&cfqg->rb_node))
> >>               cfq_rb_erase(&cfqg->rb_node, st);
> >>       cfqg->saved_workload_slice = 0;
> >> +     cfqg->generation_num = cfqd->active_generation;
> >> +     if (RB_EMPTY_ROOT(&st->rb))
> >> +             cfqd->active_generation++;
> >
> > I don't understand that what is the significance behind chaning generation
> > number when tree is idle? When tree is idle does not mean that few
> > recently deleted groups will not get backlogged soon?
> 
> It's a result of being both fair and work conserving.  If *all* the
> queues are without requests, then whoever next has requests should get
> to use the disk fully to be work-conserving.  But if the disk is
> always kept busy (by anyone), then there is competition and we have to
> provide fairness.

Well, service tree being idle does not mean that nobody else is using
disk. We could have the case where service tree is idle in between while
all the readers are in their think time phase. In that case I think one
could further refine the logic where change the generation only if tree is
idle for more than slice_idle time.

But I think we can do that later if we run into some issues. For the
time being I am fine with it as it has been working for you guys.

Can you please update the patch to correct the commit description and
repost. 

Thanks
Vivek