[libvirt] [PATCH 1/2] Storage: Suppress metadata refresh for volumes being built.

[Prerna Saxena]

On Tuesday 05 May 2015 04:22 PM, Ján Tomko wrote:
> On Tue, May 05, 2015 at 03:24:31PM +0530, Prerna Saxena wrote:
>> On Tuesday 05 May 2015 03:20 PM, Prerna Saxena wrote:
>>> On Tuesday 05 May 2015 01:52 PM, Ján Tomko wrote:
>>>> On Tue, May 05, 2015 at 08:43:21AM +0530, Prerna Saxena wrote:
>>>>> Libvirt periodically calls 'stat' on all volumes in a storage pool,
>>>>> to update fields such as 'target.allocation'.
>>>>>
>>>>> The operation doesnt make sense for a volume which is curently being allocated.
>>>> From the comments in the storage driver, the point of allowing refresh
>>>> for a volume that is currently being allocated is to track the progress
>>>> of the allocation.
>>>>
>>>>> Also, the 'target.allocation' sub-field is taken into account while copying a raw image.
>>>>> To suppress any (potential) corruption, libvirt must not attempt to refresh a volume currently being built.
>>>> What would be the corruption?
>>>>
>>>> We do not allow using a volume that is currently building as a
>>>> source for cloning the volume - storageVolCreateXMLFrom checks for
>>>> origvol->building:
>>>>
>>>>     if (origvol->building) {
>>>>         virReportError(VIR_ERR_OPERATION_INVALID,
>>>>                        _("volume '%s' is still being allocated."),
>>>>                        origvol->name);
>>>>         goto cleanup;
>>>>     }
>>>>
>>> While running libvirt on PowerPC, I saw an interesting scenario. The 'target.allocation' field seemed to change for a volume getting allocated, and this would lead to incomplete copy. This would
>>> happen at random intervals, not deterministically. While looking through the code, I found this to be the other place in code where the same field seemed to change without a lock. Hence the patch.
>>>
> Oh, I was thinking of the soure volume for some reason.
>
> We correctly lock the pool before calling refreshVol, so changing the
> object should not be an issue.
> I think the bug is in storageVolCreateXMLFrom - it drops all the locks,
> but expects the allocation not to change.

Yes, and so I sent this patch that blocks a refresh for volumes being created.

> In storageVolCreateXML we work around this by creating a shallow copy of
> the volume.
>
>>> I have sent the second patch which fixes the erring code too :
>>>
>>> -    remain = vol->target.allocation;
>>> +    remain = inputvol->target.capacity;
>>>
>> More fundamental question -- why do we offload the copying of non-raw images to qemu-img tool, but make libvirt responsible for copying raw volumes ?
>> Would it not be better if libvirt called on 'qemu-img' to copy all types of volumes, including raw ones ?
>>
> This way, libvirt can create raw volumes even without qemu-img
> installed. I don't know if there's any other reason.
>

I'm sorry, libvirt does not copy raw volumes as a 'fallback mechanism'.
Libvirt chooses to copy raw volumes on its own, but calls on qemu-img to copy all other volume types.
Is it not better to let qemu-img copy all volume types , including raw ones ?
I wanted to check if there are reasons for this decision ? I'll be happy if the community could throw some light.
Also cc'ing Cole, who had put in some fixes in this area.

Regards,

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India