Re: [Xen-devel] [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-09-26 at 11:51 +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Ian Campbell
> > Sent: 26 September 2013 11:17
> > To: Paul Durrant
> > Cc: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Wei Liu; David Vrabel
> > Subject: Re: [PATCH net v5 1/1] xen-netback: Handle backend state
> > transitions in a more robust way
> > 
> > On Wed, 2013-09-25 at 10:59 +0100, Paul Durrant wrote:
> > > When the frontend state changes netback now specifies its desired state
> > to
> > > a new function, set_backend_state(), which transitions through any
> > > necessary intermediate states.
> > > This fixes an issue observed with some old Windows frontend drivers
> > where
> > > they failed to transition through the Closing state and netback would not
> > > behave correctly.
> > 
> > I'm somewhat terrified of breaking compatibility with some old or
> > obscure frontend here. I don't think it is reasonable to try and test
> > all of those so I guess we'll just have to deal with that when it
> > happens...
> > 
> > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> > > ---
> > >  drivers/net/xen-netback/xenbus.c |  143
> > ++++++++++++++++++++++++++++++--------
> > >  1 file changed, 113 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> > netback/xenbus.c
> > > index a53782e..01329b2 100644
> > > --- a/drivers/net/xen-netback/xenbus.c
> > > +++ b/drivers/net/xen-netback/xenbus.c
> > > @@ -24,6 +24,7 @@
> > >  struct backend_info {
> > >   struct xenbus_device *dev;
> > >   struct xenvif *vif;
> > > + enum xenbus_state state;
> > 
> > This cannot just be read from xenstore because of the
> > wait-for-hotplug-script deferral stuff, right?
> > 
> 
> Yes, that's right.
> 
> > I'm not sure it is worth clarifying that, either by a comment or by
> > naming it deferred_state_change or something?
> > 
> 
> Itâs the xenbus state that's deferred, the backend state leads so I
> could name it 'desired_state' or 'target_state' perhaps; probably a
> bit ugly so I'll add a comment above that field I think.

OK.

> 
> > > -static void destroy_backend(struct xenbus_device *dev)
> > > +static void backend_connect(struct backend_info *be)
> > >  {
> > > - struct backend_info *be = dev_get_drvdata(&dev->dev);
> > > + if (be->vif)
> > > +         connect(be);
> > > +}
> > >
> > > - if (be->vif) {
> > > -         kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
> > > -         xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
> > > -         xenvif_free(be->vif);
> > > -         be->vif = NULL;
> > 
> > Where did this uevent offlining functionality end up?
> > 
> 
> It's done in netback_remove() .
> 
> > > +static inline void backend_switch_state(struct backend_info *be,
> > > +                                 enum xenbus_state state)
> > > +{
> > > + struct xenbus_device *dev = be->dev;
> > > +
> > > + pr_debug("%s -> %s\n", dev->nodename, xenbus_strstate(state));
> > > + be->state = state;
> > > +
> > > + /* If we are waiting for a hotplug script then defer the
> > > +  * actual xenbus state change.
> > 
> > Is the right/wise regardless of the state we are moving too? Might the
> > state change have effectively "cancelled" the need to wait for the
> > script?
> > 
> > The previous code only deferred when moving to Connected. If we are now
> > moving to Closed/Closing do we still need to wait?
> > 
> 
> I debated about this; we never really dealt with this before. I think
> it's legitimate for the frontend to give up waiting for a backend
> hotplug so we should be able to go from InitWait to Closing, so in
> this case be->state will be Closing when the script (hopefully)
> finishes. Thus we move to that when the watch fires rather than to
> Connected. Then the frontend can move to Closed. If the hotplug script
> never finishes then I don't think we're any more screwed than we were
> before.

OK.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel