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

[Andrew Jones <drjones@xxxxxxxxxx>]

----- 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xensource.com/xen-devel