Re: [PATCH] drivers/scsi/st.c: add reference count and related fixes

[Brian King <brking@us.ibm.com>]

I just tested out your patch on the latest git tree and this patch
fixes the scsi tape hot unplug problems I was seeing as well.


Brian

Dailey, Nate wrote:
> I've attached patches against 2.6.13rc2. These are basically identical
> to my earlier patches, as I found that all issues I'd seen in earlier
> kernels still existed in this kernel.
> 
> To summarize, the changes are: (more details in my original email)
> 
> - add a kref to the scsi_tape structure, and associate reference
> counting stuff
> 
> - set sr_request->end_io = blk_end_sync_rq so we get notified when an IO
> is rejected when the device goes away
> 
> - check rq_status when IOs complete, else we don't know that IOs
> rejected for a dead device in fact did not complete
> 
> - change last_SRpnt so it's set before an async IO is issued (in case
> st_sleep_done is bypassed)
> 
> - fix a bogus use of last_SRpnt in st_chk_result
> 
> 
> Nate Dailey
> Stratus Technologies
> 
> Signed-off-by: Nate Dailey <nate.dailey@stratus.com>
> 
> 
>>-----Original Message-----
>>From: Kai Makisara [mailto:Kai.Makisara@kolumbus.fi] 
>>Sent: Tuesday, July 12, 2005 4:08 PM
>>To: Dailey, Nate
>>Cc: Brian King; linux-scsi@vger.kernel.org
>>Subject: RE: [PATCH] drivers/scsi/st.c: add reference count 
>>and related fixes
>>
>>
>>On Tue, 12 Jul 2005, Dailey, Nate wrote:
>>
>>
>>>I'll try porting my changes to a recent kernel, verify that 
>>
>>the bugs I
>>
>>>found are still bugs, and get back to you with a new patch.
>>>
>>>In the meantime, responses to your comments:
>>>
>>>- using a new kref in the scsi_tape structure: I've given this some
>>>thought, and I think using a new kref is defintely the best 
>>
>>way to go.
>>
>>>If nothing else, this is the same way sd and sr do 
>>
>>reference counting...
>>
>>>seems easier to maintain if the code in st is as similar as 
>>
>>possible to
>>
>>>these other drivers.
>>>
>>
>>st is a character device and it should use the character device 
>>refcounting if convenient. However, I now agree that the new 
>>kref is the 
>>simplest solution.
>>
>>
>>>- my changes to how last_SRpnt is used: I'll add a comment 
>>
>>documenting
>>
>>>when last_SRpnt is valid (it's valid only for async IOs 
>>
>>that have been
>>
>>>issued, nulled out by write_behind_check when they complete)
>>>
>>
>>Good.
>>
>>
>>>- Here's why I think the use of last_SRpnt in st_chk_result 
>>
>>is bogus:
>>
>>>one of the inputs to st_chk_result is "struct scsi_request 
>>
>>* SRpnt", and
>>
>>>it seems like this is the SRpnt you want to use, rather 
>>
>>than last_SRpnt.
>>
>>>This input will be the command that st_do_scsi or write_behind_check
>>>just waited on. This might not cause any problems with the 
>>
>>current st
>>
>>>code, but it will definitely cause problems with the 
>>
>>changes I made to
>>
>>>last_SRpnt. Let me know if I'm wrong about this, though, and I can
>>>change my last_SRpnt code.
>>>
>>
>>OK. I now see your point and it seems that the solution is 
>>simple: use the 
>>SRpnt argument of st_chk_result(). The reason why the 
>>function is using 
>>last_SRpnt is moving code from one place to another and not 
>>being careful 
>>enough to change everything (I knew that in that code 
>>last_SRpnt was valid 
>>always and so this did not look like an error ;-).
>>
>>-- 
>>Kai
>>


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center