[PATCH 1/3] qemu: implementation of transient option for qcow2 file

Peter Krempa pkrempa at redhat.com
Tue Jul 7 07:12:18 UTC 2020


On Mon, Jul 06, 2020 at 14:20:23 -0400, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> 
> Here is the implementation of transient option for qcow2 file.
> This gets available <transient/> directive in domain xml file
> like as:
> 
>     <disk type='file' device='disk'>
>       <driver name='qemu' type='qcow2'/>
>       <source file='/var/lib/libvirt/images/guest.qcow2'/>
>       <target dev='vda' bus='virtio'/>
>       <transient/>
>     </disk>
> 
> The internal procedure is as follows.
> When the qemu command line options are built, a new qcow2 image is created
> with backing qcow2 image by using qemu-img command. The backing image is the
> qcow2 file which is set as <source>.
> The filename of the new qcow2 image is original-source-file.TRANSIENT.
> The qemu-img will be:
> 
>     qemu-img create -f qcow2 -F qcow2 \
>             -b /var/lib/libvirt/images/guest.qcow2 \
>             /var/lib/libvirt/images/guest.qcow2.TRANSIENT
> 
> Then, it switches the disk path, virStorageSourcePtr src->path, to
> the new qcow2 image. The new image and the backing image is handled and
> the blockdev option of qemu will be built like as:
> 
>     -blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2",
>                 "node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
>     -blockdev '{"node-name":"libvirt-2-format","read-only":true,"driver":"qcow2",
>                 "file":"libvirt-2-storage","backing":null}' \
>     -blockdev '{"driver":"file","filename":"/var/lib/libvirt/images/guest.qcow2.TRANSIENT",
>                 "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
>     -blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2",
>                 "file":"libvirt-1-storage","backing":"libvirt-2-format"}'
> 
> The new qcow2 image is removed when the Guest is shutdowned, 
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> ---
>  src/qemu/qemu_block.c    | 71 ++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_block.h    |  7 ++++
>  src/qemu/qemu_domain.c   |  4 +++
>  src/qemu/qemu_process.c  |  3 ++
>  src/qemu/qemu_validate.c |  2 +-
>  5 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 6f9c707..5eb0225 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -27,6 +27,7 @@
>  #include "viralloc.h"
>  #include "virstring.h"
>  #include "virlog.h"
> +#include "virqemu.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> @@ -3438,3 +3439,73 @@ qemuBlockUpdateRelativeBacking(virDomainObjPtr vm,
>  
>      return 0;
>  }
> +
> +int
> +qemuBlockCreateTransientDisk(virStorageSourcePtr src,
> +                             qemuDomainObjPrivatePtr priv)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    virCommandPtr cmd = NULL;
> +    g_autofree char *filename = NULL;
> +    g_autofree char *dirname = NULL;
> +    const char *qemuImgPath;
> +    char *newpath;
> +    int err = -1;
> +
> +    if ((src->format != VIR_STORAGE_FILE_QCOW2)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("transient option supports qcow2"));
> +        return -1;
> +    }
> +
> +    if (!(qemuImgPath = qemuFindQemuImgBinary(priv->driver)))
> +        return -1;
> +
> +    newpath = g_strdup_printf("%s.TRANSIENT", src->path);

This assumes that 'src' is a local disk, but the code doesn't check it.
If e.g. an NBD disk which can have 'path' NULL is used it would crash.

Specifically since we can't even create a new NBD disk this won't even
wrok.

> +
> +    if (virFileExists(newpath)) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _(" '%s' is already exists. Please remove it."), newpath);
> +        goto cleanup;
> +    }
> +
> +    if (!(cmd = virCommandNewArgList(qemuImgPath,
> +                                     "create",
> +                                     "-f",
> +                                     "qcow2",
> +                                     "-F",
> +                                     "qcow2",
> +                                     "-b",
> +                                     NULL)))
> +        goto cleanup;

I'd strongly suggest you implement this after qemu starts using the new
blockdev-create blockjob before starting the cpus and install the
overlays using the snapshot command.

The above will not work or will require many adjustments e.g. for
luks-encrypted qcow2 files. which would require substantial changes.


> +    virQEMUBuildBufferEscapeComma(&buf, src->path);
> +    virCommandAddArgBuffer(cmd, &buf);
> +
> +    virQEMUBuildBufferEscapeComma(&buf, newpath);
> +    virCommandAddArgBuffer(cmd, &buf);
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    VIR_DEBUG("Original disk: %s Transient disk: %s", src->path, newpath);
> +
> +    g_free(src->path);
> +    src->path = newpath;

This must add a new virStorageSource as 'src' and move the original to
src->backingStore in the live configuration object. we should not try to
detect the files which we know are there.


> +
> +    err = 0;
> + cleanup:
> +    virBufferFreeAndReset(&buf);
> +    virCommandFree(cmd);
> +    if (err)
> +        g_free(newpath);
> +
> +    return err;
> +}
> +
> +void
> +qemuBlockRemoveTransientDisk(virStorageSourcePtr src)
> +{
> +    VIR_DEBUG("unlink transient disk: %s ", src->path);
> +    unlink(src->path);
> +}
> diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
> index 2ad2ce1..60c6898 100644
> --- a/src/qemu/qemu_block.h
> +++ b/src/qemu/qemu_block.h
> @@ -266,3 +266,10 @@ int
>  qemuBlockUpdateRelativeBacking(virDomainObjPtr vm,
>                                 virStorageSourcePtr src,
>                                 virStorageSourcePtr topsrc);
> +
> +int
> +qemuBlockCreateTransientDisk(virStorageSourcePtr src,
> +                             qemuDomainObjPrivatePtr priv);
> +
> +void
> +qemuBlockRemoveTransientDisk(virStorageSourcePtr src);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d5e3d1a..9dbf73c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8301,6 +8301,10 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>  
>      qemuDomainGetImageIds(cfg, vm, src, disksrc, &uid, &gid);
>  
> +    if (disk->transient)
> +        if (qemuBlockCreateTransientDisk(src, vm->privateData) < 0)
> +            return -1;

The function call semantically doesn't belong to
qemuDomainDetermineDiskChain. Please put it in appropriate places in the
callers or the function which prepares the domain disk definition.

Please also note that the hotplug code path then needs to be handled
separately.

> +
>      if (virStorageFileGetMetadata(src, uid, gid, report_broken) < 0)
>          return -1;
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 999e576..859ef82 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7597,6 +7597,9 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>              }
>  
>              qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src);
> +
> +            if (disk->transient)
> +                qemuBlockRemoveTransientDisk(disk->src);

Disk hot-unplug requires then the same treatment.

>          }
>      }
>  
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 584d137..cdda6ac 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -2098,7 +2098,7 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
>          }
>      }
>  
> -    if (disk->transient) {
> +    if ((disk->transient) && (disk->src->format != VIR_STORAGE_FILE_QCOW2)) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("transient disks not supported yet"));

You can install a qcow2 overlay on top of a raw file too. IMO the
implications of using <transient/> allow that.

As said above I'd strongly prefer if the overlay is created in qemu
using the blockdev-create blockjob (there is already infrastructure in
libvirt to achieve that).

Additionally there are corner cases which this patch doesn't deal with:

1) the virDomainBlockCopy operation flattens the backing chain into the
top level only. This means that <transient/> must be stripped or the
operation rejected, as otherwise shutting down the VM would end up
removing the disk image completely.

2) the same as above is used also for non-shared-storage migration where
we use block-copy internally to transport the disks, same as above
applies. Here again it requires careful consideration of the semantics,
e.g whether to reject it or e.g. copy it into the original filename and
strip <transient/> (we can't currently copy multiple layers).

3) active-layer virDomainBlockCommit moves the data from the transient
overlay into the original (now backing image). The <transient> flag is
stored in the disk struct though, so that would mean that the original
disk source would be removed after stopping the VM. block commit must
clear the <transient> flag.

One nice-to-have but not required modification would be to allow
configuration of the transient disk's overlay path.




More information about the libvir-list mailing list