* [Qemu-devel] [2.3 PATCH v7 01/10] qapi: Add optional field "name" to block dirty bitmap
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 11:22 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
` (9 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
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>
---
block-migration.c | 2 +-
block.c | 32 +++++++++++++++++++++++++++++++-
block/mirror.c | 2 +-
include/block/block.h | 7 ++++++-
qapi/block-core.json | 4 +++-
5 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/block-migration.c b/block-migration.c
index 08db01a..6f3aa18 100644
--- a/block-migration.c
+++ b/block-migration.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/block.c b/block.c
index a612594..f94b753 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
+ char *name;
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -5291,7 +5292,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;
@@ -5299,6 +5321,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);
@@ -5309,6 +5335,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;
}
@@ -5320,6 +5347,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;
}
@@ -5338,6 +5366,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 2c6dd2a..858e4ff 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -699,7 +699,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 5450610..977f7b5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -428,8 +428,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/qapi/block-core.json b/qapi/block-core.json
index a14e6ab..569c9f5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -304,6 +304,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)
@@ -311,7 +313,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] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 01/10] qapi: Add optional field "name" to block dirty bitmap
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 01/10] qapi: Add optional field "name" to block dirty bitmap John Snow
@ 2014-11-26 11:22 ` Max Reitz
0 siblings, 0 replies; 27+ messages in thread
From: Max Reitz @ 2014-11-26 11:22 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, 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>
> ---
> block-migration.c | 2 +-
> block.c | 32 +++++++++++++++++++++++++++++++-
> block/mirror.c | 2 +-
> include/block/block.h | 7 ++++++-
> qapi/block-core.json | 4 +++-
> 5 files changed, 42 insertions(+), 5 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 01/10] qapi: Add optional field "name" to block dirty bitmap John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 12:19 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 03/10] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
` (8 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
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 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: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 19 ++++++++++++++++
block/mirror.c | 10 +-------
blockdev.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 +
qapi/block-core.json | 58 +++++++++++++++++++++++++++++++++++++++++++++++
qmp-commands.hx | 49 +++++++++++++++++++++++++++++++++++++++
6 files changed, 191 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index f94b753..a940345 100644
--- a/block.c
+++ b/block.c
@@ -5385,6 +5385,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
}
}
+#define BDB_MIN_DEF_GRANULARITY 4096
+#define BDB_MAX_DEF_GRANULARITY 65536
+#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
+
+int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
+{
+ BlockDriverInfo bdi;
+ int64_t granularity;
+
+ if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
+ granularity = MAX(BDB_MIN_DEF_GRANULARITY, bdi.cluster_size);
+ granularity = MIN(BDB_MAX_DEF_GRANULARITY, granularity);
+ } else {
+ granularity = BDB_DEFAULT_GRANULARITY;
+ }
+
+ return granularity;
+}
+
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
{
diff --git a/block/mirror.c b/block/mirror.c
index 858e4ff..3633632 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -664,15 +664,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_dbm_calc_def_granularity(target);
}
assert ((granularity & (granularity - 1)) == 0);
diff --git a/blockdev.c b/blockdev.c
index 57910b8..e2fe687 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1810,6 +1810,69 @@ 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 *device, const char *name,
+ bool has_granularity, int64_t granularity,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ Error *local_err = NULL;
+
+ if (!device) {
+ error_setg(errp, "Device to add dirty bitmap to must not be null");
+ return;
+ }
+
+ bs = bdrv_lookup_bs(device, NULL, &local_err);
+ if (!bs) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ if (!name || name[0] == '\0') {
+ error_setg(errp, "Bitmap name cannot be empty");
+ return;
+ }
+ if (has_granularity) {
+ if (granularity < 512 || !is_power_of_2(granularity)) {
+ error_setg(errp, "Granularity must be power of 2 "
+ "and at least 512");
+ return;
+ }
+ } else {
+ /* Default to cluster size, if available: */
+ granularity = bdrv_dbm_calc_def_granularity(bs);
+ }
+
+ bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+}
+
+void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ Error *local_err = NULL;
+
+ bs = bdrv_lookup_bs(device, NULL, &local_err);
+ if (!bs) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ if (!name || name[0] == '\0') {
+ error_setg(errp, "Bitmap name cannot be empty");
+ return;
+ }
+ bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Dirty bitmap not found: %s", name);
+ return;
+ }
+
+ bdrv_dirty_bitmap_make_anon(bs, bitmap);
+ bdrv_release_dirty_bitmap(bs, bitmap);
+}
+
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 977f7b5..feb84e2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -437,6 +437,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);
+int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 569c9f5..53daf49 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -864,6 +864,64 @@
'*on-target-error': 'BlockdevOnError' } }
##
+# @BlockDirtyBitmap
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmap',
+ 'data': { 'device': 'str', 'name': 'str' } }
+
+##
+# @BlockDirtyBitmapAdd
+#
+# @device: name of device 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',
+# 'base': 'BlockDirtyBitmap',
+# 'data': { '*granularity': 'int' } }
+{ 'type': 'BlockDirtyBitmapAdd',
+ 'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }
+
+##
+# @block-dirty-bitmap-add
+#
+# Create a dirty bitmap with a name on the device
+#
+# Returns: nothing on success
+# If @device is not a valid block device, 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 device
+#
+# Returns: nothing on success
+# If @device is not a valid block device, DeviceNotFound
+# If @name is not found, GenericError with an explaining message
+#
+# 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 8812401..5602dcf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1202,6 +1202,55 @@ Example:
EQMP
{
+ .name = "block-dirty-bitmap-add",
+ .args_type = "device:B,name:s,granularity:i?",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
+ },
+ {
+ .name = "block-dirty-bitmap-remove",
+ .args_type = "device:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
+ },
+
+SQMP
+
+block-dirty-bitmap-add
+----------------------
+
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+
+Arguments:
+
+- "device": device name to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with (int)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
+ "name": "bitmap0" } }
+<- { "return": {} }
+
+block-dirty-bitmap-remove
+-------------------------
+
+Stop write tracking and remove the dirty bitmap that was created with
+block-dirty-bitmap-add.
+
+Arguments:
+
+- "device": device name to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-remove", "arguments": { "device": "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] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
@ 2014-11-26 12:19 ` Max Reitz
2014-11-26 15:39 ` John Snow
0 siblings, 1 reply; 27+ messages in thread
From: Max Reitz @ 2014-11-26 12:19 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> 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 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}'
Thanks, this helps. :-)
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 19 ++++++++++++++++
> block/mirror.c | 10 +-------
> blockdev.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 1 +
> qapi/block-core.json | 58 +++++++++++++++++++++++++++++++++++++++++++++++
> qmp-commands.hx | 49 +++++++++++++++++++++++++++++++++++++++
> 6 files changed, 191 insertions(+), 9 deletions(-)
>
> diff --git a/block.c b/block.c
> index f94b753..a940345 100644
> --- a/block.c
> +++ b/block.c
> @@ -5385,6 +5385,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
> }
> }
>
> +#define BDB_MIN_DEF_GRANULARITY 4096
> +#define BDB_MAX_DEF_GRANULARITY 65536
> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
You mean this is the default for the default? ;-)
> +
> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
You may want to make this a uint64_t so it's clear that this function
does not return errors.
> +{
> + BlockDriverInfo bdi;
> + int64_t granularity;
> +
> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
> + granularity = MAX(BDB_MIN_DEF_GRANULARITY, bdi.cluster_size);
> + granularity = MIN(BDB_MAX_DEF_GRANULARITY, granularity);
> + } else {
> + granularity = BDB_DEFAULT_GRANULARITY;
> + }
> +
> + return granularity;
> +}
> +
> void bdrv_dirty_iter_init(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
> {
> diff --git a/block/mirror.c b/block/mirror.c
> index 858e4ff..3633632 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -664,15 +664,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_dbm_calc_def_granularity(target);
Maybe you should note this replacement in the commit message.
> }
>
> assert ((granularity & (granularity - 1)) == 0);
> diff --git a/blockdev.c b/blockdev.c
> index 57910b8..e2fe687 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1810,6 +1810,69 @@ 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 *device, const char *name,
> + bool has_granularity, int64_t granularity,
> + Error **errp)
> +{
> + BlockDriverState *bs;
> + Error *local_err = NULL;
> +
> + if (!device) {
> + error_setg(errp, "Device to add dirty bitmap to must not be null");
> + return;
> + }
I don't know if checking for that case makes sense, but of course it
won't hurt. But...[1]
> +
> + bs = bdrv_lookup_bs(device, NULL, &local_err);
Fair enough, I'd still like blk_by_name() and blk_bs() more
(bdrv_lookup_bs() uses blk_bs(blk_by_name()) for the device name just as
bdrv_find() did), but this is at least not a completely trivial wrapper.
> + if (!bs) {
> + error_propagate(errp, local_err);
Simply calling bdrv_lookup_bs(device, NULL, errp); suffices, no need for
a local Error object and error_propagate(). But I'm fine with it either way.
> + return;
> + }
> +
> + if (!name || name[0] == '\0') {
> + error_setg(errp, "Bitmap name cannot be empty");
> + return;
> + }
> + if (has_granularity) {
> + if (granularity < 512 || !is_power_of_2(granularity)) {
> + error_setg(errp, "Granularity must be power of 2 "
> + "and at least 512");
> + return;
> + }
> + } else {
> + /* Default to cluster size, if available: */
> + granularity = bdrv_dbm_calc_def_granularity(bs);
> + }
> +
> + bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> + Error **errp)
> +{
> + BlockDriverState *bs;
> + BdrvDirtyBitmap *bitmap;
> + Error *local_err = NULL;
[1] why aren't you minding !device here?
> +
> + bs = bdrv_lookup_bs(device, NULL, &local_err);
> + if (!bs) {
> + error_propagate(errp, local_err);
Same thing about error_propagate() here.
> + return;
> + }
> +
> + if (!name || name[0] == '\0') {
> + error_setg(errp, "Bitmap name cannot be empty");
> + return;
> + }
> + bitmap = bdrv_find_dirty_bitmap(bs, name);
> + if (!bitmap) {
> + error_setg(errp, "Dirty bitmap not found: %s", name);
> + return;
> + }
> +
> + bdrv_dirty_bitmap_make_anon(bs, bitmap);
> + bdrv_release_dirty_bitmap(bs, bitmap);
> +}
> +
> 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 977f7b5..feb84e2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -437,6 +437,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);
> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 569c9f5..53daf49 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -864,6 +864,64 @@
> '*on-target-error': 'BlockdevOnError' } }
>
> ##
> +# @BlockDirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> + 'data': { 'device': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @device: name of device 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',
> +# 'base': 'BlockDirtyBitmap',
> +# 'data': { '*granularity': 'int' } }
This part of the comment doesn't seem right...
If you left it on purpose, you should add a comment like "XXX: Should
use this representation after the code generator has been fixed to make
it work".
Max
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2014-11-26 12:19 ` Max Reitz
@ 2014-11-26 15:39 ` John Snow
2014-11-26 15:53 ` Max Reitz
2014-11-27 9:16 ` Markus Armbruster
0 siblings, 2 replies; 27+ messages in thread
From: John Snow @ 2014-11-26 15:39 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: Fam Zheng
On 11/26/2014 07:19 AM, Max Reitz wrote:
> On 2014-11-25 at 20:46, John Snow wrote:
>> From: Fam Zheng <famz@redhat.com>
>>
>> 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 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}'
>
> Thanks, this helps. :-)
>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block.c | 19 ++++++++++++++++
>> block/mirror.c | 10 +-------
>> blockdev.c | 63
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/block/block.h | 1 +
>> qapi/block-core.json | 58
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> qmp-commands.hx | 49 +++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 191 insertions(+), 9 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index f94b753..a940345 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5385,6 +5385,25 @@ int bdrv_get_dirty(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap, int64_t sector
>> }
>> }
>> +#define BDB_MIN_DEF_GRANULARITY 4096
>> +#define BDB_MAX_DEF_GRANULARITY 65536
>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>
> You mean this is the default for the default? ;-)
>
>> +
>> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>
> You may want to make this a uint64_t so it's clear that this function
> does not return errors.
>
>> +{
>> + BlockDriverInfo bdi;
>> + int64_t granularity;
>> +
>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
>> + granularity = MAX(BDB_MIN_DEF_GRANULARITY, bdi.cluster_size);
>> + granularity = MIN(BDB_MAX_DEF_GRANULARITY, granularity);
>> + } else {
>> + granularity = BDB_DEFAULT_GRANULARITY;
>> + }
>> +
>> + return granularity;
>> +}
>> +
>> void bdrv_dirty_iter_init(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>> {
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 858e4ff..3633632 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -664,15 +664,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_dbm_calc_def_granularity(target);
>
> Maybe you should note this replacement in the commit message.
>
>> }
>> assert ((granularity & (granularity - 1)) == 0);
>> diff --git a/blockdev.c b/blockdev.c
>> index 57910b8..e2fe687 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1810,6 +1810,69 @@ 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 *device, const char *name,
>> + bool has_granularity, int64_t
>> granularity,
>> + Error **errp)
>> +{
>> + BlockDriverState *bs;
>> + Error *local_err = NULL;
>> +
>> + if (!device) {
>> + error_setg(errp, "Device to add dirty bitmap to must not be
>> null");
>> + return;
>> + }
>
> I don't know if checking for that case makes sense, but of course it
> won't hurt. But...[1]
>
>> +
>> + bs = bdrv_lookup_bs(device, NULL, &local_err);
>
> Fair enough, I'd still like blk_by_name() and blk_bs() more
> (bdrv_lookup_bs() uses blk_bs(blk_by_name()) for the device name just as
> bdrv_find() did), but this is at least not a completely trivial wrapper.
>
>> + if (!bs) {
>> + error_propagate(errp, local_err);
>
> Simply calling bdrv_lookup_bs(device, NULL, errp); suffices, no need for
> a local Error object and error_propagate(). But I'm fine with it either
> way.
I found other cases where we do this in the code, I actually thought it
was a "style thing," which is why I did it so often.
I'll happily cut it out.
>> + return;
>> + }
>> +
>> + if (!name || name[0] == '\0') {
>> + error_setg(errp, "Bitmap name cannot be empty");
>> + return;
>> + }
>> + if (has_granularity) {
>> + if (granularity < 512 || !is_power_of_2(granularity)) {
>> + error_setg(errp, "Granularity must be power of 2 "
>> + "and at least 512");
>> + return;
>> + }
>> + } else {
>> + /* Default to cluster size, if available: */
>> + granularity = bdrv_dbm_calc_def_granularity(bs);
>> + }
>> +
>> + bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>> +}
>> +
>> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
>> + Error **errp)
>> +{
>> + BlockDriverState *bs;
>> + BdrvDirtyBitmap *bitmap;
>> + Error *local_err = NULL;
>
> [1] why aren't you minding !device here?
Trusting lookup to error out.
>> +
>> + bs = bdrv_lookup_bs(device, NULL, &local_err);
>> + if (!bs) {
>> + error_propagate(errp, local_err);
>
> Same thing about error_propagate() here.
Same.
>> + return;
>> + }
>> +
>> + if (!name || name[0] == '\0') {
>> + error_setg(errp, "Bitmap name cannot be empty");
>> + return;
>> + }
>> + bitmap = bdrv_find_dirty_bitmap(bs, name);
>> + if (!bitmap) {
>> + error_setg(errp, "Dirty bitmap not found: %s", name);
>> + return;
>> + }
>> +
>> + bdrv_dirty_bitmap_make_anon(bs, bitmap);
>> + bdrv_release_dirty_bitmap(bs, bitmap);
>> +}
>> +
>> 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 977f7b5..feb84e2 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -437,6 +437,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);
>> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
>> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>> int64_t sector);
>> void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int
>> nr_sectors);
>> void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int
>> nr_sectors);
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 569c9f5..53daf49 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -864,6 +864,64 @@
>> '*on-target-error': 'BlockdevOnError' } }
>> ##
>> +# @BlockDirtyBitmap
>> +#
>> +# @device: name of device which the bitmap is tracking
>> +#
>> +# @name: name of the dirty bitmap
>> +#
>> +# Since 2.3
>> +##
>> +{ 'type': 'BlockDirtyBitmap',
>> + 'data': { 'device': 'str', 'name': 'str' } }
>> +
>> +##
>> +# @BlockDirtyBitmapAdd
>> +#
>> +# @device: name of device 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',
>> +# 'base': 'BlockDirtyBitmap',
>> +# 'data': { '*granularity': 'int' } }
>
> This part of the comment doesn't seem right...
>
> If you left it on purpose, you should add a comment like "XXX: Should
> use this representation after the code generator has been fixed to make
> it work".
>
> Max
No, just another bonehead moment.
Thanks for the reviews.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2014-11-26 15:39 ` John Snow
@ 2014-11-26 15:53 ` Max Reitz
2014-11-27 9:16 ` Markus Armbruster
1 sibling, 0 replies; 27+ messages in thread
From: Max Reitz @ 2014-11-26 15:53 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-26 at 16:39, John Snow wrote:
>
>
> On 11/26/2014 07:19 AM, Max Reitz wrote:
>> On 2014-11-25 at 20:46, John Snow wrote:
>>> From: Fam Zheng <famz@redhat.com>
>>>
>>> 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 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}'
>>
>> Thanks, this helps. :-)
>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block.c | 19 ++++++++++++++++
>>> block/mirror.c | 10 +-------
>>> blockdev.c | 63
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/block/block.h | 1 +
>>> qapi/block-core.json | 58
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>> qmp-commands.hx | 49 +++++++++++++++++++++++++++++++++++++++
>>> 6 files changed, 191 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index f94b753..a940345 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -5385,6 +5385,25 @@ int bdrv_get_dirty(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap, int64_t sector
>>> }
>>> }
>>> +#define BDB_MIN_DEF_GRANULARITY 4096
>>> +#define BDB_MAX_DEF_GRANULARITY 65536
>>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>>
>> You mean this is the default for the default? ;-)
>>
>>> +
>>> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>>
>> You may want to make this a uint64_t so it's clear that this function
>> does not return errors.
>>
>>> +{
>>> + BlockDriverInfo bdi;
>>> + int64_t granularity;
>>> +
>>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
>>> + granularity = MAX(BDB_MIN_DEF_GRANULARITY, bdi.cluster_size);
>>> + granularity = MIN(BDB_MAX_DEF_GRANULARITY, granularity);
>>> + } else {
>>> + granularity = BDB_DEFAULT_GRANULARITY;
>>> + }
>>> +
>>> + return granularity;
>>> +}
>>> +
>>> void bdrv_dirty_iter_init(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>>> {
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 858e4ff..3633632 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -664,15 +664,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_dbm_calc_def_granularity(target);
>>
>> Maybe you should note this replacement in the commit message.
>>
>>> }
>>> assert ((granularity & (granularity - 1)) == 0);
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 57910b8..e2fe687 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1810,6 +1810,69 @@ 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 *device, const char *name,
>>> + bool has_granularity, int64_t
>>> granularity,
>>> + Error **errp)
>>> +{
>>> + BlockDriverState *bs;
>>> + Error *local_err = NULL;
>>> +
>>> + if (!device) {
>>> + error_setg(errp, "Device to add dirty bitmap to must not be
>>> null");
>>> + return;
>>> + }
>>
>> I don't know if checking for that case makes sense, but of course it
>> won't hurt. But...[1]
>>
>>> +
>>> + bs = bdrv_lookup_bs(device, NULL, &local_err);
>>
>> Fair enough, I'd still like blk_by_name() and blk_bs() more
>> (bdrv_lookup_bs() uses blk_bs(blk_by_name()) for the device name just as
>> bdrv_find() did), but this is at least not a completely trivial wrapper.
>>
>>> + if (!bs) {
>>> + error_propagate(errp, local_err);
>>
>> Simply calling bdrv_lookup_bs(device, NULL, errp); suffices, no need for
>> a local Error object and error_propagate(). But I'm fine with it either
>> way.
>
> I found other cases where we do this in the code, I actually thought
> it was a "style thing," which is why I did it so often.
See my reply to patch 8 for an explanation.
I still think we could have made nice use of error_propagate() for
showing some kind of backtrace (with a certain configure option), but
that idea was rejected...
> I'll happily cut it out.
>
>>> + return;
>>> + }
>>> +
>>> + if (!name || name[0] == '\0') {
>>> + error_setg(errp, "Bitmap name cannot be empty");
>>> + return;
>>> + }
>>> + if (has_granularity) {
>>> + if (granularity < 512 || !is_power_of_2(granularity)) {
>>> + error_setg(errp, "Granularity must be power of 2 "
>>> + "and at least 512");
>>> + return;
>>> + }
>>> + } else {
>>> + /* Default to cluster size, if available: */
>>> + granularity = bdrv_dbm_calc_def_granularity(bs);
>>> + }
>>> +
>>> + bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>>> +}
>>> +
>>> +void qmp_block_dirty_bitmap_remove(const char *device, const char
>>> *name,
>>> + Error **errp)
>>> +{
>>> + BlockDriverState *bs;
>>> + BdrvDirtyBitmap *bitmap;
>>> + Error *local_err = NULL;
>>
>> [1] why aren't you minding !device here?
>
> Trusting lookup to error out.
Well, then you don't need to test the !device case in
qmp_block_dirty_bitmap_add() either, I think.
Max
>>> +
>>> + bs = bdrv_lookup_bs(device, NULL, &local_err);
>>> + if (!bs) {
>>> + error_propagate(errp, local_err);
>>
>> Same thing about error_propagate() here.
>
> Same.
>
>>> + return;
>>> + }
>>> +
>>> + if (!name || name[0] == '\0') {
>>> + error_setg(errp, "Bitmap name cannot be empty");
>>> + return;
>>> + }
>>> + bitmap = bdrv_find_dirty_bitmap(bs, name);
>>> + if (!bitmap) {
>>> + error_setg(errp, "Dirty bitmap not found: %s", name);
>>> + return;
>>> + }
>>> +
>>> + bdrv_dirty_bitmap_make_anon(bs, bitmap);
>>> + bdrv_release_dirty_bitmap(bs, bitmap);
>>> +}
>>> +
>>> 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 977f7b5..feb84e2 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -437,6 +437,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);
>>> +int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
>>> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>>> int64_t sector);
>>> void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int
>>> nr_sectors);
>>> void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int
>>> nr_sectors);
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 569c9f5..53daf49 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -864,6 +864,64 @@
>>> '*on-target-error': 'BlockdevOnError' } }
>>> ##
>>> +# @BlockDirtyBitmap
>>> +#
>>> +# @device: name of device which the bitmap is tracking
>>> +#
>>> +# @name: name of the dirty bitmap
>>> +#
>>> +# Since 2.3
>>> +##
>>> +{ 'type': 'BlockDirtyBitmap',
>>> + 'data': { 'device': 'str', 'name': 'str' } }
>>> +
>>> +##
>>> +# @BlockDirtyBitmapAdd
>>> +#
>>> +# @device: name of device 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',
>>> +# 'base': 'BlockDirtyBitmap',
>>> +# 'data': { '*granularity': 'int' } }
>>
>> This part of the comment doesn't seem right...
>>
>> If you left it on purpose, you should add a comment like "XXX: Should
>> use this representation after the code generator has been fixed to make
>> it work".
>>
>> Max
>
> No, just another bonehead moment.
>
> Thanks for the reviews.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2014-11-26 15:39 ` John Snow
2014-11-26 15:53 ` Max Reitz
@ 2014-11-27 9:16 ` Markus Armbruster
1 sibling, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2014-11-27 9:16 UTC (permalink / raw)
To: John Snow; +Cc: Fam Zheng, qemu-devel, Max Reitz
John Snow <jsnow@redhat.com> writes:
> On 11/26/2014 07:19 AM, Max Reitz wrote:
>> On 2014-11-25 at 20:46, John Snow wrote:
>>> From: Fam Zheng <famz@redhat.com>
>>>
>>> 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 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}'
[...]
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 57910b8..e2fe687 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1810,6 +1810,69 @@ 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 *device, const char *name,
>>> + bool has_granularity, int64_t
>>> granularity,
>>> + Error **errp)
>>> +{
>>> + BlockDriverState *bs;
>>> + Error *local_err = NULL;
>>> +
>>> + if (!device) {
>>> + error_setg(errp, "Device to add dirty bitmap to must not be
>>> null");
>>> + return;
>>> + }
>>
>> I don't know if checking for that case makes sense, but of course it
>> won't hurt. But...[1]
>>
>>> +
>>> + bs = bdrv_lookup_bs(device, NULL, &local_err);
>>
>> Fair enough, I'd still like blk_by_name() and blk_bs() more
>> (bdrv_lookup_bs() uses blk_bs(blk_by_name()) for the device name just as
>> bdrv_find() did), but this is at least not a completely trivial wrapper.
When a QMP command deals with a backend as a whole, then it should use
the blk_ API. When it deals with individual (possibly interior) nodes,
then it has to use the bdrv_ API. Since blk_ is new, most existing code
doesn't use it, yet. Beginnings:
6007cdd blockdev: Convert qmp_eject(), qmp_change_blockdev() to BlockBackend
d829a21 block/qapi: Convert qmp_query_block() to BlockBackend
>>> + if (!bs) {
>>> + error_propagate(errp, local_err);
>>
>> Simply calling bdrv_lookup_bs(device, NULL, errp); suffices, no need for
>> a local Error object and error_propagate(). But I'm fine with it either
>> way.
>
> I found other cases where we do this in the code, I actually thought
> it was a "style thing," which is why I did it so often.
Cargo cult programming ;-P
Back to serious: I believe a chief reason for bad or ugly error handling
in new code is the many bad and ugly examples in existing code, from
where they get copied into new code by sensible people.
I've been chipping away at the bad examples. Don't hold your breath.
> I'll happily cut it out.
Yes, please.
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 03/10] block: Introduce bdrv_dirty_bitmap_granularity()
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 01/10] qapi: Add optional field "name" to block dirty bitmap John Snow
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 12:29 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 04/10] hbitmap: Add hbitmap_copy John Snow
` (7 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
This returns the granularity (in bytes) of dirty bitmap,
which matches the QMP interface and the existing query
interface.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 9 +++++++--
include/block/block.h | 2 ++
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index a940345..ea4c8d8 100644
--- a/block.c
+++ b/block.c
@@ -5364,8 +5364,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 =
- ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+ info->granularity = bdrv_dirty_bitmap_granularity(bs, bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
entry->value = info;
@@ -5404,6 +5403,12 @@ int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
return granularity;
}
+int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+ 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 feb84e2..9a62bce 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,6 +438,8 @@ 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);
int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
+int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
--
1.9.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 03/10] block: Introduce bdrv_dirty_bitmap_granularity()
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 03/10] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
@ 2014-11-26 12:29 ` Max Reitz
0 siblings, 0 replies; 27+ messages in thread
From: Max Reitz @ 2014-11-26 12:29 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> This returns the granularity (in bytes) of dirty bitmap,
> which matches the QMP interface and the existing query
> interface.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
Maybe you should have removed the R-b because of the functional changes,
but Benoît should complain about that, not me.
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 9 +++++++--
> include/block/block.h | 2 ++
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index a940345..ea4c8d8 100644
> --- a/block.c
> +++ b/block.c
> @@ -5364,8 +5364,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 =
> - ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
> + info->granularity = bdrv_dirty_bitmap_granularity(bs, bm);
> info->has_name = !!bm->name;
> info->name = g_strdup(bm->name);
> entry->value = info;
> @@ -5404,6 +5403,12 @@ int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
> return granularity;
> }
>
> +int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap)
> +{
> + return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> +}
> +
Oh, BDRV_SECTOR_SIZE is an unsigned long long. Great, I didn't know. (So
this can't overflow)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 04/10] hbitmap: Add hbitmap_copy
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (2 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 03/10] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 12:32 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap John Snow
` (6 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
This makes a deep copy of an HBitmap.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/qemu/hbitmap.h | 8 ++++++++
util/hbitmap.c | 16 ++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 550d7ce..b645cfc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,14 @@ struct HBitmapIter {
HBitmap *hbitmap_alloc(uint64_t size, int granularity);
/**
+ * hbitmap_copy:
+ * @bitmap: The original bitmap to copy.
+ *
+ * Copy a HBitmap.
+ */
+HBitmap *hbitmap_copy(const HBitmap *bitmap);
+
+/**
* hbitmap_empty:
* @hb: HBitmap to operate on.
*
diff --git a/util/hbitmap.c b/util/hbitmap.c
index b3060e6..eddc05b 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -395,3 +395,19 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
return hb;
}
+
+HBitmap *hbitmap_copy(const HBitmap *bitmap)
+{
+ int i;
+ uint64_t size;
+ HBitmap *hb = g_memdup(bitmap, sizeof(HBitmap));
+
+ size = bitmap->size;
+ for (i = HBITMAP_LEVELS; i >= 0; i--) {
+ size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+ hb->levels[i] = g_memdup(bitmap->levels[i],
+ size * sizeof(unsigned long));
+ }
+
+ return hb;
+}
--
1.9.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 04/10] hbitmap: Add hbitmap_copy
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 04/10] hbitmap: Add hbitmap_copy John Snow
@ 2014-11-26 12:32 ` Max Reitz
0 siblings, 0 replies; 27+ messages in thread
From: Max Reitz @ 2014-11-26 12:32 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> This makes a deep copy of an HBitmap.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> include/qemu/hbitmap.h | 8 ++++++++
> util/hbitmap.c | 16 ++++++++++++++++
> 2 files changed, 24 insertions(+)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 550d7ce..b645cfc 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -65,6 +65,14 @@ struct HBitmapIter {
> HBitmap *hbitmap_alloc(uint64_t size, int granularity);
>
> /**
> + * hbitmap_copy:
> + * @bitmap: The original bitmap to copy.
> + *
> + * Copy a HBitmap.
> + */
> +HBitmap *hbitmap_copy(const HBitmap *bitmap);
> +
> +/**
> * hbitmap_empty:
> * @hb: HBitmap to operate on.
> *
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index b3060e6..eddc05b 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -395,3 +395,19 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
> hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
> return hb;
> }
> +
> +HBitmap *hbitmap_copy(const HBitmap *bitmap)
> +{
> + int i;
> + uint64_t size;
> + HBitmap *hb = g_memdup(bitmap, sizeof(HBitmap));
> +
> + size = bitmap->size;
> + for (i = HBITMAP_LEVELS; i >= 0; i--) {
Should be HBITMAP_LEVELS - 1.
Max
> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + hb->levels[i] = g_memdup(bitmap->levels[i],
> + size * sizeof(unsigned long));
> + }
> +
> + return hb;
> +}
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (3 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 04/10] hbitmap: Add hbitmap_copy John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 12:43 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 06/10] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable John Snow
` (5 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 35 +++++++++++++++++++++++++++++++----
include/block/block.h | 4 ++++
2 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index ea4c8d8..9582550 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,8 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
+ int64_t size;
+ int64_t granularity;
char *name;
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -5311,6 +5313,26 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
bitmap->name = NULL;
}
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+ const BdrvDirtyBitmap *bitmap,
+ const char *name)
+{
+ BdrvDirtyBitmap *new_bitmap;
+
+ new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
+ new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
+ new_bitmap->size = bitmap->size;
+ new_bitmap->granularity = bitmap->granularity;
+ new_bitmap->name = name ? g_strdup(name) : NULL;
+ QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
+ return new_bitmap;
+}
+
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+ hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+}
+
BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
int granularity,
const char *name,
@@ -5318,6 +5340,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
{
int64_t bitmap_size;
BdrvDirtyBitmap *bitmap;
+ int sector_granularity;
assert((granularity & (granularity - 1)) == 0);
@@ -5325,8 +5348,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");
@@ -5334,7 +5357,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return NULL;
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
- bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+ bitmap->size = bitmap_size;
+ bitmap->granularity = granularity;
+ 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;
@@ -5406,7 +5431,9 @@ int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap)
{
- return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+ g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap) == \
+ bitmap->granularity);
+ return bitmap->granularity;
}
void bdrv_dirty_iter_init(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index 9a62bce..d1b5c10 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -435,6 +435,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+ const BdrvDirtyBitmap *bitmap,
+ const char *name);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
--
1.9.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap John Snow
@ 2014-11-26 12:43 ` Max Reitz
2014-11-26 16:01 ` Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Max Reitz @ 2014-11-26 12:43 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 35 +++++++++++++++++++++++++++++++----
> include/block/block.h | 4 ++++
> 2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index ea4c8d8..9582550 100644
> --- a/block.c
> +++ b/block.c
> @@ -53,6 +53,8 @@
>
> struct BdrvDirtyBitmap {
> HBitmap *bitmap;
> + int64_t size;
> + int64_t granularity;
> char *name;
> QLIST_ENTRY(BdrvDirtyBitmap) list;
> };
> @@ -5311,6 +5313,26 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> bitmap->name = NULL;
> }
>
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
> + const BdrvDirtyBitmap *bitmap,
> + const char *name)
> +{
> + BdrvDirtyBitmap *new_bitmap;
> +
> + new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
> + new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
> + new_bitmap->size = bitmap->size;
> + new_bitmap->granularity = bitmap->granularity;
> + new_bitmap->name = name ? g_strdup(name) : NULL;
You changed this in patch 1 so you may want to change it here, too. I'm
fine with it either way.
> + QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
> + return new_bitmap;
> +}
> +
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> + hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> +}
> +
> BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> int granularity,
> const char *name,
> @@ -5318,6 +5340,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> {
> int64_t bitmap_size;
> BdrvDirtyBitmap *bitmap;
> + int sector_granularity;
>
> assert((granularity & (granularity - 1)) == 0);
>
> @@ -5325,8 +5348,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");
> @@ -5334,7 +5357,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> return NULL;
> }
> bitmap = g_new0(BdrvDirtyBitmap, 1);
> - bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
> + bitmap->size = bitmap_size;
> + bitmap->granularity = granularity;
> + 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;
> @@ -5406,7 +5431,9 @@ int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
> int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap)
> {
> - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> + g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap) == \
> + bitmap->granularity);
Do you really need that backslash?
If you don't and remove it, or if you do and keep it, and with either
"name ? g_strdup(name) : NULL" left as-is or replace by "g_strdup(name)":
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
2014-11-26 12:43 ` Max Reitz
@ 2014-11-26 16:01 ` Eric Blake
2014-11-27 9:05 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Eric Blake @ 2014-11-26 16:01 UTC (permalink / raw)
To: Max Reitz, John Snow, qemu-devel; +Cc: Fam Zheng
[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]
On 11/26/2014 05:43 AM, Max Reitz wrote:
> On 2014-11-25 at 20:46, John Snow wrote:
>> From: Fam Zheng <famz@redhat.com>
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block.c | 35 +++++++++++++++++++++++++++++++----
>> include/block/block.h | 4 ++++
>> 2 files changed, 35 insertions(+), 4 deletions(-)
>>
>> @@ -5406,7 +5431,9 @@ int64_t
>> bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>> int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap)
>> {
>> - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>> + g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap)
>> == \
>> + bitmap->granularity);
>
> Do you really need that backslash?
>
> If you don't and remove it, or if you do and keep it, and with either
> "name ? g_strdup(name) : NULL" left as-is or replace by "g_strdup(name)":
>
g_strdup(NULL) is safe, so 'name ? g_strdup(name) : NULL' is pointless
and can be written as 'g_strdup(name)' with no change in functionality.
--
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: 539 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
2014-11-26 16:01 ` Eric Blake
@ 2014-11-27 9:05 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2014-11-27 9:05 UTC (permalink / raw)
To: Eric Blake; +Cc: Fam Zheng, John Snow, qemu-devel, Max Reitz
Eric Blake <eblake@redhat.com> writes:
> On 11/26/2014 05:43 AM, Max Reitz wrote:
>> On 2014-11-25 at 20:46, John Snow wrote:
>>> From: Fam Zheng <famz@redhat.com>
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block.c | 35 +++++++++++++++++++++++++++++++----
>>> include/block/block.h | 4 ++++
>>> 2 files changed, 35 insertions(+), 4 deletions(-)
>>>
>
>>> @@ -5406,7 +5431,9 @@ int64_t
>>> bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>>> int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap)
>>> {
>>> - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>>> + g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap)
>>> == \
>>> + bitmap->granularity);
>>
>> Do you really need that backslash?
>>
>> If you don't and remove it, or if you do and keep it, and with either
>> "name ? g_strdup(name) : NULL" left as-is or replace by "g_strdup(name)":
>>
>
> g_strdup(NULL) is safe, so 'name ? g_strdup(name) : NULL' is pointless
> and can be written as 'g_strdup(name)' with no change in functionality.
Yes, please. See also:
80e0090 virtio: Drop superfluous conditionals around g_strdup()
c14e984 vnc: Drop superfluous conditionals around g_strdup()
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 06/10] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (4 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 12:51 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
` (4 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
This allows to put the dirty bitmap into a disabled state where no more
writes will be tracked.
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>
---
block.c | 15 +++++++++++++
blockdev.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 2 ++
qapi/block-core.json | 28 +++++++++++++++++++++++
qmp-commands.hx | 10 +++++++++
5 files changed, 117 insertions(+)
diff --git a/block.c b/block.c
index 9582550..7217066 100644
--- a/block.c
+++ b/block.c
@@ -56,6 +56,7 @@ struct BdrvDirtyBitmap {
int64_t size;
int64_t granularity;
char *name;
+ bool enabled;
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -5361,6 +5362,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
bitmap->granularity = granularity;
bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1);
bitmap->name = g_strdup(name);
+ bitmap->enabled = true;
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
return bitmap;
}
@@ -5379,6 +5381,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
}
}
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+ bitmap->enabled = false;
+}
+
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+ bitmap->enabled = true;
+}
+
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
{
BdrvDirtyBitmap *bm;
@@ -5447,6 +5459,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
{
BdrvDirtyBitmap *bitmap;
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ if (!bitmap->enabled) {
+ continue;
+ }
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
}
diff --git a/blockdev.c b/blockdev.c
index e2fe687..baaf902 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1873,6 +1873,68 @@ void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
bdrv_release_dirty_bitmap(bs, bitmap);
}
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
+ const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ Error *local_err = NULL;
+
+ if (!device) {
+ error_setg(errp, "Device cannot be NULL");
+ return NULL;
+ }
+ if (!name) {
+ error_setg(errp, "Bitmap name cannot be NULL");
+ return NULL;
+ }
+
+ bs = bdrv_lookup_bs(device, NULL, &local_err);
+ if (!bs) {
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Dirty bitmap not found: %s", name);
+ return NULL;
+ }
+
+ return bitmap;
+}
+
+void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
+ Error **errp)
+{
+ BdrvDirtyBitmap *bitmap;
+ Error *local_err = NULL;
+
+ bitmap = block_dirty_bitmap_lookup(device, name, &local_err);
+ if (!bitmap) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ bdrv_enable_dirty_bitmap(NULL, bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
+ Error **errp)
+{
+ BdrvDirtyBitmap *bitmap;
+ Error *local_err = NULL;
+
+ bitmap = block_dirty_bitmap_lookup(device, name, &local_err);
+ if (!bitmap) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ bdrv_disable_dirty_bitmap(NULL, bitmap);
+}
+
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 d1b5c10..b6df2ae 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -440,6 +440,8 @@ BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
const BdrvDirtyBitmap *bitmap,
const char *name);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 53daf49..46967ac 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -922,6 +922,34 @@
'data': 'BlockDirtyBitmap' }
##
+# @block-dirty-bitmap-enable
+#
+# Enable a dirty bitmap on the device
+#
+# Returns: nothing on success
+# If @device is not a valid block device, DeviceNotFound
+# If @name is not found, GenericError with an explaining message
+#
+# 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 @device is not a valid block device, DeviceNotFound
+# If @name is not found, GenericError with an explaining message
+#
+# 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 5602dcf..8751a06 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1211,6 +1211,16 @@ EQMP
.args_type = "device:B,name:s",
.mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
},
+ {
+ .name = "block-dirty-bitmap-enable",
+ .args_type = "device:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_enable,
+ },
+ {
+ .name = "block-dirty-bitmap-disable",
+ .args_type = "device:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_disable,
+ },
SQMP
--
1.9.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 06/10] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 06/10] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable John Snow
@ 2014-11-26 12:51 ` Max Reitz
0 siblings, 0 replies; 27+ messages in thread
From: Max Reitz @ 2014-11-26 12:51 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> This allows to put the dirty bitmap into a disabled state where no more
> writes will be tracked.
>
> 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>
> ---
> block.c | 15 +++++++++++++
> blockdev.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 2 ++
> qapi/block-core.json | 28 +++++++++++++++++++++++
> qmp-commands.hx | 10 +++++++++
> 5 files changed, 117 insertions(+)
>
> diff --git a/block.c b/block.c
> index 9582550..7217066 100644
> --- a/block.c
> +++ b/block.c
> @@ -56,6 +56,7 @@ struct BdrvDirtyBitmap {
> int64_t size;
> int64_t granularity;
> char *name;
> + bool enabled;
> QLIST_ENTRY(BdrvDirtyBitmap) list;
> };
>
> @@ -5361,6 +5362,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> bitmap->granularity = granularity;
> bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1);
> bitmap->name = g_strdup(name);
> + bitmap->enabled = true;
> QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
> return bitmap;
> }
> @@ -5379,6 +5381,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> }
> }
>
> +void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> + bitmap->enabled = false;
> +}
> +
> +void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> + bitmap->enabled = true;
> +}
> +
> BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> {
> BdrvDirtyBitmap *bm;
> @@ -5447,6 +5459,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> {
> BdrvDirtyBitmap *bitmap;
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> + if (!bitmap->enabled) {
> + continue;
> + }
> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> }
> }
> diff --git a/blockdev.c b/blockdev.c
> index e2fe687..baaf902 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1873,6 +1873,68 @@ void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> bdrv_release_dirty_bitmap(bs, bitmap);
> }
>
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
> + const char *name,
> + Error **errp)
> +{
> + BlockDriverState *bs;
> + BdrvDirtyBitmap *bitmap;
> + Error *local_err = NULL;
> +
> + if (!device) {
> + error_setg(errp, "Device cannot be NULL");
> + return NULL;
> + }
> + if (!name) {
> + error_setg(errp, "Bitmap name cannot be NULL");
> + return NULL;
> + }
> +
> + bs = bdrv_lookup_bs(device, NULL, &local_err);
> + if (!bs) {
> + error_propagate(errp, local_err);
Comments from patch 2 apply here as well: I'm still in favor of
blk_by_name(), and you don't need local_err.
> + return NULL;
> + }
> +
> + bitmap = bdrv_find_dirty_bitmap(bs, name);
> + if (!bitmap) {
> + error_setg(errp, "Dirty bitmap not found: %s", name);
> + return NULL;
> + }
> +
> + return bitmap;
> +}
> +
> +void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
> + Error **errp)
> +{
> + BdrvDirtyBitmap *bitmap;
> + Error *local_err = NULL;
> +
> + bitmap = block_dirty_bitmap_lookup(device, name, &local_err);
> + if (!bitmap) {
> + error_propagate(errp, local_err);
Again, no need for error_propagate().
> + return;
> + }
> +
> + bdrv_enable_dirty_bitmap(NULL, bitmap);
> +}
> +
> +void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
> + Error **errp)
> +{
> + BdrvDirtyBitmap *bitmap;
> + Error *local_err = NULL;
> +
> + bitmap = block_dirty_bitmap_lookup(device, name, &local_err);
> + if (!bitmap) {
> + error_propagate(errp, local_err);
Here, too.
With or without any of the "ret = foo(..., &local_err); if (ret
indicates error) { error_propagate(errp, local_err);" replaced by "ret =
foo(..., errp); if (ret indicates error) {":
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (5 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 06/10] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 14:19 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable} John Snow
` (3 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.
There are two bitmap use modes for sync=dirty-bitmap:
- reset: backup job makes a copy of bitmap and resets the original
one.
- consume: backup job makes the original anonymous (invisible to user)
and releases it after use.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 5 ++
block/backup.c | 128 ++++++++++++++++++++++++++++++++++++++--------
block/mirror.c | 4 ++
blockdev.c | 18 ++++++-
hmp.c | 4 +-
include/block/block.h | 1 +
include/block/block_int.h | 6 +++
qapi/block-core.json | 30 +++++++++--
qmp-commands.hx | 7 +--
9 files changed, 174 insertions(+), 29 deletions(-)
diff --git a/block.c b/block.c
index 7217066..cf93148 100644
--- a/block.c
+++ b/block.c
@@ -5454,6 +5454,11 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
hbitmap_iter_init(hbi, bitmap->bitmap, 0);
}
+void bdrv_dirty_iter_set(HBitmapIter *hbi, int64_t offset)
+{
+ hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
int nr_sectors)
{
diff --git a/block/backup.c b/block/backup.c
index 792e655..8e7d135 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,10 @@ typedef struct CowRequest {
typedef struct BackupBlockJob {
BlockJob common;
BlockDriverState *target;
+ /* bitmap for sync=dirty-bitmap */
+ BdrvDirtyBitmap *sync_bitmap;
+ /* dirty bitmap granularity */
+ int64_t sync_bitmap_gran;
MirrorSyncMode sync_mode;
RateLimit limit;
BlockdevOnError on_source_error;
@@ -242,6 +246,31 @@ static void backup_complete(BlockJob *job, void *opaque)
g_free(data);
}
+static bool 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 +283,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 +307,45 @@ 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;
+ bool polyrhythmic;
+
+ bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+ /* Does the granularity happen to match our backup cluster size? */
+ polyrhythmic = (job->sync_bitmap_gran != BACKUP_CLUSTER_SIZE);
+
+ /* Find the next dirty /sector/ and copy that /cluster/ */
+ while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+ if (yield_and_check(job)) {
+ goto leave;
+ }
+ cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+ 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_dirty_iter_set(&hbi,
+ (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
+ }
+ }
} 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 +397,16 @@ 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) {
+ bdrv_release_dirty_bitmap(bs, job->sync_bitmap);
+ }
hbitmap_free(job->bitmap);
bdrv_iostatus_disable(target);
@@ -368,12 +418,15 @@ static void coroutine_fn backup_run(void *opaque)
void backup_start(BlockDriverState *bs, BlockDriverState *target,
int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
+ BitmapUseMode bitmap_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
Error **errp)
{
int64_t len;
+ BdrvDirtyBitmap *original;
assert(bs);
assert(target);
@@ -386,6 +439,35 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
return;
}
+ if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+ if (!sync_bitmap) {
+ error_setg(errp, "must provide a valid bitmap name for "
+ "\"dirty-bitmap\" sync mode");
+ return;
+ }
+
+ switch (bitmap_mode) {
+ case BITMAP_USE_MODE_RESET:
+ original = sync_bitmap;
+ sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
+ bdrv_reset_dirty_bitmap(bs, original);
+ break;
+ case BITMAP_USE_MODE_CONSUME:
+ bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
+ break;
+ default:
+ g_assert_not_reached();
+ error_setg(errp, "Invalid BitmapUseMode (0x%02x) given to backup_start.", bitmap_mode);
+ return;
+ }
+ bdrv_disable_dirty_bitmap(bs, sync_bitmap);
+ } else if (sync_bitmap) {
+ error_setg(errp,
+ "sync_bitmap provided to backup_run, but received an incompatible sync_mode (0x%02x)",
+ sync_mode);
+ return;
+ }
+
len = bdrv_getlength(bs);
if (len < 0) {
error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -403,6 +485,12 @@ 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;
+ if (sync_bitmap) {
+ job->sync_bitmap_gran =
+ bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);
+ }
job->common.len = len;
job->common.co = qemu_coroutine_create(backup_run);
qemu_coroutine_enter(job->common.co, job);
diff --git a/block/mirror.c b/block/mirror.c
index 3633632..af91ae0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -714,6 +714,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 baaf902..adf841a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1470,6 +1470,8 @@ 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_bitmap_use_mode, backup->bitmap_use_mode,
backup->has_on_source_error, backup->on_source_error,
backup->has_on_target_error, backup->on_target_error,
&local_err);
@@ -2245,6 +2247,8 @@ 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_bitmap_use_mode, enum BitmapUseMode bitmap_mode,
bool has_on_source_error, BlockdevOnError on_source_error,
bool has_on_target_error, BlockdevOnError on_target_error,
Error **errp)
@@ -2252,6 +2256,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;
@@ -2347,7 +2352,18 @@ void qmp_drive_backup(const char *device, const char *target,
bdrv_set_aio_context(target_bs, aio_context);
- backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+ if (has_bitmap) {
+ bmap = bdrv_find_dirty_bitmap(bs, bitmap);
+ if (!bmap) {
+ error_setg(errp, "has_bitmap was set, but bitmap '%s' could not be found",
+ bitmap);
+ goto out;
+ }
+ }
+
+ backup_start(bs, target_bs, speed, sync, bmap,
+ has_bitmap_use_mode ? bitmap_mode : BITMAP_USE_MODE_RESET,
+ on_source_error, on_target_error,
block_job_cb, bs, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
diff --git a/hmp.c b/hmp.c
index 63d7686..bc5a2d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -966,7 +966,9 @@ 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, false, 0, &err);
hmp_handle_error(mon, &err);
}
diff --git a/include/block/block.h b/include/block/block.h
index b6df2ae..4774fef 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -451,6 +451,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_dirty_iter_set(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 a1c17b9..215a27d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -571,6 +571,10 @@ 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.
+ * @bitmap_mode: BITMAP_USE_MODE_{CONSUME, RESET}
+ * Reset: Make a copy and reset the original bitmap.
+ * Consume: Anonymize the bitmap and free it after completion.
* @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.
@@ -581,6 +585,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
*/
void backup_start(BlockDriverState *bs, BlockDriverState *target,
int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
+ BitmapUseMode bitmap_mode,
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 46967ac..3f7a35d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -470,7 +470,7 @@
# Since: 1.3
##
{ 'enum': 'MirrorSyncMode',
- 'data': ['top', 'full', 'none'] }
+ 'data': ['top', 'full', 'none', 'dirty-bitmap'] }
##
# @BlockJobType:
@@ -633,6 +633,21 @@
'*format': 'str', '*mode': 'NewImageMode' } }
##
+# @BitmapUseMode
+#
+# An enumeration that tells QEMU what operation to take when using a bitmap
+# in drive backup sync mode dirty-bitmap.
+#
+# @consume: QEMU should just consume the bitmap and release it after using
+#
+# @reset: QEMU should reset the dirty bitmap
+#
+# Since: 2.3
+##
+{ 'enum': 'BitmapUseMode',
+'data': [ 'consume', 'reset' ] }
+
+##
# @DriveBackup
#
# @device: the name of the device which should be copied.
@@ -645,14 +660,20 @@
# 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)
+#
+# @bitmap-use-mode: #optional which operation to take when consuming @bitmap,
+# default is reset. (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).
@@ -670,7 +691,8 @@
{ 'type': 'DriveBackup',
'data': { 'device': 'str', 'target': 'str', '*format': 'str',
'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
- '*speed': 'int',
+ '*speed': 'int', '*bitmap': 'str',
+ '*bitmap-use-mode': 'BitmapUseMode',
'*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8751a06..84f9bd6 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] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
@ 2014-11-26 14:19 ` Max Reitz
0 siblings, 0 replies; 27+ messages in thread
From: Max Reitz @ 2014-11-26 14:19 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> For "dirty-bitmap" sync mode, the block job will iterate through the
> given dirty bitmap to decide if a sector needs backup (backup all the
> dirty clusters and skip clean ones), just as allocation conditions of
> "top" sync mode.
>
> There are two bitmap use modes for sync=dirty-bitmap:
>
> - reset: backup job makes a copy of bitmap and resets the original
> one.
> - consume: backup job makes the original anonymous (invisible to user)
> and releases it after use.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 5 ++
> block/backup.c | 128 ++++++++++++++++++++++++++++++++++++++--------
> block/mirror.c | 4 ++
> blockdev.c | 18 ++++++-
> hmp.c | 4 +-
> include/block/block.h | 1 +
> include/block/block_int.h | 6 +++
> qapi/block-core.json | 30 +++++++++--
> qmp-commands.hx | 7 +--
> 9 files changed, 174 insertions(+), 29 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7217066..cf93148 100644
> --- a/block.c
> +++ b/block.c
> @@ -5454,6 +5454,11 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
> hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> }
>
> +void bdrv_dirty_iter_set(HBitmapIter *hbi, int64_t offset)
> +{
> + hbitmap_iter_init(hbi, hbi->hb, offset);
> +}
> +
> void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> int nr_sectors)
> {
> diff --git a/block/backup.c b/block/backup.c
> index 792e655..8e7d135 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -37,6 +37,10 @@ typedef struct CowRequest {
> typedef struct BackupBlockJob {
> BlockJob common;
> BlockDriverState *target;
> + /* bitmap for sync=dirty-bitmap */
> + BdrvDirtyBitmap *sync_bitmap;
> + /* dirty bitmap granularity */
> + int64_t sync_bitmap_gran;
> MirrorSyncMode sync_mode;
> RateLimit limit;
> BlockdevOnError on_source_error;
> @@ -242,6 +246,31 @@ static void backup_complete(BlockJob *job, void *opaque)
> g_free(data);
> }
>
> +static bool 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 +283,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 +307,45 @@ 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;
> + bool polyrhythmic;
> +
> + bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
> + /* Does the granularity happen to match our backup cluster size? */
> + polyrhythmic = (job->sync_bitmap_gran != BACKUP_CLUSTER_SIZE);
> +
> + /* Find the next dirty /sector/ and copy that /cluster/ */
> + while ((sector = hbitmap_iter_next(&hbi)) != -1) {
> + if (yield_and_check(job)) {
> + goto leave;
> + }
> + cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
> +
> + 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_dirty_iter_set(&hbi,
> + (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
> + }
> + }
> } else {
> /* Both FULL and TOP SYNC_MODE's require copying.. */
> for (; start < end; start++) {
There is an empty line remaining after the next line (I'm not writing
this directly below or above that empty line because that would look
strange), we could remove it.
> - 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 +397,16 @@ 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) {
> + bdrv_release_dirty_bitmap(bs, job->sync_bitmap);
> + }
> hbitmap_free(job->bitmap);
>
> bdrv_iostatus_disable(target);
> @@ -368,12 +418,15 @@ static void coroutine_fn backup_run(void *opaque)
>
> void backup_start(BlockDriverState *bs, BlockDriverState *target,
> int64_t speed, MirrorSyncMode sync_mode,
> + BdrvDirtyBitmap *sync_bitmap,
> + BitmapUseMode bitmap_mode,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> BlockCompletionFunc *cb, void *opaque,
> Error **errp)
> {
> int64_t len;
> + BdrvDirtyBitmap *original;
>
> assert(bs);
> assert(target);
> @@ -386,6 +439,35 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> return;
> }
>
> + if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> + if (!sync_bitmap) {
> + error_setg(errp, "must provide a valid bitmap name for "
> + "\"dirty-bitmap\" sync mode");
> + return;
> + }
> +
> + switch (bitmap_mode) {
> + case BITMAP_USE_MODE_RESET:
> + original = sync_bitmap;
> + sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
> + bdrv_reset_dirty_bitmap(bs, original);
> + break;
> + case BITMAP_USE_MODE_CONSUME:
> + bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
> + break;
> + default:
> + g_assert_not_reached();
> + error_setg(errp, "Invalid BitmapUseMode (0x%02x) given to backup_start.", bitmap_mode);
> + return;
This looks fun. Without NDEBUG, crashing is fine, but with NDEBUG (if
someone would ever be crazy enough to define that) it's just a normal error?
I'd drop the error_setg() and the return statement; and I'd also make it
an abort(), but that's a different story, g_assert_not_reached() is more
clear in what it's for, but abort() always works (although you'd really
have to be crazy to define NDEBUG for qemu), so I'm fine with any.
Well, I'm also fine with how it's here (with error_setg() and return),
if just because the g_assert_not_reached() should never ever be skipped,
but it just looks funny.
> + }
> + bdrv_disable_dirty_bitmap(bs, sync_bitmap);
> + } else if (sync_bitmap) {
> + error_setg(errp,
> + "sync_bitmap provided to backup_run, but received an incompatible sync_mode (0x%02x)",
> + sync_mode);
We do have BitmapUseMode_lookup, maybe you want to use that.
> + return;
> + }
> +
> len = bdrv_getlength(bs);
> if (len < 0) {
> error_setg_errno(errp, -len, "unable to get length for '%s'",
> @@ -403,6 +485,12 @@ 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;
> + if (sync_bitmap) {
> + job->sync_bitmap_gran =
> + bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);
> + }
> job->common.len = len;
> job->common.co = qemu_coroutine_create(backup_run);
> qemu_coroutine_enter(job->common.co, job);
> diff --git a/block/mirror.c b/block/mirror.c
> index 3633632..af91ae0 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -714,6 +714,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 baaf902..adf841a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1470,6 +1470,8 @@ 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_bitmap_use_mode, backup->bitmap_use_mode,
> backup->has_on_source_error, backup->on_source_error,
> backup->has_on_target_error, backup->on_target_error,
> &local_err);
> @@ -2245,6 +2247,8 @@ 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_bitmap_use_mode, enum BitmapUseMode bitmap_mode,
> bool has_on_source_error, BlockdevOnError on_source_error,
> bool has_on_target_error, BlockdevOnError on_target_error,
> Error **errp)
> @@ -2252,6 +2256,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;
> @@ -2347,7 +2352,18 @@ void qmp_drive_backup(const char *device, const char *target,
>
> bdrv_set_aio_context(target_bs, aio_context);
>
> - backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
> + if (has_bitmap) {
> + bmap = bdrv_find_dirty_bitmap(bs, bitmap);
> + if (!bmap) {
> + error_setg(errp, "has_bitmap was set, but bitmap '%s' could not be found",
has_bitmap is something internal to qemu. I don't know the exact path
until this point, but I think what must happen to trigger this statement
is the user just gives a bitmap name which is not available. So the
error message should just say "I'm so very sorry, my dearest user, but I
have looked far and wide and could not locate a bitmap of that name." Or
something like that.
So I'd translate "has_bitmap was set" to "A bitmap name was specified".
Aside from these small issues regarding error messages and an empty
line, the code looks correct, so (although bdrv_dirty_iter_set() still
looks a bit magic to me, I can't explain it):
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (6 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 14:44 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 09/10] qmp: Add dirty bitmap 'enabled' field in query-block John Snow
` (2 subsequent siblings)
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
This adds three 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.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockdev.c | 147 +++++++++++++++++++++++++++++++++++++++++++------------
qapi-schema.json | 5 +-
2 files changed, 119 insertions(+), 33 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index adf841a..b98249b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1497,6 +1497,106 @@ static void drive_backup_abort(BlkTransactionState *common)
}
}
+static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ BlockDirtyBitmapAdd *action;
+
+ action = common->action->block_dirty_bitmap_add;
+ qmp_block_dirty_bitmap_add(action->device, action->name,
+ action->has_granularity, action->granularity,
+ errp);
+}
+
+static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+ BlockDirtyBitmapAdd *action;
+ BdrvDirtyBitmap *bm;
+ BlockDriverState *bs;
+
+ action = common->action->block_dirty_bitmap_add;
+ bs = bdrv_lookup_bs(action->device, NULL, NULL);
+ if (bs) {
+ bm = bdrv_find_dirty_bitmap(bs, action->name);
+ if (bm) {
+ bdrv_release_dirty_bitmap(bs, bm);
+ }
+ }
+}
+
+typedef struct BlockDirtyBitmapState {
+ BlkTransactionState common;
+ BdrvDirtyBitmap *bitmap;
+} BlockDirtyBitmapState;
+
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
+ const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ Error *local_err = NULL;
+
+ if (!device) {
+ error_setg(errp, "Device cannot be NULL");
+ return NULL;
+ }
+ if (!name) {
+ error_setg(errp, "Bitmap name cannot be NULL");
+ return NULL;
+ }
+
+ bs = bdrv_lookup_bs(device, NULL, &local_err);
+ if (!bs) {
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Dirty bitmap not found: %s", name);
+ return NULL;
+ }
+
+ return bitmap;
+}
+
+/**
+ * Enable and Disable re-uses the same preparation.
+ */
+static void block_dirty_bitmap_en_toggle_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ BlockDirtyBitmap *action;
+ Error *local_err = NULL;
+
+ action = common->action->block_dirty_bitmap_enable;
+
+ state->bitmap = block_dirty_bitmap_lookup(action->device,
+ action->name,
+ &local_err);
+ if (!state->bitmap) {
+ error_propagate(errp, local_err);
+ return;
+ }
+}
+
+static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ bdrv_enable_dirty_bitmap(NULL, state->bitmap);
+}
+
+static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ bdrv_disable_dirty_bitmap(NULL, state->bitmap);
+}
+
static void abort_prepare(BlkTransactionState *common, Error **errp)
{
error_setg(errp, "Transaction aborted using Abort action");
@@ -1529,6 +1629,21 @@ static const BdrvActionOps actions[] = {
.prepare = internal_snapshot_prepare,
.abort = internal_snapshot_abort,
},
+ [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_en_toggle_prepare,
+ .commit = block_dirty_bitmap_enable_commit,
+ },
+ [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
+ .instance_size = sizeof(BlockDirtyBitmapState),
+ .prepare = block_dirty_bitmap_en_toggle_prepare,
+ .commit = block_dirty_bitmap_disable_commit,
+ },
};
/*
@@ -1875,38 +1990,6 @@ void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
bdrv_release_dirty_bitmap(bs, bitmap);
}
-static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
- const char *name,
- Error **errp)
-{
- BlockDriverState *bs;
- BdrvDirtyBitmap *bitmap;
- Error *local_err = NULL;
-
- if (!device) {
- error_setg(errp, "Device cannot be NULL");
- return NULL;
- }
- if (!name) {
- error_setg(errp, "Bitmap name cannot be NULL");
- return NULL;
- }
-
- bs = bdrv_lookup_bs(device, NULL, &local_err);
- if (!bs) {
- error_propagate(errp, local_err);
- return NULL;
- }
-
- bitmap = bdrv_find_dirty_bitmap(bs, name);
- if (!bitmap) {
- error_setg(errp, "Dirty bitmap not found: %s", name);
- return NULL;
- }
-
- return bitmap;
-}
-
void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
Error **errp)
{
diff --git a/qapi-schema.json b/qapi-schema.json
index d0926d9..958be35 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1260,7 +1260,10 @@
'blockdev-snapshot-sync': 'BlockdevSnapshot',
'drive-backup': 'DriveBackup',
'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'
} }
##
--
1.9.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable} John Snow
@ 2014-11-26 14:44 ` Max Reitz
0 siblings, 0 replies; 27+ messages in thread
From: Max Reitz @ 2014-11-26 14:44 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Fam Zheng
On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> This adds three 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.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> blockdev.c | 147 +++++++++++++++++++++++++++++++++++++++++++------------
> qapi-schema.json | 5 +-
> 2 files changed, 119 insertions(+), 33 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index adf841a..b98249b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1497,6 +1497,106 @@ static void drive_backup_abort(BlkTransactionState *common)
> }
> }
>
> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> + Error **errp)
> +{
> + BlockDirtyBitmapAdd *action;
> +
> + action = common->action->block_dirty_bitmap_add;
> + qmp_block_dirty_bitmap_add(action->device, action->name,
> + action->has_granularity, action->granularity,
> + errp);
> +}
> +
> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapAdd *action;
> + BdrvDirtyBitmap *bm;
> + BlockDriverState *bs;
> +
> + action = common->action->block_dirty_bitmap_add;
> + bs = bdrv_lookup_bs(action->device, NULL, NULL);
> + if (bs) {
> + bm = bdrv_find_dirty_bitmap(bs, action->name);
> + if (bm) {
> + bdrv_release_dirty_bitmap(bs, bm);
> + }
> + }
> +}
> +
> +typedef struct BlockDirtyBitmapState {
> + BlkTransactionState common;
> + BdrvDirtyBitmap *bitmap;
> +} BlockDirtyBitmapState;
> +
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
> + const char *name,
> + Error **errp)
> +{
> + BlockDriverState *bs;
> + BdrvDirtyBitmap *bitmap;
> + Error *local_err = NULL;
> +
> + if (!device) {
> + error_setg(errp, "Device cannot be NULL");
> + return NULL;
> + }
> + if (!name) {
> + error_setg(errp, "Bitmap name cannot be NULL");
> + return NULL;
> + }
> +
> + bs = bdrv_lookup_bs(device, NULL, &local_err);
> + if (!bs) {
> + error_propagate(errp, local_err);
> + return NULL;
> + }
> +
> + bitmap = bdrv_find_dirty_bitmap(bs, name);
> + if (!bitmap) {
> + error_setg(errp, "Dirty bitmap not found: %s", name);
> + return NULL;
> + }
> +
> + return bitmap;
> +}
> +
> +/**
> + * Enable and Disable re-uses the same preparation.
> + */
> +static void block_dirty_bitmap_en_toggle_prepare(BlkTransactionState *common,
> + Error **errp)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + BlockDirtyBitmap *action;
> + Error *local_err = NULL;
> +
> + action = common->action->block_dirty_bitmap_enable;
common->action is a union, so this works for *disable, too (which has
the same time as *enable); but I wouldn't object a comment here that
this is indeed the case.
Or go into pedantic mode and add an assert(action ==
common->action->block_dirty_bitmap_disable).
> +
> + state->bitmap = block_dirty_bitmap_lookup(action->device,
> + action->name,
> + &local_err);
> + if (!state->bitmap) {
> + error_propagate(errp, local_err);
Again, no need for local_err or error_propagate().
Because I feel like I owe you an explanation by now why there is
local_err at all in so many places: For some functions, it is not
possible to discern whether they were successful or not just by looking
at the return value. Therefore, the only way to decide whether they
failed is by looking whether they set the Error object or not. However,
errp may be NULL, therefore in those places we cannot just look at
*errp. In those cases, we have to use a local Error object which is
passed to the function and which we then can use to determine failure or
success; on failure, it then needs to be propagated. A good example is
block_dirty_bitmap_en_toggle_prepare() itself which does not return any
value.
However, in all the places in this series where I said you could drop
the error_propagate() and local_err invoked functions which did return
error values; like here, where !state->bitmap indicates an error.
Therefore, we don't need local_err here.
> + return;
> + }
> +}
> +
> +static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + bdrv_enable_dirty_bitmap(NULL, state->bitmap);
This does work for now, but then I'm asking myself why
bdrv_enable_dirty_bitmap() takes a BDS pointer at all. I thought maybe
in the future the BDS pointer may come in handy, but with this call
you're basically saying that the function will never need the BDS pointer.
It's better to omit the BDS pointer from bdrv_enable_dirty_bitmap(),
then. If we do notice a need for it later on, the compiler will tell us
the places where we don't pass it. Like this, if the function evaluates
the parameter in the future, it will be a runtime problem.
> +}
> +
> +static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
> +{
> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> + common, common);
> + bdrv_disable_dirty_bitmap(NULL, state->bitmap);
Same here, of course.
> +}
> +
> static void abort_prepare(BlkTransactionState *common, Error **errp)
> {
> error_setg(errp, "Transaction aborted using Abort action");
> @@ -1529,6 +1629,21 @@ static const BdrvActionOps actions[] = {
> .prepare = internal_snapshot_prepare,
> .abort = internal_snapshot_abort,
> },
> + [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_en_toggle_prepare,
> + .commit = block_dirty_bitmap_enable_commit,
> + },
> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> + .instance_size = sizeof(BlockDirtyBitmapState),
> + .prepare = block_dirty_bitmap_en_toggle_prepare,
> + .commit = block_dirty_bitmap_disable_commit,
> + },
> };
>
> /*
> @@ -1875,38 +1990,6 @@ void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> bdrv_release_dirty_bitmap(bs, bitmap);
> }
>
> -static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
> - const char *name,
> - Error **errp)
> -{
> - BlockDriverState *bs;
> - BdrvDirtyBitmap *bitmap;
> - Error *local_err = NULL;
> -
> - if (!device) {
> - error_setg(errp, "Device cannot be NULL");
> - return NULL;
> - }
> - if (!name) {
> - error_setg(errp, "Bitmap name cannot be NULL");
> - return NULL;
> - }
> -
> - bs = bdrv_lookup_bs(device, NULL, &local_err);
> - if (!bs) {
> - error_propagate(errp, local_err);
> - return NULL;
> - }
> -
> - bitmap = bdrv_find_dirty_bitmap(bs, name);
> - if (!bitmap) {
> - error_setg(errp, "Dirty bitmap not found: %s", name);
> - return NULL;
> - }
> -
> - return bitmap;
> -}
> -
Is it possible to move this function further up in 6 where it is
created? If so, it doesn't need to be moved in this patch (and moving
code blocks always looks a bit ugly in patches...).
Technically, the patch is fine. If it weren't for BDS issue with the
bdrv_{en,dis}able_dirty_bitmap() calls, I'd give it an R-b.
Max
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [2.3 PATCH v7 09/10] qmp: Add dirty bitmap 'enabled' field in query-block
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (7 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable} John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 14:49 ` Max Reitz
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 10/10] qemu-iotests: Add tests for drive-backup sync=dirty-bitmap John Snow
2014-11-25 19:55 ` [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 1 +
qapi/block-core.json | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index cf93148..3acf289 100644
--- a/block.c
+++ b/block.c
@@ -5404,6 +5404,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
info->granularity = bdrv_dirty_bitmap_granularity(bs, bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
+ info->enabled = bm->enabled;
entry->value = info;
*plist = entry;
plist = &entry->next;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3f7a35d..c9d673b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -310,10 +310,13 @@
#
# @granularity: granularity of the dirty bitmap in bytes (since 1.4)
#
+# @enabled: whether the dirty bitmap is enabled (Since 2.3)
+#
# Since: 1.3
##
{ 'type': 'BlockDirtyInfo',
- 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
+ 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int',
+ 'enabled': 'bool'} }
##
# @BlockInfo:
--
1.9.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Qemu-devel] [2.3 PATCH v7 10/10] qemu-iotests: Add tests for drive-backup sync=dirty-bitmap
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (8 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 09/10] qmp: Add dirty bitmap 'enabled' field in query-block John Snow
@ 2014-11-25 19:46 ` John Snow
2014-11-26 14:52 ` Max Reitz
2014-11-25 19:55 ` [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
10 siblings, 1 reply; 27+ messages in thread
From: John Snow @ 2014-11-25 19:46 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, Fam Zheng
From: Fam Zheng <famz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/056 | 33 ++++++++++++++++++++++++++++++---
tests/qemu-iotests/056.out | 4 ++--
tests/qemu-iotests/iotests.py | 8 ++++++++
3 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index 54e4bd0..fc9114e 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -23,17 +23,17 @@
import time
import os
import iotests
-from iotests import qemu_img, qemu_io, create_image
+from iotests import qemu_img, qemu_img_map_assert, qemu_io, create_image
backing_img = os.path.join(iotests.test_dir, 'backing.img')
test_img = os.path.join(iotests.test_dir, 'test.img')
target_img = os.path.join(iotests.test_dir, 'target.img')
-class TestSyncModesNoneAndTop(iotests.QMPTestCase):
+class TestSyncModes(iotests.QMPTestCase):
image_len = 64 * 1024 * 1024 # MB
def setUp(self):
- create_image(backing_img, TestSyncModesNoneAndTop.image_len)
+ create_image(backing_img, TestSyncModes.image_len)
qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
qemu_io('-c', 'write -P0x41 0 512', test_img)
qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
@@ -64,6 +64,33 @@ class TestSyncModesNoneAndTop(iotests.QMPTestCase):
self.assertTrue(iotests.compare_images(test_img, target_img),
'target image does not match source after backup')
+ def test_sync_dirty_bitmap_missing(self):
+ self.assert_no_active_block_jobs()
+ result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
+ format=iotests.imgfmt, target=target_img)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ def test_sync_dirty_bitmap_not_found(self):
+ self.assert_no_active_block_jobs()
+ result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
+ bitmap='unknown',
+ format=iotests.imgfmt, target=target_img)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ def test_sync_dirty_bitmap(self):
+ self.assert_no_active_block_jobs()
+ result = self.vm.qmp('block-dirty-bitmap-add', device='drive0', name='bitmap0')
+ self.assert_qmp(result, 'return', {})
+ self.vm.hmp_qemu_io('drive0', 'write -P0x5a 0 512')
+ self.vm.hmp_qemu_io('drive0', 'write -P0x5a 48M 512')
+ result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
+ bitmap='bitmap0',
+ format=iotests.imgfmt, target=target_img)
+ self.assert_qmp(result, 'return', {})
+ self.wait_until_completed(check_offset=False)
+ self.assert_no_active_block_jobs()
+ qemu_img_map_assert(target_img, [0, 0x3000000])
+
def test_cancel_sync_none(self):
self.assert_no_active_block_jobs()
diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
index fbc63e6..914e373 100644
--- a/tests/qemu-iotests/056.out
+++ b/tests/qemu-iotests/056.out
@@ -1,5 +1,5 @@
-..
+.....
----------------------------------------------------------------------
-Ran 2 tests
+Ran 5 tests
OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f57f154..95bb959 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -55,6 +55,14 @@ def qemu_img_pipe(*args):
'''Run qemu-img and return its output'''
return subprocess.Popen(qemu_img_args + list(args), stdout=subprocess.PIPE).communicate()[0]
+def qemu_img_map_assert(img, offsets):
+ '''Run qemu-img map on img and check the mapped ranges'''
+ offs = []
+ for line in qemu_img_pipe('map', img).splitlines()[1:]:
+ offset, length, mapped, fname = line.split()
+ offs.append(int(offset, 16))
+ assert set(offs) == set(offsets), "mapped offsets in image '%s' not equal to '%s'" % (str(offs), str(offsets))
+
def qemu_io(*args):
'''Run qemu-io and return the stdout data'''
args = qemu_io_args + list(args)
--
1.9.3
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series
2014-11-25 19:46 [Qemu-devel] [2.3 PATCH v7 00/10] block: Incremental backup series John Snow
` (9 preceding siblings ...)
2014-11-25 19:46 ` [Qemu-devel] [2.3 PATCH v7 10/10] qemu-iotests: Add tests for drive-backup sync=dirty-bitmap John Snow
@ 2014-11-25 19:55 ` John Snow
10 siblings, 0 replies; 27+ messages in thread
From: John Snow @ 2014-11-25 19:55 UTC (permalink / raw)
To: qemu-devel
Cc: kwo >> Kevin Wolf, Fam Zheng, mre >> Max Reitz,
vladimir Sementsov-Ogievskiy, Stefan Hajnoczi,
pbon >> Paolo Bonzini
On 11/25/2014 02:46 PM, John Snow wrote:
> This is the in memory part of the incremental backup feature.
>
> With the added commands, we can create a bitmap on a block backend, from which
> point of time all the writes are tracked by the bitmap, marking sectors as
> dirty. Later, we call drive-backup and pass the bitmap to it, to do an
> incremental backup.
>
> See the last patch which adds some tests for this use case.
>
> Fam
>
> ==
>
> This is the next iteration of Fam's incremenetal backup feature.
> I have since taken this over from him and have (slowly) worked through
> the feedback to the last version of his patch and have made many
> tiny little edits.
>
> -John.
>
> v7: (highlights)
> - First version being authored by jsnow
> - Addressed most list feedback from V6, many small changes.
> All feedback was either addressed on-list (as a wontfix) or patched.
> - Replaced all error_set with error_setg
> - Replaced all bdrv_find with bdrv_lookup_bs()
> - Adjusted the max granularity to share a common function with
> backup/mirror that attempts to "guess" a reasonable default.
> It clamps between [4K,64K] currently.
> - The BdrvDirtyBitmap object now counts granularity exclusively in
> bytes to match its interface.
> It leaves the sector granularity concerns to HBitmap.
> - Reworked the backup loop to utilize the hbitmap iterator.
> There are some extra concerns to handle arrhythmic cases where the
> granularity of the bitmap does not match the backup cluster size.
> This iteration works best when it does match, but it's not a
> deal-breaker if it doesn't -- it just gets less efficient.
> - Reworked the transactional functions so that abort() wouldn't "undo"
> a redundant command. They now have been split into a prepare and a
> commit function (with state) and do not provide an abort command.
> - Added a block_dirty_bitmap_lookup(device, name, errp) function to
> shorten a few of the commands added in this series, particularly
> qmp_enable, qmp_disable, and the transaction preparations.
>
> v6: Re-send of v5.
>
> v5: Rebase to master.
>
> v4: Last version tailored by Fam Zheng.
>
> Fam Zheng (10):
> qapi: Add optional field "name" to block dirty bitmap
> qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
> block: Introduce bdrv_dirty_bitmap_granularity()
> hbitmap: Add hbitmap_copy
> block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
> qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable
> qmp: Add support of "dirty-bitmap" sync mode for drive-backup
> qapi: Add transaction support to block-dirty-bitmap-{add, enable,
> disable}
> qmp: Add dirty bitmap 'enabled' field in query-block
> qemu-iotests: Add tests for drive-backup sync=dirty-bitmap
>
> block-migration.c | 2 +-
> block.c | 114 +++++++++++++++++++--
> block/backup.c | 128 ++++++++++++++++++++----
> block/mirror.c | 16 ++-
> blockdev.c | 226 +++++++++++++++++++++++++++++++++++++++++-
> hmp.c | 4 +-
> include/block/block.h | 17 +++-
> include/block/block_int.h | 6 ++
> include/qemu/hbitmap.h | 8 ++
> qapi-schema.json | 5 +-
> qapi/block-core.json | 123 ++++++++++++++++++++++-
> qmp-commands.hx | 66 +++++++++++-
> tests/qemu-iotests/056 | 33 +++++-
> tests/qemu-iotests/056.out | 4 +-
> tests/qemu-iotests/iotests.py | 8 ++
> util/hbitmap.c | 16 +++
> 16 files changed, 722 insertions(+), 54 deletions(-)
>
Script missed the CCs. Sigh.
For convenience:
Github: https://github.com/jnsnow/qemu/tree/dbm-backup
Git: https://github.com/jnsnow/qemu.git
Thanks,
--js
^ permalink raw reply [flat|nested] 27+ messages in thread