Re: [Xen-devel] [PATCH] libxl: don't set backend state to 5 when trying to unplug a device

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-01-13 at 13:10 +0000, Roger Pau Monnà wrote:
> 2012/1/13 Ian Campbell <Ian.Campbell@xxxxxxxxxx>:
> > On Fri, 2012-01-13 at 11:57 +0000, Roger Pau Monne wrote:
> >> # HG changeset patch
> >> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> >> # Date 1326454785 -3600
> >> # Node ID 887a3229fd7a50c04981e29709bc7210dafef38f
> >> # Parent  5b2676ac13218951698c49fa0350f2ac48220f3d
> >> libxl: don't set backend state to 5 when trying to unplug a device
> >>
> >> libxl__device_remove was setting backend state to 5, which could
> >> create a race condition, since the previous check for state != 4 and
> >> setting state to 5 was not done inside of the same transaction, so the
> >> kernel could change the state to 6 in the space between the check for
> >> state != 4 and setting state to 5.
> >>
> >> I might be wrong, but since I don't think setting backend state to 5
> >> helps in any way when disconnecting a device
> >
> > It's the exact thing which makes the disconnect happen at all, isn't it?
> 
> What makes the disconnect happen on NetBSD al least is removing the
> frontend or setting the frontend state to 6, but this doesn't do
> anything at all (it might be different on Linux though).

Linux certainly uses state 5 (AKA XenbusStateClosing) in the state
machine in at least netfront+back, blkfront+back pcifront+back and
fbfront (fbback is not an in kernel driver).

e.g. when netfront sees netback go to closing then it will shut itself
down, see drivers/net/xen-netfront.c:netback_changed

> Can someone confirm that this is actually useful on Linux?

I think really any changes in these areas should be backed up with
empirical experiments on a variety of system types (both front and
back), otherwise we are basing things on supposition and heresay about
how things are supposed to/do work. No one really knows for sure
(witness the number of times we've all gone round on this).

Ian.

> 
> > Some backends (particularly the Linux ones) might also use the online
> > node but I don't think that behaviour is universal.
> >
> >>  I've just removed the
> >> xs_write.  If this is necessary, the state != 4 check and setting it
> >> to 5 should happen inside the same transaction, to avoid the kernel
> >> from changing the state behind our back.
> >
> > I think that is the right solution.
> >
> > Ian.
> >
> >> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> >>
> >> diff -r 5b2676ac1321 -r 887a3229fd7a tools/libxl/libxl_device.c
> >> --- a/tools/libxl/libxl_device.c      Mon Jan 09 16:01:44 2012 +0100
> >> +++ b/tools/libxl/libxl_device.c      Fri Jan 13 12:39:45 2012 +0100
> >> @@ -456,7 +456,6 @@ int libxl__device_remove(libxl__gc *gc,
> >>  retry_transaction:
> >>      t = xs_transaction_start(ctx->xsh);
> >>      xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/online", be_path), "0", 
> >> strlen("0"));
> >> -    xs_write(ctx->xsh, t, state_path, "5", strlen("5"));
> >>      if (!xs_transaction_end(ctx->xsh, t, 0)) {
> >>          if (errno == EAGAIN)
> >>              goto retry_transaction;
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@xxxxxxxxxxxxxxxxxxx
> >> http://lists.xensource.com/xen-devel
> >
> >



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel