Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id ; Sun, 28 Jul 2002 15:21:59 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id ; Sun, 28 Jul 2002 15:21:59 -0400 Received: from ns.virtualhost.dk ([195.184.98.160]:4309 "EHLO virtualhost.dk") by vger.kernel.org with ESMTP id ; Sun, 28 Jul 2002 15:21:58 -0400 Date: Sun, 28 Jul 2002 21:25:23 +0200 From: Jens Axboe To: martin@dalecki.de Cc: Linus Torvalds , Kernel Mailing List Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction Message-ID: <20020728212523.A3460@suse.de> References: <3D40E62B.9070202@evision.ag> <20020726143840.GC8761@suse.de> <3D416625.4050205@evision.ag> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3D416625.4050205@evision.ag> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2252 Lines: 79 On Fri, Jul 26 2002, Marcin Dalecki wrote: > 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. But the crap still got merged, sigh... Yet again an excellent point of why stuff like this should go through the maintainer. Apparently Linus blindly applies this stuff. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/