From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46228 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PBpmp-0006H1-L3 for qemu-devel@nongnu.org; Fri, 29 Oct 2010 10:18:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PBpjm-0003X1-5D for qemu-devel@nongnu.org; Fri, 29 Oct 2010 10:15:23 -0400 Received: from e4.ny.us.ibm.com ([32.97.182.144]:33805) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PBpjm-0003Wp-2a for qemu-devel@nongnu.org; Fri, 29 Oct 2010 10:15:22 -0400 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by e4.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id o9TDx4na010799 for ; Fri, 29 Oct 2010 09:59:04 -0400 Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o9TEFISs1638526 for ; Fri, 29 Oct 2010 10:15:19 -0400 Received: from d01av03.pok.ibm.com (loopback [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o9TEFIoS022642 for ; Fri, 29 Oct 2010 12:15:18 -0200 Message-ID: <4CCAD6F4.6010201@linux.vnet.ibm.com> Date: Fri, 29 Oct 2010 09:15:16 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/3] v2 Fix Block Hotplug race with drive_unplug() References: <1288030956-28383-1-git-send-email-ryanh@us.ibm.com> <1288030956-28383-3-git-send-email-ryanh@us.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Stefan Hajnoczi , Kevin Wolf , Ryan Harper , qemu-devel@nongnu.org On 10/29/2010 09:01 AM, Markus Armbruster wrote: > 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? > I don't think it's a bad idea to do that but to the extent that the block API is designed after posix file I/O, close does not usually imply flush. > And why do we have to flush here, but not before other uses of > bdrv_close(), such as eject_device()? > Good question. Kevin should also confirm, but looking at the code, I think flush() is needed before close. If there's a pending I/O event and you close before the I/O event is completed, you'll get a callback for completion against a bogus BlockDriverState. I can't find anything in either raw-posix or the generic block layer that would mitigate this. Regards, Anthony Liguori