[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 22/08/14 14:26 -0600, Eric Blake wrote:
Ping

Sorry, I thought I'd responded to this.

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'.

Is there any situation where you would not want this behavior?  The
only thing I can think of is that we need to keep the current behavior
for backwards compatibility.  If this is the case, then I'd say the
patch looks reasonable to me.


One more question... What happens if this flag is used with a drive of
type file?  Can we raise an error in that case?

* 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 redhat com>
---
 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




--
Adam Litke


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]