Re: [PATCH scsi-misc-2.6 07/13] scsi: move error handling out of scsi_init_io() into scsi_prep_fn()

[Tejun Heo]

 Greetings, James. :-)

On Fri, Apr 01, 2005 at 12:23:37PM -0600, James Bottomley wrote:
> On Thu, 2005-03-31 at 18:08 +0900, Tejun Heo wrote:
> > 	When scsi_init_io() returns BLKPREP_DEFER or BLKPREP_KILL,
> > 	it's supposed to free resources itself.  This patch
> > 	consolidates defer and kill handling into scsi_prep_fn().
> > 	This fixes a queue stall bug which occurred when sgtable
> > 	allocation failed and device_busy == 0.
> 
> This one I like, but I think it doesn't go far enough.  There are
> situations where a re-queue can also re-prepare, which means we leak sg
> lists and commands on BLKPREP_KILL because of SCSI state.

 With later request_fn() rewrite patch, all state checking are moved
to request_fn() and gets terminated with __scsi_done().  prep_fn()
only kills invalid (so, unprepped) requests.  It's in the fixed bugs
list of the request_fn() rewrite patch.  BTW, there's one more path
which leaks cmnd/sgtable, the end_that_request_*() on offline path of
request_fn() which is also fixed by request_fn() rewrite patch.

 I'm sorry about not mentioning that the bug is fixed in the later
patch.  Please review request_fn() rewrite patch.

> How about the attached.
> 
> Note, I've also altered the prepare function so req->special never gets
> altered if it points to a scsi_request. Previously it would be altered
> to point to the command, but now we couldn't put the command since the
> scsi_request we can't get to also has a copy of if.  So, if you can make
> your later REQ_SPECIAL removal work, we can dispense with sr_magic and
> simply use REQ_SPECIAL as the discriminator for the contents of req-
> >special.
> 
> James

 Yeah, I like not overwriting ->special, and removing magic is always
good. :-)

 Hmmm, I have another proposal.  What do you think about replacing
scsi_request with scsi_cmnd.  I've been looking into it and it doesn't
seem to be too much work and it won't cause problems.  The only
bothering thing is that scsi_cmnd is limited resource and should be
returned to the pool ASAP.  AFAIK, all current users of
scsi_allocate_request() releses the scsi_request as soon as they're
finished, and we can make the requirement clear in the comment
(i.e. do not cache or hold on to scsi_cmnd).  This will remove the
scsi_request/cmnd dance and make scsi midlayer slimmer in other
places.  What do you think?

 Thanks.

-- 
tejun

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/