qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-block] [PATCH] Discard old bitmap directories in QCOW2 image
Date: Mon, 1 Apr 2019 20:00:43 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]