[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] v2 Fix Block Hotplug race with drive_unplug
From: |
Ryan Harper |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] v2 Fix Block Hotplug race with drive_unplug() |
Date: |
Mon, 1 Nov 2010 16:06:14 -0500 |
User-agent: |
Mutt/1.5.6+20040907i |
* Markus Armbruster <address@hidden> [2010-10-29 09:08]:
> Ryan Harper <address@hidden> 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 <address@hidden>
> > ---
> > 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().
That works out nicely; and I can drop the drive_get_by_id() patch as
well. Thanks.
>
> > 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
> > address@hidden unplug @var{device}
> > address@hidden 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?
Well, most of the info in here is rather sparse as well, so there is
clear precedence for it's terseness; I'll be a bit more verbose in the
next version.
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
address@hidden
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH 2/3] v2 Fix Block Hotplug race with drive_unplug(),
Ryan Harper <=