[PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only

Peter Krempa posted 1 patch 3 years, 5 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/b4cba30d43fb6e1885366bfaa23e228cdbd8ddf0.1605279320.git.pkrempa@redhat.com
src/qemu/qemu_backup.c | 42 ++++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 16 deletions(-)
[PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only
Posted by Peter Krempa 3 years, 5 months ago
Libvirt's backup code has two modes:

1) push - where qemu actively writes the difference since the checkpoint
          into the output file

2) pull - where we instruct qemu to expose a frozen disk state along
          with a bitmap of blocks which changed since the checkpoint

For push mode qemu needs the temporary bitmap we use where we calculate
the actual changes to be present on the block node backing the disk.

For pull mode where we expose the bitmap via NBD qemu actually wants the
bitmap to be present for the exported block node which is the scratch
file.

Until now we've calculated the bitmap twice and installed it both to the
scratch file and to the disk node, but we don't need to since we know
when it's needed.

Pass in the 'pull' flag and decide where to install the bitmap according
to it and also when to register the bitmap name with the blockjob.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_backup.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 99dee46c4f..09f7921ea7 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -109,6 +109,7 @@ struct qemuBackupDiskData {
     virStorageSourcePtr terminator;
     virStorageSourcePtr backingStore;
     char *incrementalBitmap;
+    const char *domdiskIncrementalBitmap; /* name of temporary bitmap installed on disk source */
     qemuBlockStorageSourceChainDataPtr crdata;
     bool labelled;
     bool initialized;
@@ -201,6 +202,7 @@ qemuBackupDiskPrepareOneBitmapsChain(virStorageSourcePtr backingChain,
 static int
 qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd,
                                 virJSONValuePtr actions,
+                                bool pull,
                                 GHashTable *blockNamedNodeData)
 {
     if (!qemuBlockBitmapChainIsValid(dd->domdisk->src,
@@ -212,21 +214,29 @@ qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd,
         return -1;
     }

-    if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
-                                             dd->domdisk->src,
-                                             dd->incrementalBitmap,
-                                             dd->backupdisk->incremental,
-                                             actions,
-                                             blockNamedNodeData) < 0)
-        return -1;
+    /* For pull-mode backup, we need the bitmap to be present in the scratch
+     * file as that will be exported. For push-mode backup the bitmap is
+     * actually required on top of the image backing the disk */

-    if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
-                                             dd->store,
-                                             dd->incrementalBitmap,
-                                             dd->backupdisk->incremental,
-                                             actions,
-                                             blockNamedNodeData) < 0)
-        return -1;
+    if (pull) {
+        if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
+                                                 dd->store,
+                                                 dd->incrementalBitmap,
+                                                 dd->backupdisk->incremental,
+                                                 actions,
+                                                 blockNamedNodeData) < 0)
+            return -1;
+    } else {
+        if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
+                                                 dd->domdisk->src,
+                                                 dd->incrementalBitmap,
+                                                 dd->backupdisk->incremental,
+                                                 actions,
+                                                 blockNamedNodeData) < 0)
+            return -1;
+
+        dd->domdiskIncrementalBitmap = dd->backupdisk->incremental;
+    }

     return 0;
 }
@@ -293,12 +303,12 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,
         else
             dd->incrementalBitmap = g_strdup_printf("backup-%s", dd->domdisk->dst);

-        if (qemuBackupDiskPrepareOneBitmaps(dd, actions, blockNamedNodeData) < 0)
+        if (qemuBackupDiskPrepareOneBitmaps(dd, actions, pull, blockNamedNodeData) < 0)
             return -1;
     }

     if (!(dd->blockjob = qemuBlockJobDiskNewBackup(vm, dd->domdisk, dd->store,
-                                                   dd->incrementalBitmap)))
+                                                   dd->domdiskIncrementalBitmap)))
         return -1;

     /* use original disk as backing to prevent opening the backing chain */
-- 
2.28.0

Re: [PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only
Posted by Eric Blake 3 years, 5 months ago
On 11/13/20 8:55 AM, Peter Krempa wrote:
> Libvirt's backup code has two modes:
> 
> 1) push - where qemu actively writes the difference since the checkpoint
>           into the output file
> 
> 2) pull - where we instruct qemu to expose a frozen disk state along
>           with a bitmap of blocks which changed since the checkpoint
> 
> For push mode qemu needs the temporary bitmap we use where we calculate
> the actual changes to be present on the block node backing the disk.
> 
> For pull mode where we expose the bitmap via NBD qemu actually wants the
> bitmap to be present for the exported block node which is the scratch
> file.
> 
> Until now we've calculated the bitmap twice and installed it both to the
> scratch file and to the disk node, but we don't need to since we know
> when it's needed.
> 
> Pass in the 'pull' flag and decide where to install the bitmap according
> to it and also when to register the bitmap name with the blockjob.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_backup.c | 42 ++++++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 16 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>


> +    /* For pull-mode backup, we need the bitmap to be present in the scratch
> +     * file as that will be exported. For push-mode backup the bitmap is
> +     * actually required on top of the image backing the disk */
> 
> -    if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> -                                             dd->store,
> -                                             dd->incrementalBitmap,
> -                                             dd->backupdisk->incremental,
> -                                             actions,
> -                                             blockNamedNodeData) < 0)
> -        return -1;
> +    if (pull) {
> +        if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> +                                                 dd->store,
> +                                                 dd->incrementalBitmap,
> +                                                 dd->backupdisk->incremental,
> +                                                 actions,
> +                                                 blockNamedNodeData) < 0)

Technically, with both qemu 5.0 and qemu 5.2, the bitmap can live in the
backing store for BOTH modes, (in 5.2 and beyond, because qemu will
chase through the backing chain to find the named bitmap if it is not
present in the temporary store; in 5.0 because the temporary store is
not a filter node).  But since qemu 5.1 used a filter node for the
temporary store but was unable to chase through filter nodes, always
placing the bitmap in the temporary node for pull mode is fine.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qemu: backup: Install bitmap for incremental backup to appropriate node only
Posted by Peter Krempa 3 years, 5 months ago
On Fri, Nov 13, 2020 at 09:07:24 -0600, Eric Blake wrote:
> On 11/13/20 8:55 AM, Peter Krempa wrote:
> > Libvirt's backup code has two modes:
> > 
> > 1) push - where qemu actively writes the difference since the checkpoint
> >           into the output file
> > 
> > 2) pull - where we instruct qemu to expose a frozen disk state along
> >           with a bitmap of blocks which changed since the checkpoint
> > 
> > For push mode qemu needs the temporary bitmap we use where we calculate
> > the actual changes to be present on the block node backing the disk.
> > 
> > For pull mode where we expose the bitmap via NBD qemu actually wants the
> > bitmap to be present for the exported block node which is the scratch
> > file.
> > 
> > Until now we've calculated the bitmap twice and installed it both to the
> > scratch file and to the disk node, but we don't need to since we know
> > when it's needed.
> > 
> > Pass in the 'pull' flag and decide where to install the bitmap according
> > to it and also when to register the bitmap name with the blockjob.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_backup.c | 42 ++++++++++++++++++++++++++----------------
> >  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> 
> > +    /* For pull-mode backup, we need the bitmap to be present in the scratch
> > +     * file as that will be exported. For push-mode backup the bitmap is
> > +     * actually required on top of the image backing the disk */
> > 
> > -    if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> > -                                             dd->store,
> > -                                             dd->incrementalBitmap,
> > -                                             dd->backupdisk->incremental,
> > -                                             actions,
> > -                                             blockNamedNodeData) < 0)
> > -        return -1;
> > +    if (pull) {
> > +        if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,
> > +                                                 dd->store,
> > +                                                 dd->incrementalBitmap,
> > +                                                 dd->backupdisk->incremental,
> > +                                                 actions,
> > +                                                 blockNamedNodeData) < 0)
> 
> Technically, with both qemu 5.0 and qemu 5.2, the bitmap can live in the
> backing store for BOTH modes, (in 5.2 and beyond, because qemu will
> chase through the backing chain to find the named bitmap if it is not
> present in the temporary store; in 5.0 because the temporary store is
> not a filter node).  But since qemu 5.1 used a filter node for the
> temporary store but was unable to chase through filter nodes, always
> placing the bitmap in the temporary node for pull mode is fine.

I can consider just putting it in one place. Since incremental backup in
libvirt requires 'blockdev-reopen' and that was not made stable yet in
qemu, libvirt will officially support incremental backups only with
qemu-6.0 at soonest.