From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzK97-0004oy-NF for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:05:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzK94-0001wD-G0 for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:05:17 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:14902 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 1bzK94-0001vT-5E for qemu-devel@nongnu.org; Wed, 26 Oct 2016 05:05:14 -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> From: Vladimir Sementsov-Ogievskiy Message-ID: <03bec033-313e-978d-af1f-7d4203e9dcb8@virtuozzo.com> Date: Wed, 26 Oct 2016 12:04:56 +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. > >> >> 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. > > Trying to reuse clusters of in_use bitmaps (including contiguous=20 allocations for bitmap tables) will complicate things a lot.. We can use=20 extra clusters of one in_use bitmap to save another one, the same is for=20 bitmap tables. Extra clusters of old bitmap table (in case of resize)=20 can be used for saving other bitmap data, etc.. --=20 Best regards, Vladimir