[libvirt] [PATCH] Restore skipping of setting capacity

[Eric Blake]

On 04/23/2014 07:28 AM, John Ferlan wrote:
> Commit id 'ac9a0963' refactored out the 'withCapacity' for the
> virStorageBackendUpdateVolInfo() API.  See:

Fortunately, we haven't released this regression of mine :)

> 
> http://www.redhat.com/archives/libvir-list/2014-April/msg00043.html
> 
> This resulted in a difference in how 'virsh vol-info --pool <poolName>
> <volume>' or 'virsh vol-list vol-list --pool <poolName> --details' outputs

s/vol-list vol-list/vol-list/

> the capacity information for a directory pool with a qcow2 sparse file.
> 
> 
> Results in listing a Capacity value.  Prior to the commit, the value would
> be '1.0 MiB' (1048576 bytes). However, after the commit the output would be
> (for example) '192.50 KiB', which for my system was the size of the volume
> in my file system (eg 'ls -l TestPool/temp_vol_1' results in '197120' bytes
> or 192.50 KiB). While perhaps technically correct, it's not necessarily
> what the user expected (certainly virt-test didn't expect it).
> 
> This patch restores the code to not update the target capacity for this path

More precisely, the code needs to be careful that we prefer capacity
information learned from file format metadata (qcow2), and fall back to
capacity from file system data; my refactoring accidentally swapped
things so that file system capacity took precedence over metadata capacity.

> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/storage/storage_backend.c         | 22 +++++++++++++++-------
>  src/storage/storage_backend.h         |  5 ++++-
>  src/storage/storage_backend_disk.c    |  2 +-
>  src/storage/storage_backend_fs.c      | 11 +++++++----
>  src/storage/storage_backend_gluster.c |  2 +-
>  src/storage/storage_backend_logical.c |  2 +-
>  src/storage/storage_backend_mpath.c   |  2 +-
>  src/storage/storage_backend_scsi.c    |  2 +-
>  8 files changed, 31 insertions(+), 17 deletions(-)

This still applies without conflict after Peter's 18-patch series (I was
a bit surprised on that point, but a nice surprise); hopefully that
means that it doesn't matter whether you commit before or after Peter.

The fact that we don't have any testsuite coverage of this directly in
'make check' is a bit disconcerting; but at least the external
regression testing is covering it.

However, I'm still a bit worried that we are just attacking the
symptoms, instead of addressing things correctly.  It seems like we
should be smarter in storage_backend_* to not override a capacity that
was already set when probing the file metadata.  There's a pretty
telling comment in virStorageFileGetMetadataInternal:

    /* XXX we should consider moving virStorageBackendUpdateVolInfo
     * code into this method, for non-magic files
     */

where I think that being a bit smarter about tracing which pieces of
information are gathered in which order, and then prioritizing them so
that metadata information takes priority over stat() information, could
avoid the need to pass an updateCapacity flag through quite so many
layers of function calls.

We may still go with your patch, but let's wait a day or two to see if I
can come up with something more elegant...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140423/b956a0e0/attachment-0001.sig>