| Message ID | 20191205140123.3817-2-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen-blkback: support live update | expand |
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
> -----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
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
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,
...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(-)