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

[Andrey Shinkevich]

On 01/04/2019 21:00, Max Reitz wrote:
> On 27.02.19 16:48, Andrey Shinkevich wrote:
>>
>>
>> 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?
> 
> (Sorry for the late reply, I was on vacation :-/)
> 
> Sounds good to me, yes.
> 
> Max
> 

Hello, Max!
Welcome back.
I sent the updated patch on 28/02/2019, the message ID is
<address@hidden>
-- 
With the best regards,
Andrey Shinkevich