diff mbox

[RFC,COLO,v2,10/13] Backup: clear all bitmap when doing block checkpoint

Message ID 1427276174-9130-11-git-send-email-wency@cn.fujitsu.com
State New
Headers show

Commit Message

Wen Congyang March 25, 2015, 9:36 a.m. UTC 
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Cc: Jeff Cody <jcody@redhat.com>
---
 block/backup.c            | 12 ++++++++++++
 include/block/block_int.h |  1 +
 include/qemu/hbitmap.h    |  8 ++++++++
 util/hbitmap.c            | 19 +++++++++++++++++++
 4 files changed, 40 insertions(+)

Comments

Paolo Bonzini March 25, 2015, 12:55 p.m. UTC | #1 
On 25/03/2015 10:36, Wen Congyang wrote:
> 
> +void backup_do_checkpoint(BlockJob *job, Error **errp)
> +{
> +    BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
> +
> +    if (job->driver != &backup_job_driver) {
> +        error_setg(errp, "It is not backup job");
> +        return;
> +    }
> +
> +    hbitmap_reset_all(backup_job->bitmap);
> +}

Please add instead a block_job_do_checkpoint API, and a do_checkpoint
function pointer to BlockJobDriver.

> +{
> +#if 0
> +    hbitmap_reset(hb, 0, hb->size << hb->granularity);
> +#else
> +    uint64_t size = hb->size;
> +    unsigned int i;
> +
> +    /* Same as hbitmap_alloc() except memset() */
> +    for (i = HBITMAP_LEVELS; i-- > 0; ) {

This can be "--i >= 1"...

> +        size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> +        memset(hb->levels[i], 0, size * sizeof(unsigned long));
> +    }
> +
> +    assert(size == 1);
> +    hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);

... if you use "=" instead of "|=" here.

> +#endif
> +}

Please pick one implementation (no #if), and also add a testcase to
tests/test-hbitmap.c.

Paolo
Wen Congyang March 26, 2015, 12:59 a.m. UTC | #2 
On 03/25/2015 08:55 PM, Paolo Bonzini wrote:
> 
> 
> On 25/03/2015 10:36, Wen Congyang wrote:
>>
>> +void backup_do_checkpoint(BlockJob *job, Error **errp)
>> +{
>> +    BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
>> +
>> +    if (job->driver != &backup_job_driver) {
>> +        error_setg(errp, "It is not backup job");
>> +        return;
>> +    }
>> +
>> +    hbitmap_reset_all(backup_job->bitmap);
>> +}
> 
> Please add instead a block_job_do_checkpoint API, and a do_checkpoint
> function pointer to BlockJobDriver.
> 
>> +{
>> +#if 0
>> +    hbitmap_reset(hb, 0, hb->size << hb->granularity);
>> +#else
>> +    uint64_t size = hb->size;
>> +    unsigned int i;
>> +
>> +    /* Same as hbitmap_alloc() except memset() */
>> +    for (i = HBITMAP_LEVELS; i-- > 0; ) {
> 
> This can be "--i >= 1"...
> 
>> +        size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>> +        memset(hb->levels[i], 0, size * sizeof(unsigned long));
>> +    }
>> +
>> +    assert(size == 1);
>> +    hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
> 
> ... if you use "=" instead of "|=" here.
> 
>> +#endif
>> +}
> 
> Please pick one implementation (no #if), and also add a testcase to
> tests/test-hbitmap.c.

OK

Thanks
Wen Congyang

> 
> Paolo
> .
>
diff mbox

Patch

diff --git a/block/backup.c b/block/backup.c
index 1c535b1..4e9d535 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -435,3 +435,15 @@  void backup_start(BlockDriverState *bs, BlockDriverState *target,
     job->common.co = qemu_coroutine_create(backup_run);
     qemu_coroutine_enter(job->common.co, job);
 }
+
+void backup_do_checkpoint(BlockJob *job, Error **errp)
+{
+    BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
+
+    if (job->driver != &backup_job_driver) {
+        error_setg(errp, "It is not backup job");
+        return;
+    }
+
+    hbitmap_reset_all(backup_job->bitmap);
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 624945d..45d547b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -628,6 +628,7 @@  void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
                   Error **errp);
+void backup_do_checkpoint(BlockJob *job, Error **errp);
 
 void blk_dev_change_media_cb(BlockBackend *blk, bool load);
 bool blk_dev_has_removable_media(BlockBackend *blk);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 550d7ce..95a55e4 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -109,6 +109,14 @@  void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count);
 void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count);
 
 /**
+ * hbitmap_reset_all:
+ * @hb: HBitmap to operate on.
+ *
+ * Reset all bits in an HBitmap.
+ */
+void hbitmap_reset_all(HBitmap *hb);
+
+/**
  * hbitmap_get:
  * @hb: HBitmap to operate on.
  * @item: Bit to query (0-based).
diff --git a/util/hbitmap.c b/util/hbitmap.c
index ab13971..4111ca5 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -353,6 +353,25 @@  void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
     hb_reset_between(hb, HBITMAP_LEVELS - 1, start, last);
 }
 
+void hbitmap_reset_all(HBitmap *hb)
+{
+#if 0
+    hbitmap_reset(hb, 0, hb->size << hb->granularity);
+#else
+    uint64_t size = hb->size;
+    unsigned int i;
+
+    /* Same as hbitmap_alloc() except memset() */
+    for (i = HBITMAP_LEVELS; i-- > 0; ) {
+        size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+        memset(hb->levels[i], 0, size * sizeof(unsigned long));
+    }
+
+    assert(size == 1);
+    hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
+#endif
+}
+
 bool hbitmap_get(const HBitmap *hb, uint64_t item)
 {
     /* Compute position and bit in the last layer.  */