[patch 5/6] frontend device shutdown

[patch 5/6] frontend device shutdown

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

  Hi,

This patch makes xenbus and net/blkfront drivers correctly handle
device_shutdown().  Changes:

  * new xenbus_strstate() helper function for fancy debug output.
  * add xenbus_dev_shutdown() function which switches the devices
    into Closing state and waits for them to finish shutdown.
  * new helper function xenbus_closing_done(), which handles the
    final state transition and informs xenbus_dev_shutdown().

xenbus_closing_done() behaves differently depending on how the closing
was triggered.  If it was triggered by the backend the frontend acks the
removal by going to CLosed.  If it was triggered via device_shutdown()
the state is set back to "Initializing", to allow a possible new kernel
instance to reuse the devices (for domU kexec again ;)

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

[-- Attachment #2: frontend-shutdown --]
[-- Type: text/plain, Size: 7583 bytes --]

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
Index: source-lnx-stable-22813/drivers/xen/blkfront/blkfront.c
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/blkfront/blkfront.c	2006-08-17 15:19:20.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/blkfront/blkfront.c	2006-08-17 15:20:17.000000000 +0200
@@ -272,10 +272,13 @@ static void backend_changed(struct xenbu
 			xenbus_dev_fatal(dev, -ENODEV, "bdget failed");
 
 		down(&bd->bd_sem);
-		if (info->users > 0)
+		if (info->users)
+			printk("%-20s: %s: %d user(s) left\n", __FUNCTION__,
+			       dev->nodename, info->users);
+		if (info->users > 0 && !dev->shutdown_in_progress) {
 			xenbus_dev_error(dev, -EBUSY,
 					 "Device in use; refusing to close");
-		else
+		} else
 			blkfront_closing(dev);
 		up(&bd->bd_sem);
 		bdput(bd);
@@ -359,7 +362,7 @@ static void blkfront_closing(struct xenb
 
 	xlvbd_del(info);
 
-	xenbus_switch_state(dev, XenbusStateClosed);
+	xenbus_closing_done(dev);
 }
 
 
Index: source-lnx-stable-22813/drivers/xen/netfront/netfront.c
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/netfront/netfront.c	2006-08-17 15:19:20.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/netfront/netfront.c	2006-08-17 15:20:33.000000000 +0200
@@ -436,7 +436,7 @@ static void backend_changed(struct xenbu
 	struct netfront_info *np = dev->dev.driver_data;
 	struct net_device *netdev = np->netdev;
 
-	DPRINTK("\n");
+	DPRINTK("%s\n", xenbus_strstate(backend_state));
 
 	switch (backend_state) {
 	case XenbusStateInitialising:
@@ -447,9 +447,11 @@ static void backend_changed(struct xenbu
 		break;
 
 	case XenbusStateInitWait:
-		network_connect(netdev);
-		xenbus_switch_state(dev, XenbusStateConnected);
-		(void)send_fake_arp(netdev);
+		if (!dev->shutdown_in_progress) {
+			network_connect(netdev);
+			xenbus_switch_state(dev, XenbusStateConnected);
+			(void)send_fake_arp(netdev);
+		}
 		break;
 
 	case XenbusStateClosing:
@@ -1720,11 +1722,10 @@ static void netfront_closing(struct xenb
 {
 	struct netfront_info *info = dev->dev.driver_data;
 
-	DPRINTK("netfront_closing: %s removed\n", dev->nodename);
+	DPRINTK("%s\n", dev->nodename);
 
 	close_netdev(info);
-
-	xenbus_switch_state(dev, XenbusStateClosed);
+	xenbus_closing_done(dev);
 }
 
 
Index: source-lnx-stable-22813/drivers/xen/xenbus/xenbus_client.c
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/xenbus/xenbus_client.c	2006-08-17 15:19:20.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/xenbus/xenbus_client.c	2006-08-17 15:20:17.000000000 +0200
@@ -41,6 +41,21 @@ extern char *kasprintf(const char *fmt, 
 #define DPRINTK(fmt, args...) \
     pr_debug("xenbus_client (%s:%d) " fmt ".\n", __FUNCTION__, __LINE__, ##args)
 
+char *xenbus_strstate(enum xenbus_state state)
+{
+	static char *name[] = {
+		[ XenbusStateUnknown      ] = "Unknown",
+		[ XenbusStateInitialising ] = "Initialising",
+		[ XenbusStateInitWait     ] = "InitWait",
+		[ XenbusStateInitialised  ] = "Initialised",
+		[ XenbusStateConnected    ] = "Connected",
+		[ XenbusStateClosing      ] = "Closing",
+		[ XenbusStateClosed	  ] = "Closed",
+	};
+	return state < sizeof(name)/sizeof(name[0])
+		? name[state] : "INVALID";
+}
+
 int xenbus_watch_path(struct xenbus_device *dev, const char *path,
 		      struct xenbus_watch *watch,
 		      void (*callback)(struct xenbus_watch *,
@@ -124,6 +139,23 @@ int xenbus_switch_state(struct xenbus_de
 }
 EXPORT_SYMBOL_GPL(xenbus_switch_state);
 
+int xenbus_closing_done(struct xenbus_device *dev)
+{
+	if (dev->shutdown_in_progress) {
+		/* frontend (we) triggered closing, because of devices_shutdown(),
+		 * prepare for possible re-connect */
+		printk("%-20s: %s: => Initialising\n", __FUNCTION__, dev->nodename);
+		xenbus_switch_state(dev, XenbusStateInitialising);
+	} else {
+		/* backend triggered closing, probably the device went away,
+		 * thus we are going to close down too. */
+		printk("%-20s: %s: => Closed\n", __FUNCTION__, dev->nodename);
+		xenbus_switch_state(dev, XenbusStateClosed);
+	}
+	complete(&dev->down);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xenbus_closing_done);
 
 /**
  * Return the path to the error node for the given device, or NULL on failure.
Index: source-lnx-stable-22813/drivers/xen/xenbus/xenbus_probe.c
===================================================================
--- source-lnx-stable-22813.orig/drivers/xen/xenbus/xenbus_probe.c	2006-08-17 15:20:17.000000000 +0200
+++ source-lnx-stable-22813/drivers/xen/xenbus/xenbus_probe.c	2006-08-17 15:20:17.000000000 +0200
@@ -67,6 +67,7 @@ static int xenbus_probe_backend(const ch
 
 static int xenbus_dev_probe(struct device *_dev);
 static int xenbus_dev_remove(struct device *_dev);
+static void xenbus_dev_shutdown(struct device *_dev);
 
 /* If something in array of ids matches this device, return it. */
 static const struct xenbus_device_id *
@@ -186,6 +187,7 @@ static struct xen_bus_type xenbus_fronte
 		.match    = xenbus_match,
 		.probe    = xenbus_dev_probe,
 		.remove   = xenbus_dev_remove,
+		.shutdown = xenbus_dev_shutdown,
 	},
 	.dev = {
 		.bus_id = "xen",
@@ -240,6 +242,7 @@ static struct xen_bus_type xenbus_backen
 		.match    = xenbus_match,
 		.probe    = xenbus_dev_probe,
 		.remove   = xenbus_dev_remove,
+//		.shutdown = xenbus_dev_shutdown,
 		.uevent   = xenbus_uevent_backend,
 	},
 	.dev = {
@@ -399,6 +402,26 @@ static int xenbus_dev_remove(struct devi
 	return 0;
 }
 
+static void xenbus_dev_shutdown(struct device *_dev)
+{
+	struct xenbus_device *dev = to_xenbus_device(_dev);
+	unsigned long timeout = 5*HZ;
+
+	get_device(&dev->dev);
+	if (dev->state != XenbusStateConnected) {
+		printk("%s: %s: %s != Connected, skipping\n", __FUNCTION__,
+		       dev->nodename, xenbus_strstate(dev->state));
+		goto out;
+	}
+	dev->shutdown_in_progress = 1;
+	xenbus_switch_state(dev, XenbusStateClosing);
+	timeout = wait_for_completion_timeout(&dev->down, timeout);
+	if (!timeout)
+		printk("%s: %s timeout closing device\n", __FUNCTION__, dev->nodename);
+ out:
+	put_device(&dev->dev);
+}
+
 static int xenbus_register_driver_common(struct xenbus_driver *drv,
 					 struct xen_bus_type *bus)
 {
@@ -581,6 +604,7 @@ static int xenbus_probe_node(struct xen_
 	tmpstring += strlen(tmpstring) + 1;
 	strcpy(tmpstring, type);
 	xendev->devicetype = tmpstring;
+	init_completion(&xendev->down);
 
 	xendev->dev.parent = &bus->dev;
 	xendev->dev.bus = &bus->bus;
Index: source-lnx-stable-22813/include/xen/xenbus.h
===================================================================
--- source-lnx-stable-22813.orig/include/xen/xenbus.h	2006-08-17 15:19:20.000000000 +0200
+++ source-lnx-stable-22813/include/xen/xenbus.h	2006-08-17 15:20:17.000000000 +0200
@@ -37,6 +37,7 @@
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/mutex.h>
+#include <linux/completion.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/grant_table.h>
 #include <xen/interface/io/xenbus.h>
@@ -74,6 +75,9 @@ struct xenbus_device {
 	struct xenbus_watch otherend_watch;
 	struct device dev;
 	enum xenbus_state state;
+
+	struct completion down;
+	int shutdown_in_progress;
 };
 
 static inline struct xenbus_device *to_xenbus_device(struct device *dev)
@@ -296,4 +300,7 @@ void xenbus_dev_fatal(struct xenbus_devi
 		      ...);
 
 
+int xenbus_closing_done(struct xenbus_device *dev);
+char *xenbus_strstate(enum xenbus_state state);
+
 #endif /* _XEN_XENBUS_H */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [patch 5/6] frontend device shutdown

[Keir Fraser]

On 17/8/06 3:03 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> xenbus_closing_done() behaves differently depending on how the closing
> was triggered.  If it was triggered by the backend the frontend acks the
> removal by going to CLosed.  If it was triggered via device_shutdown()
> the state is set back to "Initializing", to allow a possible new kernel
> instance to reuse the devices (for domU kexec again ;)

I really don't like that distinction very much (moving to Initialising
instead of Closed). The problem is really in the backend, which incorrectly
interprets XenbusStateClosed as a request to completely unregister the
device. The reason it does this is because the control tools signal that
destruction should take place by deleting the frontend directory in
xenstore. This results in xenbus_read_driver_state() returning
XenbusStateClosed, hence the backend drivers have interpreted that state
value as a destruction request from the tools.

I've gone some way to cleaning this up in changeset 11203, which instead has
xenbus_read_driver_state() return XenbusStateUnknown if the frontend
directory is missing. You should use this to distinguish the Closed and
Unkown states in the backend drivers: this will mean that you won't need to
avoid entering state Closed in the frontend drivers.

 -- Keir

Re: [patch 5/6] frontend device shutdown

[Keir Fraser]

On 19/8/06 1:25 pm, "Keir Fraser" <Keir.Fraser@cl.cam.ac.uk> wrote:

> I've gone some way to cleaning this up in changeset 11203, which instead has
> xenbus_read_driver_state() return XenbusStateUnknown if the frontend
> directory is missing. You should use this to distinguish the Closed and
> Unkown states in the backend drivers: this will mean that you won't need to
> avoid entering state Closed in the frontend drivers.

By the way, changing this frontend behaviour may mean you need to tweak your
patch to the backend drivers, allowing reconnection, which I already checked
in (changeset 11194). I'm not sure though: maybe that patch will still work.

 -- Keir

Re: [patch 5/6] frontend device shutdown

[Gerd Hoffmann]

> I really don't like that distinction very much (moving to Initialising
> instead of Closed). The problem is really in the backend, which incorrectly
> interprets XenbusStateClosed as a request to completely unregister the
> device. The reason it does this is because the control tools signal that
> destruction should take place by deleting the frontend directory in
> xenstore. This results in xenbus_read_driver_state() returning
> XenbusStateClosed, hence the backend drivers have interpreted that state
> value as a destruction request from the tools.

Ok, that should work fine for the backend side.

But there is another problem on the frontend side:  xenbus_probe_node()
ignores devices which are not in Initializing state, so it doesn't
reprobe devices which are about to disappear.  The next kernel should
probe them, but the frontend state is still Closing (or Closed) ...

I think that was the initial reason to do the jumpback to Initializing
(so the new kexec'ed kernel finds an environment identical to the direct
boot).  That was also the reason to introduce the shutdown_in_progress
flag, so I have some way to avoid re-initializing of the device by the
old kernel.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

Re: [patch 5/6] frontend device shutdown

[Keir Fraser]

On 21/8/06 3:11 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> But there is another problem on the frontend side:  xenbus_probe_node()
> ignores devices which are not in Initializing state, so it doesn't
> reprobe devices which are about to disappear.  The next kernel should
> probe them, but the frontend state is still Closing (or Closed) ...
> 
> I think that was the initial reason to do the jumpback to Initializing
> (so the new kexec'ed kernel finds an environment identical to the direct
> boot).  That was also the reason to introduce the shutdown_in_progress
> flag, so I have some way to avoid re-initializing of the device by the
> old kernel.

I think the problem here is that the xenbus state machine does not
distinguish who is driving a sequence of state changes. There's no easy way
to determine whether a shutdown is triggered by e.g., backend device hotplug
or frontend driver removal / shutdown.

Moving to state Initialising in the frontend is not a great solution. For
example, what happens if a device is hotplug-removed via xm at the same time
the guest is kexec'ing? Instead of moving to Closed the guest will move to
Initialising and the hotplug request is 'lost' because the new guest kernel
instance will simply reconnect to the still-waiting backend.

I think a better solution is for hotplug events to create a new node in the
backend directory. Something nice and explicit, like 'deleted'. Backend
drivers can choose to device_unregister() only if XenbusStateUnknown or the
node 'deleted' exists. We can check for 'deleted' at the otherend in
xenbus_probe_node() and bail if we see it.

Does this sound plausible?

 -- Keir

Re: [patch 5/6] frontend device shutdown

[Gerd Hoffmann]

Keir Fraser wrote:
> I think the problem here is that the xenbus state machine does not
> distinguish who is driving a sequence of state changes. 

One of the problems, yes.

The most tricky one is the handover of the frontend device from one
kernel to the next (same issues will come up for some minios+grub based
bootloader if someone goes creating one ...).

The prerequisites we have are:

  * We _must not_ break old domUs.
  * I'd like to be able to boot old domU kernels via kexec, so the
    post-kexec environment should be identical to what the domain
    builder creates today.

The backward compatibility issue makes it very tricky to add new states,
such as "Disconnecting" for example.  Todays domU kernels expect to find
the frontend device state in Initializing and the backend device state
in Initializing or InitWait.  That are the main reasons I went the route
to switch back to Initializing state in the frontend, and I see no easy
way around that.

The backend responds by cleaning up and preparing for the frontend going
to "Connected" state again, and sets its own state to "InitWait" to
signal that to the frontend.  That is a perfectly fine environment for
the new kernel.  Unfortunaly the old kernel's frontend reacts on that
state change too, which did lead to the introduction of the
shutdown_in_progress flag in xenbus_device to avoid that.

I hope that are all gotchas I ran into (thats the problem with two weeks
vacation between writing code and submitting patches, you forget some of
the tricky details ...).

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

Re: [patch 5/6] frontend device shutdown

[Keir Fraser]

On 22/8/06 9:04 am, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>   * We _must not_ break old domUs.

Indeed.

>   * I'd like to be able to boot old domU kernels via kexec, so the
>     post-kexec environment should be identical to what the domain
>     builder creates today.

Nice to have. I might be inclined to push support for compat into the kexec
layer, though (e.g., fixing up xenstore so that old kernels will be happy),
if doing it in core xenbus code seems distasteful.

> The backward compatibility issue makes it very tricky to add new states,
> such as "Disconnecting" for example.  Todays domU kernels expect to find
> the frontend device state in Initializing and the backend device state
> in Initializing or InitWait.  That are the main reasons I went the route
> to switch back to Initializing state in the frontend, and I see no easy
> way around that.

I don't want to add new XenbusStates. I'm talking about a different,
hotplug, status that is rather different: it indicates the status of the
device from the p.o.v. Of the admin and control tools (basically, 'online'
or 'offline'). This node has quite a different purpose from the existing
state node, which is really a low-level mechanism for sync'ing connection
state machines in frontend/backend drivers.

> The backend responds by cleaning up and preparing for the frontend going
> to "Connected" state again, and sets its own state to "InitWait" to
> signal that to the frontend.  That is a perfectly fine environment for
> the new kernel.  Unfortunaly the old kernel's frontend reacts on that
> state change too, which did lead to the introduction of the
> shutdown_in_progress flag in xenbus_device to avoid that.

Two options:
 1. Always enter Closed state, and then run a cleanup phase in the kexec
code which iterates over xenstore device directories, switching
Closed->Initialising.
 2. Enter state Initialising if backend's 'hotplug_status' node indicates
'online'. Gate xenbus_probe_node() on kernel_status <= STATE_RUNNING (the
shutdown code only ever gets run after kernel_status is bumped to one of the
shutdown states, I believe).

I think (1) is architecturally cleaner, but I understand it may be a pain to
implement in the constrained post-shutdown kexec environment. I don't think
(2) is too bad, and is more along the lines of what you have already.

 -- Keir

> I hope that are all gotchas I ran into (thats the problem with two weeks
> vacation between writing code and submitting patches, you forget some of
> the tricky details ...).

Re: [patch 5/6] frontend device shutdown

[Keir Fraser]

On 22/8/06 9:30 am, "Keir Fraser" <Keir.Fraser@cl.cam.ac.uk> wrote:

> Two options:
>  1. Always enter Closed state, and then run a cleanup phase in the kexec
> code which iterates over xenstore device directories, switching
> Closed->Initialising.
>  2. Enter state Initialising if backend's 'hotplug_status' node indicates
> 'online'. Gate xenbus_probe_node() on kernel_status <= STATE_RUNNING (the
> shutdown code only ever gets run after kernel_status is bumped to one of the
> shutdown states, I believe).
> 
> I think (1) is architecturally cleaner, but I understand it may be a pain to
> implement in the constrained post-shutdown kexec environment. I don't think
> (2) is too bad, and is more along the lines of what you have already.

I had a chat with Ewan about this. We agree option (2) is sane and likely
much less work than option (1).

 -- Keir

Re: [patch 5/6] frontend device shutdown

[Gerd Hoffmann]

  Hi,

> I don't want to add new XenbusStates. I'm talking about a different,
> hotplug, status that is rather different: it indicates the status of the
> device from the p.o.v. Of the admin and control tools (basically, 'online'
> or 'offline').

I understand that, this is a related but different issue though.

> Two options:
>  1. Always enter Closed state, and then run a cleanup phase in the kexec
> code which iterates over xenstore device directories, switching
> Closed->Initialising.
>  2. Enter state Initialising if backend's 'hotplug_status' node indicates
> 'online'. Gate xenbus_probe_node() on kernel_status <= STATE_RUNNING (the
> shutdown code only ever gets run after kernel_status is bumped to one of the
> shutdown states, I believe).
> 
> I think (1) is architecturally cleaner, but I understand it may be a pain to
> implement in the constrained post-shutdown kexec environment. I don't think
> (2) is too bad, and is more along the lines of what you have already.

I'll try (1).  I don't expect it being that hard, I expect doing that in
the old kernel _after_ unregistering the watches should work just fine.

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

Re: [patch 5/6] frontend device shutdown

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

  Hi,

>>  1. Always enter Closed state, and then run a cleanup phase in the kexec
>> code which iterates over xenstore device directories, switching
>> Closed->Initialising.

> I'll try (1).  I don't expect it being that hard, I expect doing that in
> the old kernel _after_ unregistering the watches should work just fine.

Here we go, changes:

 * new helper function xenbus_strstate() for more readable debug output.
 * some of the DPRINK()'s got some more info to print added (device
   node, state on state changes).
 * Both sides will happily enter "Closed" state without deleting the
   device.
 * There is a new helper function xenbus_reinit_frontend_devices() for
   kexec:  It unregisteres the watch (to prevent the device from being
   reinitialized in the old kernel) and moves devices from Closed back
   to Initialising state.

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg

[-- Attachment #2: frontend-shutdown-2 --]
[-- Type: text/plain, Size: 9772 bytes --]

Signed-off-by: Gerd Hoffmann <kraxel@suse.de>
diff -r f681ffc9b01a linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c
--- a/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c	Mon Aug 21 12:05:11 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/blkback/xenbus.c	Tue Aug 22 11:34:33 2006 +0200
@@ -301,11 +301,11 @@ static void frontend_changed(struct xenb
 	struct backend_info *be = dev->dev.driver_data;
 	int err;
 
-	DPRINTK("");
+	DPRINTK("%s", xenbus_strstate(frontend_state));
 
 	switch (frontend_state) {
 	case XenbusStateInitialising:
-		if (dev->state == XenbusStateClosing) {
+		if (dev->state == XenbusStateClosed) {
 			printk("%s: %s: prepare for reconnect\n",
 			       __FUNCTION__, dev->nodename);
 			xenbus_switch_state(dev, XenbusStateInitWait);
@@ -331,8 +331,11 @@ static void frontend_changed(struct xenb
 		xenbus_switch_state(dev, XenbusStateClosing);
 		break;
 
+	case XenbusStateClosed:
+		xenbus_switch_state(dev, XenbusStateClosed);
+		break;
+
 	case XenbusStateUnknown:
-	case XenbusStateClosed:
 		device_unregister(&dev->dev);
 		break;
 
diff -r f681ffc9b01a linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c
--- a/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c	Mon Aug 21 12:05:11 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/blkfront/blkfront.c	Tue Aug 22 11:51:22 2006 +0200
@@ -273,7 +273,7 @@ static void backend_changed(struct xenbu
 			xenbus_dev_fatal(dev, -ENODEV, "bdget failed");
 
 		down(&bd->bd_sem);
-		if (info->users > 0)
+		if (info->users > 0 && system_state == SYSTEM_RUNNING)
 			xenbus_dev_error(dev, -EBUSY,
 					 "Device in use; refusing to close");
 		else
@@ -360,7 +360,7 @@ static void blkfront_closing(struct xenb
 
 	xlvbd_del(info);
 
-	xenbus_switch_state(dev, XenbusStateClosed);
+	xenbus_closing_done(dev);
 }
 
 
diff -r f681ffc9b01a linux-2.6-xen-sparse/drivers/xen/netback/xenbus.c
--- a/linux-2.6-xen-sparse/drivers/xen/netback/xenbus.c	Mon Aug 21 12:05:11 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/netback/xenbus.c	Tue Aug 22 11:34:33 2006 +0200
@@ -228,13 +228,13 @@ static void frontend_changed(struct xenb
 {
 	struct backend_info *be = dev->dev.driver_data;
 
-	DPRINTK("");
+	DPRINTK("%s", xenbus_strstate(frontend_state));
 
 	be->frontend_state = frontend_state;
 
 	switch (frontend_state) {
 	case XenbusStateInitialising:
-		if (dev->state == XenbusStateClosing) {
+		if (dev->state == XenbusStateClosed) {
 			printk("%s: %s: prepare for reconnect\n",
 			       __FUNCTION__, dev->nodename);
 			if (be->netif) {
@@ -260,8 +260,11 @@ static void frontend_changed(struct xenb
 		xenbus_switch_state(dev, XenbusStateClosing);
 		break;
 
+	case XenbusStateClosed:
+		xenbus_switch_state(dev, XenbusStateClosed);
+		break;
+
 	case XenbusStateUnknown:
-	case XenbusStateClosed:
 		if (be->netif != NULL)
 			kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
 		device_unregister(&dev->dev);
diff -r f681ffc9b01a linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
--- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Mon Aug 21 12:05:11 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Aug 22 11:52:13 2006 +0200
@@ -478,7 +478,7 @@ static void backend_changed(struct xenbu
 	struct netfront_info *np = dev->dev.driver_data;
 	struct net_device *netdev = np->netdev;
 
-	DPRINTK("\n");
+	DPRINTK("%s\n", xenbus_strstate(backend_state));
 
 	switch (backend_state) {
 	case XenbusStateInitialising:
@@ -1946,11 +1946,10 @@ static void netfront_closing(struct xenb
 {
 	struct netfront_info *info = dev->dev.driver_data;
 
-	DPRINTK("netfront_closing: %s removed\n", dev->nodename);
+	DPRINTK("%s\n", dev->nodename);
 
 	close_netdev(info);
-
-	xenbus_switch_state(dev, XenbusStateClosed);
+	xenbus_closing_done(dev);
 }
 
 
diff -r f681ffc9b01a linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c	Mon Aug 21 12:05:11 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_client.c	Tue Aug 22 11:53:16 2006 +0200
@@ -41,6 +41,21 @@ extern char *kasprintf(const char *fmt, 
 #define DPRINTK(fmt, args...) \
     pr_debug("xenbus_client (%s:%d) " fmt ".\n", __FUNCTION__, __LINE__, ##args)
 
+char *xenbus_strstate(enum xenbus_state state)
+{
+	static char *name[] = {
+		[ XenbusStateUnknown      ] = "Unknown",
+		[ XenbusStateInitialising ] = "Initialising",
+		[ XenbusStateInitWait     ] = "InitWait",
+		[ XenbusStateInitialised  ] = "Initialised",
+		[ XenbusStateConnected    ] = "Connected",
+		[ XenbusStateClosing      ] = "Closing",
+		[ XenbusStateClosed	  ] = "Closed",
+	};
+	return state < sizeof(name)/sizeof(name[0])
+		? name[state] : "INVALID";
+}
+
 int xenbus_watch_path(struct xenbus_device *dev, const char *path,
 		      struct xenbus_watch *watch,
 		      void (*callback)(struct xenbus_watch *,
@@ -124,6 +139,13 @@ int xenbus_switch_state(struct xenbus_de
 }
 EXPORT_SYMBOL_GPL(xenbus_switch_state);
 
+int xenbus_closing_done(struct xenbus_device *dev)
+{
+	xenbus_switch_state(dev, XenbusStateClosed);
+	complete(&dev->down);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xenbus_closing_done);
 
 /**
  * Return the path to the error node for the given device, or NULL on failure.
diff -r f681ffc9b01a linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_probe.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_probe.c	Mon Aug 21 12:05:11 2006 -0400
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_probe.c	Tue Aug 22 11:56:32 2006 +0200
@@ -73,6 +73,7 @@ static int xenbus_probe_backend(const ch
 
 static int xenbus_dev_probe(struct device *_dev);
 static int xenbus_dev_remove(struct device *_dev);
+static void xenbus_dev_shutdown(struct device *_dev);
 
 /* If something in array of ids matches this device, return it. */
 static const struct xenbus_device_id *
@@ -192,6 +193,7 @@ static struct xen_bus_type xenbus_fronte
 		.match    = xenbus_match,
 		.probe    = xenbus_dev_probe,
 		.remove   = xenbus_dev_remove,
+		.shutdown = xenbus_dev_shutdown,
 	},
 	.dev = {
 		.bus_id = "xen",
@@ -246,6 +248,7 @@ static struct xen_bus_type xenbus_backen
 		.match    = xenbus_match,
 		.probe    = xenbus_dev_probe,
 		.remove   = xenbus_dev_remove,
+//		.shutdown = xenbus_dev_shutdown,
 		.uevent   = xenbus_uevent_backend,
 	},
 	.dev = {
@@ -316,8 +319,8 @@ static void otherend_changed(struct xenb
 
 	state = xenbus_read_driver_state(dev->otherend);
 
-	DPRINTK("state is %d, %s, %s",
-		state, dev->otherend_watch.node, vec[XS_WATCH_PATH]);
+	DPRINTK("state is %s, %s, %s", xenbus_strstate(state),
+		dev->otherend_watch.node, vec[XS_WATCH_PATH]);
 	if (drv->otherend_changed)
 		drv->otherend_changed(dev, state);
 }
@@ -348,7 +351,7 @@ static int xenbus_dev_probe(struct devic
 	const struct xenbus_device_id *id;
 	int err;
 
-	DPRINTK("");
+	DPRINTK("%s", dev->nodename);
 
 	if (!drv->probe) {
 		err = -ENODEV;
@@ -393,7 +396,7 @@ static int xenbus_dev_remove(struct devi
 	struct xenbus_device *dev = to_xenbus_device(_dev);
 	struct xenbus_driver *drv = to_xenbus_driver(_dev->driver);
 
-	DPRINTK("");
+	DPRINTK("%s", dev->nodename);
 
 	free_otherend_watch(dev);
 	free_otherend_details(dev);
@@ -403,6 +406,27 @@ static int xenbus_dev_remove(struct devi
 
 	xenbus_switch_state(dev, XenbusStateClosed);
 	return 0;
+}
+
+static void xenbus_dev_shutdown(struct device *_dev)
+{
+	struct xenbus_device *dev = to_xenbus_device(_dev);
+	unsigned long timeout = 5*HZ;
+
+	DPRINTK("%s", dev->nodename);
+
+	get_device(&dev->dev);
+	if (dev->state != XenbusStateConnected) {
+		printk("%s: %s: %s != Connected, skipping\n", __FUNCTION__,
+		       dev->nodename, xenbus_strstate(dev->state));
+		goto out;
+	}
+	xenbus_switch_state(dev, XenbusStateClosing);
+	timeout = wait_for_completion_timeout(&dev->down, timeout);
+	if (!timeout)
+		printk("%s: %s timeout closing device\n", __FUNCTION__, dev->nodename);
+ out:
+	put_device(&dev->dev);
 }
 
 static int xenbus_register_driver_common(struct xenbus_driver *drv,
@@ -587,6 +611,7 @@ static int xenbus_probe_node(struct xen_
 	tmpstring += strlen(tmpstring) + 1;
 	strcpy(tmpstring, type);
 	xendev->devicetype = tmpstring;
+	init_completion(&xendev->down);
 
 	xendev->dev.parent = &bus->dev;
 	xendev->dev.bus = &bus->bus;
@@ -1168,3 +1193,23 @@ static int __init boot_wait_for_devices(
 
 late_initcall(boot_wait_for_devices);
 #endif
+
+#ifdef CONFIG_KEXEC
+static int xenbus_reinit_device(struct device *dev, void *data)
+{
+	struct xenbus_device *xendev = to_xenbus_device(dev);
+
+	if (xendev->state != XenbusStateClosed)
+		return 0;
+	free_otherend_watch(xendev);
+	xenbus_switch_state(xendev, XenbusStateInitialising);
+	return 0;
+}
+
+int xenbus_reinit_frontend_devices(void)
+{
+	DPRINTK("");
+	return bus_for_each_dev(&xenbus_frontend.bus, NULL, NULL,
+				xenbus_reinit_device);
+}
+#endif
diff -r f681ffc9b01a linux-2.6-xen-sparse/include/xen/xenbus.h
--- a/linux-2.6-xen-sparse/include/xen/xenbus.h	Mon Aug 21 12:05:11 2006 -0400
+++ b/linux-2.6-xen-sparse/include/xen/xenbus.h	Tue Aug 22 11:34:33 2006 +0200
@@ -37,6 +37,7 @@
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/mutex.h>
+#include <linux/completion.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/grant_table.h>
 #include <xen/interface/io/xenbus.h>
@@ -74,6 +75,7 @@ struct xenbus_device {
 	struct xenbus_watch otherend_watch;
 	struct device dev;
 	enum xenbus_state state;
+	struct completion down;
 };
 
 static inline struct xenbus_device *to_xenbus_device(struct device *dev)
@@ -297,4 +299,8 @@ void xenbus_dev_fatal(struct xenbus_devi
 
 int __init xenbus_dev_init(void);
 
+int xenbus_closing_done(struct xenbus_device *dev);
+char *xenbus_strstate(enum xenbus_state state);
+int xenbus_reinit_frontend_devices(void);
+
 #endif /* _XEN_XENBUS_H */


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [patch 5/6] frontend device shutdown

[Keir Fraser]

On 17/8/06 3:03 pm, "Gerd Hoffmann" <kraxel@suse.de> wrote:

> + int shutdown_in_progress;

Get rid of this field in your next patch.

The use in xenbus_client.c should go away because you should unconditionally
move to state XenbusStateClosed, fixing up backend drivers as necessary.

The use in netfront.c doesn't make much sense to me. Is it a side effect of
needing to move into state XenbusStateInitialised (which will be going
away)? I think we should be able to do without it, anyway (maybe a fix to
backend drivers will be required).

The use in blkfront.c can be replaced by a check of system_state (e.g.,
(system_state == SYSTEM_RUNNING)) as defined in linux/kernel.h.

 -- Keir

Re: [patch 5/6] frontend device shutdown

[Gerd Hoffmann]

  Hi,

>> + int shutdown_in_progress;
> 
> Get rid of this field in your next patch.

Should be easy with your changes, I'll have a look today.

> The use in netfront.c doesn't make much sense to me. Is it a side effect of
> needing to move into state XenbusStateInitialised (which will be going
> away)?

Yes.

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
http://www.suse.de/~kraxel/julika-dora.jpeg