Re: Questions about scsi_target_reap and starget/sdev lifecyle

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

Alan Stern [stern@rowland.harvard.edu] wrote:
> > Moving scsi_forget_host to after scsi_host_cancel will cause the sd cache
> > flush routines to fail.
> 
> This objection runs up against an issue we discussed some time ago.  
> Should the intended meaning of scsi_remove_host be simply that the kernel
> needs to stop using the HBA reasonably soon?  In that case you are right.  
> Or should the intended meaning be that the HBA is actually gone
> (hot-unplugged) and all further attempts to use it will fail?  In that
> case it doesn't matter.  The best ways to resolve this issue may be to
> have a separate scsi_host_gone routine or to add an extra argument to
> scsi_remove_host.
> 

I believe this was discussed in the thread below or possible another (we
have had this conversation a number of times), and the action then was not
to create another interface.
http://marc.theaimsgroup.com/?t=108701426000002&r=1&w=2


> I rather agree in principle that the cancel functionality isn't really
> needed.  Removing it will require some tricky changes to the LLDDs,
> however.  And the changes will all have to be made at once.  If some LLDDs 
> are changed and others aren't, then (depending on whether scsi_host_cancel 
> has been removed) either the changed ones will oops as they try to cancel 
> an already-cancelled command or the unchanged ones will oops as 
> uncancelled commands time out.  I've seen both kinds of errors in working 
> with usb-storage.

We really should have scsi_times_out check the state of the host and
possible the device to stop calling into the host when removes are in
progress.

> 
> > The bit is set to SHOST_REMOVE then scsi_host_cancel is called which will
> > set the bit SHOST_CANCEL. Later on scan is stopped only if state is
> > SHOST_REMOVE. Is that what you wanted?
> 
> Remember, at the moment the state is a bit-vector.  It can have both
> SHOST_CANCEL and SHOST_REMOVE set at the same time.  That is what I
> wanted.  Changing to a host state model will of course require you to do
> things differently.

Yes, I guess I should know that :-(. I had my head in the new host state
model. Yes changing to the host state model did require some different
checks, but the concept is the same.

> 
> > >  int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
> > > @@ -1347,11 +1363,14 @@
> > >  		return -EINVAL;
> > >  
> > >  	down(&shost->scan_mutex);
> > > +	if (test_bit(SHOST_REMOVE, &shost->shost_state))
> > > +		goto out;
> > >  	if (channel == SCAN_WILD_CARD) 
> > >  		for (channel = 0; channel <= shost->max_channel; channel++)
> > >  			scsi_scan_channel(shost, channel, id, lun, rescan);
> > >  	else
> > >  		scsi_scan_channel(shost, channel, id, lun, rescan);
> > > +out:
> > >  	up(&shost->scan_mutex);
> > >  
> > >  	return 0;
> > 
> > It might be better to have a wrapper function so if we change the cases
> > where we would allow scanning we can change just one place. Also we might
> > cover more states if we reverse the logic on the check and look for the
> > case we allow scanning (see previous comment about cancel). This is what I
> > did in my previous patch.
> 
> That's okay with me.  So long as all the scanning pathways are covered and 
> all scanning is stopped before scsi_forget_host runs, you can feel free to 
> improve the implementation details.
> 

I already did this in the previous patch series I posted, but received not
comments so I guess there is no need to wrap it.

> 
> I didn't do it that way because it can't be made to work correctly with
> the current code -- there's no way to know whether a target has already
> been removed.  Adding a target state model would make your approach
> feasible, but James has said that targets don't merit a state model.
> 
> Driver model klists also have their disadvantages.  If you delete a node 
> from a klist asynchronously then you cannot re-use it; it must be allowed 
> to deallocate itself when the refcount goes to 0.  And it's not possible 
> to remove nodes from a klist synchronously while traversing the klist.

Is this still true for klists. I thought the locking updates and the
addition of a klist_iter was to fix this issue though I have not spent
much time looking through the code since the changes.

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