Re: [Qemu-devel] [PATCH] Discard old bitmap directories in QCOW2 image

[Andrey Shinkevich]

On 27/02/2019 18:22, Max Reitz wrote:
> On 27.02.19 16:15, Andrey Shinkevich wrote:
>>
>>
>> On 27/02/2019 16:25, Max Reitz wrote:
>>> On 27.02.19 14:16, Denis Lunev wrote:
>>>> On 2/27/19 4:00 PM, Max Reitz wrote:
>>>>> On 18.02.19 16:36, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 12.02.2019 15:35, Andrey Shinkevich wrote:
>>>>>>> Clean QCOW2 image from bitmap obsolete directory when a new one
>>>>>>> is allocated and stored. It slows down the image growth a little bit.
>>>>>>> The flag QCOW2_DISCARD_ALWAYS allows a call to raw_co_pdiscard()
>>>>>>> that does the actual cleaning of the image on disk.
>>>>>>> With the flag QCOW2_DISCARD_OTHER, a reference count of the cluster
>>>>>>> is updated only.
>>>>>>>
>>>>>>> Signed-off-by: Andrey Shinkevich <address@hidden>
>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>>>
>>>>>> side question: should not we change 
>>>>>> discard_passthrough[QCOW2_DISCARD_OTHER] to
>>>>>> true or at least flags&BDRV_O_UNMAP by default? What is the reason of 
>>>>>> not discarding
>>>>>> things in qcow2-cluster?
>>>>> As far as I remember the reason is that whenever you clean up something
>>>>> its cluster is probably going to be reused rather soon.  So cleaning up
>>>>> takes longer, repopulating that cluster takes longer, and you save only
>>>>> rather little space.
>>>>>
>>>>> This is also why I don't know whether this patch makes much sense.
>>>>>
>>>>> Max
>>>>>
>>>> This depands upon the amount of actually free clusters in the image.
>>>> If there a lot of them at the moment, this one could be refilled
>>>> any time later on.
>>>
>>> Well, it's a trade-off.  For small structures like the bitmap directory,
>>> I don't see the point in forcefully unmapping them.  We don't do it for
>>> the old L1 table either when that has to be resized.
>>>
>>> Discarding a whole bitmap (like in clear_bitmap_table()) is something
>>> else, though.  So I was too quick to dismiss the whole patch; parts of
>>> it do make sense.
>>>
>>> For bitmap tables...  I don't know exactly.  I don't think a bitmap
>>> table is ever going to be larger than the L1 table, so it should
>>> probably be handled the same way -- which is to keep QCOW2_DISCARD_OTHER.
>>>
>>> Max
>>>
>> Yes, Max. All what you wrote about do make the sense.
>> The reason behind the proposition to discard the obsolete bitmap
>> directories in the image is that it makes debugging of the iotests
>> easier for a developer. Otherwise, lots of irrelevant symbolic
>> information in the image forces to track which one is valid.
>> Particularly, I locate the byte that designates the bitmap flags to see
>> if the flag 'in-use' is not set. And I do that for every single bitmap
>> directory in the image.
>> Again, cleaning up the obsolete bitmap directories in the image serves
>> for the convenience of debugging. What do you think about it?
> 
> I don't think convenience of debugging is a good reason to implement
> something that is probably worse behavior when not debugging.  Of
> course, if the changed behavior is neither worse nor better, then we
> might as well go for it if it eases debugging.  But I think it is a bit
> worse.
> 
> Also, if you want to debug images you have created yourself (or at least
> are used by VMs you have control over), you can simply set
> pass-discard-other=on.
> 
> Max
> 
Thank you for your answer. So, I will leave the QCOW2_DISCARD_ALWAYS
in the clear_bitmap_table() only and will prepare the next version of
the patch, shall I?
-- 
With the best regards,
Andrey Shinkevich