| Message ID | 20191210113347.3404-3-pdurrant@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen-blkback: support live update | expand |
On Tue, Dec 10, 2019 at 11:33:45AM +0000, Paul Durrant wrote: > If a driver probe() fails then leave the xenstore state alone. There is no > reason to modify it as the failure may be due to transient resource > allocation issues and hence a subsequent probe() may succeed. > > If the driver supports re-binding then only force state to closed during > remove() only in the case when the toolstack may need to clean up. This can > be detected by checking whether the state in xenstore has been set to > closing prior to device removal. > > NOTE: Re-bind support is indicated by new boolean in struct xenbus_driver, > which defaults to false. Subsequent patches will add support to > some backend drivers. My intention was to specify whether you want to close the backends on unbind in sysfs, so that an user can decide at runtime, rather than having a hardcoded value in the driver. Anyway, I'm less sure whether such runtime tunable is useful at all, so let's leave it out and can always be added afterwards. At the end of day a user wrongly doing a rmmod blkback can always recover gracefully by loading blkback again with your proposed approach to leave connections open on module removal. Sorry for the extra work. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monné <roger.pau@citrix.com> > Sent: 11 December 2019 10:06 > To: Durrant, Paul <pdurrant@amazon.com> > Cc: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org; Juergen > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; > Boris Ostrovsky <boris.ostrovsky@oracle.com> > Subject: Re: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is forced > to closed > > On Tue, Dec 10, 2019 at 11:33:45AM +0000, Paul Durrant wrote: > > If a driver probe() fails then leave the xenstore state alone. There is > no > > reason to modify it as the failure may be due to transient resource > > allocation issues and hence a subsequent probe() may succeed. > > > > If the driver supports re-binding then only force state to closed during > > remove() only in the case when the toolstack may need to clean up. This > can > > be detected by checking whether the state in xenstore has been set to > > closing prior to device removal. > > > > NOTE: Re-bind support is indicated by new boolean in struct > xenbus_driver, > > which defaults to false. Subsequent patches will add support to > > some backend drivers. > > My intention was to specify whether you want to close the > backends on unbind in sysfs, so that an user can decide at runtime, > rather than having a hardcoded value in the driver. > > Anyway, I'm less sure whether such runtime tunable is useful at all, > so let's leave it out and can always be added afterwards. At the end > of day a user wrongly doing a rmmod blkback can always recover > gracefully by loading blkback again with your proposed approach to > leave connections open on module removal. > > Sorry for the extra work. > Does this mean you don't think the extra driver flag is necessary any more? NB: now that xenbus actually takes module references you can't accidentally rmmod any more :-) Paul
On 11.12.19 11:14, Durrant, Paul wrote: >> -----Original Message----- >> From: Roger Pau Monné <roger.pau@citrix.com> >> Sent: 11 December 2019 10:06 >> To: Durrant, Paul <pdurrant@amazon.com> >> Cc: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org; Juergen >> Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; >> Boris Ostrovsky <boris.ostrovsky@oracle.com> >> Subject: Re: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is forced >> to closed >> >> On Tue, Dec 10, 2019 at 11:33:45AM +0000, Paul Durrant wrote: >>> If a driver probe() fails then leave the xenstore state alone. There is >> no >>> reason to modify it as the failure may be due to transient resource >>> allocation issues and hence a subsequent probe() may succeed. >>> >>> If the driver supports re-binding then only force state to closed during >>> remove() only in the case when the toolstack may need to clean up. This >> can >>> be detected by checking whether the state in xenstore has been set to >>> closing prior to device removal. >>> >>> NOTE: Re-bind support is indicated by new boolean in struct >> xenbus_driver, >>> which defaults to false. Subsequent patches will add support to >>> some backend drivers. >> >> My intention was to specify whether you want to close the >> backends on unbind in sysfs, so that an user can decide at runtime, >> rather than having a hardcoded value in the driver. >> >> Anyway, I'm less sure whether such runtime tunable is useful at all, >> so let's leave it out and can always be added afterwards. At the end >> of day a user wrongly doing a rmmod blkback can always recover >> gracefully by loading blkback again with your proposed approach to >> leave connections open on module removal. >> >> Sorry for the extra work. >> > > Does this mean you don't think the extra driver flag is necessary any more? NB: now that xenbus actually takes module references you can't accidentally rmmod any more :-) I'd like it to be kept, please. Juergen
> -----Original Message----- > From: Jürgen Groß <jgross@suse.com> > Sent: 11 December 2019 10:21 > To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné > <roger.pau@citrix.com> > Cc: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org; Stefano > Stabellini <sstabellini@kernel.org>; Boris Ostrovsky > <boris.ostrovsky@oracle.com> > Subject: Re: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is forced > to closed > > On 11.12.19 11:14, Durrant, Paul wrote: > >> -----Original Message----- > >> From: Roger Pau Monné <roger.pau@citrix.com> > >> Sent: 11 December 2019 10:06 > >> To: Durrant, Paul <pdurrant@amazon.com> > >> Cc: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org; > Juergen > >> Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; > >> Boris Ostrovsky <boris.ostrovsky@oracle.com> > >> Subject: Re: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is > forced > >> to closed > >> > >> On Tue, Dec 10, 2019 at 11:33:45AM +0000, Paul Durrant wrote: > >>> If a driver probe() fails then leave the xenstore state alone. There > is > >> no > >>> reason to modify it as the failure may be due to transient resource > >>> allocation issues and hence a subsequent probe() may succeed. > >>> > >>> If the driver supports re-binding then only force state to closed > during > >>> remove() only in the case when the toolstack may need to clean up. > This > >> can > >>> be detected by checking whether the state in xenstore has been set to > >>> closing prior to device removal. > >>> > >>> NOTE: Re-bind support is indicated by new boolean in struct > >> xenbus_driver, > >>> which defaults to false. Subsequent patches will add support to > >>> some backend drivers. > >> > >> My intention was to specify whether you want to close the > >> backends on unbind in sysfs, so that an user can decide at runtime, > >> rather than having a hardcoded value in the driver. > >> > >> Anyway, I'm less sure whether such runtime tunable is useful at all, > >> so let's leave it out and can always be added afterwards. At the end > >> of day a user wrongly doing a rmmod blkback can always recover > >> gracefully by loading blkback again with your proposed approach to > >> leave connections open on module removal. > >> > >> Sorry for the extra work. > >> > > > > Does this mean you don't think the extra driver flag is necessary any > more? NB: now that xenbus actually takes module references you can't > accidentally rmmod any more :-) > > I'd like it to be kept, please. > Ok. I'll leave this patch alone then. Paul > Juergen
diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index a10311c348b9..9303ff35b2bd 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -255,7 +255,6 @@ int xenbus_dev_probe(struct device *_dev) module_put(drv->driver.owner); fail: xenbus_dev_error(dev, err, "xenbus_dev_probe on %s", dev->nodename); - xenbus_switch_state(dev, XenbusStateClosed); return err; } EXPORT_SYMBOL_GPL(xenbus_dev_probe); @@ -276,7 +275,16 @@ int xenbus_dev_remove(struct device *_dev) free_otherend_details(dev); - xenbus_switch_state(dev, XenbusStateClosed); + /* + * If the toolstack has forced the device state to closing then set + * the state to closed now to allow it to be cleaned up. + * Similarly, if the driver does not support re-bind, set the + * closed. + */ + if (!drv->allow_rebind || + xenbus_read_driver_state(dev->nodename) == XenbusStateClosing) + xenbus_switch_state(dev, XenbusStateClosed); + return 0; } EXPORT_SYMBOL_GPL(xenbus_dev_remove); diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index 869c816d5f8c..24228a102141 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -93,6 +93,7 @@ struct xenbus_device_id struct xenbus_driver { const char *name; /* defaults to ids[0].devicetype */ const struct xenbus_device_id *ids; + bool allow_rebind; /* avoid setting xenstore closed during remove */ int (*probe)(struct xenbus_device *dev, const struct xenbus_device_id *id); void (*otherend_changed)(struct xenbus_device *dev,
If a driver probe() fails then leave the xenstore state alone. There is no reason to modify it as the failure may be due to transient resource allocation issues and hence a subsequent probe() may succeed. If the driver supports re-binding then only force state to closed during remove() only in the case when the toolstack may need to clean up. This can be detected by checking whether the state in xenstore has been set to closing prior to device removal. NOTE: Re-bind support is indicated by new boolean in struct xenbus_driver, which defaults to false. Subsequent patches will add support to some backend drivers. 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> v2: - Introduce the 'allow_rebind' flag - Expand the commit comment --- drivers/xen/xenbus/xenbus_probe.c | 12 ++++++++++-- include/xen/xenbus.h | 1 + 2 files changed, 11 insertions(+), 2 deletions(-)