[Qemu-devel] [PATCH 0/2] v3 Decouple block device removal from device removal

[Qemu-devel] [PATCH 0/2] v3 Decouple block device removal from device removal

[Ryan Harper]

This patch series decouples the detachment of a block device from the removal
of the backing pci-device.  Removal of a hotplugged pci device requires the
guest to respond before qemu tears down the block device. In some cases, the
guest may not respond leaving the guest with continued access to the block
device.  

The new monitor command, drive_unplug, will revoke a guests access to the
block device independently of the removal of the pci device.

The first patch adds a new drive find method, the second patch implements the
monitor command and block layer changes.

Changes since v2:
- Added QMP command for drive_unplug()

Changes since v1:
- CodingStyle fixes
- Added qemu_aio_flush() to bdrv_unplug()

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>

[Qemu-devel] [PATCH 1/2] v2 Add drive_get_by_id

[Ryan Harper]

Add a function to find a drive by id string.

Changes since v1:
-Coding Style fix

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
 blockdev.c |   13 +++++++++++++
 blockdev.h |    1 +
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ff7602b..5fc3b9b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -75,6 +75,19 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
     return NULL;
 }
 
+DriveInfo *drive_get_by_id(const char *id)
+{
+    DriveInfo *dinfo;
+
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        if (strcmp(id, dinfo->id)) {
+            continue;
+        }
+        return dinfo;
+    }
+    return NULL;
+}
+
 int drive_get_max_bus(BlockInterfaceType type)
 {
     int max_bus;
diff --git a/blockdev.h b/blockdev.h
index 653affc..19c6915 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -38,6 +38,7 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 int drive_get_max_bus(BlockInterfaceType type);
 void drive_uninit(DriveInfo *dinfo);
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
+DriveInfo *drive_get_by_id(const char *id);
 
 QemuOpts *drive_add(const char *file, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, int *fatal_error);
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH 1/2] v2 Add drive_get_by_id

[Luiz Capitulino]

On Thu, 21 Oct 2010 21:55:52 -0500
Ryan Harper <ryanh@us.ibm.com> wrote:

> Add a function to find a drive by id string.
> 
> Changes since v1:
> -Coding Style fix
> 
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> ---
>  blockdev.c |   13 +++++++++++++
>  blockdev.h |    1 +
>  2 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index ff7602b..5fc3b9b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -75,6 +75,19 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>      return NULL;
>  }
>  
> +DriveInfo *drive_get_by_id(const char *id)
> +{
> +    DriveInfo *dinfo;
> +
> +    QTAILQ_FOREACH(dinfo, &drives, next) {
> +        if (strcmp(id, dinfo->id)) {
> +            continue;
> +        }

Very minor, but the following allows you to drop a statement:

if (!strcmp()) {
  return dinfo;
}

> +        return dinfo;
> +    }
> +    return NULL;
> +}
> +
>  int drive_get_max_bus(BlockInterfaceType type)
>  {
>      int max_bus;
> diff --git a/blockdev.h b/blockdev.h
> index 653affc..19c6915 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -38,6 +38,7 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>  int drive_get_max_bus(BlockInterfaceType type);
>  void drive_uninit(DriveInfo *dinfo);
>  DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
> +DriveInfo *drive_get_by_id(const char *id);
>  
>  QemuOpts *drive_add(const char *file, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>  DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, int *fatal_error);

[Qemu-devel] [PATCH 2/2] v3 Fix Block Hotplug race with drive_unplug()

[Ryan Harper]

Block hot unplug is racy since the guest is required to acknowlege the ACPI
unplug event; this may not happen synchronously with the device removal command

This series aims to close a gap where by mgmt applications that assume the
block resource has been removed without confirming that the guest has
acknowledged the removal may re-assign the underlying device to a second guest
leading to data leakage.

This series introduces a new montor command to decouple asynchornous device
removal from restricting guest access to a block device.  We do this by creating
a new monitor command drive_unplug which maps to a bdrv_unplug() command which
does a qemu_aio_flush; bdrv_flush() and bdrv_close().  Once complete, subsequent
IO is rejected from the device and the guest will get IO errors but continue to
function.

A subsequent device removal command can be issued to remove the device, to which
the guest may or maynot respond, but as long as the unplugged bit is set, no IO
will be sumbitted.

Changes since v2:
- Added qmp command for drive_unplug

Changes since v1:
- Added qemu_aio_flush() before bdrv_flush() to wait on pending io

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
 block.c         |    7 +++++++
 block.h         |    1 +
 blockdev.c      |   26 ++++++++++++++++++++++++++
 blockdev.h      |    1 +
 hmp-commands.hx |   15 +++++++++++++++
 qmp-commands.hx |   26 ++++++++++++++++++++++++++
 6 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index a19374d..be47655 100644
--- a/block.c
+++ b/block.c
@@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable)
     }
 }
 
+void bdrv_unplug(BlockDriverState *bs)
+{
+    qemu_aio_flush();
+    bdrv_flush(bs);
+    bdrv_close(bs);
+}
+
 int bdrv_is_removable(BlockDriverState *bs)
 {
     return bs->removable;
diff --git a/block.h b/block.h
index 5f64380..732f63e 100644
--- a/block.h
+++ b/block.h
@@ -171,6 +171,7 @@ void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
 BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
 void bdrv_set_removable(BlockDriverState *bs, int removable);
+void bdrv_unplug(BlockDriverState *bs);
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_is_sg(BlockDriverState *bs);
diff --git a/blockdev.c b/blockdev.c
index 5fc3b9b..68eb329 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -610,3 +610,29 @@ int do_change_block(Monitor *mon, const char *device,
     }
     return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
+
+int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    DriveInfo *dinfo;
+    BlockDriverState *bs;
+    const char *id;
+
+    if (!qdict_haskey(qdict, "id")) {
+        qerror_report(QERR_MISSING_PARAMETER, "id");
+        return -1;
+    }
+
+    id = qdict_get_str(qdict, "id");
+    dinfo = drive_get_by_id(id);
+    if (!dinfo) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, id);
+        return -1;
+    }
+
+    /* mark block device unplugged */
+    bs = dinfo->bdrv;
+    bdrv_unplug(bs);
+
+    return 0;
+}
+ 
diff --git a/blockdev.h b/blockdev.h
index 19c6915..ecb9ac8 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -52,5 +52,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_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81999aa..7a32a2e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -68,6 +68,21 @@ Eject a removable medium (use -f to force it).
 ETEXI
 
     {
+        .name       = "drive_unplug",
+        .args_type  = "id:s",
+        .params     = "device",
+        .help       = "unplug block device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_drive_unplug,
+    },
+
+STEXI
+@item unplug @var{device}
+@findex unplug
+Unplug block device.
+ETEXI
+
+    {
         .name       = "change",
         .args_type  = "device:B,target:F,arg:s?",
         .params     = "device filename [format]",
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..e8f3d4a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -338,6 +338,32 @@ Example:
 EQMP
 
     {
+        .name       = "drive_unplug",
+        .args_type  = "id:s",
+        .params     = "device",
+        .help       = "unplug block device",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = do_drive_unplug,
+    },
+
+SQMP
+drive unplug
+----------
+
+Unplug a block device.
+
+Arguments:
+
+- "id": the device's ID (json-string)
+
+Example:
+
+-> { "execute": "drive_unplug", "arguments": { "id": "drive-virtio-blk1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "cpu",
         .args_type  = "index:i",
         .params     = "index",
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH 2/2] v3 Fix Block Hotplug race with drive_unplug()

[Luiz Capitulino]

On Thu, 21 Oct 2010 21:55:53 -0500
Ryan Harper <ryanh@us.ibm.com> wrote:

> Block hot unplug is racy since the guest is required to acknowlege the ACPI
> unplug event; this may not happen synchronously with the device removal command
> 
> This series aims to close a gap where by mgmt applications that assume the
> block resource has been removed without confirming that the guest has
> acknowledged the removal may re-assign the underlying device to a second guest
> leading to data leakage.
> 
> This series introduces a new montor command to decouple asynchornous device
> removal from restricting guest access to a block device.  We do this by creating
> a new monitor command drive_unplug which maps to a bdrv_unplug() command which
> does a qemu_aio_flush; bdrv_flush() and bdrv_close().  Once complete, subsequent
> IO is rejected from the device and the guest will get IO errors but continue to
> function.
> 
> A subsequent device removal command can be issued to remove the device, to which
> the guest may or maynot respond, but as long as the unplugged bit is set, no IO
> will be sumbitted.
> 
> Changes since v2:
> - Added qmp command for drive_unplug
> 
> Changes since v1:
> - Added qemu_aio_flush() before bdrv_flush() to wait on pending io
> 
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> ---
>  block.c         |    7 +++++++
>  block.h         |    1 +
>  blockdev.c      |   26 ++++++++++++++++++++++++++
>  blockdev.h      |    1 +
>  hmp-commands.hx |   15 +++++++++++++++
>  qmp-commands.hx |   26 ++++++++++++++++++++++++++
>  6 files changed, 76 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index a19374d..be47655 100644
> --- a/block.c
> +++ b/block.c
> @@ -1328,6 +1328,13 @@ void bdrv_set_removable(BlockDriverState *bs, int removable)
>      }
>  }
>  
> +void bdrv_unplug(BlockDriverState *bs)
> +{
> +    qemu_aio_flush();
> +    bdrv_flush(bs);
> +    bdrv_close(bs);
> +}
> +
>  int bdrv_is_removable(BlockDriverState *bs)
>  {
>      return bs->removable;
> diff --git a/block.h b/block.h
> index 5f64380..732f63e 100644
> --- a/block.h
> +++ b/block.h
> @@ -171,6 +171,7 @@ void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
>                         BlockErrorAction on_write_error);
>  BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read);
>  void bdrv_set_removable(BlockDriverState *bs, int removable);
> +void bdrv_unplug(BlockDriverState *bs);
>  int bdrv_is_removable(BlockDriverState *bs);
>  int bdrv_is_read_only(BlockDriverState *bs);
>  int bdrv_is_sg(BlockDriverState *bs);
> diff --git a/blockdev.c b/blockdev.c
> index 5fc3b9b..68eb329 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -610,3 +610,29 @@ int do_change_block(Monitor *mon, const char *device,
>      }
>      return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>  }
> +
> +int do_drive_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    DriveInfo *dinfo;
> +    BlockDriverState *bs;
> +    const char *id;
> +
> +    if (!qdict_haskey(qdict, "id")) {
> +        qerror_report(QERR_MISSING_PARAMETER, "id");
> +        return -1;
> +    }

You can drop this check, "id" is a required argument, the upper layer will
perform the check for you.

> +
> +    id = qdict_get_str(qdict, "id");
> +    dinfo = drive_get_by_id(id);
> +    if (!dinfo) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> +        return -1;
> +    }
> +
> +    /* mark block device unplugged */
> +    bs = dinfo->bdrv;
> +    bdrv_unplug(bs);
> +
> +    return 0;
> +}
> + 
> diff --git a/blockdev.h b/blockdev.h
> index 19c6915..ecb9ac8 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -52,5 +52,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_unplug(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #endif
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 81999aa..7a32a2e 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -68,6 +68,21 @@ Eject a removable medium (use -f to force it).
>  ETEXI
>  
>      {
> +        .name       = "drive_unplug",
> +        .args_type  = "id:s",
> +        .params     = "device",
> +        .help       = "unplug block device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_drive_unplug,
> +    },
> +
> +STEXI
> +@item unplug @var{device}
> +@findex unplug
> +Unplug block device.
> +ETEXI
> +
> +    {
>          .name       = "change",
>          .args_type  = "device:B,target:F,arg:s?",
>          .params     = "device filename [format]",
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 793cf1c..e8f3d4a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -338,6 +338,32 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "drive_unplug",
> +        .args_type  = "id:s",
> +        .params     = "device",
> +        .help       = "unplug block device",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_drive_unplug,
> +    },
> +
> +SQMP
> +drive unplug
> +----------
> +
> +Unplug a block device.
> +
> +Arguments:
> +
> +- "id": the device's ID (json-string)
> +
> +Example:
> +
> +-> { "execute": "drive_unplug", "arguments": { "id": "drive-virtio-blk1" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "cpu",
>          .args_type  = "index:i",
>          .params     = "index",

Re: [Qemu-devel] [PATCH 0/2] v3 Decouple block device removal from device removal

[Luiz Capitulino]

On Thu, 21 Oct 2010 21:55:51 -0500
Ryan Harper <ryanh@us.ibm.com> wrote:

> This patch series decouples the detachment of a block device from the removal
> of the backing pci-device.  Removal of a hotplugged pci device requires the
> guest to respond before qemu tears down the block device. In some cases, the
> guest may not respond leaving the guest with continued access to the block
> device.  
> 
> The new monitor command, drive_unplug, will revoke a guests access to the
> block device independently of the removal of the pci device.
> 
> The first patch adds a new drive find method, the second patch implements the
> monitor command and block layer changes.

Reviewed the monitor part, I think we're waiting for Kevin's and/or
Markus's ACK to get this merged?

> 
> Changes since v2:
> - Added QMP command for drive_unplug()
> 
> Changes since v1:
> - CodingStyle fixes
> - Added qemu_aio_flush() to bdrv_unplug()
> 
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> 

Re: [Qemu-devel] [PATCH 0/2] v3 Decouple block device removal from device removal

[Kevin Wolf]

Am 22.10.2010 17:48, schrieb Luiz Capitulino:
> On Thu, 21 Oct 2010 21:55:51 -0500
> Ryan Harper <ryanh@us.ibm.com> wrote:
> 
>> This patch series decouples the detachment of a block device from the removal
>> of the backing pci-device.  Removal of a hotplugged pci device requires the
>> guest to respond before qemu tears down the block device. In some cases, the
>> guest may not respond leaving the guest with continued access to the block
>> device.  
>>
>> The new monitor command, drive_unplug, will revoke a guests access to the
>> block device independently of the removal of the pci device.
>>
>> The first patch adds a new drive find method, the second patch implements the
>> monitor command and block layer changes.
> 
> Reviewed the monitor part, I think we're waiting for Kevin's and/or
> Markus's ACK to get this merged?

Yes, I'm waiting for Markus' comments before applying this.

Maybe we shouldn't add this to QMP yet, drive_add doesn't exist there
either because we want to do it right with blockdev_add/del. On the
other hand Markus should know much better than me what the right thing
would look like - if he says that this interface will work with whatever
we're going to get with blockdev_*, I'm fine with merging it.

Kevin

Re: [Qemu-devel] [PATCH 0/2] v3 Decouple block device removal from device removal

[Ryan Harper]

* Kevin Wolf <kwolf@redhat.com> [2010-10-22 11:12]:
> Am 22.10.2010 17:48, schrieb Luiz Capitulino:
> > On Thu, 21 Oct 2010 21:55:51 -0500
> > Ryan Harper <ryanh@us.ibm.com> wrote:
> > 
> >> This patch series decouples the detachment of a block device from the removal
> >> of the backing pci-device.  Removal of a hotplugged pci device requires the
> >> guest to respond before qemu tears down the block device. In some cases, the
> >> guest may not respond leaving the guest with continued access to the block
> >> device.  
> >>
> >> The new monitor command, drive_unplug, will revoke a guests access to the
> >> block device independently of the removal of the pci device.
> >>
> >> The first patch adds a new drive find method, the second patch implements the
> >> monitor command and block layer changes.
> > 
> > Reviewed the monitor part, I think we're waiting for Kevin's and/or
> > Markus's ACK to get this merged?
> 
> Yes, I'm waiting for Markus' comments before applying this.
> 
> Maybe we shouldn't add this to QMP yet, drive_add doesn't exist there
> either because we want to do it right with blockdev_add/del. On the
> other hand Markus should know much better than me what the right thing
> would look like - if he says that this interface will work with whatever
> we're going to get with blockdev_*, I'm fine with merging it.

I can go either way.  Anthony asked me to re-spin with a QMP version;
but the libvirt patch works either via TextMonitor or JSONMonitor.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [PATCH 0/2] v3 Decouple block device removal from device removal

[Ryan Harper]

* Kevin Wolf <kwolf@redhat.com> [2010-10-22 11:12]:
> Am 22.10.2010 17:48, schrieb Luiz Capitulino:
> > On Thu, 21 Oct 2010 21:55:51 -0500
> > Ryan Harper <ryanh@us.ibm.com> wrote:
> > 
> >> This patch series decouples the detachment of a block device from the removal
> >> of the backing pci-device.  Removal of a hotplugged pci device requires the
> >> guest to respond before qemu tears down the block device. In some cases, the
> >> guest may not respond leaving the guest with continued access to the block
> >> device.  
> >>
> >> The new monitor command, drive_unplug, will revoke a guests access to the
> >> block device independently of the removal of the pci device.
> >>
> >> The first patch adds a new drive find method, the second patch implements the
> >> monitor command and block layer changes.
> > 
> > Reviewed the monitor part, I think we're waiting for Kevin's and/or
> > Markus's ACK to get this merged?
> 
> Yes, I'm waiting for Markus' comments before applying this.
> 
> Maybe we shouldn't add this to QMP yet, drive_add doesn't exist there
> either because we want to do it right with blockdev_add/del. On the
> other hand Markus should know much better than me what the right thing
> would look like - if he says that this interface will work with whatever
> we're going to get with blockdev_*, I'm fine with merging it.

I sent a new series moving the QMP command out into  a separate patch so
if we decide to not pick it up at this time you can just drop it.

The libvirt-side change is being picked up, waiting for some testing to
complete, any chance for some time from Markus to review this week? 


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com