Re: [PATCH] storage: only fallocate when allocation matches capacity
On Wed, Sep 2, 2020 at 6:49 PM Michal Privoznik <mprivozn(a)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(a)canonical.com>
> ---
> src/storage/storage_util.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)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.
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?
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1759454
Michal
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd