Re: CVS commit: src/sys/dev/scsipi

[David Young <dyoung%pobox.com@localhost>]

On Mon, Apr 25, 2011 at 06:26:09PM +0200, Juergen Hannken-Illjes wrote:
> On Mon, Apr 25, 2011 at 10:33:14AM -0500, David Young wrote:
> > On Mon, Apr 25, 2011 at 02:14:23PM +0000, Juergen Hannken-Illjes wrote:
> > > Module Name:      src
> > > Committed By:     hannken
> > > Date:             Mon Apr 25 14:14:22 UTC 2011
> > > 
> > > Modified Files:
> > >   src/sys/dev/scsipi: scsiconf.c
> > > 
> > > Log Message:
> > > Don't kill outstanding requests when detaching a scsibus on shutdown.
> > > Both the controller and tyhe targets are still running.
> > 
> > I don't think you have fixed the actual bug.  It sounds like the
> > detachment routine is broken in every case where the controller was not
> > abruptly detached, but the driver relinquishes control of the controller
> > in an orderly fashion because of an operator command.
> 
> The actual bug was i386 shutdown, a loop of vfs_unmountall1, config_detach_all
> and vfs_unmount_forceone.  Here config_detach_all() detaches devices from
> the leafs up.
> For me it sometimes happend that the (scsi) root disk had outstanding xfers
> when it came to config_detach_all().  The disk would not detach but the
> bus detach would kill all outstanding operations.  I don't want these xfers to
> abort but the disk continue operations until all xfers are done.
> 
> And yes, this detach routine looks bogus at least ...

The bug was in scsibusdetach(), which is not doing things in the proper
order: it has to detach its children and check for error.  If no error,
then it can release the resources that the children were using.  See
attached patch.

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 344-0444 x24

Index: scsiconf.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/scsiconf.c,v
retrieving revision 1.261
diff -u -p -r1.261 scsiconf.c
--- scsiconf.c  25 Apr 2011 14:14:22 -0000      1.261
+++ scsiconf.c  25 Apr 2011 17:30:31 -0000
@@ -256,11 +256,20 @@ scsibusdetach(device_t self, int flags)
        struct scsipi_xfer *xs;
        int error;
 
+       /*
+        * Detach all of the periphs.
+        */
+       if ((error = scsipi_target_detach(chan, -1, -1, flags)) != 0)
+               return error;
+
        pmf_device_deregister(self);
 
        /*
         * Process outstanding commands (which will never complete as the
         * controller is gone).
+        *
+        * XXX Surely this is redundant?  If we get this far, the
+        * XXX peripherals have all been detached.
         */
        for (ctarget = 0; ctarget < chan->chan_ntargets; ctarget++) {
                if (ctarget == chan->chan_id)
@@ -269,8 +278,6 @@ scsibusdetach(device_t self, int flags)
                        periph = scsipi_lookup_periph(chan, ctarget, clun);
                        if (periph == NULL)
                                continue;
-                       if ((flags & DETACH_SHUTDOWN) != 0)
-                               return EBUSY;
                        TAILQ_FOREACH(xs, &periph->periph_xferq, device_q) {
                                callout_stop(&xs->xs_callout);
                                xs->error = XS_DRIVER_STUFFUP;
@@ -280,16 +287,10 @@ scsibusdetach(device_t self, int flags)
        }
 
        /*
-        * Detach all of the periphs.
-        */
-       error = scsipi_target_detach(chan, -1, -1, flags);
-
-       /*
         * Now shut down the channel.
-        * XXX only if no errors ?
         */
        scsipi_channel_shutdown(chan);
-       return (error);
+       return 0;
 }
 
 /*