[PATCH] blkfront: don't change to closing if we're busy

[PATCH] blkfront: don't change to closing if we're busy

[Andrew Jones]

We just reported to xenbus that we can't close yet, because
blkfront is still in use. So we shouldn't then immediately
state that we are closing.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 drivers/block/xen-blkfront.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 5d45688..b53cae4 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1134,7 +1134,6 @@ blkfront_closing(struct blkfront_info *info)
 	if (bdev->bd_openers) {
 		xenbus_dev_error(xbdev, -EBUSY,
 				 "Device in use; refusing to close");
-		xenbus_switch_state(xbdev, XenbusStateClosing);
 	} else {
 		xlvbd_release_gendisk(info);
 		xenbus_frontend_closed(xbdev);
-- 
1.7.7.5

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Andrew Jones]

----- Original Message -----
> We just reported to xenbus that we can't close yet, because
> blkfront is still in use. So we shouldn't then immediately
> state that we are closing.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  drivers/block/xen-blkfront.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c
> b/drivers/block/xen-blkfront.c
> index 5d45688..b53cae4 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1134,7 +1134,6 @@ blkfront_closing(struct blkfront_info *info)
>  	if (bdev->bd_openers) {
>  		xenbus_dev_error(xbdev, -EBUSY,
>  				 "Device in use; refusing to close");
> -		xenbus_switch_state(xbdev, XenbusStateClosing);
>  	} else {
>  		xlvbd_release_gendisk(info);
>  		xenbus_frontend_closed(xbdev);
> --
> 1.7.7.5
> 

Hmm, I should maybe self-nack this. The bug that led me to writing
it is likely only with older tooling, such as RHEL5's. The problem
was if you attempted to detach a mounted disk twice, then the second
time it would succeed. The guest had flipped to Closing on the first
try, and thus didn't issue an error to xenbus on the second. I see now
in libxl__initiate_device_remove() that it bails out if state != 4,
so that tooling shouldn't have this issue.

The reason I only say maybe self-nack though, is because this state
change seemed to be thrown in with another fix[1]. I'm not sure if
the new behavior on legacy hosts was considered or not. If not, then
we can consider it now. Do we want to have deferred asynch detaches
over protecting the guest from multiple detach calls on legacy hosts?

Drew

[1] b70f5fa blkfront: Lock blkfront_info when closing

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Konrad Rzeszutek Wilk]

On Thu, Feb 16, 2012 at 01:17:09PM +0100, Andrew Jones wrote:
> We just reported to xenbus that we can't close yet, because
> blkfront is still in use. So we shouldn't then immediately
> state that we are closing.

What happens if the user uses --force to unplug the device?
Will that still work?
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  drivers/block/xen-blkfront.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 5d45688..b53cae4 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1134,7 +1134,6 @@ blkfront_closing(struct blkfront_info *info)
>  	if (bdev->bd_openers) {
>  		xenbus_dev_error(xbdev, -EBUSY,
>  				 "Device in use; refusing to close");
> -		xenbus_switch_state(xbdev, XenbusStateClosing);
>  	} else {
>  		xlvbd_release_gendisk(info);
>  		xenbus_frontend_closed(xbdev);
> -- 
> 1.7.7.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Andrew Jones]

----- Original Message -----
> On Thu, Feb 16, 2012 at 01:17:09PM +0100, Andrew Jones wrote:
> > We just reported to xenbus that we can't close yet, because
> > blkfront is still in use. So we shouldn't then immediately
> > state that we are closing.
> 
> What happens if the user uses --force to unplug the device?
> Will that still work?

With RHEL5 tooling I get the same results. That is, the device
is forcibly detached, and then any task in the guest that still
attempts to use (or even just unmount) the device hangs.

I don't know anything about xl, but afaict block-detach
doesn't take a 'force' option?

Drew

> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  drivers/block/xen-blkfront.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkfront.c
> > b/drivers/block/xen-blkfront.c
> > index 5d45688..b53cae4 100644
> > --- a/drivers/block/xen-blkfront.c
> > +++ b/drivers/block/xen-blkfront.c
> > @@ -1134,7 +1134,6 @@ blkfront_closing(struct blkfront_info *info)
> >  	if (bdev->bd_openers) {
> >  		xenbus_dev_error(xbdev, -EBUSY,
> >  				 "Device in use; refusing to close");
> > -		xenbus_switch_state(xbdev, XenbusStateClosing);
> >  	} else {
> >  		xlvbd_release_gendisk(info);
> >  		xenbus_frontend_closed(xbdev);
> > --
> > 1.7.7.5
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Konrad Rzeszutek Wilk]

On Fri, Feb 17, 2012 at 07:50:11AM -0500, Andrew Jones wrote:
> 
> 
> ----- Original Message -----
> > On Thu, Feb 16, 2012 at 01:17:09PM +0100, Andrew Jones wrote:
> > > We just reported to xenbus that we can't close yet, because
> > > blkfront is still in use. So we shouldn't then immediately
> > > state that we are closing.
> > 
> > What happens if the user uses --force to unplug the device?
> > Will that still work?
> 
> With RHEL5 tooling I get the same results. That is, the device
> is forcibly detached, and then any task in the guest that still
> attempts to use (or even just unmount) the device hangs.
> 
> I don't know anything about xl, but afaict block-detach
> doesn't take a 'force' option?

konrad@phenom:~/ssd/linux$ xm block-detach
xend [ERROR] Config file does not exist: /etc/xen/xend-config.sxp
Error: 'xm block-detach' requires between 2 and 3 arguments.

Usage: xm block-detach <Domain> <DevId> [-f|--force]

Destroy a domain's virtual block device.


> 
> Drew
> 
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  drivers/block/xen-blkfront.c |    1 -
> > >  1 files changed, 0 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/block/xen-blkfront.c
> > > b/drivers/block/xen-blkfront.c
> > > index 5d45688..b53cae4 100644
> > > --- a/drivers/block/xen-blkfront.c
> > > +++ b/drivers/block/xen-blkfront.c
> > > @@ -1134,7 +1134,6 @@ blkfront_closing(struct blkfront_info *info)
> > >  	if (bdev->bd_openers) {
> > >  		xenbus_dev_error(xbdev, -EBUSY,
> > >  				 "Device in use; refusing to close");
> > > -		xenbus_switch_state(xbdev, XenbusStateClosing);
> > >  	} else {
> > >  		xlvbd_release_gendisk(info);
> > >  		xenbus_frontend_closed(xbdev);
> > > --
> > > 1.7.7.5
> > > 
> > > 
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xensource.com
> > > http://lists.xensource.com/xen-devel
> > 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Andrew Jones]

----- Original Message -----
> On Fri, Feb 17, 2012 at 07:50:11AM -0500, Andrew Jones wrote:
> > 
> > 
> > ----- Original Message -----
> > > On Thu, Feb 16, 2012 at 01:17:09PM +0100, Andrew Jones wrote:
> > > > We just reported to xenbus that we can't close yet, because
> > > > blkfront is still in use. So we shouldn't then immediately
> > > > state that we are closing.
> > > 
> > > What happens if the user uses --force to unplug the device?
> > > Will that still work?
> > 
> > With RHEL5 tooling I get the same results. That is, the device
> > is forcibly detached, and then any task in the guest that still
> > attempts to use (or even just unmount) the device hangs.
> > 
> > I don't know anything about xl, but afaict block-detach
> > doesn't take a 'force' option?
> 
> konrad@phenom:~/ssd/linux$ xm block-detach
> xend [ERROR] Config file does not exist: /etc/xen/xend-config.sxp
> Error: 'xm block-detach' requires between 2 and 3 arguments.
> 
> Usage: xm block-detach <Domain> <DevId> [-f|--force]
> 
> Destroy a domain's virtual block device.

That's 'xm', not 'xl'. I tested RHEL5's 'xm' with the force option
and, as I said above, I got the same results before and after this
patch. I believe the problem (the non-force case) may exist with
latest 'xm' too (not just RHEL5's), but I didn't really check, since
I know 'xl' is the tool that truly matters for upstream now. As I
said before, the problem shouldn't exist with 'xl' though, since it
bails on state != 4, and afaict it doesn't have a force option.
Anyway, I don't think this patch would break that path even if it
did.

I'm actually going to send a v2 of this patch in a few minutes. I'm
just finishing it up now.

Drew

> 
> 
> > 
> > Drew
> > 
> > > > 
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > >  drivers/block/xen-blkfront.c |    1 -
> > > >  1 files changed, 0 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/block/xen-blkfront.c
> > > > b/drivers/block/xen-blkfront.c
> > > > index 5d45688..b53cae4 100644
> > > > --- a/drivers/block/xen-blkfront.c
> > > > +++ b/drivers/block/xen-blkfront.c
> > > > @@ -1134,7 +1134,6 @@ blkfront_closing(struct blkfront_info
> > > > *info)
> > > >  	if (bdev->bd_openers) {
> > > >  		xenbus_dev_error(xbdev, -EBUSY,
> > > >  				 "Device in use; refusing to close");
> > > > -		xenbus_switch_state(xbdev, XenbusStateClosing);
> > > >  	} else {
> > > >  		xlvbd_release_gendisk(info);
> > > >  		xenbus_frontend_closed(xbdev);
> > > > --
> > > > 1.7.7.5
> > > > 
> > > > 
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@lists.xensource.com
> > > > http://lists.xensource.com/xen-devel
> > > 
> 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Andrew Jones]

On Thu, Feb 16, 2012 at 12:33:32PM -0500, Andrew Jones wrote:
> Hmm, I should maybe self-nack this. The bug that led me to writing
> it is likely only with older tooling, such as RHEL5's. The problem
> was if you attempted to detach a mounted disk twice, then the second
> time it would succeed. The guest had flipped to Closing on the first
> try, and thus didn't issue an error to xenbus on the second. I see

Actually, it's even worse than I thought. Just a single attempt to
detach the device will cause the guest grief (with RHEL5's tools
anyway). Once xenbus shows the device is in the Closing state, then
tasks accessing the device may hang.

> The reason I only say maybe self-nack though, is because this state
> change seemed to be thrown in with another fix[1]. I'm not sure if
> the new behavior on legacy hosts was considered or not. If not, then
> we can consider it now. Do we want to have deferred asynch detaches
> over protecting the guest from multiple detach calls on legacy hosts?
> 

I guess we can keep the feature and protect the guest with a patch like
I'll send in a moment. It doesn't really work for me with a RHEL5 host
though. The deferred close works from the pov of the guest, but the
state of the block device gets left in Closed after the guest unmounts
it, and then RHEL5's tools can't detach/reattach it. If the deferred
close ever worked on other Xen hosts though, then I don't believe this
patch would break it, and it will also keep the guest safe when run on
hosts that don't treat state=Closing the way it currently expects.

Drew

[PATCH v2] blkfront: don't change to closing if we're busy

[Andrew Jones]

We just reported to xenbus that we can't close yet, because we're still
in use. So we shouldn't then immediately tell xenbus that we are
closing. If we do, then we risk bypassing the chance to complain again,
if we're told to close again, and we're still in use. The reason this
code was here was to implement a deferred close. We can still support
this feature by adding a member to our private data, and setting that
instead. Besides being able to complain each time, this patch fixes
a problem when running on hosts with legacy tools that may interpret
the lack of a complaint or state=Closing incorrectly, and then yank
the device out from under the guest.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 drivers/block/xen-blkfront.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 2f22874..d7f2e03 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -103,6 +103,7 @@ struct blkfront_info
 	unsigned int discard_granularity;
 	unsigned int discard_alignment;
 	int is_ready;
+	int closing;
 };
 
 static DEFINE_SPINLOCK(blkif_io_lock);
@@ -1134,7 +1135,7 @@ blkfront_closing(struct blkfront_info *info)
 	if (bdev->bd_openers) {
 		xenbus_dev_error(xbdev, -EBUSY,
 				 "Device in use; refusing to close");
-		xenbus_switch_state(xbdev, XenbusStateClosing);
+		info->closing = 1;
 	} else {
 		xlvbd_release_gendisk(info);
 		xenbus_frontend_closed(xbdev);
@@ -1423,8 +1424,10 @@ static int blkif_release(struct gendisk *disk, fmode_t mode)
 	mutex_lock(&info->mutex);
 	xbdev = info->xbdev;
 
-	if (xbdev && xbdev->state == XenbusStateClosing) {
+	if (xbdev && (xbdev->state == XenbusStateClosing || info->closing)) {
 		/* pending switch to state closed */
+		if (xbdev->state != XenbusStateClosing)
+			xenbus_switch_state(xbdev, XenbusStateClosing);
 		dev_info(disk_to_dev(bdev->bd_disk), "releasing disk\n");
 		xlvbd_release_gendisk(info);
 		xenbus_frontend_closed(info->xbdev);
-- 
1.7.7.5

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Konrad Rzeszutek Wilk]

On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
> On Thu, Feb 16, 2012 at 12:33:32PM -0500, Andrew Jones wrote:
> > Hmm, I should maybe self-nack this. The bug that led me to writing
> > it is likely only with older tooling, such as RHEL5's. The problem
> > was if you attempted to detach a mounted disk twice, then the second
> > time it would succeed. The guest had flipped to Closing on the first
> > try, and thus didn't issue an error to xenbus on the second. I see
> 
> Actually, it's even worse than I thought. Just a single attempt to
> detach the device will cause the guest grief (with RHEL5's tools
> anyway). Once xenbus shows the device is in the Closing state, then
> tasks accessing the device may hang.
> 
> > The reason I only say maybe self-nack though, is because this state
> > change seemed to be thrown in with another fix[1]. I'm not sure if
> > the new behavior on legacy hosts was considered or not. If not, then
> > we can consider it now. Do we want to have deferred asynch detaches
> > over protecting the guest from multiple detach calls on legacy hosts?
> > 
> 
> I guess we can keep the feature and protect the guest with a patch like
> I'll send in a moment. It doesn't really work for me with a RHEL5 host
> though. The deferred close works from the pov of the guest, but the
> state of the block device gets left in Closed after the guest unmounts
> it, and then RHEL5's tools can't detach/reattach it. If the deferred
> close ever worked on other Xen hosts though, then I don't believe this
> patch would break it, and it will also keep the guest safe when run on
> hosts that don't treat state=Closing the way it currently expects.

There was another fix that sounds similar to this in the backend.
6f5986bce558e64fe867bff600a2127a3cb0c006

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Andrew Jones]

----- Original Message -----
> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
> > On Thu, Feb 16, 2012 at 12:33:32PM -0500, Andrew Jones wrote:
> > > Hmm, I should maybe self-nack this. The bug that led me to
> > > writing
> > > it is likely only with older tooling, such as RHEL5's. The
> > > problem
> > > was if you attempted to detach a mounted disk twice, then the
> > > second
> > > time it would succeed. The guest had flipped to Closing on the
> > > first
> > > try, and thus didn't issue an error to xenbus on the second. I
> > > see
> > 
> > Actually, it's even worse than I thought. Just a single attempt to
> > detach the device will cause the guest grief (with RHEL5's tools
> > anyway). Once xenbus shows the device is in the Closing state, then
> > tasks accessing the device may hang.
> > 
> > > The reason I only say maybe self-nack though, is because this
> > > state
> > > change seemed to be thrown in with another fix[1]. I'm not sure
> > > if
> > > the new behavior on legacy hosts was considered or not. If not,
> > > then
> > > we can consider it now. Do we want to have deferred asynch
> > > detaches
> > > over protecting the guest from multiple detach calls on legacy
> > > hosts?
> > > 
> > 
> > I guess we can keep the feature and protect the guest with a patch
> > like
> > I'll send in a moment. It doesn't really work for me with a RHEL5
> > host
> > though. The deferred close works from the pov of the guest, but the
> > state of the block device gets left in Closed after the guest
> > unmounts
> > it, and then RHEL5's tools can't detach/reattach it. If the
> > deferred
> > close ever worked on other Xen hosts though, then I don't believe
> > this
> > patch would break it, and it will also keep the guest safe when run
> > on
> > hosts that don't treat state=Closing the way it currently expects.
> 
> There was another fix that sounds similar to this in the backend.
> 6f5986bce558e64fe867bff600a2127a3cb0c006
> 

Thanks for the pointer. It doesn't look like the upstream 2.6.18
tree has that, but it probably would be a good idea there too.
However, even with that ability to patch backends, we probably
want the frontends to be more robust on legacy backends for a while
longer. So, it probably would be best to avoid changing the state to
Closing while we're still busy, unless it's absolutely necessary.

Drew

Re: [Xen-devel] [PATCH v2] blkfront: don't change to closing if we're busy

[Andrew Jones]

----- Original Message -----
> We just reported to xenbus that we can't close yet, because we're
> still
> in use. So we shouldn't then immediately tell xenbus that we are
> closing. If we do, then we risk bypassing the chance to complain
> again,
> if we're told to close again, and we're still in use. The reason this
> code was here was to implement a deferred close. We can still support
> this feature by adding a member to our private data, and setting that
> instead. Besides being able to complain each time, this patch fixes
> a problem when running on hosts with legacy tools that may interpret
> the lack of a complaint or state=Closing incorrectly, and then yank
> the device out from under the guest.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  drivers/block/xen-blkfront.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c
> b/drivers/block/xen-blkfront.c
> index 2f22874..d7f2e03 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -103,6 +103,7 @@ struct blkfront_info
>  	unsigned int discard_granularity;
>  	unsigned int discard_alignment;
>  	int is_ready;
> +	int closing;
>  };
>  
>  static DEFINE_SPINLOCK(blkif_io_lock);
> @@ -1134,7 +1135,7 @@ blkfront_closing(struct blkfront_info *info)
>  	if (bdev->bd_openers) {
>  		xenbus_dev_error(xbdev, -EBUSY,
>  				 "Device in use; refusing to close");
> -		xenbus_switch_state(xbdev, XenbusStateClosing);
> +		info->closing = 1;
>  	} else {
>  		xlvbd_release_gendisk(info);
>  		xenbus_frontend_closed(xbdev);
> @@ -1423,8 +1424,10 @@ static int blkif_release(struct gendisk *disk,
> fmode_t mode)
>  	mutex_lock(&info->mutex);
>  	xbdev = info->xbdev;
>  
> -	if (xbdev && xbdev->state == XenbusStateClosing) {
> +	if (xbdev && (xbdev->state == XenbusStateClosing || info->closing))

Ugh. This isn't right either. After discussing with Igor Mammedov, I
see that I missed that xbdev->state will now never be XenbusStateClosing.
We need the xenbus_read_driver_state() to come back for getting the
state (that was also changed with 7fd152f4b6ae).

However, I'm starting to wonder if supporting "deferred detach" is
worth the complexity. If we took a pass through xen-blkfront.c and
removed all code related to it, and perhaps also all code related to a
"forced detach", then I suspect the driver could be nicely simplified.

Do we need deferred and forced detach support? Forced seems like a bad
idea. For what would a guest use a disk that could disappear with little
notice? Deferred also seems strange. It seems like the host and guest
admins should generally better coordinate and synchronize these types
of activities. It's possible there are good usecases that I'm missing.
I'd be happy to hear examples.

Thanks,
Drew

> {
>  		/* pending switch to state closed */
> +		if (xbdev->state != XenbusStateClosing)
> +			xenbus_switch_state(xbdev, XenbusStateClosing);
>  		dev_info(disk_to_dev(bdev->bd_disk), "releasing disk\n");
>  		xlvbd_release_gendisk(info);
>  		xenbus_frontend_closed(info->xbdev);
> --
> 1.7.7.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Jan Beulich]

>>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:

> 
> ----- Original Message -----
>> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
>> > On Thu, Feb 16, 2012 at 12:33:32PM -0500, Andrew Jones wrote:
>> > > Hmm, I should maybe self-nack this. The bug that led me to
>> > > writing
>> > > it is likely only with older tooling, such as RHEL5's. The
>> > > problem
>> > > was if you attempted to detach a mounted disk twice, then the
>> > > second
>> > > time it would succeed. The guest had flipped to Closing on the
>> > > first
>> > > try, and thus didn't issue an error to xenbus on the second. I
>> > > see
>> > 
>> > Actually, it's even worse than I thought. Just a single attempt to
>> > detach the device will cause the guest grief (with RHEL5's tools
>> > anyway). Once xenbus shows the device is in the Closing state, then
>> > tasks accessing the device may hang.
>> > 
>> > > The reason I only say maybe self-nack though, is because this
>> > > state
>> > > change seemed to be thrown in with another fix[1]. I'm not sure
>> > > if
>> > > the new behavior on legacy hosts was considered or not. If not,
>> > > then
>> > > we can consider it now. Do we want to have deferred asynch
>> > > detaches
>> > > over protecting the guest from multiple detach calls on legacy
>> > > hosts?
>> > > 
>> > 
>> > I guess we can keep the feature and protect the guest with a patch
>> > like
>> > I'll send in a moment. It doesn't really work for me with a RHEL5
>> > host
>> > though. The deferred close works from the pov of the guest, but the
>> > state of the block device gets left in Closed after the guest
>> > unmounts
>> > it, and then RHEL5's tools can't detach/reattach it. If the
>> > deferred
>> > close ever worked on other Xen hosts though, then I don't believe
>> > this
>> > patch would break it, and it will also keep the guest safe when run
>> > on
>> > hosts that don't treat state=Closing the way it currently expects.
>> 
>> There was another fix that sounds similar to this in the backend.
>> 6f5986bce558e64fe867bff600a2127a3cb0c006
>> 
> 
> Thanks for the pointer. It doesn't look like the upstream 2.6.18
> tree has that, but it probably would be a good idea there too.

While I had seen the change and considered pulling it in, I wasn't
really convinced this is the right behavior here: After all, if the host
admin requested a resource to be removed from a guest, it shouldn't
depend on the guest whether and when to honor that request, yet
by deferring the disconnect you basically allow the guest to continue
using the disk indefinitely.

Jan

> However, even with that ability to patch backends, we probably
> want the frontends to be more robust on legacy backends for a while
> longer. So, it probably would be best to avoid changing the state to
> Closing while we're still busy, unless it's absolutely necessary.
> 
> Drew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Andrew Jones]

----- Original Message -----
> >>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:
> 
> > 
> > ----- Original Message -----
> >> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
> >> > On Thu, Feb 16, 2012 at 12:33:32PM -0500, Andrew Jones wrote:
> >> > > Hmm, I should maybe self-nack this. The bug that led me to
> >> > > writing
> >> > > it is likely only with older tooling, such as RHEL5's. The
> >> > > problem
> >> > > was if you attempted to detach a mounted disk twice, then the
> >> > > second
> >> > > time it would succeed. The guest had flipped to Closing on the
> >> > > first
> >> > > try, and thus didn't issue an error to xenbus on the second. I
> >> > > see
> >> > 
> >> > Actually, it's even worse than I thought. Just a single attempt
> >> > to
> >> > detach the device will cause the guest grief (with RHEL5's tools
> >> > anyway). Once xenbus shows the device is in the Closing state,
> >> > then
> >> > tasks accessing the device may hang.
> >> > 
> >> > > The reason I only say maybe self-nack though, is because this
> >> > > state
> >> > > change seemed to be thrown in with another fix[1]. I'm not
> >> > > sure
> >> > > if
> >> > > the new behavior on legacy hosts was considered or not. If
> >> > > not,
> >> > > then
> >> > > we can consider it now. Do we want to have deferred asynch
> >> > > detaches
> >> > > over protecting the guest from multiple detach calls on legacy
> >> > > hosts?
> >> > > 
> >> > 
> >> > I guess we can keep the feature and protect the guest with a
> >> > patch
> >> > like
> >> > I'll send in a moment. It doesn't really work for me with a
> >> > RHEL5
> >> > host
> >> > though. The deferred close works from the pov of the guest, but
> >> > the
> >> > state of the block device gets left in Closed after the guest
> >> > unmounts
> >> > it, and then RHEL5's tools can't detach/reattach it. If the
> >> > deferred
> >> > close ever worked on other Xen hosts though, then I don't
> >> > believe
> >> > this
> >> > patch would break it, and it will also keep the guest safe when
> >> > run
> >> > on
> >> > hosts that don't treat state=Closing the way it currently
> >> > expects.
> >> 
> >> There was another fix that sounds similar to this in the backend.
> >> 6f5986bce558e64fe867bff600a2127a3cb0c006
> >> 
> > 
> > Thanks for the pointer. It doesn't look like the upstream 2.6.18
> > tree has that, but it probably would be a good idea there too.
> 
> While I had seen the change and considered pulling it in, I wasn't
> really convinced this is the right behavior here: After all, if the
> host
> admin requested a resource to be removed from a guest, it shouldn't
> depend on the guest whether and when to honor that request, yet
> by deferring the disconnect you basically allow the guest to continue
> using the disk indefinitely.
> 

I agree. Yesterday I wrote[1] asking if "deferred detach" is really
something we want. At the moment, Igor and I are poking through
xen-blkfront.c, and currently we'd rather see the feature dropped
in favor of a simplified driver. One that has less release paths,
and/or release paths with more consistent locking behavior.

Drew

[1] http://lists.xen.org/archives/html/xen-devel/2012-02/msg01672.html


> Jan
> 
> > However, even with that ability to patch backends, we probably
> > want the frontends to be more robust on legacy backends for a while
> > longer. So, it probably would be best to avoid changing the state
> > to
> > Closing while we're still busy, unless it's absolutely necessary.
> > 
> > Drew
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xensource.com/xen-devel
> 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Jan Beulich]

>>> On 21.02.12 at 10:23, Andrew Jones <drjones@redhat.com> wrote:
>> >>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:
>> >> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
>> >> There was another fix that sounds similar to this in the backend.
>> >> 6f5986bce558e64fe867bff600a2127a3cb0c006
>> >> 
>> > 
>> > Thanks for the pointer. It doesn't look like the upstream 2.6.18
>> > tree has that, but it probably would be a good idea there too.
>> 
>> While I had seen the change and considered pulling it in, I wasn't
>> really convinced this is the right behavior here: After all, if the
>> host
>> admin requested a resource to be removed from a guest, it shouldn't
>> depend on the guest whether and when to honor that request, yet
>> by deferring the disconnect you basically allow the guest to continue
>> using the disk indefinitely.
>> 
> 
> I agree. Yesterday I wrote[1] asking if "deferred detach" is really
> something we want. At the moment, Igor and I are poking through
> xen-blkfront.c, and currently we'd rather see the feature dropped
> in favor of a simplified driver. One that has less release paths,
> and/or release paths with more consistent locking behavior.

I must have missed this, or it's one more instance of delayed mail
delivery via xen-devel.

Konrad - care to revert that original change as having barked up
the wrong tree?

Jan

> [1] http://lists.xen.org/archives/html/xen-devel/2012-02/msg01672.html 

Re: [PATCH] blkfront: don't change to closing if we're busy

[Jan Beulich]

>>> On 21.02.12 at 10:23, Andrew Jones <drjones@redhat.com> wrote:
>> >>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:
>> >> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
>> >> There was another fix that sounds similar to this in the backend.
>> >> 6f5986bce558e64fe867bff600a2127a3cb0c006
>> >> 
>> > 
>> > Thanks for the pointer. It doesn't look like the upstream 2.6.18
>> > tree has that, but it probably would be a good idea there too.
>> 
>> While I had seen the change and considered pulling it in, I wasn't
>> really convinced this is the right behavior here: After all, if the
>> host
>> admin requested a resource to be removed from a guest, it shouldn't
>> depend on the guest whether and when to honor that request, yet
>> by deferring the disconnect you basically allow the guest to continue
>> using the disk indefinitely.
>> 
> 
> I agree. Yesterday I wrote[1] asking if "deferred detach" is really
> something we want. At the moment, Igor and I are poking through
> xen-blkfront.c, and currently we'd rather see the feature dropped
> in favor of a simplified driver. One that has less release paths,
> and/or release paths with more consistent locking behavior.

I must have missed this, or it's one more instance of delayed mail
delivery via xen-devel.

Konrad - care to revert that original change as having barked up
the wrong tree?

Jan

> [1] http://lists.xen.org/archives/html/xen-devel/2012-02/msg01672.html 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Konrad Rzeszutek Wilk]

On Tue, Feb 21, 2012 at 09:38:41AM +0000, Jan Beulich wrote:
> >>> On 21.02.12 at 10:23, Andrew Jones <drjones@redhat.com> wrote:
> >> >>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:
> >> >> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
> >> >> There was another fix that sounds similar to this in the backend.
> >> >> 6f5986bce558e64fe867bff600a2127a3cb0c006
> >> >> 
> >> > 
> >> > Thanks for the pointer. It doesn't look like the upstream 2.6.18
> >> > tree has that, but it probably would be a good idea there too.
> >> 
> >> While I had seen the change and considered pulling it in, I wasn't
> >> really convinced this is the right behavior here: After all, if the
> >> host
> >> admin requested a resource to be removed from a guest, it shouldn't
> >> depend on the guest whether and when to honor that request, yet
> >> by deferring the disconnect you basically allow the guest to continue
> >> using the disk indefinitely.
> >> 
> > 
> > I agree. Yesterday I wrote[1] asking if "deferred detach" is really
> > something we want. At the moment, Igor and I are poking through
> > xen-blkfront.c, and currently we'd rather see the feature dropped
> > in favor of a simplified driver. One that has less release paths,
> > and/or release paths with more consistent locking behavior.
> 
> I must have missed this, or it's one more instance of delayed mail
> delivery via xen-devel.
> 
> Konrad - care to revert that original change as having barked up
> the wrong tree?

Meaning the 6f5986bce558e64fe867bff600a2127a3cb0c006?

Lets CC Joe Jin here to get his input. I recall that the --force argument
still works with that patch so the admin can still choose to terminate the
state. Which I thought was the point of the --force - as in if the
guest is still using it, we won't be yanking it out until we are
completly sure.


> 
> Jan
> 
> > [1] http://lists.xen.org/archives/html/xen-devel/2012-02/msg01672.html 
> 

Re: [PATCH] blkfront: don't change to closing if we're busy

[Konrad Rzeszutek Wilk]

On Tue, Feb 21, 2012 at 09:38:41AM +0000, Jan Beulich wrote:
> >>> On 21.02.12 at 10:23, Andrew Jones <drjones@redhat.com> wrote:
> >> >>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:
> >> >> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
> >> >> There was another fix that sounds similar to this in the backend.
> >> >> 6f5986bce558e64fe867bff600a2127a3cb0c006
> >> >> 
> >> > 
> >> > Thanks for the pointer. It doesn't look like the upstream 2.6.18
> >> > tree has that, but it probably would be a good idea there too.
> >> 
> >> While I had seen the change and considered pulling it in, I wasn't
> >> really convinced this is the right behavior here: After all, if the
> >> host
> >> admin requested a resource to be removed from a guest, it shouldn't
> >> depend on the guest whether and when to honor that request, yet
> >> by deferring the disconnect you basically allow the guest to continue
> >> using the disk indefinitely.
> >> 
> > 
> > I agree. Yesterday I wrote[1] asking if "deferred detach" is really
> > something we want. At the moment, Igor and I are poking through
> > xen-blkfront.c, and currently we'd rather see the feature dropped
> > in favor of a simplified driver. One that has less release paths,
> > and/or release paths with more consistent locking behavior.
> 
> I must have missed this, or it's one more instance of delayed mail
> delivery via xen-devel.
> 
> Konrad - care to revert that original change as having barked up
> the wrong tree?

Meaning the 6f5986bce558e64fe867bff600a2127a3cb0c006?

Lets CC Joe Jin here to get his input. I recall that the --force argument
still works with that patch so the admin can still choose to terminate the
state. Which I thought was the point of the --force - as in if the
guest is still using it, we won't be yanking it out until we are
completly sure.


> 
> Jan
> 
> > [1] http://lists.xen.org/archives/html/xen-devel/2012-02/msg01672.html 
> 

Re: [Xen-devel] [PATCH] blkfront: don't change to closing if we're busy

[Jan Beulich]

>>> On 21.02.12 at 15:36, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, Feb 21, 2012 at 09:38:41AM +0000, Jan Beulich wrote:
>> >>> On 21.02.12 at 10:23, Andrew Jones <drjones@redhat.com> wrote:
>> >> >>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:
>> >> >> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
>> >> >> There was another fix that sounds similar to this in the backend.
>> >> >> 6f5986bce558e64fe867bff600a2127a3cb0c006
>> >> >> 
>> >> > 
>> >> > Thanks for the pointer. It doesn't look like the upstream 2.6.18
>> >> > tree has that, but it probably would be a good idea there too.
>> >> 
>> >> While I had seen the change and considered pulling it in, I wasn't
>> >> really convinced this is the right behavior here: After all, if the
>> >> host
>> >> admin requested a resource to be removed from a guest, it shouldn't
>> >> depend on the guest whether and when to honor that request, yet
>> >> by deferring the disconnect you basically allow the guest to continue
>> >> using the disk indefinitely.
>> >> 
>> > 
>> > I agree. Yesterday I wrote[1] asking if "deferred detach" is really
>> > something we want. At the moment, Igor and I are poking through
>> > xen-blkfront.c, and currently we'd rather see the feature dropped
>> > in favor of a simplified driver. One that has less release paths,
>> > and/or release paths with more consistent locking behavior.
>> 
>> I must have missed this, or it's one more instance of delayed mail
>> delivery via xen-devel.
>> 
>> Konrad - care to revert that original change as having barked up
>> the wrong tree?
> 
> Meaning the 6f5986bce558e64fe867bff600a2127a3cb0c006?
> 
> Lets CC Joe Jin here to get his input. I recall that the --force argument
> still works with that patch so the admin can still choose to terminate the
> state. Which I thought was the point of the --force - as in if the
> guest is still using it, we won't be yanking it out until we are
> completly sure.

Actually I meanwhile think that rather than fully reverting the
change, xen_blkif_disconnect() should be called in both the
XenbusStateClosing and XenbusStateClosed cases, not the least
since the frontend only ever sets the state to Closing when there
are still active users. Not doing the disconnect can e.g. result in
grant reference (and page) leaks e.g. when the frontend driver
gets unloaded while there are still existing (but obviously unused)
devices, and those can't even be eliminated by adding leak
handling code to gnttab_end_foreign_access() (the freeing then
gets deferred until a frontend driver gets loaded again and re-
attaches to the device - currently impossible in the upstream
driver as xlblk_exit() fails to call unregister_blkdev(); see
http://xenbits.xen.org/hg/linux-2.6.18-xen.hg/rev/99dc6737898b).

Jan

Re: [PATCH] blkfront: don't change to closing if we're busy

[Jan Beulich]

>>> On 21.02.12 at 15:36, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, Feb 21, 2012 at 09:38:41AM +0000, Jan Beulich wrote:
>> >>> On 21.02.12 at 10:23, Andrew Jones <drjones@redhat.com> wrote:
>> >> >>> On 20.02.12 at 11:35, Andrew Jones <drjones@redhat.com> wrote:
>> >> >> On Fri, Feb 17, 2012 at 05:52:54PM +0100, Andrew Jones wrote:
>> >> >> There was another fix that sounds similar to this in the backend.
>> >> >> 6f5986bce558e64fe867bff600a2127a3cb0c006
>> >> >> 
>> >> > 
>> >> > Thanks for the pointer. It doesn't look like the upstream 2.6.18
>> >> > tree has that, but it probably would be a good idea there too.
>> >> 
>> >> While I had seen the change and considered pulling it in, I wasn't
>> >> really convinced this is the right behavior here: After all, if the
>> >> host
>> >> admin requested a resource to be removed from a guest, it shouldn't
>> >> depend on the guest whether and when to honor that request, yet
>> >> by deferring the disconnect you basically allow the guest to continue
>> >> using the disk indefinitely.
>> >> 
>> > 
>> > I agree. Yesterday I wrote[1] asking if "deferred detach" is really
>> > something we want. At the moment, Igor and I are poking through
>> > xen-blkfront.c, and currently we'd rather see the feature dropped
>> > in favor of a simplified driver. One that has less release paths,
>> > and/or release paths with more consistent locking behavior.
>> 
>> I must have missed this, or it's one more instance of delayed mail
>> delivery via xen-devel.
>> 
>> Konrad - care to revert that original change as having barked up
>> the wrong tree?
> 
> Meaning the 6f5986bce558e64fe867bff600a2127a3cb0c006?
> 
> Lets CC Joe Jin here to get his input. I recall that the --force argument
> still works with that patch so the admin can still choose to terminate the
> state. Which I thought was the point of the --force - as in if the
> guest is still using it, we won't be yanking it out until we are
> completly sure.

Actually I meanwhile think that rather than fully reverting the
change, xen_blkif_disconnect() should be called in both the
XenbusStateClosing and XenbusStateClosed cases, not the least
since the frontend only ever sets the state to Closing when there
are still active users. Not doing the disconnect can e.g. result in
grant reference (and page) leaks e.g. when the frontend driver
gets unloaded while there are still existing (but obviously unused)
devices, and those can't even be eliminated by adding leak
handling code to gnttab_end_foreign_access() (the freeing then
gets deferred until a frontend driver gets loaded again and re-
attaches to the device - currently impossible in the upstream
driver as xlblk_exit() fails to call unregister_blkdev(); see
http://xenbits.xen.org/hg/linux-2.6.18-xen.hg/rev/99dc6737898b).

Jan