Re: Questions about scsi_target_reap and starget/sdev lifecyle

[Mike Anderson <andmike@us.ibm.com>]

Alan Stern [stern@rowland.harvard.edu] wrote:
> On Wed, 15 Jun 2005, James Bottomley wrote:
> 
> > On Wed, 2005-06-15 at 17:11 -0400, Alan Stern wrote:
> > > This means that scsi_target_reap can be called and the __targets list 
> > > changed essentially at any time (subject only to the host_lock).  Hence it 
> > > is impossible for scsi_forget_host to iterate through the list of targets 
> > > belonging to the host: While it is working to remove one target, the next 
> > > target on the list (stored in the tmp variable) might be removed by 
> > > another thread.
> > 
> > It's no better nor worse than we already have.  As has been said many
> > times before, we need a proper host state model.
> > 
> > > In fact there doesn't seem to be any safe way to remove all the targets
> > > from a host.  And what's to prevent scsi_target_reap being called twice
> > > for the same target?
> > 
> > The usage, if you look at the code ... it's alloc/reap or inc
> > reap_ref/reap
> 
> Okay, I understand a little better now.
> 
> Would it hurt anything to change scsi_remove_host so that SHOST_DEL gets
> set at the start, before scsi_forget_host is called?  If that were done,
> then we could make every pathway that adds a device acquire the scan_mutex
> and fail if SHOST_DEL is already set.  Thus no devices would ever be added
> after forget_host had removed the existing ones, which can happen as
> things stand.
> 
> Furthermore, forget_host could be changed to loop over the __devices list
> instead of the __targets list.  The net effect would be the same: All the
> devices on the host would be removed and the targets would automatically
> get reaped by scsi_device_dev_release.  There wouldn't be any need to
> iterate over the targets at all.
> 
> As a final change, the new loop-over-devices in forget_host and the one in
> __scsi_remove_target should be made to skip over devices with SDEV_DEL
> already set, and each time they call scsi_remove_device the loops should
> restart from the beginning.  This will eliminate the problem of the tmp
> pointer being pulled out from under the code (even though it has 
> quadratic behavior).
> 
> Do these changes sound okay?
> 

I have something similar that I was testing since you mentioned the
problem the other day. Our build machine went down so I will need to wait
until tomorrow to get access to my patches for a post, if you have already
rolled a patch we can compare notes when I post.

What I did in the patch sequence was:
	1.) Recode the scsi_host state model to align with the device
	state model (i.e. added scsi_host_set_state function and
	associated changes).
	2.) Made shost cancel matched the scsi device state model and set
	this at the top of scsi_remove_host. Previous cancel code was not
	doing anything as the list was normally empty do to
	scsi_forget_host being called first. Also there where possible
	race conditions that you previously mentioned about canceling
	commands in this method.
	3.) Added choke point checks under the scan_mutex to determine if
	scanning is allowed (scsi_host_scan_allowed).
	4.) Added a target state model to match the scsi device state
	model.
	5.) I use the STGT_CANCEL and STGT_DEL states in scsi_forget_host
	to skip over entries in the list as I am no longer using the
	_safe version.
This looks like a good starting point with limited testing. I also need to
more states to the target state model as I only added a few that I needed
for the delete code.

Anyway sorry about the delay.

-andmike
--
Michael Anderson
andmike@us.ibm.com