[PATCH] storage: only fallocate when allocation matches capacity

[Daniel P. Berrangé]

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 at redhat.com> 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 at canonical.com>
> > > ---
> > >   src/storage/storage_util.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > Reviewed-by: Michal Privoznik <mprivozn at redhat.com>
> >
> > 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 :|