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

[Cole Robinson]

On 05/05/2015 10:44 PM, Prerna Saxena wrote:
> 
> 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.

My recollection is that like Jan says we implement raw volume creation
ourselves so that we could work without qemu-img in that case, like if
qemu-img isn't installed. And then raw cloning was kind of built upon the raw
creation code.

But IMO we should drop all that stuff and just use qemu-img unconditionally,
it's ubiquitous at this point.

- Cole