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

[Luiz Capitulino <lcapitulino@redhat.com>]

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