Re: [PATCH] storage: only fallocate when allocation matches capacity

On Thu, Sep 3, 2020 at 12:36 PM Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > 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(a)redhat.com&gt; 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(a)canonical.com&gt; > > > > --- > > > > src/storage/storage_util.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > > > > > > 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. Independent to the issue I initially wanted to solve and to the other discussion on why fallocate on ZFS makes barely any sense I like the suggestion Daniel.