From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57076) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QmQLz-0004HN-Q1 for qemu-devel@nongnu.org; Thu, 28 Jul 2011 09:10:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QmQLv-0007ux-E8 for qemu-devel@nongnu.org; Thu, 28 Jul 2011 09:10:19 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:60067) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QmQLv-0007ur-7e for qemu-devel@nongnu.org; Thu, 28 Jul 2011 09:10:15 -0400 Received: by gyg8 with SMTP id 8so2128148gyg.4 for ; Thu, 28 Jul 2011 06:10:14 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4E31389A.4080609@redhat.com> References: <20110727113000.25109.16204.sendpatchset@skannery> <20110727113045.25109.54866.sendpatchset@skannery> <4E3015EC.7070004@msgid.tls.msk.ru> <4E31389A.4080609@redhat.com> Date: Thu, 28 Jul 2011 14:10:14 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Supriya Kannery , Michael Tokarev , qemu-devel@nongnu.org, Christoph Hellwig On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf wrote: > Am 27.07.2011 16:51, schrieb Stefan Hajnoczi: >> 2011/7/27 Michael Tokarev : >>> 27.07.2011 15:30, Supriya Kannery wrote: >>>> New command "block_set" added for dynamically changing any of the bloc= k >>>> device parameters. For now, dynamic setting of hostcache params using = this >>>> command is implemented. Other block device parameter changes, can be >>>> integrated in similar lines. >>>> >>>> Signed-off-by: Supriya Kannery >>>> >>>> --- >>>> =A0block.c =A0 =A0 =A0 =A0 | =A0 54 ++++++++++++++++++++++++++++++++++= +++++++++++++++ >>>> =A0block.h =A0 =A0 =A0 =A0 | =A0 =A02 + >>>> =A0blockdev.c =A0 =A0 =A0| =A0 61 ++++++++++++++++++++++++++++++++++++= ++++++++++++++++++++ >>>> =A0blockdev.h =A0 =A0 =A0| =A0 =A01 >>>> =A0hmp-commands.hx | =A0 14 ++++++++++++ >>>> =A0qemu-config.c =A0 | =A0 13 +++++++++++ >>>> =A0qemu-option.c =A0 | =A0 25 ++++++++++++++++++++++ >>>> =A0qemu-option.h =A0 | =A0 =A02 + >>>> =A0qmp-commands.hx | =A0 28 +++++++++++++++++++++++++ >>>> =A09 files changed, 200 insertions(+) >>>> >>>> Index: qemu/block.c >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>> --- qemu.orig/block.c >>>> +++ qemu/block.c >>>> @@ -651,6 +651,34 @@ unlink_and_fail: >>>> =A0 =A0 =A0return ret; >>>> =A0} >>>> >>>> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags) >>>> +{ >>>> + =A0 =A0BlockDriver *drv =3D bs->drv; >>>> + =A0 =A0int ret =3D 0, open_flags; >>>> + >>>> + =A0 =A0/* Quiesce IO for the given block device */ >>>> + =A0 =A0qemu_aio_flush(); >>>> + =A0 =A0if (bdrv_flush(bs)) { >>>> + =A0 =A0 =A0 =A0qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name)= ; >>>> + =A0 =A0 =A0 =A0return ret; >>>> + =A0 =A0} >>>> + =A0 =A0open_flags =3D bs->open_flags; >>>> + =A0 =A0bdrv_close(bs); >>>> + >>>> + =A0 =A0ret =3D bdrv_open(bs, bs->filename, bdrv_flags, drv); >>>> + =A0 =A0if (ret < 0) { >>>> + =A0 =A0 =A0 =A0/* Reopen failed. Try to open with original flags */ >>>> + =A0 =A0 =A0 =A0qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); >>>> + =A0 =A0 =A0 =A0ret =3D bdrv_open(bs, bs->filename, open_flags, drv); >>>> + =A0 =A0 =A0 =A0if (ret < 0) { >>>> + =A0 =A0 =A0 =A0 =A0 =A0/* Reopen failed with orig and modified flags= */ >>>> + =A0 =A0 =A0 =A0 =A0 =A0abort(); >>>> + =A0 =A0 =A0 =A0} >>> >>> Can we please avoid this stuff completely? =A0Just keep the >>> old device open still, until you're sure new one is ok. >>> >>> Or else it will be quite dangerous command in many cases. >>> For example, after -runas/-chroot, or additional selinux >>> settings or whatnot. =A0And in this case, instead of merely >>> returning an error, we'll see abort(). =A0Boom. >> >> Slight complication for image formats that use a dirty bit here. =A0QED >> has a dirty bit. =A0VMDK also specifies one but we don't implement it >> today. >> >> If the image file is dirty then all its metadata will be scanned for >> consistency when it is opened. =A0This increases the bdrv_open() time >> and hence the downtime of the VM. =A0So it is not ideal to open the >> image file twice, even though there is no consistency problem. > > In general I really like understatements, but opening an image file > twice isn't only "not ideal", but should be considered verboten. Point taken. >> I'll think about this some more, there are a couple of solutions like >> keeping only the file descriptor around, introducing a flush command >> that makes sure the file is in a clean state, or changing QED to not >> do this. > > Changing the format drivers doesn't really look like the right solution. > > Keeping the fd around looks okay, we can probably achieve this by > introducing a bdrv_reopen function. It means that we may need to reopen > the format layer, but it can't really fail. My concern is that this assumes a single file descriptor. For vmdk there may be multiple split files. I'm almost starting to think bdrv_reopen() should be recursive down the BlockDriverState tree. Stefan