[PATCH] storage: only fallocate when allocation matches capacity

[Christian Ehrhardt]

On Thu, Sep 3, 2020 at 12:36 PM Daniel P. Berrangé <berrange at redhat.com> 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 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.

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.

I think the past try to implicitly derive from allocation size has
proven to be misleading at best.

> 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 :|
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd