On Thu, Sep 03, 2020 at 12:18:42PM +0200, Christian Ehrhardt wrote: > On Wed, Sep 2, 2020 at 6:49 PM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote: > > > > On 9/2/20 3:58 PM, Christian Ehrhardt wrote: > > > In c9ec7088 "storage: extend preallocation flags support for qemu-img" > > > the option to fallocate was added and meant to be active when (quote): > > > "the XML described storage <allocation> matches its <capacity>" > > > > > > Up until recently 81a3042a12 "storage_util: fix qemu-img sparse allocation" > > > the compared allocation size was an order of magnitude too small, but still > > > it does use fallocate too often unless capacity>allocation. > > > > > > This change fixes the comparison to match the intended description > > > of the feature. > > > > > > Fixes: c9ec7088c7a3f4cd26bb471f1f243931fff6f4f9 > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759454 > > > Fixes: https://bugs.launchpad.net/ubuntu/focal/+source/libvirt/+bug/1847105 > > > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx> > > > --- > > > src/storage/storage_util.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx> > > > > And sorry for making the mess earlier (~2 years ago). > > no problem - it turned out to be even more confusing. > > Due to some further testing and encouraged by feedback in the same > direction by Richard Lager (on CC now) I realized that while the > suggested change reads correct it will still not help my case :-/ > > Even if my fix lands, we are back to square one and would need > virt-manager to submit a different XML. > Remember: my target here would be to come back to pralloca=metadata as > it was before for image creations from virt-manager. > I've started that aspect of the discussion at the BZ [1] already. > > On the libvirt side allocation>capacity sounds like being wrong anyway. It is a bit wierd as an input XML from a mgmt app. It is to be expected as an output XML from libvirt though. Some filesystems, notably XFS, will sometimes speculatively over-allocate data extents in the belief that further size-extending writes will probably arrive. So you can end up with allocated blocks being greater than the current logical file size. > And if that is so we have these possible conditions: > - capacity==allocation now and before my change falloc > - capacity>allocation now and before my change metadata > - capacity<allocation before my change falloc, afterwards metadata > (but this one seems invalid anyway) > > So I wonder are we really back at me asking Cole to let virt-manager > request things differently which is how this started about a year ago? > Or was I wrong trying to make the code to match the wording in the > commit that added it and do we actually want it to behave differently > (read no falloc) for the XMLs sent by virt-manager as of today? I think we should provide three flags VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA VIR_STORAGE_VOL_CREATE_PREALLOC_NONE VIR_STORAGE_VOL_CREATE_PREALLOC_PAYLOAD as a way to get explicit behaviour, with those flags ignoring the "allocation" field. Only look at "allocation" if none of the flags are given for sake of back compat. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
