Re: [PATCH] cfq-iosched: don't allow aliased requests to starve others

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Jul 23, 2010 at 11:07:11AM -0400, Jeff Moyer wrote:

[..]
> >> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> >> index 7982b83..0d8d2cd 100644
> >> --- a/block/cfq-iosched.c
> >> +++ b/block/cfq-iosched.c
> >> @@ -417,6 +417,7 @@ static inline int cfqg_busy_async_queues(struct cfq_data *cfqd,
> >>  }
> >>  
> >>  static void cfq_dispatch_insert(struct request_queue *, struct request *);
> >> +static struct request *cfq_check_fifo(struct cfq_queue *cfqq);
> >>  static struct cfq_queue *cfq_get_queue(struct cfq_data *, bool,
> >>  				       struct io_context *, gfp_t);
> >>  static struct cfq_io_context *cfq_cic_lookup(struct cfq_data *,
> >> @@ -1394,10 +1395,22 @@ static void cfq_add_rq_rb(struct request *rq)
> >>  
> >>  	/*
> >>  	 * looks a little odd, but the first insert might return an alias.
> >> -	 * if that happens, put the alias on the dispatch list
> >> +	 * If that happens, put the alias on the dispatch list, but don't
> >> +	 * allow issuing of aliased requests to starve out the queue.
> >>  	 */
> >> -	while ((__alias = elv_rb_add(&cfqq->sort_list, rq)) != NULL)
> >> +	while ((__alias = elv_rb_add(&cfqq->sort_list, rq)) != NULL) {
> >> +		int fifo_checked = cfq_cfqq_fifo_expire(cfqq);
> >> +		struct request *__rq;
> >> +
> >>  		cfq_dispatch_insert(cfqd->queue, __alias);
> >> +		cfq_clear_cfqq_fifo_expire(cfqq);
> >> +		while ((__rq = cfq_check_fifo(cfqq))) {
> >> +			cfq_dispatch_insert(cfqd->queue, __rq);
> >> +			cfq_clear_cfqq_fifo_expire(cfqq);
> >> +		}
> >> +		if (fifo_checked)
> >> +			cfq_mark_cfqq_fifo_expire(cfqq);
> >> +	}
> >
> > Jeff,
> >
> > IIUC correctly upon receiving an alias, you are checking fifo of same
> > cfqq and not the fifo of other cfqqs which are blocked. So dispatching
> > more expired requests from the cfqq which is generating lots of aliases
> > can at best only worsen the problem. I am wondering how does this patch
> > help in dispatching requests from other cfqqs. 
> 
> I think you're missing a key element, here.  Let's assume some harmless
> process has the I/O scheduler.  Then, your pathological alias generator
> process comes in and inserts a request into the queue.  If that request
> is an alias, then it gets moved to the dispatch list immediately, even
> though the cfqq for that process is not currently being served.
> 
> Now, I'm not sure why we ever get back into select_queue if the request
> queue is always full of these aliases.  I didn't dig down that far, but
> I did verify that progress was made in my simple reproducer.

Jeff, I do understand the problem here. Just that I don't understand
how this patch is going to fix it for CFQ. How this patch will make
sure that requests from other starved queue are put onto dispatch queue.

Also I have been trying to reproduce the issue with 2.6.36-rc6 kernel
and have not been successful so far.

Thanks
Vivek