Daniel Veillard wrote:
> On Wed, Apr 15, 2009 at 03:28:54PM -0400, Cole Robinson wrote:
>> Hi all,
>>
>> Attached is a stab at an API for cloning a storage volume. This patch
>> implements the command for FS volumes, along with a virsh command
>> 'vol-clone'. This builds on the previously posted 'drop pool lock during
>> volume creation' work.
>>
>> The new API call looks like:
>>
>> /**
>> * virStorageVolClone:
>> * @vol: pointer to storage vol to clone
>> * @xmldesc: description of volume to create
>> * @flags: flags for creation (unused, pass 0)
>> *
>> * Clone a storage volume within the parent pool.
>> * Information for the new volume (name, perms)
>> * are passed via a typical volume XML description
>> * Not all pools support cloning of volumes
>> *
>> * return the storage volume, or NULL on error
>> */
>> virStorageVolPtr
>> virStorageVolClone(virStorageVolPtr vol,
>> const char *xmldesc,
>> unsigned int flags)
>
> I was initially surprized by the xmldesc, but I think it makes perfect
> sense, based on existing APIs and the fact we may augment the amount of
> metadata and it's simpler to build the API directly with room for the
> extensions.
> I wonder if the entry point could be made to accept NULL as the input,
> then libvirt generate a name/uuid, keep other attributes the same and
> the returned object can be inspected to discover name/uuid.
>
That seems reasonable. If people think it's useful behavior, I can add it.
>> The user passes information about the new volume via <volume> XML.
>> Currently we only use the passed name and permissions values, but
>> backingStore info could also be relevant, and 'format' if we allow
>> cloning between image formats.
>
> We don't have an UUID for a volume ?
>
No, the unique identifier is the <key> field which is the absolute path
to the volume on the host. I don't think any of the storage drivers
actually allow manually setting this field, it is either generated using
the parent pool path and the new volume name, or dictated by storage
(/dev/sda3 or similar).
>> Current limitations:
>>
>> - Parsing the 'clone' xml will complain if the 'capacity' element hasn't
>> been specified, even though it is meaningless for the new volume. Not
>> too sure what the right thing to do here is.
>
> add an argument to the parsing routine asking to disable that check ?
>
Yeah I figured i'd end up down that route. I'll check it out.
>> - A clone of a raw sparse file will turn out fully allocated. Could
>> probably borrow code from 'cp' to try and get this right.
>
> I would not consider this a blocker though a change in behaviour there
> later on might be considered disruptive (or a great improvement
> depending who you're asking !)
>
Agreed, not critical.
>> - Doesn't handle backing stores. I think we would need to call out to
>> 'qemu-img convert' to have any chance of getting this right. And if we
>> do that, we could also allow cloning from one image format to another.
>>
>> A lot of the patch is just plumbing, which I will break out into
>> separate patches on the next post. For now, most of the interesting bits
>> are at near the bottom.
>
> No need to keep the generated doc files in the patch IMHO
>
Okay, I'll drop them.
Thanks,
Cole
> [...]
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index a028b21..0f4dd27 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -1047,6 +1047,9 @@ const char* virStorageVolGetKey (virStorageVolPtr vol);
>> virStorageVolPtr virStorageVolCreateXML (virStoragePoolPtr pool,
>> const char *xmldesc,
>> unsigned int flags);
>> +virStorageVolPtr virStorageVolClone (virStorageVolPtr vol,
>> + const char *xmldesc,
>> + unsigned int flags);
>> int virStorageVolDelete (virStorageVolPtr vol,
>> unsigned int flags);
>> int virStorageVolRef (virStorageVolPtr vol);
>
> Fine
>
> [...]
>> +/**
>> + * virStorageVolClone:
>> + * @vol: pointer to storage vol to clone
>> + * @xmldesc: description of volume to create
>> + * @flags: flags for creation (unused, pass 0)
>> + *
>> + * Clone a storage volume within the parent pool.
>> + * Information for the new volume (name, perms)
>> + * are passed via a typical volume XML description
>> + * Not all pools support cloning of volumes
>> + *
>> + * return the storage volume, or NULL on error
>> + */
>> +virStorageVolPtr
>> +virStorageVolClone(virStorageVolPtr vol,
>> + const char *xmldesc,
>> + unsigned int flags)
>> +{
>> + DEBUG("vol=%p, flags=%u", vol, flags);
>> +
>> + virResetLastError();
>> +
>> + if (!VIR_IS_STORAGE_VOL(vol)) {
>> + virLibConnError(NULL, VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__);
>> + return (NULL);
>> + }
>> +
>> + if (vol->conn->flags & VIR_CONNECT_RO) {
>> + virLibConnError(vol->conn, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
>> + goto error;
>> + }
>> +
>> + if (vol->conn->storageDriver && vol->conn->storageDriver->volClone) {
>> + virStorageVolPtr ret;
>> + ret = vol->conn->storageDriver->volClone (vol, xmldesc, flags);
>> + if (!ret)
>> + goto error;
>> + return ret;
>> + }
>> +
>> + virLibConnError (vol->conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
>> +
>> +error:
>> + /* Copy to connection error object for back compatability */
>> + virSetConnError(vol->conn);
>> + return NULL;
>> +}
>
> okay too
>
> [...]
>> +static int
>> +virStorageBackendFileSystemVolClone(virConnectPtr conn,
>> + virStorageVolDefPtr origvol,
>> + virStorageVolDefPtr newvol,
>> + unsigned int flags ATTRIBUTE_UNUSED)
>> +{
>> +
>> + int origfd = -1;
>> + int newfd = -1;
>> + int ret = -1;
>> + size_t bytes = 1024 * 1024;
>
> so the copy is done megabytes by megabytes
> That sounds a reasonable intermediate compromize between the daemon
> memory usage and the I/O efficiency, feedback from an I/O expert or
> based on cp or tar code could be interesting.
>
> In general patch looks good to me, I got a bit lost in the pool
> locking, but finally it sounds fine.
>
> Daniel
>
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list