Re: [libvirt] [PATCH] Restore skipping of setting capacity
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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)