Re: xbd detachment

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Sat, Jul 25, 2009 at 03:50:39PM -0500, David Young wrote:
> > > Should this routine follow some other protocol in order to close down
> > > and revoke the grant_ref_t?
> >
> > Hardly, revocation will end in panic() (you cannot free a grant table  
> > entry while there is a read/write lock acquired by the other end on the  
> > referenced page).
> 
> What I mean is that xbd_xenbus_detach() may not use the correct
> protocol, and that is why the transfer/write/read status does not
> clear.  For example, what if xbd(4) indicates to the dom0 that it is
> finished with the ring, and then queues a transfer on the ring due to a
> programming mistake?
> 
> It seems to me that xbd_xenbus_detach() may not be used very much, if at
> all.  Moreover, if it has ever been used before, then I think that the
> Dom0, not the DomU, had initiated the device's detachment.  That may, in
> fact, make a difference, even if it should not.

Actually it does. xbd_xenbus_detach() is called by the xenbus handler
though config_detach(); at this point the backed has switched to
XenbusStateClosing then XenbusStateClosed.
For a locally-initiated detach, the frontend need to switch to
XenbusStateClosing, then wait for the backend to switch to XenbusStateClosing
then XenbusStateClosed. This will cause config_detach() to be called
a second time. 

Would the (untested) attached patch help ?

-- 
Manuel Bouyer, LIP6, Universite Paris VI.           
Manuel.Bouyer%lip6.fr@localhost
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: xbd_xenbus.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/xbd_xenbus.c,v
retrieving revision 1.39
diff -u -p -u -r1.39 xbd_xenbus.c
--- xbd_xenbus.c        3 Mar 2009 19:04:41 -0000       1.39
+++ xbd_xenbus.c        28 Jul 2009 16:30:42 -0000
@@ -110,6 +110,9 @@ struct xbd_xenbus_softc {
 #define BLKIF_STATE_CONNECTED    1
 #define BLKIF_STATE_SUSPENDED    2
        int sc_shutdown;
+#define BLKIF_SHUTDOWN_RUN    0 /* no shutdown */
+#define BLKIF_SHUTDOWN_REMOTE 1 /* backend-initiated shutdown in progress */
+#define BLKIF_SHUTDOWN_LOCAL  2 /* locally-initiated shutdown in progress */
 
        uint64_t sc_sectors; /* number of sectors for this device */
        u_long sc_secsize; /* sector size */
@@ -249,7 +252,7 @@ xbd_xenbus_attach(device_t parent, devic
        }
 
        sc->sc_backend_status = BLKIF_STATE_DISCONNECTED;
-       sc->sc_shutdown = 1;
+       sc->sc_shutdown = BLKIF_SHUTDOWN_REMOTE;
        /* initialise shared structures and tell backend that we are ready */
        xbd_xenbus_resume(sc);
 
@@ -266,12 +269,22 @@ xbd_xenbus_detach(device_t dev, int flag
        int s, bmaj, cmaj, i, mn;
        s = splbio();
        DPRINTF(("%s: xbd_detach\n", device_xname(dev)));
-       if (sc->sc_shutdown == 0) {
-               sc->sc_shutdown = 1;
+       if (sc->sc_shutdown == BLKIF_SHUTDOWN_LOCAL) {
+               /* xbd_xenbus_detach already in progress */
+               wakeup(xbd_xenbus_detach);
+               splx(s);
+               return EALREADY;
+       }
+       if (sc->sc_shutdown == BLKIF_SHUTDOWN_RUN) {
+               sc->sc_shutdown = BLKIF_SHUTDOWN_LOCAL;
                /* wait for requests to complete */
                while (sc->sc_backend_status == BLKIF_STATE_CONNECTED &&
                    sc->sc_dksc.sc_dkdev.dk_stats->io_busy > 0)
                        tsleep(xbd_xenbus_detach, PRIBIO, "xbddetach", hz/2);
+               xenbus_switch_state(sc->sc_xbusd, NULL, XenbusStateClosing);
+               while (xenbus_read_driver_state(sc->sc_xbusd->xbusd_otherend)
+                   != XenbusStateClosed)
+                       tsleep(xbd_xenbus_detach, PRIBIO, "xbddetach2", hz/2);
        }
        splx(s);
 
@@ -412,7 +425,7 @@ static void xbd_backend_changed(void *ar
                break;
        case XenbusStateClosing:
                s = splbio();
-               sc->sc_shutdown = 1;
+               sc->sc_shutdown = BLKIF_SHUTDOWN_REMOTE;
                /* wait for requests to complete */
                while (sc->sc_backend_status == BLKIF_STATE_CONNECTED &&
                    sc->sc_dksc.sc_dkdev.dk_stats->io_busy > 0)
@@ -432,7 +445,7 @@ static void xbd_backend_changed(void *ar
                        return;
 
                xbd_connect(sc);
-               sc->sc_shutdown = 0;
+               sc->sc_shutdown = BLKIF_SHUTDOWN_RUN;
                hypervisor_enable_event(sc->sc_evtchn);
 
                sc->sc_xbdsize =