From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:56773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSogG-0006YI-JU for qemu-devel@nongnu.org; Tue, 22 Nov 2011 06:38:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RSogF-0006uA-6z for qemu-devel@nongnu.org; Tue, 22 Nov 2011 06:38:28 -0500 Received: from e23smtp03.au.ibm.com ([202.81.31.145]:59925) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSogE-0006tu-Mv for qemu-devel@nongnu.org; Tue, 22 Nov 2011 06:38:27 -0500 Received: from /spool/local by e23smtp03.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 22 Nov 2011 11:33:06 +1000 Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pAMBbrc24976696 for ; Tue, 22 Nov 2011 22:37:53 +1100 Received: from d23av01.au.ibm.com (loopback [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pAMBbqko008550 for ; Tue, 22 Nov 2011 22:37:53 +1100 Message-ID: <4ECB84A1.8000700@in.ibm.com> Date: Tue, 22 Nov 2011 16:46:49 +0530 From: supriya kannery MIME-Version: 1.0 References: <20111111064707.15024.69847.sendpatchset@skannery.in.ibm.com> <20111111064818.15024.2323.sendpatchset@skannery.in.ibm.com> <4ECA407A.6030507@in.ibm.com> <4ECB7866.50405@in.ibm.com> <4ECB81A5.60600@redhat.com> In-Reply-To: <4ECB81A5.60600@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , Luiz Capitulino , Christoph Hellwig , Supriya Kannery Kevin Wolf wrote: > Am 22.11.2011 11:24, schrieb supriya kannery: > >> Stefan Hajnoczi wrote: >> >>> On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery wrote: >>> >>> >>>> Stefan Hajnoczi wrote: >>>> >>>> >>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery >>>>> wrote: >>>>> >>>>> >>>>> >>>>>> @@ -708,17 +731,31 @@ int bdrv_reopen(BlockDriverState *bs, in >>>>>> qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name); >>>>>> return ret; >>>>>> } >>>>>> - open_flags = bs->open_flags; >>>>>> - bdrv_close(bs); >>>>>> >>>>>> - ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); >>>>>> - if (ret < 0) { >>>>>> - /* Reopen failed. Try to open with original flags */ >>>>>> - qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); >>>>>> - ret = bdrv_open(bs, bs->filename, open_flags, drv); >>>>>> + /* Use driver specific reopen() if available */ >>>>>> + if (drv->bdrv_reopen_prepare) { >>>>>> >>>>>> >>>>>> >>>>> This seems weird to me because we're saying a driver may have >>>>> drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare() >>>>> function doesn't check and return -ENOTSUP. >>>>> >>>>> >>>>> >>>> If drv->bdrv_reopen_prepare == NULL , then we are not calling the >>>> publick bdrv_reopen_prepare at all. Unless we later call public >>>> bdrv_reopen_prepare >>>> from elsewhere without checking drv->bdrv_reopen_prepare, a check for >>>> -ENOTSUP inside the public one is not needed right? >>>> >>>> Also, we are handling reopening for even those drivers which don't >>>> have its own bdrv_reopen_prepare defined, by taking the "else" >>>> control path. So condition for reporting "ENOTSUP" shouldn't come >>>> up as of now. Please let me know your thoughts. >>>> >>>> >>> How does VMDK implement its prepare/commit/abort? It needs to use the >>> "public" bdrv_reopen_prepare() function on its image files. >>> >>> >> bdrv_reopen() is the public interface which gets called by any of the >> image formats. >> So VMDK or any image format has to call bdrv_reopen which decides to call >> driver specific prepare/commit/abort or simply close and reopen the file. >> > > No, that doesn't work. In order to get all-or-nothing semantics, you > need to explicitly prepare all child images and only when you know the > results of all preparations, you can decide whether to commit or abort all. > bdrv_reopen_prepare/commit/abort will be implemented specific to VMDK in vmdk.c. Then for vmdk, drv->bdrv_reopen_prepare() will handle preparing child images and return success to bdrv_reopen () only if all of them get prepared successfully. The prepare/commit/abort concept we took up considering vmdk's special case of multiple files. So it is bdrv_reopen() which is public and called by hostcache change request for any of the image formats. It then routes the processing to respective prepare/commit/abort implemented by the drivers, including VMDK. In cases where drivers don't have their own implementation, default route is taken which is simply closing and opening the file. - thanks, Supriya > Kevin > >