Re: [PATCH v11 6/9] Make scsi_remove_host() wait until error handling finished

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2013-06-25 at 11:13 -0500, Michael Christie wrote:
> On Jun 25, 2013, at 10:31 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> > On 06/25/13 15:45, James Bottomley wrote:
> >> On Tue, 2013-06-25 at 11:01 +0200, Bart Van Assche wrote:
> >>> There is a difference though between moving the EH kthread_stop() call
> >>> and the patch at the start of this thread: moving the EH kthread_stop()
> >>> call does not prevent that an ioctl like SG_SCSI_RESET triggers an eh_*
> >>> callback after scsi_remove_host() has finished. However, the
> >>> scsi_begin_eh() / scsi_end_eh() functions do prevent that an ioctl can
> >>> cause an eh_* callback to be invoked after scsi_remove_device() finished.
> >> 
> >> OK, but this doesn't tell me what you're trying to achieve.
> >> 
> >> An eh function is allowable as long as the host hadn't had the release
> >> callback executed.  That means you must have to have a reference to the
> >> device/host to execute the eh function, which is currently guaranteed
> >> for all invocations.
> > 
> > That raises a new question: how is an LLD expected to clean up resources without triggering a race condition ? What you wrote means that it's not safe for an LLD to start cleaning up the resources needed by the eh_* callbacks immediately after scsi_remove_device() returns since it it not guaranteed that at that time all references to the device have already been dropped.
> > 
> 
> 
> A callback in the device/target/host (whatever is needed) release
> function would do this right? If I understand James right, I think he
> suggested something like this in another mail.

Exactly ... at least that's what we should do.

If I look at what we actually do: all the HBAs treat scsi_remove_host as
a waited for transition.  The reason this works is the loop over
__scsi_remove_device() in scsi_forget_host().  By the time that loop
returns, every scsi_device is gone (and so is every target).  Because
blk_cleanup_queue() induces a synchronous wait for the queue to die in
__scsi_remove_device(), there can be no outstanding I/O and no eh
activity for the device when it returns (and no possibility of starting
any).  Thus at the end of scsi_forget_host, we have no devices to start
I/O and no eh activity, so the final put will be the last.

James