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 <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 >
Hello, Max! Welcome back. I sent the updated patch on 28/02/2019, the message ID is <1551346019-293202-1-git-send-email-andrey.shinkev...@virtuozzo.com> -- With the best regards, Andrey Shinkevich
