From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51756) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c3mXr-00085y-C8 for qemu-devel@nongnu.org; Mon, 07 Nov 2016 11:13:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c3mXm-0007oX-EX for qemu-devel@nongnu.org; Mon, 07 Nov 2016 11:13:15 -0500 Received: from mailhub.sw.ru ([195.214.232.25]:30790 helo=relay.sw.ru) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c3mXl-0007o3-VW for qemu-devel@nongnu.org; Mon, 07 Nov 2016 11:13:10 -0500 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-13-git-send-email-vsementsov@virtuozzo.com> <0cb068bb-e625-ecb3-0078-b20828920619@redhat.com> <5ce110e0-f204-affb-897f-7c6bada6739f@virtuozzo.com> <8ed2ee67-cf67-a80f-902f-c40a1409a2cd@redhat.com> <36fc9101-439b-0c4b-14d3-25045800a785@redhat.com> <7a302497-b114-e60f-c5b2-5609e7e55b50@virtuozzo.com> From: Vladimir Sementsov-Ogievskiy Message-ID: Date: Mon, 7 Nov 2016 19:12:49 +0300 MIME-Version: 1.0 In-Reply-To: <7a302497-b114-e60f-c5b2-5609e7e55b50@virtuozzo.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 12/22] qcow2-bitmap: add IN_USE flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com 25.10.2016 13:53, Vladimir Sementsov-Ogievskiy wrote: > 24.10.2016 20:18, Max Reitz wrote: >> On 24.10.2016 19:08, Max Reitz wrote: >>> On 24.10.2016 13:35, Vladimir Sementsov-Ogievskiy wrote: >>>> 24.10.2016 13:32, Vladimir Sementsov-Ogievskiy =D0=BF=D0=B8=D1=88=D0= =B5=D1=82: >>>>> 21.10.2016 22:58, Max Reitz =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> 07.10.2016 22:44, Max Reitz =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>>>>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >>>>>>>>> This flag means that the bitmap is now in use by the software o= r >>>>>>>>> was not >>>>>>>>> successfully saved. In any way, with this flag set the bitmap=20 >>>>>>>>> data >>>>>>>>> must >>>>>>>>> be considered inconsistent and should not be loaded. >>>>>>>>> >>>>>>>>> With current implementation this flag is never set, as we just=20 >>>>>>>>> remove >>>>>>>>> bitmaps from the image after loading. But it defined in qcow2=20 >>>>>>>>> spec >>>>>>>>> and >>>>>>>>> must be handled. Also, it can be used in future, if async=20 >>>>>>>>> schemes of >>>>>>>>> bitmap loading/saving are implemented. >>>>>>>>> >>>>>>>>> We also remove in-use bitmaps from the image on open. >>>>>>>>> >>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>>>>>>>> >>>>>>>>> --- >>>>>>>>> block/qcow2-bitmap.c | 17 ++++++++++++++++- >>>>>>>>> 1 file changed, 16 insertions(+), 1 deletion(-) >>>>>>>> Don't you want to make use of this flag? It would appear useful = to >>>>>>>> me if >>>>>>>> you just marked autoload bitmaps as in_use instead of deleting=20 >>>>>>>> them >>>>>>>> from >>>>>>>> the image when it's opened and then overwrite them when the=20 >>>>>>>> image is >>>>>>>> closed. >>>>>>> And what is the use of it? >>>>>> You don't need to free any bitmaps when opening the file, and you=20 >>>>>> don't >>>>>> need to allocate any new bitmap space when closing it. >>>>> As bitmaps are sparce in file, I need to allocate new space when >>>>> closing. Or free it... >>>> >>>> Is it a real problem to reallocate space in qcow2? If so, to minimaz= e >>>> allocations, I will have to make a list of clusters of in_use=20 >>>> bitmaps on >>>> close, and then save current bitmaps to these clusters (and allocate= d >>>> some additional clusters, or free extra clusters). >>> It's not a real problem, but it does take time, and I maintain that=20 >>> it's >>> time it doesn't need to take because you can just use the in_use flag. >>> >>> I wouldn't worry about reusing clusters of other bitmaps. Of course w= e >>> can do that later on in some optimization but not now. >>> >>> I just mean the basic case of some auto-loaded bitmap that is not bei= ng >>> deleted while the VM is running and is just saved back to the image=20 >>> file >>> once it is closed. I don't expect that users will always consume all = of >>> the auto-loaded bitmaps while the VM is active... >>> >>>> Also, if I don't free them on open, I'll have to free them on=20 >>>> remove of >>>> bdrv dirty bitmap.. >>> Yes, so? That takes time, but that is something the user will probabl= y >>> expect. I wouldn't expect opening of the file to take that time. >>> >>> Overall, it doesn't matter time-wise whether you free the bitmap data >>> when opening the image file or when the dirty bitmap is actually=20 >>> deleted >>> by the user or when the image file is closed. Just setting the single >>> in_use flag for all of the auto-loaded bitmaps will basically not tak= e >>> any time. >>> >>> On the other hand, as soon as just a single auto-loaded bitmap surviv= es >>> a VM (or qemu-img) invocation, you will very, very likely safe at lea= st >>> some time because writing the bitmap to the disk can reuse at least=20 >>> some >>> of the existing clusters. >> By the way, dealing with removal of bitmaps when they are deleted by t= he >> user is also positive when it comes to migration or read-only disks th= at >> are later reopened R/W: Currently, as far as I can see, you just keep >> the auto-load bitmaps in the image if the image is part of an incoming >> migration or opened read-only, but then you continue under the >> assumption that they are removed from the image. That's not very good. > > You are right, I need to handle reopening more carefully. In my way, I=20 > need to delete bitmaps when reopening R/W and in yours - set in_use bit. Now I think, that loading auto_loading bitmaps in qcow2_open is wrong. I=20 should load them only in bdrv_open, to avoid reloading bitmaps on=20 drv->bdrv_open/drv->bdrv_close (they are called from bdrv_snapshot_goto). So, it would be like this: on bdrv_open, after drv->bdrv_open call drv->load_autoloading_bitmaps, which will load bitmaps, mark them in_use in the image (if it is=20 writable), create corresponding BdrvDirtyBitmaps. If there _any_=20 conflicts with existing BdrvDirtyBitmaps then fail. on bdrv_close, before drv->bdrv_close call drv->store_persitstent_bitmaps= , which will store persistent bitmaps, set in_use to false and _delete_=20 corresponding BdrvDirtyBitmaps. Also, in qcow2_reopen_prepare in case of changing write-ability from=20 false to true we need to mark corresponding bitmaps in the image as=20 in_use.. And something like this for incoming migration too. qcow2_open will only load header extension fields to bs->opaque, and=20 check these fields. It will not load bitmap directory. > >> >> If you delete the bitmaps only when the user asks you to, then you can >> just return an error that the bitmap cannot be removed at this time >> because the image file is read-only or used in migration (and I don't >> think the latter can even happen when it's the user who has requested >> removal of the bitmap). >> >> Max >> > > Ok, I'll rewrite this way, using 'in_use' bit and trying to avoid=20 > allocate/free overhead. > > --=20 Best regards, Vladimir