[1/4] xenbus: move xenbus_dev_shutdown() into frontend code...
diff mbox series

Message ID 20191205140123.3817-2-pdurrant@amazon.com
State Superseded
Headers show
Series
  • xen-blkback: support live update
Related show

Commit Message

Paul Durrant Dec. 5, 2019, 2:01 p.m. UTC
...and make it static

xenbus_dev_shutdown() is seemingly intended to cause clean shutdown of PV
frontends when a guest is rebooted. Indeed the function waits for a
conpletion which is only set by a call to xenbus_frontend_closed().

This patch removes the shutdown() method from backends and moves
xenbus_dev_shutdown() from xenbus_probe.c into xenbus_probe_frontend.c,
renaming it appropriately and making it static.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
---
 drivers/xen/xenbus/xenbus.h                |  2 --
 drivers/xen/xenbus/xenbus_probe.c          | 23 ---------------------
 drivers/xen/xenbus/xenbus_probe_backend.c  |  1 -
 drivers/xen/xenbus/xenbus_probe_frontend.c | 24 +++++++++++++++++++++-
 4 files changed, 23 insertions(+), 27 deletions(-)

Comments

Jürgen Groß Dec. 9, 2019, 11:33 a.m. UTC | #1
On 05.12.19 15:01, Paul Durrant wrote:
> ...and make it static
> 
> xenbus_dev_shutdown() is seemingly intended to cause clean shutdown of PV
> frontends when a guest is rebooted. Indeed the function waits for a
> conpletion which is only set by a call to xenbus_frontend_closed().
> 
> This patch removes the shutdown() method from backends and moves
> xenbus_dev_shutdown() from xenbus_probe.c into xenbus_probe_frontend.c,
> renaming it appropriately and making it static.

Is this a good move considering driver domains?

At least I'd expect the commit message addressing the expected behavior
with rebooting a driver domain and why this patch isn't making things
worse.


Juergen
Paul Durrant Dec. 9, 2019, 11:55 a.m. UTC | #2
> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 11:34
> To: Durrant, Paul <pdurrant@amazon.com>; linux-kernel@vger.kernel.org;
> xen-devel@lists.xenproject.org
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH 1/4] xenbus: move xenbus_dev_shutdown() into frontend
> code...
> 
> On 05.12.19 15:01, Paul Durrant wrote:
> > ...and make it static
> >
> > xenbus_dev_shutdown() is seemingly intended to cause clean shutdown of
> PV
> > frontends when a guest is rebooted. Indeed the function waits for a
> > conpletion which is only set by a call to xenbus_frontend_closed().
> >
> > This patch removes the shutdown() method from backends and moves
> > xenbus_dev_shutdown() from xenbus_probe.c into xenbus_probe_frontend.c,
> > renaming it appropriately and making it static.
> 
> Is this a good move considering driver domains?

I don't think it can have ever worked properly for driver domains, and with the rest of the patches a backend should be able go away and return unannounced (as long as the domain id is kept... for which patches need to be upstreamed into Xen).

> 
> At least I'd expect the commit message addressing the expected behavior
> with rebooting a driver domain and why this patch isn't making things
> worse.
> 

For a clean reboot I'd expect the toolstack to shut down the protocol before rebooting the driver domain, so the backend shutdown method is moot. And I don't believe re-startable driver domains were something that ever made it into support (because of the non-persistent domid problem). I can add something to the commit comment to that effect if you'd like.

  Paul
Jürgen Groß Dec. 9, 2019, 11:57 a.m. UTC | #3
On 09.12.19 12:55, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <jgross@suse.com>
>> Sent: 09 December 2019 11:34
>> To: Durrant, Paul <pdurrant@amazon.com>; linux-kernel@vger.kernel.org;
>> xen-devel@lists.xenproject.org
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>; Stefano Stabellini
>> <sstabellini@kernel.org>
>> Subject: Re: [PATCH 1/4] xenbus: move xenbus_dev_shutdown() into frontend
>> code...
>>
>> On 05.12.19 15:01, Paul Durrant wrote:
>>> ...and make it static
>>>
>>> xenbus_dev_shutdown() is seemingly intended to cause clean shutdown of
>> PV
>>> frontends when a guest is rebooted. Indeed the function waits for a
>>> conpletion which is only set by a call to xenbus_frontend_closed().
>>>
>>> This patch removes the shutdown() method from backends and moves
>>> xenbus_dev_shutdown() from xenbus_probe.c into xenbus_probe_frontend.c,
>>> renaming it appropriately and making it static.
>>
>> Is this a good move considering driver domains?
> 
> I don't think it can have ever worked properly for driver domains, and with the rest of the patches a backend should be able go away and return unannounced (as long as the domain id is kept... for which patches need to be upstreamed into Xen).
> 
>>
>> At least I'd expect the commit message addressing the expected behavior
>> with rebooting a driver domain and why this patch isn't making things
>> worse.
>>
> 
> For a clean reboot I'd expect the toolstack to shut down the protocol before rebooting the driver domain, so the backend shutdown method is moot. And I don't believe re-startable driver domains were something that ever made it into support (because of the non-persistent domid problem). I can add something to the commit comment to that effect if you'd like.

Yes, please do so.

With this you can add my:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Patch
diff mbox series

diff --git a/drivers/xen/xenbus/xenbus.h b/drivers/xen/xenbus/xenbus.h
index d75a2385b37c..5f5b8a7d5b80 100644
--- a/drivers/xen/xenbus/xenbus.h
+++ b/drivers/xen/xenbus/xenbus.h
@@ -116,8 +116,6 @@  int xenbus_probe_devices(struct xen_bus_type *bus);
 
 void xenbus_dev_changed(const char *node, struct xen_bus_type *bus);
 
-void xenbus_dev_shutdown(struct device *_dev);
-
 int xenbus_dev_suspend(struct device *dev);
 int xenbus_dev_resume(struct device *dev);
 int xenbus_dev_cancel(struct device *dev);
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 4461f4583476..a10311c348b9 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -281,29 +281,6 @@  int xenbus_dev_remove(struct device *_dev)
 }
 EXPORT_SYMBOL_GPL(xenbus_dev_remove);
 
-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) {
-		pr_info("%s: %s: %s != Connected, skipping\n",
-			__func__, dev->nodename, xenbus_strstate(dev->state));
-		goto out;
-	}
-	xenbus_switch_state(dev, XenbusStateClosing);
-	timeout = wait_for_completion_timeout(&dev->down, timeout);
-	if (!timeout)
-		pr_info("%s: %s timeout closing device\n",
-			__func__, dev->nodename);
- out:
-	put_device(&dev->dev);
-}
-EXPORT_SYMBOL_GPL(xenbus_dev_shutdown);
-
 int xenbus_register_driver_common(struct xenbus_driver *drv,
 				  struct xen_bus_type *bus,
 				  struct module *owner, const char *mod_name)
diff --git a/drivers/xen/xenbus/xenbus_probe_backend.c b/drivers/xen/xenbus/xenbus_probe_backend.c
index b0bed4faf44c..14876faff3b0 100644
--- a/drivers/xen/xenbus/xenbus_probe_backend.c
+++ b/drivers/xen/xenbus/xenbus_probe_backend.c
@@ -198,7 +198,6 @@  static struct xen_bus_type xenbus_backend = {
 		.uevent		= xenbus_uevent_backend,
 		.probe		= xenbus_dev_probe,
 		.remove		= xenbus_dev_remove,
-		.shutdown	= xenbus_dev_shutdown,
 		.dev_groups	= xenbus_dev_groups,
 	},
 };
diff --git a/drivers/xen/xenbus/xenbus_probe_frontend.c b/drivers/xen/xenbus/xenbus_probe_frontend.c
index a7d90a719cea..8a1650bbe18f 100644
--- a/drivers/xen/xenbus/xenbus_probe_frontend.c
+++ b/drivers/xen/xenbus/xenbus_probe_frontend.c
@@ -126,6 +126,28 @@  static int xenbus_frontend_dev_probe(struct device *dev)
 	return xenbus_dev_probe(dev);
 }
 
+static void xenbus_frontend_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) {
+		pr_info("%s: %s: %s != Connected, skipping\n",
+			__func__, dev->nodename, xenbus_strstate(dev->state));
+		goto out;
+	}
+	xenbus_switch_state(dev, XenbusStateClosing);
+	timeout = wait_for_completion_timeout(&dev->down, timeout);
+	if (!timeout)
+		pr_info("%s: %s timeout closing device\n",
+			__func__, dev->nodename);
+ out:
+	put_device(&dev->dev);
+}
+
 static const struct dev_pm_ops xenbus_pm_ops = {
 	.suspend	= xenbus_dev_suspend,
 	.resume		= xenbus_frontend_dev_resume,
@@ -146,7 +168,7 @@  static struct xen_bus_type xenbus_frontend = {
 		.uevent		= xenbus_uevent_frontend,
 		.probe		= xenbus_frontend_dev_probe,
 		.remove		= xenbus_dev_remove,
-		.shutdown	= xenbus_dev_shutdown,
+		.shutdown	= xenbus_frontend_dev_shutdown,
 		.dev_groups	= xenbus_dev_groups,
 
 		.pm		= &xenbus_pm_ops,