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

[PATCH 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: don't discard pending work in FRONT/BACK_RING_ATTACH
  xen-blkback: support dynamic unbind/bind

 drivers/block/xen-blkback/xenbus.c         | 12 ++++----
 drivers/xen/xenbus/xenbus.h                |  2 --
 drivers/xen/xenbus/xenbus_probe.c          | 34 ++++++----------------
 drivers/xen/xenbus/xenbus_probe_backend.c  |  1 -
 drivers/xen/xenbus/xenbus_probe_frontend.c | 24 ++++++++++++++-
 include/xen/interface/io/ring.h            |  4 +--
 6 files changed, 40 insertions(+), 37 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 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.

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

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

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

[Jürgen Groß]

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

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

[Durrant, Paul]

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

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

[Jürgen Groß]

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 2/4] xenbus: limit when state is forced to closed

[Paul Durrant]

Only force state to closed 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.

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_probe.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index a10311c348b9..c54a53da0106 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);
@@ -264,6 +263,7 @@ int xenbus_dev_remove(struct device *_dev)
 {
 	struct xenbus_device *dev = to_xenbus_device(_dev);
 	struct xenbus_driver *drv = to_xenbus_driver(_dev->driver);
+	enum xenbus_state state;
 
 	DPRINTK("%s", dev->nodename);
 
@@ -276,7 +276,14 @@ int xenbus_dev_remove(struct device *_dev)
 
 	free_otherend_details(dev);
 
-	xenbus_switch_state(dev, XenbusStateClosed);
+	/*
+	 * If the toolstack had force the device state to closing then set
+	 * the state to closed now to allow it to be cleaned up.
+	 */
+	state = xenbus_read_driver_state(dev->nodename);
+	if (state == XenbusStateClosing)
+		xenbus_switch_state(dev, XenbusStateClosed);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xenbus_dev_remove);
-- 
2.20.1

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

[Roger Pau Monné]

On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> Only force state to closed 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.

I'm not sure I see the point of this, I would expect that a failure to
probe or the removal of the device would leave the xenbus state as
closed, which is consistent with the actual driver state.

Can you explain what's the benefit of leaving a device without a
driver in such unknown state?

Thanks, Roger.

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

[Jürgen Groß]

On 09.12.19 12:39, Roger Pau Monné wrote:
> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
>> Only force state to closed 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.
> 
> I'm not sure I see the point of this, I would expect that a failure to
> probe or the removal of the device would leave the xenbus state as
> closed, which is consistent with the actual driver state.
> 
> Can you explain what's the benefit of leaving a device without a
> driver in such unknown state?

And more concerning: did you check that no frontend/backend is
relying on the closed state to be visible without closing having been
set before?


Juergen

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

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 11:55
> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
> <pdurrant@amazon.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On 09.12.19 12:39, Roger Pau Monné wrote:
> > On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> >> Only force state to closed 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.
> >
> > I'm not sure I see the point of this, I would expect that a failure to
> > probe or the removal of the device would leave the xenbus state as
> > closed, which is consistent with the actual driver state.
> >
> > Can you explain what's the benefit of leaving a device without a
> > driver in such unknown state?
> 
> And more concerning: did you check that no frontend/backend is
> relying on the closed state to be visible without closing having been
> set before?

Blkfront doesn't seem to mind and I believe the Windows PV drivers cope, but I don't really understand the comment since this patch is actually removing a case where the backend transitions directly to closed.

  Paul

> 
> 
> Juergen

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

[Jürgen Groß]

On 09.12.19 13:03, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <jgross@suse.com>
>> Sent: 09 December 2019 11:55
>> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
>> <pdurrant@amazon.com>
>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>
>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
>> closed
>>
>> On 09.12.19 12:39, Roger Pau Monné wrote:
>>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
>>>> Only force state to closed 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.
>>>
>>> I'm not sure I see the point of this, I would expect that a failure to
>>> probe or the removal of the device would leave the xenbus state as
>>> closed, which is consistent with the actual driver state.
>>>
>>> Can you explain what's the benefit of leaving a device without a
>>> driver in such unknown state?
>>
>> And more concerning: did you check that no frontend/backend is
>> relying on the closed state to be visible without closing having been
>> set before?
> 
> Blkfront doesn't seem to mind and I believe the Windows PV drivers cope, but I don't really understand the comment since this patch is actually removing a case where the backend transitions directly to closed.

I'm not speaking of blkfront/blkback only, but of net, tpm, scsi, pvcall
etc. frontends/backends. After all you are modifying a function common
to all PV driver pairs.

You are removing a state switc to "closed" in case the state was _not_
"closing" before. So any PV driver reacting to "closed" of the other end
in case the previous state might not have been "closing" before is at
risk to misbehave with your patch.


Juergen

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

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 12:09
> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On 09.12.19 13:03, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jürgen Groß <jgross@suse.com>
> >> Sent: 09 December 2019 11:55
> >> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
> >> <pdurrant@amazon.com>
> >> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> Stefano
> >> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >> <boris.ostrovsky@oracle.com>
> >> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
> to
> >> closed
> >>
> >> On 09.12.19 12:39, Roger Pau Monné wrote:
> >>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> >>>> Only force state to closed 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.
> >>>
> >>> I'm not sure I see the point of this, I would expect that a failure to
> >>> probe or the removal of the device would leave the xenbus state as
> >>> closed, which is consistent with the actual driver state.
> >>>
> >>> Can you explain what's the benefit of leaving a device without a
> >>> driver in such unknown state?
> >>
> >> And more concerning: did you check that no frontend/backend is
> >> relying on the closed state to be visible without closing having been
> >> set before?
> >
> > Blkfront doesn't seem to mind and I believe the Windows PV drivers cope,
> but I don't really understand the comment since this patch is actually
> removing a case where the backend transitions directly to closed.
> 
> I'm not speaking of blkfront/blkback only, but of net, tpm, scsi, pvcall
> etc. frontends/backends. After all you are modifying a function common
> to all PV driver pairs.
> 
> You are removing a state switc to "closed" in case the state was _not_
> "closing" before.

Yes, which AFAIK is against the intention of the generic PV protocol such that it ever existed anyway.

> So any PV driver reacting to "closed" of the other end
> in case the previous state might not have been "closing" before is at
> risk to misbehave with your patch.

Well, they will see nothing now. If the state was not closing, it gets left alone, so the frontend shouldn't do anything. The only risk that I can see is that some frontend/backend pair needed a direct 4 -> 6 transition to support 'unbind' before but AFAIK nothing has ever supported that, and blk and net crash'n'burn if you try that on upstream as it stands. A clean unplug would always set state to 5 first, since that's part of the unplug protocol.

  Paul

> 
> Juergen

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

[Jürgen Groß]

On 09.12.19 13:19, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <jgross@suse.com>
>> Sent: 09 December 2019 12:09
>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
>> <roger.pau@citrix.com>
>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>
>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
>> closed
>>
>> On 09.12.19 13:03, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Jürgen Groß <jgross@suse.com>
>>>> Sent: 09 December 2019 11:55
>>>> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
>>>> <pdurrant@amazon.com>
>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
>> Stefano
>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>>>> <boris.ostrovsky@oracle.com>
>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
>> to
>>>> closed
>>>>
>>>> On 09.12.19 12:39, Roger Pau Monné wrote:
>>>>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
>>>>>> Only force state to closed 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.
>>>>>
>>>>> I'm not sure I see the point of this, I would expect that a failure to
>>>>> probe or the removal of the device would leave the xenbus state as
>>>>> closed, which is consistent with the actual driver state.
>>>>>
>>>>> Can you explain what's the benefit of leaving a device without a
>>>>> driver in such unknown state?
>>>>
>>>> And more concerning: did you check that no frontend/backend is
>>>> relying on the closed state to be visible without closing having been
>>>> set before?
>>>
>>> Blkfront doesn't seem to mind and I believe the Windows PV drivers cope,
>> but I don't really understand the comment since this patch is actually
>> removing a case where the backend transitions directly to closed.
>>
>> I'm not speaking of blkfront/blkback only, but of net, tpm, scsi, pvcall
>> etc. frontends/backends. After all you are modifying a function common
>> to all PV driver pairs.
>>
>> You are removing a state switc to "closed" in case the state was _not_
>> "closing" before.
> 
> Yes, which AFAIK is against the intention of the generic PV protocol such that it ever existed anyway.

While this might be the case we should _not_ break any guests
running now. So this kind of reasoning is dangerous.

> 
>> So any PV driver reacting to "closed" of the other end
>> in case the previous state might not have been "closing" before is at
>> risk to misbehave with your patch.
> 
> Well, they will see nothing now. If the state was not closing, it gets left alone, so the frontend shouldn't do anything. The only risk that I can see is that some frontend/backend pair needed a direct 4 -> 6 transition to support 'unbind' before but AFAIK nothing has ever supported that, and blk and net crash'n'burn if you try that on upstream as it stands. A clean unplug would always set state to 5 first, since that's part of the unplug protocol.

That was my question: are you sure all current and previous
guest frontends and backends are handling unplug this way?

Not "should handle", but "do handle".


Juergen

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

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 13:39
> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On 09.12.19 13:19, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jürgen Groß <jgross@suse.com>
> >> Sent: 09 December 2019 12:09
> >> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> >> <roger.pau@citrix.com>
> >> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> Stefano
> >> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >> <boris.ostrovsky@oracle.com>
> >> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
> to
> >> closed
> >>
> >> On 09.12.19 13:03, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Jürgen Groß <jgross@suse.com>
> >>>> Sent: 09 December 2019 11:55
> >>>> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
> >>>> <pdurrant@amazon.com>
> >>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> >> Stefano
> >>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >>>> <boris.ostrovsky@oracle.com>
> >>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> forced
> >> to
> >>>> closed
> >>>>
> >>>> On 09.12.19 12:39, Roger Pau Monné wrote:
> >>>>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> >>>>>> Only force state to closed 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.
> >>>>>
> >>>>> I'm not sure I see the point of this, I would expect that a failure
> to
> >>>>> probe or the removal of the device would leave the xenbus state as
> >>>>> closed, which is consistent with the actual driver state.
> >>>>>
> >>>>> Can you explain what's the benefit of leaving a device without a
> >>>>> driver in such unknown state?
> >>>>
> >>>> And more concerning: did you check that no frontend/backend is
> >>>> relying on the closed state to be visible without closing having been
> >>>> set before?
> >>>
> >>> Blkfront doesn't seem to mind and I believe the Windows PV drivers
> cope,
> >> but I don't really understand the comment since this patch is actually
> >> removing a case where the backend transitions directly to closed.
> >>
> >> I'm not speaking of blkfront/blkback only, but of net, tpm, scsi,
> pvcall
> >> etc. frontends/backends. After all you are modifying a function common
> >> to all PV driver pairs.
> >>
> >> You are removing a state switc to "closed" in case the state was _not_
> >> "closing" before.
> >
> > Yes, which AFAIK is against the intention of the generic PV protocol
> such that it ever existed anyway.
> 
> While this might be the case we should _not_ break any guests
> running now. So this kind of reasoning is dangerous.
> 
> >
> >> So any PV driver reacting to "closed" of the other end
> >> in case the previous state might not have been "closing" before is at
> >> risk to misbehave with your patch.
> >
> > Well, they will see nothing now. If the state was not closing, it gets
> left alone, so the frontend shouldn't do anything. The only risk that I
> can see is that some frontend/backend pair needed a direct 4 -> 6
> transition to support 'unbind' before but AFAIK nothing has ever supported
> that, and blk and net crash'n'burn if you try that on upstream as it
> stands. A clean unplug would always set state to 5 first, since that's
> part of the unplug protocol.
> 
> That was my question: are you sure all current and previous
> guest frontends and backends are handling unplug this way?
> 
> Not "should handle", but "do handle".

That depends on the toolstack. IIUC the only 'supported' toolstack is xl/libxl, which will set 'state' to 5 and 'online' to 0 to initiate an unplug.

  Paul

> 
> 
> Juergen

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

[Jürgen Groß]

On 09.12.19 15:06, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <jgross@suse.com>
>> Sent: 09 December 2019 13:39
>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
>> <roger.pau@citrix.com>
>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>
>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
>> closed
>>
>> On 09.12.19 13:19, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Jürgen Groß <jgross@suse.com>
>>>> Sent: 09 December 2019 12:09
>>>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
>>>> <roger.pau@citrix.com>
>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
>> Stefano
>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>>>> <boris.ostrovsky@oracle.com>
>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
>> to
>>>> closed
>>>>
>>>> On 09.12.19 13:03, Durrant, Paul wrote:
>>>>>> -----Original Message-----
>>>>>> From: Jürgen Groß <jgross@suse.com>
>>>>>> Sent: 09 December 2019 11:55
>>>>>> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
>>>>>> <pdurrant@amazon.com>
>>>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
>>>> Stefano
>>>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>>>>>> <boris.ostrovsky@oracle.com>
>>>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
>> forced
>>>> to
>>>>>> closed
>>>>>>
>>>>>> On 09.12.19 12:39, Roger Pau Monné wrote:
>>>>>>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
>>>>>>>> Only force state to closed 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.
>>>>>>>
>>>>>>> I'm not sure I see the point of this, I would expect that a failure
>> to
>>>>>>> probe or the removal of the device would leave the xenbus state as
>>>>>>> closed, which is consistent with the actual driver state.
>>>>>>>
>>>>>>> Can you explain what's the benefit of leaving a device without a
>>>>>>> driver in such unknown state?
>>>>>>
>>>>>> And more concerning: did you check that no frontend/backend is
>>>>>> relying on the closed state to be visible without closing having been
>>>>>> set before?
>>>>>
>>>>> Blkfront doesn't seem to mind and I believe the Windows PV drivers
>> cope,
>>>> but I don't really understand the comment since this patch is actually
>>>> removing a case where the backend transitions directly to closed.
>>>>
>>>> I'm not speaking of blkfront/blkback only, but of net, tpm, scsi,
>> pvcall
>>>> etc. frontends/backends. After all you are modifying a function common
>>>> to all PV driver pairs.
>>>>
>>>> You are removing a state switc to "closed" in case the state was _not_
>>>> "closing" before.
>>>
>>> Yes, which AFAIK is against the intention of the generic PV protocol
>> such that it ever existed anyway.
>>
>> While this might be the case we should _not_ break any guests
>> running now. So this kind of reasoning is dangerous.
>>
>>>
>>>> So any PV driver reacting to "closed" of the other end
>>>> in case the previous state might not have been "closing" before is at
>>>> risk to misbehave with your patch.
>>>
>>> Well, they will see nothing now. If the state was not closing, it gets
>> left alone, so the frontend shouldn't do anything. The only risk that I
>> can see is that some frontend/backend pair needed a direct 4 -> 6
>> transition to support 'unbind' before but AFAIK nothing has ever supported
>> that, and blk and net crash'n'burn if you try that on upstream as it
>> stands. A clean unplug would always set state to 5 first, since that's
>> part of the unplug protocol.
>>
>> That was my question: are you sure all current and previous
>> guest frontends and backends are handling unplug this way?
>>
>> Not "should handle", but "do handle".
> 
> That depends on the toolstack. IIUC the only 'supported' toolstack is xl/libxl, which will set 'state' to 5 and 'online' to 0 to initiate an unplug.

I guess libvirt/libxl is doing the same?

At least at SUSE we still have some customers running xend based
Xen installations with recent Linux or Windows guests.


Juergen

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

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 14:10
> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On 09.12.19 15:06, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jürgen Groß <jgross@suse.com>
> >> Sent: 09 December 2019 13:39
> >> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> >> <roger.pau@citrix.com>
> >> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> Stefano
> >> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >> <boris.ostrovsky@oracle.com>
> >> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
> to
> >> closed
> >>
> >> On 09.12.19 13:19, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Jürgen Groß <jgross@suse.com>
> >>>> Sent: 09 December 2019 12:09
> >>>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> >>>> <roger.pau@citrix.com>
> >>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> >> Stefano
> >>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >>>> <boris.ostrovsky@oracle.com>
> >>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> forced
> >> to
> >>>> closed
> >>>>
> >>>> On 09.12.19 13:03, Durrant, Paul wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Jürgen Groß <jgross@suse.com>
> >>>>>> Sent: 09 December 2019 11:55
> >>>>>> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
> >>>>>> <pdurrant@amazon.com>
> >>>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> >>>> Stefano
> >>>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >>>>>> <boris.ostrovsky@oracle.com>
> >>>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> >> forced
> >>>> to
> >>>>>> closed
> >>>>>>
> >>>>>> On 09.12.19 12:39, Roger Pau Monné wrote:
> >>>>>>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> >>>>>>>> Only force state to closed 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.
> >>>>>>>
> >>>>>>> I'm not sure I see the point of this, I would expect that a
> failure
> >> to
> >>>>>>> probe or the removal of the device would leave the xenbus state as
> >>>>>>> closed, which is consistent with the actual driver state.
> >>>>>>>
> >>>>>>> Can you explain what's the benefit of leaving a device without a
> >>>>>>> driver in such unknown state?
> >>>>>>
> >>>>>> And more concerning: did you check that no frontend/backend is
> >>>>>> relying on the closed state to be visible without closing having
> been
> >>>>>> set before?
> >>>>>
> >>>>> Blkfront doesn't seem to mind and I believe the Windows PV drivers
> >> cope,
> >>>> but I don't really understand the comment since this patch is
> actually
> >>>> removing a case where the backend transitions directly to closed.
> >>>>
> >>>> I'm not speaking of blkfront/blkback only, but of net, tpm, scsi,
> >> pvcall
> >>>> etc. frontends/backends. After all you are modifying a function
> common
> >>>> to all PV driver pairs.
> >>>>
> >>>> You are removing a state switc to "closed" in case the state was
> _not_
> >>>> "closing" before.
> >>>
> >>> Yes, which AFAIK is against the intention of the generic PV protocol
> >> such that it ever existed anyway.
> >>
> >> While this might be the case we should _not_ break any guests
> >> running now. So this kind of reasoning is dangerous.
> >>
> >>>
> >>>> So any PV driver reacting to "closed" of the other end
> >>>> in case the previous state might not have been "closing" before is at
> >>>> risk to misbehave with your patch.
> >>>
> >>> Well, they will see nothing now. If the state was not closing, it gets
> >> left alone, so the frontend shouldn't do anything. The only risk that I
> >> can see is that some frontend/backend pair needed a direct 4 -> 6
> >> transition to support 'unbind' before but AFAIK nothing has ever
> supported
> >> that, and blk and net crash'n'burn if you try that on upstream as it
> >> stands. A clean unplug would always set state to 5 first, since that's
> >> part of the unplug protocol.
> >>
> >> That was my question: are you sure all current and previous
> >> guest frontends and backends are handling unplug this way?
> >>
> >> Not "should handle", but "do handle".
> >
> > That depends on the toolstack. IIUC the only 'supported' toolstack is
> xl/libxl, which will set 'state' to 5 and 'online' to 0 to initiate an
> unplug.
> 
> I guess libvirt/libxl is doing the same?
> 

The unplug mechansism is all in libxl AFAICT, so it should be identical.

> At least at SUSE we still have some customers running xend based
> Xen installations with recent Linux or Windows guests.
> 

Is that something the upstream code can/should support though? I'd be surprised if xend is actually doing anything different to libxl since I've been coding the Windows PV drivers to trigger off the combined closing/online transition for as long as I can remember.

  Paul

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

[Jürgen Groß]

On 09.12.19 15:23, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <jgross@suse.com>
>> Sent: 09 December 2019 14:10
>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
>> <roger.pau@citrix.com>
>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>
>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
>> closed
>>
>> On 09.12.19 15:06, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Jürgen Groß <jgross@suse.com>
>>>> Sent: 09 December 2019 13:39
>>>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
>>>> <roger.pau@citrix.com>
>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
>> Stefano
>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>>>> <boris.ostrovsky@oracle.com>
>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
>> to
>>>> closed
>>>>
>>>> On 09.12.19 13:19, Durrant, Paul wrote:
>>>>>> -----Original Message-----
>>>>>> From: Jürgen Groß <jgross@suse.com>
>>>>>> Sent: 09 December 2019 12:09
>>>>>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
>>>>>> <roger.pau@citrix.com>
>>>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
>>>> Stefano
>>>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>>>>>> <boris.ostrovsky@oracle.com>
>>>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
>> forced
>>>> to
>>>>>> closed
>>>>>>
>>>>>> On 09.12.19 13:03, Durrant, Paul wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Jürgen Groß <jgross@suse.com>
>>>>>>>> Sent: 09 December 2019 11:55
>>>>>>>> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
>>>>>>>> <pdurrant@amazon.com>
>>>>>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
>>>>>> Stefano
>>>>>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
>>>>>>>> <boris.ostrovsky@oracle.com>
>>>>>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
>>>> forced
>>>>>> to
>>>>>>>> closed
>>>>>>>>
>>>>>>>> On 09.12.19 12:39, Roger Pau Monné wrote:
>>>>>>>>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
>>>>>>>>>> Only force state to closed 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.
>>>>>>>>>
>>>>>>>>> I'm not sure I see the point of this, I would expect that a
>> failure
>>>> to
>>>>>>>>> probe or the removal of the device would leave the xenbus state as
>>>>>>>>> closed, which is consistent with the actual driver state.
>>>>>>>>>
>>>>>>>>> Can you explain what's the benefit of leaving a device without a
>>>>>>>>> driver in such unknown state?
>>>>>>>>
>>>>>>>> And more concerning: did you check that no frontend/backend is
>>>>>>>> relying on the closed state to be visible without closing having
>> been
>>>>>>>> set before?
>>>>>>>
>>>>>>> Blkfront doesn't seem to mind and I believe the Windows PV drivers
>>>> cope,
>>>>>> but I don't really understand the comment since this patch is
>> actually
>>>>>> removing a case where the backend transitions directly to closed.
>>>>>>
>>>>>> I'm not speaking of blkfront/blkback only, but of net, tpm, scsi,
>>>> pvcall
>>>>>> etc. frontends/backends. After all you are modifying a function
>> common
>>>>>> to all PV driver pairs.
>>>>>>
>>>>>> You are removing a state switc to "closed" in case the state was
>> _not_
>>>>>> "closing" before.
>>>>>
>>>>> Yes, which AFAIK is against the intention of the generic PV protocol
>>>> such that it ever existed anyway.
>>>>
>>>> While this might be the case we should _not_ break any guests
>>>> running now. So this kind of reasoning is dangerous.
>>>>
>>>>>
>>>>>> So any PV driver reacting to "closed" of the other end
>>>>>> in case the previous state might not have been "closing" before is at
>>>>>> risk to misbehave with your patch.
>>>>>
>>>>> Well, they will see nothing now. If the state was not closing, it gets
>>>> left alone, so the frontend shouldn't do anything. The only risk that I
>>>> can see is that some frontend/backend pair needed a direct 4 -> 6
>>>> transition to support 'unbind' before but AFAIK nothing has ever
>> supported
>>>> that, and blk and net crash'n'burn if you try that on upstream as it
>>>> stands. A clean unplug would always set state to 5 first, since that's
>>>> part of the unplug protocol.
>>>>
>>>> That was my question: are you sure all current and previous
>>>> guest frontends and backends are handling unplug this way?
>>>>
>>>> Not "should handle", but "do handle".
>>>
>>> That depends on the toolstack. IIUC the only 'supported' toolstack is
>> xl/libxl, which will set 'state' to 5 and 'online' to 0 to initiate an
>> unplug.
>>
>> I guess libvirt/libxl is doing the same?
>>
> 
> The unplug mechansism is all in libxl AFAICT, so it should be identical.
> 
>> At least at SUSE we still have some customers running xend based
>> Xen installations with recent Linux or Windows guests.
>>
> 
> Is that something the upstream code can/should support though? I'd be surprised if xend is actually doing anything different to libxl since I've been coding the Windows PV drivers to trigger off the combined closing/online transition for as long as I can remember.

I'd rather not have to carry a private patch for new Linux kernel to be
able to run on those hosts.

AFAIK you at Amazon have some quite old Xen installations, too. How are
you handling that (assuming the customer is updating the kernel to a
recent version in his guest)?


Juergen

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

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 14:41
> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On 09.12.19 15:23, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jürgen Groß <jgross@suse.com>
> >> Sent: 09 December 2019 14:10
> >> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> >> <roger.pau@citrix.com>
> >> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> Stefano
> >> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >> <boris.ostrovsky@oracle.com>
> >> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced
> to
> >> closed
> >>
> >> On 09.12.19 15:06, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Jürgen Groß <jgross@suse.com>
> >>>> Sent: 09 December 2019 13:39
> >>>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> >>>> <roger.pau@citrix.com>
> >>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> >> Stefano
> >>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >>>> <boris.ostrovsky@oracle.com>
> >>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> forced
> >> to
> >>>> closed
> >>>>
> >>>> On 09.12.19 13:19, Durrant, Paul wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Jürgen Groß <jgross@suse.com>
> >>>>>> Sent: 09 December 2019 12:09
> >>>>>> To: Durrant, Paul <pdurrant@amazon.com>; Roger Pau Monné
> >>>>>> <roger.pau@citrix.com>
> >>>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> >>>> Stefano
> >>>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >>>>>> <boris.ostrovsky@oracle.com>
> >>>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> >> forced
> >>>> to
> >>>>>> closed
> >>>>>>
> >>>>>> On 09.12.19 13:03, Durrant, Paul wrote:
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Jürgen Groß <jgross@suse.com>
> >>>>>>>> Sent: 09 December 2019 11:55
> >>>>>>>> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
> >>>>>>>> <pdurrant@amazon.com>
> >>>>>>>> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> >>>>>> Stefano
> >>>>>>>> Stabellini <sstabellini@kernel.org>; Boris Ostrovsky
> >>>>>>>> <boris.ostrovsky@oracle.com>
> >>>>>>>> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> >>>> forced
> >>>>>> to
> >>>>>>>> closed
> >>>>>>>>
> >>>>>>>> On 09.12.19 12:39, Roger Pau Monné wrote:
> >>>>>>>>> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> >>>>>>>>>> Only force state to closed 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.
> >>>>>>>>>
> >>>>>>>>> I'm not sure I see the point of this, I would expect that a
> >> failure
> >>>> to
> >>>>>>>>> probe or the removal of the device would leave the xenbus state
> as
> >>>>>>>>> closed, which is consistent with the actual driver state.
> >>>>>>>>>
> >>>>>>>>> Can you explain what's the benefit of leaving a device without a
> >>>>>>>>> driver in such unknown state?
> >>>>>>>>
> >>>>>>>> And more concerning: did you check that no frontend/backend is
> >>>>>>>> relying on the closed state to be visible without closing having
> >> been
> >>>>>>>> set before?
> >>>>>>>
> >>>>>>> Blkfront doesn't seem to mind and I believe the Windows PV drivers
> >>>> cope,
> >>>>>> but I don't really understand the comment since this patch is
> >> actually
> >>>>>> removing a case where the backend transitions directly to closed.
> >>>>>>
> >>>>>> I'm not speaking of blkfront/blkback only, but of net, tpm, scsi,
> >>>> pvcall
> >>>>>> etc. frontends/backends. After all you are modifying a function
> >> common
> >>>>>> to all PV driver pairs.
> >>>>>>
> >>>>>> You are removing a state switc to "closed" in case the state was
> >> _not_
> >>>>>> "closing" before.
> >>>>>
> >>>>> Yes, which AFAIK is against the intention of the generic PV protocol
> >>>> such that it ever existed anyway.
> >>>>
> >>>> While this might be the case we should _not_ break any guests
> >>>> running now. So this kind of reasoning is dangerous.
> >>>>
> >>>>>
> >>>>>> So any PV driver reacting to "closed" of the other end
> >>>>>> in case the previous state might not have been "closing" before is
> at
> >>>>>> risk to misbehave with your patch.
> >>>>>
> >>>>> Well, they will see nothing now. If the state was not closing, it
> gets
> >>>> left alone, so the frontend shouldn't do anything. The only risk that
> I
> >>>> can see is that some frontend/backend pair needed a direct 4 -> 6
> >>>> transition to support 'unbind' before but AFAIK nothing has ever
> >> supported
> >>>> that, and blk and net crash'n'burn if you try that on upstream as it
> >>>> stands. A clean unplug would always set state to 5 first, since
> that's
> >>>> part of the unplug protocol.
> >>>>
> >>>> That was my question: are you sure all current and previous
> >>>> guest frontends and backends are handling unplug this way?
> >>>>
> >>>> Not "should handle", but "do handle".
> >>>
> >>> That depends on the toolstack. IIUC the only 'supported' toolstack is
> >> xl/libxl, which will set 'state' to 5 and 'online' to 0 to initiate an
> >> unplug.
> >>
> >> I guess libvirt/libxl is doing the same?
> >>
> >
> > The unplug mechansism is all in libxl AFAICT, so it should be identical.
> >
> >> At least at SUSE we still have some customers running xend based
> >> Xen installations with recent Linux or Windows guests.
> >>
> >
> > Is that something the upstream code can/should support though? I'd be
> surprised if xend is actually doing anything different to libxl since I've
> been coding the Windows PV drivers to trigger off the combined
> closing/online transition for as long as I can remember.
> 
> I'd rather not have to carry a private patch for new Linux kernel to be
> able to run on those hosts.
> 
> AFAIK you at Amazon have some quite old Xen installations, too. How are
> you handling that (assuming the customer is updating the kernel to a
> recent version in his guest)?

I'm not aware of any problems running with xend (but I'm not I the loop on everything). I think it is still a reasonably safe assumption that xend initiated unplug cleanly and doesn't rely on a 4 -> 6 transition.

  Paul

> 
> 
> Juergen

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

[Durrant, Paul]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 09 December 2019 11:39
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Juergen
> Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> > Only force state to closed 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.
> 
> I'm not sure I see the point of this, I would expect that a failure to
> probe or the removal of the device would leave the xenbus state as
> closed, which is consistent with the actual driver state.
> 
> Can you explain what's the benefit of leaving a device without a
> driver in such unknown state?
> 

If probe fails then I think it should leave the state alone. If the state is moved to closed then basically you just killed that connection to the guest (as the frontend will normally close down when it sees this change) so, if the probe failure was due to a bug in blkback or, e.g., a transient resource issue then it's game over as far as that guest goes.
The ultimate goal here is PV backend re-load that is completely transparent to the guest. Modifying anything in xenstore compromises that so we need to be careful.

  Paul

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

[Roger Pau Monné]

On Mon, Dec 09, 2019 at 12:01:38PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: 09 December 2019 11:39
> > To: Durrant, Paul <pdurrant@amazon.com>
> > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Juergen
> > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> > closed
> > 
> > On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> > > Only force state to closed 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.
> > 
> > I'm not sure I see the point of this, I would expect that a failure to
> > probe or the removal of the device would leave the xenbus state as
> > closed, which is consistent with the actual driver state.
> > 
> > Can you explain what's the benefit of leaving a device without a
> > driver in such unknown state?
> > 
> 
> If probe fails then I think it should leave the state alone. If the
> state is moved to closed then basically you just killed that
> connection to the guest (as the frontend will normally close down
> when it sees this change) so, if the probe failure was due to a bug
> in blkback or, e.g., a transient resource issue then it's game over
> as far as that guest goes.

But the connection can be restarted by switching the backend to the
init state again.

> The ultimate goal here is PV backend re-load that is completely transparent to the guest. Modifying anything in xenstore compromises that so we need to be careful.

That's a fine goal, but not switching to closed state in
xenbus_dev_remove seems wrong, as you have actually left the frontend
without a matching backend and with the state not set to closed.

Ie: that would be fine if you explicitly state this is some kind of
internal blkback reload, but not for the general case where blkback
has been unbound. I think we need someway to difference a blkback
reload vs a unbound.

Thanks, Roger.

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

[Durrant, Paul]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 09 December 2019 12:26
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Juergen
> Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On Mon, Dec 09, 2019 at 12:01:38PM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > Sent: 09 December 2019 11:39
> > > To: Durrant, Paul <pdurrant@amazon.com>
> > > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> Juergen
> > > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> forced to
> > > closed
> > >
> > > On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> > > > Only force state to closed 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.
> > >
> > > I'm not sure I see the point of this, I would expect that a failure to
> > > probe or the removal of the device would leave the xenbus state as
> > > closed, which is consistent with the actual driver state.
> > >
> > > Can you explain what's the benefit of leaving a device without a
> > > driver in such unknown state?
> > >
> >
> > If probe fails then I think it should leave the state alone. If the
> > state is moved to closed then basically you just killed that
> > connection to the guest (as the frontend will normally close down
> > when it sees this change) so, if the probe failure was due to a bug
> > in blkback or, e.g., a transient resource issue then it's game over
> > as far as that guest goes.
> 
> But the connection can be restarted by switching the backend to the
> init state again.

Too late. The frontend saw closed and you already lost.

> 
> > The ultimate goal here is PV backend re-load that is completely
> transparent to the guest. Modifying anything in xenstore compromises that
> so we need to be careful.
> 
> That's a fine goal, but not switching to closed state in
> xenbus_dev_remove seems wrong, as you have actually left the frontend
> without a matching backend and with the state not set to closed.
> 

Why is this a problem? With this series fully applied a (block) backend can come and go without needing to change the state. Relying on guests to DTRT is not a sustainable option for a cloud deployment.

> Ie: that would be fine if you explicitly state this is some kind of
> internal blkback reload, but not for the general case where blkback
> has been unbound. I think we need someway to difference a blkback
> reload vs a unbound.
> 

Why do we need that though? Why is it advantageous for a backend to go to closed. No PV backends cope with an unbind as-is, and a toolstack initiated unplug will always set state to 5 anyway. So TBH any state transition done directly in the xenbus code looks wrong to me anyway (but appears to be a necessary evil to keep the toolstack working in the event it spawns a backend where there is actually to driver present, or it doesn't come online).

  Paul

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

[Roger Pau Monné]

On Mon, Dec 09, 2019 at 12:40:47PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: 09 December 2019 12:26
> > To: Durrant, Paul <pdurrant@amazon.com>
> > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Juergen
> > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> > closed
> > 
> > On Mon, Dec 09, 2019 at 12:01:38PM +0000, Durrant, Paul wrote:
> > > > -----Original Message-----
> > > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > > Sent: 09 December 2019 11:39
> > > > To: Durrant, Paul <pdurrant@amazon.com>
> > > > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> > Juergen
> > > > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > > > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> > forced to
> > > > closed
> > > >
> > > > On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> > > > > Only force state to closed 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.
> > > >
> > > > I'm not sure I see the point of this, I would expect that a failure to
> > > > probe or the removal of the device would leave the xenbus state as
> > > > closed, which is consistent with the actual driver state.
> > > >
> > > > Can you explain what's the benefit of leaving a device without a
> > > > driver in such unknown state?
> > > >
> > >
> > > If probe fails then I think it should leave the state alone. If the
> > > state is moved to closed then basically you just killed that
> > > connection to the guest (as the frontend will normally close down
> > > when it sees this change) so, if the probe failure was due to a bug
> > > in blkback or, e.g., a transient resource issue then it's game over
> > > as far as that guest goes.
> > 
> > But the connection can be restarted by switching the backend to the
> > init state again.
> 
> Too late. The frontend saw closed and you already lost.
> 
> > 
> > > The ultimate goal here is PV backend re-load that is completely
> > transparent to the guest. Modifying anything in xenstore compromises that
> > so we need to be careful.
> > 
> > That's a fine goal, but not switching to closed state in
> > xenbus_dev_remove seems wrong, as you have actually left the frontend
> > without a matching backend and with the state not set to closed.
> > 
> 
> Why is this a problem? With this series fully applied a (block) backend can come and go without needing to change the state. Relying on guests to DTRT is not a sustainable option for a cloud deployment.
> 
> > Ie: that would be fine if you explicitly state this is some kind of
> > internal blkback reload, but not for the general case where blkback
> > has been unbound. I think we need someway to difference a blkback
> > reload vs a unbound.
> > 
> 
> Why do we need that though? Why is it advantageous for a backend to go to closed. No PV backends cope with an unbind as-is, and a toolstack initiated unplug will always set state to 5 anyway. So TBH any state transition done directly in the xenbus code looks wrong to me anyway (but appears to be a necessary evil to keep the toolstack working in the event it spawns a backend where there is actually to driver present, or it doesn't come online).

IMO the normal flow for unbind would be to attempt to close open
connections and then remove the driver: leaving frontends connected
without any attached backends is not correct, and will just block the
guest frontend until requests start timing out.

I can see the reasoning for doing that for the purpose of updating a
blkback module without guests noticing, but I would prefer that
leaving connections open was an option that could be given when
unbinding (or maybe a driver option in sysfs?), so that the default
behaviour would be to try to close everything when unbinding if
possible.

Thanks, Roger.

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

[Durrant, Paul]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 09 December 2019 14:29
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Juergen
> Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On Mon, Dec 09, 2019 at 12:40:47PM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > Sent: 09 December 2019 12:26
> > > To: Durrant, Paul <pdurrant@amazon.com>
> > > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> Juergen
> > > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> forced to
> > > closed
> > >
> > > On Mon, Dec 09, 2019 at 12:01:38PM +0000, Durrant, Paul wrote:
> > > > > -----Original Message-----
> > > > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > > > Sent: 09 December 2019 11:39
> > > > > To: Durrant, Paul <pdurrant@amazon.com>
> > > > > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> > > Juergen
> > > > > Gross <jgross@suse.com>; Stefano Stabellini
> <sstabellini@kernel.org>;
> > > > > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > > > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> > > forced to
> > > > > closed
> > > > >
> > > > > On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> > > > > > Only force state to closed 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.
> > > > >
> > > > > I'm not sure I see the point of this, I would expect that a
> failure to
> > > > > probe or the removal of the device would leave the xenbus state as
> > > > > closed, which is consistent with the actual driver state.
> > > > >
> > > > > Can you explain what's the benefit of leaving a device without a
> > > > > driver in such unknown state?
> > > > >
> > > >
> > > > If probe fails then I think it should leave the state alone. If the
> > > > state is moved to closed then basically you just killed that
> > > > connection to the guest (as the frontend will normally close down
> > > > when it sees this change) so, if the probe failure was due to a bug
> > > > in blkback or, e.g., a transient resource issue then it's game over
> > > > as far as that guest goes.
> > >
> > > But the connection can be restarted by switching the backend to the
> > > init state again.
> >
> > Too late. The frontend saw closed and you already lost.
> >
> > >
> > > > The ultimate goal here is PV backend re-load that is completely
> > > transparent to the guest. Modifying anything in xenstore compromises
> that
> > > so we need to be careful.
> > >
> > > That's a fine goal, but not switching to closed state in
> > > xenbus_dev_remove seems wrong, as you have actually left the frontend
> > > without a matching backend and with the state not set to closed.
> > >
> >
> > Why is this a problem? With this series fully applied a (block) backend
> can come and go without needing to change the state. Relying on guests to
> DTRT is not a sustainable option for a cloud deployment.
> >
> > > Ie: that would be fine if you explicitly state this is some kind of
> > > internal blkback reload, but not for the general case where blkback
> > > has been unbound. I think we need someway to difference a blkback
> > > reload vs a unbound.
> > >
> >
> > Why do we need that though? Why is it advantageous for a backend to go
> to closed. No PV backends cope with an unbind as-is, and a toolstack
> initiated unplug will always set state to 5 anyway. So TBH any state
> transition done directly in the xenbus code looks wrong to me anyway (but
> appears to be a necessary evil to keep the toolstack working in the event
> it spawns a backend where there is actually to driver present, or it
> doesn't come online).
> 
> IMO the normal flow for unbind would be to attempt to close open
> connections and then remove the driver: leaving frontends connected
> without any attached backends is not correct, and will just block the
> guest frontend until requests start timing out.
> 
> I can see the reasoning for doing that for the purpose of updating a
> blkback module without guests noticing, but I would prefer that
> leaving connections open was an option that could be given when
> unbinding (or maybe a driver option in sysfs?), so that the default
> behaviour would be to try to close everything when unbinding if
> possible.

Well unbind is pretty useless now IMO since bind doesn't work, and a transition straight to closed is just plain wrong anyway. But, we could have a flag that the backend driver sets to say that it supports transparent re-bind that gates this code. Would that make you feel more comfortable?

If you want unbind to actually do a proper unplug then that's extra work and not really something I want to tackle (and re-bind would still need to be toolstack initiated as something would have to re-create the xenstore area).

  Paul

> 
> Thanks, Roger.

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

[Roger Pau Monné]

On Mon, Dec 09, 2019 at 02:41:40PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: 09 December 2019 14:29
> > To: Durrant, Paul <pdurrant@amazon.com>
> > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Juergen
> > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> > closed
> > 
> > On Mon, Dec 09, 2019 at 12:40:47PM +0000, Durrant, Paul wrote:
> > > > -----Original Message-----
> > > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > > Sent: 09 December 2019 12:26
> > > > To: Durrant, Paul <pdurrant@amazon.com>
> > > > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> > Juergen
> > > > Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > > > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> > forced to
> > > > closed
> > > >
> > > > On Mon, Dec 09, 2019 at 12:01:38PM +0000, Durrant, Paul wrote:
> > > > > > -----Original Message-----
> > > > > > From: Roger Pau Monné <roger.pau@citrix.com>
> > > > > > Sent: 09 December 2019 11:39
> > > > > > To: Durrant, Paul <pdurrant@amazon.com>
> > > > > > Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org;
> > > > Juergen
> > > > > > Gross <jgross@suse.com>; Stefano Stabellini
> > <sstabellini@kernel.org>;
> > > > > > Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > > > > Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is
> > > > forced to
> > > > > > closed
> > > > > >
> > > > > > On Thu, Dec 05, 2019 at 02:01:21PM +0000, Paul Durrant wrote:
> > > > > > > Only force state to closed 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.
> > > > > >
> > > > > > I'm not sure I see the point of this, I would expect that a
> > failure to
> > > > > > probe or the removal of the device would leave the xenbus state as
> > > > > > closed, which is consistent with the actual driver state.
> > > > > >
> > > > > > Can you explain what's the benefit of leaving a device without a
> > > > > > driver in such unknown state?
> > > > > >
> > > > >
> > > > > If probe fails then I think it should leave the state alone. If the
> > > > > state is moved to closed then basically you just killed that
> > > > > connection to the guest (as the frontend will normally close down
> > > > > when it sees this change) so, if the probe failure was due to a bug
> > > > > in blkback or, e.g., a transient resource issue then it's game over
> > > > > as far as that guest goes.
> > > >
> > > > But the connection can be restarted by switching the backend to the
> > > > init state again.
> > >
> > > Too late. The frontend saw closed and you already lost.
> > >
> > > >
> > > > > The ultimate goal here is PV backend re-load that is completely
> > > > transparent to the guest. Modifying anything in xenstore compromises
> > that
> > > > so we need to be careful.
> > > >
> > > > That's a fine goal, but not switching to closed state in
> > > > xenbus_dev_remove seems wrong, as you have actually left the frontend
> > > > without a matching backend and with the state not set to closed.
> > > >
> > >
> > > Why is this a problem? With this series fully applied a (block) backend
> > can come and go without needing to change the state. Relying on guests to
> > DTRT is not a sustainable option for a cloud deployment.
> > >
> > > > Ie: that would be fine if you explicitly state this is some kind of
> > > > internal blkback reload, but not for the general case where blkback
> > > > has been unbound. I think we need someway to difference a blkback
> > > > reload vs a unbound.
> > > >
> > >
> > > Why do we need that though? Why is it advantageous for a backend to go
> > to closed. No PV backends cope with an unbind as-is, and a toolstack
> > initiated unplug will always set state to 5 anyway. So TBH any state
> > transition done directly in the xenbus code looks wrong to me anyway (but
> > appears to be a necessary evil to keep the toolstack working in the event
> > it spawns a backend where there is actually to driver present, or it
> > doesn't come online).
> > 
> > IMO the normal flow for unbind would be to attempt to close open
> > connections and then remove the driver: leaving frontends connected
> > without any attached backends is not correct, and will just block the
> > guest frontend until requests start timing out.
> > 
> > I can see the reasoning for doing that for the purpose of updating a
> > blkback module without guests noticing, but I would prefer that
> > leaving connections open was an option that could be given when
> > unbinding (or maybe a driver option in sysfs?), so that the default
> > behaviour would be to try to close everything when unbinding if
> > possible.
> 
> Well unbind is pretty useless now IMO since bind doesn't work, and a transition straight to closed is just plain wrong anyway.

Why do you claim that a straight transition into the closed state is
wrong?

I don't see any such mention in blkif.h, which also doesn't contain
any guidelines regarding closing state transitions, so unless
otherwise stated somewhere else transitions into closed can happen
from any state IMO.

> But, we could have a flag that the backend driver sets to say that it supports transparent re-bind that gates this code. Would that make you feel more comfortable?

Having an option to leave state untouched when unbinding would be fine
for me, otherwise state should be set to closed when unbinding. I
don't think there's anything else that needs to be done in this
regard, the cleanup should be exactly the same the only difference
being the setting of all the active backends to closed state.

> If you want unbind to actually do a proper unplug then that's extra work and not really something I want to tackle (and re-bind would still need to be toolstack initiated as something would have to re-create the xenstore area).

Why do you say the xenstore area would need to be recreated?

Setting state to closed shouldn't cause any cleanup of the xenstore
area, as that should already happen for example when using pvgrub
since in that case grub itself disconnects and already causes a
transition to closed and a re-attachment afterwards by the guest
kernel.

Thanks, Roger.

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

[Durrant, Paul]

> -----Original Message-----
[snip]
> >
> > Well unbind is pretty useless now IMO since bind doesn't work, and a
> transition straight to closed is just plain wrong anyway.
> 
> Why do you claim that a straight transition into the closed state is
> wrong?

It's badly documented, I agree, but have a look at https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/xen-netback/xenbus.c#n480. Connected -> Closed is not a valid transition, and I don't think it was ever intended to be.

> 
> I don't see any such mention in blkif.h, which also doesn't contain
> any guidelines regarding closing state transitions, so unless
> otherwise stated somewhere else transitions into closed can happen
> from any state IMO.
> 

They can, but it is even more poorly documented what should be done in this case.

> > But, we could have a flag that the backend driver sets to say that it
> supports transparent re-bind that gates this code. Would that make you
> feel more comfortable?
> 
> Having an option to leave state untouched when unbinding would be fine
> for me, otherwise state should be set to closed when unbinding. I
> don't think there's anything else that needs to be done in this
> regard, the cleanup should be exactly the same the only difference
> being the setting of all the active backends to closed state.
> 

Ok, I'll add such a flag and define it for blkback only, in patch #4 i.e. when it actually gains the ability to rebind.

> > If you want unbind to actually do a proper unplug then that's extra work
> and not really something I want to tackle (and re-bind would still need to
> be toolstack initiated as something would have to re-create the xenstore
> area).
> 
> Why do you say the xenstore area would need to be recreated?
> 
> Setting state to closed shouldn't cause any cleanup of the xenstore
> area, as that should already happen for example when using pvgrub
> since in that case grub itself disconnects and already causes a
> transition to closed and a re-attachment afterwards by the guest
> kernel.
> 

For some reason, when I originally tested, the xenstore area disappeared. I checked again and it did not this time. I just ended up with a frontend stuck in state 5 (because it is the system disk and won't go offline) trying to talk to a non-existent backend. Upon re-bind the backend goes into state 5 (because it sees the 5 in the frontend) and leaves the guest wedged.

  Paul

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

[Roger Pau Monné]

On Mon, Dec 09, 2019 at 04:26:15PM +0000, Durrant, Paul wrote:
> > > If you want unbind to actually do a proper unplug then that's extra work
> > and not really something I want to tackle (and re-bind would still need to
> > be toolstack initiated as something would have to re-create the xenstore
> > area).
> > 
> > Why do you say the xenstore area would need to be recreated?
> > 
> > Setting state to closed shouldn't cause any cleanup of the xenstore
> > area, as that should already happen for example when using pvgrub
> > since in that case grub itself disconnects and already causes a
> > transition to closed and a re-attachment afterwards by the guest
> > kernel.
> > 
> 
> For some reason, when I originally tested, the xenstore area disappeared. I checked again and it did not this time. I just ended up with a frontend stuck in state 5 (because it is the system disk and won't go offline) trying to talk to a non-existent backend. Upon re-bind the backend goes into state 5 (because it sees the 5 in the frontend) and leaves the guest wedged.

Likely blkfront should go back to init state, but anyway, that's not
something that needs fixing as part of this series.

Thanks, Roger.

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

[Durrant, Paul]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 09 December 2019 17:18
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.org; Juergen
> Gross <jgross@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: Re: [Xen-devel] [PATCH 2/4] xenbus: limit when state is forced to
> closed
> 
> On Mon, Dec 09, 2019 at 04:26:15PM +0000, Durrant, Paul wrote:
> > > > If you want unbind to actually do a proper unplug then that's extra
> work
> > > and not really something I want to tackle (and re-bind would still
> need to
> > > be toolstack initiated as something would have to re-create the
> xenstore
> > > area).
> > >
> > > Why do you say the xenstore area would need to be recreated?
> > >
> > > Setting state to closed shouldn't cause any cleanup of the xenstore
> > > area, as that should already happen for example when using pvgrub
> > > since in that case grub itself disconnects and already causes a
> > > transition to closed and a re-attachment afterwards by the guest
> > > kernel.
> > >
> >
> > For some reason, when I originally tested, the xenstore area
> disappeared. I checked again and it did not this time. I just ended up
> with a frontend stuck in state 5 (because it is the system disk and won't
> go offline) trying to talk to a non-existent backend. Upon re-bind the
> backend goes into state 5 (because it sees the 5 in the frontend) and
> leaves the guest wedged.
> 
> Likely blkfront should go back to init state, but anyway, that's not
> something that needs fixing as part of this series.
> 

Ok, cool.

I am wondering though whether we ought to suppress bind/unbind for drivers that don't whitelist themselves (through the new xenbus_driver flag that I'll add). It's somewhat misleading that the nodes are there but don't necessarily work.

  Paul

[PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH

[Paul Durrant]

Currently these macros will skip over any requests/responses that are
added to the shared ring whilst it is detached. 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.

NOTE: These macros are currently unused. BACK_RING_ATTACH(), however, will
      be used 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>
---
 include/xen/interface/io/ring.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 3f40501fc60b..405adfed87e6 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -143,14 +143,14 @@ struct __name##_back_ring {						\
 #define FRONT_RING_ATTACH(_r, _s, __size) do {				\
     (_r)->sring = (_s);							\
     (_r)->req_prod_pvt = (_s)->req_prod;				\
-    (_r)->rsp_cons = (_s)->rsp_prod;					\
+    (_r)->rsp_cons = (_s)->req_prod;					\
     (_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)->req_cons = (_s)->rsp_prod;					\
     (_r)->nr_ents = __RING_SIZE(_s, __size);				\
 } while (0)
 
-- 
2.20.1

Re: [Xen-devel] [PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH

[Roger Pau Monné]

On Thu, Dec 05, 2019 at 02:01:22PM +0000, Paul Durrant wrote:
> Currently these macros will skip over any requests/responses that are
> added to the shared ring whilst it is detached. 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.
> 
> NOTE: These macros are currently unused. BACK_RING_ATTACH(), however, will
>       be used in a subsequent patch.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Those headers come from Xen, and should be modified in Xen first and
then imported into Linux IMO.

Thanks, Roger.

Re: [Xen-devel] [PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH

[Jürgen Groß]

On 09.12.19 12:41, Roger Pau Monné wrote:
> On Thu, Dec 05, 2019 at 02:01:22PM +0000, Paul Durrant wrote:
>> Currently these macros will skip over any requests/responses that are
>> added to the shared ring whilst it is detached. 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.
>>
>> NOTE: These macros are currently unused. BACK_RING_ATTACH(), however, will
>>        be used in a subsequent patch.
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> Those headers come from Xen, and should be modified in Xen first and
> then imported into Linux IMO.

In theory, yes. But the Xen variant doesn't contain the ATTACH macros.


Juergen

RE: [Xen-devel] [PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Jürgen Groß
> Sent: 09 December 2019 11:52
> To: Roger Pau Monné <roger.pau@citrix.com>; Durrant, Paul
> <pdurrant@amazon.com>
> Cc: xen-devel@lists.xenproject.org; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> linux-kernel@vger.kernel.org
> Subject: Re: [Xen-devel] [PATCH 3/4] xen/interface: don't discard pending
> work in FRONT/BACK_RING_ATTACH
> 
> On 09.12.19 12:41, Roger Pau Monné wrote:
> > On Thu, Dec 05, 2019 at 02:01:22PM +0000, Paul Durrant wrote:
> >> Currently these macros will skip over any requests/responses that are
> >> added to the shared ring whilst it is detached. 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.
> >>
> >> NOTE: These macros are currently unused. BACK_RING_ATTACH(), however,
> will
> >>        be used in a subsequent patch.
> >>
> >> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> >
> > Those headers come from Xen, and should be modified in Xen first and
> > then imported into Linux IMO.
> 
> In theory, yes. But the Xen variant doesn't contain the ATTACH macros.
> 

OOI do we have a policy about this? Re-importing headers into Linux wholesale is always slightly painful because of interdependencies and style checking issues.

  Paul

> 
> Juergen
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH

[Jürgen Groß]

On 05.12.19 15:01, Paul Durrant wrote:
> Currently these macros will skip over any requests/responses that are
> added to the shared ring whilst it is detached. 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.
> 
> NOTE: These macros are currently unused. BACK_RING_ATTACH(), however, will
>        be used 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>
> ---
>   include/xen/interface/io/ring.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> index 3f40501fc60b..405adfed87e6 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
> @@ -143,14 +143,14 @@ struct __name##_back_ring {						\
>   #define FRONT_RING_ATTACH(_r, _s, __size) do {				\
>       (_r)->sring = (_s);							\
>       (_r)->req_prod_pvt = (_s)->req_prod;				\
> -    (_r)->rsp_cons = (_s)->rsp_prod;					\
> +    (_r)->rsp_cons = (_s)->req_prod;					\
>       (_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)->req_cons = (_s)->rsp_prod;					\
>       (_r)->nr_ents = __RING_SIZE(_s, __size);				\
>   } while (0)

Lets look at all possible scenarios where BACK_RING_ATTACH()
might happen:

Initially (after [FRONT|BACK]_RING_INIT(), leaving _pvt away):
req_prod=0, rsp_cons=0, rsp_prod=0, req_cons=0
Using BACK_RING_ATTACH() is fine (no change)

Request queued:
req_prod=1, rsp_cons=0, rsp_prod=0, req_cons=0
Using BACK_RING_ATTACH() is fine (no change)

and taken by backend:
req_prod=1, rsp_cons=0, rsp_prod=0, req_cons=1
Using BACK_RING_ATTACH() is resetting req_cons to 0, will result
in redoing request (for blk this is fine, other devices like SCSI
tapes will have issues with that). One possible solution would be
to ensure all taken requests are either stopped or the response
is queued already.

Response queued:
req_prod=1, rsp_cons=0, rsp_prod=1, req_cons=1
Using BACK_RING_ATTACH() is fine (no change)

Response taken:
req_prod=1, rsp_cons=1, rsp_prod=1, req_cons=1
Using BACK_RING_ATTACH() is fine (no change)

In general I believe the [FRONT|BACK]_RING_ATTACH() macros are not
fine to be used in the current state, as the *_pvt fields normally not
accessible by the other end are initialized using the (possibly
untrusted) values from the shared ring. There needs at least to be a
test for the values to be sane, and your change should not result in the
same value to be read twice, as it could have changed in between.

As this is an error which can happen in other OS's, too, I'd recommend
to add the adapted macros (plus a comment regarding the possible
problem noted above for special devices like tapes) to the Xen variant
of ring.h.


Juergen

RE: [PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 13:55
> 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 3/4] xen/interface: don't discard pending work in
> FRONT/BACK_RING_ATTACH
> 
> On 05.12.19 15:01, Paul Durrant wrote:
> > Currently these macros will skip over any requests/responses that are
> > added to the shared ring whilst it is detached. 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.
> >
> > NOTE: These macros are currently unused. BACK_RING_ATTACH(), however,
> will
> >        be used 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>
> > ---
> >   include/xen/interface/io/ring.h | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/xen/interface/io/ring.h
> b/include/xen/interface/io/ring.h
> > index 3f40501fc60b..405adfed87e6 100644
> > --- a/include/xen/interface/io/ring.h
> > +++ b/include/xen/interface/io/ring.h
> > @@ -143,14 +143,14 @@ struct __name##_back_ring {
> 		\
> >   #define FRONT_RING_ATTACH(_r, _s, __size) do {				\
> >       (_r)->sring = (_s);							\
> >       (_r)->req_prod_pvt = (_s)->req_prod;				\
> > -    (_r)->rsp_cons = (_s)->rsp_prod;					\
> > +    (_r)->rsp_cons = (_s)->req_prod;					\
> >       (_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)->req_cons = (_s)->rsp_prod;					\
> >       (_r)->nr_ents = __RING_SIZE(_s, __size);				\
> >   } while (0)
> 
> Lets look at all possible scenarios where BACK_RING_ATTACH()
> might happen:
> 
> Initially (after [FRONT|BACK]_RING_INIT(), leaving _pvt away):
> req_prod=0, rsp_cons=0, rsp_prod=0, req_cons=0
> Using BACK_RING_ATTACH() is fine (no change)
> 
> Request queued:
> req_prod=1, rsp_cons=0, rsp_prod=0, req_cons=0
> Using BACK_RING_ATTACH() is fine (no change)
> 
> and taken by backend:
> req_prod=1, rsp_cons=0, rsp_prod=0, req_cons=1
> Using BACK_RING_ATTACH() is resetting req_cons to 0, will result
> in redoing request (for blk this is fine, other devices like SCSI
> tapes will have issues with that). One possible solution would be
> to ensure all taken requests are either stopped or the response
> is queued already.

Yes, it is the assumption that a backend will drain and complete any requests it is handling, but it will not deal with new ones being posted by the frontend. This does appear to be the case for blkback.

> 
> Response queued:
> req_prod=1, rsp_cons=0, rsp_prod=1, req_cons=1
> Using BACK_RING_ATTACH() is fine (no change)
> 
> Response taken:
> req_prod=1, rsp_cons=1, rsp_prod=1, req_cons=1
> Using BACK_RING_ATTACH() is fine (no change)
> 
> In general I believe the [FRONT|BACK]_RING_ATTACH() macros are not
> fine to be used in the current state, as the *_pvt fields normally not
> accessible by the other end are initialized using the (possibly
> untrusted) values from the shared ring. There needs at least to be a
> test for the values to be sane, and your change should not result in the
> same value to be read twice, as it could have changed in between.

What test would you apply to sanitize the value of the pvt pointer? Another option would be to have a backend write its pvt value into the xenstore backend area when the ring is unmapped, so that a new instance definitely resumes where the old one left off. The value of rsp_prod could, of course, be overwritten by the guest at any time and so there's little point in attempting sanitize it.

> 
> As this is an error which can happen in other OS's, too, I'd recommend
> to add the adapted macros (plus a comment regarding the possible
> problem noted above for special devices like tapes) to the Xen variant
> of ring.h.
> 

I can certainly send a patch to Xen once we agree on the final definition.

  Paul

> 
> Juergen

Re: [PATCH 3/4] xen/interface: don't discard pending work in FRONT/BACK_RING_ATTACH

[Jürgen Groß]

On 09.12.19 17:38, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jürgen Groß <jgross@suse.com>
>> Sent: 09 December 2019 13:55
>> 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 3/4] xen/interface: don't discard pending work in
>> FRONT/BACK_RING_ATTACH
>>
>> On 05.12.19 15:01, Paul Durrant wrote:
>>> Currently these macros will skip over any requests/responses that are
>>> added to the shared ring whilst it is detached. 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.
>>>
>>> NOTE: These macros are currently unused. BACK_RING_ATTACH(), however,
>> will
>>>         be used 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>
>>> ---
>>>    include/xen/interface/io/ring.h | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/xen/interface/io/ring.h
>> b/include/xen/interface/io/ring.h
>>> index 3f40501fc60b..405adfed87e6 100644
>>> --- a/include/xen/interface/io/ring.h
>>> +++ b/include/xen/interface/io/ring.h
>>> @@ -143,14 +143,14 @@ struct __name##_back_ring {
>> 		\
>>>    #define FRONT_RING_ATTACH(_r, _s, __size) do {				\
>>>        (_r)->sring = (_s);							\
>>>        (_r)->req_prod_pvt = (_s)->req_prod;				\
>>> -    (_r)->rsp_cons = (_s)->rsp_prod;					\
>>> +    (_r)->rsp_cons = (_s)->req_prod;					\
>>>        (_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)->req_cons = (_s)->rsp_prod;					\
>>>        (_r)->nr_ents = __RING_SIZE(_s, __size);				\
>>>    } while (0)
>>
>> Lets look at all possible scenarios where BACK_RING_ATTACH()
>> might happen:
>>
>> Initially (after [FRONT|BACK]_RING_INIT(), leaving _pvt away):
>> req_prod=0, rsp_cons=0, rsp_prod=0, req_cons=0
>> Using BACK_RING_ATTACH() is fine (no change)
>>
>> Request queued:
>> req_prod=1, rsp_cons=0, rsp_prod=0, req_cons=0
>> Using BACK_RING_ATTACH() is fine (no change)
>>
>> and taken by backend:
>> req_prod=1, rsp_cons=0, rsp_prod=0, req_cons=1
>> Using BACK_RING_ATTACH() is resetting req_cons to 0, will result
>> in redoing request (for blk this is fine, other devices like SCSI
>> tapes will have issues with that). One possible solution would be
>> to ensure all taken requests are either stopped or the response
>> is queued already.
> 
> Yes, it is the assumption that a backend will drain and complete any requests it is handling, but it will not deal with new ones being posted by the frontend. This does appear to be the case for blkback.
> 
>>
>> Response queued:
>> req_prod=1, rsp_cons=0, rsp_prod=1, req_cons=1
>> Using BACK_RING_ATTACH() is fine (no change)
>>
>> Response taken:
>> req_prod=1, rsp_cons=1, rsp_prod=1, req_cons=1
>> Using BACK_RING_ATTACH() is fine (no change)
>>
>> In general I believe the [FRONT|BACK]_RING_ATTACH() macros are not
>> fine to be used in the current state, as the *_pvt fields normally not
>> accessible by the other end are initialized using the (possibly
>> untrusted) values from the shared ring. There needs at least to be a
>> test for the values to be sane, and your change should not result in the
>> same value to be read twice, as it could have changed in between.
> 
> What test would you apply to sanitize the value of the pvt pointer?

For the BACK_RING_ATTACH() case rsp_prod_pvt should not be between
req_prod and req_cons, and req_cons - rsp_prod_pvt should be <= ring
size IMO.

> Another option would be to have a backend write its pvt value into the xenstore backend area when the ring is unmapped, so that a new instance definitely resumes where the old one left off. The value of rsp_prod could, of course, be overwritten by the guest at any time and so there's little point in attempting sanitize it.

I don't think this would be necessary. With above validation in place
all the guest could do would be to shoot itself in the foot.


Juergen

[PATCH 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 dd if=/dev/urandom of=test.img bs=1M count=1024; 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>
---
 drivers/block/xen-blkback/xenbus.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index e8c5c54e1d26..0b82740c4a9d 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -196,24 +196,24 @@ static int xen_blkif_map(struct xen_blkif_ring *ring, grant_ref_t *gref,
 	{
 		struct blkif_sring *sring;
 		sring = (struct blkif_sring *)ring->blk_ring;
-		BACK_RING_INIT(&ring->blk_rings.native, sring,
-			       XEN_PAGE_SIZE * nr_grefs);
+		BACK_RING_ATTACH(&ring->blk_rings.native, sring,
+				 XEN_PAGE_SIZE * nr_grefs);
 		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);
+		BACK_RING_ATTACH(&ring->blk_rings.x86_32, sring_x86_32,
+				 XEN_PAGE_SIZE * nr_grefs);
 		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);
+		BACK_RING_ATTACH(&ring->blk_rings.x86_64, sring_x86_64,
+				 XEN_PAGE_SIZE * nr_grefs);
 		break;
 	}
 	default:
-- 
2.20.1

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

[Roger Pau Monné]

On Thu, Dec 05, 2019 at 02:01:23PM +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 dd if=/dev/urandom of=test.img bs=1M count=1024; 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

So this does unbind blkback while leaving the PV interface as
connected?

Thanks, Roger.

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

[Durrant, Paul]

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 09 December 2019 12:17
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: linux-kernel@vger.kernel.org; xen-devel@lists.xenproject.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 4/4] xen-blkback: support dynamic unbind/bind
> 
> On Thu, Dec 05, 2019 at 02:01:23PM +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 dd if=/dev/urandom of=test.img bs=1M count=1024; 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
> 
> So this does unbind blkback while leaving the PV interface as
> connected?
> 

Yes, everything is left in place in the frontend. The backend detaches from the ring, closes its end of the event channels, etc. but the guest can still send requests which will get serviced when the new backend attaches.

  Paul

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

[Jürgen Groß]

On 05.12.19 15:01, 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 dd if=/dev/urandom of=test.img bs=1M count=1024; 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.

Could you do the same test with mixed reads/writes and verification of
the read/written data, please? A write-only test is not _that_
convincing regarding correctness. It only proves the guest is not
crashing.

I'm fine with the general approach, though.


Juergen

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

[Durrant, Paul]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 09 December 2019 13:58
> To: Durrant, Paul <pdurrant@amazon.com>; linux-kernel@vger.kernel.org;
> xen-devel@lists.xenproject.org
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Jens Axboe <axboe@kernel.dk>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 4/4] xen-blkback: support dynamic unbind/bind
> 
> On 05.12.19 15:01, 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 dd if=/dev/urandom of=test.img bs=1M count=1024; 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.
> 
> Could you do the same test with mixed reads/writes and verification of
> the read/written data, please? A write-only test is not _that_
> convincing regarding correctness. It only proves the guest is not
> crashing.

Sure. I'll find something that will verify content.

> 
> I'm fine with the general approach, though.
> 

Cool, thanks,

  Paul

> 
> Juergen