Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe <axboe@suse.de>]

On Fri, Jul 26 2002, Marcin Dalecki wrote:
> The attached patch does the following:

Looks fine to me. One thing sticks out though:

> +	/*
> +	 * tell I/O scheduler that this isn't a regular read/write (ie it
> +	 * must not attempt merges on this) and that it acts as a soft
> +	 * barrier
> +	 */
> +	rq->flags &= REQ_QUEUED;

this can't be right. Either it's a bug for REQ_QUEUED to be set here, or
it needs to end the tag properly.

> +	rq->flags |= REQ_SPECIAL | REQ_BARRIER;
> +
> +	rq->special = data;
> +
> +	spin_lock_irqsave(q->queue_lock, flags);
> +	/* If command is tagged, release the tag */
> +	if(blk_rq_tagged(rq))
> +		blk_queue_end_tag(q, rq);

woops, you just possible leaked a tag. I really don't think you should
mix inserting a special and ending a tag into the same function, makes
no sense. If a driver wants that it should do:

	if (blk_rq_tagged(rq))
		blk_queue_end_tag(q, rq);

	blk_insert_request(q, rq, bla bla);

Also, please use the right spacing, if(bla :-)

So kill any reference to tagging (and REQ_QUEUED)i in
blk_insert_request, and I'm ok with it.

-- 
Jens Axboe