From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=34698 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P9K4z-0006Qc-6b for qemu-devel@nongnu.org; Fri, 22 Oct 2010 12:02:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P9JsT-0003Dd-7p for qemu-devel@nongnu.org; Fri, 22 Oct 2010 11:49:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:23306) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P9JsS-0003DJ-Nj for qemu-devel@nongnu.org; Fri, 22 Oct 2010 11:49:57 -0400 Date: Fri, 22 Oct 2010 13:46:53 -0200 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 2/2] v3 Fix Block Hotplug race with drive_unplug() Message-ID: <20101022134653.24c43dfb@doriath> In-Reply-To: <1287716153-25305-3-git-send-email-ryanh@us.ibm.com> References: <1287716153-25305-1-git-send-email-ryanh@us.ibm.com> <1287716153-25305-3-git-send-email-ryanh@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ryan Harper Cc: Stefan Hajnoczi , Anthony Liguori , qemu-devel@nongnu.org, Kevin Wolf On Thu, 21 Oct 2010 21:55:53 -0500 Ryan Harper 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 > --- > 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",