* [Qemu-devel] [PATCH v13 01/17] qapi: Add optional field "name" to block dirty bitmap
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 19:58 ` Eric Blake
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type John Snow
` (16 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
From: Fam Zheng <famz@redhat.com>
This field will be set for user created dirty bitmap. Also pass in an
error pointer to bdrv_create_dirty_bitmap, so when a name is already
taken on this BDS, it can report an error message. This is not global
check, two BDSes can have dirty bitmap with a common name.
Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
be used later when other QMP commands want to reference dirty bitmap by
name.
Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 32 +++++++++++++++++++++++++++++++-
block/mirror.c | 2 +-
include/block/block.h | 7 ++++++-
migration/block.c | 2 +-
qapi/block-core.json | 4 +++-
5 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 210fd5f..c9c2359 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
+ char *name;
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -5366,7 +5367,28 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
return true;
}
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
+{
+ BdrvDirtyBitmap *bm;
+
+ assert(name);
+ QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+ if (bm->name && !strcmp(name, bm->name)) {
+ return bm;
+ }
+ }
+ return NULL;
+}
+
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+ g_free(bitmap->name);
+ bitmap->name = NULL;
+}
+
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+ int granularity,
+ const char *name,
Error **errp)
{
int64_t bitmap_size;
@@ -5374,6 +5396,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
assert((granularity & (granularity - 1)) == 0);
+ if (name && bdrv_find_dirty_bitmap(bs, name)) {
+ error_setg(errp, "Bitmap already exists: %s", name);
+ return NULL;
+ }
granularity >>= BDRV_SECTOR_BITS;
assert(granularity);
bitmap_size = bdrv_nb_sectors(bs);
@@ -5384,6 +5410,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+ bitmap->name = g_strdup(name);
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
return bitmap;
}
@@ -5395,6 +5422,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
if (bm == bitmap) {
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
+ g_free(bitmap->name);
g_free(bitmap);
return;
}
@@ -5413,6 +5441,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
info->count = bdrv_get_dirty_count(bs, bm);
info->granularity =
((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+ info->has_name = !!bm->name;
+ info->name = g_strdup(bm->name);
entry->value = info;
*plist = entry;
plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index 4056164..f073ad7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -703,7 +703,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
s->granularity = granularity;
s->buf_size = MAX(buf_size, granularity);
- s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, errp);
+ s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
if (!s->dirty_bitmap) {
return;
}
diff --git a/include/block/block.h b/include/block/block.h
index 321295e..0988b77 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,8 +438,13 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
struct HBitmapIter;
typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+ int granularity,
+ const char *name,
Error **errp);
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
+ const char *name);
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
diff --git a/migration/block.c b/migration/block.c
index 0c76106..5f9b3e5 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -319,7 +319,7 @@ static int set_dirty_tracking(void)
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
- NULL);
+ NULL, NULL);
if (!bmds->dirty_bitmap) {
ret = -errno;
goto fail;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a3fdaf0..829ba1c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -326,6 +326,8 @@
#
# Block dirty bitmap information.
#
+# @name: #optional the name of the dirty bitmap (Since 2.3)
+#
# @count: number of dirty bytes according to the dirty bitmap
#
# @granularity: granularity of the dirty bitmap in bytes (since 1.4)
@@ -333,7 +335,7 @@
# Since: 1.3
##
{ 'type': 'BlockDirtyInfo',
- 'data': {'count': 'int', 'granularity': 'int'} }
+ 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
##
# @BlockInfo:
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 01/17] qapi: Add optional field "name" to block dirty bitmap
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 01/17] qapi: Add optional field "name" to block dirty bitmap John Snow
@ 2015-02-16 19:58 ` Eric Blake
0 siblings, 0 replies; 41+ messages in thread
From: Eric Blake @ 2015-02-16 19:58 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, mreitz, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]
On 02/13/2015 03:08 PM, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> This field will be set for user created dirty bitmap. Also pass in an
> error pointer to bdrv_create_dirty_bitmap, so when a name is already
> taken on this BDS, it can report an error message. This is not global
> check, two BDSes can have dirty bitmap with a common name.
>
> Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
> be used later when other QMP commands want to reference dirty bitmap by
> name.
>
> Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 32 +++++++++++++++++++++++++++++++-
> block/mirror.c | 2 +-
> include/block/block.h | 7 ++++++-
> migration/block.c | 2 +-
> qapi/block-core.json | 4 +++-
> 5 files changed, 42 insertions(+), 5 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 01/17] qapi: Add optional field "name" to block dirty bitmap John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 19:49 ` Max Reitz
2015-02-16 20:03 ` Eric Blake
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
` (15 subsequent siblings)
17 siblings, 2 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
We treat this field with a variety of different types everywhere
in the code. Now it's just uint32_t.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 11 ++++++-----
block/mirror.c | 4 ++--
include/block/block.h | 2 +-
include/block/block_int.h | 2 +-
qapi/block-core.json | 2 +-
5 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index c9c2359..6e08b26 100644
--- a/block.c
+++ b/block.c
@@ -5387,12 +5387,13 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
}
BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
- int granularity,
+ uint32_t granularity,
const char *name,
Error **errp)
{
int64_t bitmap_size;
BdrvDirtyBitmap *bitmap;
+ uint32_t sector_granularity;
assert((granularity & (granularity - 1)) == 0);
@@ -5400,8 +5401,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
error_setg(errp, "Bitmap already exists: %s", name);
return NULL;
}
- granularity >>= BDRV_SECTOR_BITS;
- assert(granularity);
+ sector_granularity = granularity >> BDRV_SECTOR_BITS;
+ assert(sector_granularity);
bitmap_size = bdrv_nb_sectors(bs);
if (bitmap_size < 0) {
error_setg_errno(errp, -bitmap_size, "could not get length of device");
@@ -5409,7 +5410,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return NULL;
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
- bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+ bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
bitmap->name = g_strdup(name);
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
return bitmap;
@@ -5440,7 +5441,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
info->count = bdrv_get_dirty_count(bs, bm);
info->granularity =
- ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+ ((uint32_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
entry->value = info;
diff --git a/block/mirror.c b/block/mirror.c
index f073ad7..f8aac33 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -656,7 +656,7 @@ static const BlockJobDriver commit_active_job_driver = {
static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
const char *replaces,
- int64_t speed, int64_t granularity,
+ int64_t speed, uint32_t granularity,
int64_t buf_size,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
@@ -717,7 +717,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
void mirror_start(BlockDriverState *bs, BlockDriverState *target,
const char *replaces,
- int64_t speed, int64_t granularity, int64_t buf_size,
+ int64_t speed, uint32_t granularity, int64_t buf_size,
MirrorSyncMode mode, BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb,
diff --git a/include/block/block.h b/include/block/block.h
index 0988b77..abdf8d6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -439,7 +439,7 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
struct HBitmapIter;
typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
- int granularity,
+ uint32_t granularity,
const char *name,
Error **errp);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7ad1950..be99ec0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -576,7 +576,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
*/
void mirror_start(BlockDriverState *bs, BlockDriverState *target,
const char *replaces,
- int64_t speed, int64_t granularity, int64_t buf_size,
+ int64_t speed, uint32_t granularity, int64_t buf_size,
MirrorSyncMode mode, BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 829ba1c..39291e7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -335,7 +335,7 @@
# Since: 1.3
##
{ 'type': 'BlockDirtyInfo',
- 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
+ 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} }
##
# @BlockInfo:
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type John Snow
@ 2015-02-16 19:49 ` Max Reitz
2015-02-16 19:51 ` John Snow
2015-02-16 20:03 ` Eric Blake
1 sibling, 1 reply; 41+ messages in thread
From: Max Reitz @ 2015-02-16 19:49 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, John Snow wrote:
> We treat this field with a variety of different types everywhere
> in the code. Now it's just uint32_t.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 11 ++++++-----
> block/mirror.c | 4 ++--
> include/block/block.h | 2 +-
> include/block/block_int.h | 2 +-
> qapi/block-core.json | 2 +-
> 5 files changed, 11 insertions(+), 10 deletions(-)
Considering you called this series the "$MY_NAME Fixup issue", I'm
assuming this patch is the response to something I said, maybe that
Coverity might complain about one right-shift of a 64 bit value with the
result stored in a 32 bit integer. If so, that would have been
preventable by assert((x >> 9) <= INT_MAX), so it's not that you would
have to force everything to be 32 bits wide.
Anyway, I'm fine with either 32 or 64 bits (2 GB maximum granularity
should be good for now):
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type
2015-02-16 19:49 ` Max Reitz
@ 2015-02-16 19:51 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-16 19:51 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel
On 02/16/2015 02:49 PM, Max Reitz wrote:
> On 2015-02-13 at 17:08, John Snow wrote:
>> We treat this field with a variety of different types everywhere
>> in the code. Now it's just uint32_t.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block.c | 11 ++++++-----
>> block/mirror.c | 4 ++--
>> include/block/block.h | 2 +-
>> include/block/block_int.h | 2 +-
>> qapi/block-core.json | 2 +-
>> 5 files changed, 11 insertions(+), 10 deletions(-)
>
> Considering you called this series the "$MY_NAME Fixup issue", I'm
> assuming this patch is the response to something I said, maybe that
> Coverity might complain about one right-shift of a 64 bit value with the
> result stored in a 32 bit integer. If so, that would have been
> preventable by assert((x >> 9) <= INT_MAX), so it's not that you would
> have to force everything to be 32 bits wide.
>
> Anyway, I'm fine with either 32 or 64 bits (2 GB maximum granularity
> should be good for now):
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
You mentioned the types weren't /actually/ consistent, and that did
actually bother me, so I did try to go out of my way to correct it.
I chose uint32_t to match the existing interface for granularity via the
drive mirror command, and just whackamole'd the types until they
flattened out.
Doing it any other way would involve changing other interface options
elsewhere, and a 2GiB granularity should seriously be sufficient,
considering we currently generally limit it to under 64KiB :)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type John Snow
2015-02-16 19:49 ` Max Reitz
@ 2015-02-16 20:03 ` Eric Blake
1 sibling, 0 replies; 41+ messages in thread
From: Eric Blake @ 2015-02-16 20:03 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, mreitz, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]
On 02/13/2015 03:08 PM, John Snow wrote:
> We treat this field with a variety of different types everywhere
> in the code. Now it's just uint32_t.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 11 ++++++-----
> block/mirror.c | 4 ++--
> include/block/block.h | 2 +-
> include/block/block_int.h | 2 +-
> qapi/block-core.json | 2 +-
> 5 files changed, 11 insertions(+), 10 deletions(-)
>
> +++ b/qapi/block-core.json
> @@ -335,7 +335,7 @@
> # Since: 1.3
> ##
> { 'type': 'BlockDirtyInfo',
> - 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
> + 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} }
The wire format of QMP is unchanged. Prior to this patch, a user could
pass a value larger than 4G and get past initial validation, but as
other code then capped the value (for example, drive-mirror caps at a
maximum of 64M), we aren't changing working semantics. Narrowing a type
is not always backwards-compatible, but in this case it looks fine. So:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 01/17] qapi: Add optional field "name" to block dirty bitmap John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 02/17] qmp: Ensure consistent granularity type John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 19:53 ` Max Reitz
2015-02-16 20:22 ` Eric Blake
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 04/17] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
` (14 subsequent siblings)
17 siblings, 2 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
The new command pair is added to manage user created dirty bitmap. The
dirty bitmap's name is mandatory and must be unique for the same device,
but different devices can have bitmaps with the same names.
The granularity is an optional field. If it is not specified, we will
choose a default granularity based on the cluster size if available,
clamped to between 4K and 64K to mirror how the 'mirror' code was
already choosing granularity. If we do not have cluster size info
available, we choose 64K. This code has been factored out into a helper
shared with block/mirror.
This patch also introduces the 'block_dirty_bitmap_lookup' helper,
which takes a device name and a dirty bitmap name and validates the
lookup, returning NULL and setting errp if there is a problem with
either field. This helper will be re-used in future patches in this
series.
The types added to block-core.json will be re-used in future patches
in this series, see:
'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 20 ++++++++++
block/mirror.c | 10 +----
blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 +
qapi/block-core.json | 55 +++++++++++++++++++++++++++
qmp-commands.hx | 51 +++++++++++++++++++++++++
6 files changed, 228 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 6e08b26..60dbdbd 100644
--- a/block.c
+++ b/block.c
@@ -5461,6 +5461,26 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
}
}
+/**
+ * Chooses a default granularity based on the existing cluster size,
+ * but clamped between [4K, 64K]. Defaults to 64K in the case that there
+ * is no cluster size information available.
+ */
+uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
+{
+ BlockDriverInfo bdi;
+ uint32_t granularity;
+
+ if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
+ granularity = MAX(4096, bdi.cluster_size);
+ granularity = MIN(65536, granularity);
+ } else {
+ granularity = 65536;
+ }
+
+ return granularity;
+}
+
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
{
diff --git a/block/mirror.c b/block/mirror.c
index f8aac33..1cb700e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -668,15 +668,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
MirrorBlockJob *s;
if (granularity == 0) {
- /* Choose the default granularity based on the target file's cluster
- * size, clamped between 4k and 64k. */
- BlockDriverInfo bdi;
- if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) {
- granularity = MAX(4096, bdi.cluster_size);
- granularity = MIN(65536, granularity);
- } else {
- granularity = 65536;
- }
+ granularity = bdrv_get_default_bitmap_granularity(target);
}
assert ((granularity & (granularity - 1)) == 0);
diff --git a/blockdev.c b/blockdev.c
index 7d34960..8842e39 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1173,6 +1173,48 @@ out_aio_context:
return NULL;
}
+/**
+ * Return a dirty bitmap (if present), after validating
+ * the node reference and bitmap names. Returns NULL on error,
+ * including when the BDS and/or bitmap is not found.
+ */
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+ const char *name,
+ BlockDriverState **pbs,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ if (!node) {
+ error_setg(errp, "Node cannot be NULL");
+ return NULL;
+ }
+ if (!name) {
+ error_setg(errp, "Bitmap name cannot be NULL");
+ return NULL;
+ }
+
+ bs = bdrv_lookup_bs(node, node, NULL);
+ if (!bs) {
+ error_setg(errp, "Node '%s' not found", node);
+ return NULL;
+ }
+
+ /* If caller provided a BDS*, provide the result of that lookup, too. */
+ if (pbs) {
+ *pbs = bs;
+ }
+
+ bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Dirty bitmap '%s' not found", name);
+ return NULL;
+ }
+
+ return bitmap;
+}
+
/* New and old BlockDriverState structs for atomic group operations */
typedef struct BlkTransactionState BlkTransactionState;
@@ -1953,6 +1995,64 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
aio_context_release(aio_context);
}
+void qmp_block_dirty_bitmap_add(const char *node, const char *name,
+ bool has_granularity, uint32_t granularity,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BlockDriverState *bs;
+
+ if (!name || name[0] == '\0') {
+ error_setg(errp, "Bitmap name cannot be empty");
+ return;
+ }
+
+ bs = bdrv_lookup_bs(node, node, errp);
+ if (!bs) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (has_granularity) {
+ if (granularity < 512 || !is_power_of_2(granularity)) {
+ error_setg(errp, "Granularity must be power of 2 "
+ "and at least 512");
+ goto out;
+ }
+ } else {
+ /* Default to cluster size, if available: */
+ granularity = bdrv_get_default_bitmap_granularity(bs);
+ }
+
+ bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+
+ out:
+ aio_context_release(aio_context);
+}
+
+void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap || !bs) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ bdrv_dirty_bitmap_make_anon(bs, bitmap);
+ bdrv_release_dirty_bitmap(bs, bitmap);
+
+ aio_context_release(aio_context);
+}
+
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/block.h b/include/block/block.h
index abdf8d6..3589132 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -447,6 +447,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
+uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 39291e7..764719d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -959,6 +959,61 @@
'*on-target-error': 'BlockdevOnError' } }
##
+# @BlockDirtyBitmap
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmap',
+ 'data': { 'node': 'str', 'name': 'str' } }
+
+##
+# @BlockDirtyBitmapAdd
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# @granularity: #optional the bitmap granularity, default is 64k for
+# block-dirty-bitmap-add
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmapAdd',
+ 'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+
+##
+# @block-dirty-bitmap-add
+#
+# Create a dirty bitmap with a name on the node
+#
+# Returns: nothing on success
+# If @node is not a valid block device or node, DeviceNotFound
+# If @name is already taken, GenericError with an explanation
+#
+# Since 2.3
+##
+{ 'command': 'block-dirty-bitmap-add',
+ 'data': 'BlockDirtyBitmapAdd' }
+
+##
+# @block-dirty-bitmap-remove
+#
+# Remove a dirty bitmap on the node
+#
+# Returns: nothing on success
+# If @node is not a valid block device or node, DeviceNotFound
+# If @name is not found, GenericError with an explanation
+#
+# Since 2.3
+##
+{ 'command': 'block-dirty-bitmap-remove',
+ 'data': 'BlockDirtyBitmap' }
+
+##
# @block_set_io_throttle:
#
# Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a85d847..881a810 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1244,6 +1244,57 @@ Example:
EQMP
{
+ .name = "block-dirty-bitmap-add",
+ .args_type = "node:B,name:s,granularity:i?",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
+ },
+ {
+ .name = "block-dirty-bitmap-remove",
+ .args_type = "node:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
+ },
+
+SQMP
+
+block-dirty-bitmap-add
+----------------------
+Since 2.3
+
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+
+Arguments:
+
+- "node": device/node on which to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with (int, optional)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-add", "arguments": { "node": "drive0",
+ "name": "bitmap0" } }
+<- { "return": {} }
+
+block-dirty-bitmap-remove
+-------------------------
+Since 2.3
+
+Stop write tracking and remove the dirty bitmap that was created with
+block-dirty-bitmap-add.
+
+Arguments:
+
+- "node": device/node on which to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node": "drive0",
+ "name": "bitmap0" } }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "blockdev-snapshot-sync",
.args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
@ 2015-02-16 19:53 ` Max Reitz
2015-02-16 20:22 ` Eric Blake
1 sibling, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-02-16 19:53 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, John Snow wrote:
> The new command pair is added to manage user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
>
> The granularity is an optional field. If it is not specified, we will
> choose a default granularity based on the cluster size if available,
> clamped to between 4K and 64K to mirror how the 'mirror' code was
> already choosing granularity. If we do not have cluster size info
> available, we choose 64K. This code has been factored out into a helper
> shared with block/mirror.
>
> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
> which takes a device name and a dirty bitmap name and validates the
> lookup, returning NULL and setting errp if there is a problem with
> either field. This helper will be re-used in future patches in this
> series.
>
> The types added to block-core.json will be re-used in future patches
> in this series, see:
> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 20 ++++++++++
> block/mirror.c | 10 +----
> blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 1 +
> qapi/block-core.json | 55 +++++++++++++++++++++++++++
> qmp-commands.hx | 51 +++++++++++++++++++++++++
> 6 files changed, 228 insertions(+), 9 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
2015-02-16 19:53 ` Max Reitz
@ 2015-02-16 20:22 ` Eric Blake
2015-02-16 20:31 ` John Snow
1 sibling, 1 reply; 41+ messages in thread
From: Eric Blake @ 2015-02-16 20:22 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, mreitz, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]
On 02/13/2015 03:08 PM, John Snow wrote:
> The new command pair is added to manage user created dirty bitmap. The
s/manage/manage a/
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
>
> The granularity is an optional field. If it is not specified, we will
> choose a default granularity based on the cluster size if available,
> clamped to between 4K and 64K to mirror how the 'mirror' code was
> already choosing granularity. If we do not have cluster size info
> available, we choose 64K. This code has been factored out into a helper
> shared with block/mirror.
The drive-mirror command in block-core.json documents that it supports a
granularity between 512 and 64M, not 64k. Is there a bug here? See
more below.
>
> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
> which takes a device name and a dirty bitmap name and validates the
> lookup, returning NULL and setting errp if there is a problem with
> either field. This helper will be re-used in future patches in this
> series.
>
> The types added to block-core.json will be re-used in future patches
> in this series, see:
> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 20 ++++++++++
> block/mirror.c | 10 +----
> blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 1 +
> qapi/block-core.json | 55 +++++++++++++++++++++++++++
> qmp-commands.hx | 51 +++++++++++++++++++++++++
> 6 files changed, 228 insertions(+), 9 deletions(-)
>
> +
> + if (has_granularity) {
> + if (granularity < 512 || !is_power_of_2(granularity)) {
> + error_setg(errp, "Granularity must be power of 2 "
> + "and at least 512");
> + goto out;
> + }
> + } else {
> + /* Default to cluster size, if available: */
> + granularity = bdrv_get_default_bitmap_granularity(bs);
> + }
This enforces a minimum granularity, but should we also be enforcing a
maximum? That is, if the user does not provide granularity, we default
between 4k and 64k based on cluster size, but if the user DOES provide
granularity, we allow as small as 512, but as large as the user can
pass. Should we enforce a maximum of 64M?
--
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 --]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2015-02-16 20:22 ` Eric Blake
@ 2015-02-16 20:31 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-16 20:31 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: kwolf, famz, armbru, mreitz, vsementsov, stefanha
On 02/16/2015 03:22 PM, Eric Blake wrote:
> On 02/13/2015 03:08 PM, John Snow wrote:
>> The new command pair is added to manage user created dirty bitmap. The
>
> s/manage/manage a/
>
>> dirty bitmap's name is mandatory and must be unique for the same device,
>> but different devices can have bitmaps with the same names.
>>
>> The granularity is an optional field. If it is not specified, we will
>> choose a default granularity based on the cluster size if available,
>> clamped to between 4K and 64K to mirror how the 'mirror' code was
>> already choosing granularity. If we do not have cluster size info
>> available, we choose 64K. This code has been factored out into a helper
>> shared with block/mirror.
>
> The drive-mirror command in block-core.json documents that it supports a
> granularity between 512 and 64M, not 64k. Is there a bug here? See
> more below.
qmp_drive_mirror DOES clamp the user-specified value to [512, 64M]. It
does also allow '0', which gets passed through to mirror_start_job,
which chooses a default based on the cluster_size, clamped to [512, 64K].
>
>>
>> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
>> which takes a device name and a dirty bitmap name and validates the
>> lookup, returning NULL and setting errp if there is a problem with
>> either field. This helper will be re-used in future patches in this
>> series.
>>
>> The types added to block-core.json will be re-used in future patches
>> in this series, see:
>> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block.c | 20 ++++++++++
>> block/mirror.c | 10 +----
>> blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/block/block.h | 1 +
>> qapi/block-core.json | 55 +++++++++++++++++++++++++++
>> qmp-commands.hx | 51 +++++++++++++++++++++++++
>> 6 files changed, 228 insertions(+), 9 deletions(-)
>>
>
>> +
>> + if (has_granularity) {
>> + if (granularity < 512 || !is_power_of_2(granularity)) {
>> + error_setg(errp, "Granularity must be power of 2 "
>> + "and at least 512");
>> + goto out;
>> + }
>> + } else {
>> + /* Default to cluster size, if available: */
>> + granularity = bdrv_get_default_bitmap_granularity(bs);
>> + }
>
> This enforces a minimum granularity, but should we also be enforcing a
> maximum? That is, if the user does not provide granularity, we default
> between 4k and 64k based on cluster size, but if the user DOES provide
> granularity, we allow as small as 512, but as large as the user can
> pass. Should we enforce a maximum of 64M?
>
>
Not a bug as such, though not a purposeful decision on my part either.
I don't have a concrete reason to prohibit what kinds of granularities
the user can supply, but when it came to picking a default I decided to
imitate what mirror was already doing, which I explain above.
The existing code is basically: "Try to pick a sane default if the user
didn't give us one, but allow the user to choose something bananas if
they want to. Maybe they know better than we do."
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 04/17] block: Introduce bdrv_dirty_bitmap_granularity()
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (2 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 19:57 ` Max Reitz
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 05/17] hbitmap: add hbitmap_merge John Snow
` (13 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
This returns the granularity (in bytes) of dirty bitmap,
which matches the QMP interface and the existing query
interface.
Small adjustments are made to ensure that granularity (in bytes)
is handled consistently as a uint32_t throughout the code.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 8 ++++++--
include/block/block.h | 1 +
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 60dbdbd..8b40e38 100644
--- a/block.c
+++ b/block.c
@@ -5440,8 +5440,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
info->count = bdrv_get_dirty_count(bs, bm);
- info->granularity =
- ((uint32_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+ info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
entry->value = info;
@@ -5481,6 +5480,11 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
return granularity;
}
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+{
+ return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+}
+
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
{
diff --git a/include/block/block.h b/include/block/block.h
index 3589132..41b8418 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -448,6 +448,7 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 04/17] block: Introduce bdrv_dirty_bitmap_granularity()
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 04/17] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
@ 2015-02-16 19:57 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-02-16 19:57 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, John Snow wrote:
> This returns the granularity (in bytes) of dirty bitmap,
> which matches the QMP interface and the existing query
> interface.
>
> Small adjustments are made to ensure that granularity (in bytes)
> is handled consistently as a uint32_t throughout the code.
This part of the patch seems to have been replaced by patch 2.
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 8 ++++++--
> include/block/block.h | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
If you remove said paragraph (or if the respective maintainer does), or
if you have good reason to keep it:
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 05/17] hbitmap: add hbitmap_merge
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (3 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 04/17] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-20 9:34 ` Stefan Hajnoczi
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 06/17] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable John Snow
` (12 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
We add a bitmap merge operation to assist in error cases
where we wish to combine two bitmaps together.
This is algorithmically O(bits) provided HBITMAP_LEVELS remains
constant. For a full bitmap on a 64bit machine:
sum(bits/64^k, k, 0, HBITMAP_LEVELS) ~= 1.01587 * bits
We may be able to improve running speed for particularly sparse
bitmaps by using iterators, but the running time for dense maps
will be worse.
We present the simpler solution first, and we can refine it later
if needed.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/qemu/hbitmap.h | 11 +++++++++++
util/hbitmap.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 550d7ce..c19c1cb 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,17 @@ struct HBitmapIter {
HBitmap *hbitmap_alloc(uint64_t size, int granularity);
/**
+ * hbitmap_merge:
+ * @a: The bitmap to store the result in.
+ * @b: The bitmap to merge into @a.
+ *
+ * Merge two bitmaps together.
+ * A := A (BITOR) B.
+ * B is left unmodified.
+ */
+bool hbitmap_merge(HBitmap *a, const HBitmap *b);
+
+/**
* hbitmap_empty:
* @hb: HBitmap to operate on.
*
diff --git a/util/hbitmap.c b/util/hbitmap.c
index ab13971..962ff29 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -395,3 +395,35 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
return hb;
}
+
+/**
+ * Given HBitmaps A and B, let A := A (BITOR) B.
+ * Bitmap B will not be modified.
+ */
+bool hbitmap_merge(HBitmap *a, const HBitmap *b)
+{
+ int i, j;
+ uint64_t size;
+
+ if ((a->size != b->size) || (a->granularity != b->granularity)) {
+ return false;
+ }
+
+ if (hbitmap_count(b) == 0) {
+ return true;
+ }
+
+ /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
+ * It may be possible to improve running times for sparsely populated maps
+ * by using hbitmap_iter_next, but this is suboptimal for dense maps.
+ */
+ size = a->size;
+ for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
+ size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+ for (j = 0; j < size; j++) {
+ a->levels[i][j] |= b->levels[i][j];
+ }
+ }
+
+ return true;
+}
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 05/17] hbitmap: add hbitmap_merge
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 05/17] hbitmap: add hbitmap_merge John Snow
@ 2015-02-20 9:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-02-20 9:34 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, armbru, qemu-devel, vsementsov, mreitz
[-- Attachment #1: Type: text/plain, Size: 898 bytes --]
On Fri, Feb 13, 2015 at 05:08:46PM -0500, John Snow wrote:
> We add a bitmap merge operation to assist in error cases
> where we wish to combine two bitmaps together.
>
> This is algorithmically O(bits) provided HBITMAP_LEVELS remains
> constant. For a full bitmap on a 64bit machine:
> sum(bits/64^k, k, 0, HBITMAP_LEVELS) ~= 1.01587 * bits
>
> We may be able to improve running speed for particularly sparse
> bitmaps by using iterators, but the running time for dense maps
> will be worse.
>
> We present the simpler solution first, and we can refine it later
> if needed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> include/qemu/hbitmap.h | 11 +++++++++++
> util/hbitmap.c | 32 ++++++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 06/17] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (4 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 05/17] hbitmap: add hbitmap_merge John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 07/17] block: Add bitmap successors John Snow
` (11 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
This allows to put the dirty bitmap into a disabled state where it is
read only. A disabled bitmap will ignore any attempts to set or reset
any of its bits, but can otherwise be renamed, deleted, or re-enabled.
It will be used before backup or writing to persistent file.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 25 +++++++++++++++++++++++++
blockdev.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 3 +++
qapi/block-core.json | 28 ++++++++++++++++++++++++++++
qmp-commands.hx | 10 ++++++++++
5 files changed, 106 insertions(+)
diff --git a/block.c b/block.c
index 8b40e38..1fa2ed5 100644
--- a/block.c
+++ b/block.c
@@ -54,6 +54,7 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
char *name;
+ bool disabled;
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -5412,10 +5413,16 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
bitmap->name = g_strdup(name);
+ bitmap->disabled = false;
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
return bitmap;
}
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
+{
+ return !bitmap->disabled;
+}
+
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
BdrvDirtyBitmap *bm, *next;
@@ -5430,6 +5437,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
}
}
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ bitmap->disabled = true;
+}
+
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ bitmap->disabled = false;
+}
+
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
{
BdrvDirtyBitmap *bm;
@@ -5494,12 +5511,14 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
@@ -5508,6 +5527,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
{
BdrvDirtyBitmap *bitmap;
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ continue;
+ }
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
}
@@ -5517,6 +5539,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
{
BdrvDirtyBitmap *bitmap;
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ continue;
+ }
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
}
diff --git a/blockdev.c b/blockdev.c
index 8842e39..d3599ac 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2053,6 +2053,46 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
aio_context_release(aio_context);
}
+void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ bdrv_enable_dirty_bitmap(bitmap);
+
+ aio_context_release(aio_context);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ bdrv_disable_dirty_bitmap(bitmap);
+
+ aio_context_release(aio_context);
+}
+
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/block.h b/include/block/block.h
index 41b8418..ae7e2ab 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -446,9 +446,12 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 764719d..a82f8fa 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1014,6 +1014,34 @@
'data': 'BlockDirtyBitmap' }
##
+# @block-dirty-bitmap-enable
+#
+# Enable a dirty bitmap on the device
+#
+# Returns: nothing on success
+# If @node is not a valid block device, DeviceNotFound
+# If @name is not found, GenericError with an explanation
+#
+# Since 2.3
+##
+{ 'command': 'block-dirty-bitmap-enable',
+ 'data': 'BlockDirtyBitmap' }
+
+##
+# @block-dirty-bitmap-disable
+#
+# Disable a dirty bitmap on the device
+#
+# Returns: nothing on success
+# If @node is not a valid block device, DeviceNotFound
+# If @name is not found, GenericError with an explanation
+#
+# Since 2.3
+##
+{ 'command': 'block-dirty-bitmap-disable',
+ 'data': 'BlockDirtyBitmap' }
+
+##
# @block_set_io_throttle:
#
# Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 881a810..ce7782f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1253,6 +1253,16 @@ EQMP
.args_type = "node:B,name:s",
.mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
},
+ {
+ .name = "block-dirty-bitmap-enable",
+ .args_type = "node:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_enable,
+ },
+ {
+ .name = "block-dirty-bitmap-disable",
+ .args_type = "node:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_disable,
+ },
SQMP
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v13 07/17] block: Add bitmap successors
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (5 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 06/17] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 08/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
` (10 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
be created just prior to a sensitive operation (e.g. Incremental Backup)
that can either succeed or fail, but during the course of which we still
want a bitmap tracking writes.
On creating a successor, we "freeze" the parent bitmap which prevents
its deletion, enabling, anonymization, or creating a bitmap with the
same name.
On success, the parent bitmap can "abdicate" responsibility to the
successor, which will inherit its name. The successor will have been
tracking writes during the course of the backup operation. The parent
will be safely deleted.
On failure, we can "reclaim" the successor from the parent, unifying
them such that the resulting bitmap describes all writes occurring since
the last successful backup, for instance. Reclamation will thaw the
parent, but not explicitly re-enable it.
BdrvDirtyBitmap operations that target a single bitmap are protected
by assertions that the bitmap is not frozen and/or disabled.
BdrvDirtyBitmap operations that target a group of bitmaps, such as
bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a
conditional instead.
QMP transactions that enable/disable bitmaps have extra error checking
surrounding them that prevent modifying bitmaps that are frozen.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++-
blockdev.c | 22 +++++++++++
include/block/block.h | 10 +++++
qapi/block-core.json | 3 ++
4 files changed, 138 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 1fa2ed5..665ae69 100644
--- a/block.c
+++ b/block.c
@@ -51,8 +51,17 @@
#include <windows.h>
#endif
+/**
+ * A BdrvDirtyBitmap can be in three possible states:
+ * (1) successor is false and disabled is false: full r/w mode
+ * (2) successor is false and disabled is true: read only mode ("disabled")
+ * (3) successor is set: frozen mode.
+ * A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
+ * or enabled. A frozen bitmap can only abdicate() or reclaim().
+ */
struct BdrvDirtyBitmap {
HBitmap *bitmap;
+ BdrvDirtyBitmap *successor;
char *name;
bool disabled;
QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -5383,6 +5392,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
g_free(bitmap->name);
bitmap->name = NULL;
}
@@ -5418,9 +5428,98 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return bitmap;
}
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+{
+ return bitmap->successor;
+}
+
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
{
- return !bitmap->disabled;
+ return !(bitmap->disabled || bitmap->successor);
+}
+
+/**
+ * Create a successor bitmap destined to replace this bitmap after an operation.
+ * Requires that the bitmap is not frozen and has no successor.
+ */
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, Error **errp)
+{
+ uint64_t granularity;
+ BdrvDirtyBitmap *child;
+
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp, "Cannot create a successor for a bitmap that is "
+ "currently frozen");
+ return -1;
+ }
+ assert(!bitmap->successor);
+
+ /* Create an anonymous successor */
+ granularity = bdrv_dirty_bitmap_granularity(bitmap);
+ child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+ if (!child) {
+ return -1;
+ }
+
+ /* Successor will be on or off based on our current state. */
+ child->disabled = bitmap->disabled;
+
+ /* Install the successor and freeze the parent */
+ bitmap->successor = child;
+ return 0;
+}
+
+/**
+ * For a bitmap with a successor, yield our name to the successor,
+ * Delete the old bitmap, and return a handle to the new bitmap.
+ */
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp)
+{
+ char *name;
+ BdrvDirtyBitmap *successor = bitmap->successor;
+
+ if (successor == NULL) {
+ error_setg(errp, "Cannot relinquish control if "
+ "there's no successor present");
+ return NULL;
+ }
+
+ name = bitmap->name;
+ bitmap->name = NULL;
+ successor->name = name;
+ bitmap->successor = NULL;
+ bdrv_release_dirty_bitmap(bs, bitmap);
+
+ return successor;
+}
+
+/**
+ * In cases of failure where we can no longer safely delete the parent,
+ * We may wish to re-join the parent and child/successor.
+ * The merged parent will be un-frozen, but not explicitly re-enabled.
+ */
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ Error **errp)
+{
+ BdrvDirtyBitmap *successor = parent->successor;
+
+ if (!successor) {
+ error_setg(errp, "Cannot reclaim a successor when none is present");
+ return NULL;
+ }
+
+ if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
+ error_setg(errp, "Merging of parent and successor bitmap failed");
+ return NULL;
+ }
+ bdrv_release_dirty_bitmap(bs, successor);
+ parent->successor = NULL;
+
+ return parent;
}
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
@@ -5428,6 +5527,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
BdrvDirtyBitmap *bm, *next;
QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
if (bm == bitmap) {
+ assert(!bdrv_dirty_bitmap_frozen(bm));
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
g_free(bitmap->name);
@@ -5439,11 +5539,13 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
bitmap->disabled = true;
}
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
bitmap->disabled = false;
}
diff --git a/blockdev.c b/blockdev.c
index d3599ac..9ecba7d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2047,9 +2047,16 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently frozen and cannot be removed",
+ name);
+ goto out;
+ }
bdrv_dirty_bitmap_make_anon(bs, bitmap);
bdrv_release_dirty_bitmap(bs, bitmap);
+ out:
aio_context_release(aio_context);
}
@@ -2068,8 +2075,16 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently frozen and cannot be modified",
+ name);
+ goto out;
+ }
+
bdrv_enable_dirty_bitmap(bitmap);
+ out:
aio_context_release(aio_context);
}
@@ -2088,8 +2103,15 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently frozen and cannot be modified",
+ name);
+ goto out;
+ }
bdrv_disable_dirty_bitmap(bitmap);
+ out:
aio_context_release(aio_context);
}
diff --git a/include/block/block.h b/include/block/block.h
index ae7e2ab..60e8b6d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -442,6 +442,15 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
uint32_t granularity,
const char *name,
Error **errp);
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
@@ -452,6 +461,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a82f8fa..f00e3bb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1007,6 +1007,7 @@
# Returns: nothing on success
# If @node is not a valid block device or node, DeviceNotFound
# If @name is not found, GenericError with an explanation
+# if @name is frozen by an operation, GenericError
#
# Since 2.3
##
@@ -1021,6 +1022,7 @@
# Returns: nothing on success
# If @node is not a valid block device, DeviceNotFound
# If @name is not found, GenericError with an explanation
+# if @name is frozen by an operation, GenericError
#
# Since 2.3
##
@@ -1035,6 +1037,7 @@
# Returns: nothing on success
# If @node is not a valid block device, DeviceNotFound
# If @name is not found, GenericError with an explanation
+# if @name is frozen by an operation, GenericError
#
# Since 2.3
##
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v13 08/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (6 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 07/17] block: Add bitmap successors John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 20:04 ` Max Reitz
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 09/17] qmp: add block-dirty-bitmap-clear John Snow
` (9 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
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 <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 9 +++
block/backup.c | 149 +++++++++++++++++++++++++++++++++++++++-------
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, 172 insertions(+), 34 deletions(-)
diff --git a/block.c b/block.c
index 665ae69..6e3f817 100644
--- a/block.c
+++ b/block.c
@@ -5648,6 +5648,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..d46c0a3 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,31 @@ 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 void coroutine_fn backup_run(void *opaque)
{
BackupBlockJob *job = opaque;
@@ -254,13 +281,13 @@ static void coroutine_fn backup_run(void *opaque)
};
int64_t start, end;
int ret = 0;
+ bool error_is_read;
QLIST_INIT(&job->inflight_reqs);
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);
job->bitmap = hbitmap_alloc(end, 0);
@@ -278,28 +305,62 @@ static void coroutine_fn backup_run(void *opaque)
qemu_coroutine_yield();
job->common.busy = true;
}
+ } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+ /* Dirty Bitmap sync has a slightly different iteration method */
+ HBitmapIter hbi;
+ int64_t sector;
+ int64_t cluster;
+ int64_t last_cluster = -1;
+ bool polyrhythmic;
+
+ bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+ /* Does the granularity happen to match our backup cluster size? */
+ polyrhythmic = (bdrv_dirty_bitmap_granularity(job->sync_bitmap) !=
+ BACKUP_CLUSTER_SIZE);
+
+ /* Find the next dirty /sector/ and copy that /cluster/ */
+ while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+ cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+ /* Fake progress updates for any clusters we skipped,
+ * excluding this current cluster. */
+ if (cluster != last_cluster + 1) {
+ job->common.offset += ((cluster - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+ }
+
+ if (yield_and_check(job)) {
+ goto leave;
+ }
+
+ 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) {
+ goto leave;
+ }
+ } while (ret < 0);
+
+ /* Advance (or rewind) our iterator if we need to. */
+ if (polyrhythmic) {
+ bdrv_set_dirty_iter(&hbi,
+ (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
+ }
+
+ last_cluster = cluster;
+ }
+
+ /* Play some final catchup with the progress meter */
+ if (last_cluster + 1 < end) {
+ job->common.offset += ((end - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+ }
} else {
/* Both FULL and TOP SYNC_MODE's require copying.. */
for (; start < end; start++) {
- bool error_is_read;
-
- if (block_job_is_cancelled(&job->common)) {
- break;
- }
-
- /* 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)) {
+ if (yield_and_check(job)) {
break;
}
@@ -351,12 +412,26 @@ static void coroutine_fn backup_run(void *opaque)
}
}
+leave:
notifier_with_return_remove(&before_write);
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&job->flush_rwlock);
qemu_co_rwlock_unlock(&job->flush_rwlock);
+ if (job->sync_bitmap) {
+ BdrvDirtyBitmap *bm;
+ if (ret < 0) {
+ /* Merge the successor back into the parent, delete nothing. */
+ bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
+ assert(bm);
+ bdrv_enable_dirty_bitmap(job->sync_bitmap);
+ } else {
+ /* Everything is fine, delete this bitmap and install the backup. */
+ bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
+ assert(bm);
+ }
+ }
hbitmap_free(job->bitmap);
bdrv_iostatus_disable(target);
@@ -369,6 +444,7 @@ static void coroutine_fn backup_run(void *opaque)
void backup_start(BlockDriverState *bs, BlockDriverState *target,
int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
@@ -412,17 +488,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;
+ }
+
+ /* Create a new bitmap, and freeze/disable this one. */
+ if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+ 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;
+ }
+
len = bdrv_getlength(bs);
if (len < 0) {
error_setg_errno(errp, -len, "unable to get length for '%s'",
bdrv_get_device_name(bs));
- return;
+ goto error;
}
BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
cb, opaque, errp);
if (!job) {
- return;
+ goto error;
}
bdrv_op_block_all(target, job->common.blocker);
@@ -431,7 +526,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
job->on_target_error = on_target_error;
job->target = target;
job->sync_mode = sync_mode;
+ job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
+ sync_bitmap : NULL;
job->common.len = len;
job->common.co = qemu_coroutine_create(backup_run);
qemu_coroutine_enter(job->common.co, job);
+ return;
+
+ error:
+ if (sync_bitmap) {
+ bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+ }
}
diff --git a/block/mirror.c b/block/mirror.c
index 1cb700e..f89eccf 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -718,6 +718,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
bool is_none_mode;
BlockDriverState *base;
+ if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+ error_setg(errp, "Sync mode 'dirty-bitmap' not supported");
+ return;
+ }
is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
mirror_start_job(bs, target, replaces,
diff --git a/blockdev.c b/blockdev.c
index 9ecba7d..6990986 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1569,6 +1569,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
backup->sync,
backup->has_mode, backup->mode,
backup->has_speed, backup->speed,
+ backup->has_bitmap, backup->bitmap,
backup->has_on_source_error, backup->on_source_error,
backup->has_on_target_error, backup->on_target_error,
&local_err);
@@ -2429,6 +2430,7 @@ void qmp_drive_backup(const char *device, const char *target,
enum MirrorSyncMode sync,
bool has_mode, enum NewImageMode mode,
bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
bool has_on_source_error, BlockdevOnError on_source_error,
bool has_on_target_error, BlockdevOnError on_target_error,
Error **errp)
@@ -2436,6 +2438,7 @@ void qmp_drive_backup(const char *device, const char *target,
BlockDriverState *bs;
BlockDriverState *target_bs;
BlockDriverState *source = NULL;
+ BdrvDirtyBitmap *bmap = NULL;
AioContext *aio_context;
BlockDriver *drv = NULL;
Error *local_err = NULL;
@@ -2534,7 +2537,16 @@ 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, "Bitmap '%s' could not be found", bitmap);
+ goto out;
+ }
+ }
+
+ backup_start(bs, target_bs, speed, sync, bmap,
+ on_source_error, on_target_error,
block_job_cb, bs, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
@@ -2592,8 +2604,8 @@ void qmp_blockdev_backup(const char *device, const char *target,
bdrv_ref(target_bs);
bdrv_set_aio_context(target_bs, aio_context);
- backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
- block_job_cb, bs, &local_err);
+ backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
+ on_target_error, block_job_cb, bs, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
error_propagate(errp, local_err);
diff --git a/hmp.c b/hmp.c
index b47f331..015499f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1027,7 +1027,8 @@ 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, 0, false, 0, &err);
+ true, mode, false, 0, false, NULL,
+ false, 0, false, 0, &err);
hmp_handle_error(mon, &err);
}
diff --git a/include/block/block.h b/include/block/block.h
index 60e8b6d..be35b57 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -469,6 +469,7 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_enable_copy_on_read(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index be99ec0..b8409ef 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -588,6 +588,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
* @target: Block device to write to.
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @sync_mode: What parts of the disk image should be copied to the destination.
+ * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_DIRTY_BITMAP.
* @on_source_error: The action to take upon error reading from the source.
* @on_target_error: The action to take upon error writing to the target.
* @cb: Completion function for the job.
@@ -598,6 +599,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
*/
void backup_start(BlockDriverState *bs, BlockDriverState *target,
int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f00e3bb..dca83c5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -508,10 +508,12 @@
#
# @none: only copy data written from now on
#
+# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.3
+#
# Since: 1.3
##
{ 'enum': 'MirrorSyncMode',
- 'data': ['top', 'full', 'none'] }
+ 'data': ['top', 'full', 'none', 'dirty-bitmap'] }
##
# @BlockJobType:
@@ -686,14 +688,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.3)
+#
# @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).
@@ -711,7 +716,7 @@
{ 'type': 'DriveBackup',
'data': { 'device': 'str', 'target': 'str', '*format': 'str',
'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
- '*speed': 'int',
+ '*speed': 'int', '*bitmap': 'str',
'*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ce7782f..5aa3845 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1048,7 +1048,7 @@ EQMP
{
.name = "drive-backup",
.args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
- "on-source-error:s?,on-target-error:s?",
+ "bitmap:s?,on-source-error:s?,on-target-error:s?",
.mhandler.cmd_new = qmp_marshal_input_drive_backup,
},
@@ -1075,8 +1075,9 @@ Arguments:
(json-string, optional)
- "sync": what parts of the disk image should be copied to the destination;
possibilities include "full" for all the disk, "top" for only the sectors
- allocated in the topmost image, or "none" to only replicate new I/O
- (MirrorSyncMode).
+ allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in
+ the bitmap, or "none" to only replicate new I/O (MirrorSyncMode).
+- "bitmap": dirty bitmap name for sync==dirty-bitmap
- "mode": whether and how QEMU should create a new image
(NewImageMode, optional, default 'absolute-paths')
- "speed": the maximum speed, in bytes per second (json-int, optional)
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 08/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 08/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
@ 2015-02-16 20:04 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-02-16 20:04 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, 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 <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 9 +++
> block/backup.c | 149 +++++++++++++++++++++++++++++++++++++++-------
> 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, 172 insertions(+), 34 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 09/17] qmp: add block-dirty-bitmap-clear
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (7 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 08/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 20:05 ` Max Reitz
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations John Snow
` (8 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
Add bdrv_clear_dirty_bitmap and a matching QMP command,
qmp_block_dirty_bitmap_clear that enables a user to reset
the bitmap attached to a drive.
This allows us to reset a bitmap in the event of a full
drive backup.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 8 ++++++++
blockdev.c | 37 +++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 +
qapi/block-core.json | 14 ++++++++++++++
qmp-commands.hx | 24 ++++++++++++++++++++++++
5 files changed, 84 insertions(+)
diff --git a/block.c b/block.c
index 6e3f817..e2da4cb 100644
--- a/block.c
+++ b/block.c
@@ -62,6 +62,7 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
BdrvDirtyBitmap *successor;
+ int64_t size;
char *name;
bool disabled;
QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -5422,6 +5423,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
+ bitmap->size = bitmap_size;
bitmap->name = g_strdup(name);
bitmap->disabled = false;
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
@@ -5624,6 +5626,12 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+}
+
static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
int nr_sectors)
{
diff --git a/blockdev.c b/blockdev.c
index 6990986..8422e94 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2116,6 +2116,43 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
aio_context_release(aio_context);
}
+/**
+ * Completely clear a bitmap, for the purposes of synchronizing a bitmap
+ * immediately after a full backup operation.
+ */
+void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently frozen and cannot be modified",
+ name);
+ goto out;
+ } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently disabled and cannot be cleared",
+ name);
+ goto out;
+ }
+
+ bdrv_clear_dirty_bitmap(bitmap);
+
+ out:
+ aio_context_release(aio_context);
+}
+
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/block.h b/include/block/block.h
index be35b57..67227a2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -467,6 +467,7 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index dca83c5..bfdb293 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1050,6 +1050,20 @@
'data': 'BlockDirtyBitmap' }
##
+# @block-dirty-bitmap-clear
+#
+# Clear (reset) a dirty bitmap on the device
+#
+# Returns: nothing on success
+# If @node is not a valid block device, DeviceNotFound
+# If @name is not found, GenericError with an explanation
+#
+# Since 2.3
+##
+{ 'command': 'block-dirty-bitmap-clear',
+ 'data': 'BlockDirtyBitmap' }
+
+##
# @block_set_io_throttle:
#
# Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5aa3845..21416fa 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1264,6 +1264,11 @@ EQMP
.args_type = "node:B,name:s",
.mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_disable,
},
+ {
+ .name = "block-dirty-bitmap-clear",
+ .args_type = "node:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_clear,
+ },
SQMP
@@ -1303,6 +1308,25 @@ Example:
"name": "bitmap0" } }
<- { "return": {} }
+block-dirty-bitmap-clear
+------------------------
+Since 2.3
+
+Reset the dirty bitmap associated with a node so that an incremental backup
+from this point in time forward will only backup clusters modified after this
+clear operation.
+
+Arguments:
+
+- "node": device/node on which to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-clear", "arguments": { "node": "drive0",
+ "name": "bitmap0" } }
+<- { "return": {} }
+
EQMP
{
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 09/17] qmp: add block-dirty-bitmap-clear
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 09/17] qmp: add block-dirty-bitmap-clear John Snow
@ 2015-02-16 20:05 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-02-16 20:05 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, John Snow wrote:
> Add bdrv_clear_dirty_bitmap and a matching QMP command,
> qmp_block_dirty_bitmap_clear that enables a user to reset
> the bitmap attached to a drive.
>
> This allows us to reset a bitmap in the event of a full
> drive backup.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 8 ++++++++
> blockdev.c | 37 +++++++++++++++++++++++++++++++++++++
> include/block/block.h | 1 +
> qapi/block-core.json | 14 ++++++++++++++
> qmp-commands.hx | 24 ++++++++++++++++++++++++
> 5 files changed, 84 insertions(+)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (8 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 09/17] qmp: add block-dirty-bitmap-clear John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 20:28 ` Max Reitz
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 11/17] qmp: Add dirty bitmap status fields in query-block John Snow
` (7 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
This adds four qmp commands to transactions.
Users can stop a dirty bitmap, start backup of it, and start another
dirty bitmap atomically, so that the dirty bitmap is tracked
incrementally and we don't miss any write.
For starting a new incremental backup chain, users can also chain
together a bitmap clear and a full block backup.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockdev.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 10 +++-
2 files changed, 179 insertions(+), 1 deletion(-)
diff --git a/blockdev.c b/blockdev.c
index 8422e94..20fea62 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1675,6 +1675,153 @@ static void blockdev_backup_clean(BlkTransactionState *common)
}
}
+typedef struct BlockDirtyBitmapState {
+ BlkTransactionState common;
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+ AioContext *aio_context;
+} BlockDirtyBitmapState;
+
+static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ BlockDirtyBitmapAdd *action;
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ action = common->action->block_dirty_bitmap_add;
+ /* AIO context taken within qmp_block_dirty_bitmap_add */
+ qmp_block_dirty_bitmap_add(action->node, action->name,
+ action->has_granularity, action->granularity,
+ errp);
+
+ state->bitmap = block_dirty_bitmap_lookup(action->node, action->name,
+ NULL, NULL);
+}
+
+static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+ BlockDirtyBitmapAdd *action;
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ action = common->action->block_dirty_bitmap_add;
+ /* Should not fail meaningfully: IF the bitmap was added via .prepare(),
+ * then the node reference and bitmap name must have been valid.
+ * THUS: any failure here could only indicate the lack of a bitmap at all.
+ * We will use the presence of the bitmap field to check that we succeeded
+ * in the first place, though.
+ */
+ if (state->bitmap) {
+ qmp_block_dirty_bitmap_remove(action->node, action->name, NULL);
+ }
+}
+
+/**
+ * Enable and Disable re-use the same preparation.
+ */
+static void block_dirty_bitmap_toggle_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ BlockDirtyBitmap *action;
+ BlockDriverState *bs;
+
+ /* We may be used by either enable or disable;
+ * We use the "enable" member of the union here,
+ * but "disable" should be functionally equivalent: */
+ action = common->action->block_dirty_bitmap_enable;
+ assert(action == common->action->block_dirty_bitmap_disable);
+
+ state->bitmap = block_dirty_bitmap_lookup(action->node,
+ action->name,
+ &bs,
+ errp);
+ if (!state->bitmap) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
+ error_setg(errp, "Cannot modify a frozen bitmap");
+ return;
+ }
+
+ /* AioContext is released in .clean() */
+ state->aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(state->aio_context);
+}
+
+static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ bdrv_enable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ bdrv_disable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_toggle_clean(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ if (state->aio_context) {
+ aio_context_release(state->aio_context);
+ }
+}
+
+static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ BlockDirtyBitmap *action;
+
+ action = common->action->block_dirty_bitmap_clear;
+ state->bitmap = block_dirty_bitmap_lookup(action->node,
+ action->name,
+ &state->bs,
+ errp);
+ if (!state->bitmap) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
+ error_setg(errp, "Cannot modify a frozen bitmap");
+ return;
+ } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
+ error_setg(errp, "Cannot clear a disabled bitmap");
+ return;
+ }
+
+ /* AioContext is released in .clean() */
+ state->aio_context = bdrv_get_aio_context(state->bs);
+ aio_context_acquire(state->aio_context);
+}
+
+static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ bdrv_clear_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ if (state->aio_context) {
+ aio_context_release(state->aio_context);
+ }
+}
+
static void abort_prepare(BlkTransactionState *common, Error **errp)
{
error_setg(errp, "Transaction aborted using Abort action");
@@ -1715,6 +1862,29 @@ static const BdrvActionOps actions[] = {
.abort = internal_snapshot_abort,
.clean = internal_snapshot_clean,
},
+ [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
+ .instance_size = sizeof(BlkTransactionState),
+ .prepare = block_dirty_bitmap_add_prepare,
+ .abort = block_dirty_bitmap_add_abort,
+ },
+ [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
+ .instance_size = sizeof(BlockDirtyBitmapState),
+ .prepare = block_dirty_bitmap_toggle_prepare,
+ .commit = block_dirty_bitmap_enable_commit,
+ .clean = block_dirty_bitmap_toggle_clean,
+ },
+ [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
+ .instance_size = sizeof(BlockDirtyBitmapState),
+ .prepare = block_dirty_bitmap_toggle_prepare,
+ .commit = block_dirty_bitmap_disable_commit,
+ .clean = block_dirty_bitmap_toggle_clean,
+ },
+ [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
+ .instance_size = sizeof(BlockDirtyBitmapState),
+ .prepare = block_dirty_bitmap_clear_prepare,
+ .commit = block_dirty_bitmap_clear_commit,
+ .clean = block_dirty_bitmap_clear_clean,
+ }
};
/*
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..0c31203 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1332,6 +1332,10 @@
# abort since 1.6
# blockdev-snapshot-internal-sync since 1.7
# blockdev-backup since 2.3
+# block-dirty-bitmap-add since 2.3
+# block-dirty-bitmap-enable since 2.3
+# block-dirty-bitmap-disable since 2.3
+# block-dirty-bitmap-clear since 2.3
##
{ 'union': 'TransactionAction',
'data': {
@@ -1339,7 +1343,11 @@
'drive-backup': 'DriveBackup',
'blockdev-backup': 'BlockdevBackup',
'abort': 'Abort',
- 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+ 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+ 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
+ 'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
+ 'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+ 'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
} }
##
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations John Snow
@ 2015-02-16 20:28 ` Max Reitz
2015-02-16 21:33 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Max Reitz @ 2015-02-16 20:28 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, John Snow wrote:
> This adds four qmp commands to transactions.
>
> Users can stop a dirty bitmap, start backup of it, and start another
> dirty bitmap atomically, so that the dirty bitmap is tracked
> incrementally and we don't miss any write.
>
> For starting a new incremental backup chain, users can also chain
> together a bitmap clear and a full block backup.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> blockdev.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qapi-schema.json | 10 +++-
> 2 files changed, 179 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 8422e94..20fea62 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1675,6 +1675,153 @@ static void blockdev_backup_clean(BlkTransactionState *common)
> }
> }
>
> +typedef struct BlockDirtyBitmapState {
> + BlkTransactionState common;
> + BdrvDirtyBitmap *bitmap;
> + BlockDriverState *bs;
> + AioContext *aio_context;
> +} BlockDirtyBitmapState;
> +
> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> + Error **errp)
> +{
> + BlockDirtyBitmapAdd *action;
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> + action = common->action->block_dirty_bitmap_add;
> + /* AIO context taken within qmp_block_dirty_bitmap_add */
> + qmp_block_dirty_bitmap_add(action->node, action->name,
> + action->has_granularity, action->granularity,
> + errp);
> +
> + state->bitmap = block_dirty_bitmap_lookup(action->node, action->name,
> + NULL, NULL);
If the bitmap has been added successfully, this will succeed. If the
bitmap has not been added successfully because a bitmap with the same
name already exists, this will succeed, too. Therefore, this fails in
one case of failure. See [1].
> +}
> +
> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapAdd *action;
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
Indentation is one space off.
> + common, common);
> +
> + action = common->action->block_dirty_bitmap_add;
> + /* Should not fail meaningfully: IF the bitmap was added via .prepare(),
> + * then the node reference and bitmap name must have been valid.
> + * THUS: any failure here could only indicate the lack of a bitmap at all.
> + * We will use the presence of the bitmap field to check that we succeeded
> + * in the first place, though.
> + */
> + if (state->bitmap) {
> + qmp_block_dirty_bitmap_remove(action->node, action->name, NULL);
[1] As said above, state->bitmap will be non-NULL if the bitmap existed
before. Thus, this may remove a pre-existing bitmap which has not been
added by this transaction. Case in point:
$ x86_64-softmmu/qemu-system-x86_64 -drive
if=none,id=drv,file=test.qcow2 -qmp stdio -machine accel=qtest -display
none -nodefaults
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2},
"package": ""}, "capabilities": []}}
{'execute':'qmp_capabilities'}
{"return": {}}
{'execute':'query-block'}
{"return": [{"device": "drv", "locked": false, "removable": true,
"inserted": {"iops_rd": 0, "detect_zeroes": "off", "image":
{"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size":
65536, "format": "qcow2", "actual-size": 200704, "format-specific":
{"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false,
"corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false,
"backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0,
"write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0,
"cache": {"no-flush": false, "direct": false, "writeback": true},
"file": "test.qcow2", "encryption_key_missing": false}, "tray_open":
false, "type": "unknown"}]}
{'execute':'transaction','arguments':{'actions':[{'type':'block-dirty-bitmap-add','data':{'node':'drv','name':'foo'}}]}}
{"return": {}}
{'execute':'query-block'}
{"return": [{"device": "drv", "locked": false, "removable": true,
"inserted": {"iops_rd": 0, "detect_zeroes": "off", "image":
{"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size":
65536, "format": "qcow2", "actual-size": 200704, "format-specific":
{"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false,
"corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false,
"backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0,
"write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0,
"cache": {"no-flush": false, "direct": false, "writeback": true},
"file": "test.qcow2", "encryption_key_missing": false}, "tray_open":
false, "dirty-bitmaps": [{"name": "foo", "granularity": 65536, "count":
0}], "type": "unknown"}]}
{'execute':'transaction','arguments':{'actions':[{'type':'block-dirty-bitmap-add','data':{'node':'drv','name':'foo'}}]}}
{"error": {"class": "GenericError", "desc": "Bitmap already exists: foo"}}
{'execute':'query-block'}
{"return": [{"device": "drv", "locked": false, "removable": true,
"inserted": {"iops_rd": 0, "detect_zeroes": "off", "image":
{"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size":
65536, "format": "qcow2", "actual-size": 200704, "format-specific":
{"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false,
"corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false,
"backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0,
"write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0,
"cache": {"no-flush": false, "direct": false, "writeback": true},
"file": "test.qcow2", "encryption_key_missing": false}, "tray_open":
false, "type": "unknown"}]}
No dirty-bitmaps any more for drv.
(You'll want to heed [2] first before trying this, or you'll get a
segfault in the very first transaction)
I see two ways to solve this: As I said, you are free to call
qmp_block_dirty_bitmap_remove() even if creating the bitmap failed;
there is only one case where you must not call that function, and that
is if the bitmap existed before the transaction. In that case, you
should be calling block_dirty_bitmap_lookup() before
qmp_block_dirty_bitmap_add(), and if it succeeds you should not be
calling qmp_block_dirty_bitmap_remove(); if it fails you can call
qmp_block_dirty_bitmap_remove() (although I personally find that ugly
because you may be calling that function knowing that it will fail).
The other way is to evaluate the error returned by
qmp_block_dirty_bitmap_add() (pass it to a local Error pointer, remember
whether that was non-NULL, propagate it to errp). If there was an error,
do not call qmp_block_dirty_bitmap_remove() in
block_dirtybitmap_add_abort().
Max
> + }
> +}
> +
> +/**
> + * Enable and Disable re-use the same preparation.
> + */
> +static void block_dirty_bitmap_toggle_prepare(BlkTransactionState *common,
> + Error **errp)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + BlockDirtyBitmap *action;
> + BlockDriverState *bs;
> +
> + /* We may be used by either enable or disable;
> + * We use the "enable" member of the union here,
> + * but "disable" should be functionally equivalent: */
> + action = common->action->block_dirty_bitmap_enable;
> + assert(action == common->action->block_dirty_bitmap_disable);
> +
> + state->bitmap = block_dirty_bitmap_lookup(action->node,
> + action->name,
> + &bs,
> + errp);
> + if (!state->bitmap) {
> + return;
> + }
> +
> + if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> + error_setg(errp, "Cannot modify a frozen bitmap");
> + return;
> + }
> +
> + /* AioContext is released in .clean() */
> + state->aio_context = bdrv_get_aio_context(bs);
> + aio_context_acquire(state->aio_context);
> +}
> +
> +static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + bdrv_enable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + bdrv_disable_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_toggle_clean(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> + if (state->aio_context) {
> + aio_context_release(state->aio_context);
> + }
> +}
> +
> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
> + Error **errp)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + BlockDirtyBitmap *action;
> +
> + action = common->action->block_dirty_bitmap_clear;
> + state->bitmap = block_dirty_bitmap_lookup(action->node,
> + action->name,
> + &state->bs,
> + errp);
> + if (!state->bitmap) {
> + return;
> + }
> +
> + if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> + error_setg(errp, "Cannot modify a frozen bitmap");
> + return;
> + } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
> + error_setg(errp, "Cannot clear a disabled bitmap");
> + return;
> + }
> +
> + /* AioContext is released in .clean() */
> + state->aio_context = bdrv_get_aio_context(state->bs);
> + aio_context_acquire(state->aio_context);
> +}
> +
> +static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + bdrv_clear_dirty_bitmap(state->bitmap);
> +}
> +
> +static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> +
> + if (state->aio_context) {
> + aio_context_release(state->aio_context);
> + }
> +}
> +
> static void abort_prepare(BlkTransactionState *common, Error **errp)
> {
> error_setg(errp, "Transaction aborted using Abort action");
> @@ -1715,6 +1862,29 @@ static const BdrvActionOps actions[] = {
> .abort = internal_snapshot_abort,
> .clean = internal_snapshot_clean,
> },
> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
> + .instance_size = sizeof(BlkTransactionState),
[2] This should be sizeof(BlockDirtyBitmapState).
> + .prepare = block_dirty_bitmap_add_prepare,
> + .abort = block_dirty_bitmap_add_abort,
> + },
> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
> + .instance_size = sizeof(BlockDirtyBitmapState),
> + .prepare = block_dirty_bitmap_toggle_prepare,
> + .commit = block_dirty_bitmap_enable_commit,
> + .clean = block_dirty_bitmap_toggle_clean,
> + },
> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> + .instance_size = sizeof(BlockDirtyBitmapState),
> + .prepare = block_dirty_bitmap_toggle_prepare,
> + .commit = block_dirty_bitmap_disable_commit,
> + .clean = block_dirty_bitmap_toggle_clean,
> + },
> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
> + .instance_size = sizeof(BlockDirtyBitmapState),
> + .prepare = block_dirty_bitmap_clear_prepare,
> + .commit = block_dirty_bitmap_clear_commit,
> + .clean = block_dirty_bitmap_clear_clean,
> + }
> };
>
> /*
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e16f8eb..0c31203 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1332,6 +1332,10 @@
> # abort since 1.6
> # blockdev-snapshot-internal-sync since 1.7
> # blockdev-backup since 2.3
> +# block-dirty-bitmap-add since 2.3
> +# block-dirty-bitmap-enable since 2.3
> +# block-dirty-bitmap-disable since 2.3
> +# block-dirty-bitmap-clear since 2.3
> ##
> { 'union': 'TransactionAction',
> 'data': {
> @@ -1339,7 +1343,11 @@
> 'drive-backup': 'DriveBackup',
> 'blockdev-backup': 'BlockdevBackup',
> 'abort': 'Abort',
> - 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> + 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> + 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
> + 'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
> + 'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
> + 'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
> } }
>
> ##
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations
2015-02-16 20:28 ` Max Reitz
@ 2015-02-16 21:33 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-16 21:33 UTC (permalink / raw)
To: Max Reitz, qemu-devel
On 02/16/2015 03:28 PM, Max Reitz wrote:
> On 2015-02-13 at 17:08, John Snow wrote:
>> This adds four qmp commands to transactions.
>>
>> Users can stop a dirty bitmap, start backup of it, and start another
>> dirty bitmap atomically, so that the dirty bitmap is tracked
>> incrementally and we don't miss any write.
>>
>> For starting a new incremental backup chain, users can also chain
>> together a bitmap clear and a full block backup.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> blockdev.c | 170
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> qapi-schema.json | 10 +++-
>> 2 files changed, 179 insertions(+), 1 deletion(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 8422e94..20fea62 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1675,6 +1675,153 @@ static void
>> blockdev_backup_clean(BlkTransactionState *common)
>> }
>> }
>> +typedef struct BlockDirtyBitmapState {
>> + BlkTransactionState common;
>> + BdrvDirtyBitmap *bitmap;
>> + BlockDriverState *bs;
>> + AioContext *aio_context;
>> +} BlockDirtyBitmapState;
>> +
>> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
>> + Error **errp)
>> +{
>> + BlockDirtyBitmapAdd *action;
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> +
>> + action = common->action->block_dirty_bitmap_add;
>> + /* AIO context taken within qmp_block_dirty_bitmap_add */
>> + qmp_block_dirty_bitmap_add(action->node, action->name,
>> + action->has_granularity,
>> action->granularity,
>> + errp);
>> +
>> + state->bitmap = block_dirty_bitmap_lookup(action->node,
>> action->name,
>> + NULL, NULL);
>
> If the bitmap has been added successfully, this will succeed. If the
> bitmap has not been added successfully because a bitmap with the same
> name already exists, this will succeed, too. Therefore, this fails in
> one case of failure. See [1].
I would have liked to have been in the room with myself when I made the
decision that this patch would correct the behavior you spotted last time.
Surely I could have stopped myself from doing it.
>
>> +}
>> +
>> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
>> +{
>> + BlockDirtyBitmapAdd *action;
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>
> Indentation is one space off.
>
>> + common, common);
>> +
>> + action = common->action->block_dirty_bitmap_add;
>> + /* Should not fail meaningfully: IF the bitmap was added via
>> .prepare(),
>> + * then the node reference and bitmap name must have been valid.
>> + * THUS: any failure here could only indicate the lack of a
>> bitmap at all.
>> + * We will use the presence of the bitmap field to check that we
>> succeeded
>> + * in the first place, though.
>> + */
>> + if (state->bitmap) {
>> + qmp_block_dirty_bitmap_remove(action->node, action->name, NULL);
>
> [1] As said above, state->bitmap will be non-NULL if the bitmap existed
> before. Thus, this may remove a pre-existing bitmap which has not been
> added by this transaction. Case in point:
>
> $ x86_64-softmmu/qemu-system-x86_64 -drive
> if=none,id=drv,file=test.qcow2 -qmp stdio -machine accel=qtest -display
> none -nodefaults
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2},
> "package": ""}, "capabilities": []}}
> {'execute':'qmp_capabilities'}
> {"return": {}}
> {'execute':'query-block'}
> {"return": [{"device": "drv", "locked": false, "removable": true,
> "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image":
> {"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size":
> 65536, "format": "qcow2", "actual-size": 200704, "format-specific":
> {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false,
> "corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false,
> "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0,
> "write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0,
> "cache": {"no-flush": false, "direct": false, "writeback": true},
> "file": "test.qcow2", "encryption_key_missing": false}, "tray_open":
> false, "type": "unknown"}]}
> {'execute':'transaction','arguments':{'actions':[{'type':'block-dirty-bitmap-add','data':{'node':'drv','name':'foo'}}]}}
>
> {"return": {}}
> {'execute':'query-block'}
> {"return": [{"device": "drv", "locked": false, "removable": true,
> "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image":
> {"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size":
> 65536, "format": "qcow2", "actual-size": 200704, "format-specific":
> {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false,
> "corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false,
> "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0,
> "write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0,
> "cache": {"no-flush": false, "direct": false, "writeback": true},
> "file": "test.qcow2", "encryption_key_missing": false}, "tray_open":
> false, "dirty-bitmaps": [{"name": "foo", "granularity": 65536, "count":
> 0}], "type": "unknown"}]}
> {'execute':'transaction','arguments':{'actions':[{'type':'block-dirty-bitmap-add','data':{'node':'drv','name':'foo'}}]}}
>
> {"error": {"class": "GenericError", "desc": "Bitmap already exists: foo"}}
> {'execute':'query-block'}
> {"return": [{"device": "drv", "locked": false, "removable": true,
> "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image":
> {"virtual-size": 67108864, "filename": "test.qcow2", "cluster-size":
> 65536, "format": "qcow2", "actual-size": 200704, "format-specific":
> {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false,
> "corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false,
> "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0,
> "write_threshold": 0, "encrypted": false, "bps": 0, "bps_rd": 0,
> "cache": {"no-flush": false, "direct": false, "writeback": true},
> "file": "test.qcow2", "encryption_key_missing": false}, "tray_open":
> false, "type": "unknown"}]}
>
> No dirty-bitmaps any more for drv.
>
> (You'll want to heed [2] first before trying this, or you'll get a
> segfault in the very first transaction)
>
> I see two ways to solve this: As I said, you are free to call
> qmp_block_dirty_bitmap_remove() even if creating the bitmap failed;
> there is only one case where you must not call that function, and that
> is if the bitmap existed before the transaction. In that case, you
> should be calling block_dirty_bitmap_lookup() before
> qmp_block_dirty_bitmap_add(), and if it succeeds you should not be
> calling qmp_block_dirty_bitmap_remove(); if it fails you can call
> qmp_block_dirty_bitmap_remove() (although I personally find that ugly
> because you may be calling that function knowing that it will fail).
>
> The other way is to evaluate the error returned by
> qmp_block_dirty_bitmap_add() (pass it to a local Error pointer, remember
> whether that was non-NULL, propagate it to errp). If there was an error,
> do not call qmp_block_dirty_bitmap_remove() in
> block_dirtybitmap_add_abort().
>
> Max
>
>> + }
>> +}
>> +
>> +/**
>> + * Enable and Disable re-use the same preparation.
>> + */
>> +static void block_dirty_bitmap_toggle_prepare(BlkTransactionState
>> *common,
>> + Error **errp)
>> +{
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> + BlockDirtyBitmap *action;
>> + BlockDriverState *bs;
>> +
>> + /* We may be used by either enable or disable;
>> + * We use the "enable" member of the union here,
>> + * but "disable" should be functionally equivalent: */
>> + action = common->action->block_dirty_bitmap_enable;
>> + assert(action == common->action->block_dirty_bitmap_disable);
>> +
>> + state->bitmap = block_dirty_bitmap_lookup(action->node,
>> + action->name,
>> + &bs,
>> + errp);
>> + if (!state->bitmap) {
>> + return;
>> + }
>> +
>> + if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
>> + error_setg(errp, "Cannot modify a frozen bitmap");
>> + return;
>> + }
>> +
>> + /* AioContext is released in .clean() */
>> + state->aio_context = bdrv_get_aio_context(bs);
>> + aio_context_acquire(state->aio_context);
>> +}
>> +
>> +static void block_dirty_bitmap_enable_commit(BlkTransactionState
>> *common)
>> +{
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> + bdrv_enable_dirty_bitmap(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_disable_commit(BlkTransactionState
>> *common)
>> +{
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> + bdrv_disable_dirty_bitmap(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_toggle_clean(BlkTransactionState *common)
>> +{
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> +
>> + if (state->aio_context) {
>> + aio_context_release(state->aio_context);
>> + }
>> +}
>> +
>> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState
>> *common,
>> + Error **errp)
>> +{
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> + BlockDirtyBitmap *action;
>> +
>> + action = common->action->block_dirty_bitmap_clear;
>> + state->bitmap = block_dirty_bitmap_lookup(action->node,
>> + action->name,
>> + &state->bs,
>> + errp);
>> + if (!state->bitmap) {
>> + return;
>> + }
>> +
>> + if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
>> + error_setg(errp, "Cannot modify a frozen bitmap");
>> + return;
>> + } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
>> + error_setg(errp, "Cannot clear a disabled bitmap");
>> + return;
>> + }
>> +
>> + /* AioContext is released in .clean() */
>> + state->aio_context = bdrv_get_aio_context(state->bs);
>> + aio_context_acquire(state->aio_context);
>> +}
>> +
>> +static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
>> +{
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> + bdrv_clear_dirty_bitmap(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
>> +{
>> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> + common, common);
>> +
>> + if (state->aio_context) {
>> + aio_context_release(state->aio_context);
>> + }
>> +}
>> +
>> static void abort_prepare(BlkTransactionState *common, Error **errp)
>> {
>> error_setg(errp, "Transaction aborted using Abort action");
>> @@ -1715,6 +1862,29 @@ static const BdrvActionOps actions[] = {
>> .abort = internal_snapshot_abort,
>> .clean = internal_snapshot_clean,
>> },
>> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
>> + .instance_size = sizeof(BlkTransactionState),
>
> [2] This should be sizeof(BlockDirtyBitmapState).
>
>> + .prepare = block_dirty_bitmap_add_prepare,
>> + .abort = block_dirty_bitmap_add_abort,
>> + },
>> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
>> + .instance_size = sizeof(BlockDirtyBitmapState),
>> + .prepare = block_dirty_bitmap_toggle_prepare,
>> + .commit = block_dirty_bitmap_enable_commit,
>> + .clean = block_dirty_bitmap_toggle_clean,
>> + },
>> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
>> + .instance_size = sizeof(BlockDirtyBitmapState),
>> + .prepare = block_dirty_bitmap_toggle_prepare,
>> + .commit = block_dirty_bitmap_disable_commit,
>> + .clean = block_dirty_bitmap_toggle_clean,
>> + },
>> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
>> + .instance_size = sizeof(BlockDirtyBitmapState),
>> + .prepare = block_dirty_bitmap_clear_prepare,
>> + .commit = block_dirty_bitmap_clear_commit,
>> + .clean = block_dirty_bitmap_clear_clean,
>> + }
>> };
>> /*
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index e16f8eb..0c31203 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1332,6 +1332,10 @@
>> # abort since 1.6
>> # blockdev-snapshot-internal-sync since 1.7
>> # blockdev-backup since 2.3
>> +# block-dirty-bitmap-add since 2.3
>> +# block-dirty-bitmap-enable since 2.3
>> +# block-dirty-bitmap-disable since 2.3
>> +# block-dirty-bitmap-clear since 2.3
>> ##
>> { 'union': 'TransactionAction',
>> 'data': {
>> @@ -1339,7 +1343,11 @@
>> 'drive-backup': 'DriveBackup',
>> 'blockdev-backup': 'BlockdevBackup',
>> 'abort': 'Abort',
>> - 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
>> + 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> + 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>> + 'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>> + 'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>> + 'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
>> } }
>> ##
>
>
--
—js
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 11/17] qmp: Add dirty bitmap status fields in query-block
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (9 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 10/17] qapi: Add transaction support to block-dirty-bitmap operations John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 12/17] block: add BdrvDirtyBitmap documentation John Snow
` (6 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
Adds the "disabled" and "frozen" status booleans.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 2 ++
qapi/block-core.json | 7 ++++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index e2da4cb..a7f56f4 100644
--- a/block.c
+++ b/block.c
@@ -5564,6 +5564,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
+ info->disabled = bm->disabled;
+ info->frozen = bdrv_dirty_bitmap_frozen(bm);
entry->value = info;
*plist = entry;
plist = &entry->next;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bfdb293..02bf2b6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -332,10 +332,15 @@
#
# @granularity: granularity of the dirty bitmap in bytes (since 1.4)
#
+# @disabled: whether the dirty bitmap is disabled (Since 2.3)
+#
+# @frozen: whether the dirty bitmap is frozen (Since 2.3)
+#
# Since: 1.3
##
{ 'type': 'BlockDirtyInfo',
- 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} }
+ 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+ 'disabled': 'bool', 'frozen': 'bool'} }
##
# @BlockInfo:
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v13 12/17] block: add BdrvDirtyBitmap documentation
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (10 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 11/17] qmp: Add dirty bitmap status fields in query-block John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 13/17] block: Ensure consistent bitmap function prototypes John Snow
` (5 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index a7f56f4..c1c11aa 100644
--- a/block.c
+++ b/block.c
@@ -60,11 +60,11 @@
* or enabled. A frozen bitmap can only abdicate() or reclaim().
*/
struct BdrvDirtyBitmap {
- HBitmap *bitmap;
- BdrvDirtyBitmap *successor;
- int64_t size;
- char *name;
- bool disabled;
+ HBitmap *bitmap; /* Dirty sector bitmap implementation */
+ BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
+ char *name; /* Optional non-empty unique ID */
+ int64_t size; /* Size of the bitmap (Number of sectors) */
+ bool disabled; /* Bitmap is read-only */
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v13 13/17] block: Ensure consistent bitmap function prototypes
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (11 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 12/17] block: add BdrvDirtyBitmap documentation John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 14/17] iotests: add invalid input incremental backup tests John Snow
` (4 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
We often don't need the BlockDriverState for functions
that operate on bitmaps. Remove it.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 13 ++++++-------
block/backup.c | 2 +-
block/mirror.c | 26 ++++++++++----------------
blockdev.c | 2 +-
include/block/block.h | 11 +++++------
migration/block.c | 7 +++----
6 files changed, 26 insertions(+), 35 deletions(-)
diff --git a/block.c b/block.c
index c1c11aa..f3a6dd4 100644
--- a/block.c
+++ b/block.c
@@ -5391,7 +5391,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
return NULL;
}
-void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
{
assert(!bdrv_dirty_bitmap_frozen(bitmap));
g_free(bitmap->name);
@@ -5560,7 +5560,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
- info->count = bdrv_get_dirty_count(bs, bm);
+ info->count = bdrv_get_dirty_count(bm);
info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
@@ -5608,20 +5608,19 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
}
-void bdrv_dirty_iter_init(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
+void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
{
hbitmap_iter_init(hbi, bitmap->bitmap, 0);
}
-void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
assert(bdrv_dirty_bitmap_enabled(bitmap));
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
-void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
assert(bdrv_dirty_bitmap_enabled(bitmap));
@@ -5667,7 +5666,7 @@ void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
hbitmap_iter_init(hbi, hbi->hb, offset);
}
-int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
{
return hbitmap_count(bitmap->bitmap);
}
diff --git a/block/backup.c b/block/backup.c
index d46c0a3..41bd9af 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -313,7 +313,7 @@ static void coroutine_fn backup_run(void *opaque)
int64_t last_cluster = -1;
bool polyrhythmic;
- bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+ bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
/* Does the granularity happen to match our backup cluster size? */
polyrhythmic = (bdrv_dirty_bitmap_granularity(job->sync_bitmap) !=
BACKUP_CLUSTER_SIZE);
diff --git a/block/mirror.c b/block/mirror.c
index f89eccf..dcd6f65 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -125,11 +125,9 @@ static void mirror_write_complete(void *opaque, int ret)
MirrorOp *op = opaque;
MirrorBlockJob *s = op->s;
if (ret < 0) {
- BlockDriverState *source = s->common.bs;
BlockErrorAction action;
- bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
- op->nb_sectors);
+ bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, op->nb_sectors);
action = mirror_error_action(s, false, -ret);
if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
s->ret = ret;
@@ -143,11 +141,9 @@ static void mirror_read_complete(void *opaque, int ret)
MirrorOp *op = opaque;
MirrorBlockJob *s = op->s;
if (ret < 0) {
- BlockDriverState *source = s->common.bs;
BlockErrorAction action;
- bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
- op->nb_sectors);
+ bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, op->nb_sectors);
action = mirror_error_action(s, true, -ret);
if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
s->ret = ret;
@@ -170,10 +166,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
s->sector_num = hbitmap_iter_next(&s->hbi);
if (s->sector_num < 0) {
- bdrv_dirty_iter_init(source, s->dirty_bitmap, &s->hbi);
+ bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
s->sector_num = hbitmap_iter_next(&s->hbi);
- trace_mirror_restart_iter(s,
- bdrv_get_dirty_count(source, s->dirty_bitmap));
+ trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
assert(s->sector_num >= 0);
}
@@ -288,8 +283,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
next_sector += sectors_per_chunk;
}
- bdrv_reset_dirty_bitmap(source, s->dirty_bitmap, sector_num,
- nb_sectors);
+ bdrv_reset_dirty_bitmap(s->dirty_bitmap, sector_num, nb_sectors);
/* Copy the dirty cluster. */
s->in_flight++;
@@ -446,7 +440,7 @@ static void coroutine_fn mirror_run(void *opaque)
assert(n > 0);
if (ret == 1) {
- bdrv_set_dirty_bitmap(bs, s->dirty_bitmap, sector_num, n);
+ bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
sector_num = next;
} else {
sector_num += n;
@@ -454,7 +448,7 @@ static void coroutine_fn mirror_run(void *opaque)
}
}
- bdrv_dirty_iter_init(bs, s->dirty_bitmap, &s->hbi);
+ bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
for (;;) {
uint64_t delay_ns = 0;
@@ -466,7 +460,7 @@ static void coroutine_fn mirror_run(void *opaque)
goto immediate_exit;
}
- cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
+ cnt = bdrv_get_dirty_count(s->dirty_bitmap);
/* s->common.offset contains the number of bytes already processed so
* far, cnt is the number of dirty sectors remaining and
* s->sectors_in_flight is the number of sectors currently being
@@ -516,7 +510,7 @@ static void coroutine_fn mirror_run(void *opaque)
should_complete = s->should_complete ||
block_job_is_cancelled(&s->common);
- cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
+ cnt = bdrv_get_dirty_count(s->dirty_bitmap);
}
}
@@ -531,7 +525,7 @@ static void coroutine_fn mirror_run(void *opaque)
*/
trace_mirror_before_drain(s, cnt);
bdrv_drain(bs);
- cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
+ cnt = bdrv_get_dirty_count(s->dirty_bitmap);
}
ret = 0;
diff --git a/blockdev.c b/blockdev.c
index 20fea62..13068c7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2224,7 +2224,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
name);
goto out;
}
- bdrv_dirty_bitmap_make_anon(bs, bitmap);
+ bdrv_dirty_bitmap_make_anon(bitmap);
bdrv_release_dirty_bitmap(bs, bitmap);
out:
diff --git a/include/block/block.h b/include/block/block.h
index 67227a2..f6a50ae 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -453,7 +453,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
Error **errp);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
-void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
@@ -463,15 +463,14 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
-void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
-void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
-void bdrv_dirty_iter_init(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
-int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
void bdrv_enable_copy_on_read(BlockDriverState *bs);
void bdrv_disable_copy_on_read(BlockDriverState *bs);
diff --git a/migration/block.c b/migration/block.c
index 5f9b3e5..1a9e7cd 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -303,7 +303,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
nr_sectors, blk_mig_read_cb, blk);
- bdrv_reset_dirty_bitmap(bs, bmds->dirty_bitmap, cur_sector, nr_sectors);
+ bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
qemu_mutex_unlock_iothread();
bmds->cur_sector = cur_sector + nr_sectors;
@@ -496,8 +496,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
g_free(blk);
}
- bdrv_reset_dirty_bitmap(bmds->bs, bmds->dirty_bitmap, sector,
- nr_sectors);
+ bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
break;
}
sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
@@ -583,7 +582,7 @@ static int64_t get_remaining_dirty(void)
int64_t dirty = 0;
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
- dirty += bdrv_get_dirty_count(bmds->bs, bmds->dirty_bitmap);
+ dirty += bdrv_get_dirty_count(bmds->dirty_bitmap);
}
return dirty << BDRV_SECTOR_BITS;
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v13 14/17] iotests: add invalid input incremental backup tests
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (12 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 13/17] block: Ensure consistent bitmap function prototypes John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case John Snow
` (3 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/112 | 89 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/112.out | 5 +++
tests/qemu-iotests/group | 1 +
3 files changed, 95 insertions(+)
create mode 100644 tests/qemu-iotests/112
create mode 100644 tests/qemu-iotests/112.out
diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112
new file mode 100644
index 0000000..7985cd1
--- /dev/null
+++ b/tests/qemu-iotests/112
@@ -0,0 +1,89 @@
+#!/usr/bin/env python
+#
+# Tests for incremental drive-backup
+#
+# Copyright (C) 2015 John Snow for Red Hat, Inc.
+#
+# Based on 056.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+
+
+def io_write_patterns(img, patterns):
+ for pattern in patterns:
+ iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
+
+
+class TestIncrementalBackup(iotests.QMPTestCase):
+ def setUp(self):
+ self.bitmaps = list()
+ self.files = list()
+ self.vm = iotests.VM()
+ self.test_img = os.path.join(iotests.test_dir, 'base.img')
+ self.full_bak = os.path.join(iotests.test_dir, 'backup.img')
+ self.foo_img = os.path.join(iotests.test_dir, 'foo.bar')
+ self.img_create(self.test_img, iotests.imgfmt)
+ self.vm.add_drive(self.test_img)
+ # Create a base image with a distinctive patterning
+ io_write_patterns(self.test_img, (('0x41', 0, 512),
+ ('0xd5', '1M', '32k'),
+ ('0xdc', '32M', '124k')))
+ self.vm.launch()
+
+
+ def img_create(self, img, fmt=iotests.imgfmt, size='64M',
+ parent=None, parentFormat=None):
+ plist = list()
+ if parent:
+ if parentFormat is None:
+ parentFormat = fmt
+ iotests.qemu_img('create', '-f', fmt, img, size,
+ '-b', parent, '-F', parentFormat)
+ else:
+ iotests.qemu_img('create', '-f', fmt, img, size)
+ self.files.append(img)
+
+ def test_sync_dirty_bitmap_missing(self):
+ self.assert_no_active_block_jobs()
+ self.files.append(self.foo_img)
+ result = self.vm.qmp('drive-backup', device='drive0',
+ sync='dirty-bitmap', format=iotests.imgfmt,
+ target=self.foo_img)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+
+ def test_sync_dirty_bitmap_not_found(self):
+ self.assert_no_active_block_jobs()
+ self.files.append(self.foo_img)
+ result = self.vm.qmp('drive-backup', device='drive0',
+ sync='dirty-bitmap', bitmap='unknown',
+ format=iotests.imgfmt, target=self.foo_img)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for filename in self.files:
+ try:
+ os.remove(filename)
+ except OSError:
+ pass
+
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
new file mode 100644
index 0000000..fbc63e6
--- /dev/null
+++ b/tests/qemu-iotests/112.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 4b2b93b..b4ddf1b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -114,6 +114,7 @@
109 rw auto
110 rw auto backing quick
111 rw auto quick
+112 rw auto backing
113 rw auto quick
114 rw auto quick
116 rw auto quick
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (13 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 14/17] iotests: add invalid input incremental backup tests John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 20:36 ` Max Reitz
2015-02-20 10:02 ` Stefan Hajnoczi
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 16/17] iotests: add transactional incremental backup test John Snow
` (2 subsequent siblings)
17 siblings, 2 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/112 | 122 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/112.out | 4 +-
2 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112
index 7985cd1..da8f0e0 100644
--- a/tests/qemu-iotests/112
+++ b/tests/qemu-iotests/112
@@ -28,6 +28,36 @@ def io_write_patterns(img, patterns):
for pattern in patterns:
iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
+class Bitmap:
+ def __init__(self, name, node):
+ self.name = name
+ self.node = node
+ self.pattern = os.path.join(iotests.test_dir.replace('%', '%%'),
+ '%s.backup.%%i.img' % name)
+ self.num = 0
+ self.backups = list()
+
+ def new_target(self, num=None):
+ if num is None:
+ num = self.num
+ self.num = num + 1
+ target = self.pattern % num
+ self.backups.append(target)
+ return target
+
+ def last_target(self):
+ return self.backups[-1]
+
+ def del_target(self):
+ os.remove(self.backups.pop())
+ self.num -= 1
+
+ def __del__(self):
+ for backup in self.backups:
+ try:
+ os.remove(backup)
+ except OSError:
+ pass
class TestIncrementalBackup(iotests.QMPTestCase):
def setUp(self):
@@ -58,6 +88,98 @@ class TestIncrementalBackup(iotests.QMPTestCase):
iotests.qemu_img('create', '-f', fmt, img, size)
self.files.append(img)
+
+ def create_full_backup(self, drive='drive0'):
+ res = self.vm.qmp('drive-backup', device=drive,
+ sync='full', format=iotests.imgfmt,
+ target=self.full_bak)
+ self.assert_qmp(res, 'return', {})
+ self.wait_until_completed(drive)
+ self.check_full_backup()
+ self.files.append(self.full_bak)
+
+
+ def check_full_backup(self):
+ self.assertTrue(iotests.compare_images(self.test_img, self.full_bak))
+
+
+ def add_bitmap(self, name, node='drive0'):
+ bitmap = Bitmap(name, node)
+ self.bitmaps.append(bitmap)
+ result = self.vm.qmp('block-dirty-bitmap-add', node=bitmap.node,
+ name=bitmap.name)
+ self.assert_qmp(result, 'return', {})
+ return bitmap
+
+
+ def create_incremental(self, bitmap=None, num=None,
+ parent=None, parentFormat=None, validate=True):
+ if bitmap is None:
+ bitmap = self.bitmaps[-1]
+
+ # If this is the first incremental backup for a bitmap,
+ # use the full backup as a backing image. Otherwise, use
+ # the last incremental backup.
+ if parent is None:
+ if bitmap.num == 0:
+ parent = self.full_bak
+ else:
+ parent = bitmap.last_target()
+
+ target = bitmap.new_target(num)
+ self.img_create(target, iotests.imgfmt, parent=parent)
+
+ result = self.vm.qmp('drive-backup', device=bitmap.node,
+ sync='dirty-bitmap', bitmap=bitmap.name,
+ format=iotests.imgfmt, target=target,
+ mode='existing')
+ self.assert_qmp(result, 'return', {})
+
+ event = self.wait_until_completed(bitmap.node, check_offset=validate)
+ if validate:
+ return self.check_incremental(target)
+
+
+ def check_incremental(self, target=None):
+ if target is None:
+ target = self.bitmaps[-1].last_target()
+ self.assertTrue(iotests.compare_images(self.test_img, target))
+ return True
+
+
+ def hmp_io_writes(self, drive, patterns):
+ for pattern in patterns:
+ self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern)
+ self.vm.hmp_qemu_io(drive, 'flush')
+
+
+ def test_incremental_simple(self):
+ '''
+ Test: Create and verify three incremental backups.
+
+ Create a bitmap and a full backup before VM execution begins,
+ then create a series of three incremental backups "during execution,"
+ i.e.; after IO requests begin modifying the drive.
+ '''
+ self.create_full_backup()
+ self.add_bitmap('bitmap0', 'drive0')
+
+ # Sanity: Create a "hollow" incremental backup
+ self.create_incremental()
+ # Three writes: One complete overwrite, one new segment,
+ # and one partial overlap.
+ self.hmp_io_writes('drive0', (('0xab', 0, 512),
+ ('0xfe', '16M', '256k'),
+ ('0x64', '32736k', '64k')))
+ self.create_incremental()
+ # Three more writes, one of each kind, like above
+ self.hmp_io_writes('drive0', (('0x9a', 0, 512),
+ ('0x55', '8M', '352k'),
+ ('0x78', '15872k', '1M')))
+ self.create_incremental()
+ return True
+
+
def test_sync_dirty_bitmap_missing(self):
self.assert_no_active_block_jobs()
self.files.append(self.foo_img)
diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
index fbc63e6..8d7e996 100644
--- a/tests/qemu-iotests/112.out
+++ b/tests/qemu-iotests/112.out
@@ -1,5 +1,5 @@
-..
+...
----------------------------------------------------------------------
-Ran 2 tests
+Ran 3 tests
OK
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case John Snow
@ 2015-02-16 20:36 ` Max Reitz
2015-02-20 10:02 ` Stefan Hajnoczi
1 sibling, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-02-16 20:36 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/112 | 122 +++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/112.out | 4 +-
> 2 files changed, 124 insertions(+), 2 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case John Snow
2015-02-16 20:36 ` Max Reitz
@ 2015-02-20 10:02 ` Stefan Hajnoczi
2015-02-20 14:22 ` Max Reitz
2015-02-20 16:05 ` John Snow
1 sibling, 2 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-02-20 10:02 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, armbru, qemu-devel, vsementsov, mreitz
[-- Attachment #1: Type: text/plain, Size: 894 bytes --]
On Fri, Feb 13, 2015 at 05:08:56PM -0500, John Snow wrote:
> + def check_incremental(self, target=None):
> + if target is None:
> + target = self.bitmaps[-1].last_target()
> + self.assertTrue(iotests.compare_images(self.test_img, target))
> + return True
> +
> +
> + def hmp_io_writes(self, drive, patterns):
> + for pattern in patterns:
> + self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern)
> + self.vm.hmp_qemu_io(drive, 'flush')
Please don't read image files while the guest is still running. Image
formats do not support concurrent readers and writers.
Three options:
1. Stop the guest before accessing self.test_img
2. Use drive-backup to make a full copy of self.test_img that can be
accessed.
3. Test specific patterns in the backup file instead of using
compare_images (qemu-io -c 'read -P 0xff 0 512' img)
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case
2015-02-20 10:02 ` Stefan Hajnoczi
@ 2015-02-20 14:22 ` Max Reitz
2015-02-20 16:05 ` John Snow
1 sibling, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-02-20 14:22 UTC (permalink / raw)
To: Stefan Hajnoczi, John Snow; +Cc: kwolf, famz, qemu-devel, armbru, vsementsov
On 2015-02-20 at 05:02, Stefan Hajnoczi wrote:
> On Fri, Feb 13, 2015 at 05:08:56PM -0500, John Snow wrote:
>> + def check_incremental(self, target=None):
>> + if target is None:
>> + target = self.bitmaps[-1].last_target()
>> + self.assertTrue(iotests.compare_images(self.test_img, target))
>> + return True
>> +
>> +
>> + def hmp_io_writes(self, drive, patterns):
>> + for pattern in patterns:
>> + self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern)
>> + self.vm.hmp_qemu_io(drive, 'flush')
> Please don't read image files while the guest is still running. Image
> formats do not support concurrent readers and writers.
If I'm not mistaken, there is no guest. VMs started using iotests.py
have -machine accel=qtest set.
Max
>
> Three options:
> 1. Stop the guest before accessing self.test_img
> 2. Use drive-backup to make a full copy of self.test_img that can be
> accessed.
> 3. Test specific patterns in the backup file instead of using
> compare_images (qemu-io -c 'read -P 0xff 0 512' img)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case
2015-02-20 10:02 ` Stefan Hajnoczi
2015-02-20 14:22 ` Max Reitz
@ 2015-02-20 16:05 ` John Snow
1 sibling, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-20 16:05 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, famz, armbru, qemu-devel, vsementsov, mreitz
On 02/20/2015 05:02 AM, Stefan Hajnoczi wrote:
> On Fri, Feb 13, 2015 at 05:08:56PM -0500, John Snow wrote:
>> + def check_incremental(self, target=None):
>> + if target is None:
>> + target = self.bitmaps[-1].last_target()
>> + self.assertTrue(iotests.compare_images(self.test_img, target))
>> + return True
>> +
>> +
>> + def hmp_io_writes(self, drive, patterns):
>> + for pattern in patterns:
>> + self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern)
>> + self.vm.hmp_qemu_io(drive, 'flush')
>
> Please don't read image files while the guest is still running. Image
> formats do not support concurrent readers and writers.
>
> Three options:
> 1. Stop the guest before accessing self.test_img
> 2. Use drive-backup to make a full copy of self.test_img that can be
> accessed.
> 3. Test specific patterns in the backup file instead of using
> compare_images (qemu-io -c 'read -P 0xff 0 512' img)
>
I'm not sure I understand the complication, here. There is no "running
guest," so these HMP writes are emulating the guest writes. As Max says,
we're using -qtest here.
If you really want, I can introduce emulated stop/resume commands just
for the sake of demonstration (I had them in there before) but for
iotests I don't see why it's strictly necessary.
--js
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v13 16/17] iotests: add transactional incremental backup test
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (14 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 15/17] iotests: add simple incremental backup case John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 17/17] iotests: add incremental backup failure recovery test John Snow
2015-02-20 11:09 ` [Qemu-devel] [PATCH v13 00/17] block: incremental backup series Stefan Hajnoczi
17 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/112 | 49 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/112.out | 4 ++--
2 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112
index da8f0e0..59eb1b1 100644
--- a/tests/qemu-iotests/112
+++ b/tests/qemu-iotests/112
@@ -180,6 +180,55 @@ class TestIncrementalBackup(iotests.QMPTestCase):
return True
+ def test_incremental_transaction(self):
+ '''Test: Verify backups made from transactionally created bitmaps.
+
+ Create a bitmap "before" VM execution begins, then create a second
+ bitmap AFTER writes have already occurred. Use transactions to create
+ a full backup and synchronize both bitmaps to this backup.
+ Create an incremental backup through both bitmaps and verify that
+ both backups match the full backup.
+ '''
+ bitmap0 = self.add_bitmap('bitmap0', 'drive0')
+ self.hmp_io_writes('drive0', (('0xab', 0, 512),
+ ('0xfe', '16M', '256k'),
+ ('0x64', '32736k', '64k')))
+ bitmap1 = self.add_bitmap('bitmap1', 'drive0')
+
+ result = self.vm.qmp('transaction', actions=[
+ {
+ 'type': 'block-dirty-bitmap-clear',
+ 'data': { 'node': 'drive0',
+ 'name': 'bitmap0' },
+ },
+ {
+ 'type': 'block-dirty-bitmap-clear',
+ 'data': { 'node': 'drive0',
+ 'name': 'bitmap1' },
+ },
+ {
+ 'type': 'drive-backup',
+ 'data': { 'device': 'drive0',
+ 'sync': 'full',
+ 'format': iotests.imgfmt,
+ 'target': self.full_bak },
+ }
+ ])
+ self.assert_qmp(result, 'return', {})
+ self.wait_until_completed()
+ self.files.append(self.full_bak)
+ self.check_full_backup()
+
+ self.hmp_io_writes('drive0', (('0x9a', 0, 512),
+ ('0x55', '8M', '352k'),
+ ('0x78', '15872k', '1M')))
+ # Both bitmaps should be in sync and create fully valid
+ # incremental backups
+ res1 = self.create_incremental(bitmap0)
+ res2 = self.create_incremental(bitmap1)
+ self.assertTrue(res1 and res2)
+
+
def test_sync_dirty_bitmap_missing(self):
self.assert_no_active_block_jobs()
self.files.append(self.foo_img)
diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
index 8d7e996..89968f3 100644
--- a/tests/qemu-iotests/112.out
+++ b/tests/qemu-iotests/112.out
@@ -1,5 +1,5 @@
-...
+....
----------------------------------------------------------------------
-Ran 3 tests
+Ran 4 tests
OK
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* [Qemu-devel] [PATCH v13 17/17] iotests: add incremental backup failure recovery test
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (15 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 16/17] iotests: add transactional incremental backup test John Snow
@ 2015-02-13 22:08 ` John Snow
2015-02-16 20:49 ` Max Reitz
2015-02-20 11:09 ` [Qemu-devel] [PATCH v13 00/17] block: incremental backup series Stefan Hajnoczi
17 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-13 22:08 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, famz, John Snow, armbru, mreitz, vsementsov, stefanha
To test the failure case, we modify iotests.py to allow us to specify
that we'd like to allow failures when we wait for block job events.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/112 | 57 ++++++++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/112.out | 4 +--
tests/qemu-iotests/iotests.py | 6 +++--
3 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112
index 59eb1b1..da0bf6d 100644
--- a/tests/qemu-iotests/112
+++ b/tests/qemu-iotests/112
@@ -135,7 +135,11 @@ class TestIncrementalBackup(iotests.QMPTestCase):
mode='existing')
self.assert_qmp(result, 'return', {})
- event = self.wait_until_completed(bitmap.node, check_offset=validate)
+ event = self.wait_until_completed(bitmap.node, check_offset=validate,
+ allow_failures=(not validate))
+ if 'error' in event['data']:
+ bitmap.del_target()
+ return False
if validate:
return self.check_incremental(target)
@@ -229,6 +233,57 @@ class TestIncrementalBackup(iotests.QMPTestCase):
self.assertTrue(res1 and res2)
+ def test_incremental_failure(self):
+ '''Test: Verify backups made after a failure are correct.
+
+ Simulate a failure during an incremental backup block job,
+ emulate additional writes, then create another incremental backup
+ afterwards and verify that the backup created is correct.
+ '''
+
+ # Create a blkdebug interface to this img as 'drive1'
+ result = self.vm.qmp('blockdev-add', options={
+ 'id': 'drive1',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'blkdebug',
+ 'image': {
+ 'driver': 'file',
+ 'filename': self.test_img
+ },
+ 'set-state': [{
+ 'event': 'flush_to_disk',
+ 'state': 1,
+ 'new_state': 2
+ }],
+ 'inject-error': [{
+ 'event': 'read_aio',
+ 'errno': 5,
+ 'state': 2,
+ 'immediately': False,
+ 'once': True
+ }],
+ }
+ })
+ self.assert_qmp(result, 'return', {})
+
+ self.create_full_backup()
+ self.add_bitmap('bitmap0', 'drive1')
+ # Note: at this point, during a normal execution,
+ # Assume that the VM resumes and begins issuing IO requests here.
+
+ self.hmp_io_writes('drive1', (('0xab', 0, 512),
+ ('0xfe', '16M', '256k'),
+ ('0x64', '32736k', '64k')))
+
+ result = self.create_incremental(validate=False)
+ self.assertFalse(result)
+ self.hmp_io_writes('drive1', (('0x9a', 0, 512),
+ ('0x55', '8M', '352k'),
+ ('0x78', '15872k', '1M')))
+ self.create_incremental()
+
+
def test_sync_dirty_bitmap_missing(self):
self.assert_no_active_block_jobs()
self.files.append(self.foo_img)
diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
index 89968f3..914e373 100644
--- a/tests/qemu-iotests/112.out
+++ b/tests/qemu-iotests/112.out
@@ -1,5 +1,5 @@
-....
+.....
----------------------------------------------------------------------
-Ran 4 tests
+Ran 5 tests
OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 241b5ee..cf5faac 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -258,14 +258,16 @@ class QMPTestCase(unittest.TestCase):
self.assert_no_active_block_jobs()
return result
- def wait_until_completed(self, drive='drive0', check_offset=True):
+ def wait_until_completed(self, drive='drive0', check_offset=True,
+ allow_failures=False):
'''Wait for a block job to finish, returning the event'''
completed = False
while not completed:
for event in self.vm.get_qmp_events(wait=True):
if event['event'] == 'BLOCK_JOB_COMPLETED':
self.assert_qmp(event, 'data/device', drive)
- self.assert_qmp_absent(event, 'data/error')
+ if not allow_failures:
+ self.assert_qmp_absent(event, 'data/error')
if check_offset:
self.assert_qmp(event, 'data/offset', event['data']['len'])
completed = True
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 17/17] iotests: add incremental backup failure recovery test
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 17/17] iotests: add incremental backup failure recovery test John Snow
@ 2015-02-16 20:49 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-02-16 20:49 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, famz, armbru, vsementsov, stefanha
On 2015-02-13 at 17:08, John Snow wrote:
> To test the failure case, we modify iotests.py to allow us to specify
> that we'd like to allow failures when we wait for block job events.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/112 | 57 ++++++++++++++++++++++++++++++++++++++++++-
> tests/qemu-iotests/112.out | 4 +--
> tests/qemu-iotests/iotests.py | 6 +++--
> 3 files changed, 62 insertions(+), 5 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 00/17] block: incremental backup series
2015-02-13 22:08 [Qemu-devel] [PATCH v13 00/17] block: incremental backup series John Snow
` (16 preceding siblings ...)
2015-02-13 22:08 ` [Qemu-devel] [PATCH v13 17/17] iotests: add incremental backup failure recovery test John Snow
@ 2015-02-20 11:09 ` Stefan Hajnoczi
2015-02-20 16:50 ` Kashyap Chamarthy
2015-02-20 17:20 ` John Snow
17 siblings, 2 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-02-20 11:09 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, armbru, qemu-devel, vsementsov, mreitz
[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]
On Fri, Feb 13, 2015 at 05:08:41PM -0500, John Snow wrote:
> This series requires: [PATCH v3] blkdebug: fix "once" rule
>
> Welcome to the "incremental backup" newsletter, where we discuss
> exciting developments in non-redundant backup technology.
> This issue is called the "Max Reitz Fixup" issue.
>
> This patchset enables the in-memory part of the incremental backup
> feature. There are two series on the mailing list now by Vladimir
> Sementsov-Ogievskiy that enable the migration and persistence of
> dirty bitmaps.
>
> This series and most patches were originally by Fam Zheng.
Please add docs/incremental-backup.txt explaining how the commands are
intended to be used to perform backups. The QAPI documentation is
sparse and QMP clients would probably need to read the code to
understand how these commands work.
I'm not sure I understand the need for all the commands: add, remove,
enable, disable, clear. Why are these commands necessary?
I think just add and remove should be enough:
Initial full backup
-------------------
Use transaction to add bitmaps to all drives atomically (i.e.
point-in-time snapshot across all drives) and launch drive-backup (full)
on all drives.
In your patch the bitmap starts disabled. In my example bitmaps are
always enabled unless they have a successor.
Incremental backup
------------------
Use transaction to launch drive-backup (bitmap mode) on all drives.
If backup completes successfully we'll be able to run the same command
again for the next incremental backup.
If backup fails I see a problem when taking consistent incremental
backups across multiple drives:
Imagine 2 drives (A and B). Transaction is used to launch drive-backup
on both with their respective dirty bitmaps. drive-backup A fails and
merges successor dirty bits so nothing is lost, but what do we do with
drive-backup B?
drive-backup B could be in-progress or it could have completed before
drive-backup A failed. Now we're in trouble because we need to take
consistent snapshots across all drives but we've thrown away the dirty
bitmap for drive B!
Stopping incremental backup
---------------------------
Use transaction to remove bitmaps on all drives. This case is easy.
Finally, what about bdrv_truncate()? When the disk size changes the
dirty bitmaps keep the old size. Seems likely to cause problems. Have
you thought about this case?
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 00/17] block: incremental backup series
2015-02-20 11:09 ` [Qemu-devel] [PATCH v13 00/17] block: incremental backup series Stefan Hajnoczi
@ 2015-02-20 16:50 ` Kashyap Chamarthy
2015-02-20 17:20 ` John Snow
1 sibling, 0 replies; 41+ messages in thread
From: Kashyap Chamarthy @ 2015-02-20 16:50 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, famz, armbru, qemu-devel, vsementsov, mreitz, John Snow
On Fri, Feb 20, 2015 at 11:09:20AM +0000, Stefan Hajnoczi wrote:
> On Fri, Feb 13, 2015 at 05:08:41PM -0500, John Snow wrote:
> > This series requires: [PATCH v3] blkdebug: fix "once" rule
> >
> > Welcome to the "incremental backup" newsletter, where we discuss
> > exciting developments in non-redundant backup technology.
> > This issue is called the "Max Reitz Fixup" issue.
> >
> > This patchset enables the in-memory part of the incremental backup
> > feature. There are two series on the mailing list now by Vladimir
> > Sementsov-Ogievskiy that enable the migration and persistence of
> > dirty bitmaps.
> >
> > This series and most patches were originally by Fam Zheng.
>
> Please add docs/incremental-backup.txt explaining how the commands are
> intended to be used to perform backups. The QAPI documentation is
> sparse and QMP clients would probably need to read the code to
> understand how these commands work.
Thanks for noting this, I have this series applied locally to test and
was intending to going through the iotests for initial documentation
when I actually make time to test this sometime next week :-)
--
/kashyap
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 00/17] block: incremental backup series
2015-02-20 11:09 ` [Qemu-devel] [PATCH v13 00/17] block: incremental backup series Stefan Hajnoczi
2015-02-20 16:50 ` Kashyap Chamarthy
@ 2015-02-20 17:20 ` John Snow
2015-02-23 17:52 ` Stefan Hajnoczi
1 sibling, 1 reply; 41+ messages in thread
From: John Snow @ 2015-02-20 17:20 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, famz, armbru, qemu-devel, vsementsov, mreitz
On 02/20/2015 06:09 AM, Stefan Hajnoczi wrote:
> On Fri, Feb 13, 2015 at 05:08:41PM -0500, John Snow wrote:
>> This series requires: [PATCH v3] blkdebug: fix "once" rule
>>
>> Welcome to the "incremental backup" newsletter, where we discuss
>> exciting developments in non-redundant backup technology.
>> This issue is called the "Max Reitz Fixup" issue.
>>
>> This patchset enables the in-memory part of the incremental backup
>> feature. There are two series on the mailing list now by Vladimir
>> Sementsov-Ogievskiy that enable the migration and persistence of
>> dirty bitmaps.
>>
>> This series and most patches were originally by Fam Zheng.
>
> Please add docs/incremental-backup.txt explaining how the commands are
> intended to be used to perform backups. The QAPI documentation is
> sparse and QMP clients would probably need to read the code to
> understand how these commands work.
>
OK. I wrote a markdown formatted file that explains it all pretty well;
should I check it in as-is, or should I convert it to some other format?
https://github.com/jnsnow/qemu/blob/bitmap-demo/docs/bitmaps.md
> I'm not sure I understand the need for all the commands: add, remove,
> enable, disable, clear. Why are these commands necessary?
add/remove are self explanatory.
clear allows you to re-sync a bitmap to a full backup after you have
already been running for some time. See the readme for the usage case.
Yes, you *COULD* delete and re-create the bitmap with add/remove, but why?
Clear is nice, I stand by it.
enable/disable: Not necessarily useful for the simple case at this exact
moment; they can be used to track writes during a period of time if
desired. You could use them with transactions to record activity through
different periods of time. They were originally used for what the
"frozen" case covers now, but I left them in.
I could stand to part with them if you think they detract more than
help. They seemed like useful primitives. My default action will be to
leave them in, still.
>
> I think just add and remove should be enough:
>
> Initial full backup
> -------------------
> Use transaction to add bitmaps to all drives atomically (i.e.
> point-in-time snapshot across all drives) and launch drive-backup (full)
> on all drives.
>
> In your patch the bitmap starts disabled. In my example bitmaps are
> always enabled unless they have a successor.
No they don't:
bitmap->disabled = false;
>
> Incremental backup
> ------------------
> Use transaction to launch drive-backup (bitmap mode) on all drives.
>
> If backup completes successfully we'll be able to run the same command
> again for the next incremental backup.
>
> If backup fails I see a problem when taking consistent incremental
> backups across multiple drives:
>
> Imagine 2 drives (A and B). Transaction is used to launch drive-backup
> on both with their respective dirty bitmaps. drive-backup A fails and
> merges successor dirty bits so nothing is lost, but what do we do with
> drive-backup B?
>
> drive-backup B could be in-progress or it could have completed before
> drive-backup A failed. Now we're in trouble because we need to take
> consistent snapshots across all drives but we've thrown away the dirty
> bitmap for drive B!
Just re-run the transaction. If one failed but one succeeded, just run
it again. The one that succeeded prior will now have a trivial
incremental backup, and the one that failed will have a new valid
incremental backup. The two new incrementals will be point in time
consistent.
E.G.:
[full_a] <-- [incremental_a_0: FAILED]
[full_b] <-- [incremental_b_0: SUCCESS]
then re-run:
[full_a] <------------------------ [incremental_a_1]
[full_b] <-- [incremental_b_0] <-- [incremental_b_1]
If the extra image in the chain for the one that succeeded is
problematic, you can use other tools to consolidate them later.
Either way, how do you propose getting a consistent snapshot across
multiple drives after a failure? The only recovery option is to just
create a new snapshot *later*, you can never go back to what was, just
like you can't for single drives.
libvirt can tell which portions of the transaction ultimately succeeded
by the BLOCK_JOB_COMPLETED events that it receives back, one per each drive.
For drives that complete successfully, it can make a mental note that it
has a new proper incremental. For drives that do not, it can make a note
that it needs to try for a new consistent drives-wide incremental,
erasing the half-baked attempt that got created last time.
I am not convinced there is a problem. Since we don't want filename
management (&c) as part of this solution inside of QEMU, there is
nothing left to do except within libvirt.
>
> Stopping incremental backup
> ---------------------------
> Use transaction to remove bitmaps on all drives. This case is easy.
>
> Finally, what about bdrv_truncate()? When the disk size changes the
> dirty bitmaps keep the old size. Seems likely to cause problems. Have
> you thought about this case?
>
Only just recently when reviewing Vladimir's bitmap persistence patches,
actually.
> Stefan
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [Qemu-devel] [PATCH v13 00/17] block: incremental backup series
2015-02-20 17:20 ` John Snow
@ 2015-02-23 17:52 ` Stefan Hajnoczi
2015-02-23 19:16 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-02-23 17:52 UTC (permalink / raw)
To: John Snow
Cc: kwolf, famz, armbru, qemu-devel, vsementsov, Stefan Hajnoczi, mreitz
[-- Attachment #1: Type: text/plain, Size: 7303 bytes --]
On Fri, Feb 20, 2015 at 12:20:53PM -0500, John Snow wrote:
Eric: I've added you on CC for a libvirt perspective on the APIs that
this series introduces. We're discussing how incremental backup
failures should be handled when there are multiple drives attached to
the guest. Please see towards the bottom of this email.
> On 02/20/2015 06:09 AM, Stefan Hajnoczi wrote:
> >On Fri, Feb 13, 2015 at 05:08:41PM -0500, John Snow wrote:
> >>This series requires: [PATCH v3] blkdebug: fix "once" rule
> >>
> >>Welcome to the "incremental backup" newsletter, where we discuss
> >>exciting developments in non-redundant backup technology.
> >>This issue is called the "Max Reitz Fixup" issue.
> >>
> >>This patchset enables the in-memory part of the incremental backup
> >>feature. There are two series on the mailing list now by Vladimir
> >>Sementsov-Ogievskiy that enable the migration and persistence of
> >>dirty bitmaps.
> >>
> >>This series and most patches were originally by Fam Zheng.
> >
> >Please add docs/incremental-backup.txt explaining how the commands are
> >intended to be used to perform backups. The QAPI documentation is
> >sparse and QMP clients would probably need to read the code to
> >understand how these commands work.
> >
>
> OK. I wrote a markdown formatted file that explains it all pretty well;
> should I check it in as-is, or should I convert it to some other format?
>
> https://github.com/jnsnow/qemu/blob/bitmap-demo/docs/bitmaps.md
Thanks, that is helpful. The multi-disk use case is most complex,
please include it.
Please submit to docs/ as plain text.
> >I'm not sure I understand the need for all the commands: add, remove,
> >enable, disable, clear. Why are these commands necessary?
>
> add/remove are self explanatory.
>
> clear allows you to re-sync a bitmap to a full backup after you have already
> been running for some time. See the readme for the usage case. Yes, you
> *COULD* delete and re-create the bitmap with add/remove, but why?
> Clear is nice, I stand by it.
>
> enable/disable: Not necessarily useful for the simple case at this exact
> moment; they can be used to track writes during a period of time if desired.
> You could use them with transactions to record activity through different
> periods of time. They were originally used for what the "frozen" case covers
> now, but I left them in.
>
> I could stand to part with them if you think they detract more than help.
> They seemed like useful primitives. My default action will be to leave them
> in, still.
>
> >
> >I think just add and remove should be enough:
> >
> >Initial full backup
> >-------------------
> >Use transaction to add bitmaps to all drives atomically (i.e.
> >point-in-time snapshot across all drives) and launch drive-backup (full)
> >on all drives.
> >
> >In your patch the bitmap starts disabled. In my example bitmaps are
> >always enabled unless they have a successor.
>
> No they don't:
>
> bitmap->disabled = false;
Great, I missed that. In that case enable/disable are truly optional.
They can be added in the future without needing to change the 'add'
command's default.
All code qemu.git requires code review, testing, maintenance, and in the
case of QMP, a commitment to keep the command forever. Please drop
enable/disable for now.
> >
> >Incremental backup
> >------------------
> >Use transaction to launch drive-backup (bitmap mode) on all drives.
> >
> >If backup completes successfully we'll be able to run the same command
> >again for the next incremental backup.
> >
> >If backup fails I see a problem when taking consistent incremental
> >backups across multiple drives:
> >
> >Imagine 2 drives (A and B). Transaction is used to launch drive-backup
> >on both with their respective dirty bitmaps. drive-backup A fails and
> >merges successor dirty bits so nothing is lost, but what do we do with
> >drive-backup B?
> >
> >drive-backup B could be in-progress or it could have completed before
> >drive-backup A failed. Now we're in trouble because we need to take
> >consistent snapshots across all drives but we've thrown away the dirty
> >bitmap for drive B!
>
> Just re-run the transaction. If one failed but one succeeded, just run it
> again. The one that succeeded prior will now have a trivial incremental
> backup, and the one that failed will have a new valid incremental backup.
> The two new incrementals will be point in time consistent.
>
> E.G.:
>
> [full_a] <-- [incremental_a_0: FAILED]
> [full_b] <-- [incremental_b_0: SUCCESS]
>
> then re-run:
>
> [full_a] <------------------------ [incremental_a_1]
> [full_b] <-- [incremental_b_0] <-- [incremental_b_1]
>
> If the extra image in the chain for the one that succeeded is problematic,
> you can use other tools to consolidate them later.
Each client implementation needs to duplicate the logic for partial
backups. There can be arbitrary levels of partial backups since failure
could occur twice, three times, etc in a row. We're forcing all clients
to deal with failure themselves or to fall back to full backup on
failure.
The "client" may be a backup application that wants QEMU to send data
directly to a NBD server. In order for that to work, the backup
application also needs to keep state or fall back to full backup on any
error.
In other words, making the API require a stateful client is ugly.
> Either way, how do you propose getting a consistent snapshot across multiple
> drives after a failure? The only recovery option is to just create a new
> snapshot *later*, you can never go back to what was, just like you can't for
> single drives.
This is the same problem as for a single drive. There the solution is
to merge the old and new bitmaps, and the client has to take a later
snapshot like you explained.
That policy is fine and should apply to multiple drives too. Take a new
snapshot if there was a failure.
> I am not convinced there is a problem. Since we don't want filename
> management (&c) as part of this solution inside of QEMU, there is nothing
> left to do except within libvirt.
No filenames are involved. Just don't throw away the frozen dirty
bitmap until the entire group of backup operations completes. It's
equivalent to what we do in the single drive case.
> >Stopping incremental backup
> >---------------------------
> >Use transaction to remove bitmaps on all drives. This case is easy.
> >
> >Finally, what about bdrv_truncate()? When the disk size changes the
> >dirty bitmaps keep the old size. Seems likely to cause problems. Have
> >you thought about this case?
> >
>
> Only just recently when reviewing Vladimir's bitmap persistence patches,
> actually.
This patch series needs to address the issue since the QMP API to create
bitmaps is exposed here.
The drive cannot be resized during the backup operation. Luckily we're
already protected against that by the op blocker API.
The question is what happens to existing bitmaps (enabled, disabled, or
frozen) when there is no backup blockjob running and the drive is
resized. I think it makes sense to resize the bitmaps along with the
drive.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v13 00/17] block: incremental backup series
2015-02-23 17:52 ` Stefan Hajnoczi
@ 2015-02-23 19:16 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-02-23 19:16 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, famz, armbru, qemu-devel, vsementsov, Stefan Hajnoczi, mreitz
On 02/23/2015 12:52 PM, Stefan Hajnoczi wrote:
> On Fri, Feb 20, 2015 at 12:20:53PM -0500, John Snow wrote:
>
> Eric: I've added you on CC for a libvirt perspective on the APIs that
> this series introduces. We're discussing how incremental backup
> failures should be handled when there are multiple drives attached to
> the guest. Please see towards the bottom of this email.
>
>> On 02/20/2015 06:09 AM, Stefan Hajnoczi wrote:
>>> On Fri, Feb 13, 2015 at 05:08:41PM -0500, John Snow wrote:
>>>> This series requires: [PATCH v3] blkdebug: fix "once" rule
>>>>
>>>> Welcome to the "incremental backup" newsletter, where we discuss
>>>> exciting developments in non-redundant backup technology.
>>>> This issue is called the "Max Reitz Fixup" issue.
>>>>
>>>> This patchset enables the in-memory part of the incremental backup
>>>> feature. There are two series on the mailing list now by Vladimir
>>>> Sementsov-Ogievskiy that enable the migration and persistence of
>>>> dirty bitmaps.
>>>>
>>>> This series and most patches were originally by Fam Zheng.
>>>
>>> Please add docs/incremental-backup.txt explaining how the commands are
>>> intended to be used to perform backups. The QAPI documentation is
>>> sparse and QMP clients would probably need to read the code to
>>> understand how these commands work.
>>>
>>
>> OK. I wrote a markdown formatted file that explains it all pretty well;
>> should I check it in as-is, or should I convert it to some other format?
>>
>> https://github.com/jnsnow/qemu/blob/bitmap-demo/docs/bitmaps.md
>
> Thanks, that is helpful. The multi-disk use case is most complex,
> please include it.
>
> Please submit to docs/ as plain text.
>
>>> I'm not sure I understand the need for all the commands: add, remove,
>>> enable, disable, clear. Why are these commands necessary?
>>
>> add/remove are self explanatory.
>>
>> clear allows you to re-sync a bitmap to a full backup after you have already
>> been running for some time. See the readme for the usage case. Yes, you
>> *COULD* delete and re-create the bitmap with add/remove, but why?
>> Clear is nice, I stand by it.
>>
>> enable/disable: Not necessarily useful for the simple case at this exact
>> moment; they can be used to track writes during a period of time if desired.
>> You could use them with transactions to record activity through different
>> periods of time. They were originally used for what the "frozen" case covers
>> now, but I left them in.
>>
>> I could stand to part with them if you think they detract more than help.
>> They seemed like useful primitives. My default action will be to leave them
>> in, still.
>>
>>>
>>> I think just add and remove should be enough:
>>>
>>> Initial full backup
>>> -------------------
>>> Use transaction to add bitmaps to all drives atomically (i.e.
>>> point-in-time snapshot across all drives) and launch drive-backup (full)
>>> on all drives.
>>>
>>> In your patch the bitmap starts disabled. In my example bitmaps are
>>> always enabled unless they have a successor.
>>
>> No they don't:
>>
>> bitmap->disabled = false;
>
> Great, I missed that. In that case enable/disable are truly optional.
> They can be added in the future without needing to change the 'add'
> command's default.
>
I will remove the interface, but the status will remain. Vladimir uses
the read-only mode for migrations.
> All code qemu.git requires code review, testing, maintenance, and in the
> case of QMP, a commitment to keep the command forever. Please drop
> enable/disable for now.
>
>>>
>>> Incremental backup
>>> ------------------
>>> Use transaction to launch drive-backup (bitmap mode) on all drives.
>>>
>>> If backup completes successfully we'll be able to run the same command
>>> again for the next incremental backup.
>>>
>>> If backup fails I see a problem when taking consistent incremental
>>> backups across multiple drives:
>>>
>>> Imagine 2 drives (A and B). Transaction is used to launch drive-backup
>>> on both with their respective dirty bitmaps. drive-backup A fails and
>>> merges successor dirty bits so nothing is lost, but what do we do with
>>> drive-backup B?
>>>
>>> drive-backup B could be in-progress or it could have completed before
>>> drive-backup A failed. Now we're in trouble because we need to take
>>> consistent snapshots across all drives but we've thrown away the dirty
>>> bitmap for drive B!
>>
>> Just re-run the transaction. If one failed but one succeeded, just run it
>> again. The one that succeeded prior will now have a trivial incremental
>> backup, and the one that failed will have a new valid incremental backup.
>> The two new incrementals will be point in time consistent.
>>
>> E.G.:
>>
>> [full_a] <-- [incremental_a_0: FAILED]
>> [full_b] <-- [incremental_b_0: SUCCESS]
>>
>> then re-run:
>>
>> [full_a] <------------------------ [incremental_a_1]
>> [full_b] <-- [incremental_b_0] <-- [incremental_b_1]
>>
>> If the extra image in the chain for the one that succeeded is problematic,
>> you can use other tools to consolidate them later.
>
> Each client implementation needs to duplicate the logic for partial
> backups. There can be arbitrary levels of partial backups since failure
> could occur twice, three times, etc in a row. We're forcing all clients
> to deal with failure themselves or to fall back to full backup on
> failure.
>
> The "client" may be a backup application that wants QEMU to send data
> directly to a NBD server. In order for that to work, the backup
> application also needs to keep state or fall back to full backup on any
> error.
>
> In other words, making the API require a stateful client is ugly.
>
Every interface is beautiful in its own special way.
>> Either way, how do you propose getting a consistent snapshot across multiple
>> drives after a failure? The only recovery option is to just create a new
>> snapshot *later*, you can never go back to what was, just like you can't for
>> single drives.
>
> This is the same problem as for a single drive. There the solution is
> to merge the old and new bitmaps, and the client has to take a later
> snapshot like you explained.
>
> That policy is fine and should apply to multiple drives too. Take a new
> snapshot if there was a failure.
>
OK; I will need to cook something up to allow us to run another action
on all the drive backups after they all complete...
>> I am not convinced there is a problem. Since we don't want filename
>> management (&c) as part of this solution inside of QEMU, there is nothing
>> left to do except within libvirt.
>
> No filenames are involved. Just don't throw away the frozen dirty
> bitmap until the entire group of backup operations completes. It's
> equivalent to what we do in the single drive case.
>
>>> Stopping incremental backup
>>> ---------------------------
>>> Use transaction to remove bitmaps on all drives. This case is easy.
>>>
>>> Finally, what about bdrv_truncate()? When the disk size changes the
>>> dirty bitmaps keep the old size. Seems likely to cause problems. Have
>>> you thought about this case?
>>>
>>
>> Only just recently when reviewing Vladimir's bitmap persistence patches,
>> actually.
>
> This patch series needs to address the issue since the QMP API to create
> bitmaps is exposed here.
>
> The drive cannot be resized during the backup operation. Luckily we're
> already protected against that by the op blocker API.
>
> The question is what happens to existing bitmaps (enabled, disabled, or
> frozen) when there is no backup blockjob running and the drive is
> resized. I think it makes sense to resize the bitmaps along with the
> drive.
>
> Stefan
>
It should not be possible to resize a frozen drive, since they only
freeze during an operation.
I agree that read-only bitmaps should just go ahead and perform the
resize, however.
^ permalink raw reply [flat|nested] 41+ messages in thread