Re: Questions about scsi_target_reap and starget/sdev lifecyle

Mike Anderson
2005-06-21T21:08:08Z

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