[PATCH v2 0/4] xen-blkback: support live update

[PATCH v2 0/4] xen-blkback: support live update

[Paul Durrant]

Patch #1 is clean-up for an apparent mis-feature.

Paul Durrant (4):
  xenbus: move xenbus_dev_shutdown() into frontend code...
  xenbus: limit when state is forced to closed
  xen/interface: re-define FRONT/BACK_RING_ATTACH()
  xen-blkback: support dynamic unbind/bind

 drivers/block/xen-blkback/xenbus.c         | 59 +++++++++++++++-------
 drivers/xen/xenbus/xenbus.h                |  2 -
 drivers/xen/xenbus/xenbus_probe.c          | 35 ++++---------
 drivers/xen/xenbus/xenbus_probe_backend.c  |  1 -
 drivers/xen/xenbus/xenbus_probe_frontend.c | 24 ++++++++-
 include/xen/interface/io/ring.h            | 29 ++++-------
 include/xen/xenbus.h                       |  1 +
 7 files changed, 84 insertions(+), 67 deletions(-)
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Juergen Gross <jgross@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
-- 
2.20.1

[PATCH v2 1/4] xenbus: move xenbus_dev_shutdown() into frontend code...

[Paul Durrant]

...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.

NOTE: In the case where the backend is running in a driver domain, the
      toolstack should have already terminated any frontends that may be
      using it (since Xen does not support re-startable PV driver domains)
      so xenbus_dev_shutdown() should never be called.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.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(-)

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,
-- 
2.20.1

[PATCH v2 2/4] xenbus: limit when state is forced to closed

[Paul Durrant]

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(-)

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,
-- 
2.20.1

Re: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is forced to closed

[Roger Pau Monné]

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.

RE: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is forced to closed

[Durrant, Paul]

> -----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

Re: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is forced to closed

[Jürgen Groß]

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

RE: [Xen-devel] [PATCH v2 2/4] xenbus: limit when state is forced to closed

[Durrant, Paul]

> -----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

[PATCH v2 3/4] xen/interface: re-define FRONT/BACK_RING_ATTACH()

[Paul Durrant]

Currently these macros are defined to re-initialize a front/back ring
(respectively) to values read from the shared ring in such a way that any
requests/responses that are added to the shared ring whilst the front/back
is detached will be skipped over. This, in general, is not a desirable
semantic since most frontend implementations will eventually block waiting
for a response which would either never appear or never be processed.

Since the macros are currently unused, take this opportunity to re-define
them to re-initialize a front/back ring using specified values. This also
allows FRONT/BACK_RING_INIT() to be re-defined in terms of
FRONT/BACK_RING_ATTACH() using a specified value of 0.

NOTE: BACK_RING_ATTACH() will be used directly in a subsequent patch.

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>

A patch to add the FRONT/BACK_RING_ATTACH() macros to the canonical
ring.h in xen.git will be sent once the definitions have been agreed.

v2:
 - change definitions to take explicit initial index values
---
 include/xen/interface/io/ring.h | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 3f40501fc60b..2af7a1cd6658 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -125,35 +125,24 @@ struct __name##_back_ring {						\
     memset((_s)->pad, 0, sizeof((_s)->pad));				\
 } while(0)
 
-#define FRONT_RING_INIT(_r, _s, __size) do {				\
-    (_r)->req_prod_pvt = 0;						\
-    (_r)->rsp_cons = 0;							\
+#define FRONT_RING_ATTACH(_r, _s, _i, __size) do {			\
+    (_r)->req_prod_pvt = (_i);						\
+    (_r)->rsp_cons = (_i);						\
     (_r)->nr_ents = __RING_SIZE(_s, __size);				\
     (_r)->sring = (_s);							\
 } while (0)
 
-#define BACK_RING_INIT(_r, _s, __size) do {				\
-    (_r)->rsp_prod_pvt = 0;						\
-    (_r)->req_cons = 0;							\
-    (_r)->nr_ents = __RING_SIZE(_s, __size);				\
-    (_r)->sring = (_s);							\
-} while (0)
+#define FRONT_RING_INIT(_r, _s, __size) FRONT_RING_ATTACH(_r, _s, 0, __size)
 
-/* Initialize to existing shared indexes -- for recovery */
-#define FRONT_RING_ATTACH(_r, _s, __size) do {				\
-    (_r)->sring = (_s);							\
-    (_r)->req_prod_pvt = (_s)->req_prod;				\
-    (_r)->rsp_cons = (_s)->rsp_prod;					\
+#define BACK_RING_ATTACH(_r, _s, _i, __size) do {			\
+    (_r)->rsp_prod_pvt = (_i);						\
+    (_r)->req_cons = (_i);						\
     (_r)->nr_ents = __RING_SIZE(_s, __size);				\
-} while (0)
-
-#define BACK_RING_ATTACH(_r, _s, __size) do {				\
     (_r)->sring = (_s);							\
-    (_r)->rsp_prod_pvt = (_s)->rsp_prod;				\
-    (_r)->req_cons = (_s)->req_prod;					\
-    (_r)->nr_ents = __RING_SIZE(_s, __size);				\
 } while (0)
 
+#define BACK_RING_INIT(_r, _s, __size) BACK_RING_ATTACH(_r, _s, 0, __size)
+
 /* How big is this ring? */
 #define RING_SIZE(_r)							\
     ((_r)->nr_ents)
-- 
2.20.1

[PATCH v2 4/4] xen-blkback: support dynamic unbind/bind

[Paul Durrant]

By simply re-attaching to shared rings during connect_ring() rather than
assuming they are freshly allocated (i.e assuming the counters are zero)
it is possible for vbd instances to be unbound and re-bound from and to
(respectively) a running guest.

This has been tested by running:

while true;
  do fio --name=randwrite --ioengine=libaio --iodepth=16 \
  --rw=randwrite --bs=4k --direct=1 --size=1G --verify=crc32;
  done

in a PV guest whilst running:

while true;
  do echo vbd-$DOMID-$VBD >unbind;
  echo unbound;
  sleep 5;
  echo vbd-$DOMID-$VBD >bind;
  echo bound;
  sleep 3;
  done

in dom0 from /sys/bus/xen-backend/drivers/vbd to continuously unbind and
re-bind its system disk image.

This is a highly useful feature for a backend module as it allows it to be
unloaded and re-loaded (i.e. updated) without requiring domUs to be halted.
This was also tested by running:

while true;
  do echo vbd-$DOMID-$VBD >unbind;
  echo unbound;
  sleep 5;
  rmmod xen-blkback;
  echo unloaded;
  sleep 1;
  modprobe xen-blkback;
  echo bound;
  cd $(pwd);
  sleep 3;
  done

in dom0 whilst running the same loop as above in the (single) PV guest.

Some (less stressful) testing has also been done using a Windows HVM guest
with the latest 9.0 PV drivers installed.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

v2:
 - Apply a sanity check to the value of rsp_prod and fail the re-attach
   if it is implausible
 - Set allow_rebind to prevent ring from being closed on unbind
 - Update test workload from dd to fio (with verification)
---
 drivers/block/xen-blkback/xenbus.c | 59 +++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index e8c5c54e1d26..13d09630b237 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -181,6 +181,8 @@ static int xen_blkif_map(struct xen_blkif_ring *ring, grant_ref_t *gref,
 {
 	int err;
 	struct xen_blkif *blkif = ring->blkif;
+	struct blkif_common_sring *sring_common;
+	RING_IDX rsp_prod, req_prod;
 
 	/* Already connected through? */
 	if (ring->irq)
@@ -191,46 +193,66 @@ static int xen_blkif_map(struct xen_blkif_ring *ring, grant_ref_t *gref,
 	if (err < 0)
 		return err;
 
+	sring_common = (struct blkif_common_sring *)ring->blk_ring;
+	rsp_prod = READ_ONCE(sring_common->rsp_prod);
+	req_prod = READ_ONCE(sring_common->req_prod);
+
 	switch (blkif->blk_protocol) {
 	case BLKIF_PROTOCOL_NATIVE:
 	{
-		struct blkif_sring *sring;
-		sring = (struct blkif_sring *)ring->blk_ring;
-		BACK_RING_INIT(&ring->blk_rings.native, sring,
-			       XEN_PAGE_SIZE * nr_grefs);
+		struct blkif_sring *sring_native =
+			(struct blkif_sring *)ring->blk_ring;
+		unsigned int size = __RING_SIZE(sring_native,
+						XEN_PAGE_SIZE * nr_grefs);
+
+		BACK_RING_ATTACH(&ring->blk_rings.native, sring_native,
+				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
+		err = (req_prod - rsp_prod > size) ? -EIO : 0;
 		break;
 	}
 	case BLKIF_PROTOCOL_X86_32:
 	{
-		struct blkif_x86_32_sring *sring_x86_32;
-		sring_x86_32 = (struct blkif_x86_32_sring *)ring->blk_ring;
-		BACK_RING_INIT(&ring->blk_rings.x86_32, sring_x86_32,
-			       XEN_PAGE_SIZE * nr_grefs);
+		struct blkif_x86_32_sring *sring_x86_32 =
+			(struct blkif_x86_32_sring *)ring->blk_ring;
+		unsigned int size = __RING_SIZE(sring_x86_32,
+						XEN_PAGE_SIZE * nr_grefs);
+
+		BACK_RING_ATTACH(&ring->blk_rings.x86_32, sring_x86_32,
+				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
+		err = (req_prod - rsp_prod > size) ? -EIO : 0;
 		break;
 	}
 	case BLKIF_PROTOCOL_X86_64:
 	{
-		struct blkif_x86_64_sring *sring_x86_64;
-		sring_x86_64 = (struct blkif_x86_64_sring *)ring->blk_ring;
-		BACK_RING_INIT(&ring->blk_rings.x86_64, sring_x86_64,
-			       XEN_PAGE_SIZE * nr_grefs);
+		struct blkif_x86_64_sring *sring_x86_64 =
+			(struct blkif_x86_64_sring *)ring->blk_ring;
+		unsigned int size = __RING_SIZE(sring_x86_64,
+						XEN_PAGE_SIZE * nr_grefs);
+
+		BACK_RING_ATTACH(&ring->blk_rings.x86_64, sring_x86_64,
+				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
+		err = (req_prod - rsp_prod > size) ? -EIO : 0;
 		break;
 	}
 	default:
 		BUG();
 	}
+	if (err < 0)
+		goto fail;
 
 	err = bind_interdomain_evtchn_to_irqhandler(blkif->domid, evtchn,
 						    xen_blkif_be_int, 0,
 						    "blkif-backend", ring);
-	if (err < 0) {
-		xenbus_unmap_ring_vfree(blkif->be->dev, ring->blk_ring);
-		ring->blk_rings.common.sring = NULL;
-		return err;
-	}
+	if (err < 0)
+		goto fail;
 	ring->irq = err;
 
 	return 0;
+
+fail:
+	xenbus_unmap_ring_vfree(blkif->be->dev, ring->blk_ring);
+	ring->blk_rings.common.sring = NULL;
+	return err;
 }
 
 static int xen_blkif_disconnect(struct xen_blkif *blkif)
@@ -1121,7 +1143,8 @@ static struct xenbus_driver xen_blkbk_driver = {
 	.ids  = xen_blkbk_ids,
 	.probe = xen_blkbk_probe,
 	.remove = xen_blkbk_remove,
-	.otherend_changed = frontend_changed
+	.otherend_changed = frontend_changed,
+	.allow_rebind = true,
 };
 
 int xen_blkif_xenbus_init(void)
-- 
2.20.1

Re: [PATCH v2 4/4] xen-blkback: support dynamic unbind/bind

[Roger Pau Monné]

On Tue, Dec 10, 2019 at 11:33:47AM +0000, Paul Durrant wrote:
> By simply re-attaching to shared rings during connect_ring() rather than
> assuming they are freshly allocated (i.e assuming the counters are zero)
> it is possible for vbd instances to be unbound and re-bound from and to
> (respectively) a running guest.
> 
> This has been tested by running:
> 
> while true;
>   do fio --name=randwrite --ioengine=libaio --iodepth=16 \
>   --rw=randwrite --bs=4k --direct=1 --size=1G --verify=crc32;
>   done
> 
> in a PV guest whilst running:
> 
> while true;
>   do echo vbd-$DOMID-$VBD >unbind;
>   echo unbound;
>   sleep 5;

Is there anyway to know when the unbind has finished? AFAICT
xen_blkif_disconnect will return EBUSY if there are in flight
requests, and the disconnect won't be completed until those requests
are finished.

>   echo vbd-$DOMID-$VBD >bind;
>   echo bound;
>   sleep 3;
>   done
> 
> in dom0 from /sys/bus/xen-backend/drivers/vbd to continuously unbind and
> re-bind its system disk image.
> 
> This is a highly useful feature for a backend module as it allows it to be
> unloaded and re-loaded (i.e. updated) without requiring domUs to be halted.
> This was also tested by running:
> 
> while true;
>   do echo vbd-$DOMID-$VBD >unbind;
>   echo unbound;
>   sleep 5;
>   rmmod xen-blkback;
>   echo unloaded;
>   sleep 1;
>   modprobe xen-blkback;
>   echo bound;
>   cd $(pwd);
>   sleep 3;
>   done
> 
> in dom0 whilst running the same loop as above in the (single) PV guest.
> 
> Some (less stressful) testing has also been done using a Windows HVM guest
> with the latest 9.0 PV drivers installed.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> 
> v2:
>  - Apply a sanity check to the value of rsp_prod and fail the re-attach
>    if it is implausible
>  - Set allow_rebind to prevent ring from being closed on unbind
>  - Update test workload from dd to fio (with verification)
> ---
>  drivers/block/xen-blkback/xenbus.c | 59 +++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index e8c5c54e1d26..13d09630b237 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -181,6 +181,8 @@ static int xen_blkif_map(struct xen_blkif_ring *ring, grant_ref_t *gref,
>  {
>  	int err;
>  	struct xen_blkif *blkif = ring->blkif;
> +	struct blkif_common_sring *sring_common;
> +	RING_IDX rsp_prod, req_prod;
>  
>  	/* Already connected through? */
>  	if (ring->irq)
> @@ -191,46 +193,66 @@ static int xen_blkif_map(struct xen_blkif_ring *ring, grant_ref_t *gref,
>  	if (err < 0)
>  		return err;
>  
> +	sring_common = (struct blkif_common_sring *)ring->blk_ring;
> +	rsp_prod = READ_ONCE(sring_common->rsp_prod);
> +	req_prod = READ_ONCE(sring_common->req_prod);
> +
>  	switch (blkif->blk_protocol) {
>  	case BLKIF_PROTOCOL_NATIVE:
>  	{
> -		struct blkif_sring *sring;
> -		sring = (struct blkif_sring *)ring->blk_ring;
> -		BACK_RING_INIT(&ring->blk_rings.native, sring,
> -			       XEN_PAGE_SIZE * nr_grefs);
> +		struct blkif_sring *sring_native =
> +			(struct blkif_sring *)ring->blk_ring;

I think you can constify both sring_native and sring_common (and the
other instances below).

> +		unsigned int size = __RING_SIZE(sring_native,
> +						XEN_PAGE_SIZE * nr_grefs);
> +
> +		BACK_RING_ATTACH(&ring->blk_rings.native, sring_native,
> +				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> +		err = (req_prod - rsp_prod > size) ? -EIO : 0;
>  		break;
>  	}
>  	case BLKIF_PROTOCOL_X86_32:
>  	{
> -		struct blkif_x86_32_sring *sring_x86_32;
> -		sring_x86_32 = (struct blkif_x86_32_sring *)ring->blk_ring;
> -		BACK_RING_INIT(&ring->blk_rings.x86_32, sring_x86_32,
> -			       XEN_PAGE_SIZE * nr_grefs);
> +		struct blkif_x86_32_sring *sring_x86_32 =
> +			(struct blkif_x86_32_sring *)ring->blk_ring;
> +		unsigned int size = __RING_SIZE(sring_x86_32,
> +						XEN_PAGE_SIZE * nr_grefs);
> +
> +		BACK_RING_ATTACH(&ring->blk_rings.x86_32, sring_x86_32,
> +				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> +		err = (req_prod - rsp_prod > size) ? -EIO : 0;
>  		break;
>  	}
>  	case BLKIF_PROTOCOL_X86_64:
>  	{
> -		struct blkif_x86_64_sring *sring_x86_64;
> -		sring_x86_64 = (struct blkif_x86_64_sring *)ring->blk_ring;
> -		BACK_RING_INIT(&ring->blk_rings.x86_64, sring_x86_64,
> -			       XEN_PAGE_SIZE * nr_grefs);
> +		struct blkif_x86_64_sring *sring_x86_64 =
> +			(struct blkif_x86_64_sring *)ring->blk_ring;
> +		unsigned int size = __RING_SIZE(sring_x86_64,
> +						XEN_PAGE_SIZE * nr_grefs);
> +
> +		BACK_RING_ATTACH(&ring->blk_rings.x86_64, sring_x86_64,
> +				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> +		err = (req_prod - rsp_prod > size) ? -EIO : 0;

This is repeated for all ring types, might be worth to pull it out of
the switch...

>  		break;
>  	}
>  	default:
>  		BUG();
>  	}
> +	if (err < 0)
> +		goto fail;

...and placed here instead?

Thanks, Roger.

RE: [PATCH v2 4/4] xen-blkback: support dynamic unbind/bind

[Durrant, Paul]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 11 December 2019 10:46
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: xen-devel@lists.xenproject.org; linux-kernel@vger.kernel.org; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Jens Axboe <axboe@kernel.dk>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>; Juergen Gross
> <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v2 4/4] xen-blkback: support dynamic unbind/bind
> 
> On Tue, Dec 10, 2019 at 11:33:47AM +0000, Paul Durrant wrote:
> > By simply re-attaching to shared rings during connect_ring() rather than
> > assuming they are freshly allocated (i.e assuming the counters are zero)
> > it is possible for vbd instances to be unbound and re-bound from and to
> > (respectively) a running guest.
> >
> > This has been tested by running:
> >
> > while true;
> >   do fio --name=randwrite --ioengine=libaio --iodepth=16 \
> >   --rw=randwrite --bs=4k --direct=1 --size=1G --verify=crc32;
> >   done
> >
> > in a PV guest whilst running:
> >
> > while true;
> >   do echo vbd-$DOMID-$VBD >unbind;
> >   echo unbound;
> >   sleep 5;
> 
> Is there anyway to know when the unbind has finished? AFAICT
> xen_blkif_disconnect will return EBUSY if there are in flight
> requests, and the disconnect won't be completed until those requests
> are finished.

Yes, the device sysfs node will disappear when remove() completes.

> 
> >   echo vbd-$DOMID-$VBD >bind;
> >   echo bound;
> >   sleep 3;
> >   done
> >
> > in dom0 from /sys/bus/xen-backend/drivers/vbd to continuously unbind and
> > re-bind its system disk image.
> >
> > This is a highly useful feature for a backend module as it allows it to
> be
> > unloaded and re-loaded (i.e. updated) without requiring domUs to be
> halted.
> > This was also tested by running:
> >
> > while true;
> >   do echo vbd-$DOMID-$VBD >unbind;
> >   echo unbound;
> >   sleep 5;
> >   rmmod xen-blkback;
> >   echo unloaded;
> >   sleep 1;
> >   modprobe xen-blkback;
> >   echo bound;
> >   cd $(pwd);
> >   sleep 3;
> >   done
> >
> > in dom0 whilst running the same loop as above in the (single) PV guest.
> >
> > Some (less stressful) testing has also been done using a Windows HVM
> guest
> > with the latest 9.0 PV drivers installed.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > ---
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> >
> > v2:
> >  - Apply a sanity check to the value of rsp_prod and fail the re-attach
> >    if it is implausible
> >  - Set allow_rebind to prevent ring from being closed on unbind
> >  - Update test workload from dd to fio (with verification)
> > ---
> >  drivers/block/xen-blkback/xenbus.c | 59 +++++++++++++++++++++---------
> >  1 file changed, 41 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-
> blkback/xenbus.c
> > index e8c5c54e1d26..13d09630b237 100644
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -181,6 +181,8 @@ static int xen_blkif_map(struct xen_blkif_ring
> *ring, grant_ref_t *gref,
> >  {
> >  	int err;
> >  	struct xen_blkif *blkif = ring->blkif;
> > +	struct blkif_common_sring *sring_common;
> > +	RING_IDX rsp_prod, req_prod;
> >
> >  	/* Already connected through? */
> >  	if (ring->irq)
> > @@ -191,46 +193,66 @@ static int xen_blkif_map(struct xen_blkif_ring
> *ring, grant_ref_t *gref,
> >  	if (err < 0)
> >  		return err;
> >
> > +	sring_common = (struct blkif_common_sring *)ring->blk_ring;
> > +	rsp_prod = READ_ONCE(sring_common->rsp_prod);
> > +	req_prod = READ_ONCE(sring_common->req_prod);
> > +
> >  	switch (blkif->blk_protocol) {
> >  	case BLKIF_PROTOCOL_NATIVE:
> >  	{
> > -		struct blkif_sring *sring;
> > -		sring = (struct blkif_sring *)ring->blk_ring;
> > -		BACK_RING_INIT(&ring->blk_rings.native, sring,
> > -			       XEN_PAGE_SIZE * nr_grefs);
> > +		struct blkif_sring *sring_native =
> > +			(struct blkif_sring *)ring->blk_ring;
> 
> I think you can constify both sring_native and sring_common (and the
> other instances below).

Yes, I can do that. I don't think the macros would mind.

> 
> > +		unsigned int size = __RING_SIZE(sring_native,
> > +						XEN_PAGE_SIZE * nr_grefs);
> > +
> > +		BACK_RING_ATTACH(&ring->blk_rings.native, sring_native,
> > +				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> > +		err = (req_prod - rsp_prod > size) ? -EIO : 0;
> >  		break;
> >  	}
> >  	case BLKIF_PROTOCOL_X86_32:
> >  	{
> > -		struct blkif_x86_32_sring *sring_x86_32;
> > -		sring_x86_32 = (struct blkif_x86_32_sring *)ring->blk_ring;
> > -		BACK_RING_INIT(&ring->blk_rings.x86_32, sring_x86_32,
> > -			       XEN_PAGE_SIZE * nr_grefs);
> > +		struct blkif_x86_32_sring *sring_x86_32 =
> > +			(struct blkif_x86_32_sring *)ring->blk_ring;
> > +		unsigned int size = __RING_SIZE(sring_x86_32,
> > +						XEN_PAGE_SIZE * nr_grefs);
> > +
> > +		BACK_RING_ATTACH(&ring->blk_rings.x86_32, sring_x86_32,
> > +				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> > +		err = (req_prod - rsp_prod > size) ? -EIO : 0;
> >  		break;
> >  	}
> >  	case BLKIF_PROTOCOL_X86_64:
> >  	{
> > -		struct blkif_x86_64_sring *sring_x86_64;
> > -		sring_x86_64 = (struct blkif_x86_64_sring *)ring->blk_ring;
> > -		BACK_RING_INIT(&ring->blk_rings.x86_64, sring_x86_64,
> > -			       XEN_PAGE_SIZE * nr_grefs);
> > +		struct blkif_x86_64_sring *sring_x86_64 =
> > +			(struct blkif_x86_64_sring *)ring->blk_ring;
> > +		unsigned int size = __RING_SIZE(sring_x86_64,
> > +						XEN_PAGE_SIZE * nr_grefs);
> > +
> > +		BACK_RING_ATTACH(&ring->blk_rings.x86_64, sring_x86_64,
> > +				 rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> > +		err = (req_prod - rsp_prod > size) ? -EIO : 0;
> 
> This is repeated for all ring types, might be worth to pull it out of
> the switch...
> 

I did wonder about that... I'll do in v3.

> >  		break;
> >  	}
> >  	default:
> >  		BUG();
> >  	}
> > +	if (err < 0)
> > +		goto fail;
> 
> ...and placed here instead?

Indeed.

  Cheers,
    Paul

> 
> Thanks, Roger.

RE: [PATCH v2 4/4] xen-blkback: support dynamic unbind/bind

[Durrant, Paul]

> -----Original Message-----
> > > diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-
> > blkback/xenbus.c
> > > index e8c5c54e1d26..13d09630b237 100644
> > > --- a/drivers/block/xen-blkback/xenbus.c
> > > +++ b/drivers/block/xen-blkback/xenbus.c
> > > @@ -181,6 +181,8 @@ static int xen_blkif_map(struct xen_blkif_ring
> > *ring, grant_ref_t *gref,
> > >  {
> > >  	int err;
> > >  	struct xen_blkif *blkif = ring->blkif;
> > > +	struct blkif_common_sring *sring_common;
> > > +	RING_IDX rsp_prod, req_prod;
> > >
> > >  	/* Already connected through? */
> > >  	if (ring->irq)
> > > @@ -191,46 +193,66 @@ static int xen_blkif_map(struct xen_blkif_ring
> > *ring, grant_ref_t *gref,
> > >  	if (err < 0)
> > >  		return err;
> > >
> > > +	sring_common = (struct blkif_common_sring *)ring->blk_ring;
> > > +	rsp_prod = READ_ONCE(sring_common->rsp_prod);
> > > +	req_prod = READ_ONCE(sring_common->req_prod);
> > > +
> > >  	switch (blkif->blk_protocol) {
> > >  	case BLKIF_PROTOCOL_NATIVE:
> > >  	{
> > > -		struct blkif_sring *sring;
> > > -		sring = (struct blkif_sring *)ring->blk_ring;
> > > -		BACK_RING_INIT(&ring->blk_rings.native, sring,
> > > -			       XEN_PAGE_SIZE * nr_grefs);
> > > +		struct blkif_sring *sring_native =
> > > +			(struct blkif_sring *)ring->blk_ring;
> >
> > I think you can constify both sring_native and sring_common (and the
> > other instances below).
> 
> Yes, I can do that. I don't think the macros would mind.
> 

Spoke to soon. They do mind, of course, because the sring pointer in the front/back ring is not (and should not) be const. I can const sring_common but no others.

  Paul