Re: [libvirt] [PATCH] blockcopy: allow block device destination

Ping

On 08/13/2014 02:00 PM, Eric Blake wrote: > To date, anyone performing a block copy and pivot ends up with > the destination being treated as <disk type='file'>. While this > works for data access for a block device, it has at least one > noticeable shortcoming: virDomainGetBlockInfo() reports allocation > differently for block devices visited as files (the size of the > device) than for block devices visited as <disk type='block'> > (the maximum sector used, as reported by qemu); and this difference > is significant when trying to manage qcow2 format on block devices > that can be grown as needed. > > I still plan to add a virDomainBlockCopy() API that takes the > destination disk as XML, allowing full expressive capability > to copy to a network disk. But a new API can't be backported, > while a new flag to an existing API can. So this patch enhances > blockcopy to let the user flag when the resulting XML after the > copy must list the device as type='block'.

> * include/libvirt/libvirt.h.in (VIR_DOMAIN_BLOCK_REBASE_COPY_DEV): > New flag. > * src/libvirt.c (virDomainBlockRebase): Document it. > * tools/virsh-domain.c (opts_block_copy, blockJobImpl): Add > --blockdev option. > * tools/virsh.pod (blockcopy): Document it. > * src/qemu/qemu_driver.c (qemuDomainBlockRebase): Allow new flag. > (qemuDomainBlockCopy): Remember the flag. > * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Test it. > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > --- > include/libvirt/libvirt.h.in | 2 ++ > src/libvirt.c | 8 ++++++-- > src/qemu/qemu_driver.c | 12 ++++++++---- > .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 ++-- > tools/virsh-domain.c | 6 ++++++ > tools/virsh.pod | 7 +++++-- > 6 files changed, 29 insertions(+), 10 deletions(-) > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index 47ea695..f2a02ea 100644 > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -2590,6 +2590,8 @@ typedef enum { > VIR_DOMAIN_BLOCK_REBASE_RELATIVE = 1 << 4, /* Keep backing chain > referenced using relative > names */ > + VIR_DOMAIN_BLOCK_REBASE_COPY_DEV = 1 << 5, /* Treat destination as block > + device instead of file */ > } virDomainBlockRebaseFlags; > > int virDomainBlockRebase(virDomainPtr dom, const char *disk, > diff --git a/src/libvirt.c b/src/libvirt.c > index 992e4f2..c4643e8 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c > @@ -19881,7 +19881,10 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, > * pre-create files with relative backing file names, rather than the default > * of absolute backing file names; as a security precaution, you should > * generally only use reuse_ext with the shallow flag and a non-raw > - * destination file. > + * destination file. By default, the copy destination will be treated as > + * type='file', but using VIR_DOMAIN_BLOCK_REBASE_COPY_DEV treats the > + * destination as type='block' (affecting how virDomainGetBlockInfo() will > + * report allocation after pivoting). > * > * A copy job has two parts; in the first phase, the @bandwidth parameter > * affects how fast the source is pulled into the destination, and the job > @@ -19950,7 +19953,8 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, > virCheckNonNullArgGoto(base, error); > } else if (flags & (VIR_DOMAIN_BLOCK_REBASE_SHALLOW | > VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | > - VIR_DOMAIN_BLOCK_REBASE_COPY_RAW)) { > + VIR_DOMAIN_BLOCK_REBASE_COPY_RAW | > + VIR_DOMAIN_BLOCK_REBASE_COPY_DEV)) { > virReportInvalidArg(flags, > _("use of flags in %s requires a copy job"), > __FUNCTION__); > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index b6219ba..e74227e 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -15295,7 +15295,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, > > /* Preliminaries: find the disk we are editing, sanity checks */ > virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | > - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT, -1); > + VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | > + VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1); > > priv = vm->privateData; > cfg = virQEMUDriverGetConfig(driver); > @@ -15374,7 +15375,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, > if (VIR_ALLOC(mirror) < 0) > goto endjob; > /* XXX Allow non-file mirror destinations */ > - mirror->type = VIR_STORAGE_TYPE_FILE; > + mirror->type = flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV ? > + VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE; > > if (format) { > if ((mirror->format = virStorageFileFormatTypeFromString(format)) <= 0) { > @@ -15466,7 +15468,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, > VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | > VIR_DOMAIN_BLOCK_REBASE_COPY | > VIR_DOMAIN_BLOCK_REBASE_COPY_RAW | > - VIR_DOMAIN_BLOCK_REBASE_RELATIVE, -1); > + VIR_DOMAIN_BLOCK_REBASE_RELATIVE | > + VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1); > > if (!(vm = qemuDomObjFromDomain(dom))) > return -1; > @@ -15482,7 +15485,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, > format = "raw"; > flags &= ~(VIR_DOMAIN_BLOCK_REBASE_COPY | > VIR_DOMAIN_BLOCK_REBASE_COPY_RAW); > - return qemuDomainBlockCopy(vm, dom->conn, path, base, format, bandwidth, flags); > + return qemuDomainBlockCopy(vm, dom->conn, path, base, format, > + bandwidth, flags); > } > > return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth, NULL, > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml > index 46f2a3e..7495a45 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml > +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml > @@ -17,8 +17,8 @@ > <disk type='block' device='disk'> > <source dev='/dev/HostVG/QEMUGuest1'/> > <backingStore/> > - <mirror type='file' file='/dev/HostVG/QEMUGuest1Copy' job='copy' ready='yes'> > - <source file='/dev/HostVG/QEMUGuest1Copy'/> > + <mirror type='block' job='copy' ready='yes'> > + <source dev='/dev/HostVG/QEMUGuest1Copy'/> > </mirror> > <target dev='hda' bus='ide'/> > <address type='drive' controller='0' bus='0' target='0' unit='0'/> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index f7193cb..1fbd36e 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -1521,6 +1521,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, > flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; > if (vshCommandOptBool(cmd, "raw")) > flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; > + if (vshCommandOptBool(cmd, "blockdev")) > + flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; > if (vshCommandOptStringReq(ctl, cmd, "dest", &base) < 0) > goto cleanup; > ret = virDomainBlockRebase(dom, path, base, bandwidth, flags); > @@ -1828,6 +1830,10 @@ static const vshCmdOptDef opts_block_copy[] = { > .type = VSH_OT_BOOL, > .help = N_("use raw destination file") > }, > + {.name = "blockdev", > + .type = VSH_OT_BOOL, > + .help = N_("copy destination is block device instead of regular file") > + }, > {.name = "wait", > .type = VSH_OT_BOOL, > .help = N_("wait for job to reach mirroring phase") > diff --git a/tools/virsh.pod b/tools/virsh.pod > index f07deec..8178868 100644 > --- a/tools/virsh.pod > +++ b/tools/virsh.pod > @@ -873,7 +873,8 @@ unlimited. The hypervisor can choose whether to reject the value or > convert it to the maximum value allowed. > > =item B<blockcopy> I<domain> I<path> I<dest> [I<bandwidth>] [I<--shallow>] > -[I<--reuse-external>] [I<--raw>] [I<--wait> [I<--async>] [I<--verbose>]] > +[I<--reuse-external>] [I<--raw>] [I<--blockdev>] > +[I<--wait> [I<--async>] [I<--verbose>]] > [{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>] > > Copy a disk backing image chain to I<dest>. By default, this command > @@ -891,7 +892,9 @@ The format of the destination is determined by the first match in the > following list: if I<--raw> is specified, it will be raw; if > I<--reuse-external> is specified, the existing destination is probed > for a format; and in all other cases, the destination format will > -match the source format. > +match the source format. The destination is treated as a regular > +file unless I<--blockdev> is used to signal that it is a block > +device. > > By default, the copy job runs in the background, and consists of two > phases. Initially, the job must copy all data from the source, and > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org