Re: [PATCH] Fix device removal on net and block frontend drivers

[Murillo Fernandes Bernardes <mfb@br.ibm.com>]

On Monday 21 November 2005 16:31, Ewan Mellor wrote:
> On Mon, Nov 21, 2005 at 11:49:19AM -0500, Stefan Berger wrote:

> Could you please enable the DPRINTK in xenbus_probe and see whether the
> Closed is coming through the test in otherend_changed?  This would help
> diagnose the problem.
>

on DomU:

xenbus_probe (xenbus_suspend:831) .
xenbus_probe (suspend_dev:786) .
xenbus_probe (suspend_dev:786) .
xenbus_probe (suspend_dev:786) .
xenbus_probe (resume_dev:806) .
xenbus_probe (otherend_changed:301) state is 
6, /local/domain/0/backend/vif/5/0/state, /local/domain/0/backend/vif/5/0/state.
xenbus_probe (otherend_changed:301) state is 
6, /local/domain/0/backend/vbd/5/770/state, /local/domain/0/backend/vbd/5/770/state.
xenbus_probe (resume_dev:806) .
xenbus_probe (backend_changed:764) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (resume_dev:806) .
xenbus_probe (otherend_changed:301) state is 
4, /local/domain/0/backend/vbd/6/769/state, /local/domain/0/backend/vbd/6/769/state.
xenbus_probe (frontend_changed:756) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (otherend_changed:301) state is 
4, /local/domain/0/backend/vbd/6/769/state, /local/domain/0/backend/vbd/6/769/state.
xenbus_probe (otherend_changed:301) state is 
4, /local/domain/0/backend/vbd/6/770/state, /local/domain/0/backend/vbd/6/770/state.
xenbus_probe (frontend_changed:756) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (otherend_changed:301) state is 
4, /local/domain/0/backend/vbd/6/770/state, /local/domain/0/backend/vbd/6/770/state.
xenbus_probe (otherend_changed:301) state is 
4, /local/domain/0/backend/vif/6/0/state, /local/domain/0/backend/vif/6/0/state.
xenbus_probe (frontend_changed:756) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (frontend_changed:756) .
xenbus_probe (otherend_changed:301) state is 
4, /local/domain/0/backend/vif/6/0/state, /local/domain/0/backend/vif/6/0/state.

> The intention with xenbus_read_driver_state returning Closed was that this
> was the correct way of forcing the driver to close down if the path goes
> away, as in normal use the backend path should not just disappear, and for
> resumption we have a way to detect that.  Perhaps one or other of these
> things should change, but it's not clear to me which one it is, or if
> indeed this is the problem at all.
>
> > What I am seeing is that after a suspend / resume the interface 'eth0' is
> > completely gone. 'ifconfig -a' shows everything, but no eth0.
> >
> > You might only want to unregister if the domain was not suspended. So you
> > probably need to implement the .suspend function in the frontend and set
> > a state variable to know whether the domain is being hibernated, and you
> > clear that variable in the .resume. You check that variable when the
> > driver is going into the 'Closed' state and only unregister if not in
> > 'suspend' mode.
>
> If this is necessary, and it's not clear to me that it is, then this is a
> facility that Xenbus should provide in general, rather than each driver
> having to hack around the problem itself.

What about a XenbusStateSuspended?

>
> Returning to Murillo's patch, I assumed that the unregister_netdev in
> close_netdev would implicitly call device_unregister, and that this was the
> correct way to close down the device.  Is this not the case?
>
It is not happening. All references to device were cleared ? I'm not sure if 
it is needed in this case.

> There is the different issue that Xend does not check for the existence or
> state of a device before hotplugging a new one.  This means that the
> frontend might not have time to see the Closing before having a chance to
> close down, for example.  This is a problem with Xend that needs to be
> fixed there.  Xend should refuse to hotplug a device if the frontend for
> the old one has not yet closed down.  This is not to say that Murillo's
> patch is wrong, but simply to say that I expect wider issues than can be
> fixed by this patch alone.
>


-- 
Murillo Fernandes Bernardes
IBM Linux Technology Center