Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Marcin Dalecki]

Jens Axboe wrote:
> On Fri, Jul 26 2002, Marcin Dalecki wrote:
> 
>>The attached patch does the following:
> 
> 
> Looks fine to me. One thing sticks out though:

Hey it was *literal* cut and paste from SCSI code after all ;-)

>>+	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);

Yes.

> 	blk_insert_request(q, rq, bla bla);
> 
> Also, please use the right spacing, if(bla :-)

Cut and paste damage from SCSI code.... no argument here.

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

Ah, yes I'm pretty sure now. I looked up how blk_queue_end_tag()
works and it's indeed the case -> setting the flag
and undoing it immediately doesn't make sense anyway.
(Even the collateral damage to tag allocation aside...)
This was perhaps "defensive coding" by the SCSI people?

You are right the

rq->flags &= REQ_QUEUED;

and the

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

should be just removed and things are fine.
They only survive becouse they don't provide a tag for the request in
first place.

Thanks for pointing it out.