From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=48444 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PD1aD-0005QF-JP for qemu-devel@nongnu.org; Mon, 01 Nov 2010 17:06:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PD1aC-0002H5-74 for qemu-devel@nongnu.org; Mon, 01 Nov 2010 17:06:25 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:51843) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PD1aC-0002Gi-47 for qemu-devel@nongnu.org; Mon, 01 Nov 2010 17:06:24 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e9.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id oA1KjMqH032372 for ; Mon, 1 Nov 2010 16:45:22 -0400 Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id oA1L6KVa178978 for ; Mon, 1 Nov 2010 17:06:20 -0400 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id oA1L6JY5022546 for ; Mon, 1 Nov 2010 15:06:20 -0600 Date: Mon, 1 Nov 2010 16:06:14 -0500 From: Ryan Harper Subject: Re: [Qemu-devel] [PATCH 2/3] v2 Fix Block Hotplug race with drive_unplug() Message-ID: <20101101210614.GF22904@us.ibm.com> References: <1288030956-28383-1-git-send-email-ryanh@us.ibm.com> <1288030956-28383-3-git-send-email-ryanh@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Stefan Hajnoczi , Anthony Liguori , Ryan Harper , qemu-devel@nongnu.org, Kevin Wolf * Markus Armbruster [2010-10-29 09:08]: > Ryan Harper 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 > > --- > > 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 > > +@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? 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 ryanh@us.ibm.com