Re: [PATCH linux-2.6-block:master 02/05] blk: update ioscheds to use generic dispatch queue

[Tejun Heo <htejun@gmail.com>]

On Thu, Oct 20, 2005 at 01:21:09PM +0200, Jens Axboe wrote:
> On Wed, Oct 19 2005, Tejun Heo wrote:
> > 02_blk_generic-dispatch-queue-update-for-ioscheds.patch
> > 
> > 	This patch updates all four ioscheds to use generic dispatch
> > 	queue.  There's one behavior change in as-iosched.
> > 
> > 	* In as-iosched, when force dispatching
> > 	  (ELEVATOR_INSERT_BACK), batch_data_dir is reset to REQ_SYNC
> > 	  and changed_batch and new_batch are cleared to zero.  This
> > 	  prevernts AS from doing incorrect update_write_batch after
> > 	  the forced dispatched requests are finished.
> > 
> > 	* In cfq-iosched, cfqd->rq_in_driver currently counts the
> > 	  number of activated (removed) requests to determine
> > 	  whether queue-kicking is needed and cfq_max_depth has been
> > 	  reached.  With generic dispatch queue, I think counting
> > 	  the number of dispatched requests would be more appropriate.
> > 
> > 	* cfq_max_depth can be lowered to 1 again.
> 
> I applied this one as well, with some minor changes. The biggest one is
> a cleanup of the 'force' logic, it seems to be a little mixed up in this
> patch. You use it for forcing dispatch, which is fine. But then it also
> doubles as whether you want to sort insert on the generic queue or just
> add to the tail?

 When forced dispatch occurs, all requests in a elevator get dumped
into the dispatch queue.  Specific elevators are free to dump in any
order and it's likely that specific elevators don't dump in the
optimal order - e.g. for cfq, it will dump each cfqq's in order which
results in unnecessary seeks.  That's why all the current ioscheds
tells elv_dispatch_insert() to perform global dispatch queue sorting
when they dump requests due to force argument.  Maybe add comments to
explain this?

>   
> > -		if (cfq_class_idle(cfqq))
> > -			max_dispatch = 1;
> > +		if (force)
> > +			max_dispatch = INT_MAX;
> > +		else
> > +			max_dispatch =
> > +				cfq_class_idle(cfqq) ? 1 : cfqd->cfq_quantum;
> 
> Also, please don't use these ?: constructs, I absolutely hate them as
> they are weird to read.

 Hehheh, I actually like "?:"s, but I'll stay away from it.

 Thanks.

-- 
tejun