Re: [BUG] Yet another scsi_cmnd leak?

[Mike Christie <michaelc@cs.wisc.edu>]

Alan Stern wrote:
> On Thu, 29 Sep 2005, Mike Christie wrote:
> 
> 
>>Alan Stern wrote:
>>
>>>James:
>>>
>>>This report is based on 2.6.14-rc2-git6.  The code in your scsi-misc-2.6 
>>>git tree is somewhat different (and I don't know which is more current), 
>>>but it still contains the same bug.
>>>
>>>
>>>In scsi_prep_fn, a request can get deferred if scsi_init_io fails to 
>>>allocate an sg table.  When this happens, the scsi_cmnd isn't released and 
>>>the request is not marked DONTPREP.
>>>
>>>Then when scsi_prep_fn is called again, the request may be killed for 
>>>a number of reasons.  The code branches to the kill: label near the end of 
>>>the routine, which returns BLKPREP_KILL.
>>>
>>>Isn't it true that when this happens, the scsi_cmnd allocated during the
>>>original prep will never be released?
>>>
>>>It appears that scsi_prep_fn is undecided about whether or not the request
>>>is allowed to have a scsi_cmnd already.  The jumps to kill: seem to assume
>>>that it isn't, but the code for allocating a new scsi_cmnd tests for an
>>>existing one first.
>>>
>>
>>The gotos used to be just a return BLKPREP* and were added so I did not 
>>have to write DID_NO_CONNECT or unplug multiple times :) I think you are 
>>right and we need to further unwind what a previous prep had done 
>>becuase when we return with BLKPREP_KILL we only hear about this command 
>>again if it's request has a end_io function or waiting completion.
> 
> 
> I'm still not very clear about the conditions under which a request on the
> queue can be partially prepared -- for example, scsi_cmnd assigned but 
> not the sg table.  For the normal submission pathways, it looks like this 
> happens only when the sg allocation fails.  In those cases it wouldn't 
> hurt to release the scsi_cmnd before deferring.  Or before returning 
> BLKPREP_KILL.
> 
> But what about other pathways?  As long as the special scsi_request things 
> exist, I don't know what should be done.  I saw you had submitted patches 
> to get rid of them; how far has that progressed?
> 

I am not done. I still have osst to convert and I think Doug found a bug 
I cannot reproduce. I was not sure if everyone was happy with the 
max_sectors and the SCSI_MAX_PHYS_SEGMENTS compile option either.

I think I need to change the interface too. Passing in a scatterlist is 
nice beucase we do not have to touch the ULDs much, but I think if we 
went Christoph's route and used a array of bio_bvecs it might be nicer. 
We would need a bio helper function that could build bios from bvecs 
though and I think maybe that should be done based on the bioset stuff. 
I am not sure though.