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. 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. Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list