From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55011) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzKPI-0002lD-Na for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:22:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzKPH-00077R-C7 for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:22:00 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:28554 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 1bzKPG-000770-SA for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:21:59 -0400 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> <03bec033-313e-978d-af1f-7d4203e9dcb8@virtuozzo.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <35139506-653c-f37b-1c48-c1f7106f1708@virtuozzo.com> Date: Wed, 26 Oct 2016 12:21:44 +0300 MIME-Version: 1.0 In-Reply-To: <03bec033-313e-978d-af1f-7d4203e9dcb8@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 26.10.2016 12:04, Vladimir Sementsov-Ogievskiy wrote: > 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 = or >>>>>>>>>> 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=20 >>>>>>>>>> just 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=20 >>>>>>>>> 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=20 >>>>>>> you 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 minima= ze >>>>> 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 allocat= ed >>>>> 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 fla= g. >>>> >>>> I wouldn't worry about reusing clusters of other bitmaps. Of course = we >>>> 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=20 >>>> being >>>> 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=20 >>>> 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 probab= ly >>>> 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 dat= a >>>> 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 singl= e >>>> in_use flag for all of the auto-loaded bitmaps will basically not ta= ke >>>> any time. >>>> >>>> On the other hand, as soon as just a single auto-loaded bitmap=20 >>>> survives >>>> a VM (or qemu-img) invocation, you will very, very likely safe at=20 >>>> least >>>> 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=20 >>> the >>> user is also positive when it comes to migration or read-only disks=20 >>> that >>> 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 incomin= g >>> 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,=20 >> I need to delete bitmaps when reopening R/W and in yours - set in_use=20 >> bit. >> >>> >>> If you delete the bitmaps only when the user asks you to, then you ca= n >>> 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. >> >> > > Trying to reuse clusters of in_use bitmaps (including contiguous=20 > allocations for bitmap tables) will complicate things a lot.. We can=20 > use extra clusters of one in_use bitmap to save another one, the same=20 > is for bitmap tables. Extra clusters of old bitmap table (in case of=20 > resize) can be used for saving other bitmap data, etc.. > > A compromise strategy of reusing: 1. accumulate all data clusters of in_use bitmaps, which was loaded by=20 this qcow2 instance (we will not touch other old in_use bitmaps) to=20 free_clusters list 2. for each persistent BdrvDirtyBitmap: If there is corresponding in_use bitmap in the image, and its=20 table size =3D=3D needed table size (there was no resizes), then let's re= use=20 bitmap table. else, move old bitmap table clusters to free_clusters and=20 allocate new table. 3. for each persistent BdrvDirtyBitmap: For bitmap data clusters, take them from free_clusters list, and=20 if it is empty - allocate new clusters. 4. free extra clusters from free_clusters list if any. This strategy is not optimal, but not bad I thing. Is it ok for you?=20 (I'm not sure that this all is not premature optimization, and may be=20 true way is to just free all old staff and reallocate it, as I do.) --=20 Best regards, Vladimir