Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
On 04/08/2015 04:19 PM, John Snow wrote:
> For "dirty-bitmap" sync mode, the block job will iterate through the
> given dirty bitmap to decide if a sector needs backup (backup all the
> dirty clusters and skip clean ones), just as allocation conditions of
> "top" sync mode.
>
> Signed-off-by: Fam Zheng
> Signed-off-by: John Snow
> ---
> block.c | 9 +++
> block/backup.c| 156
> +++---
> block/mirror.c| 4 ++
> blockdev.c| 18 +-
> hmp.c | 3 +-
> include/block/block.h | 1 +
> include/block/block_int.h | 2 +
> qapi/block-core.json | 13 ++--
> qmp-commands.hx | 7 ++-
> 9 files changed, 180 insertions(+), 33 deletions(-)
>
Just reviewing the interface...
> +++ b/qapi/block-core.json
> @@ -512,10 +512,12 @@
> #
> # @none: only copy data written from now on
> #
> +# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.4
> +#
> # Since: 1.3
> ##
> { 'enum': 'MirrorSyncMode',
> - 'data': ['top', 'full', 'none'] }
> + 'data': ['top', 'full', 'none', 'dirty-bitmap'] }
>
> ##
> # @BlockJobType:
> @@ -690,14 +692,17 @@
> # probe if @mode is 'existing', else the format of the source
> #
> # @sync: what parts of the disk image should be copied to the destination
> -#(all the disk, only the sectors allocated in the topmost image, or
> -#only new I/O).
> +#(all the disk, only the sectors allocated in the topmost image,
> from a
> +#dirty bitmap, or only new I/O).
> #
> # @mode: #optional whether and how QEMU should create a new image, default is
> #'absolute-paths'.
> #
> # @speed: #optional the maximum speed, in bytes per second
> #
> +# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
> +# (Since 2.4)
> +#
> # @on-source-error: #optional the action to take on an error on the source,
> # default 'report'. 'stop' and 'enospc' can only be used
> # if the block device supports io-status (see BlockInfo).
> @@ -715,7 +720,7 @@
> { 'type': 'DriveBackup',
>'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -'*speed': 'int',
> +'*speed': 'int', '*bitmap': 'str',
Looks okay. Is it an error if bitmap is supplied, but mode is not
dirty-bitmap? Likewise, if mode is dirty-bitmap but bitmap is not supplied?
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
On 04/17/2015 06:51 PM, Eric Blake wrote:
On 04/08/2015 04:19 PM, John Snow wrote:
For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.
Signed-off-by: Fam Zheng
Signed-off-by: John Snow
---
block.c | 9 +++
block/backup.c| 156 +++---
block/mirror.c| 4 ++
blockdev.c| 18 +-
hmp.c | 3 +-
include/block/block.h | 1 +
include/block/block_int.h | 2 +
qapi/block-core.json | 13 ++--
qmp-commands.hx | 7 ++-
9 files changed, 180 insertions(+), 33 deletions(-)
Just reviewing the interface...
+++ b/qapi/block-core.json
@@ -512,10 +512,12 @@
#
# @none: only copy data written from now on
#
+# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.4
+#
# Since: 1.3
##
{ 'enum': 'MirrorSyncMode',
- 'data': ['top', 'full', 'none'] }
+ 'data': ['top', 'full', 'none', 'dirty-bitmap'] }
##
# @BlockJobType:
@@ -690,14 +692,17 @@
# probe if @mode is 'existing', else the format of the source
#
# @sync: what parts of the disk image should be copied to the destination
-#(all the disk, only the sectors allocated in the topmost image, or
-#only new I/O).
+#(all the disk, only the sectors allocated in the topmost image, from a
+#dirty bitmap, or only new I/O).
#
# @mode: #optional whether and how QEMU should create a new image, default is
#'absolute-paths'.
#
# @speed: #optional the maximum speed, in bytes per second
#
+# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
+# (Since 2.4)
+#
# @on-source-error: #optional the action to take on an error on the source,
# default 'report'. 'stop' and 'enospc' can only be used
# if the block device supports io-status (see BlockInfo).
@@ -715,7 +720,7 @@
{ 'type': 'DriveBackup',
'data': { 'device': 'str', 'target': 'str', '*format': 'str',
'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-'*speed': 'int',
+'*speed': 'int', '*bitmap': 'str',
Looks okay. Is it an error if bitmap is supplied, but mode is not
dirty-bitmap? Likewise, if mode is dirty-bitmap but bitmap is not supplied?
Yes:
+if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+if (!sync_bitmap) {
+error_setg(errp, "must provide a valid bitmap name for "
+ "\"dirty-bitmap\" sync mode");
+return;
+}
+
[...]
+} else if (sync_bitmap) {
+error_setg(errp,
+ "a sync_bitmap was provided to backup_run, "
+ "but received an incompatible sync_mode (%s)",
+ MirrorSyncMode_lookup[sync_mode]);
+return;
+}
+
Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
On 04/17/2015 05:02 PM, John Snow wrote:
>>> { 'type': 'DriveBackup',
>>> 'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>>> 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
>>> -'*speed': 'int',
>>> +'*speed': 'int', '*bitmap': 'str',
>>
>> Looks okay. Is it an error if bitmap is supplied, but mode is not
>> dirty-bitmap? Likewise, if mode is dirty-bitmap but bitmap is not
>> supplied?
>>
>
> Yes:
Then we _could_ do this as a flat union (here, using a shorthand syntax
of anonymous inline types, although qapi does not yet support it):
{ 'type': 'DriveBackupBase',
'data': { 'device': 'str', 'target': 'str', '*format': 'str',
'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
'*speed': 'int', '*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError' } }
{ 'union': 'DriveBackup', 'base': 'DriveBackupBase',
'discriminator': 'sync',
'data': { 'top': {}, 'full': {}, 'none': {},
'dirty-bitmap': { 'bitmap': 'str' } } }
which would enforce that 'bitmap' be present exactly when
'sync':'dirty-bitmap'. But that's probably overkill; I don't expect you
to do it (especially since qapi shorthand for anonymous inline types is
not there yet).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
On 09.04.2015 00:19, John Snow wrote:
For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.
Signed-off-by: Fam Zheng
Signed-off-by: John Snow
---
block.c | 9 +++
block/backup.c| 156 +++---
block/mirror.c| 4 ++
blockdev.c| 18 +-
hmp.c | 3 +-
include/block/block.h | 1 +
include/block/block_int.h | 2 +
qapi/block-core.json | 13 ++--
qmp-commands.hx | 7 ++-
9 files changed, 180 insertions(+), 33 deletions(-)
diff --git a/block.c b/block.c
index 9d30379..2367311 100644
--- a/block.c
+++ b/block.c
@@ -5717,6 +5717,15 @@ static void bdrv_reset_dirty(BlockDriverState *bs,
int64_t cur_sector,
}
}
+/**
+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+assert(hbi->hb);
+hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
return hbitmap_count(bitmap->bitmap);
diff --git a/block/backup.c b/block/backup.c
index 1c535b1..8513917 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,8 @@ typedef struct CowRequest {
typedef struct BackupBlockJob {
BlockJob common;
BlockDriverState *target;
+/* bitmap for sync=dirty-bitmap */
+BdrvDirtyBitmap *sync_bitmap;
MirrorSyncMode sync_mode;
RateLimit limit;
BlockdevOnError on_source_error;
@@ -242,6 +244,92 @@ static void backup_complete(BlockJob *job, void *opaque)
g_free(data);
}
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+{
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+/* we need to yield so that qemu_aio_flush() returns.
+ * (without, VM does not reboot)
+ */
+if (job->common.speed) {
+uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+ job->sectors_read);
+job->sectors_read = 0;
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+} else {
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+}
+
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+return false;
+}
+
+static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+{
+bool error_is_read;
+int ret = 0;
+int clusters_per_iter;
+uint32_t granularity;
+int64_t sector;
+int64_t cluster;
+int64_t end;
+int64_t last_cluster = -1;
+BlockDriverState *bs = job->common.bs;
+HBitmapIter hbi;
+
+granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
+clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
DIV_ROUND_UP(granularity, BACKUP_CLUSTER_SIZE) would've worked, too
(instead of the MAX()), but since both are powers of two, this is
equivalent.
+bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+
+/* Find the next dirty sector(s) */
+while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped */
+if (cluster != last_cluster + 1) {
+job->common.offset += ((cluster - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+}
+
+for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
+if (yield_and_check(job)) {
+return ret;
+}
+
+do {
+ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER,
&error_is_read);
+if ((ret < 0) &&
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+return ret;
+}
Now that I'm reading this code again... The other backup implementation
handles retries differently; it redoes the whole loop, with the
effective difference being that it calls yield_and_check() between every
retry. Would it make sense to move the yield_and_check() call into this
loop?
+} while (ret < 0);
+}
+
+/* If the bitmap granularity is smaller than the backup granularity,
+ * we need to advance the iterator pointer to the next cluster. */
+if (granularity < BACKUP_CLUSTER_SIZE) {
Actually, whenever BACKUP_CLUSTER_SIZE isn't a factor of granularity.
Both are powers of two, though, so that's the case iff granularity <
BACKUP_CLUSTER_SIZE. (thus, the condition is correct)
+bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
+
[Qemu-block] [PATCH v5 10/21] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.
Signed-off-by: Fam Zheng
Signed-off-by: John Snow
---
block.c | 9 +++
block/backup.c| 156 +++---
block/mirror.c| 4 ++
blockdev.c| 18 +-
hmp.c | 3 +-
include/block/block.h | 1 +
include/block/block_int.h | 2 +
qapi/block-core.json | 13 ++--
qmp-commands.hx | 7 ++-
9 files changed, 180 insertions(+), 33 deletions(-)
diff --git a/block.c b/block.c
index 9d30379..2367311 100644
--- a/block.c
+++ b/block.c
@@ -5717,6 +5717,15 @@ static void bdrv_reset_dirty(BlockDriverState *bs,
int64_t cur_sector,
}
}
+/**
+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+assert(hbi->hb);
+hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
return hbitmap_count(bitmap->bitmap);
diff --git a/block/backup.c b/block/backup.c
index 1c535b1..8513917 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,8 @@ typedef struct CowRequest {
typedef struct BackupBlockJob {
BlockJob common;
BlockDriverState *target;
+/* bitmap for sync=dirty-bitmap */
+BdrvDirtyBitmap *sync_bitmap;
MirrorSyncMode sync_mode;
RateLimit limit;
BlockdevOnError on_source_error;
@@ -242,6 +244,92 @@ static void backup_complete(BlockJob *job, void *opaque)
g_free(data);
}
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+{
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+/* we need to yield so that qemu_aio_flush() returns.
+ * (without, VM does not reboot)
+ */
+if (job->common.speed) {
+uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+ job->sectors_read);
+job->sectors_read = 0;
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+} else {
+block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+}
+
+if (block_job_is_cancelled(&job->common)) {
+return true;
+}
+
+return false;
+}
+
+static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+{
+bool error_is_read;
+int ret = 0;
+int clusters_per_iter;
+uint32_t granularity;
+int64_t sector;
+int64_t cluster;
+int64_t end;
+int64_t last_cluster = -1;
+BlockDriverState *bs = job->common.bs;
+HBitmapIter hbi;
+
+granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
+clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
+bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+
+/* Find the next dirty sector(s) */
+while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+/* Fake progress updates for any clusters we skipped */
+if (cluster != last_cluster + 1) {
+job->common.offset += ((cluster - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+}
+
+for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
+if (yield_and_check(job)) {
+return ret;
+}
+
+do {
+ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+BACKUP_SECTORS_PER_CLUSTER,
&error_is_read);
+if ((ret < 0) &&
+backup_error_action(job, error_is_read, -ret) ==
+BLOCK_ERROR_ACTION_REPORT) {
+return ret;
+}
+} while (ret < 0);
+}
+
+/* If the bitmap granularity is smaller than the backup granularity,
+ * we need to advance the iterator pointer to the next cluster. */
+if (granularity < BACKUP_CLUSTER_SIZE) {
+bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
+}
+
+last_cluster = cluster - 1;
+}
+
+/* Play some final catchup with the progress meter */
+end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
+if (last_cluster + 1 < end) {
+job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE);
+}
+
+return ret;
+}
+
static void coroutine_fn backup_run(void *opaque)
{
BackupBlockJob *job = opaque;
@@ -259,8 +347,7 @@ static void coroutine_fn backup_run(void *opaque)
qemu_co_rwlock_init(&job->flush_rwlock);
start = 0;
-end = DIV_ROUND_UP(job->common.len / BDRV_SECTOR_SIZE,
- BACKUP_SECTORS_PER_CLUSTER);
+end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE)
