Re: oops on usb storage device disconnect with 2.6.14-rc1

James Bottomley
2005-09-16T01:46:41Z

On Thu, 2005-09-15 at 16:55 -0700, Mike Anderson wrote:
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> > On Thu, 2005-09-15 at 15:19 -0700, Mike Anderson wrote:
> > > A side effect of not applying Alan's previous patch that added
> > > SHOST_RECOVERY to the SHOST_CANCEL: state is that we will not move to the
> > > SHOST_CANCEL and subsequently not to SHOST_DEL state if the eh is active
> > > during the start of scsi_remove_host. I sent mail on the 7th indicating to
> > > include that state change hunk of the diff, but I guess that overlapped
> > > with your newer state changes.
> > > http://marc.theaimsgroup.com/?l=linux-scsi&m=112238726326927&w=2
> >
> > Yes, but that's not really legitimate since it introduces a bifurcation
> > in the state machine ... when the eh terminates it will come back to
> > running even if it went in from cancel.
>
> Clarification here as I do not see the split in the state machine or the
> transition back to running from cancel. If the above patch is applied we
> can transition to cancel from recovery if eh already has started. When eh
> is complete scsi_restart_operations transition to running will fail as we
> are in the cancel state.

OK, the current model has 5 states, three of which are relevant:
RUNNING, RECOVER and CANCEL. The problem is that if the error handler
is operating when someone calls scsi_remove_host() we're actually
simultaneously in the RECOVER and CANCEL states. This is the
bifurcation. Currently the model forbids the transition RECOVER-
>CANCEL, so we wrongly stay in the recover state (and even worse
transition back to RUNNING if the eh thread finishes). The patch would
permit this transition so we'd now wrongly go into the CANCEL state
instead (in spite of the error handler still being active---meaning that
the not in recovery checks all wrongly pass).

The solution is either to update our model to reflect reality: adding
the parallel CANCEL_RECOVER (and DEL_RECOVER) states that allow our
model to reflect simultaneous cancel and recovery. Or we update reality
to reflect our model: make scsi_remove_host() wait for the recovery
thread to finish if it's active and never let it start once we
transition to CANCEL.

> That said I like the idea below of waiting / terminating the eh thread
> prior to transitioning to cancel. There is some introduction of asymmetry
> here in scsi_remove_host as the eh thread is created in scsi_host_alloc,
> but possibly later patches could move the eh creation to scsi_add_host
> (unless I forgot the reason it needed to be earlier).

That's the "update reality to reflect our model" option.

> > > I still do not understand as I asked in a previous comment why we are not
> > > shutting down the eh_thread in scsi_remove_host and also why simpler state
> > > model updates could not solve the problem.
> >
> > Well, it goes back to whether we wait for the thread or not. To shut
> > the thread down, we also need to wait for it to complete.
> >
> > As far as the state model goes, we either need to wait for the eh thread
> > before transitioning to cancel or introduce the extra states that
> > reflect the parallel eh transitions.
>
> I like waiting for the eh thread to complete / shutdown prior to
> transitioning to cancel.

Well ... I'm less keen on this. Making one thing wait for another
introduces complex dependencies into the system. Such complex
interactions always seem to come back to bite us one way or another.
Since we can model the system without introducing the dependency I'd
really rather do it that way.

James

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at: