On 04/24/2014 02:11 AM, Daniel P. Berrange wrote: > 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 :) >> >> 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. After more searching, I've learned that when the storage volume is first populated, we are getting things right (all volumes went through virStorageBackendProbeTarget which populated capacity first from the file system then updated it from the metadata); but later when it is time for virsh vol-dumpxml, virStorageBackendUpdateVolTargetInfo is called to refresh the data but does not look at the metadata, thus overwriting things back to the file size. If a volume has been resized by 'virsh vol-resize' in the meantime, neither the original capacity, nor the the file size capacity are correct - the only correct solution is to have the backend once again read the metadata. I'm still trying to figure out the best way to do that, but posted a couple patches along the way while still working on the issue. >> >> 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. The existing tests/virstoragetest.c validated that we are getting capacity correct when probing; where it failed was that it doesn't test that the higher level vol-dumpxml refresh uses the information vs. throwing it away. Yes, I plan to enhance the testsuite to ensure this case gets covered. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list