9:51 a.m.
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