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 <andrey.shinkev...@virtuozzo.com> >>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>>>>>> >>>>>>> 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
signature.asc
Description: OpenPGP digital signature