On 01/27/2012 04:40 PM, Zeeshan Ali (Khattak) wrote:
> From: "Zeeshan Ali (Khattak)" <zeeshanak@xxxxxxxxx>
>
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list