[libvirt] [PATCH] Restore skipping of setting capacity

[Daniel P. Berrange]

On Wed, Apr 23, 2014 at 11:58:44AM -0600, Eric Blake wrote:
> 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...

Can we get a unit test for this disk probing scenario so we can catch
any future regression too.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|