[PATCH] storage: only fallocate when allocation matches capacity

[Daniel P. Berrangé]

On Thu, Sep 03, 2020 at 03:40:22PM +0200, Christian Ehrhardt wrote:
> 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.

We could add a 4th flag. 

   VIR_STORAGE_VOL_CREATE_PREALLOC_NONE
   VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA
   VIR_STORAGE_VOL_CREATE_PREALLOC_TRY_PAYLOAD
   VIR_STORAGE_VOL_CREATE_PREALLOC_FORCE_PAYLOAD

TRY_PAYLOAD == plain  fallocate() only, ignore ENOSYS

FORCE_PAYLOAD == fallocate() and fallback to write zeros on ENOSYS

virt-manager would try TRY_PAYLOAD so it mostly does the right thing
without being terribly slow on FS that lack fallocate().

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