On 2014-11-26 at 18:41, John Snow wrote:
From: Fam Zheng <f...@redhat.com>
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.
There are two bitmap use modes for sync=dirty-bitmap:
- reset: backup job makes a copy of bitmap and resets the original
one.
- consume: backup job makes the original anonymous (invisible to user)
and releases it after use.
Signed-off-by: Fam Zheng <f...@redhat.com>
Signed-off-by: John Snow <js...@redhat.com>
---
block.c | 5 ++
block/backup.c | 130 ++++++++++++++++++++++++++++++++++++++--------
block/mirror.c | 4 ++
blockdev.c | 18 ++++++-
hmp.c | 4 +-
include/block/block.h | 1 +
include/block/block_int.h | 6 +++
qapi/block-core.json | 30 +++++++++--
qmp-commands.hx | 7 +--
9 files changed, 175 insertions(+), 30 deletions(-)
[snip]
diff --git a/block/backup.c b/block/backup.c
index 792e655..2aab68f 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -386,6 +438,36 @@ void backup_start(BlockDriverState *bs, BlockDriverState
*target,
return;
}
+ 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;
+ }
+
+ switch (bitmap_mode) {
+ case BITMAP_USE_MODE_RESET:
+ original = sync_bitmap;
+ sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
+ bdrv_reset_dirty_bitmap(bs, original);
+ break;
+ case BITMAP_USE_MODE_CONSUME:
+ bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
+ break;
+ default:
+ error_setg(errp, "Invalid BitmapUseMode (%s) given to
backup_start",
+ BitmapUseMode_lookup[bitmap_mode]);
+ return;
Well, this is basically the opposite of what I'd done, but making it
difficult by formatting a nice error message for the user in places
which should never be reached instead of just aborting is not wrong, in
my opinion (I do know that others have other opinions).
[snip]
diff --git a/blockdev.c b/blockdev.c
index 276a31b..1a56959 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2337,7 +2342,18 @@ void qmp_drive_backup(const char *device, const char
*target,
bdrv_set_aio_context(target_bs, aio_context);
- backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+ if (has_bitmap) {
+ bmap = bdrv_find_dirty_bitmap(bs, bitmap);
+ if (!bmap) {
+ error_setg(errp, "A bitmap name was given, but bitmap '%s' could not be
found",
+ bitmap);
Well, I do know I said that I'd translate "has_bitmap was set" to "A
bitmap name was specified", but I assumed you'd remove the redundancy
yourself. ;-)
Since this line needs to be reworked anyway (see Fam's reply), it can be
(imho) shortened to "Bitmap '%s' could not be found". The user should
know that he/she has specified a bitmap, and that it did have that name;
the only thing he/she doesn't know so far is that qemu failed to find
that bitmap, so that's the information we need to give.
With that line somehow shortened/split to 80 characters (I'd be fine
with keeping its content as-is, I just think it'd sound strange to the
user):
Reviewed-by: Max Reitz <mre...@redhat.com>
