[Qemu-devel] [PATCH 0/2] block/hmp: add sereval hmp commands for incremental backup

[Qemu-devel] [PATCH 0/2] block/hmp: add sereval hmp commands for incremental backup

[Rudy Zhang]

Add sereval hmp commands for incremental backup.
It need a bitmap to backup data from drive-image to incremental image,
so before it need add bitmap for this device to track io.

Usage:
   drive_backup [-n] [-f] device target [bitmap] [format]

Example:
   # qemu-img create -f qcow2 /path/incremental-1.qcow2 -b /path/full.qcow2
   (qemu) block_dirty_bitmap_add drive-virtio-disk0 bitmap0
   (qemu) drive-backup -n drive-virtio-disk0 /path/incremental-1.qcow2 bitmap0

Rudy Zhang (2):
  hmp: add hmp command for incremental backup
  hmp: add hmp commands dirty bitmap add/clear/remove'

 hmp-commands.hx | 47 ++++++++++++++++++++++++++++++--
 hmp.c           | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hmp.h           |  3 +++
 3 files changed, 129 insertions(+), 4 deletions(-)

-- 
2.6.4

[Qemu-devel] [PATCH 1/2] hmp: add hmp command for incremental backup

[Rudy Zhang]

Add hmp command for incremental backup in drive-backup.
It need a bitmap to backup data from drive-image to incremental image,
so before it need add bitmap for this device to track io.
Usage:
	drive_backup [-n] [-f] device target [bitmap] [format]

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
---
 hmp-commands.hx |  5 +++--
 hmp.c           | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..7378aaa 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1180,12 +1180,13 @@ ETEXI
 
     {
         .name       = "drive_backup",
-        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
-        .params     = "[-n] [-f] device target [format]",
+        .args_type  = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?",
+        .params     = "[-n] [-f] device target [bitmap] [format]",
         .help       = "initiates a point-in-time\n\t\t\t"
                       "copy for a device. The device's contents are\n\t\t\t"
                       "copied to the new image file, excluding data that\n\t\t\t"
                       "is written after the command is started.\n\t\t\t"
+                      "With bitmap will start incremental backup.\n\t\t\t"
                       "The -n flag requests QEMU to reuse the image found\n\t\t\t"
                       "in new-image-file, instead of recreating it from scratch.\n\t\t\t"
                       "The -f flag requests QEMU to copy the whole disk,\n\t\t\t"
diff --git a/hmp.c b/hmp.c
index 54f2620..f8c33cd 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1086,11 +1086,13 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
     const char *filename = qdict_get_str(qdict, "target");
+    const char *bitmap = qdict_get_try_str(qdict, "bitmap");
     const char *format = qdict_get_try_str(qdict, "format");
     bool reuse = qdict_get_try_bool(qdict, "reuse", false);
     bool full = qdict_get_try_bool(qdict, "full", false);
     enum NewImageMode mode;
     Error *err = NULL;
+    enum MirrorSyncMode sync;
 
     if (!filename) {
         error_setg(&err, QERR_MISSING_PARAMETER, "target");
@@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
         return;
     }
 
+    if (full && bitmap) {
+        error_setg(&err, "Parameter 'bitmap' if conflict with '-f'");
+        hmp_handle_error(mon, &err);
+        return;
+    } else if (full)
+        sync = MIRROR_SYNC_MODE_FULL;
+    else if (bitmap)
+        sync = MIRROR_SYNC_MODE_INCREMENTAL;
+    else
+        sync = MIRROR_SYNC_MODE_TOP;
+
     if (reuse) {
         mode = NEW_IMAGE_MODE_EXISTING;
     } else {
@@ -1105,8 +1118,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
     }
 
     qmp_drive_backup(device, filename, !!format, format,
-                     full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-                     true, mode, false, 0, false, NULL,
+                     sync, true, mode, false, 0, !!bitmap, bitmap,
                      false, 0, false, 0, &err);
     hmp_handle_error(mon, &err);
 }
-- 
2.6.4

Re: [Qemu-devel] [PATCH 1/2] hmp: add hmp command for incremental backup

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]

On 01/21/2016 04:22 AM, Rudy Zhang wrote:
> Add hmp command for incremental backup in drive-backup.
> It need a bitmap to backup data from drive-image to incremental image,
> so before it need add bitmap for this device to track io.
> Usage:
> 	drive_backup [-n] [-f] device target [bitmap] [format]
> 
> Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
> ---
>  hmp-commands.hx |  5 +++--
>  hmp.c           | 16 ++++++++++++++--
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index bb52e4d..7378aaa 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1180,12 +1180,13 @@ ETEXI
>  
>      {
>          .name       = "drive_backup",
> -        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
> -        .params     = "[-n] [-f] device target [format]",
> +        .args_type  = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?",
> +        .params     = "[-n] [-f] device target [bitmap] [format]",

This is HMP, so it may not matter, but this is not backwards compatible.
 Scripts targetting the old style of passing a format will now have that
format string interpreted as a bitmap name with no format.  Better would
be to stick [bitmap] at the end, not the middle.


> @@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
>          return;
>      }
>  
> +    if (full && bitmap) {
> +        error_setg(&err, "Parameter 'bitmap' if conflict with '-f'");

s/if conflict/conflicts/

> +        hmp_handle_error(mon, &err);
> +        return;
> +    } else if (full)
> +        sync = MIRROR_SYNC_MODE_FULL;

Needs {}.  Run your patch through scripts/checkpatch.pl, to flag this
and other style violations.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] hmp: add hmp command for incremental backup

[John Snow]

On 01/21/2016 11:39 AM, Eric Blake wrote:
> On 01/21/2016 04:22 AM, Rudy Zhang wrote:
>> Add hmp command for incremental backup in drive-backup.
>> It need a bitmap to backup data from drive-image to incremental image,
>> so before it need add bitmap for this device to track io.
>> Usage:
>> 	drive_backup [-n] [-f] device target [bitmap] [format]
>>
>> Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
>> ---
>>  hmp-commands.hx |  5 +++--
>>  hmp.c           | 16 ++++++++++++++--
>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index bb52e4d..7378aaa 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1180,12 +1180,13 @@ ETEXI
>>  
>>      {
>>          .name       = "drive_backup",
>> -        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
>> -        .params     = "[-n] [-f] device target [format]",
>> +        .args_type  = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?",
>> +        .params     = "[-n] [-f] device target [bitmap] [format]",
> 
> This is HMP, so it may not matter, but this is not backwards compatible.
>  Scripts targetting the old style of passing a format will now have that
> format string interpreted as a bitmap name with no format.  Better would
> be to stick [bitmap] at the end, not the middle.
> 

I'd like to also point out that the method of intuiting the backup mode
based on the parameters present won't expand very well as we add more
modes, especially if there's ever an addition for a differential backup
mode.

Maybe it's fine because it's HMP and backwards compatibility is not a
real concern ...

> 
>> @@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
>>          return;
>>      }
>>  
>> +    if (full && bitmap) {
>> +        error_setg(&err, "Parameter 'bitmap' if conflict with '-f'");
> 
> s/if conflict/conflicts/
> 
>> +        hmp_handle_error(mon, &err);
>> +        return;
>> +    } else if (full)
>> +        sync = MIRROR_SYNC_MODE_FULL;
> 
> Needs {}.  Run your patch through scripts/checkpatch.pl, to flag this
> and other style violations.
> 

-- 
—js

Re: [Qemu-devel] [PATCH 1/2] hmp: add hmp command for incremental backup

[张敏]

On 16/1/22 上午12:39, Eric Blake wrote:
> On 01/21/2016 04:22 AM, Rudy Zhang wrote:
>> Add hmp command for incremental backup in drive-backup.
>> It need a bitmap to backup data from drive-image to incremental image,
>> so before it need add bitmap for this device to track io.
>> Usage:
>> drive_backup [-n] [-f] device target [bitmap] [format]
>>
>> Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
>> ---
>>   hmp-commands.hx |  5 +++--
>>   hmp.c           | 16 ++++++++++++++--
>>   2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index bb52e4d..7378aaa 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1180,12 +1180,13 @@ ETEXI
>>
>>       {
>>           .name       = "drive_backup",
>> -        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
>> -        .params     = "[-n] [-f] device target [format]",
>> +        .args_type  = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?",
>> +        .params     = "[-n] [-f] device target [bitmap] [format]",
> This is HMP, so it may not matter, but this is not backwards compatible.
>   Scripts targetting the old style of passing a format will now have that
> format string interpreted as a bitmap name with no format.  Better would
> be to stick [bitmap] at the end, not the middle.

But I have a question: If I don't want to input a 'format', only use 'bitmap',
it will let 'bitmap' as 'format', This problem how to do it.

>
>> @@ -1098,6 +1100,17 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
>>           return;
>>       }
>>
>> +    if (full && bitmap) {
>> +        error_setg(&err, "Parameter 'bitmap' if conflict with '-f'");
> s/if conflict/conflicts/

oh，made a mistake.

>> +        hmp_handle_error(mon, &err);
>> +        return;
>> +    } else if (full)
>> +        sync = MIRROR_SYNC_MODE_FULL;
> Needs {}.  Run your patch through scripts/checkpatch.pl, to flag this
> and other style violations.

I have checked these patches，but I ignored these warnings.

Re: [Qemu-devel] [PATCH 1/2] hmp: add hmp command for incremental backup

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

On 01/21/2016 07:04 PM, 张敏 wrote:

>>> -        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
>>> -        .params     = "[-n] [-f] device target [format]",
>>> +        .args_type  = "reuse:-n,full:-f,device:B,target:s,bitmap:s?,format:s?",
>>> +        .params     = "[-n] [-f] device target [bitmap] [format]",
>> This is HMP, so it may not matter, but this is not backwards compatible.
>>   Scripts targetting the old style of passing a format will now have that
>> format string interpreted as a bitmap name with no format.  Better would
>> be to stick [bitmap] at the end, not the middle.
> 
> But I have a question: If I don't want to input a 'format', only use 'bitmap',
> it will let 'bitmap' as 'format', This problem how to do it.

Several solutions, some nicer than others, some more complicated than
others.  I don't have any strong preference, so much as pointing out the
design space:

1. You don't.  If you want to use 'bitmap', you MUST supply 'format'
(supplying format is good anyways, as implicit formats have led to CVEs
in the past).

1.a. Possible variation: Teach the command to allow empty '' format to
be a synonym for an implicit format, so that it could look like:

drive_backup device target '' /path/to/bitmap

2. You modify the HMP parser to accept optionally-named arguments, so
that you can then supply later optional arguments by name without having
to provide the earlier optional arguments.  Maybe looking something like:

drive_backup device target --bitmap=/path/to/bitmap

3. Instead of trying to overload an existing command, you create a new
command.  Particularly since your overload already declared that '-f'
and 'bitmap' are incompatible.

4. maybe something else?

>> Needs {}.  Run your patch through scripts/checkpatch.pl, to flag this
>> and other style violations.
> 
> I have checked these patches，but I ignored these warnings.

Not a good idea.  And even in the rare case that you plan on ignoring
the warnings, you should at least document in the commit message your
justification for doing so.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/2] hmp: add hmp commands dirty bitmap add/clear/remove'

[Rudy Zhang]

Add several hmp commands: 'block_dirty_bitmap_add', 'block_dirty_bitmap_clear',
'block_dirty_bitmap_remove'. The bitmap is used for incremental backup to trace io.

Usage:
	block_dirty_bitmap_add device bitmap [granularity] -- Add dirty bitmap for 'device'
	block_dirty_bitmap_clear device bitmap -- Clear dirty bitmap for 'device'
	block_dirty_bitmap_remove device bitmap -- Remove dirty bitmap for 'device'

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>
---
 hmp-commands.hx | 42 +++++++++++++++++++++++++++++++++++
 hmp.c           | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp.h           |  3 +++
 3 files changed, 113 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7378aaa..8f1f95b 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -154,6 +154,48 @@ ETEXI
     },
 
 STEXI
+@item block_dirty_bitmap_add
+@findex block_dirty_bitmap_add
+Add the block dirty bitmap.
+ETEXI
+
+    {
+        .name       = "block_dirty_bitmap_add",
+        .args_type  = "node:B,bitmap:s,granularity:o?",
+        .params     = "device bitmap [granularity]",
+        .help       = "Add dirty bitmap for 'device'",
+        .mhandler.cmd = hmp_block_dirty_bitmap_add,
+    },
+
+STEXI
+@item block_dirty_bitmap_clear
+@findex block_dirty_bitmap_clear
+Clear the block dirty bitmap.
+ETEXI
+
+    {
+        .name       = "block_dirty_bitmap_clear",
+        .args_type  = "node:B,bitmap:s",
+        .params     = "device bitmap",
+        .help       = "Clear dirty bitmap for 'device'",
+        .mhandler.cmd = hmp_block_dirty_bitmap_clear,
+    },
+
+STEXI
+@item block_dirty_bitmap_remove
+@findex block_dirty_bitmap_remove
+Remove the block dirty bitmap.
+ETEXI
+
+    {
+        .name       = "block_dirty_bitmap_remove",
+        .args_type  = "node:B,bitmap:s",
+        .params     = "device bitmap",
+        .help       = "Remove dirty bitmap for 'device'",
+        .mhandler.cmd = hmp_block_dirty_bitmap_remove,
+    },
+
+STEXI
 @item block_job_resume
 @findex block_job_resume
 Resume a paused block streaming operation.
diff --git a/hmp.c b/hmp.c
index f8c33cd..b5eecbc 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1495,6 +1495,74 @@ void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &error);
 }
 
+void hmp_block_dirty_bitmap_add(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *node = qdict_get_str(qdict, "node");
+    const char *bitmap = qdict_get_str(qdict, "bitmap");
+    uint32_t granularity = qdict_get_try_int(qdict, "granularity", 0);
+
+    if (!node) {
+        error_setg(&error, QERR_MISSING_PARAMETER, "node");
+        hmp_handle_error(mon, &error);
+        return;
+    }
+
+    if (!bitmap) {
+        error_setg(&error, QERR_MISSING_PARAMETER, "bitmap");
+        hmp_handle_error(mon, &error);
+        return;
+    }
+
+    qmp_block_dirty_bitmap_add(node, bitmap,
+                             !!granularity, granularity, &error);
+    hmp_handle_error(mon, &error);
+}
+
+void hmp_block_dirty_bitmap_clear(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *node = qdict_get_str(qdict, "node");
+    const char *bitmap = qdict_get_str(qdict, "bitmap");
+
+    if (!node) {
+        error_setg(&error, QERR_MISSING_PARAMETER, "node");
+        hmp_handle_error(mon, &error);
+        return;
+    }
+
+    if (!bitmap) {
+        error_setg(&error, QERR_MISSING_PARAMETER, "bitmap");
+        hmp_handle_error(mon, &error);
+        return;
+    }
+
+    qmp_block_dirty_bitmap_clear(node, bitmap, &error);
+    hmp_handle_error(mon, &error);
+}
+
+void hmp_block_dirty_bitmap_remove(Monitor *mon, const QDict *qdict)
+{
+    Error *error = NULL;
+    const char *node = qdict_get_str(qdict, "node");
+    const char *bitmap = qdict_get_str(qdict, "bitmap");
+
+    if (!node) {
+        error_setg(&error, QERR_MISSING_PARAMETER, "node");
+        hmp_handle_error(mon, &error);
+        return;
+    }
+
+    if (!bitmap) {
+        error_setg(&error, QERR_MISSING_PARAMETER, "bitmap");
+        hmp_handle_error(mon, &error);
+        return;
+    }
+
+    qmp_block_dirty_bitmap_remove(node, bitmap, &error);
+    hmp_handle_error(mon, &error);
+}
+
 typedef struct HMPMigrationStatus
 {
     QEMUTimer *timer;
diff --git a/hmp.h b/hmp.h
index a8c5b5a..ea07116 100644
--- a/hmp.h
+++ b/hmp.h
@@ -81,6 +81,9 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
 void hmp_block_job_pause(Monitor *mon, const QDict *qdict);
 void hmp_block_job_resume(Monitor *mon, const QDict *qdict);
 void hmp_block_job_complete(Monitor *mon, const QDict *qdict);
+void hmp_block_dirty_bitmap_add(Monitor *mon, const QDict *qdict);
+void hmp_block_dirty_bitmap_clear(Monitor *mon, const QDict *qdict);
+void hmp_block_dirty_bitmap_remove(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
 void hmp_device_add(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
-- 
2.6.4