On 07.11.2016 17:12, 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 пишет:
>>>>>> 21.10.2016 22:58, Max Reitz пишет:
>>>>>>> On 21.10.2016 17:34, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> 07.10.2016 22:44, Max Reitz пишет:
>>>>>>>>> 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
>>>>>>>>>> data
>>>>>>>>>> must
>>>>>>>>>> be considered inconsistent and should not be loaded.
>>>>>>>>>>
>>>>>>>>>> With current implementation this flag is never set, as we just
>>>>>>>>>> remove
>>>>>>>>>> bitmaps from the image after loading. But it defined in qcow2
>>>>>>>>>> spec
>>>>>>>>>> and
>>>>>>>>>> must be handled. Also, it can be used in future, if async
>>>>>>>>>> schemes of
>>>>>>>>>> bitmap loading/saving are implemented.
>>>>>>>>>>
>>>>>>>>>> We also remove in-use bitmaps from the image on open.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>>>>>>>> <vsementsov@virtuozzo.com>
>>>>>>>>>> ---
>>>>>>>>>>     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
>>>>>>>>> them
>>>>>>>>> from
>>>>>>>>> the image when it's opened and then overwrite them when the
>>>>>>>>> image is
>>>>>>>>> closed.
>>>>>>>> And what is the use of it?
>>>>>>> You don't need to free any bitmaps when opening the file, and 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 minimaze
>>>>> allocations, I will have to make a list of clusters of in_use
>>>>> bitmaps on
>>>>> close, and then save current bitmaps to these clusters (and allocated
>>>>> some additional clusters, or free extra clusters).
>>>> It's not a real problem, but it does take time, and I maintain that
>>>> 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 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 being
>>>> deleted while the VM is running and is just saved back to the image
>>>> 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
>>>>> remove of
>>>>> bdrv dirty bitmap..
>>>> Yes, so? That takes time, but that is something the user will probably
>>>> 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
>>>> 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 take
>>>> any time.
>>>>
>>>> On the other hand, as soon as just a single auto-loaded bitmap survives
>>>> a VM (or qemu-img) invocation, you will very, very likely safe at least
>>>> some time because writing the bitmap to the disk can reuse at least
>>>> some
>>>> of the existing clusters.
>>> By the way, dealing with removal of bitmaps when they are deleted by the
>>> user is also positive when it comes to migration or read-only disks 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 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
>> 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
> should load them only in bdrv_open, to avoid reloading bitmaps on
> 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
> writable), create corresponding BdrvDirtyBitmaps. If there _any_
> 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_
> corresponding BdrvDirtyBitmaps.

Sounds good.

> Also, in qcow2_reopen_prepare in case of changing write-ability from
> false to true we need to mark corresponding bitmaps in the image as
> in_use.. And something like this for incoming migration too.

Right.

> qcow2_open will only load header extension fields to bs->opaque, and
> check these fields. It will not load bitmap directory.

Yes, that sounds good.

Max