[libvirt] Add virStorageVolResize()

[Eric Blake]

On 01/27/2012 04:40 PM, Zeeshan Ali (Khattak) wrote:
> From: "Zeeshan Ali (Khattak)" <zeeshanak at gnome.org>
> 
> Add a new function to allow changing of capacity of storage volumes.

I know you still have to rebase, but here's some more review comments:

> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 75ed676..a37bf7c 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -44,6 +44,11 @@ typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjP
>  typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStoragePoolObjPtr pool,
>                                               virStorageVolDefPtr origvol, virStorageVolDefPtr newvol,
>                                               unsigned int flags);
> +typedef int (*virStorageBackendVolumeResize)(virConnectPtr conn,
> +                                             virStoragePoolObjPtr pool,
> +                                             virStorageVolDefPtr vol,
> +                                             unsigned long long capacity,

You'll want to change this to long long.

> +                                             unsigned int flags);
>  
>  
> +static int
> +virStorageBackendFilesystemResizeQemuImg(const char *path,
> +                                         unsigned long long capacity)
> +{
> +    int ret = -1;
> +    char *img_tool;
> +    virCommandPtr cmd = NULL;
> +
> +    /* KVM is usually ahead of qemu on features, so try that first */
> +    img_tool = virFindFileInPath("kvm-img");
> +    if (!img_tool)
> +        img_tool = virFindFileInPath("qemu-img");

I'm surprised we're not caching this in some helper function.  Oh well,
that's an independent cleanup.

> +
> +    cmd = virCommandNew(img_tool);
> +    virCommandAddArgList(cmd, "resize", path, NULL);
> +    virCommandAddArgFormat(cmd, "%llu", capacity);

Guess what - qemu-img can do delta resizing, as well as shrinking.  In
fact, qemu-img can even do (sparse-only) resizing of raw files!  But for
raw files, I think we are better off doing it ourselves (as your patch
already did), as it means fewer forks and more control over whether the
result is sparse.

qemu-img resize foo 10M # reset size to 10M, regardless of whether that
is bigger or smaller - which means you have to check the capacity
yourself before calling qemu-img with a smaller capacity

qemu-img resize foo +10M # add 10M to capacity

qemu-img resize foo -10M # remove 10M from capacity

That all means that you have to be careful of the incoming flags, and
control whether you output a sign in the command line argument.


> +/**
> + * Resize a volume
> + */
> +static int
> +virStorageBackendFileSystemVolResize(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                                     virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> +                                     virStorageVolDefPtr vol,
> +                                     unsigned long long capacity,
> +                                     unsigned int flags)
> +{
> +    virCheckFlags(0, -1);
> +
> +    if (vol->target.format == VIR_STORAGE_FILE_RAW) {
> +        return virStorageFileResize(vol->target.path, capacity);

That signature loses the notion of whether you are doing a delta or an
absolute change.  Either you need to convert delta into absolute before
forwarding lower down the chain, or you need to add a bool parameter
stating whether a delta change is desired.

> +    } else if (vol->target.format == VIR_STORAGE_FILE_DIR) {
> +        virStorageReportError(VIR_ERR_NO_SUPPORT,
> +                              "%s", _("Changing size of directory based "
> +                                      "volumes is not supported"));
> +        return -1;


> @@ -1199,6 +1255,7 @@ virStorageBackend virStorageBackendDirectory = {
>      .createVol = virStorageBackendFileSystemVolCreate,
>      .refreshVol = virStorageBackendFileSystemVolRefresh,
>      .deleteVol = virStorageBackendFileSystemVolDelete,
> +    .resizeVol = virStorageBackendFileSystemVolResize,

I'd leave this one out.  That way, you don't have to handle
VIR_STORAGE_FILE_DIR in virStorageBackendFileSystemVolResize in the
first place.

>  
> +static int
> +storageVolumeResize(virStorageVolPtr obj,
> +                    unsigned long long capacity,
> +                    unsigned int flags)
> +{
> +    virStorageDriverStatePtr driver = obj->conn->storagePrivateData;
> +    virStorageBackendPtr backend;
> +    virStoragePoolObjPtr pool = NULL;
> +    virStorageVolDefPtr vol = NULL;
> +    int ret = -1;
> +
> +    virCheckFlags(0, -1);

Make sure you permit the flags you plan on supporting in at least one
backend (and it's okay if you don't support them all up front).

> +++ b/src/util/storage_file.c
> @@ -931,6 +931,22 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta)
>      VIR_FREE(meta);
>  }
>  
> +/**
> + * virStorageFileResize:
> + *
> + * Change the capacity of the raw storage file at 'path'.
> + */
> +int
> +virStorageFileResize(const char *path, unsigned long long capacity)
> +{
> +    if (truncate(path, capacity) < 0) {
> +        virReportSystemError(errno, _("Failed to truncate file '%s'"), path);
> +        return -1;
> +    }

Here, if the allocate flag is specified, you'd want two implementations:

If posix_fallocate exists, then open() the file, and use posix_fallocate
to ensure that the delta from the old size to the new size is allocated.

If it does not exist, then you need a while loop that writes a '\0' byte
every 4096 bytes or so, in order to force the capacity increase to be
non-sparse.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120127/85f5e56e/attachment-0001.sig>