Ryan Harper <ry...@us.ibm.com> writes:
> 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 v1:
> - Added qemu_aio_flush() before bdrv_flush() to wait on pending io
>
> Signed-off-by: Ryan Harper <ry...@us.ibm.com>
> ---
> block.c | 7 +++++++
> block.h | 1 +
> blockdev.c | 26 ++++++++++++++++++++++++++
> blockdev.h | 1 +
> hmp-commands.hx | 15 +++++++++++++++
> 5 files changed, 50 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);
> +}
Stupid question: why doesn't bdrv_close() flush automatically?
And why do we have to flush here, but not before other uses of
bdrv_close(), such as eject_device()?
> +
> 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;
> + }
As Luiz pointed out, this check is redundant.
> +
> + 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;
> +}
> +
What about:
const char *id = qdict_get_str(qdict, "id");
BlockDriverState *bs;
bs = bdrv_find(id);
if (!bs) {
qerror_report(QERR_DEVICE_NOT_FOUND, id);
return -1;
}
bdrv_unplug(bs);
return 0;
Precedence: commit f8b6cc00 replaced uses of drive_get_by_id() by
bdrv_find().
> 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.
A bit terse, isn't it? What does it mean to unplug a block device?
What's its observable effect on the guest? Does it look like disk gone
completely south, perhaps?
> +ETEXI
> +
> + {
> .name = "change",
> .args_type = "device:B,target:F,arg:s?",
> .params = "device filename [format]",
