* [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series
@ 2015-03-02 23:19 John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation John Snow
` (18 more replies)
0 siblings, 19 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
New topic, new version, same great incremental backup taste.
This series relies on [Qemu-devel] [PULL 30/69] blkdebug: fix "once" rule,
part of Stefan's 69 patch pull request submitted 2015-02-27.
This patchset enables the in-memory part of the incremental backup
feature, without transactional support.
Support for transactions will come at a later date, but getting this
portion upstream will help stabilize work on bitmap persistence and
bitmap migration.
Transactional support is being postponed to allow more development
to create a transactional callback system that will allow objects used
by the drive backup routines to perform cleanup actions conditionally
based on the outcome of all backups in the transaction group.
This series was once said to have been written by a man known only
to Qemu-Devel as "Fam Zheng", A programming maverick hellbent on never
again wasting space by making full backups when he did not have to. The
true origins of this series are now lost to the sands of time.
(12+ revisions later: Thanks, Fam!)
===
v2:
===
01: Added a new opening blurb.
Adjusted codeblock indentations to be 4 spaces instead of 3,
so it works as MD or GFMD.
Adjusted errors explanation.
Make visual separations between json data and shell commands
Eliminate any ligering single quotes
07: Remember that memset takes bytes, not n_items ...
===
v1:
===
Deletions:
- Removed Transactions, to be added later.
- Removed Transaction tests, as above.
Changes:
01: Indentation fixes.
Removed enable/disable documentation.
Added a note that transactions aren't implemented yet.
Removed my needless commas
Added error case documentation.
07: QMP enable/disable commands are deleted.
14: Some comments concerning assertions.
Scrub re-alloc memory if we expand the array.
Do not attempt to scrub memory if fix_count is 0
Changes made with Reviews kept:
02: Since 2.4
04: Since 2.4
Demingled the QMP command documentation.
08: Additions to what was qmp_block_dirty_enable/disable
are no longer present as those function no longer exist.
09: Since 2.4
10: Since 2.4
Demingled QMP command documentation.
11: Since 2.4
15: Test 112 --> 124
17: Number of tests altered. (Only 4, now.)
Fam Zheng (1):
qapi: Add optional field "name" to block dirty bitmap
John Snow (16):
docs: incremental backup documentation
qmp: Ensure consistent granularity type
qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
block: Introduce bdrv_dirty_bitmap_granularity()
hbitmap: add hbitmap_merge
block: Add bitmap disabled status
block: Add bitmap successors
qmp: Add support of "dirty-bitmap" sync mode for drive-backup
qmp: add block-dirty-bitmap-clear
qmp: Add dirty bitmap status fields in query-block
block: add BdrvDirtyBitmap documentation
block: Ensure consistent bitmap function prototypes
block: Resize bitmaps on bdrv_truncate
iotests: add invalid input incremental backup tests
iotests: add simple incremental backup case
iotests: add incremental backup failure recovery test
block.c | 248 +++++++++++++++++++++++++++++++--
block/backup.c | 149 ++++++++++++++++----
block/mirror.c | 46 +++----
blockdev.c | 162 +++++++++++++++++++++-
docs/bitmaps.md | 311 ++++++++++++++++++++++++++++++++++++++++++
hmp.c | 3 +-
include/block/block.h | 34 ++++-
include/block/block_int.h | 4 +-
include/qemu/hbitmap.h | 21 +++
migration/block.c | 9 +-
qapi/block-core.json | 92 ++++++++++++-
qmp-commands.hx | 92 ++++++++++++-
tests/qemu-iotests/124 | 266 ++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/124.out | 5 +
tests/qemu-iotests/group | 1 +
tests/qemu-iotests/iotests.py | 6 +-
util/hbitmap.c | 87 ++++++++++++
17 files changed, 1446 insertions(+), 90 deletions(-)
create mode 100644 docs/bitmaps.md
create mode 100644 tests/qemu-iotests/124
create mode 100644 tests/qemu-iotests/124.out
--
1.9.3
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-03 15:28 ` Max Reitz
2015-03-11 13:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 02/17] qapi: Add optional field "name" to block dirty bitmap John Snow
` (17 subsequent siblings)
18 siblings, 2 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/bitmaps.md | 311 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 311 insertions(+)
create mode 100644 docs/bitmaps.md
diff --git a/docs/bitmaps.md b/docs/bitmaps.md
new file mode 100644
index 0000000..df1c134
--- /dev/null
+++ b/docs/bitmaps.md
@@ -0,0 +1,311 @@
+# Dirty Bitmaps and Incremental Backup
+
+* Dirty Bitmaps are objects that track which data needs to be backed up for the
+ next incremental backup.
+
+* Dirty bitmaps can be created at any time and attached to any node
+ (not just complete drives.)
+
+## Dirty Bitmap Names
+
+* A dirty bitmap's name is unique to the node, but bitmaps attached to different
+nodes can share the same name.
+
+## Bitmap Modes
+
+* A Bitmap can be "enabled" (tracking writes, the default) or "disabled"
+(read-only, I/O is ignored.) This state is currently only changed internally
+for the purposes of migration, and otherwise remains enabled.
+
+* A Bitmap can be "frozen," which means that it is currently in-use by a backup
+operation and cannot be deleted, enabled, disabled, renamed, written to, reset,
+etc.
+
+## Basic QMP Usage
+
+### Supported Commands ###
+
+* block-dirty-bitmap-add
+* block-dirty-bitmap-remove
+* block-dirty-bitmap-clear
+
+### Creation
+
+* To create a new bitmap, enabled, on the drive with id=drive0:
+
+```json
+{ "execute": "block-dirty-bitmap-add",
+ "arguments": {
+ "node": "drive0",
+ "name": "bitmap0"
+ }
+}
+```
+
+* This bitmap will have a default granularity that matches the cluster size of
+its associated drive, if available, clamped to between [4KiB, 64KiB].
+The current default for qcow2 is 64KiB.
+
+* To create a new bitmap that tracks changes in 32KiB segments:
+
+```json
+{ "execute": "block-dirty-bitmap-add",
+ "arguments": {
+ "node": "drive0",
+ "name": "bitmap0",
+ "granularity": 32768
+ }
+}
+```
+
+### Deletion
+
+* Can be performed on a disabled bitmap, but not a frozen one.
+
+* Because bitmaps are only unique to the node to which they are attached,
+you must specify the node/drive name here, too.
+
+```json
+{ "execute": "block-dirty-bitmap-remove",
+ "arguments": {
+ "node": "drive0",
+ "name": "bitmap0"
+ }
+}
+```
+
+### Resetting
+
+* Resetting a bitmap will clear all information it holds.
+* An incremental backup created from an empty bitmap will copy no data,
+as if nothing has changed.
+
+```json
+{ "execute": "block-dirty-bitmap-clear",
+ "arguments": {
+ "node": "drive0",
+ "name": "bitmap0"
+ }
+}
+```
+
+## Transactions (Not yet implemented)
+
+* Transactional commands are forthcoming in a future version,
+ and are not yet available for use. This section serves as
+ documentation of intent for their design and usage.
+
+### Justification
+Bitmaps can be safely modified when the VM is paused or halted by using
+the basic QMP commands. For instance, you might perform the following actions:
+
+1. Boot the VM in a paused state.
+2. Create a full drive backup of drive0.
+3. Create a new bitmap attached to drive0.
+4. Resume execution of the VM.
+5. Incremental backups are ready to be created.
+
+At this point, the bitmap and drive backup would be correctly in sync,
+and incremental backups made from this point forward would be correctly aligned
+to the full drive backup.
+
+This is not particularly useful if we decide we want to start incremental
+backups after the VM has been running for a while, for which we will need to
+perform actions such as the following:
+
+1. Boot the VM and begin execution.
+2. Using a single transaction, perform the following operations:
+ * Create bitmap0.
+ * Create a full drive backup of drive0.
+3. Incremental backups are now ready to be created.
+
+### Supported Bitmap Transactions
+
+* block-dirty-bitmap-add
+* block-dirty-bitmap-clear
+
+The usages are identical to their respective QMP commands, but see below
+for examples.
+
+### Example: New Incremental Backup
+
+As outlined in the justification, perhaps we want to create a new incremental
+backup chain attached to a drive.
+
+```json
+{ "execute": "transaction",
+ "arguments": {
+ "actions": [
+ {"type": "block-dirty-bitmap-add",
+ "data": {"node": "drive0", "name": "bitmap0"} },
+ {"type": "drive-backup",
+ "data": {"device": "drive0", "target": "/path/to/full_backup.img",
+ "sync": "full", "format": "qcow2"} }
+ ]
+ }
+}
+```
+
+### Example: New Incremental Backup Anchor Point
+
+Maybe we just want to create a new full backup with an existing bitmap and
+want to reset the bitmap to track the new chain.
+
+```json
+{ "execute": "transaction",
+ "arguments": {
+ "actions": [
+ {"type": "block-dirty-bitmap-clear",
+ "data": {"node": "drive0", "name": "bitmap0"} },
+ {"type": "drive-backup",
+ "data": {"device": "drive0", "target": "/path/to/new_full_backup.img",
+ "sync": "full", "format": "qcow2"} }
+ ]
+ }
+}
+```
+
+## Incremental Backups
+
+The star of the show.
+
+**Nota Bene!** Only incremental backups of entire drives are supported for now.
+So despite the fact that you can attach a bitmap to any arbitrary node, they are
+only currently useful when attached to the root node. This is because
+drive-backup only supports drives/devices instead of arbitrary nodes.
+
+### Example: First Incremental Backup
+
+1. Create a full backup and sync it to the dirty bitmap, as in the transactional
+examples above; or with the VM offline, manually create a full copy and then
+create a new bitmap before the VM begins execution.
+
+ * Let's assume the full backup is named 'full_backup.img'.
+ * Let's assume the bitmap you created is 'bitmap0' attached to 'drive0'.
+
+2. Create a destination image for the incremental backup that utilizes the
+full backup as a backing image.
+
+ * Let's assume it is named 'incremental.0.img'.
+
+ ```sh
+ # qemu-img create -f qcow2 incremental.0.img -b full_backup.img -F qcow2
+ ```
+
+3. Issue the incremental backup command:
+
+ ```json
+ { "execute": "drive-backup",
+ "arguments": {
+ "device": "drive0",
+ "bitmap": "bitmap0",
+ "target": "incremental.0.img",
+ "format": "qcow2",
+ "sync": "dirty-bitmap",
+ "mode": "existing"
+ }
+ }
+ ```
+
+### Example: Second Incremental Backup
+
+1. Create a new destination image for the incremental backup that points to the
+ previous one, e.g.: 'incremental.1.img'
+
+ ```sh
+ # qemu-img create -f qcow2 incremental.1.img -b incremental.0.img -F qcow2
+ ```
+
+2. Issue a new incremental backup command. The only difference here is that we
+ have changed the target image below.
+
+ ```json
+ { "execute": "drive-backup",
+ "arguments": {
+ "device": "drive0",
+ "bitmap": "bitmap0",
+ "target": "incremental.1.img",
+ "format": "qcow2",
+ "sync": "dirty-bitmap",
+ "mode": "existing"
+ }
+ }
+ ```
+
+## Errors
+
+* In the event of an error that occurs after a backup job is successfully
+ launched, either by a direct QMP command or a QMP transaction, the user
+ will receive a BLOCK_JOB_COMPLETE event with a failure message.
+
+* In this case, the incremental backup data contained within the bitmap is
+ safely rolled back, and the data within the bitmap is not lost. The image
+ file created for the failed attempt can be safely deleted.
+
+* Once the underlying problem is fixed (e.g. more storage space is freed up),
+ you can simply retry the incremental backup command with the same bitmap.
+
+### Example
+
+1. Create a target image:
+
+ ```sh
+ # qemu-img create -f qcow2 incremental.0.img -b full_backup.img -F qcow2
+ ```
+
+2. Attempt to create an incremental backup via QMP:
+
+ ```json
+ { "execute": "drive-backup",
+ "arguments": {
+ "device": "drive0",
+ "bitmap": "bitmap0",
+ "target": "incremental.0.img",
+ "format": "qcow2",
+ "sync": "dirty-bitmap",
+ "mode": "existing"
+ }
+ }
+ ```
+
+3. Receive an event notifying us of failure:
+
+ ```json
+ { "timestamp": { "seconds": 1424709442, "microseconds": 844524 },
+ "data": { "speed": 0, "offset": 0, "len": 67108864,
+ "error": "No space left on device",
+ "device": "drive1", "type": "backup" },
+ "event": "BLOCK_JOB_COMPLETED" }
+ ```
+
+4. Delete the failed incremental, and re-create the image.
+
+ ```sh
+ # rm incremental.0.img
+ # qemu-img create -f qcow2 incremental.0.img -b full_backup.img -F qcow2
+ ```
+
+5. Retry the command after fixing the underlaying problem,
+ such as freeing up space on the backup volume:
+
+ ```json
+ { "execute": "drive-backup",
+ "arguments": {
+ "device": "drive0",
+ "bitmap": "bitmap0",
+ "target": "incremental.0.img",
+ "format": "qcow2",
+ "sync": "dirty-bitmap",
+ "mode": "existing"
+ }
+ }
+ ```
+
+6. Receive confirmation that the job completed successfully:
+
+ ```json
+ { "timestamp": { "seconds": 1424709668, "microseconds": 526525 },
+ "data": { "device": "drive1", "type": "backup",
+ "speed": 0, "len": 67108864, "offset": 67108864},
+ "event": "BLOCK_JOB_COMPLETED" }
+ ```
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 02/17] qapi: Add optional field "name" to block dirty bitmap
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-11 15:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 03/17] qmp: Ensure consistent granularity type John Snow
` (16 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
From: Fam Zheng <famz@redhat.com>
This field will be set for user created dirty bitmap. Also pass in an
error pointer to bdrv_create_dirty_bitmap, so when a name is already
taken on this BDS, it can report an error message. This is not global
check, two BDSes can have dirty bitmap with a common name.
Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
be used later when other QMP commands want to reference dirty bitmap by
name.
Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 32 +++++++++++++++++++++++++++++++-
block/mirror.c | 2 +-
include/block/block.h | 7 ++++++-
migration/block.c | 2 +-
qapi/block-core.json | 4 +++-
5 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index e2408f2..99c6e00 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
+ char *name;
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -5404,7 +5405,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;
@@ -5412,6 +5434,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);
@@ -5422,6 +5448,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;
}
@@ -5433,6 +5460,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;
}
@@ -5451,6 +5479,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
info->count = bdrv_get_dirty_count(bs, bm);
info->granularity =
((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+ info->has_name = !!bm->name;
+ info->name = g_strdup(bm->name);
entry->value = info;
*plist = entry;
plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index 4056164..f073ad7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -703,7 +703,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
s->granularity = granularity;
s->buf_size = MAX(buf_size, granularity);
- s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, errp);
+ s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
if (!s->dirty_bitmap) {
return;
}
diff --git a/include/block/block.h b/include/block/block.h
index cea8a2a..ece4270 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -450,8 +450,13 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
struct HBitmapIter;
typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+ int granularity,
+ const char *name,
Error **errp);
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
+ const char *name);
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
diff --git a/migration/block.c b/migration/block.c
index 0c76106..5f9b3e5 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -319,7 +319,7 @@ static int set_dirty_tracking(void)
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
- NULL);
+ NULL, NULL);
if (!bmds->dirty_bitmap) {
ret = -errno;
goto fail;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 90586a5..787fb4b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -329,6 +329,8 @@
#
# Block dirty bitmap information.
#
+# @name: #optional the name of the dirty bitmap (Since 2.4)
+#
# @count: number of dirty bytes according to the dirty bitmap
#
# @granularity: granularity of the dirty bitmap in bytes (since 1.4)
@@ -336,7 +338,7 @@
# Since: 1.3
##
{ 'type': 'BlockDirtyInfo',
- 'data': {'count': 'int', 'granularity': 'int'} }
+ 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
##
# @BlockInfo:
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 03/17] qmp: Ensure consistent granularity type
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 02/17] qapi: Add optional field "name" to block dirty bitmap John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-11 15:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 04/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
` (15 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
We treat this field with a variety of different types everywhere
in the code. Now it's just uint32_t.
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 11 ++++++-----
block/mirror.c | 4 ++--
include/block/block.h | 2 +-
include/block/block_int.h | 2 +-
qapi/block-core.json | 2 +-
5 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 99c6e00..2c3cadc 100644
--- a/block.c
+++ b/block.c
@@ -5425,12 +5425,13 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
}
BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
- int granularity,
+ uint32_t granularity,
const char *name,
Error **errp)
{
int64_t bitmap_size;
BdrvDirtyBitmap *bitmap;
+ uint32_t sector_granularity;
assert((granularity & (granularity - 1)) == 0);
@@ -5438,8 +5439,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");
@@ -5447,7 +5448,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return NULL;
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
- bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+ bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
bitmap->name = g_strdup(name);
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
return bitmap;
@@ -5478,7 +5479,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
info->count = bdrv_get_dirty_count(bs, bm);
info->granularity =
- ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+ ((uint32_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
entry->value = info;
diff --git a/block/mirror.c b/block/mirror.c
index f073ad7..f8aac33 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -656,7 +656,7 @@ static const BlockJobDriver commit_active_job_driver = {
static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
const char *replaces,
- int64_t speed, int64_t granularity,
+ int64_t speed, uint32_t granularity,
int64_t buf_size,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
@@ -717,7 +717,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
void mirror_start(BlockDriverState *bs, BlockDriverState *target,
const char *replaces,
- int64_t speed, int64_t granularity, int64_t buf_size,
+ int64_t speed, uint32_t granularity, int64_t buf_size,
MirrorSyncMode mode, BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb,
diff --git a/include/block/block.h b/include/block/block.h
index ece4270..21d70e6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -451,7 +451,7 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
struct HBitmapIter;
typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
- int granularity,
+ uint32_t granularity,
const char *name,
Error **errp);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dccb092..fb9e100 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -590,7 +590,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
*/
void mirror_start(BlockDriverState *bs, BlockDriverState *target,
const char *replaces,
- int64_t speed, int64_t granularity, int64_t buf_size,
+ int64_t speed, uint32_t granularity, int64_t buf_size,
MirrorSyncMode mode, BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 787fb4b..678e91f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -338,7 +338,7 @@
# Since: 1.3
##
{ 'type': 'BlockDirtyInfo',
- 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
+ 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} }
##
# @BlockInfo:
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 04/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (2 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 03/17] qmp: Ensure consistent granularity type John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-11 13:58 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 05/17] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
` (14 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
The new command pair is added to manage a user created dirty bitmap. The
dirty bitmap's name is mandatory and must be unique for the same device,
but different devices can have bitmaps with the same names.
The granularity is an optional field. If it is not specified, we will
choose a default granularity based on the cluster size if available,
clamped to between 4K and 64K to mirror how the 'mirror' code was
already choosing granularity. If we do not have cluster size info
available, we choose 64K. This code has been factored out into a helper
shared with block/mirror.
This patch also introduces the 'block_dirty_bitmap_lookup' helper,
which takes a device name and a dirty bitmap name and validates the
lookup, returning NULL and setting errp if there is a problem with
either field. This helper will be re-used in future patches in this
series.
The types added to block-core.json will be re-used in future patches
in this series, see:
'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 20 ++++++++++
block/mirror.c | 10 +----
blockdev.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 +
qapi/block-core.json | 55 +++++++++++++++++++++++++++
qmp-commands.hx | 56 ++++++++++++++++++++++++++++
6 files changed, 233 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 2c3cadc..c627448 100644
--- a/block.c
+++ b/block.c
@@ -5499,6 +5499,26 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
}
}
+/**
+ * Chooses a default granularity based on the existing cluster size,
+ * but clamped between [4K, 64K]. Defaults to 64K in the case that there
+ * is no cluster size information available.
+ */
+uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
+{
+ BlockDriverInfo bdi;
+ uint32_t granularity;
+
+ if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
+ granularity = MAX(4096, bdi.cluster_size);
+ granularity = MIN(65536, granularity);
+ } else {
+ granularity = 65536;
+ }
+
+ return granularity;
+}
+
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
{
diff --git a/block/mirror.c b/block/mirror.c
index f8aac33..1cb700e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -668,15 +668,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
MirrorBlockJob *s;
if (granularity == 0) {
- /* Choose the default granularity based on the target file's cluster
- * size, clamped between 4k and 64k. */
- BlockDriverInfo bdi;
- if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) {
- granularity = MAX(4096, bdi.cluster_size);
- granularity = MIN(65536, granularity);
- } else {
- granularity = 65536;
- }
+ granularity = bdrv_get_default_bitmap_granularity(target);
}
assert ((granularity & (granularity - 1)) == 0);
diff --git a/blockdev.c b/blockdev.c
index ae73539..c40c08c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1162,6 +1162,48 @@ out_aio_context:
return NULL;
}
+/**
+ * Return a dirty bitmap (if present), after validating
+ * the node reference and bitmap names. Returns NULL on error,
+ * including when the BDS and/or bitmap is not found.
+ */
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
+ const char *name,
+ BlockDriverState **pbs,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ if (!node) {
+ error_setg(errp, "Node cannot be NULL");
+ return NULL;
+ }
+ if (!name) {
+ error_setg(errp, "Bitmap name cannot be NULL");
+ return NULL;
+ }
+
+ bs = bdrv_lookup_bs(node, node, NULL);
+ if (!bs) {
+ error_setg(errp, "Node '%s' not found", node);
+ return NULL;
+ }
+
+ /* If caller provided a BDS*, provide the result of that lookup, too. */
+ if (pbs) {
+ *pbs = bs;
+ }
+
+ bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Dirty bitmap '%s' not found", name);
+ return NULL;
+ }
+
+ return bitmap;
+}
+
/* New and old BlockDriverState structs for atomic group operations */
typedef struct BlkTransactionState BlkTransactionState;
@@ -1942,6 +1984,64 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
aio_context_release(aio_context);
}
+void qmp_block_dirty_bitmap_add(const char *node, const char *name,
+ bool has_granularity, uint32_t granularity,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BlockDriverState *bs;
+
+ if (!name || name[0] == '\0') {
+ error_setg(errp, "Bitmap name cannot be empty");
+ return;
+ }
+
+ bs = bdrv_lookup_bs(node, node, errp);
+ if (!bs) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (has_granularity) {
+ if (granularity < 512 || !is_power_of_2(granularity)) {
+ error_setg(errp, "Granularity must be power of 2 "
+ "and at least 512");
+ goto out;
+ }
+ } else {
+ /* Default to cluster size, if available: */
+ granularity = bdrv_get_default_bitmap_granularity(bs);
+ }
+
+ bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+
+ out:
+ aio_context_release(aio_context);
+}
+
+void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap || !bs) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ bdrv_dirty_bitmap_make_anon(bs, bitmap);
+ bdrv_release_dirty_bitmap(bs, bitmap);
+
+ aio_context_release(aio_context);
+}
+
int hmp_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 21d70e6..b2d019b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -459,6 +459,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
+uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 678e91f..e2ad437 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -962,6 +962,61 @@
'*on-target-error': 'BlockdevOnError' } }
##
+# @BlockDirtyBitmap
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# Since 2.4
+##
+{ 'type': 'BlockDirtyBitmap',
+ 'data': { 'node': 'str', 'name': 'str' } }
+
+##
+# @BlockDirtyBitmapAdd
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# @granularity: #optional the bitmap granularity, default is 64k for
+# block-dirty-bitmap-add
+#
+# Since 2.4
+##
+{ 'type': 'BlockDirtyBitmapAdd',
+ 'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+
+##
+# @block-dirty-bitmap-add
+#
+# Create a dirty bitmap with a name on the node
+#
+# Returns: nothing on success
+# If @node is not a valid block device or node, DeviceNotFound
+# If @name is already taken, GenericError with an explanation
+#
+# Since 2.4
+##
+{ 'command': 'block-dirty-bitmap-add',
+ 'data': 'BlockDirtyBitmapAdd' }
+
+##
+# @block-dirty-bitmap-remove
+#
+# Remove a dirty bitmap on the node
+#
+# Returns: nothing on success
+# If @node is not a valid block device or node, DeviceNotFound
+# If @name is not found, GenericError with an explanation
+#
+# Since 2.4
+##
+{ 'command': 'block-dirty-bitmap-remove',
+ 'data': 'BlockDirtyBitmap' }
+
+##
# @block_set_io_throttle:
#
# Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a85d847..5c5c0cb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1244,6 +1244,62 @@ Example:
EQMP
{
+ .name = "block-dirty-bitmap-add",
+ .args_type = "node:B,name:s,granularity:i?",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
+ },
+
+SQMP
+
+block-dirty-bitmap-add
+----------------------
+Since 2.4
+
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+
+Arguments:
+
+- "node": device/node on which to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with (int, optional)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-add", "arguments": { "node": "drive0",
+ "name": "bitmap0" } }
+<- { "return": {} }
+
+EQMP
+
+ {
+ .name = "block-dirty-bitmap-remove",
+ .args_type = "node:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
+ },
+
+SQMP
+
+block-dirty-bitmap-remove
+-------------------------
+Since 2.4
+
+Stop write tracking and remove the dirty bitmap that was created with
+block-dirty-bitmap-add.
+
+Arguments:
+
+- "node": device/node on which to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node": "drive0",
+ "name": "bitmap0" } }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "blockdev-snapshot-sync",
.args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 05/17] block: Introduce bdrv_dirty_bitmap_granularity()
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (3 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 04/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-11 15:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 06/17] hbitmap: add hbitmap_merge John Snow
` (13 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
This returns the granularity (in bytes) of dirty bitmap,
which matches the QMP interface and the existing query
interface.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 8 ++++++--
include/block/block.h | 1 +
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index c627448..9a82826 100644
--- a/block.c
+++ b/block.c
@@ -5478,8 +5478,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
info->count = bdrv_get_dirty_count(bs, bm);
- info->granularity =
- ((uint32_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+ info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
entry->value = info;
@@ -5519,6 +5518,11 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
return granularity;
}
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+{
+ return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+}
+
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
{
diff --git a/include/block/block.h b/include/block/block.h
index b2d019b..2a11fbc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -460,6 +460,7 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 06/17] hbitmap: add hbitmap_merge
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (4 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 05/17] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-11 15:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 07/17] block: Add bitmap disabled status John Snow
` (12 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
We add a bitmap merge operation to assist in error cases
where we wish to combine two bitmaps together.
This is algorithmically O(bits) provided HBITMAP_LEVELS remains
constant. For a full bitmap on a 64bit machine:
sum(bits/64^k, k, 0, HBITMAP_LEVELS) ~= 1.01587 * bits
We may be able to improve running speed for particularly sparse
bitmaps by using iterators, but the running time for dense maps
will be worse.
We present the simpler solution first, and we can refine it later
if needed.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/qemu/hbitmap.h | 11 +++++++++++
util/hbitmap.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 550d7ce..c19c1cb 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,17 @@ struct HBitmapIter {
HBitmap *hbitmap_alloc(uint64_t size, int granularity);
/**
+ * hbitmap_merge:
+ * @a: The bitmap to store the result in.
+ * @b: The bitmap to merge into @a.
+ *
+ * Merge two bitmaps together.
+ * A := A (BITOR) B.
+ * B is left unmodified.
+ */
+bool hbitmap_merge(HBitmap *a, const HBitmap *b);
+
+/**
* hbitmap_empty:
* @hb: HBitmap to operate on.
*
diff --git a/util/hbitmap.c b/util/hbitmap.c
index ab13971..962ff29 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -395,3 +395,35 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
return hb;
}
+
+/**
+ * Given HBitmaps A and B, let A := A (BITOR) B.
+ * Bitmap B will not be modified.
+ */
+bool hbitmap_merge(HBitmap *a, const HBitmap *b)
+{
+ int i, j;
+ uint64_t size;
+
+ if ((a->size != b->size) || (a->granularity != b->granularity)) {
+ return false;
+ }
+
+ if (hbitmap_count(b) == 0) {
+ return true;
+ }
+
+ /* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
+ * It may be possible to improve running times for sparsely populated maps
+ * by using hbitmap_iter_next, but this is suboptimal for dense maps.
+ */
+ size = a->size;
+ for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
+ size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+ for (j = 0; j < size; j++) {
+ a->levels[i][j] |= b->levels[i][j];
+ }
+ }
+
+ return true;
+}
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 07/17] block: Add bitmap disabled status
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (5 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 06/17] hbitmap: add hbitmap_merge John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-11 16:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 08/17] block: Add bitmap successors John Snow
` (11 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Add a status indicating the enabled/disabled state of the bitmap.
A bitmap is by default enabled, but you can lock the bitmap into
a read-only state by setting disabled = true.
A previous version of this patch added a QMP interface for changing
the state of the bitmap, but it has since been removed for now until
a use case emerges where this state must be revealed to the user.
The disabled state WILL be used internally for bitmap migration and
bitmap persistence.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 25 +++++++++++++++++++++++++
include/block/block.h | 3 +++
2 files changed, 28 insertions(+)
diff --git a/block.c b/block.c
index 9a82826..7e71aba 100644
--- a/block.c
+++ b/block.c
@@ -54,6 +54,7 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
char *name;
+ bool disabled;
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -5450,10 +5451,16 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
bitmap->name = g_strdup(name);
+ bitmap->disabled = false;
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
return bitmap;
}
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
+{
+ return !bitmap->disabled;
+}
+
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
BdrvDirtyBitmap *bm, *next;
@@ -5468,6 +5475,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
}
}
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ bitmap->disabled = true;
+}
+
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ bitmap->disabled = false;
+}
+
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
{
BdrvDirtyBitmap *bm;
@@ -5532,12 +5549,14 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
@@ -5546,6 +5565,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
{
BdrvDirtyBitmap *bitmap;
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ continue;
+ }
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
}
@@ -5555,6 +5577,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
{
BdrvDirtyBitmap *bitmap;
QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ continue;
+ }
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
}
diff --git a/include/block/block.h b/include/block/block.h
index 2a11fbc..879a3af 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -458,9 +458,12 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 08/17] block: Add bitmap successors
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (6 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 07/17] block: Add bitmap disabled status John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 09/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
` (10 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
be created just prior to a sensitive operation (e.g. Incremental Backup)
that can either succeed or fail, but during the course of which we still
want a bitmap tracking writes.
On creating a successor, we "freeze" the parent bitmap which prevents
its deletion, enabling, anonymization, or creating a bitmap with the
same name.
On success, the parent bitmap can "abdicate" responsibility to the
successor, which will inherit its name. The successor will have been
tracking writes during the course of the backup operation. The parent
will be safely deleted.
On failure, we can "reclaim" the successor from the parent, unifying
them such that the resulting bitmap describes all writes occurring since
the last successful backup, for instance. Reclamation will thaw the
parent, but not explicitly re-enable it.
BdrvDirtyBitmap operations that target a single bitmap are protected
by assertions that the bitmap is not frozen and/or disabled.
BdrvDirtyBitmap operations that target a group of bitmaps, such as
bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a
conditional instead.
QMP transactions that enable/disable bitmaps have extra error checking
surrounding them that prevent modifying bitmaps that are frozen.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++-
blockdev.c | 7 ++++
include/block/block.h | 10 +++++
qapi/block-core.json | 1 +
4 files changed, 121 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 7e71aba..e8f66a6 100644
--- a/block.c
+++ b/block.c
@@ -51,8 +51,17 @@
#include <windows.h>
#endif
+/**
+ * A BdrvDirtyBitmap can be in three possible states:
+ * (1) successor is false and disabled is false: full r/w mode
+ * (2) successor is false and disabled is true: read only mode ("disabled")
+ * (3) successor is set: frozen mode.
+ * A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
+ * or enabled. A frozen bitmap can only abdicate() or reclaim().
+ */
struct BdrvDirtyBitmap {
HBitmap *bitmap;
+ BdrvDirtyBitmap *successor;
char *name;
bool disabled;
QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -5421,6 +5430,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
g_free(bitmap->name);
bitmap->name = NULL;
}
@@ -5456,9 +5466,98 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return bitmap;
}
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+{
+ return bitmap->successor;
+}
+
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
{
- return !bitmap->disabled;
+ return !(bitmap->disabled || bitmap->successor);
+}
+
+/**
+ * Create a successor bitmap destined to replace this bitmap after an operation.
+ * Requires that the bitmap is not frozen and has no successor.
+ */
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, Error **errp)
+{
+ uint64_t granularity;
+ BdrvDirtyBitmap *child;
+
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp, "Cannot create a successor for a bitmap that is "
+ "currently frozen");
+ return -1;
+ }
+ assert(!bitmap->successor);
+
+ /* Create an anonymous successor */
+ granularity = bdrv_dirty_bitmap_granularity(bitmap);
+ child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+ if (!child) {
+ return -1;
+ }
+
+ /* Successor will be on or off based on our current state. */
+ child->disabled = bitmap->disabled;
+
+ /* Install the successor and freeze the parent */
+ bitmap->successor = child;
+ return 0;
+}
+
+/**
+ * For a bitmap with a successor, yield our name to the successor,
+ * Delete the old bitmap, and return a handle to the new bitmap.
+ */
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp)
+{
+ char *name;
+ BdrvDirtyBitmap *successor = bitmap->successor;
+
+ if (successor == NULL) {
+ error_setg(errp, "Cannot relinquish control if "
+ "there's no successor present");
+ return NULL;
+ }
+
+ name = bitmap->name;
+ bitmap->name = NULL;
+ successor->name = name;
+ bitmap->successor = NULL;
+ bdrv_release_dirty_bitmap(bs, bitmap);
+
+ return successor;
+}
+
+/**
+ * In cases of failure where we can no longer safely delete the parent,
+ * We may wish to re-join the parent and child/successor.
+ * The merged parent will be un-frozen, but not explicitly re-enabled.
+ */
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ Error **errp)
+{
+ BdrvDirtyBitmap *successor = parent->successor;
+
+ if (!successor) {
+ error_setg(errp, "Cannot reclaim a successor when none is present");
+ return NULL;
+ }
+
+ if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
+ error_setg(errp, "Merging of parent and successor bitmap failed");
+ return NULL;
+ }
+ bdrv_release_dirty_bitmap(bs, successor);
+ parent->successor = NULL;
+
+ return parent;
}
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
@@ -5466,6 +5565,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
BdrvDirtyBitmap *bm, *next;
QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
if (bm == bitmap) {
+ assert(!bdrv_dirty_bitmap_frozen(bm));
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
g_free(bitmap->name);
@@ -5477,11 +5577,13 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
bitmap->disabled = true;
}
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
bitmap->disabled = false;
}
diff --git a/blockdev.c b/blockdev.c
index c40c08c..77595e5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2036,9 +2036,16 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently frozen and cannot be removed",
+ name);
+ goto out;
+ }
bdrv_dirty_bitmap_make_anon(bs, bitmap);
bdrv_release_dirty_bitmap(bs, bitmap);
+ out:
aio_context_release(aio_context);
}
diff --git a/include/block/block.h b/include/block/block.h
index 879a3af..5919185 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -454,6 +454,15 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
uint32_t granularity,
const char *name,
Error **errp);
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
@@ -464,6 +473,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e2ad437..1bc8190 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1010,6 +1010,7 @@
# Returns: nothing on success
# If @node is not a valid block device or node, DeviceNotFound
# If @name is not found, GenericError with an explanation
+# if @name is frozen by an operation, GenericError
#
# Since 2.4
##
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 09/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (7 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 08/17] block: Add bitmap successors John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 10/17] qmp: add block-dirty-bitmap-clear John Snow
` (9 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 9 +++
block/backup.c | 149 +++++++++++++++++++++++++++++++++++++++-------
block/mirror.c | 4 ++
blockdev.c | 18 +++++-
hmp.c | 3 +-
include/block/block.h | 1 +
include/block/block_int.h | 2 +
qapi/block-core.json | 13 ++--
qmp-commands.hx | 7 ++-
9 files changed, 172 insertions(+), 34 deletions(-)
diff --git a/block.c b/block.c
index e8f66a6..3ecc2dd 100644
--- a/block.c
+++ b/block.c
@@ -5686,6 +5686,15 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
}
}
+/**
+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+ assert(hbi->hb);
+ hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
return hbitmap_count(bitmap->bitmap);
diff --git a/block/backup.c b/block/backup.c
index 1c535b1..d46c0a3 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,8 @@ typedef struct CowRequest {
typedef struct BackupBlockJob {
BlockJob common;
BlockDriverState *target;
+ /* bitmap for sync=dirty-bitmap */
+ BdrvDirtyBitmap *sync_bitmap;
MirrorSyncMode sync_mode;
RateLimit limit;
BlockdevOnError on_source_error;
@@ -242,6 +244,31 @@ static void backup_complete(BlockJob *job, void *opaque)
g_free(data);
}
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+{
+ if (block_job_is_cancelled(&job->common)) {
+ return true;
+ }
+
+ /* we need to yield so that qemu_aio_flush() returns.
+ * (without, VM does not reboot)
+ */
+ if (job->common.speed) {
+ uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+ job->sectors_read);
+ job->sectors_read = 0;
+ block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+ } else {
+ block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+ }
+
+ if (block_job_is_cancelled(&job->common)) {
+ return true;
+ }
+
+ return false;
+}
+
static void coroutine_fn backup_run(void *opaque)
{
BackupBlockJob *job = opaque;
@@ -254,13 +281,13 @@ static void coroutine_fn backup_run(void *opaque)
};
int64_t start, end;
int ret = 0;
+ bool error_is_read;
QLIST_INIT(&job->inflight_reqs);
qemu_co_rwlock_init(&job->flush_rwlock);
start = 0;
- end = DIV_ROUND_UP(job->common.len / BDRV_SECTOR_SIZE,
- BACKUP_SECTORS_PER_CLUSTER);
+ end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
job->bitmap = hbitmap_alloc(end, 0);
@@ -278,28 +305,62 @@ static void coroutine_fn backup_run(void *opaque)
qemu_coroutine_yield();
job->common.busy = true;
}
+ } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+ /* Dirty Bitmap sync has a slightly different iteration method */
+ HBitmapIter hbi;
+ int64_t sector;
+ int64_t cluster;
+ int64_t last_cluster = -1;
+ bool polyrhythmic;
+
+ bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+ /* Does the granularity happen to match our backup cluster size? */
+ polyrhythmic = (bdrv_dirty_bitmap_granularity(job->sync_bitmap) !=
+ BACKUP_CLUSTER_SIZE);
+
+ /* Find the next dirty /sector/ and copy that /cluster/ */
+ while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+ cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+ /* Fake progress updates for any clusters we skipped,
+ * excluding this current cluster. */
+ if (cluster != last_cluster + 1) {
+ job->common.offset += ((cluster - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+ }
+
+ if (yield_and_check(job)) {
+ goto leave;
+ }
+
+ do {
+ ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+ BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
+ if ((ret < 0) &&
+ backup_error_action(job, error_is_read, -ret) ==
+ BLOCK_ERROR_ACTION_REPORT) {
+ goto leave;
+ }
+ } while (ret < 0);
+
+ /* Advance (or rewind) our iterator if we need to. */
+ if (polyrhythmic) {
+ bdrv_set_dirty_iter(&hbi,
+ (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
+ }
+
+ last_cluster = cluster;
+ }
+
+ /* Play some final catchup with the progress meter */
+ if (last_cluster + 1 < end) {
+ job->common.offset += ((end - last_cluster - 1) *
+ BACKUP_CLUSTER_SIZE);
+ }
} else {
/* Both FULL and TOP SYNC_MODE's require copying.. */
for (; start < end; start++) {
- bool error_is_read;
-
- if (block_job_is_cancelled(&job->common)) {
- break;
- }
-
- /* we need to yield so that qemu_aio_flush() returns.
- * (without, VM does not reboot)
- */
- if (job->common.speed) {
- uint64_t delay_ns = ratelimit_calculate_delay(
- &job->limit, job->sectors_read);
- job->sectors_read = 0;
- block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
- } else {
- block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
- }
-
- if (block_job_is_cancelled(&job->common)) {
+ if (yield_and_check(job)) {
break;
}
@@ -351,12 +412,26 @@ static void coroutine_fn backup_run(void *opaque)
}
}
+leave:
notifier_with_return_remove(&before_write);
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&job->flush_rwlock);
qemu_co_rwlock_unlock(&job->flush_rwlock);
+ if (job->sync_bitmap) {
+ BdrvDirtyBitmap *bm;
+ if (ret < 0) {
+ /* Merge the successor back into the parent, delete nothing. */
+ bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
+ assert(bm);
+ bdrv_enable_dirty_bitmap(job->sync_bitmap);
+ } else {
+ /* Everything is fine, delete this bitmap and install the backup. */
+ bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
+ assert(bm);
+ }
+ }
hbitmap_free(job->bitmap);
bdrv_iostatus_disable(target);
@@ -369,6 +444,7 @@ static void coroutine_fn backup_run(void *opaque)
void backup_start(BlockDriverState *bs, BlockDriverState *target,
int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
@@ -412,17 +488,36 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
return;
}
+ if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+ if (!sync_bitmap) {
+ error_setg(errp, "must provide a valid bitmap name for "
+ "\"dirty-bitmap\" sync mode");
+ return;
+ }
+
+ /* Create a new bitmap, and freeze/disable this one. */
+ if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+ return;
+ }
+ } else if (sync_bitmap) {
+ error_setg(errp,
+ "a sync_bitmap was provided to backup_run, "
+ "but received an incompatible sync_mode (%s)",
+ MirrorSyncMode_lookup[sync_mode]);
+ return;
+ }
+
len = bdrv_getlength(bs);
if (len < 0) {
error_setg_errno(errp, -len, "unable to get length for '%s'",
bdrv_get_device_name(bs));
- return;
+ goto error;
}
BackupBlockJob *job = block_job_create(&backup_job_driver, bs, speed,
cb, opaque, errp);
if (!job) {
- return;
+ goto error;
}
bdrv_op_block_all(target, job->common.blocker);
@@ -431,7 +526,15 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
job->on_target_error = on_target_error;
job->target = target;
job->sync_mode = sync_mode;
+ job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
+ sync_bitmap : NULL;
job->common.len = len;
job->common.co = qemu_coroutine_create(backup_run);
qemu_coroutine_enter(job->common.co, job);
+ return;
+
+ error:
+ if (sync_bitmap) {
+ bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+ }
}
diff --git a/block/mirror.c b/block/mirror.c
index 1cb700e..f89eccf 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -718,6 +718,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
bool is_none_mode;
BlockDriverState *base;
+ if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+ error_setg(errp, "Sync mode 'dirty-bitmap' not supported");
+ return;
+ }
is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
mirror_start_job(bs, target, replaces,
diff --git a/blockdev.c b/blockdev.c
index 77595e5..b432736 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1558,6 +1558,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
backup->sync,
backup->has_mode, backup->mode,
backup->has_speed, backup->speed,
+ backup->has_bitmap, backup->bitmap,
backup->has_on_source_error, backup->on_source_error,
backup->has_on_target_error, backup->on_target_error,
&local_err);
@@ -2362,6 +2363,7 @@ void qmp_drive_backup(const char *device, const char *target,
enum MirrorSyncMode sync,
bool has_mode, enum NewImageMode mode,
bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
bool has_on_source_error, BlockdevOnError on_source_error,
bool has_on_target_error, BlockdevOnError on_target_error,
Error **errp)
@@ -2369,6 +2371,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;
@@ -2467,7 +2470,16 @@ void qmp_drive_backup(const char *device, const char *target,
bdrv_set_aio_context(target_bs, aio_context);
- backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+ if (has_bitmap) {
+ bmap = bdrv_find_dirty_bitmap(bs, bitmap);
+ if (!bmap) {
+ error_setg(errp, "Bitmap '%s' could not be found", bitmap);
+ goto out;
+ }
+ }
+
+ backup_start(bs, target_bs, speed, sync, bmap,
+ on_source_error, on_target_error,
block_job_cb, bs, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
@@ -2525,8 +2537,8 @@ void qmp_blockdev_backup(const char *device, const char *target,
bdrv_ref(target_bs);
bdrv_set_aio_context(target_bs, aio_context);
- backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
- block_job_cb, bs, &local_err);
+ backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
+ on_target_error, block_job_cb, bs, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
error_propagate(errp, local_err);
diff --git a/hmp.c b/hmp.c
index 735097c..72883b0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1028,7 +1028,8 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
qmp_drive_backup(device, filename, !!format, format,
full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
- true, mode, false, 0, false, 0, false, 0, &err);
+ true, mode, false, 0, false, NULL,
+ false, 0, false, 0, &err);
hmp_handle_error(mon, &err);
}
diff --git a/include/block/block.h b/include/block/block.h
index 5919185..e2d8331 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -481,6 +481,7 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_enable_copy_on_read(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index fb9e100..e0d5561 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -602,6 +602,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
* @target: Block device to write to.
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @sync_mode: What parts of the disk image should be copied to the destination.
+ * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_DIRTY_BITMAP.
* @on_source_error: The action to take upon error reading from the source.
* @on_target_error: The action to take upon error writing to the target.
* @cb: Completion function for the job.
@@ -612,6 +613,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
*/
void backup_start(BlockDriverState *bs, BlockDriverState *target,
int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1bc8190..f012de2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -511,10 +511,12 @@
#
# @none: only copy data written from now on
#
+# @dirty-bitmap: only copy data described by the dirty bitmap. Since: 2.4
+#
# Since: 1.3
##
{ 'enum': 'MirrorSyncMode',
- 'data': ['top', 'full', 'none'] }
+ 'data': ['top', 'full', 'none', 'dirty-bitmap'] }
##
# @BlockJobType:
@@ -689,14 +691,17 @@
# probe if @mode is 'existing', else the format of the source
#
# @sync: what parts of the disk image should be copied to the destination
-# (all the disk, only the sectors allocated in the topmost image, or
-# only new I/O).
+# (all the disk, only the sectors allocated in the topmost image, from a
+# dirty bitmap, or only new I/O).
#
# @mode: #optional whether and how QEMU should create a new image, default is
# 'absolute-paths'.
#
# @speed: #optional the maximum speed, in bytes per second
#
+# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
+# (Since 2.4)
+#
# @on-source-error: #optional the action to take on an error on the source,
# default 'report'. 'stop' and 'enospc' can only be used
# if the block device supports io-status (see BlockInfo).
@@ -714,7 +719,7 @@
{ 'type': 'DriveBackup',
'data': { 'device': 'str', 'target': 'str', '*format': 'str',
'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
- '*speed': 'int',
+ '*speed': 'int', '*bitmap': 'str',
'*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c5c0cb..a605d88 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1048,7 +1048,7 @@ EQMP
{
.name = "drive-backup",
.args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
- "on-source-error:s?,on-target-error:s?",
+ "bitmap:s?,on-source-error:s?,on-target-error:s?",
.mhandler.cmd_new = qmp_marshal_input_drive_backup,
},
@@ -1075,8 +1075,9 @@ Arguments:
(json-string, optional)
- "sync": what parts of the disk image should be copied to the destination;
possibilities include "full" for all the disk, "top" for only the sectors
- allocated in the topmost image, or "none" to only replicate new I/O
- (MirrorSyncMode).
+ allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in
+ the bitmap, or "none" to only replicate new I/O (MirrorSyncMode).
+- "bitmap": dirty bitmap name for sync==dirty-bitmap
- "mode": whether and how QEMU should create a new image
(NewImageMode, optional, default 'absolute-paths')
- "speed": the maximum speed, in bytes per second (json-int, optional)
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 10/17] qmp: add block-dirty-bitmap-clear
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (8 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 09/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 11/17] qmp: Add dirty bitmap status fields in query-block John Snow
` (8 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Add bdrv_clear_dirty_bitmap and a matching QMP command,
qmp_block_dirty_bitmap_clear that enables a user to reset
the bitmap attached to a drive.
This allows us to reset a bitmap in the event of a full
drive backup.
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 8 ++++++++
blockdev.c | 37 +++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 +
qapi/block-core.json | 14 ++++++++++++++
qmp-commands.hx | 29 +++++++++++++++++++++++++++++
5 files changed, 89 insertions(+)
diff --git a/block.c b/block.c
index 3ecc2dd..d969b24 100644
--- a/block.c
+++ b/block.c
@@ -62,6 +62,7 @@
struct BdrvDirtyBitmap {
HBitmap *bitmap;
BdrvDirtyBitmap *successor;
+ int64_t size;
char *name;
bool disabled;
QLIST_ENTRY(BdrvDirtyBitmap) list;
@@ -5460,6 +5461,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(sector_granularity) - 1);
+ bitmap->size = bitmap_size;
bitmap->name = g_strdup(name);
bitmap->disabled = false;
QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
@@ -5662,6 +5664,12 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+}
+
static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
int nr_sectors)
{
diff --git a/blockdev.c b/blockdev.c
index b432736..eaca97d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2050,6 +2050,43 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
aio_context_release(aio_context);
}
+/**
+ * Completely clear a bitmap, for the purposes of synchronizing a bitmap
+ * immediately after a full backup operation.
+ */
+void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
+ Error **errp)
+{
+ AioContext *aio_context;
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+
+ bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+ if (!bitmap) {
+ return;
+ }
+
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently frozen and cannot be modified",
+ name);
+ goto out;
+ } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently disabled and cannot be cleared",
+ name);
+ goto out;
+ }
+
+ bdrv_clear_dirty_bitmap(bitmap);
+
+ out:
+ aio_context_release(aio_context);
+}
+
int hmp_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 e2d8331..ca8e91a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -479,6 +479,7 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_iter_init(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f012de2..50970c4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1023,6 +1023,20 @@
'data': 'BlockDirtyBitmap' }
##
+# @block-dirty-bitmap-clear
+#
+# Clear (reset) a dirty bitmap on the device
+#
+# Returns: nothing on success
+# If @node is not a valid block device, DeviceNotFound
+# If @name is not found, GenericError with an explanation
+#
+# Since 2.4
+##
+{ 'command': 'block-dirty-bitmap-clear',
+ 'data': 'BlockDirtyBitmap' }
+
+##
# @block_set_io_throttle:
#
# Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a605d88..3389fff 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1301,6 +1301,35 @@ Example:
EQMP
{
+ .name = "block-dirty-bitmap-clear",
+ .args_type = "node:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_clear,
+ },
+
+SQMP
+
+block-dirty-bitmap-clear
+------------------------
+Since 2.4
+
+Reset the dirty bitmap associated with a node so that an incremental backup
+from this point in time forward will only backup clusters modified after this
+clear operation.
+
+Arguments:
+
+- "node": device/node on which to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-clear", "arguments": { "node": "drive0",
+ "name": "bitmap0" } }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "blockdev-snapshot-sync",
.args_type = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
.mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 11/17] qmp: Add dirty bitmap status fields in query-block
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (9 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 10/17] qmp: add block-dirty-bitmap-clear John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-11 16:54 ` Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 12/17] block: add BdrvDirtyBitmap documentation John Snow
` (7 subsequent siblings)
18 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Adds the "disabled" and "frozen" status booleans.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 2 ++
qapi/block-core.json | 7 ++++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index d969b24..06b4264 100644
--- a/block.c
+++ b/block.c
@@ -5602,6 +5602,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
+ info->disabled = bm->disabled;
+ info->frozen = bdrv_dirty_bitmap_frozen(bm);
entry->value = info;
*plist = entry;
plist = &entry->next;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 50970c4..7e4e14b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -335,10 +335,15 @@
#
# @granularity: granularity of the dirty bitmap in bytes (since 1.4)
#
+# @disabled: whether the dirty bitmap is disabled (Since 2.4)
+#
+# @frozen: whether the dirty bitmap is frozen (Since 2.4)
+#
# Since: 1.3
##
{ 'type': 'BlockDirtyInfo',
- 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} }
+ 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+ 'disabled': 'bool', 'frozen': 'bool'} }
##
# @BlockInfo:
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 12/17] block: add BdrvDirtyBitmap documentation
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (10 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 11/17] qmp: Add dirty bitmap status fields in query-block John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 13/17] block: Ensure consistent bitmap function prototypes John Snow
` (6 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 06b4264..55243f9 100644
--- a/block.c
+++ b/block.c
@@ -60,11 +60,11 @@
* or enabled. A frozen bitmap can only abdicate() or reclaim().
*/
struct BdrvDirtyBitmap {
- HBitmap *bitmap;
- BdrvDirtyBitmap *successor;
- int64_t size;
- char *name;
- bool disabled;
+ HBitmap *bitmap; /* Dirty sector bitmap implementation */
+ BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
+ char *name; /* Optional non-empty unique ID */
+ int64_t size; /* Size of the bitmap (Number of sectors) */
+ bool disabled; /* Bitmap is read-only */
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 13/17] block: Ensure consistent bitmap function prototypes
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (11 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 12/17] block: add BdrvDirtyBitmap documentation John Snow
@ 2015-03-02 23:19 ` John Snow
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate John Snow
` (5 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:19 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
We often don't need the BlockDriverState for functions
that operate on bitmaps. Remove it.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 13 ++++++-------
block/backup.c | 2 +-
block/mirror.c | 26 ++++++++++----------------
blockdev.c | 2 +-
include/block/block.h | 11 +++++------
migration/block.c | 7 +++----
6 files changed, 26 insertions(+), 35 deletions(-)
diff --git a/block.c b/block.c
index 55243f9..e6b2696 100644
--- a/block.c
+++ b/block.c
@@ -5429,7 +5429,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
return NULL;
}
-void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
{
assert(!bdrv_dirty_bitmap_frozen(bitmap));
g_free(bitmap->name);
@@ -5598,7 +5598,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
- info->count = bdrv_get_dirty_count(bs, bm);
+ info->count = bdrv_get_dirty_count(bm);
info->granularity = bdrv_dirty_bitmap_granularity(bm);
info->has_name = !!bm->name;
info->name = g_strdup(bm->name);
@@ -5646,20 +5646,19 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
}
-void bdrv_dirty_iter_init(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
+void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
{
hbitmap_iter_init(hbi, bitmap->bitmap, 0);
}
-void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
assert(bdrv_dirty_bitmap_enabled(bitmap));
hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
}
-void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors)
{
assert(bdrv_dirty_bitmap_enabled(bitmap));
@@ -5705,7 +5704,7 @@ void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
hbitmap_iter_init(hbi, hbi->hb, offset);
}
-int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
{
return hbitmap_count(bitmap->bitmap);
}
diff --git a/block/backup.c b/block/backup.c
index d46c0a3..41bd9af 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -313,7 +313,7 @@ static void coroutine_fn backup_run(void *opaque)
int64_t last_cluster = -1;
bool polyrhythmic;
- bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+ bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
/* Does the granularity happen to match our backup cluster size? */
polyrhythmic = (bdrv_dirty_bitmap_granularity(job->sync_bitmap) !=
BACKUP_CLUSTER_SIZE);
diff --git a/block/mirror.c b/block/mirror.c
index f89eccf..dcd6f65 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -125,11 +125,9 @@ static void mirror_write_complete(void *opaque, int ret)
MirrorOp *op = opaque;
MirrorBlockJob *s = op->s;
if (ret < 0) {
- BlockDriverState *source = s->common.bs;
BlockErrorAction action;
- bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
- op->nb_sectors);
+ bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, op->nb_sectors);
action = mirror_error_action(s, false, -ret);
if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
s->ret = ret;
@@ -143,11 +141,9 @@ static void mirror_read_complete(void *opaque, int ret)
MirrorOp *op = opaque;
MirrorBlockJob *s = op->s;
if (ret < 0) {
- BlockDriverState *source = s->common.bs;
BlockErrorAction action;
- bdrv_set_dirty_bitmap(source, s->dirty_bitmap, op->sector_num,
- op->nb_sectors);
+ bdrv_set_dirty_bitmap(s->dirty_bitmap, op->sector_num, op->nb_sectors);
action = mirror_error_action(s, true, -ret);
if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
s->ret = ret;
@@ -170,10 +166,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
s->sector_num = hbitmap_iter_next(&s->hbi);
if (s->sector_num < 0) {
- bdrv_dirty_iter_init(source, s->dirty_bitmap, &s->hbi);
+ bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
s->sector_num = hbitmap_iter_next(&s->hbi);
- trace_mirror_restart_iter(s,
- bdrv_get_dirty_count(source, s->dirty_bitmap));
+ trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
assert(s->sector_num >= 0);
}
@@ -288,8 +283,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
next_sector += sectors_per_chunk;
}
- bdrv_reset_dirty_bitmap(source, s->dirty_bitmap, sector_num,
- nb_sectors);
+ bdrv_reset_dirty_bitmap(s->dirty_bitmap, sector_num, nb_sectors);
/* Copy the dirty cluster. */
s->in_flight++;
@@ -446,7 +440,7 @@ static void coroutine_fn mirror_run(void *opaque)
assert(n > 0);
if (ret == 1) {
- bdrv_set_dirty_bitmap(bs, s->dirty_bitmap, sector_num, n);
+ bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
sector_num = next;
} else {
sector_num += n;
@@ -454,7 +448,7 @@ static void coroutine_fn mirror_run(void *opaque)
}
}
- bdrv_dirty_iter_init(bs, s->dirty_bitmap, &s->hbi);
+ bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
for (;;) {
uint64_t delay_ns = 0;
@@ -466,7 +460,7 @@ static void coroutine_fn mirror_run(void *opaque)
goto immediate_exit;
}
- cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
+ cnt = bdrv_get_dirty_count(s->dirty_bitmap);
/* s->common.offset contains the number of bytes already processed so
* far, cnt is the number of dirty sectors remaining and
* s->sectors_in_flight is the number of sectors currently being
@@ -516,7 +510,7 @@ static void coroutine_fn mirror_run(void *opaque)
should_complete = s->should_complete ||
block_job_is_cancelled(&s->common);
- cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
+ cnt = bdrv_get_dirty_count(s->dirty_bitmap);
}
}
@@ -531,7 +525,7 @@ static void coroutine_fn mirror_run(void *opaque)
*/
trace_mirror_before_drain(s, cnt);
bdrv_drain(bs);
- cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
+ cnt = bdrv_get_dirty_count(s->dirty_bitmap);
}
ret = 0;
diff --git a/blockdev.c b/blockdev.c
index eaca97d..e0671be 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2043,7 +2043,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
name);
goto out;
}
- bdrv_dirty_bitmap_make_anon(bs, bitmap);
+ bdrv_dirty_bitmap_make_anon(bitmap);
bdrv_release_dirty_bitmap(bs, bitmap);
out:
diff --git a/include/block/block.h b/include/block/block.h
index ca8e91a..a8c6369 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -465,7 +465,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
Error **errp);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
-void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
@@ -475,15 +475,14 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
-void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
-void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
-void bdrv_dirty_iter_init(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
-int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
void bdrv_enable_copy_on_read(BlockDriverState *bs);
void bdrv_disable_copy_on_read(BlockDriverState *bs);
diff --git a/migration/block.c b/migration/block.c
index 5f9b3e5..1a9e7cd 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -303,7 +303,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
nr_sectors, blk_mig_read_cb, blk);
- bdrv_reset_dirty_bitmap(bs, bmds->dirty_bitmap, cur_sector, nr_sectors);
+ bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
qemu_mutex_unlock_iothread();
bmds->cur_sector = cur_sector + nr_sectors;
@@ -496,8 +496,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
g_free(blk);
}
- bdrv_reset_dirty_bitmap(bmds->bs, bmds->dirty_bitmap, sector,
- nr_sectors);
+ bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, sector, nr_sectors);
break;
}
sector += BDRV_SECTORS_PER_DIRTY_CHUNK;
@@ -583,7 +582,7 @@ static int64_t get_remaining_dirty(void)
int64_t dirty = 0;
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
- dirty += bdrv_get_dirty_count(bmds->bs, bmds->dirty_bitmap);
+ dirty += bdrv_get_dirty_count(bmds->dirty_bitmap);
}
return dirty << BDRV_SECTOR_BITS;
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (12 preceding siblings ...)
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 13/17] block: Ensure consistent bitmap function prototypes John Snow
@ 2015-03-02 23:20 ` John Snow
2015-03-03 15:29 ` Max Reitz
` (2 more replies)
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 15/17] iotests: add invalid input incremental backup tests John Snow
` (4 subsequent siblings)
18 siblings, 3 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:20 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 22 ++++++++++++++++++++
include/block/block.h | 1 +
include/qemu/hbitmap.h | 10 +++++++++
util/hbitmap.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 88 insertions(+)
diff --git a/block.c b/block.c
index e6b2696..5eaa874 100644
--- a/block.c
+++ b/block.c
@@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
ret = drv->bdrv_truncate(bs, offset);
if (ret == 0) {
ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
+ bdrv_dirty_bitmap_truncate(bs);
if (bs->blk) {
blk_dev_resize_cb(bs->blk);
}
@@ -5562,6 +5563,27 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
return parent;
}
+static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size)
+{
+ /* Should only be frozen during a block backup job, which should have
+ * blocked any resize actions. */
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
+ hbitmap_truncate(bitmap->bitmap, size);
+}
+
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+{
+ BdrvDirtyBitmap *bitmap;
+ uint64_t size = bdrv_nb_sectors(bs);
+
+ QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ continue;
+ }
+ dirty_bitmap_truncate(bitmap, size);
+ }
+}
+
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
BdrvDirtyBitmap *bm, *next;
diff --git a/include/block/block.h b/include/block/block.h
index a8c6369..3a85690 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -454,6 +454,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
uint32_t granularity,
const char *name,
Error **errp);
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index c19c1cb..a75157e 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,16 @@ struct HBitmapIter {
HBitmap *hbitmap_alloc(uint64_t size, int granularity);
/**
+ * hbitmap_truncate:
+ * @hb: The bitmap to change the size of.
+ * @size: The number of elements to change the bitmap to accommodate.
+ *
+ * truncate or grow an existing bitmap to accommodate a new number of elements.
+ * This may invalidate existing HBitmapIterators.
+ */
+void hbitmap_truncate(HBitmap *hb, uint64_t size);
+
+/**
* hbitmap_merge:
* @a: The bitmap to store the result in.
* @b: The bitmap to merge into @a.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 962ff29..dd8c2f3 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -90,6 +90,9 @@ struct HBitmap {
* bitmap will still allocate HBITMAP_LEVELS arrays.
*/
unsigned long *levels[HBITMAP_LEVELS];
+
+ /* The length of each levels[] array. */
+ uint64_t sizes[HBITMAP_LEVELS];
};
/* Advance hbi to the next nonzero word and return it. hbi->pos
@@ -384,6 +387,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
hb->granularity = granularity;
for (i = HBITMAP_LEVELS; i-- > 0; ) {
size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+ hb->sizes[i] = size;
hb->levels[i] = g_new0(unsigned long, size);
}
@@ -396,6 +400,57 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
return hb;
}
+void hbitmap_truncate(HBitmap *hb, uint64_t size)
+{
+ bool truncate;
+ unsigned i;
+ uint64_t num_elements = size;
+ uint64_t old;
+
+ /* Size comes in as logical elements, adjust for granularity. */
+ size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
+ assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
+ truncate = size < hb->size;
+
+ if (size == hb->size) {
+ /* A hard day's work */
+ return;
+ }
+
+ hb->size = size;
+ for (i = HBITMAP_LEVELS; i-- > 0; ) {
+ size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+ if (hb->sizes[i] == size) {
+ continue;
+ }
+ old = hb->sizes[i];
+ hb->sizes[i] = size;
+ hb->levels[i] = g_realloc_n(hb->levels[i], size, sizeof(unsigned long));
+ if (!truncate) {
+ memset(&hb->levels[i][old], 0x00,
+ (size - old) * sizeof(*hb->levels[i]));
+ }
+ }
+ assert(size == 1);
+
+ /* Clear out any "extra space" we may have that the user didn't request:
+ * It may have garbage data in it, now. */
+ if (truncate) {
+ /* Due to granularity fuzziness, we may accidentally reset some of
+ * the last bits that are actually valid. So, record the current value,
+ * reset the "dead range," then re-set the one element we care about. */
+ uint64_t fix_count = (hb->size << hb->granularity) - num_elements;
+ if (fix_count) {
+ bool set = hbitmap_get(hb, num_elements - 1);
+ hbitmap_reset(hb, num_elements, fix_count);
+ if (set) {
+ hbitmap_set(hb, num_elements - 1, 1);
+ }
+ }
+ }
+}
+
+
/**
* Given HBitmaps A and B, let A := A (BITOR) B.
* Bitmap B will not be modified.
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 15/17] iotests: add invalid input incremental backup tests
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (13 preceding siblings ...)
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate John Snow
@ 2015-03-02 23:20 ` John Snow
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 16/17] iotests: add simple incremental backup case John Snow
` (3 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:20 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/124 | 89 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/124.out | 5 +++
tests/qemu-iotests/group | 1 +
3 files changed, 95 insertions(+)
create mode 100644 tests/qemu-iotests/124
create mode 100644 tests/qemu-iotests/124.out
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
new file mode 100644
index 0000000..7985cd1
--- /dev/null
+++ b/tests/qemu-iotests/124
@@ -0,0 +1,89 @@
+#!/usr/bin/env python
+#
+# Tests for incremental drive-backup
+#
+# Copyright (C) 2015 John Snow for Red Hat, Inc.
+#
+# Based on 056.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+
+
+def io_write_patterns(img, patterns):
+ for pattern in patterns:
+ iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
+
+
+class TestIncrementalBackup(iotests.QMPTestCase):
+ def setUp(self):
+ self.bitmaps = list()
+ self.files = list()
+ self.vm = iotests.VM()
+ self.test_img = os.path.join(iotests.test_dir, 'base.img')
+ self.full_bak = os.path.join(iotests.test_dir, 'backup.img')
+ self.foo_img = os.path.join(iotests.test_dir, 'foo.bar')
+ self.img_create(self.test_img, iotests.imgfmt)
+ self.vm.add_drive(self.test_img)
+ # Create a base image with a distinctive patterning
+ io_write_patterns(self.test_img, (('0x41', 0, 512),
+ ('0xd5', '1M', '32k'),
+ ('0xdc', '32M', '124k')))
+ self.vm.launch()
+
+
+ def img_create(self, img, fmt=iotests.imgfmt, size='64M',
+ parent=None, parentFormat=None):
+ plist = list()
+ if parent:
+ if parentFormat is None:
+ parentFormat = fmt
+ iotests.qemu_img('create', '-f', fmt, img, size,
+ '-b', parent, '-F', parentFormat)
+ else:
+ iotests.qemu_img('create', '-f', fmt, img, size)
+ self.files.append(img)
+
+ def test_sync_dirty_bitmap_missing(self):
+ self.assert_no_active_block_jobs()
+ self.files.append(self.foo_img)
+ result = self.vm.qmp('drive-backup', device='drive0',
+ sync='dirty-bitmap', format=iotests.imgfmt,
+ target=self.foo_img)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+
+ def test_sync_dirty_bitmap_not_found(self):
+ self.assert_no_active_block_jobs()
+ self.files.append(self.foo_img)
+ result = self.vm.qmp('drive-backup', device='drive0',
+ sync='dirty-bitmap', bitmap='unknown',
+ format=iotests.imgfmt, target=self.foo_img)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for filename in self.files:
+ try:
+ os.remove(filename)
+ except OSError:
+ pass
+
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
new file mode 100644
index 0000000..fbc63e6
--- /dev/null
+++ b/tests/qemu-iotests/124.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 93a0250..df79c1a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -121,3 +121,4 @@
114 rw auto quick
116 rw auto quick
123 rw auto quick
+124 rw auto backing
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 16/17] iotests: add simple incremental backup case
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (14 preceding siblings ...)
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 15/17] iotests: add invalid input incremental backup tests John Snow
@ 2015-03-02 23:20 ` John Snow
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 17/17] iotests: add incremental backup failure recovery test John Snow
` (2 subsequent siblings)
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:20 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/124 | 122 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/124.out | 4 +-
2 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 7985cd1..da8f0e0 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -28,6 +28,36 @@ def io_write_patterns(img, patterns):
for pattern in patterns:
iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
+class Bitmap:
+ def __init__(self, name, node):
+ self.name = name
+ self.node = node
+ self.pattern = os.path.join(iotests.test_dir.replace('%', '%%'),
+ '%s.backup.%%i.img' % name)
+ self.num = 0
+ self.backups = list()
+
+ def new_target(self, num=None):
+ if num is None:
+ num = self.num
+ self.num = num + 1
+ target = self.pattern % num
+ self.backups.append(target)
+ return target
+
+ def last_target(self):
+ return self.backups[-1]
+
+ def del_target(self):
+ os.remove(self.backups.pop())
+ self.num -= 1
+
+ def __del__(self):
+ for backup in self.backups:
+ try:
+ os.remove(backup)
+ except OSError:
+ pass
class TestIncrementalBackup(iotests.QMPTestCase):
def setUp(self):
@@ -58,6 +88,98 @@ class TestIncrementalBackup(iotests.QMPTestCase):
iotests.qemu_img('create', '-f', fmt, img, size)
self.files.append(img)
+
+ def create_full_backup(self, drive='drive0'):
+ res = self.vm.qmp('drive-backup', device=drive,
+ sync='full', format=iotests.imgfmt,
+ target=self.full_bak)
+ self.assert_qmp(res, 'return', {})
+ self.wait_until_completed(drive)
+ self.check_full_backup()
+ self.files.append(self.full_bak)
+
+
+ def check_full_backup(self):
+ self.assertTrue(iotests.compare_images(self.test_img, self.full_bak))
+
+
+ def add_bitmap(self, name, node='drive0'):
+ bitmap = Bitmap(name, node)
+ self.bitmaps.append(bitmap)
+ result = self.vm.qmp('block-dirty-bitmap-add', node=bitmap.node,
+ name=bitmap.name)
+ self.assert_qmp(result, 'return', {})
+ return bitmap
+
+
+ def create_incremental(self, bitmap=None, num=None,
+ parent=None, parentFormat=None, validate=True):
+ if bitmap is None:
+ bitmap = self.bitmaps[-1]
+
+ # If this is the first incremental backup for a bitmap,
+ # use the full backup as a backing image. Otherwise, use
+ # the last incremental backup.
+ if parent is None:
+ if bitmap.num == 0:
+ parent = self.full_bak
+ else:
+ parent = bitmap.last_target()
+
+ target = bitmap.new_target(num)
+ self.img_create(target, iotests.imgfmt, parent=parent)
+
+ result = self.vm.qmp('drive-backup', device=bitmap.node,
+ sync='dirty-bitmap', bitmap=bitmap.name,
+ format=iotests.imgfmt, target=target,
+ mode='existing')
+ self.assert_qmp(result, 'return', {})
+
+ event = self.wait_until_completed(bitmap.node, check_offset=validate)
+ if validate:
+ return self.check_incremental(target)
+
+
+ def check_incremental(self, target=None):
+ if target is None:
+ target = self.bitmaps[-1].last_target()
+ self.assertTrue(iotests.compare_images(self.test_img, target))
+ return True
+
+
+ def hmp_io_writes(self, drive, patterns):
+ for pattern in patterns:
+ self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern)
+ self.vm.hmp_qemu_io(drive, 'flush')
+
+
+ def test_incremental_simple(self):
+ '''
+ Test: Create and verify three incremental backups.
+
+ Create a bitmap and a full backup before VM execution begins,
+ then create a series of three incremental backups "during execution,"
+ i.e.; after IO requests begin modifying the drive.
+ '''
+ self.create_full_backup()
+ self.add_bitmap('bitmap0', 'drive0')
+
+ # Sanity: Create a "hollow" incremental backup
+ self.create_incremental()
+ # Three writes: One complete overwrite, one new segment,
+ # and one partial overlap.
+ self.hmp_io_writes('drive0', (('0xab', 0, 512),
+ ('0xfe', '16M', '256k'),
+ ('0x64', '32736k', '64k')))
+ self.create_incremental()
+ # Three more writes, one of each kind, like above
+ self.hmp_io_writes('drive0', (('0x9a', 0, 512),
+ ('0x55', '8M', '352k'),
+ ('0x78', '15872k', '1M')))
+ self.create_incremental()
+ return True
+
+
def test_sync_dirty_bitmap_missing(self):
self.assert_no_active_block_jobs()
self.files.append(self.foo_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index fbc63e6..8d7e996 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-..
+...
----------------------------------------------------------------------
-Ran 2 tests
+Ran 3 tests
OK
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH v2 17/17] iotests: add incremental backup failure recovery test
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (15 preceding siblings ...)
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 16/17] iotests: add simple incremental backup case John Snow
@ 2015-03-02 23:20 ` John Snow
2015-03-03 15:26 ` [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series Max Reitz
2015-03-11 16:57 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
18 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-02 23:20 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, armbru, vsementsov, stefanha, mreitz
To test the failure case, we modify iotests.py to allow us to specify
that we'd like to allow failures when we wait for block job events.
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/124 | 57 ++++++++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/124.out | 4 +--
tests/qemu-iotests/iotests.py | 6 +++--
3 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index da8f0e0..1c07387 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -135,7 +135,11 @@ class TestIncrementalBackup(iotests.QMPTestCase):
mode='existing')
self.assert_qmp(result, 'return', {})
- event = self.wait_until_completed(bitmap.node, check_offset=validate)
+ event = self.wait_until_completed(bitmap.node, check_offset=validate,
+ allow_failures=(not validate))
+ if 'error' in event['data']:
+ bitmap.del_target()
+ return False
if validate:
return self.check_incremental(target)
@@ -180,6 +184,57 @@ class TestIncrementalBackup(iotests.QMPTestCase):
return True
+ def test_incremental_failure(self):
+ '''Test: Verify backups made after a failure are correct.
+
+ Simulate a failure during an incremental backup block job,
+ emulate additional writes, then create another incremental backup
+ afterwards and verify that the backup created is correct.
+ '''
+
+ # Create a blkdebug interface to this img as 'drive1'
+ result = self.vm.qmp('blockdev-add', options={
+ 'id': 'drive1',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'blkdebug',
+ 'image': {
+ 'driver': 'file',
+ 'filename': self.test_img
+ },
+ 'set-state': [{
+ 'event': 'flush_to_disk',
+ 'state': 1,
+ 'new_state': 2
+ }],
+ 'inject-error': [{
+ 'event': 'read_aio',
+ 'errno': 5,
+ 'state': 2,
+ 'immediately': False,
+ 'once': True
+ }],
+ }
+ })
+ self.assert_qmp(result, 'return', {})
+
+ self.create_full_backup()
+ self.add_bitmap('bitmap0', 'drive1')
+ # Note: at this point, during a normal execution,
+ # Assume that the VM resumes and begins issuing IO requests here.
+
+ self.hmp_io_writes('drive1', (('0xab', 0, 512),
+ ('0xfe', '16M', '256k'),
+ ('0x64', '32736k', '64k')))
+
+ result = self.create_incremental(validate=False)
+ self.assertFalse(result)
+ self.hmp_io_writes('drive1', (('0x9a', 0, 512),
+ ('0x55', '8M', '352k'),
+ ('0x78', '15872k', '1M')))
+ self.create_incremental()
+
+
def test_sync_dirty_bitmap_missing(self):
self.assert_no_active_block_jobs()
self.files.append(self.foo_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 8d7e996..89968f3 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-...
+....
----------------------------------------------------------------------
-Ran 3 tests
+Ran 4 tests
OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1402854..0522501 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -273,14 +273,16 @@ class QMPTestCase(unittest.TestCase):
self.assert_no_active_block_jobs()
return result
- def wait_until_completed(self, drive='drive0', check_offset=True):
+ def wait_until_completed(self, drive='drive0', check_offset=True,
+ allow_failures=False):
'''Wait for a block job to finish, returning the event'''
completed = False
while not completed:
for event in self.vm.get_qmp_events(wait=True):
if event['event'] == 'BLOCK_JOB_COMPLETED':
self.assert_qmp(event, 'data/device', drive)
- self.assert_qmp_absent(event, 'data/error')
+ if not allow_failures:
+ self.assert_qmp_absent(event, 'data/error')
if check_offset:
self.assert_qmp(event, 'data/offset', event['data']['len'])
completed = True
--
1.9.3
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (16 preceding siblings ...)
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 17/17] iotests: add incremental backup failure recovery test John Snow
@ 2015-03-03 15:26 ` Max Reitz
2015-03-11 16:57 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
18 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-03-03 15:26 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 2015-03-02 at 18:19, John Snow wrote:
> New topic, new version, same great incremental backup taste.
>
> This series relies on [Qemu-devel] [PULL 30/69] blkdebug: fix "once" rule,
> part of Stefan's 69 patch pull request submitted 2015-02-27.
>
> This patchset enables the in-memory part of the incremental backup
> feature, without transactional support.
>
> Support for transactions will come at a later date, but getting this
> portion upstream will help stabilize work on bitmap persistence and
> bitmap migration.
>
> Transactional support is being postponed to allow more development
> to create a transactional callback system that will allow objects used
> by the drive backup routines to perform cleanup actions conditionally
> based on the outcome of all backups in the transaction group.
>
> This series was once said to have been written by a man known only
> to Qemu-Devel as "Fam Zheng", A programming maverick hellbent on never
> again wasting space by making full backups when he did not have to. The
> true origins of this series are now lost to the sands of time.
>
> (12+ revisions later: Thanks, Fam!)
>
> ===
> v2:
> ===
>
> 01: Added a new opening blurb.
> Adjusted codeblock indentations to be 4 spaces instead of 3,
That was easy...
Max
> so it works as MD or GFMD.
> Adjusted errors explanation.
> Make visual separations between json data and shell commands
> Eliminate any ligering single quotes
>
> 07: Remember that memset takes bytes, not n_items ...
>
> ===
> v1:
> ===
>
> Deletions:
> - Removed Transactions, to be added later.
> - Removed Transaction tests, as above.
>
> Changes:
> 01: Indentation fixes.
> Removed enable/disable documentation.
> Added a note that transactions aren't implemented yet.
> Removed my needless commas
> Added error case documentation.
>
> 07: QMP enable/disable commands are deleted.
>
> 14: Some comments concerning assertions.
> Scrub re-alloc memory if we expand the array.
> Do not attempt to scrub memory if fix_count is 0
>
> Changes made with Reviews kept:
>
> 02: Since 2.4
> 04: Since 2.4
> Demingled the QMP command documentation.
> 08: Additions to what was qmp_block_dirty_enable/disable
> are no longer present as those function no longer exist.
> 09: Since 2.4
> 10: Since 2.4
> Demingled QMP command documentation.
> 11: Since 2.4
> 15: Test 112 --> 124
> 17: Number of tests altered. (Only 4, now.)
>
> Fam Zheng (1):
> qapi: Add optional field "name" to block dirty bitmap
>
> John Snow (16):
> docs: incremental backup documentation
> qmp: Ensure consistent granularity type
> qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
> block: Introduce bdrv_dirty_bitmap_granularity()
> hbitmap: add hbitmap_merge
> block: Add bitmap disabled status
> block: Add bitmap successors
> qmp: Add support of "dirty-bitmap" sync mode for drive-backup
> qmp: add block-dirty-bitmap-clear
> qmp: Add dirty bitmap status fields in query-block
> block: add BdrvDirtyBitmap documentation
> block: Ensure consistent bitmap function prototypes
> block: Resize bitmaps on bdrv_truncate
> iotests: add invalid input incremental backup tests
> iotests: add simple incremental backup case
> iotests: add incremental backup failure recovery test
>
> block.c | 248 +++++++++++++++++++++++++++++++--
> block/backup.c | 149 ++++++++++++++++----
> block/mirror.c | 46 +++----
> blockdev.c | 162 +++++++++++++++++++++-
> docs/bitmaps.md | 311 ++++++++++++++++++++++++++++++++++++++++++
> hmp.c | 3 +-
> include/block/block.h | 34 ++++-
> include/block/block_int.h | 4 +-
> include/qemu/hbitmap.h | 21 +++
> migration/block.c | 9 +-
> qapi/block-core.json | 92 ++++++++++++-
> qmp-commands.hx | 92 ++++++++++++-
> tests/qemu-iotests/124 | 266 ++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/124.out | 5 +
> tests/qemu-iotests/group | 1 +
> tests/qemu-iotests/iotests.py | 6 +-
> util/hbitmap.c | 87 ++++++++++++
> 17 files changed, 1446 insertions(+), 90 deletions(-)
> create mode 100644 docs/bitmaps.md
> create mode 100644 tests/qemu-iotests/124
> create mode 100644 tests/qemu-iotests/124.out
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation John Snow
@ 2015-03-03 15:28 ` Max Reitz
2015-03-11 13:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
1 sibling, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-03-03 15:28 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 2015-03-02 at 18:19, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> docs/bitmaps.md | 311 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 311 insertions(+)
> create mode 100644 docs/bitmaps.md
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate John Snow
@ 2015-03-03 15:29 ` Max Reitz
2015-03-03 16:02 ` Max Reitz
2015-03-11 16:18 ` Stefan Hajnoczi
2 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-03-03 15:29 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 2015-03-02 at 18:20, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 22 ++++++++++++++++++++
> include/block/block.h | 1 +
> include/qemu/hbitmap.h | 10 +++++++++
> util/hbitmap.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 88 insertions(+)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate John Snow
2015-03-03 15:29 ` Max Reitz
@ 2015-03-03 16:02 ` Max Reitz
2015-03-03 21:24 ` John Snow
2015-03-11 16:18 ` Stefan Hajnoczi
2 siblings, 1 reply; 41+ messages in thread
From: Max Reitz @ 2015-03-03 16:02 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 2015-03-02 at 18:20, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 22 ++++++++++++++++++++
> include/block/block.h | 1 +
> include/qemu/hbitmap.h | 10 +++++++++
> util/hbitmap.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 88 insertions(+)
>
> diff --git a/block.c b/block.c
> index e6b2696..5eaa874 100644
> --- a/block.c
> +++ b/block.c
> @@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
> ret = drv->bdrv_truncate(bs, offset);
> if (ret == 0) {
> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> + bdrv_dirty_bitmap_truncate(bs);
> if (bs->blk) {
> blk_dev_resize_cb(bs->blk);
> }
> @@ -5562,6 +5563,27 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> return parent;
> }
>
> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size)
> +{
> + /* Should only be frozen during a block backup job, which should have
> + * blocked any resize actions. */
> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
> + hbitmap_truncate(bitmap->bitmap, size);
> +}
> +
> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> +{
> + BdrvDirtyBitmap *bitmap;
> + uint64_t size = bdrv_nb_sectors(bs);
> +
> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
> + continue;
> + }
Hm!
The assert() above doesn't do anything because this condition will just
skip over frozen bitmaps. Maybe we should drop it?
I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will always
be false anyway (hence the assertion), but it would stand with this
block dropped, too.
Max
> + dirty_bitmap_truncate(bitmap, size);
> + }
> +}
> +
> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> {
> BdrvDirtyBitmap *bm, *next;
> diff --git a/include/block/block.h b/include/block/block.h
> index a8c6369..3a85690 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -454,6 +454,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> uint32_t granularity,
> const char *name,
> Error **errp);
> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index c19c1cb..a75157e 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -65,6 +65,16 @@ struct HBitmapIter {
> HBitmap *hbitmap_alloc(uint64_t size, int granularity);
>
> /**
> + * hbitmap_truncate:
> + * @hb: The bitmap to change the size of.
> + * @size: The number of elements to change the bitmap to accommodate.
> + *
> + * truncate or grow an existing bitmap to accommodate a new number of elements.
> + * This may invalidate existing HBitmapIterators.
> + */
> +void hbitmap_truncate(HBitmap *hb, uint64_t size);
> +
> +/**
> * hbitmap_merge:
> * @a: The bitmap to store the result in.
> * @b: The bitmap to merge into @a.
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 962ff29..dd8c2f3 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -90,6 +90,9 @@ struct HBitmap {
> * bitmap will still allocate HBITMAP_LEVELS arrays.
> */
> unsigned long *levels[HBITMAP_LEVELS];
> +
> + /* The length of each levels[] array. */
> + uint64_t sizes[HBITMAP_LEVELS];
> };
>
> /* Advance hbi to the next nonzero word and return it. hbi->pos
> @@ -384,6 +387,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
> hb->granularity = granularity;
> for (i = HBITMAP_LEVELS; i-- > 0; ) {
> size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + hb->sizes[i] = size;
> hb->levels[i] = g_new0(unsigned long, size);
> }
>
> @@ -396,6 +400,57 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
> return hb;
> }
>
> +void hbitmap_truncate(HBitmap *hb, uint64_t size)
> +{
> + bool truncate;
> + unsigned i;
> + uint64_t num_elements = size;
> + uint64_t old;
> +
> + /* Size comes in as logical elements, adjust for granularity. */
> + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
> + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
> + truncate = size < hb->size;
> +
> + if (size == hb->size) {
> + /* A hard day's work */
> + return;
> + }
> +
> + hb->size = size;
> + for (i = HBITMAP_LEVELS; i-- > 0; ) {
> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + if (hb->sizes[i] == size) {
> + continue;
> + }
> + old = hb->sizes[i];
> + hb->sizes[i] = size;
> + hb->levels[i] = g_realloc_n(hb->levels[i], size, sizeof(unsigned long));
> + if (!truncate) {
> + memset(&hb->levels[i][old], 0x00,
> + (size - old) * sizeof(*hb->levels[i]));
> + }
> + }
> + assert(size == 1);
> +
> + /* Clear out any "extra space" we may have that the user didn't request:
> + * It may have garbage data in it, now. */
> + if (truncate) {
> + /* Due to granularity fuzziness, we may accidentally reset some of
> + * the last bits that are actually valid. So, record the current value,
> + * reset the "dead range," then re-set the one element we care about. */
> + uint64_t fix_count = (hb->size << hb->granularity) - num_elements;
> + if (fix_count) {
> + bool set = hbitmap_get(hb, num_elements - 1);
> + hbitmap_reset(hb, num_elements, fix_count);
> + if (set) {
> + hbitmap_set(hb, num_elements - 1, 1);
> + }
> + }
> + }
> +}
> +
> +
> /**
> * Given HBitmaps A and B, let A := A (BITOR) B.
> * Bitmap B will not be modified.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-03 16:02 ` Max Reitz
@ 2015-03-03 21:24 ` John Snow
2015-03-03 21:27 ` Max Reitz
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-03 21:24 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 03/03/2015 11:02 AM, Max Reitz wrote:
> On 2015-03-02 at 18:20, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block.c | 22 ++++++++++++++++++++
>> include/block/block.h | 1 +
>> include/qemu/hbitmap.h | 10 +++++++++
>> util/hbitmap.c | 55
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 88 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index e6b2696..5eaa874 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
>> offset)
>> ret = drv->bdrv_truncate(bs, offset);
>> if (ret == 0) {
>> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>> + bdrv_dirty_bitmap_truncate(bs);
>> if (bs->blk) {
>> blk_dev_resize_cb(bs->blk);
>> }
>> @@ -5562,6 +5563,27 @@ BdrvDirtyBitmap
>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> return parent;
>> }
>> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
>> size)
>> +{
>> + /* Should only be frozen during a block backup job, which should
>> have
>> + * blocked any resize actions. */
>> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> + hbitmap_truncate(bitmap->bitmap, size);
>> +}
>> +
>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>> +{
>> + BdrvDirtyBitmap *bitmap;
>> + uint64_t size = bdrv_nb_sectors(bs);
>> +
>> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> + continue;
>> + }
>
> Hm!
>
> The assert() above doesn't do anything because this condition will just
> skip over frozen bitmaps. Maybe we should drop it?
>
> I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will always
> be false anyway (hence the assertion), but it would stand with this
> block dropped, too.
>
> Max
>
It's there for parity with the other bdrv helpers that perform a similar
check. I am guarding against, in the future, any potential users from
deciding to call this function directly for whichever reason.
Requesting an action on a drive that contains bitmaps that are not ready
for the action is OK. Requesting an action on a bitmap that is not ready
for the action should never happen, hence the 'documentation assert.'
My default action will be to leave it in.
--js
>> + dirty_bitmap_truncate(bitmap, size);
>> + }
>> +}
>> +
>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
>> *bitmap)
>> {
>> BdrvDirtyBitmap *bm, *next;
>> diff --git a/include/block/block.h b/include/block/block.h
>> index a8c6369..3a85690 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -454,6 +454,7 @@ BdrvDirtyBitmap
>> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>> uint32_t granularity,
>> const char *name,
>> Error **errp);
>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap,
>> Error **errp);
>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>> index c19c1cb..a75157e 100644
>> --- a/include/qemu/hbitmap.h
>> +++ b/include/qemu/hbitmap.h
>> @@ -65,6 +65,16 @@ struct HBitmapIter {
>> HBitmap *hbitmap_alloc(uint64_t size, int granularity);
>> /**
>> + * hbitmap_truncate:
>> + * @hb: The bitmap to change the size of.
>> + * @size: The number of elements to change the bitmap to accommodate.
>> + *
>> + * truncate or grow an existing bitmap to accommodate a new number of
>> elements.
>> + * This may invalidate existing HBitmapIterators.
>> + */
>> +void hbitmap_truncate(HBitmap *hb, uint64_t size);
>> +
>> +/**
>> * hbitmap_merge:
>> * @a: The bitmap to store the result in.
>> * @b: The bitmap to merge into @a.
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 962ff29..dd8c2f3 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -90,6 +90,9 @@ struct HBitmap {
>> * bitmap will still allocate HBITMAP_LEVELS arrays.
>> */
>> unsigned long *levels[HBITMAP_LEVELS];
>> +
>> + /* The length of each levels[] array. */
>> + uint64_t sizes[HBITMAP_LEVELS];
>> };
>> /* Advance hbi to the next nonzero word and return it. hbi->pos
>> @@ -384,6 +387,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int
>> granularity)
>> hb->granularity = granularity;
>> for (i = HBITMAP_LEVELS; i-- > 0; ) {
>> size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>> + hb->sizes[i] = size;
>> hb->levels[i] = g_new0(unsigned long, size);
>> }
>> @@ -396,6 +400,57 @@ HBitmap *hbitmap_alloc(uint64_t size, int
>> granularity)
>> return hb;
>> }
>> +void hbitmap_truncate(HBitmap *hb, uint64_t size)
>> +{
>> + bool truncate;
>> + unsigned i;
>> + uint64_t num_elements = size;
>> + uint64_t old;
>> +
>> + /* Size comes in as logical elements, adjust for granularity. */
>> + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
>> + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
>> + truncate = size < hb->size;
>> +
>> + if (size == hb->size) {
>> + /* A hard day's work */
>> + return;
>> + }
>> +
>> + hb->size = size;
>> + for (i = HBITMAP_LEVELS; i-- > 0; ) {
>> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>> + if (hb->sizes[i] == size) {
>> + continue;
>> + }
>> + old = hb->sizes[i];
>> + hb->sizes[i] = size;
>> + hb->levels[i] = g_realloc_n(hb->levels[i], size,
>> sizeof(unsigned long));
>> + if (!truncate) {
>> + memset(&hb->levels[i][old], 0x00,
>> + (size - old) * sizeof(*hb->levels[i]));
>> + }
>> + }
>> + assert(size == 1);
>> +
>> + /* Clear out any "extra space" we may have that the user didn't
>> request:
>> + * It may have garbage data in it, now. */
>> + if (truncate) {
>> + /* Due to granularity fuzziness, we may accidentally reset
>> some of
>> + * the last bits that are actually valid. So, record the
>> current value,
>> + * reset the "dead range," then re-set the one element we
>> care about. */
>> + uint64_t fix_count = (hb->size << hb->granularity) -
>> num_elements;
>> + if (fix_count) {
>> + bool set = hbitmap_get(hb, num_elements - 1);
>> + hbitmap_reset(hb, num_elements, fix_count);
>> + if (set) {
>> + hbitmap_set(hb, num_elements - 1, 1);
>> + }
>> + }
>> + }
>> +}
>> +
>> +
>> /**
>> * Given HBitmaps A and B, let A := A (BITOR) B.
>> * Bitmap B will not be modified.
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-03 21:24 ` John Snow
@ 2015-03-03 21:27 ` Max Reitz
2015-03-03 22:48 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Max Reitz @ 2015-03-03 21:27 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 2015-03-03 at 16:24, John Snow wrote:
>
>
> On 03/03/2015 11:02 AM, Max Reitz wrote:
>> On 2015-03-02 at 18:20, John Snow wrote:
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block.c | 22 ++++++++++++++++++++
>>> include/block/block.h | 1 +
>>> include/qemu/hbitmap.h | 10 +++++++++
>>> util/hbitmap.c | 55
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 88 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index e6b2696..5eaa874 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
>>> offset)
>>> ret = drv->bdrv_truncate(bs, offset);
>>> if (ret == 0) {
>>> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>>> + bdrv_dirty_bitmap_truncate(bs);
>>> if (bs->blk) {
>>> blk_dev_resize_cb(bs->blk);
>>> }
>>> @@ -5562,6 +5563,27 @@ BdrvDirtyBitmap
>>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>> return parent;
>>> }
>>> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
>>> size)
>>> +{
>>> + /* Should only be frozen during a block backup job, which should
>>> have
>>> + * blocked any resize actions. */
>>> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
>>> + hbitmap_truncate(bitmap->bitmap, size);
>>> +}
>>> +
>>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>> +{
>>> + BdrvDirtyBitmap *bitmap;
>>> + uint64_t size = bdrv_nb_sectors(bs);
>>> +
>>> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>>> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>> + continue;
>>> + }
>>
>> Hm!
>>
>> The assert() above doesn't do anything because this condition will just
>> skip over frozen bitmaps. Maybe we should drop it?
>>
>> I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will always
>> be false anyway (hence the assertion), but it would stand with this
>> block dropped, too.
>>
>> Max
>>
>
> It's there for parity with the other bdrv helpers that perform a
> similar check. I am guarding against, in the future, any potential
> users from deciding to call this function directly for whichever reason.
>
> Requesting an action on a drive that contains bitmaps that are not
> ready for the action is OK. Requesting an action on a bitmap that is
> not ready for the action should never happen, hence the 'documentation
> assert.'
>
> My default action will be to leave it in.
Technically that's fine (which is why I said my R-b stands), my only
problem with that is that if something really strange happens and we
actually end up with a frozen bitmap here, the action we're taking is
wrong. The bitmap will have the wrong size after the BDS was truncated,
and that's not right.
I'd find just calling dirty_bitmap_truncate() regardless of the frozen
state more correct, the idea being "We want to resize all bitmaps,
because all bitmaps attached to this BDS need to be resized", and then,
in dirty_bitmap_truncate(), we notice that we cannot resize a frozen
bitmap. But we need to. So the assertion fails.
Max
> --js
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-03 21:27 ` Max Reitz
@ 2015-03-03 22:48 ` John Snow
2015-03-04 13:54 ` Max Reitz
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-03 22:48 UTC (permalink / raw)
To: Max Reitz, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 03/03/2015 04:27 PM, Max Reitz wrote:
> On 2015-03-03 at 16:24, John Snow wrote:
>>
>>
>> On 03/03/2015 11:02 AM, Max Reitz wrote:
>>> On 2015-03-02 at 18:20, John Snow wrote:
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> block.c | 22 ++++++++++++++++++++
>>>> include/block/block.h | 1 +
>>>> include/qemu/hbitmap.h | 10 +++++++++
>>>> util/hbitmap.c | 55
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 88 insertions(+)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index e6b2696..5eaa874 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
>>>> offset)
>>>> ret = drv->bdrv_truncate(bs, offset);
>>>> if (ret == 0) {
>>>> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>>>> + bdrv_dirty_bitmap_truncate(bs);
>>>> if (bs->blk) {
>>>> blk_dev_resize_cb(bs->blk);
>>>> }
>>>> @@ -5562,6 +5563,27 @@ BdrvDirtyBitmap
>>>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>>> return parent;
>>>> }
>>>> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
>>>> size)
>>>> +{
>>>> + /* Should only be frozen during a block backup job, which should
>>>> have
>>>> + * blocked any resize actions. */
>>>> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
>>>> + hbitmap_truncate(bitmap->bitmap, size);
>>>> +}
>>>> +
>>>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>>> +{
>>>> + BdrvDirtyBitmap *bitmap;
>>>> + uint64_t size = bdrv_nb_sectors(bs);
>>>> +
>>>> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>>>> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>>> + continue;
>>>> + }
>>>
>>> Hm!
>>>
>>> The assert() above doesn't do anything because this condition will just
>>> skip over frozen bitmaps. Maybe we should drop it?
>>>
>>> I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will always
>>> be false anyway (hence the assertion), but it would stand with this
>>> block dropped, too.
>>>
>>> Max
>>>
>>
>> It's there for parity with the other bdrv helpers that perform a
>> similar check. I am guarding against, in the future, any potential
>> users from deciding to call this function directly for whichever reason.
>>
>> Requesting an action on a drive that contains bitmaps that are not
>> ready for the action is OK. Requesting an action on a bitmap that is
>> not ready for the action should never happen, hence the 'documentation
>> assert.'
>>
>> My default action will be to leave it in.
>
> Technically that's fine (which is why I said my R-b stands), my only
> problem with that is that if something really strange happens and we
> actually end up with a frozen bitmap here, the action we're taking is
> wrong. The bitmap will have the wrong size after the BDS was truncated,
> and that's not right.
>
If something really strange happens I would rather prefer the person
making the really strange patch to know sooner rather than later that
they're violating some assumptions made for incremental backups.
The only way the bitmap can be frozen is currently during a backup,
which should prevent any user-facing resize actions from the outset.
If that changes, it's important that we know about it. truncating a
frozen bitmap is not a circumstance I want to entertain being possible.
> I'd find just calling dirty_bitmap_truncate() regardless of the frozen
> state more correct, the idea being "We want to resize all bitmaps,
> because all bitmaps attached to this BDS need to be resized", and then,
> in dirty_bitmap_truncate(), we notice that we cannot resize a frozen
> bitmap. But we need to. So the assertion fails.
>
This is a good point, though. By the above reasoning, it's better to
force this whole command to fail exquisitely. Since we already do a soft
user check that should suffice up at qmp_drive_resize, we should already
be avoiding the assert() under normal operating conditions.
I'd rather not re-spin for this one item, though ... If it's fine by
everyone else I'll tidy this up in the transaction patch that's going
out shortly.
> Max
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-03 22:48 ` John Snow
@ 2015-03-04 13:54 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2015-03-04 13:54 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, famz, qemu-devel, armbru, vsementsov, stefanha
On 2015-03-03 at 17:48, John Snow wrote:
>
>
> On 03/03/2015 04:27 PM, Max Reitz wrote:
>> On 2015-03-03 at 16:24, John Snow wrote:
>>>
>>>
>>> On 03/03/2015 11:02 AM, Max Reitz wrote:
>>>> On 2015-03-02 at 18:20, John Snow wrote:
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> block.c | 22 ++++++++++++++++++++
>>>>> include/block/block.h | 1 +
>>>>> include/qemu/hbitmap.h | 10 +++++++++
>>>>> util/hbitmap.c | 55
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 4 files changed, 88 insertions(+)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index e6b2696..5eaa874 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -3543,6 +3543,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
>>>>> offset)
>>>>> ret = drv->bdrv_truncate(bs, offset);
>>>>> if (ret == 0) {
>>>>> ret = refresh_total_sectors(bs, offset >>
>>>>> BDRV_SECTOR_BITS);
>>>>> + bdrv_dirty_bitmap_truncate(bs);
>>>>> if (bs->blk) {
>>>>> blk_dev_resize_cb(bs->blk);
>>>>> }
>>>>> @@ -5562,6 +5563,27 @@ BdrvDirtyBitmap
>>>>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>>>> return parent;
>>>>> }
>>>>> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
>>>>> size)
>>>>> +{
>>>>> + /* Should only be frozen during a block backup job, which should
>>>>> have
>>>>> + * blocked any resize actions. */
>>>>> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
>>>>> + hbitmap_truncate(bitmap->bitmap, size);
>>>>> +}
>>>>> +
>>>>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>>>> +{
>>>>> + BdrvDirtyBitmap *bitmap;
>>>>> + uint64_t size = bdrv_nb_sectors(bs);
>>>>> +
>>>>> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>>>>> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
>>>>> + continue;
>>>>> + }
>>>>
>>>> Hm!
>>>>
>>>> The assert() above doesn't do anything because this condition will
>>>> just
>>>> skip over frozen bitmaps. Maybe we should drop it?
>>>>
>>>> I guess my R-b stands since bdrv_dirty_bitmap_frozen(bitmap) will
>>>> always
>>>> be false anyway (hence the assertion), but it would stand with this
>>>> block dropped, too.
>>>>
>>>> Max
>>>>
>>>
>>> It's there for parity with the other bdrv helpers that perform a
>>> similar check. I am guarding against, in the future, any potential
>>> users from deciding to call this function directly for whichever
>>> reason.
>>>
>>> Requesting an action on a drive that contains bitmaps that are not
>>> ready for the action is OK. Requesting an action on a bitmap that is
>>> not ready for the action should never happen, hence the 'documentation
>>> assert.'
>>>
>>> My default action will be to leave it in.
>>
>> Technically that's fine (which is why I said my R-b stands), my only
>> problem with that is that if something really strange happens and we
>> actually end up with a frozen bitmap here, the action we're taking is
>> wrong. The bitmap will have the wrong size after the BDS was truncated,
>> and that's not right.
>>
>
> If something really strange happens I would rather prefer the person
> making the really strange patch to know sooner rather than later that
> they're violating some assumptions made for incremental backups.
Me, too. That's why I'd like an assertion. :-)
> The only way the bitmap can be frozen is currently during a backup,
> which should prevent any user-facing resize actions from the outset.
Right. This is why I'm (grudgingly) fine with it.
> If that changes, it's important that we know about it. truncating a
> frozen bitmap is not a circumstance I want to entertain being possible.
That's why I'd like the assertion to do something instead of being
skipped silently. :-)
>> I'd find just calling dirty_bitmap_truncate() regardless of the frozen
>> state more correct, the idea being "We want to resize all bitmaps,
>> because all bitmaps attached to this BDS need to be resized", and then,
>> in dirty_bitmap_truncate(), we notice that we cannot resize a frozen
>> bitmap. But we need to. So the assertion fails.
>>
>
> This is a good point, though. By the above reasoning, it's better to
> force this whole command to fail exquisitely. Since we already do a
> soft user check that should suffice up at qmp_drive_resize, we should
> already be avoiding the assert() under normal operating conditions.
Right, an assert() is for cases that cannot happen (some call it "for
contracts").
> I'd rather not re-spin for this one item, though ... If it's fine by
> everyone else I'll tidy this up in the transaction patch that's going
> out shortly.
That's completely fine with me.
Max
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 01/17] docs: incremental backup documentation
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation John Snow
2015-03-03 15:28 ` Max Reitz
@ 2015-03-11 13:43 ` Stefan Hajnoczi
2015-03-11 14:19 ` John Snow
1 sibling, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 13:43 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 873 bytes --]
On Mon, Mar 02, 2015 at 06:19:47PM -0500, John Snow wrote:
> +## Bitmap Modes
> +
> +* A Bitmap can be "enabled" (tracking writes, the default) or "disabled"
> +(read-only, I/O is ignored.) This state is currently only changed internally
> +for the purposes of migration, and otherwise remains enabled.
From a QMP API perspective this information is not relevant. The
documentation is clearer without the concept of enabled/disabled.
> +## Errors
> +
> +* In the event of an error that occurs after a backup job is successfully
> + launched, either by a direct QMP command or a QMP transaction, the user
> + will receive a BLOCK_JOB_COMPLETE event with a failure message.
I would add BLOCK_JOB_CANCELLED just in case. When aborting a QMP
transaction it's likely that the job will be cancelled and the user will
not get a BLOCK_JOB_COMPLETE event.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 04/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 04/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
@ 2015-03-11 13:58 ` Stefan Hajnoczi
2015-03-11 14:23 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 13:58 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]
On Mon, Mar 02, 2015 at 06:19:50PM -0500, John Snow wrote:
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
> + const char *name,
> + BlockDriverState **pbs,
> + Error **errp)
> +{
> + BlockDriverState *bs;
> + BdrvDirtyBitmap *bitmap;
> +
> + if (!node) {
> + error_setg(errp, "Node cannot be NULL");
> + return NULL;
> + }
> + if (!name) {
> + error_setg(errp, "Bitmap name cannot be NULL");
> + return NULL;
> + }
> +
> + bs = bdrv_lookup_bs(node, node, NULL);
> + if (!bs) {
> + error_setg(errp, "Node '%s' not found", node);
> + return NULL;
> + }
> +
> + /* If caller provided a BDS*, provide the result of that lookup, too. */
> + if (pbs) {
> + *pbs = bs;
> + }
> +
> + bitmap = bdrv_find_dirty_bitmap(bs, name);
AioContext is not held here. I'm worried that a block job (running
under the AioContext) could change or delete the bitmap at the same time
as this function is running.
This function should acquire the AioContext before calling
bdrv_find_dirty_bitmap() and fill out an AioContext** parameter so the
caller can continue to use bs/bitmap under the lock.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 01/17] docs: incremental backup documentation
2015-03-11 13:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2015-03-11 14:19 ` John Snow
2015-03-11 14:43 ` Eric Blake
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2015-03-11 14:19 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: vladimir Sementsov-Ogievskiy, famz >> Fam Zheng,
qemu-devel, armbru >> Markus Armbruster
On 03/11/2015 09:43 AM, Stefan Hajnoczi wrote:
> On Mon, Mar 02, 2015 at 06:19:47PM -0500, John Snow wrote:
>> +## Bitmap Modes
>> +
>> +* A Bitmap can be "enabled" (tracking writes, the default) or "disabled"
>> +(read-only, I/O is ignored.) This state is currently only changed internally
>> +for the purposes of migration, and otherwise remains enabled.
>
> From a QMP API perspective this information is not relevant. The
> documentation is clearer without the concept of enabled/disabled.
>
Hm ... I suppose; If you want all mention of this gone from user view, I
should actually remove this status field from the query, too. It is
entirely possible to have a state where the bitmap is not frozen, but it
is disabled (migration, perhaps others in the future?) so I left the
status visible to the user for now.
libvirt et al could likely rely on the migration status to know not to
manipulate bitmaps, but having that status information directly in the
bitmap didn't bother me.
>> +## Errors
>> +
>> +* In the event of an error that occurs after a backup job is successfully
>> + launched, either by a direct QMP command or a QMP transaction, the user
>> + will receive a BLOCK_JOB_COMPLETE event with a failure message.
>
> I would add BLOCK_JOB_CANCELLED just in case. When aborting a QMP
> transaction it's likely that the job will be cancelled and the user will
> not get a BLOCK_JOB_COMPLETE event.
>
I can elaborate, sure.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 04/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
2015-03-11 13:58 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2015-03-11 14:23 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-11 14:23 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
On 03/11/2015 09:58 AM, Stefan Hajnoczi wrote:
> On Mon, Mar 02, 2015 at 06:19:50PM -0500, John Snow wrote:
>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
>> + const char *name,
>> + BlockDriverState **pbs,
>> + Error **errp)
>> +{
>> + BlockDriverState *bs;
>> + BdrvDirtyBitmap *bitmap;
>> +
>> + if (!node) {
>> + error_setg(errp, "Node cannot be NULL");
>> + return NULL;
>> + }
>> + if (!name) {
>> + error_setg(errp, "Bitmap name cannot be NULL");
>> + return NULL;
>> + }
>> +
>> + bs = bdrv_lookup_bs(node, node, NULL);
>> + if (!bs) {
>> + error_setg(errp, "Node '%s' not found", node);
>> + return NULL;
>> + }
>> +
>> + /* If caller provided a BDS*, provide the result of that lookup, too. */
>> + if (pbs) {
>> + *pbs = bs;
>> + }
>> +
>> + bitmap = bdrv_find_dirty_bitmap(bs, name);
>
> AioContext is not held here. I'm worried that a block job (running
> under the AioContext) could change or delete the bitmap at the same time
> as this function is running.
>
> This function should acquire the AioContext before calling
> bdrv_find_dirty_bitmap() and fill out an AioContext** parameter so the
> caller can continue to use bs/bitmap under the lock.
>
OK, I'll audit all uses of this function with this in mind.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 01/17] docs: incremental backup documentation
2015-03-11 14:19 ` John Snow
@ 2015-03-11 14:43 ` Eric Blake
2015-03-11 14:45 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Eric Blake @ 2015-03-11 14:43 UTC (permalink / raw)
To: John Snow, Stefan Hajnoczi
Cc: vladimir Sementsov-Ogievskiy, famz >> Fam Zheng,
qemu-devel, armbru >> Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]
On 03/11/2015 08:19 AM, John Snow wrote:
>
>
> On 03/11/2015 09:43 AM, Stefan Hajnoczi wrote:
>> On Mon, Mar 02, 2015 at 06:19:47PM -0500, John Snow wrote:
>>> +## Bitmap Modes
>>> +
>>> +* A Bitmap can be "enabled" (tracking writes, the default) or
>>> "disabled"
>>> +(read-only, I/O is ignored.) This state is currently only changed
>>> internally
>>> +for the purposes of migration, and otherwise remains enabled.
>>
>> From a QMP API perspective this information is not relevant. The
>> documentation is clearer without the concept of enabled/disabled.
>>
>
> Hm ... I suppose; If you want all mention of this gone from user view, I
> should actually remove this status field from the query, too. It is
> entirely possible to have a state where the bitmap is not frozen, but it
> is disabled (migration, perhaps others in the future?) so I left the
> status visible to the user for now.
>
> libvirt et al could likely rely on the migration status to know not to
> manipulate bitmaps, but having that status information directly in the
> bitmap didn't bother me.
With a back-compat perspective, it's easier to hide the field now and
add it later if we discover that it would have been useful to expose,
than it is to expose it now and then have to support it forever even
though no one uses it. I'm not yet sure if libvirt can make use of
knowing whether a bitmap is frozen/disabled.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 01/17] docs: incremental backup documentation
2015-03-11 14:43 ` Eric Blake
@ 2015-03-11 14:45 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-11 14:45 UTC (permalink / raw)
To: Eric Blake, Stefan Hajnoczi
Cc: vladimir Sementsov-Ogievskiy, famz >> Fam Zheng,
qemu-devel, armbru >> Markus Armbruster
On 03/11/2015 10:43 AM, Eric Blake wrote:
> On 03/11/2015 08:19 AM, John Snow wrote:
>>
>>
>> On 03/11/2015 09:43 AM, Stefan Hajnoczi wrote:
>>> On Mon, Mar 02, 2015 at 06:19:47PM -0500, John Snow wrote:
>>>> +## Bitmap Modes
>>>> +
>>>> +* A Bitmap can be "enabled" (tracking writes, the default) or
>>>> "disabled"
>>>> +(read-only, I/O is ignored.) This state is currently only changed
>>>> internally
>>>> +for the purposes of migration, and otherwise remains enabled.
>>>
>>> From a QMP API perspective this information is not relevant. The
>>> documentation is clearer without the concept of enabled/disabled.
>>>
>>
>> Hm ... I suppose; If you want all mention of this gone from user view, I
>> should actually remove this status field from the query, too. It is
>> entirely possible to have a state where the bitmap is not frozen, but it
>> is disabled (migration, perhaps others in the future?) so I left the
>> status visible to the user for now.
>>
>> libvirt et al could likely rely on the migration status to know not to
>> manipulate bitmaps, but having that status information directly in the
>> bitmap didn't bother me.
>
> With a back-compat perspective, it's easier to hide the field now and
> add it later if we discover that it would have been useful to expose,
> than it is to expose it now and then have to support it forever even
> though no one uses it. I'm not yet sure if libvirt can make use of
> knowing whether a bitmap is frozen/disabled.
>
Makes sense. I suspect I will leave the frozen status there, though,
because applying an operation to a frozen bitmap WILL result in an
error, and it would be nice to be able to avoid that scenario instead of
it being a hidden status that sometimes fails your commands.
Granted, it is only currently in this status while it is being USED for
a job, so it may not be mechanically useful...
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 05/17] block: Introduce bdrv_dirty_bitmap_granularity()
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 05/17] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
@ 2015-03-11 15:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 15:34 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 521 bytes --]
On Mon, Mar 02, 2015 at 06:19:51PM -0500, John Snow wrote:
> This returns the granularity (in bytes) of dirty bitmap,
> which matches the QMP interface and the existing query
> interface.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> block.c | 8 ++++++--
> include/block/block.h | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 03/17] qmp: Ensure consistent granularity type
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 03/17] qmp: Ensure consistent granularity type John Snow
@ 2015-03-11 15:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 15:34 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 629 bytes --]
On Mon, Mar 02, 2015 at 06:19:49PM -0500, John Snow wrote:
> We treat this field with a variety of different types everywhere
> in the code. Now it's just uint32_t.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 11 ++++++-----
> block/mirror.c | 4 ++--
> include/block/block.h | 2 +-
> include/block/block_int.h | 2 +-
> qapi/block-core.json | 2 +-
> 5 files changed, 11 insertions(+), 10 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 02/17] qapi: Add optional field "name" to block dirty bitmap
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 02/17] qapi: Add optional field "name" to block dirty bitmap John Snow
@ 2015-03-11 15:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 15:34 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]
On Mon, Mar 02, 2015 at 06:19:48PM -0500, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> This field will be set for user created dirty bitmap. Also pass in an
> error pointer to bdrv_create_dirty_bitmap, so when a name is already
> taken on this BDS, it can report an error message. This is not global
> check, two BDSes can have dirty bitmap with a common name.
>
> Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
> be used later when other QMP commands want to reference dirty bitmap by
> name.
>
> Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> block.c | 32 +++++++++++++++++++++++++++++++-
> block/mirror.c | 2 +-
> include/block/block.h | 7 ++++++-
> migration/block.c | 2 +-
> qapi/block-core.json | 4 +++-
> 5 files changed, 42 insertions(+), 5 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 06/17] hbitmap: add hbitmap_merge
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 06/17] hbitmap: add hbitmap_merge John Snow
@ 2015-03-11 15:43 ` Stefan Hajnoczi
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 15:43 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 952 bytes --]
On Mon, Mar 02, 2015 at 06:19:52PM -0500, John Snow wrote:
> We add a bitmap merge operation to assist in error cases
> where we wish to combine two bitmaps together.
>
> This is algorithmically O(bits) provided HBITMAP_LEVELS remains
> constant. For a full bitmap on a 64bit machine:
> sum(bits/64^k, k, 0, HBITMAP_LEVELS) ~= 1.01587 * bits
>
> We may be able to improve running speed for particularly sparse
> bitmaps by using iterators, but the running time for dense maps
> will be worse.
>
> We present the simpler solution first, and we can refine it later
> if needed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> include/qemu/hbitmap.h | 11 +++++++++++
> util/hbitmap.c | 32 ++++++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate John Snow
2015-03-03 15:29 ` Max Reitz
2015-03-03 16:02 ` Max Reitz
@ 2015-03-11 16:18 ` Stefan Hajnoczi
2015-03-11 17:04 ` John Snow
2 siblings, 1 reply; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 16:18 UTC (permalink / raw)
To: John Snow
Cc: kwolf, famz, qemu-block, qemu-devel, armbru, vsementsov,
stefanha, mreitz
[-- Attachment #1: Type: text/plain, Size: 4337 bytes --]
On Mon, Mar 02, 2015 at 06:20:00PM -0500, John Snow wrote:
> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size)
> +{
> + /* Should only be frozen during a block backup job, which should have
> + * blocked any resize actions. */
> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
> + hbitmap_truncate(bitmap->bitmap, size);
> +}
> +
> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> +{
> + BdrvDirtyBitmap *bitmap;
> + uint64_t size = bdrv_nb_sectors(bs);
> +
> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
> + continue;
> + }
> + dirty_bitmap_truncate(bitmap, size);
If you inline this function here then the discussion about assert() vs
skipping frozen bitmaps goes away. Why is dirty_bitmap_truncate() a
function?
> + }
> +}
Why is bdrv_dirty_bitmap_truncate() a public API? I expected this code
to be inline or called as a static function by bdrv_truncate().
> /**
> + * hbitmap_truncate:
> + * @hb: The bitmap to change the size of.
> + * @size: The number of elements to change the bitmap to accommodate.
> + *
> + * truncate or grow an existing bitmap to accommodate a new number of elements.
> + * This may invalidate existing HBitmapIterators.
> + */
> +void hbitmap_truncate(HBitmap *hb, uint64_t size);
Please include a tests/test-hbitmap.c test case.
Interesting cases:
1. New size equals old size (odd but possible)
2. Growing less than sizeof(unsigned long)
3. Growing more than sizeof(unsigned long)
4. Shrinking less than sizeof(unsigned long)
5. Shrinking more than sizeof(unsigned long)
> +void hbitmap_truncate(HBitmap *hb, uint64_t size)
> +{
> + bool truncate;
> + unsigned i;
> + uint64_t num_elements = size;
> + uint64_t old;
> +
> + /* Size comes in as logical elements, adjust for granularity. */
> + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
> + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
> + truncate = size < hb->size;
Here "truncate" means "shrink".
"shrink" is a clearer name since the function name is already "truncate"
but that concept includes both increasing or decreasing size.
> +
> + if (size == hb->size) {
> + /* A hard day's work */
> + return;
> + }
> +
> + hb->size = size;
> + for (i = HBITMAP_LEVELS; i-- > 0; ) {
> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + if (hb->sizes[i] == size) {
> + continue;
> + }
> + old = hb->sizes[i];
> + hb->sizes[i] = size;
I was wondering what sizes[] is used for. Not a very useful struct
field since it's only needed by this rarely called function.
It would be clearer to calculate 'old' alongside 'size' each loop
iteration. The size[] field can be dropped, 'old' becomes 'old_size',
and 'size' becomes 'new_size':
old_size = hb->size;
for (i = HBITMAP_LEVELS; i-- > 0; ) {
old_size = MAX((old_size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
new_size = MAX((new_size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + hb->levels[i] = g_realloc_n(hb->levels[i], size, sizeof(unsigned long));
> + if (!truncate) {
> + memset(&hb->levels[i][old], 0x00,
> + (size - old) * sizeof(*hb->levels[i]));
> + }
> + }
> + assert(size == 1);
> +
> + /* Clear out any "extra space" we may have that the user didn't request:
> + * It may have garbage data in it, now. */
> + if (truncate) {
> + /* Due to granularity fuzziness, we may accidentally reset some of
> + * the last bits that are actually valid. So, record the current value,
> + * reset the "dead range," then re-set the one element we care about. */
> + uint64_t fix_count = (hb->size << hb->granularity) - num_elements;
> + if (fix_count) {
> + bool set = hbitmap_get(hb, num_elements - 1);
> + hbitmap_reset(hb, num_elements, fix_count);
> + if (set) {
> + hbitmap_set(hb, num_elements - 1, 1);
> + }
> + }
Calling hbitmap_reset() with an out-of-bounds index seems hacky to me.
Why doesn't the for loop's if (!truncate) have an else statement to mask
no longer visible bits? Maybe I'm missing why that's hard to do.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 07/17] block: Add bitmap disabled status
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 07/17] block: Add bitmap disabled status John Snow
@ 2015-03-11 16:34 ` Stefan Hajnoczi
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 16:34 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 700 bytes --]
On Mon, Mar 02, 2015 at 06:19:53PM -0500, John Snow wrote:
> Add a status indicating the enabled/disabled state of the bitmap.
> A bitmap is by default enabled, but you can lock the bitmap into
> a read-only state by setting disabled = true.
>
> A previous version of this patch added a QMP interface for changing
> the state of the bitmap, but it has since been removed for now until
> a use case emerges where this state must be revealed to the user.
>
> The disabled state WILL be used internally for bitmap migration and
> bitmap persistence.
Only the enabled and frozen state are used by this series.
Please move this patch to a series that uses the disabled state.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 11/17] qmp: Add dirty bitmap status fields in query-block
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 11/17] qmp: Add dirty bitmap status fields in query-block John Snow
@ 2015-03-11 16:54 ` Stefan Hajnoczi
0 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 16:54 UTC (permalink / raw)
To: John Snow
Cc: kwolf, famz, qemu-block, qemu-devel, armbru, vsementsov,
stefanha, mreitz
[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]
On Mon, Mar 02, 2015 at 06:19:57PM -0500, John Snow wrote:
> Adds the "disabled" and "frozen" status booleans.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 2 ++
> qapi/block-core.json | 7 ++++++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index d969b24..06b4264 100644
> --- a/block.c
> +++ b/block.c
> @@ -5602,6 +5602,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> info->granularity = bdrv_dirty_bitmap_granularity(bm);
> info->has_name = !!bm->name;
> info->name = g_strdup(bm->name);
> + info->disabled = bm->disabled;
> + info->frozen = bdrv_dirty_bitmap_frozen(bm);
> entry->value = info;
> *plist = entry;
> plist = &entry->next;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 50970c4..7e4e14b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -335,10 +335,15 @@
> #
> # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
> #
> +# @disabled: whether the dirty bitmap is disabled (Since 2.4)
> +#
> +# @frozen: whether the dirty bitmap is frozen (Since 2.4)
> +#
> # Since: 1.3
> ##
> { 'type': 'BlockDirtyInfo',
> - 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} }
> + 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> + 'disabled': 'bool', 'frozen': 'bool'} }
frozen is useful information because the bitmap cannot be deleted while
frozen.
I'm not sure disabled is needed since it is not used separately from
frozen in this patch series. Please add it later, when disabled is
actually needed.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/17] block: transactionless incremental backup series
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
` (17 preceding siblings ...)
2015-03-03 15:26 ` [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series Max Reitz
@ 2015-03-11 16:57 ` Stefan Hajnoczi
18 siblings, 0 replies; 41+ messages in thread
From: Stefan Hajnoczi @ 2015-03-11 16:57 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, armbru, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 4562 bytes --]
On Mon, Mar 02, 2015 at 06:19:46PM -0500, John Snow wrote:
> New topic, new version, same great incremental backup taste.
>
> This series relies on [Qemu-devel] [PULL 30/69] blkdebug: fix "once" rule,
> part of Stefan's 69 patch pull request submitted 2015-02-27.
>
> This patchset enables the in-memory part of the incremental backup
> feature, without transactional support.
>
> Support for transactions will come at a later date, but getting this
> portion upstream will help stabilize work on bitmap persistence and
> bitmap migration.
>
> Transactional support is being postponed to allow more development
> to create a transactional callback system that will allow objects used
> by the drive backup routines to perform cleanup actions conditionally
> based on the outcome of all backups in the transaction group.
>
> This series was once said to have been written by a man known only
> to Qemu-Devel as "Fam Zheng", A programming maverick hellbent on never
> again wasting space by making full backups when he did not have to. The
> true origins of this series are now lost to the sands of time.
>
> (12+ revisions later: Thanks, Fam!)
>
> ===
> v2:
> ===
>
> 01: Added a new opening blurb.
> Adjusted codeblock indentations to be 4 spaces instead of 3,
> so it works as MD or GFMD.
> Adjusted errors explanation.
> Make visual separations between json data and shell commands
> Eliminate any ligering single quotes
>
> 07: Remember that memset takes bytes, not n_items ...
>
> ===
> v1:
> ===
>
> Deletions:
> - Removed Transactions, to be added later.
> - Removed Transaction tests, as above.
>
> Changes:
> 01: Indentation fixes.
> Removed enable/disable documentation.
> Added a note that transactions aren't implemented yet.
> Removed my needless commas
> Added error case documentation.
>
> 07: QMP enable/disable commands are deleted.
>
> 14: Some comments concerning assertions.
> Scrub re-alloc memory if we expand the array.
> Do not attempt to scrub memory if fix_count is 0
>
> Changes made with Reviews kept:
>
> 02: Since 2.4
> 04: Since 2.4
> Demingled the QMP command documentation.
> 08: Additions to what was qmp_block_dirty_enable/disable
> are no longer present as those function no longer exist.
> 09: Since 2.4
> 10: Since 2.4
> Demingled QMP command documentation.
> 11: Since 2.4
> 15: Test 112 --> 124
> 17: Number of tests altered. (Only 4, now.)
>
> Fam Zheng (1):
> qapi: Add optional field "name" to block dirty bitmap
>
> John Snow (16):
> docs: incremental backup documentation
> qmp: Ensure consistent granularity type
> qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
> block: Introduce bdrv_dirty_bitmap_granularity()
> hbitmap: add hbitmap_merge
> block: Add bitmap disabled status
> block: Add bitmap successors
> qmp: Add support of "dirty-bitmap" sync mode for drive-backup
> qmp: add block-dirty-bitmap-clear
> qmp: Add dirty bitmap status fields in query-block
> block: add BdrvDirtyBitmap documentation
> block: Ensure consistent bitmap function prototypes
> block: Resize bitmaps on bdrv_truncate
> iotests: add invalid input incremental backup tests
> iotests: add simple incremental backup case
> iotests: add incremental backup failure recovery test
>
> block.c | 248 +++++++++++++++++++++++++++++++--
> block/backup.c | 149 ++++++++++++++++----
> block/mirror.c | 46 +++----
> blockdev.c | 162 +++++++++++++++++++++-
> docs/bitmaps.md | 311 ++++++++++++++++++++++++++++++++++++++++++
> hmp.c | 3 +-
> include/block/block.h | 34 ++++-
> include/block/block_int.h | 4 +-
> include/qemu/hbitmap.h | 21 +++
> migration/block.c | 9 +-
> qapi/block-core.json | 92 ++++++++++++-
> qmp-commands.hx | 92 ++++++++++++-
> tests/qemu-iotests/124 | 266 ++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/124.out | 5 +
> tests/qemu-iotests/group | 1 +
> tests/qemu-iotests/iotests.py | 6 +-
> util/hbitmap.c | 87 ++++++++++++
> 17 files changed, 1446 insertions(+), 90 deletions(-)
> create mode 100644 docs/bitmaps.md
> create mode 100644 tests/qemu-iotests/124
> create mode 100644 tests/qemu-iotests/124.out
Looks pretty close.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate
2015-03-11 16:18 ` Stefan Hajnoczi
@ 2015-03-11 17:04 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2015-03-11 17:04 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, famz, qemu-block, qemu-devel, armbru, vsementsov,
stefanha, mreitz
On 03/11/2015 12:18 PM, Stefan Hajnoczi wrote:
> On Mon, Mar 02, 2015 at 06:20:00PM -0500, John Snow wrote:
>> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size)
>> +{
>> + /* Should only be frozen during a block backup job, which should have
>> + * blocked any resize actions. */
>> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
>> + hbitmap_truncate(bitmap->bitmap, size);
>> +}
>> +
>> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>> +{
>> + BdrvDirtyBitmap *bitmap;
>> + uint64_t size = bdrv_nb_sectors(bs);
>> +
>> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> + continue;
>> + }
>> + dirty_bitmap_truncate(bitmap, size);
>
> If you inline this function here then the discussion about assert() vs
> skipping frozen bitmaps goes away. Why is dirty_bitmap_truncate() a
> function?
>
Symmetry with other bitmap functions.
>> + }
>> +}
>
> Why is bdrv_dirty_bitmap_truncate() a public API? I expected this code
> to be inline or called as a static function by bdrv_truncate().
>
OK, fixing that.
>> /**
>> + * hbitmap_truncate:
>> + * @hb: The bitmap to change the size of.
>> + * @size: The number of elements to change the bitmap to accommodate.
>> + *
>> + * truncate or grow an existing bitmap to accommodate a new number of elements.
>> + * This may invalidate existing HBitmapIterators.
>> + */
>> +void hbitmap_truncate(HBitmap *hb, uint64_t size);
>
> Please include a tests/test-hbitmap.c test case.
>
> Interesting cases:
> 1. New size equals old size (odd but possible)
> 2. Growing less than sizeof(unsigned long)
> 3. Growing more than sizeof(unsigned long)
> 4. Shrinking less than sizeof(unsigned long)
> 5. Shrinking more than sizeof(unsigned long)
>
;_; OK, you're right...
>> +void hbitmap_truncate(HBitmap *hb, uint64_t size)
>> +{
>> + bool truncate;
>> + unsigned i;
>> + uint64_t num_elements = size;
>> + uint64_t old;
>> +
>> + /* Size comes in as logical elements, adjust for granularity. */
>> + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
>> + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
>> + truncate = size < hb->size;
>
> Here "truncate" means "shrink".
>
> "shrink" is a clearer name since the function name is already "truncate"
> but that concept includes both increasing or decreasing size.
>
Yes, fair enough. Was relying on what I consider the colloquial
definition of truncate.
>> +
>> + if (size == hb->size) {
>> + /* A hard day's work */
>> + return;
>> + }
>> +
>> + hb->size = size;
>> + for (i = HBITMAP_LEVELS; i-- > 0; ) {
>> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>> + if (hb->sizes[i] == size) {
>> + continue;
>> + }
>> + old = hb->sizes[i];
>> + hb->sizes[i] = size;
>
> I was wondering what sizes[] is used for. Not a very useful struct
> field since it's only needed by this rarely called function.
>
In future patches, we tend to recalculate the size of each array a lot.
I decided I wanted to cache it so we could stop duplicating that code
over and over.
It comes up in migration and persistence a lot. It's easier to just add
it now instead of allow the duplication to sneak in and then patch it
out everywhere.
> It would be clearer to calculate 'old' alongside 'size' each loop
> iteration. The size[] field can be dropped, 'old' becomes 'old_size',
> and 'size' becomes 'new_size':
>
> old_size = hb->size;
> for (i = HBITMAP_LEVELS; i-- > 0; ) {
> old_size = MAX((old_size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> new_size = MAX((new_size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>
>> + hb->levels[i] = g_realloc_n(hb->levels[i], size, sizeof(unsigned long));
>> + if (!truncate) {
>> + memset(&hb->levels[i][old], 0x00,
>> + (size - old) * sizeof(*hb->levels[i]));
>> + }
>> + }
>> + assert(size == 1);
>> +
>> + /* Clear out any "extra space" we may have that the user didn't request:
>> + * It may have garbage data in it, now. */
>> + if (truncate) {
>> + /* Due to granularity fuzziness, we may accidentally reset some of
>> + * the last bits that are actually valid. So, record the current value,
>> + * reset the "dead range," then re-set the one element we care about. */
>> + uint64_t fix_count = (hb->size << hb->granularity) - num_elements;
>> + if (fix_count) {
>> + bool set = hbitmap_get(hb, num_elements - 1);
>> + hbitmap_reset(hb, num_elements, fix_count);
>> + if (set) {
>> + hbitmap_set(hb, num_elements - 1, 1);
>> + }
>> + }
>
> Calling hbitmap_reset() with an out-of-bounds index seems hacky to me.
>
It's the simplest way to re-use the existing code to recursively clear
out any bits that are set that shouldn't be.
> Why doesn't the for loop's if (!truncate) have an else statement to mask
> no longer visible bits? Maybe I'm missing why that's hard to do.
>
I just didn't see a reason to replicate the logic of what hbitmap_reset
already does, so I didn't bother to try.
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2015-03-11 17:04 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-02 23:19 [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 01/17] docs: incremental backup documentation John Snow
2015-03-03 15:28 ` Max Reitz
2015-03-11 13:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-11 14:19 ` John Snow
2015-03-11 14:43 ` Eric Blake
2015-03-11 14:45 ` John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 02/17] qapi: Add optional field "name" to block dirty bitmap John Snow
2015-03-11 15:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 03/17] qmp: Ensure consistent granularity type John Snow
2015-03-11 15:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 04/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove John Snow
2015-03-11 13:58 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-11 14:23 ` John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 05/17] block: Introduce bdrv_dirty_bitmap_granularity() John Snow
2015-03-11 15:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 06/17] hbitmap: add hbitmap_merge John Snow
2015-03-11 15:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 07/17] block: Add bitmap disabled status John Snow
2015-03-11 16:34 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 08/17] block: Add bitmap successors John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 09/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 10/17] qmp: add block-dirty-bitmap-clear John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 11/17] qmp: Add dirty bitmap status fields in query-block John Snow
2015-03-11 16:54 ` Stefan Hajnoczi
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 12/17] block: add BdrvDirtyBitmap documentation John Snow
2015-03-02 23:19 ` [Qemu-devel] [PATCH v2 13/17] block: Ensure consistent bitmap function prototypes John Snow
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate John Snow
2015-03-03 15:29 ` Max Reitz
2015-03-03 16:02 ` Max Reitz
2015-03-03 21:24 ` John Snow
2015-03-03 21:27 ` Max Reitz
2015-03-03 22:48 ` John Snow
2015-03-04 13:54 ` Max Reitz
2015-03-11 16:18 ` Stefan Hajnoczi
2015-03-11 17:04 ` John Snow
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 15/17] iotests: add invalid input incremental backup tests John Snow
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 16/17] iotests: add simple incremental backup case John Snow
2015-03-02 23:20 ` [Qemu-devel] [PATCH v2 17/17] iotests: add incremental backup failure recovery test John Snow
2015-03-03 15:26 ` [Qemu-devel] [PATCH v2 00/17] block: transactionless incremental backup series Max Reitz
2015-03-11 16:57 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.