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@xxxxxxxxxx> > > --- > > 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list