Patchwork [1/2] Implement drive_del to decouple block removal from device removal

login
register
mail settings
Submitter Ryan Harper
Date Nov. 12, 2010, 3:38 p.m.
Message ID <1289576289-27888-2-git-send-email-ryanh@us.ibm.com>
Download mbox | patch
Permalink /patch/70978/
State New
Headers show

Comments

Ryan Harper - Nov. 12, 2010, 3:38 p.m.
Currently device hotplug removal code is tied to device removal via
ACPI.  All pci devices that are removable via device_del() require the
guest to respond to the request.  In some cases the guest may not
respond leaving the device still accessible to the guest.  The management
layer doesn't currently have a reliable way to revoke access to host
resource in the presence of an uncooperative guest.

This patch implements a new monitor command, drive_del, which
provides an explicit command to revoke access to a host block device.

drive_del first quiesces the block device (qemu_aio_flush;
bdrv_flush() and bdrv_close()).  This prevents further IO from being
submitted against the host device.  Finally, drive_del cleans up
pointers between the drive object (host resource) and the device
object (guest resource).

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
 blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
 blockdev.h      |    1 +
 hmp-commands.hx |   18 ++++++++++++++++++
 3 files changed, 58 insertions(+), 0 deletions(-)
Markus Armbruster - Nov. 12, 2010, 4:27 p.m.
Ryan Harper <ryanh@us.ibm.com> writes:

> Currently device hotplug removal code is tied to device removal via
> ACPI.  All pci devices that are removable via device_del() require the
> guest to respond to the request.  In some cases the guest may not
> respond leaving the device still accessible to the guest.  The management
> layer doesn't currently have a reliable way to revoke access to host
> resource in the presence of an uncooperative guest.
>
> This patch implements a new monitor command, drive_del, which
> provides an explicit command to revoke access to a host block device.
>
> drive_del first quiesces the block device (qemu_aio_flush;
> bdrv_flush() and bdrv_close()).  This prevents further IO from being
> submitted against the host device.  Finally, drive_del cleans up
> pointers between the drive object (host resource) and the device
> object (guest resource).
>
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> ---
>  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
>  blockdev.h      |    1 +
>  hmp-commands.hx |   18 ++++++++++++++++++
>  3 files changed, 58 insertions(+), 0 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 6cb179a..f6ac439 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -14,6 +14,8 @@
>  #include "qemu-option.h"
>  #include "qemu-config.h"
>  #include "sysemu.h"
> +#include "hw/qdev.h"
> +#include "block_int.h"
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>  
> @@ -597,3 +599,40 @@ int do_change_block(Monitor *mon, const char *device,
>      }
>      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>  }
> +
> +int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *id = qdict_get_str(qdict, "id");
> +    BlockDriverState *bs;
> +    BlockDriverState **ptr;
> +    Property *prop;
> +
> +    bs = bdrv_find(id);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> +        return -1;
> +    }
> +
> +    /* quiesce block driver; prevent further io */
> +    qemu_aio_flush();
> +    bdrv_flush(bs);
> +    bdrv_close(bs);
> +
> +    /* clean up guest state from pointing to host resource by
> +     * finding and removing DeviceState "drive" property */
> +    for (prop = bs->peer->info->props; prop && prop->name; prop++) {
> +        if (prop->info->type == PROP_TYPE_DRIVE) {
> +            ptr = qdev_get_prop_ptr(bs->peer, prop);
> +            if ((*ptr) == bs) {

Superfluous parenthesis around *ptr.  Not worth a respin; I've tormented
you enough ;)


> +                bdrv_detach(bs, bs->peer);
> +                *ptr = NULL;
> +                break;
> +            }
> +        }
> +    }
> +
> +    /* clean up host side */
> +    drive_uninit(drive_get_by_blockdev(bs));
> +
> +    return 0;
> +}
> diff --git a/blockdev.h b/blockdev.h
> index 653affc..2a0559e 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -51,5 +51,6 @@ int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_change_block(Monitor *mon, const char *device,
>                      const char *filename, const char *fmt);
> +int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #endif
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e5585ba..d6dc18c 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -68,6 +68,24 @@ Eject a removable medium (use -f to force it).
>  ETEXI
>  
>      {
> +        .name       = "drive_del",
> +        .args_type  = "id:s",
> +        .params     = "device",
> +        .help       = "remove host block device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_drive_del,
> +    },
> +
> +STEXI
> +@item delete @var{device}
> +@findex delete
> +Remove host block device.  The result is that guest generated IO is no longer
> +submitted against the host device underlying the disk.  Once a drive has
> +been deleted, the QEMU Block layer returns -EIO which results in IO 
> +errors in the guest for applications that are reading/writing to the device.
> +ETEXI
> +
> +    {
>          .name       = "change",
>          .args_type  = "device:B,target:F,arg:s?",
>          .params     = "device filename [format]",
Kevin Wolf - Nov. 12, 2010, 4:39 p.m.
Am 12.11.2010 16:38, schrieb Ryan Harper:
> Currently device hotplug removal code is tied to device removal via
> ACPI.  All pci devices that are removable via device_del() require the
> guest to respond to the request.  In some cases the guest may not
> respond leaving the device still accessible to the guest.  The management
> layer doesn't currently have a reliable way to revoke access to host
> resource in the presence of an uncooperative guest.
> 
> This patch implements a new monitor command, drive_del, which
> provides an explicit command to revoke access to a host block device.
> 
> drive_del first quiesces the block device (qemu_aio_flush;
> bdrv_flush() and bdrv_close()).  This prevents further IO from being
> submitted against the host device.  Finally, drive_del cleans up
> pointers between the drive object (host resource) and the device
> object (guest resource).
> 
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> ---
>  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
>  blockdev.h      |    1 +
>  hmp-commands.hx |   18 ++++++++++++++++++
>  3 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 6cb179a..f6ac439 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -14,6 +14,8 @@
>  #include "qemu-option.h"
>  #include "qemu-config.h"
>  #include "sysemu.h"
> +#include "hw/qdev.h"
> +#include "block_int.h"
>  
>  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
>  
> @@ -597,3 +599,40 @@ int do_change_block(Monitor *mon, const char *device,
>      }
>      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>  }
> +
> +int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *id = qdict_get_str(qdict, "id");
> +    BlockDriverState *bs;
> +    BlockDriverState **ptr;
> +    Property *prop;
> +
> +    bs = bdrv_find(id);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> +        return -1;
> +    }
> +
> +    /* quiesce block driver; prevent further io */
> +    qemu_aio_flush();
> +    bdrv_flush(bs);
> +    bdrv_close(bs);
> +
> +    /* clean up guest state from pointing to host resource by
> +     * finding and removing DeviceState "drive" property */
> +    for (prop = bs->peer->info->props; prop && prop->name; prop++) {
> +        if (prop->info->type == PROP_TYPE_DRIVE) {
> +            ptr = qdev_get_prop_ptr(bs->peer, prop);
> +            if ((*ptr) == bs) {
> +                bdrv_detach(bs, bs->peer);
> +                *ptr = NULL;
> +                break;
> +            }
> +        }
> +    }
> +
> +    /* clean up host side */
> +    drive_uninit(drive_get_by_blockdev(bs));
> +
> +    return 0;
> +}
> diff --git a/blockdev.h b/blockdev.h
> index 653affc..2a0559e 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -51,5 +51,6 @@ int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_change_block(Monitor *mon, const char *device,
>                      const char *filename, const char *fmt);
> +int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #endif
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e5585ba..d6dc18c 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -68,6 +68,24 @@ Eject a removable medium (use -f to force it).
>  ETEXI
>  
>      {
> +        .name       = "drive_del",
> +        .args_type  = "id:s",
> +        .params     = "device",
> +        .help       = "remove host block device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_drive_del,
> +    },
> +
> +STEXI
> +@item delete @var{device}
> +@findex delete

I think this should be @item drive_del and @findex drive_del.

Kevin
Ryan Harper - Nov. 12, 2010, 4:45 p.m.
* Kevin Wolf <kwolf@redhat.com> [2010-11-12 10:43]:
> Am 12.11.2010 16:38, schrieb Ryan Harper:
> > Currently device hotplug removal code is tied to device removal via
> > ACPI.  All pci devices that are removable via device_del() require the
> > guest to respond to the request.  In some cases the guest may not
> > respond leaving the device still accessible to the guest.  The management
> > layer doesn't currently have a reliable way to revoke access to host
> > resource in the presence of an uncooperative guest.
> > 
> > This patch implements a new monitor command, drive_del, which
> > provides an explicit command to revoke access to a host block device.
> > 
> > drive_del first quiesces the block device (qemu_aio_flush;
> > bdrv_flush() and bdrv_close()).  This prevents further IO from being
> > submitted against the host device.  Finally, drive_del cleans up
> > pointers between the drive object (host resource) and the device
> > object (guest resource).
> > 
> > Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> > ---
> >  blockdev.c      |   39 +++++++++++++++++++++++++++++++++++++++
> >  blockdev.h      |    1 +
> >  hmp-commands.hx |   18 ++++++++++++++++++
> >  3 files changed, 58 insertions(+), 0 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 6cb179a..f6ac439 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -14,6 +14,8 @@
> >  #include "qemu-option.h"
> >  #include "qemu-config.h"
> >  #include "sysemu.h"
> > +#include "hw/qdev.h"
> > +#include "block_int.h"
> >  
> >  static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
> >  
> > @@ -597,3 +599,40 @@ int do_change_block(Monitor *mon, const char *device,
> >      }
> >      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> >  }
> > +
> > +int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > +{
> > +    const char *id = qdict_get_str(qdict, "id");
> > +    BlockDriverState *bs;
> > +    BlockDriverState **ptr;
> > +    Property *prop;
> > +
> > +    bs = bdrv_find(id);
> > +    if (!bs) {
> > +        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> > +        return -1;
> > +    }
> > +
> > +    /* quiesce block driver; prevent further io */
> > +    qemu_aio_flush();
> > +    bdrv_flush(bs);
> > +    bdrv_close(bs);
> > +
> > +    /* clean up guest state from pointing to host resource by
> > +     * finding and removing DeviceState "drive" property */
> > +    for (prop = bs->peer->info->props; prop && prop->name; prop++) {
> > +        if (prop->info->type == PROP_TYPE_DRIVE) {
> > +            ptr = qdev_get_prop_ptr(bs->peer, prop);
> > +            if ((*ptr) == bs) {
> > +                bdrv_detach(bs, bs->peer);
> > +                *ptr = NULL;
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    /* clean up host side */
> > +    drive_uninit(drive_get_by_blockdev(bs));
> > +
> > +    return 0;
> > +}
> > diff --git a/blockdev.h b/blockdev.h
> > index 653affc..2a0559e 100644
> > --- a/blockdev.h
> > +++ b/blockdev.h
> > @@ -51,5 +51,6 @@ int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data);
> >  int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
> >  int do_change_block(Monitor *mon, const char *device,
> >                      const char *filename, const char *fmt);
> > +int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
> >  
> >  #endif
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index e5585ba..d6dc18c 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -68,6 +68,24 @@ Eject a removable medium (use -f to force it).
> >  ETEXI
> >  
> >      {
> > +        .name       = "drive_del",
> > +        .args_type  = "id:s",
> > +        .params     = "device",
> > +        .help       = "remove host block device",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = do_drive_del,
> > +    },
> > +
> > +STEXI
> > +@item delete @var{device}
> > +@findex delete
> 
> I think this should be @item drive_del and @findex drive_del.

Yep.

> 
> Kevin

Patch

diff --git a/blockdev.c b/blockdev.c
index 6cb179a..f6ac439 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -14,6 +14,8 @@ 
 #include "qemu-option.h"
 #include "qemu-config.h"
 #include "sysemu.h"
+#include "hw/qdev.h"
+#include "block_int.h"
 
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
@@ -597,3 +599,40 @@  int do_change_block(Monitor *mon, const char *device,
     }
     return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
+
+int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    BlockDriverState *bs;
+    BlockDriverState **ptr;
+    Property *prop;
+
+    bs = bdrv_find(id);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, id);
+        return -1;
+    }
+
+    /* quiesce block driver; prevent further io */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+    bdrv_close(bs);
+
+    /* clean up guest state from pointing to host resource by
+     * finding and removing DeviceState "drive" property */
+    for (prop = bs->peer->info->props; prop && prop->name; prop++) {
+        if (prop->info->type == PROP_TYPE_DRIVE) {
+            ptr = qdev_get_prop_ptr(bs->peer, prop);
+            if ((*ptr) == bs) {
+                bdrv_detach(bs, bs->peer);
+                *ptr = NULL;
+                break;
+            }
+        }
+    }
+
+    /* clean up host side */
+    drive_uninit(drive_get_by_blockdev(bs));
+
+    return 0;
+}
diff --git a/blockdev.h b/blockdev.h
index 653affc..2a0559e 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -51,5 +51,6 @@  int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_change_block(Monitor *mon, const char *device,
                     const char *filename, const char *fmt);
+int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e5585ba..d6dc18c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -68,6 +68,24 @@  Eject a removable medium (use -f to force it).
 ETEXI
 
     {
+        .name       = "drive_del",
+        .args_type  = "id:s",
+        .params     = "device",
+        .help       = "remove host block device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_drive_del,
+    },
+
+STEXI
+@item delete @var{device}
+@findex delete
+Remove host block device.  The result is that guest generated IO is no longer
+submitted against the host device underlying the disk.  Once a drive has
+been deleted, the QEMU Block layer returns -EIO which results in IO 
+errors in the guest for applications that are reading/writing to the device.
+ETEXI
+
+    {
         .name       = "change",
         .args_type  = "device:B,target:F,arg:s?",
         .params     = "device filename [format]",