* [PULL 00/19] Bitmaps patches
@ 2019-10-11 21:25 John Snow
2019-10-11 21:25 ` [PULL 01/19] util/hbitmap: strict hbitmap_reset John Snow
` (19 more replies)
0 siblings, 20 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
The following changes since commit 98b2e3c9ab3abfe476a2b02f8f51813edb90e72d:
Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2019-10-08 16:08:35 +0100)
are available in the Git repository at:
https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
for you to fetch changes up to b97d9a1014b61dd0980e7f4a0c9ca1e3b0aaa761:
dirty-bitmaps: remove deprecated autoload parameter (2019-10-09 17:02:45 -0400)
----------------------------------------------------------------
Pull request
----------------------------------------------------------------
John Snow (2):
MAINTAINERS: Add Vladimir as a reviewer for bitmaps
dirty-bitmaps: remove deprecated autoload parameter
Vladimir Sementsov-Ogievskiy (17):
util/hbitmap: strict hbitmap_reset
block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
block/dirty-bitmap: return int from
bdrv_remove_persistent_dirty_bitmap
block/qcow2: proper locking on bitmap add/remove paths
block/dirty-bitmap: drop meta
block/dirty-bitmap: add bs link
block/dirty-bitmap: drop BdrvDirtyBitmap.mutex
block/dirty-bitmap: refactor bdrv_dirty_bitmap_next
block: switch reopen queue from QSIMPLEQ to QTAILQ
block: reverse order for reopen commits
iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
block/qcow2-bitmap: do not remove bitmaps on reopen-ro
iotests: add test 260 to check bitmap life after snapshot + commit
block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit
qemu-deprecated.texi | 20 ++-
qapi/block-core.json | 6 +-
block/qcow2.h | 19 +--
include/block/block.h | 2 +-
include/block/block_int.h | 20 +--
include/block/dirty-bitmap.h | 34 ++--
include/qemu/hbitmap.h | 5 +
block.c | 79 +++------
block/backup.c | 14 +-
block/dirty-bitmap.c | 290 +++++++++++++++++++--------------
block/mirror.c | 4 +-
block/qcow2-bitmap.c | 212 +++++++++++++++---------
block/qcow2.c | 22 ++-
blockdev.c | 40 ++---
migration/block-dirty-bitmap.c | 11 +-
migration/block.c | 4 +-
tests/test-hbitmap.c | 2 +-
util/hbitmap.c | 4 +
MAINTAINERS | 3 +-
tests/qemu-iotests/165 | 57 ++++++-
tests/qemu-iotests/165.out | 4 +-
tests/qemu-iotests/260 | 89 ++++++++++
tests/qemu-iotests/260.out | 52 ++++++
tests/qemu-iotests/group | 1 +
24 files changed, 624 insertions(+), 370 deletions(-)
create mode 100755 tests/qemu-iotests/260
create mode 100644 tests/qemu-iotests/260.out
--
2.21.0
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PULL 01/19] util/hbitmap: strict hbitmap_reset
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:48 ` Eric Blake
2019-10-11 21:25 ` [PULL 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c John Snow
` (18 subsequent siblings)
19 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
hbitmap_reset has an unobvious property: it rounds requested region up.
It may provoke bugs, like in recently fixed write-blocking mode of
mirror: user calls reset on unaligned region, not keeping in mind that
there are possible unrelated dirty bytes, covered by rounded-up region
and information of this unrelated "dirtiness" will be lost.
Make hbitmap_reset strict: assert that arguments are aligned, allowing
only one exception when @start + @count == hb->orig_size. It's needed
to comfort users of hbitmap_next_dirty_area, which cares about
hb->orig_size.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20190806152611.280389-1-vsementsov@virtuozzo.com>
[Maintainer edit: Max's suggestions from on-list. --js]
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/qemu/hbitmap.h | 5 +++++
tests/test-hbitmap.c | 2 +-
util/hbitmap.c | 4 ++++
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 4afbe6292e3..1bf944ca3d1 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -132,6 +132,11 @@ void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count);
* @count: Number of bits to reset.
*
* Reset a consecutive range of bits in an HBitmap.
+ * @start and @count must be aligned to bitmap granularity. The only exception
+ * is resetting the tail of the bitmap: @count may be equal to hb->orig_size -
+ * @start, in this case @count may be not aligned. The sum of @start + @count is
+ * allowed to be greater than hb->orig_size, but only if @start < hb->orig_size
+ * and @start + @count = ALIGN_UP(hb->orig_size, granularity).
*/
void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count);
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index eed5d288cbc..e1f867085f4 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -423,7 +423,7 @@ static void test_hbitmap_granularity(TestHBitmapData *data,
hbitmap_test_check(data, 0);
hbitmap_test_set(data, 0, 3);
g_assert_cmpint(hbitmap_count(data->hb), ==, 4);
- hbitmap_test_reset(data, 0, 1);
+ hbitmap_test_reset(data, 0, 2);
g_assert_cmpint(hbitmap_count(data->hb), ==, 2);
}
diff --git a/util/hbitmap.c b/util/hbitmap.c
index fd44c897ab0..757d39e360a 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
/* Compute range in the last layer. */
uint64_t first;
uint64_t last = start + count - 1;
+ uint64_t gran = 1ULL << hb->granularity;
+
+ assert(!(start & (gran - 1)));
+ assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
trace_hbitmap_reset(hb, start, count,
start >> hb->granularity, last >> hb->granularity);
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
2019-10-11 21:25 ` [PULL 01/19] util/hbitmap: strict hbitmap_reset John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap John Snow
` (17 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
block/dirty-bitmap.c seems to be more appropriate for it and
bdrv_remove_persistent_dirty_bitmap already in it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190920082543.23444-2-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 22 ----------------------
block/dirty-bitmap.c | 22 ++++++++++++++++++++++
2 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/block.c b/block.c
index 59441248451..bea03cfcc92 100644
--- a/block.c
+++ b/block.c
@@ -6555,25 +6555,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
}
-
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp)
-{
- BlockDriver *drv = bs->drv;
-
- if (!drv) {
- error_setg_errno(errp, ENOMEDIUM,
- "Can't store persistent bitmaps to %s",
- bdrv_get_device_or_node_name(bs));
- return false;
- }
-
- if (!drv->bdrv_can_store_new_dirty_bitmap) {
- error_setg_errno(errp, ENOTSUP,
- "Can't store persistent bitmaps to %s",
- bdrv_get_device_or_node_name(bs));
- return false;
- }
-
- return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c8..8f42015db95 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -464,6 +464,28 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
}
}
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
+{
+ BlockDriver *drv = bs->drv;
+
+ if (!drv) {
+ error_setg_errno(errp, ENOMEDIUM,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+ return false;
+ }
+
+ if (!drv->bdrv_can_store_new_dirty_bitmap) {
+ error_setg_errno(errp, ENOTSUP,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+ return false;
+ }
+
+ return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
+
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
bdrv_dirty_bitmap_lock(bitmap);
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
2019-10-11 21:25 ` [PULL 01/19] util/hbitmap: strict hbitmap_reset John Snow
2019-10-11 21:25 ` [PULL 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 04/19] block/qcow2: proper locking on bitmap add/remove paths John Snow
` (16 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
It's more comfortable to not deal with local_err.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190920082543.23444-3-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 5 ++---
include/block/block_int.h | 6 +++---
include/block/dirty-bitmap.h | 5 ++---
block/dirty-bitmap.c | 9 +++++----
block/qcow2-bitmap.c | 18 ++++++++++--------
blockdev.c | 7 +++----
6 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index a488d761ff0..2ed54821635 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -747,9 +747,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
Error **errp);
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp);
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+ Error **errp);
ssize_t coroutine_fn
qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 0422acdf1c4..503ac9e3cd2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -556,9 +556,9 @@ struct BlockDriver {
const char *name,
uint32_t granularity,
Error **errp);
- void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
- const char *name,
- Error **errp);
+ int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
/**
* Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4b4b731b469..07503b03b53 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
Error **errp);
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp);
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+ Error **errp);
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8f42015db95..d1ae2e19229 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
* not fail.
* This function doesn't release corresponding BdrvDirtyBitmap.
*/
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp)
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+ Error **errp)
{
if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
- bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+ return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
}
+
+ return 0;
}
bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ede..9821c1628f5 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1404,9 +1404,8 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
return NULL;
}
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp)
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+ Error **errp)
{
int ret;
BDRVQcow2State *s = bs->opaque;
@@ -1416,18 +1415,19 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
if (s->nb_bitmaps == 0) {
/* Absence of the bitmap is not an error: see explanation above
* bdrv_remove_persistent_dirty_bitmap() definition. */
- return;
+ return 0;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
if (bm_list == NULL) {
- return;
+ return -EIO;
}
bm = find_bitmap_by_name(bm_list, name);
if (bm == NULL) {
- goto fail;
+ ret = -EINVAL;
+ goto out;
}
QSIMPLEQ_REMOVE(bm_list, bm, Qcow2Bitmap, entry);
@@ -1435,14 +1435,16 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
ret = update_ext_header_and_dir(bs, bm_list);
if (ret < 0) {
error_setg_errno(errp, -ret, "Failed to update bitmap extension");
- goto fail;
+ goto out;
}
free_bitmap_clusters(bs, &bm->table);
-fail:
+out:
bitmap_free(bm);
bitmap_list_free(bm_list);
+
+ return ret;
}
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
diff --git a/blockdev.c b/blockdev.c
index fbef6845c84..0813adfb2b1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2940,15 +2940,14 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
}
if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+ int ret;
AioContext *aio_context = bdrv_get_aio_context(bs);
- Error *local_err = NULL;
aio_context_acquire(aio_context);
- bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
+ ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
aio_context_release(aio_context);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
+ if (ret < 0) {
return NULL;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 04/19] block/qcow2: proper locking on bitmap add/remove paths
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (2 preceding siblings ...)
2019-10-11 21:25 ` [PULL 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 05/19] block/dirty-bitmap: drop meta John Snow
` (15 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
qmp_block_dirty_bitmap_add and do_block_dirty_bitmap_remove do acquire
aio context since 0a6c86d024c52b. But this is not enough: we also must
lock qcow2 mutex when access in-image metadata. Especially it concerns
freeing qcow2 clusters.
To achieve this, move qcow2_can_store_new_dirty_bitmap and
qcow2_remove_persistent_dirty_bitmap to coroutine context.
Since we work in coroutines in correct aio context, we don't need
context acquiring in blockdev.c anymore, drop it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190920082543.23444-4-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 11 ++--
include/block/block_int.h | 10 ++--
block/dirty-bitmap.c | 102 +++++++++++++++++++++++++++++++++++---
block/qcow2-bitmap.c | 24 ++++++---
block/qcow2.c | 5 +-
blockdev.c | 27 +++-------
6 files changed, 131 insertions(+), 48 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 2ed54821635..113d99bf520 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -743,12 +743,13 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- uint32_t granularity,
- Error **errp);
-int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ uint32_t granularity,
Error **errp);
+int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
ssize_t coroutine_fn
qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 503ac9e3cd2..1e54486ad14 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -552,13 +552,13 @@ struct BlockDriver {
* field of BlockDirtyBitmap's in case of success.
*/
int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
- bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
- const char *name,
- uint32_t granularity,
- Error **errp);
- int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+ bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
+ uint32_t granularity,
Error **errp);
+ int (*bdrv_co_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
/**
* Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d1ae2e19229..03e0872b972 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -26,6 +26,7 @@
#include "trace.h"
#include "block/block_int.h"
#include "block/blockjob.h"
+#include "qemu/main-loop.h"
struct BdrvDirtyBitmap {
QemuMutex *mutex;
@@ -455,18 +456,59 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
* not fail.
* This function doesn't release corresponding BdrvDirtyBitmap.
*/
+static int coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+ Error **errp)
+{
+ if (bs->drv && bs->drv->bdrv_co_remove_persistent_dirty_bitmap) {
+ return bs->drv->bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
+ }
+
+ return 0;
+}
+
+typedef struct BdrvRemovePersistentDirtyBitmapCo {
+ BlockDriverState *bs;
+ const char *name;
+ Error **errp;
+ int ret;
+} BdrvRemovePersistentDirtyBitmapCo;
+
+static void coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
+{
+ BdrvRemovePersistentDirtyBitmapCo *s = opaque;
+
+ s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
+ aio_wait_kick();
+}
+
int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
{
- if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
- return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+ if (qemu_in_coroutine()) {
+ return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
+ } else {
+ Coroutine *co;
+ BdrvRemovePersistentDirtyBitmapCo s = {
+ .bs = bs,
+ .name = name,
+ .errp = errp,
+ .ret = -EINPROGRESS,
+ };
+
+ co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry,
+ &s);
+ bdrv_coroutine_enter(bs, co);
+ BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS);
+
+ return s.ret;
}
-
- return 0;
}
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp)
+static bool coroutine_fn
+bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
{
BlockDriver *drv = bs->drv;
@@ -477,14 +519,58 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
return false;
}
- if (!drv->bdrv_can_store_new_dirty_bitmap) {
+ if (!drv->bdrv_co_can_store_new_dirty_bitmap) {
error_setg_errno(errp, ENOTSUP,
"Can't store persistent bitmaps to %s",
bdrv_get_device_or_node_name(bs));
return false;
}
- return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+ return drv->bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
+
+typedef struct BdrvCanStoreNewDirtyBitmapCo {
+ BlockDriverState *bs;
+ const char *name;
+ uint32_t granularity;
+ Error **errp;
+ bool ret;
+
+ bool in_progress;
+} BdrvCanStoreNewDirtyBitmapCo;
+
+static void coroutine_fn bdrv_co_can_store_new_dirty_bitmap_entry(void *opaque)
+{
+ BdrvCanStoreNewDirtyBitmapCo *s = opaque;
+
+ s->ret = bdrv_co_can_store_new_dirty_bitmap(s->bs, s->name, s->granularity,
+ s->errp);
+ s->in_progress = false;
+ aio_wait_kick();
+}
+
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
+{
+ if (qemu_in_coroutine()) {
+ return bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+ } else {
+ Coroutine *co;
+ BdrvCanStoreNewDirtyBitmapCo s = {
+ .bs = bs,
+ .name = name,
+ .granularity = granularity,
+ .errp = errp,
+ .in_progress = true,
+ };
+
+ co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry,
+ &s);
+ bdrv_coroutine_enter(bs, co);
+ BDRV_POLL_WHILE(bs, s.in_progress);
+
+ return s.ret;
+ }
}
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 9821c1628f5..644837eb033 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1404,12 +1404,13 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
return NULL;
}
-int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
- Error **errp)
+int coroutine_fn qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp)
{
int ret;
BDRVQcow2State *s = bs->opaque;
- Qcow2Bitmap *bm;
+ Qcow2Bitmap *bm = NULL;
Qcow2BitmapList *bm_list;
if (s->nb_bitmaps == 0) {
@@ -1418,10 +1419,13 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
return 0;
}
+ qemu_co_mutex_lock(&s->lock);
+
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
if (bm_list == NULL) {
- return -EIO;
+ ret = -EIO;
+ goto out;
}
bm = find_bitmap_by_name(bm_list, name);
@@ -1441,6 +1445,8 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
free_bitmap_clusters(bs, &bm->table);
out:
+ qemu_co_mutex_unlock(&s->lock);
+
bitmap_free(bm);
bitmap_list_free(bm_list);
@@ -1615,10 +1621,10 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
return 0;
}
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- uint32_t granularity,
- Error **errp)
+bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ uint32_t granularity,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
bool found;
@@ -1655,8 +1661,10 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
goto fail;
}
+ qemu_co_mutex_lock(&s->lock);
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
+ qemu_co_mutex_unlock(&s->lock);
if (bm_list == NULL) {
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 4d16393e618..43a6cc423de 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5263,8 +5263,9 @@ BlockDriver bdrv_qcow2 = {
.bdrv_attach_aio_context = qcow2_attach_aio_context,
.bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
- .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
- .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
+ .bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap,
+ .bdrv_co_remove_persistent_dirty_bitmap =
+ qcow2_co_remove_persistent_dirty_bitmap,
};
static void bdrv_qcow2_init(void)
diff --git a/blockdev.c b/blockdev.c
index 0813adfb2b1..228ce94a88c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2898,16 +2898,10 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
disabled = false;
}
- if (persistent) {
- AioContext *aio_context = bdrv_get_aio_context(bs);
- bool ok;
-
- aio_context_acquire(aio_context);
- ok = bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
- aio_context_release(aio_context);
- if (!ok) {
- return;
- }
+ if (persistent &&
+ !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+ {
+ return;
}
bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
@@ -2939,17 +2933,10 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
return NULL;
}
- if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
- int ret;
- AioContext *aio_context = bdrv_get_aio_context(bs);
-
- aio_context_acquire(aio_context);
- ret = bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
- aio_context_release(aio_context);
-
- if (ret < 0) {
+ if (bdrv_dirty_bitmap_get_persistence(bitmap) &&
+ bdrv_remove_persistent_dirty_bitmap(bs, name, errp) < 0)
+ {
return NULL;
- }
}
if (release) {
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 05/19] block/dirty-bitmap: drop meta
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (3 preceding siblings ...)
2019-10-11 21:25 ` [PULL 04/19] block/qcow2: proper locking on bitmap add/remove paths John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 06/19] block/dirty-bitmap: add bs link John Snow
` (14 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Drop meta bitmaps, as they are unused.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190916141911.5255-2-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/dirty-bitmap.h | 5 ----
block/dirty-bitmap.c | 46 ------------------------------------
2 files changed, 51 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 07503b03b53..973056778aa 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,9 +18,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
uint32_t granularity,
const char *name,
Error **errp);
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int chunk_size);
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
@@ -54,7 +51,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes);
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes);
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
@@ -96,7 +92,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 03e0872b972..4ecf18d5df7 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -31,7 +31,6 @@
struct BdrvDirtyBitmap {
QemuMutex *mutex;
HBitmap *bitmap; /* Dirty bitmap implementation */
- HBitmap *meta; /* Meta dirty bitmap */
bool busy; /* Bitmap is busy, it can't be used via QMP */
BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
char *name; /* Optional non-empty unique ID */
@@ -127,36 +126,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return bitmap;
}
-/* bdrv_create_meta_dirty_bitmap
- *
- * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
- * when a dirty status bit in @bitmap is changed (either from reset to set or
- * the other way around), its respective meta dirty bitmap bit will be marked
- * dirty as well.
- *
- * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
- * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
- * track.
- */
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int chunk_size)
-{
- assert(!bitmap->meta);
- qemu_mutex_lock(bitmap->mutex);
- bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
- chunk_size * BITS_PER_BYTE);
- qemu_mutex_unlock(bitmap->mutex);
-}
-
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-{
- assert(bitmap->meta);
- qemu_mutex_lock(bitmap->mutex);
- hbitmap_free_meta(bitmap->bitmap);
- bitmap->meta = NULL;
- qemu_mutex_unlock(bitmap->mutex);
-}
-
int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
{
return bitmap->size;
@@ -320,7 +289,6 @@ static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
assert(!bitmap->active_iterators);
assert(!bdrv_dirty_bitmap_busy(bitmap));
assert(!bdrv_dirty_bitmap_has_successor(bitmap));
- assert(!bitmap->meta);
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
g_free(bitmap->name);
@@ -666,15 +634,6 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
return iter;
}
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
-{
- BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
- hbitmap_iter_init(&iter->hbi, bitmap->meta, 0);
- iter->bitmap = bitmap;
- bitmap->active_iterators++;
- return iter;
-}
-
void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
{
if (!iter) {
@@ -821,11 +780,6 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
return hbitmap_count(bitmap->bitmap);
}
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
-{
- return hbitmap_count(bitmap->meta);
-}
-
bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
{
return bitmap->readonly;
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 06/19] block/dirty-bitmap: add bs link
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (4 preceding siblings ...)
2019-10-11 21:25 ` [PULL 05/19] block/dirty-bitmap: drop meta John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex John Snow
` (13 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Add bs field to BdrvDirtyBitmap structure. Drop BlockDriverState
parameter from bitmap APIs where possible.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190916141911.5255-3-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/dirty-bitmap.h | 14 +++++---------
block/backup.c | 14 ++++++--------
block/dirty-bitmap.c | 24 ++++++++++++------------
block/mirror.c | 4 ++--
block/qcow2-bitmap.c | 6 +++---
blockdev.c | 6 +++---
migration/block-dirty-bitmap.c | 7 +++----
migration/block.c | 4 ++--
8 files changed, 36 insertions(+), 43 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 973056778aa..2f9b088e11e 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,21 +18,18 @@ 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,
+int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap,
Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *bitmap,
Error **errp);
void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
Error **errp);
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp);
@@ -106,8 +103,7 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
uint64_t bytes);
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
uint64_t *offset, uint64_t *bytes);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
Error **errp);
#endif
diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6d..acb67da3a7b 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -352,7 +352,6 @@ static int coroutine_fn backup_before_write_notify(
static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
{
BdrvDirtyBitmap *bm;
- BlockDriverState *bs = blk_bs(job->common.blk);
bool sync = (((ret == 0) || (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS)) \
&& (job->bitmap_mode != BITMAP_SYNC_MODE_NEVER));
@@ -361,13 +360,13 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
* We succeeded, or we always intended to sync the bitmap.
* Delete this bitmap and install the child.
*/
- bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
+ bm = bdrv_dirty_bitmap_abdicate(job->sync_bitmap, NULL);
} else {
/*
* We failed, or we never intended to sync the bitmap anyway.
* Merge the successor back into the parent, keeping all data.
*/
- bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
+ bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
}
assert(bm);
@@ -398,10 +397,9 @@ static void backup_abort(Job *job)
static void backup_clean(Job *job)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
- BlockDriverState *bs = blk_bs(s->common.blk);
if (s->copy_bitmap) {
- bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
+ bdrv_release_dirty_bitmap(s->copy_bitmap);
s->copy_bitmap = NULL;
}
@@ -679,7 +677,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
}
/* Create a new bitmap, and freeze/disable this one. */
- if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+ if (bdrv_dirty_bitmap_create_successor(sync_bitmap, errp) < 0) {
return NULL;
}
}
@@ -758,10 +756,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
error:
if (copy_bitmap) {
assert(!job || !job->copy_bitmap);
- bdrv_release_dirty_bitmap(bs, copy_bitmap);
+ bdrv_release_dirty_bitmap(copy_bitmap);
}
if (sync_bitmap) {
- bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+ bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL);
}
if (job) {
backup_clean(&job->common.job);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4ecf18d5df7..44453ff8241 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -30,6 +30,7 @@
struct BdrvDirtyBitmap {
QemuMutex *mutex;
+ BlockDriverState *bs;
HBitmap *bitmap; /* Dirty bitmap implementation */
bool busy; /* Bitmap is busy, it can't be used via QMP */
BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
@@ -115,6 +116,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return NULL;
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
+ bitmap->bs = bs;
bitmap->mutex = &bs->dirty_bitmap_mutex;
bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
bitmap->size = bitmap_size;
@@ -237,8 +239,7 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
* The successor will be enabled if the parent bitmap was.
* Called with BQL taken.
*/
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, Error **errp)
+int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap, Error **errp)
{
uint64_t granularity;
BdrvDirtyBitmap *child;
@@ -254,7 +255,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Create an anonymous successor */
granularity = bdrv_dirty_bitmap_granularity(bitmap);
- child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+ child = bdrv_create_dirty_bitmap(bitmap->bs, granularity, NULL, errp);
if (!child) {
return -1;
}
@@ -300,8 +301,7 @@ static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
* delete the old bitmap, and return a handle to the new bitmap.
* Called with BQL taken.
*/
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
Error **errp)
{
char *name;
@@ -320,7 +320,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
successor->persistent = bitmap->persistent;
bitmap->persistent = false;
bitmap->busy = false;
- bdrv_release_dirty_bitmap(bs, bitmap);
+ bdrv_release_dirty_bitmap(bitmap);
return successor;
}
@@ -332,8 +332,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
* The marged parent will be enabled if and only if the successor was enabled.
* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
*/
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
- BdrvDirtyBitmap *parent,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
Error **errp)
{
BdrvDirtyBitmap *successor = parent->successor;
@@ -357,14 +356,13 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
}
/* Called with BQL taken. */
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
- BdrvDirtyBitmap *parent,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *parent,
Error **errp)
{
BdrvDirtyBitmap *ret;
qemu_mutex_lock(parent->mutex);
- ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
+ ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp);
qemu_mutex_unlock(parent->mutex);
return ret;
@@ -390,8 +388,10 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
}
/* Called with BQL taken. */
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
+ BlockDriverState *bs = bitmap->bs;
+
bdrv_dirty_bitmaps_lock(bs);
bdrv_release_dirty_bitmap_locked(bitmap);
bdrv_dirty_bitmaps_unlock(bs);
diff --git a/block/mirror.c b/block/mirror.c
index fe984efb90b..a6c50caea44 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -638,7 +638,7 @@ static int mirror_exit_common(Job *job)
bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
}
- bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
+ bdrv_release_dirty_bitmap(s->dirty_bitmap);
/* Make sure that the source BDS doesn't go away during bdrv_replace_node,
* before we can call bdrv_drained_end */
@@ -1709,7 +1709,7 @@ fail:
blk_unref(s->target);
bs_opaque->job = NULL;
if (s->dirty_bitmap) {
- bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
+ bdrv_release_dirty_bitmap(s->dirty_bitmap);
}
job_early_fail(&s->common.job);
}
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 644837eb033..687087d2bc2 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -374,7 +374,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
fail:
g_free(bitmap_table);
if (bitmap != NULL) {
- bdrv_release_dirty_bitmap(bs, bitmap);
+ bdrv_release_dirty_bitmap(bitmap);
}
return NULL;
@@ -941,7 +941,7 @@ fail:
static void release_dirty_bitmap_helper(gpointer bitmap,
gpointer bs)
{
- bdrv_release_dirty_bitmap(bs, bitmap);
+ bdrv_release_dirty_bitmap(bitmap);
}
/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
@@ -1577,7 +1577,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
continue;
}
- bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+ bdrv_release_dirty_bitmap(bm->dirty_bitmap);
}
bitmap_list_free(bm_list);
diff --git a/blockdev.c b/blockdev.c
index 228ce94a88c..9b6eca66430 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2178,7 +2178,7 @@ static void block_dirty_bitmap_remove_commit(BlkActionState *common)
common, common);
bdrv_dirty_bitmap_set_busy(state->bitmap, false);
- bdrv_release_dirty_bitmap(state->bs, state->bitmap);
+ bdrv_release_dirty_bitmap(state->bitmap);
}
static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2940,7 +2940,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
}
if (release) {
- bdrv_release_dirty_bitmap(bs, bitmap);
+ bdrv_release_dirty_bitmap(bitmap);
}
if (bitmap_bs) {
@@ -3072,7 +3072,7 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
out:
- bdrv_release_dirty_bitmap(bs, anon);
+ bdrv_release_dirty_bitmap(anon);
return dst;
}
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 5121f86d734..793f249aa5b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -474,7 +474,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
DirtyBitmapLoadBitmapState *b;
- bdrv_dirty_bitmap_create_successor(s->bs, s->bitmap, &local_err);
+ bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
if (local_err) {
error_report_err(local_err);
return -EINVAL;
@@ -535,13 +535,12 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
bdrv_dirty_bitmap_lock(s->bitmap);
if (enabled_bitmaps == NULL) {
/* in postcopy */
- bdrv_reclaim_dirty_bitmap_locked(s->bs, s->bitmap, &error_abort);
+ bdrv_reclaim_dirty_bitmap_locked(s->bitmap, &error_abort);
bdrv_enable_dirty_bitmap_locked(s->bitmap);
} else {
/* target not started, successor must be empty */
int64_t count = bdrv_get_dirty_count(s->bitmap);
- BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bs,
- s->bitmap,
+ BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bitmap,
NULL);
/* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
* must be) or on merge fail, but merge can't fail when second
diff --git a/migration/block.c b/migration/block.c
index 8e493820707..c90288ed290 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -361,7 +361,7 @@ static int set_dirty_tracking(void)
fail:
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
if (bmds->dirty_bitmap) {
- bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap);
+ bdrv_release_dirty_bitmap(bmds->dirty_bitmap);
}
}
return ret;
@@ -374,7 +374,7 @@ static void unset_dirty_tracking(void)
BlkMigDevState *bmds;
QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
- bdrv_release_dirty_bitmap(blk_bs(bmds->blk), bmds->dirty_bitmap);
+ bdrv_release_dirty_bitmap(bmds->dirty_bitmap);
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (5 preceding siblings ...)
2019-10-11 21:25 ` [PULL 06/19] block/dirty-bitmap: add bs link John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next John Snow
` (12 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
mutex field is just a pointer to bs->dirty_bitmap_mutex, so no needs
to store it in BdrvDirtyBitmap when we have bs pointer in it (since
previous patch).
Drop mutex field. Constantly use bdrv_dirty_bitmaps_lock/unlock in
block/dirty-bitmap.c to make it more obvious that it's not per-bitmap
lock. Still, for simplicity, leave bdrv_dirty_bitmap_lock/unlock
functions as an external API.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190916141911.5255-4-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/dirty-bitmap.c | 84 +++++++++++++++++++++-----------------------
1 file changed, 41 insertions(+), 43 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 44453ff8241..4e5c87a907f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -29,7 +29,6 @@
#include "qemu/main-loop.h"
struct BdrvDirtyBitmap {
- QemuMutex *mutex;
BlockDriverState *bs;
HBitmap *bitmap; /* Dirty bitmap implementation */
bool busy; /* Bitmap is busy, it can't be used via QMP */
@@ -72,12 +71,12 @@ static inline void bdrv_dirty_bitmaps_unlock(BlockDriverState *bs)
void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap)
{
- qemu_mutex_lock(bitmap->mutex);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
}
void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap)
{
- qemu_mutex_unlock(bitmap->mutex);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
/* Called with BQL or dirty_bitmap lock taken. */
@@ -117,7 +116,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
}
bitmap = g_new0(BdrvDirtyBitmap, 1);
bitmap->bs = bs;
- bitmap->mutex = &bs->dirty_bitmap_mutex;
bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
bitmap->size = bitmap_size;
bitmap->name = g_strdup(name);
@@ -151,9 +149,9 @@ static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap *bitmap)
void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
{
- qemu_mutex_lock(bitmap->mutex);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bitmap->busy = busy;
- qemu_mutex_unlock(bitmap->mutex);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
/* Called with BQL taken. */
@@ -278,10 +276,10 @@ void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
/* Called with BQL taken. */
void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
{
- assert(bitmap->mutex == bitmap->successor->mutex);
- qemu_mutex_lock(bitmap->mutex);
+ assert(bitmap->bs == bitmap->successor->bs);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bdrv_enable_dirty_bitmap_locked(bitmap->successor);
- qemu_mutex_unlock(bitmap->mutex);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
/* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */
@@ -361,9 +359,9 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *parent,
{
BdrvDirtyBitmap *ret;
- qemu_mutex_lock(parent->mutex);
+ bdrv_dirty_bitmaps_lock(parent->bs);
ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp);
- qemu_mutex_unlock(parent->mutex);
+ bdrv_dirty_bitmaps_unlock(parent->bs);
return ret;
}
@@ -543,16 +541,16 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
- bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bitmap->disabled = true;
- bdrv_dirty_bitmap_unlock(bitmap);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
- bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bdrv_enable_dirty_bitmap_locked(bitmap);
- bdrv_dirty_bitmap_unlock(bitmap);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
@@ -593,9 +591,9 @@ bool bdrv_dirty_bitmap_get_locked(BdrvDirtyBitmap *bitmap, int64_t offset)
bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset)
{
bool ret;
- bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
ret = bdrv_dirty_bitmap_get_locked(bitmap, offset);
- bdrv_dirty_bitmap_unlock(bitmap);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
return ret;
}
@@ -660,9 +658,9 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
{
- bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes);
- bdrv_dirty_bitmap_unlock(bitmap);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
/* Called within bdrv_dirty_bitmap_lock..unlock */
@@ -676,15 +674,15 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
{
- bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bdrv_reset_dirty_bitmap_locked(bitmap, offset, bytes);
- bdrv_dirty_bitmap_unlock(bitmap);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
{
assert(!bdrv_dirty_bitmap_readonly(bitmap));
- bdrv_dirty_bitmap_lock(bitmap);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
if (!out) {
hbitmap_reset_all(bitmap->bitmap);
} else {
@@ -693,7 +691,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
hbitmap_granularity(backup));
*out = backup;
}
- bdrv_dirty_bitmap_unlock(bitmap);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
@@ -788,9 +786,9 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
/* Called with BQL taken. */
void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value)
{
- qemu_mutex_lock(bitmap->mutex);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bitmap->readonly = value;
- qemu_mutex_unlock(bitmap->mutex);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
@@ -808,27 +806,27 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
/* Called with BQL taken. */
void bdrv_dirty_bitmap_set_persistence(BdrvDirtyBitmap *bitmap, bool persistent)
{
- qemu_mutex_lock(bitmap->mutex);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bitmap->persistent = persistent;
- qemu_mutex_unlock(bitmap->mutex);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
/* Called with BQL taken. */
void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
{
- qemu_mutex_lock(bitmap->mutex);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
assert(bitmap->persistent == true);
bitmap->inconsistent = true;
bitmap->disabled = true;
- qemu_mutex_unlock(bitmap->mutex);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
/* Called with BQL taken. */
void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip)
{
- qemu_mutex_lock(bitmap->mutex);
+ bdrv_dirty_bitmaps_lock(bitmap->bs);
bitmap->skip_store = skip;
- qemu_mutex_unlock(bitmap->mutex);
+ bdrv_dirty_bitmaps_unlock(bitmap->bs);
}
bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap)
@@ -888,9 +886,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
{
bool ret;
- qemu_mutex_lock(dest->mutex);
- if (src->mutex != dest->mutex) {
- qemu_mutex_lock(src->mutex);
+ bdrv_dirty_bitmaps_lock(dest->bs);
+ if (src->bs != dest->bs) {
+ bdrv_dirty_bitmaps_lock(src->bs);
}
if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
@@ -910,9 +908,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
assert(ret);
out:
- qemu_mutex_unlock(dest->mutex);
- if (src->mutex != dest->mutex) {
- qemu_mutex_unlock(src->mutex);
+ bdrv_dirty_bitmaps_unlock(dest->bs);
+ if (src->bs != dest->bs) {
+ bdrv_dirty_bitmaps_unlock(src->bs);
}
}
@@ -936,9 +934,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
assert(!bdrv_dirty_bitmap_inconsistent(src));
if (lock) {
- qemu_mutex_lock(dest->mutex);
- if (src->mutex != dest->mutex) {
- qemu_mutex_lock(src->mutex);
+ bdrv_dirty_bitmaps_lock(dest->bs);
+ if (src->bs != dest->bs) {
+ bdrv_dirty_bitmaps_lock(src->bs);
}
}
@@ -951,9 +949,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
}
if (lock) {
- qemu_mutex_unlock(dest->mutex);
- if (src->mutex != dest->mutex) {
- qemu_mutex_unlock(src->mutex);
+ bdrv_dirty_bitmaps_unlock(dest->bs);
+ if (src->bs != dest->bs) {
+ bdrv_dirty_bitmaps_unlock(src->bs);
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (6 preceding siblings ...)
2019-10-11 21:25 ` [PULL 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ John Snow
` (11 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
bdrv_dirty_bitmap_next is always used in same pattern. So, split it
into _next and _first, instead of combining two functions into one and
add FOR_EACH_DIRTY_BITMAP macro.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190916141911.5255-5-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/dirty-bitmap.h | 9 +++++++--
block.c | 4 +---
block/dirty-bitmap.c | 11 +++++++----
block/qcow2-bitmap.c | 8 ++------
migration/block-dirty-bitmap.c | 4 +---
5 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 2f9b088e11e..257f0f67046 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -96,8 +96,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap);
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
+#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
+for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
+ bitmap = bdrv_dirty_bitmap_next(bitmap))
+
char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
uint64_t bytes);
diff --git a/block.c b/block.c
index bea03cfcc92..5b5b0337acc 100644
--- a/block.c
+++ b/block.c
@@ -5363,9 +5363,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
}
}
- for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
- bm = bdrv_dirty_bitmap_next(bs, bm))
- {
+ FOR_EACH_DIRTY_BITMAP(bs, bm) {
bdrv_dirty_bitmap_skip_store(bm, false);
}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4e5c87a907f..6065db80949 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -851,11 +851,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
return false;
}
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap)
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
{
- return bitmap == NULL ? QLIST_FIRST(&bs->dirty_bitmaps) :
- QLIST_NEXT(bitmap, list);
+ return QLIST_FIRST(&bs->dirty_bitmaps);
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap)
+{
+ return QLIST_NEXT(bitmap, list);
}
char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 687087d2bc2..99812b418b8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1488,9 +1488,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
}
/* check constraints and names */
- for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
- {
+ FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
const char *name = bdrv_dirty_bitmap_name(bitmap);
uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
Qcow2Bitmap *bm;
@@ -1610,9 +1608,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
return -EINVAL;
}
- for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
- {
+ FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
bdrv_dirty_bitmap_set_readonly(bitmap, true);
}
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 793f249aa5b..7eafface614 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void)
for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
const char *name = bdrv_get_device_or_node_name(bs);
- for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
- {
+ FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
if (!bdrv_dirty_bitmap_name(bitmap)) {
continue;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (7 preceding siblings ...)
2019-10-11 21:25 ` [PULL 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 10/19] block: reverse order for reopen commits John Snow
` (10 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
We'll need reverse-foreach in the following commit, QTAILQ support it,
so move to QTAILQ.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20190927122355.7344-2-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/block.h | 2 +-
block.c | 24 ++++++++++++------------
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 37c9de7446d..f5099435136 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -195,7 +195,7 @@ typedef struct HDGeometry {
#define BDRV_BLOCK_EOF 0x20
#define BDRV_BLOCK_RECURSE 0x40
-typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
+typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
typedef struct BDRVReopenState {
BlockDriverState *bs;
diff --git a/block.c b/block.c
index 5b5b0337acc..aaf5d796284 100644
--- a/block.c
+++ b/block.c
@@ -1719,7 +1719,7 @@ typedef struct BlockReopenQueueEntry {
bool prepared;
bool perms_checked;
BDRVReopenState state;
- QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+ QTAILQ_ENTRY(BlockReopenQueueEntry) entry;
} BlockReopenQueueEntry;
/*
@@ -1732,7 +1732,7 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, BlockDriverState *bs)
BlockReopenQueueEntry *entry;
if (q != NULL) {
- QSIMPLEQ_FOREACH(entry, q, entry) {
+ QTAILQ_FOREACH(entry, q, entry) {
if (entry->state.bs == bs) {
return entry->state.flags;
}
@@ -3249,7 +3249,7 @@ static bool bdrv_recurse_has_child(BlockDriverState *bs,
* Adds a BlockDriverState to a simple queue for an atomic, transactional
* reopen of multiple devices.
*
- * bs_queue can either be an existing BlockReopenQueue that has had QSIMPLE_INIT
+ * bs_queue can either be an existing BlockReopenQueue that has had QTAILQ_INIT
* already performed, or alternatively may be NULL a new BlockReopenQueue will
* be created and initialized. This newly created BlockReopenQueue should be
* passed back in for subsequent calls that are intended to be of the same
@@ -3290,7 +3290,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
if (bs_queue == NULL) {
bs_queue = g_new0(BlockReopenQueue, 1);
- QSIMPLEQ_INIT(bs_queue);
+ QTAILQ_INIT(bs_queue);
}
if (!options) {
@@ -3298,7 +3298,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
}
/* Check if this BlockDriverState is already in the queue */
- QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+ QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
if (bs == bs_entry->state.bs) {
break;
}
@@ -3354,7 +3354,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
if (!bs_entry) {
bs_entry = g_new0(BlockReopenQueueEntry, 1);
- QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+ QTAILQ_INSERT_TAIL(bs_queue, bs_entry, entry);
} else {
qobject_unref(bs_entry->state.options);
qobject_unref(bs_entry->state.explicit_options);
@@ -3455,7 +3455,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
assert(bs_queue != NULL);
- QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+ QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
assert(bs_entry->state.bs->quiesce_counter > 0);
if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, errp)) {
goto cleanup;
@@ -3463,7 +3463,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
bs_entry->prepared = true;
}
- QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+ QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
BDRVReopenState *state = &bs_entry->state;
ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
state->shared_perm, NULL, NULL, errp);
@@ -3489,13 +3489,13 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
/* If we reach this point, we have success and just need to apply the
* changes
*/
- QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+ QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
bdrv_reopen_commit(&bs_entry->state);
}
ret = 0;
cleanup_perm:
- QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+ QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
BDRVReopenState *state = &bs_entry->state;
if (!bs_entry->perms_checked) {
@@ -3512,7 +3512,7 @@ cleanup_perm:
}
}
cleanup:
- QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+ QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
if (ret) {
if (bs_entry->prepared) {
bdrv_reopen_abort(&bs_entry->state);
@@ -3552,7 +3552,7 @@ static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
{
BlockReopenQueueEntry *entry;
- QSIMPLEQ_FOREACH(entry, q, entry) {
+ QTAILQ_FOREACH(entry, q, entry) {
BlockDriverState *bs = entry->state.bs;
BdrvChild *child;
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 10/19] block: reverse order for reopen commits
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (8 preceding siblings ...)
2019-10-11 21:25 ` [PULL 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW John Snow
` (9 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
can't work, as qcow2 needs write access to file child, to mark bitmaps
in-image with IN_USE flag. But usually children goes after parents in
reopen queue and file child is still RO on qcow2 reopen commit. Reverse
reopen order to fix it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Acked-by: Max Reitz <mreitz@redhat.com>
Acked-by: John Snow <jsnow@redhat.com>
Message-id: 20190927122355.7344-3-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index aaf5d796284..c548885608d 100644
--- a/block.c
+++ b/block.c
@@ -3486,10 +3486,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
bs_entry->perms_checked = true;
}
- /* If we reach this point, we have success and just need to apply the
- * changes
+ /*
+ * If we reach this point, we have success and just need to apply the
+ * changes.
+ *
+ * Reverse order is used to comfort qcow2 driver: on commit it need to write
+ * IN_USE flag to the image, to mark bitmaps in the image as invalid. But
+ * children are usually goes after parents in reopen-queue, so go from last
+ * to first element.
*/
- QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
+ QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
bdrv_reopen_commit(&bs_entry->state);
}
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (9 preceding siblings ...)
2019-10-11 21:25 ` [PULL 10/19] block: reverse order for reopen commits John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps John Snow
` (8 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reopening bitmaps to RW was broken prior to previous commit. Check that
it works now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20190927122355.7344-4-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/165 | 57 ++++++++++++++++++++++++++++++++++++--
tests/qemu-iotests/165.out | 4 +--
2 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 5650dc7c874..951ea011a27 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
os.remove(disk)
def mkVm(self):
- return iotests.VM().add_drive(disk)
+ return iotests.VM().add_drive(disk, opts='node-name=node0')
def mkVmRo(self):
- return iotests.VM().add_drive(disk, opts='readonly=on')
+ return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
def getSha256(self):
result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
@@ -102,6 +102,59 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
self.vm.shutdown()
+ def test_reopen_rw(self):
+ self.vm = self.mkVm()
+ self.vm.launch()
+ self.qmpAddBitmap()
+
+ # Calculate hashes
+
+ self.writeRegions(regions1)
+ sha256_1 = self.getSha256()
+
+ self.writeRegions(regions2)
+ sha256_2 = self.getSha256()
+ assert sha256_1 != sha256_2 # Otherwise, it's not very interesting.
+
+ result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
+ name='bitmap0')
+ self.assert_qmp(result, 'return', {})
+
+ # Start with regions1
+
+ self.writeRegions(regions1)
+ assert sha256_1 == self.getSha256()
+
+ self.vm.shutdown()
+
+ self.vm = self.mkVmRo()
+ self.vm.launch()
+
+ assert sha256_1 == self.getSha256()
+
+ # Check that we are in RO mode and can't modify bitmap.
+ self.writeRegions(regions2)
+ assert sha256_1 == self.getSha256()
+
+ # Reopen to RW
+ result = self.vm.qmp('x-blockdev-reopen', **{
+ 'node-name': 'node0',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': disk
+ },
+ 'read-only': False
+ })
+ self.assert_qmp(result, 'return', {})
+
+ # Check that bitmap is reopened to RW and we can write to it.
+ self.writeRegions(regions2)
+ assert sha256_2 == self.getSha256()
+
+ self.vm.shutdown()
+
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2'],
supported_protocols=['file'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
index ae1213e6f86..fbc63e62f88 100644
--- a/tests/qemu-iotests/165.out
+++ b/tests/qemu-iotests/165.out
@@ -1,5 +1,5 @@
-.
+..
----------------------------------------------------------------------
-Ran 1 tests
+Ran 2 tests
OK
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (10 preceding siblings ...)
2019-10-11 21:25 ` [PULL 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint() John Snow
` (7 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Firstly, no reason to optimize failure path. Then, function name is
ambiguous: it checks for readonly and similar things, but someone may
think that it will ignore normal bitmaps which was just unchanged, and
this is in bad relation with the fact that we should drop IN_USE flag
for unchanged bitmaps in the image.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190927122355.7344-5-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/dirty-bitmap.h | 1 -
block/dirty-bitmap.c | 12 ------------
block/qcow2-bitmap.c | 23 +++++++++++++----------
3 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 257f0f67046..958e7474fb5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -95,7 +95,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6065db80949..4bbb251b2c9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -839,18 +839,6 @@ bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
return bitmap->inconsistent;
}
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
-{
- BdrvDirtyBitmap *bm;
- QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
- if (bm->persistent && !bm->readonly && !bm->skip_store) {
- return true;
- }
- }
-
- return false;
-}
-
BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
{
return QLIST_FIRST(&bs->dirty_bitmaps);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 99812b418b8..6dfc0835485 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1464,16 +1464,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
Qcow2Bitmap *bm;
QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
Qcow2BitmapTable *tb, *tb_next;
-
- if (!bdrv_has_changed_persistent_bitmaps(bs)) {
- /* nothing to do */
- return;
- }
-
- if (!can_write(bs)) {
- error_setg(errp, "No write access");
- return;
- }
+ bool need_write = false;
QSIMPLEQ_INIT(&drop_tables);
@@ -1499,6 +1490,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
continue;
}
+ need_write = true;
+
if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
name);
@@ -1537,6 +1530,15 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
bm->dirty_bitmap = bitmap;
}
+ if (!need_write) {
+ goto success;
+ }
+
+ if (!can_write(bs)) {
+ error_setg(errp, "No write access");
+ goto fail;
+ }
+
/* allocate clusters and store bitmaps */
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
if (bm->dirty_bitmap == NULL) {
@@ -1578,6 +1580,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
bdrv_release_dirty_bitmap(bm->dirty_bitmap);
}
+success:
bitmap_list_free(bm_list);
return;
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (11 preceding siblings ...)
2019-10-11 21:25 ` [PULL 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro John Snow
` (6 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
The function is unused, drop it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190927122355.7344-6-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 2 --
block/qcow2-bitmap.c | 15 +--------------
2 files changed, 1 insertion(+), 16 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 113d99bf520..b3398a13c20 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -737,8 +737,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
Error **errp);
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6dfc0835485..ebc1afccd3d 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1102,8 +1102,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
return list;
}
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp)
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
@@ -1111,10 +1110,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
GSList *ro_dirty_bitmaps = NULL;
int ret = 0;
- if (header_updated != NULL) {
- *header_updated = false;
- }
-
if (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
return 0;
@@ -1156,9 +1151,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
error_setg_errno(errp, -ret, "Can't update bitmap directory");
goto out;
}
- if (header_updated != NULL) {
- *header_updated = true;
- }
g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
}
@@ -1169,11 +1161,6 @@ out:
return ret;
}
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
-{
- return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
-}
-
/* Checks to see if it's safe to resize bitmaps */
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
{
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (12 preceding siblings ...)
2019-10-11 21:25 ` [PULL 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint() John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 15/19] iotests: add test 260 to check bitmap life after snapshot + commit John Snow
` (5 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
readonly. But the latter don't work, as
qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
It's OK for inactivation but bad idea for reopen-ro. And this leads to
the following bug:
Assume we have persistent bitmap 'bitmap0'.
Create external snapshot
bitmap0 is stored and therefore removed
Commit snapshot
now we have no bitmaps
Do some writes from guest (*)
they are not marked in bitmap
Shutdown
Start
bitmap0 is loaded as valid, but it is actually broken! It misses
writes (*)
Incremental backup
it will be inconsistent
So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
reopening bitmaps to rw is broken too, so the whole scenario will not
work after this patch and we can't enable corresponding test cases in
260 iotests still. Reopening bitmaps rw will be fixed in the following
patches.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20190927122355.7344-7-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 3 ++-
block/qcow2-bitmap.c | 49 ++++++++++++++++++++++++++++++--------------
block/qcow2.c | 2 +-
3 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index b3398a13c20..8d293f2b64e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -739,7 +739,8 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+ bool release_stored, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ebc1afccd3d..f7dfb40256e 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1440,7 +1440,32 @@ out:
return ret;
}
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/*
+ * qcow2_store_persistent_dirty_bitmaps
+ *
+ * Stores persistent BdrvDirtyBitmap objects.
+ *
+ * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
+ * image. This is used in two cases, both via qcow2_inactivate:
+ * 1. bdrv_close: It's correct to remove bitmaps on close.
+ * 2. migration: If bitmaps are migrated through migration channel via
+ * 'dirty-bitmaps' migration capability they are not handled by this code.
+ * Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
+ * invalidation.
+ *
+ * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
+ * inactivation means that we lose control on disk, and therefore on bitmaps,
+ * we should sync them and do not touch more.
+ *
+ * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
+ * when we need to store them, as image is still under our control, and it's
+ * good to keep all the bitmaps in read-only mode. Moreover, keeping them
+ * read-only is correct because this is what would happen if we opened the node
+ * readonly to begin with, and whether we opened directly or reopened to that
+ * state shouldn't matter for the state we get afterward.
+ */
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+ bool release_stored, Error **errp)
{
BdrvDirtyBitmap *bitmap;
BDRVQcow2State *s = bs->opaque;
@@ -1551,20 +1576,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
g_free(tb);
}
- QSIMPLEQ_FOREACH(bm, bm_list, entry) {
- /* For safety, we remove bitmap after storing.
- * We may be here in two cases:
- * 1. bdrv_close. It's ok to drop bitmap.
- * 2. inactivation. It means migration without 'dirty-bitmaps'
- * capability, so bitmaps are not marked with
- * BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
- * and reload on invalidation.
- */
- if (bm->dirty_bitmap == NULL) {
- continue;
- }
+ if (release_stored) {
+ QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+ if (bm->dirty_bitmap == NULL) {
+ continue;
+ }
- bdrv_release_dirty_bitmap(bm->dirty_bitmap);
+ bdrv_release_dirty_bitmap(bm->dirty_bitmap);
+ }
}
success:
@@ -1592,7 +1611,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
BdrvDirtyBitmap *bitmap;
Error *local_err = NULL;
- qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
+ qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
return -EINVAL;
diff --git a/block/qcow2.c b/block/qcow2.c
index 43a6cc423de..7ab1aad9779 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2359,7 +2359,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
int ret, result = 0;
Error *local_err = NULL;
- qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
+ qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
if (local_err != NULL) {
result = -EINVAL;
error_reportf_err(local_err, "Lost persistent bitmaps during "
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 15/19] iotests: add test 260 to check bitmap life after snapshot + commit
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (13 preceding siblings ...)
2019-10-11 21:25 ` [PULL 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw John Snow
` (4 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20190927122355.7344-8-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/260 | 89 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/260.out | 52 ++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 142 insertions(+)
create mode 100755 tests/qemu-iotests/260
create mode 100644 tests/qemu-iotests/260.out
diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
new file mode 100755
index 00000000000..4f6082c9d22
--- /dev/null
+++ b/tests/qemu-iotests/260
@@ -0,0 +1,89 @@
+#!/usr/bin/env python
+#
+# Tests for temporary external snapshot when we have bitmaps.
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# 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 iotests
+from iotests import qemu_img_create, file_path, log, filter_qmp_event
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+base, top = file_path('base', 'top')
+size = 64 * 1024 * 3
+
+
+def print_bitmap(msg, vm):
+ result = vm.qmp('query-block')['return'][0]
+ if 'dirty-bitmaps' in result:
+ bitmap = result['dirty-bitmaps'][0]
+ log('{}: name={} dirty-clusters={}'.format(msg, bitmap['name'],
+ bitmap['count'] // 64 // 1024))
+ else:
+ log(msg + ': not found')
+
+
+def test(persistent, restart):
+ assert persistent or not restart
+ log("\nTestcase {}persistent {} restart\n".format(
+ '' if persistent else 'non-', 'with' if restart else 'without'))
+
+ qemu_img_create('-f', iotests.imgfmt, base, str(size))
+
+ vm = iotests.VM().add_drive(base)
+ vm.launch()
+
+ vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0',
+ persistent=persistent)
+ vm.hmp_qemu_io('drive0', 'write 0 64K')
+ print_bitmap('initial bitmap', vm)
+
+ vm.qmp_log('blockdev-snapshot-sync', device='drive0', snapshot_file=top,
+ format=iotests.imgfmt, filters=[iotests.filter_qmp_testfiles])
+ vm.hmp_qemu_io('drive0', 'write 64K 512')
+ print_bitmap('check that no bitmaps are in snapshot', vm)
+
+ if restart:
+ log("... Restart ...")
+ vm.shutdown()
+ vm = iotests.VM().add_drive(top)
+ vm.launch()
+
+ vm.qmp_log('block-commit', device='drive0', top=top,
+ filters=[iotests.filter_qmp_testfiles])
+ ev = vm.events_wait((('BLOCK_JOB_READY', None),
+ ('BLOCK_JOB_COMPLETED', None)))
+ log(filter_qmp_event(ev))
+ if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
+ vm.shutdown()
+ log(vm.get_log())
+ exit()
+
+ vm.qmp_log('block-job-complete', device='drive0')
+ ev = vm.event_wait('BLOCK_JOB_COMPLETED')
+ log(filter_qmp_event(ev))
+ print_bitmap('check bitmap after commit', vm)
+
+ vm.hmp_qemu_io('drive0', 'write 128K 64K')
+ print_bitmap('check updated bitmap', vm)
+
+ vm.shutdown()
+
+
+test(persistent=False, restart=False)
+test(persistent=True, restart=False)
+test(persistent=True, restart=True)
diff --git a/tests/qemu-iotests/260.out b/tests/qemu-iotests/260.out
new file mode 100644
index 00000000000..2f0d98d0365
--- /dev/null
+++ b/tests/qemu-iotests/260.out
@@ -0,0 +1,52 @@
+
+Testcase non-persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": false}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
+
+Testcase persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": true}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
+
+Testcase persistent with restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": true}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+... Restart ...
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5805a79d9e8..0c1e5ef4140 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -273,6 +273,7 @@
256 rw quick
257 rw
258 rw quick
+260 rw auto quick
262 rw quick migration
263 rw quick
265 rw auto quick
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (14 preceding siblings ...)
2019-10-11 21:25 ` [PULL 15/19] iotests: add test 260 to check bitmap life after snapshot + commit John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit John Snow
` (3 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
- Correct check for write access to file child, and in correct place
(only if we want to write).
- Support reopen rw -> rw (which will be used in following commit),
for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
bitmap is marked IN_USE in the image.
- Consider unexpected bitmap as a corruption and check other
combinations of in-image and in-RAM bitmaps.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20190927122355.7344-9-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 77 +++++++++++++++++++++++++++++++++-----------
1 file changed, 58 insertions(+), 19 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f7dfb40256e..98294a76965 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
GSList *ro_dirty_bitmaps = NULL;
- int ret = 0;
+ int ret = -EINVAL;
+ bool need_header_update = false;
if (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
return 0;
}
- if (!can_write(bs)) {
- error_setg(errp, "Can't write to the image on reopening bitmaps rw");
- return -EINVAL;
- }
-
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
if (bm_list == NULL) {
@@ -1128,32 +1124,75 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
- if (bitmap == NULL) {
- continue;
- }
- if (!bdrv_dirty_bitmap_readonly(bitmap)) {
- error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
- "not marked as readonly. This is a bug, something went "
- "wrong. All of the bitmaps may be corrupted", bm->name);
- ret = -EINVAL;
+ if (!bitmap) {
+ error_setg(errp, "Unexpected bitmap '%s' in image '%s'",
+ bm->name, bs->filename);
goto out;
}
- bm->flags |= BME_FLAG_IN_USE;
- ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+ if (!(bm->flags & BME_FLAG_IN_USE)) {
+ if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+ error_setg(errp, "Corruption: bitmap '%s' is not marked IN_USE "
+ "in the image '%s' and not marked readonly in RAM",
+ bm->name, bs->filename);
+ goto out;
+ }
+ if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+ error_setg(errp, "Corruption: bitmap '%s' is inconsistent but "
+ "is not marked IN_USE in the image '%s'", bm->name,
+ bs->filename);
+ goto out;
+ }
+
+ bm->flags |= BME_FLAG_IN_USE;
+ need_header_update = true;
+ } else {
+ /*
+ * What if flags already has BME_FLAG_IN_USE ?
+ *
+ * 1. if we are reopening RW -> RW it's OK, of course.
+ * 2. if we are reopening RO -> RW:
+ * 2.1 if @bitmap is inconsistent, it's OK. It means that it was
+ * inconsistent (IN_USE) when we loaded it
+ * 2.2 if @bitmap is not inconsistent. This seems to be impossible
+ * and implies third party interaction. Let's error-out for
+ * safety.
+ */
+ if (bdrv_dirty_bitmap_readonly(bitmap) &&
+ !bdrv_dirty_bitmap_inconsistent(bitmap))
+ {
+ error_setg(errp, "Corruption: bitmap '%s' is marked IN_USE "
+ "in the image '%s' but it is readonly and "
+ "consistent in RAM",
+ bm->name, bs->filename);
+ goto out;
+ }
+ }
+
+ if (bdrv_dirty_bitmap_readonly(bitmap)) {
+ ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+ }
}
- if (ro_dirty_bitmaps != NULL) {
+ if (need_header_update) {
+ if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) {
+ error_setg(errp, "Failed to reopen bitmaps rw: no write access "
+ "the protocol file");
+ goto out;
+ }
+
/* in_use flags must be updated */
ret = update_ext_header_and_dir_in_place(bs, bm_list);
if (ret < 0) {
- error_setg_errno(errp, -ret, "Can't update bitmap directory");
+ error_setg_errno(errp, -ret, "Cannot update bitmap directory");
goto out;
}
- g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
}
+ g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
+ ret = 0;
+
out:
g_slist_free(ro_dirty_bitmaps);
bitmap_list_free(bm_list);
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (15 preceding siblings ...)
2019-10-11 21:25 ` [PULL 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps John Snow
` (2 subsequent siblings)
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
The only reason I can imagine for this strange code at the very-end of
bdrv_reopen_commit is the fact that bs->read_only updated after
calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
check for being writable, when actually it only need writable file
child not self.
So, as it's fixed, let's move things to correct place.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Acked-by: Max Reitz <mreitz@redhat.com>
Message-id: 20190927122355.7344-10-vsementsov@virtuozzo.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/block_int.h | 6 ------
block.c | 19 -------------------
block/qcow2.c | 15 ++++++++++++++-
3 files changed, 14 insertions(+), 26 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1e54486ad14..9ceca23ef75 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -546,12 +546,6 @@ struct BlockDriver {
uint64_t parent_perm, uint64_t parent_shared,
uint64_t *nperm, uint64_t *nshared);
- /**
- * Bitmaps should be marked as 'IN_USE' in the image on reopening image
- * as rw. This handler should realize it. It also should unset readonly
- * field of BlockDirtyBitmap's in case of success.
- */
- int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
uint32_t granularity,
diff --git a/block.c b/block.c
index c548885608d..ba09d97e0a2 100644
--- a/block.c
+++ b/block.c
@@ -3935,16 +3935,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
BlockDriver *drv;
BlockDriverState *bs;
BdrvChild *child;
- bool old_can_write, new_can_write;
assert(reopen_state != NULL);
bs = reopen_state->bs;
drv = bs->drv;
assert(drv != NULL);
- old_can_write =
- !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-
/* If there are any driver level actions to take */
if (drv->bdrv_reopen_commit) {
drv->bdrv_reopen_commit(reopen_state);
@@ -3988,21 +3984,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
}
bdrv_refresh_limits(bs, NULL);
-
- new_can_write =
- !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
- if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
- Error *local_err = NULL;
- if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) {
- /* This is not fatal, bitmaps just left read-only, so all following
- * writes will fail. User can remove read-only bitmaps to unblock
- * writes.
- */
- error_reportf_err(local_err,
- "%s: Failed to make dirty bitmaps writable: ",
- bdrv_get_node_name(bs));
- }
- }
}
/*
diff --git a/block/qcow2.c b/block/qcow2.c
index 7ab1aad9779..b652bf884e5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1834,6 +1834,20 @@ fail:
static void qcow2_reopen_commit(BDRVReopenState *state)
{
qcow2_update_options_commit(state->bs, state->opaque);
+ if (state->flags & BDRV_O_RDWR) {
+ Error *local_err = NULL;
+
+ if (qcow2_reopen_bitmaps_rw(state->bs, &local_err) < 0) {
+ /*
+ * This is not fatal, bitmaps just left read-only, so all following
+ * writes will fail. User can remove read-only bitmaps to unblock
+ * writes or retry reopen.
+ */
+ error_reportf_err(local_err,
+ "%s: Failed to make dirty bitmaps writable: ",
+ bdrv_get_node_name(state->bs));
+ }
+ }
g_free(state->opaque);
}
@@ -5262,7 +5276,6 @@ BlockDriver bdrv_qcow2 = {
.bdrv_detach_aio_context = qcow2_detach_aio_context,
.bdrv_attach_aio_context = qcow2_attach_aio_context,
- .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
.bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap,
.bdrv_co_remove_persistent_dirty_bitmap =
qcow2_co_remove_persistent_dirty_bitmap,
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (16 preceding siblings ...)
2019-10-11 21:25 ` [PULL 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-11 21:25 ` [PULL 19/19] dirty-bitmaps: remove deprecated autoload parameter John Snow
2019-10-14 16:11 ` [PULL 00/19] Bitmaps patches Peter Maydell
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
I already try to make sure all bitmaps patches have been reviewed by both
Red Hat and Virtuozzo anyway, so this formalizes the arrangement.
Fam meanwhile is no longer as active, so I am removing him as a co-maintainer
simply to reflect the current practice.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20191005194448.16629-2-jsnow@redhat.com
---
MAINTAINERS | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 3ca814850e0..a08c92a4162 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1816,8 +1816,8 @@ F: qapi/transaction.json
T: git https://repo.or.cz/qemu/armbru.git block-next
Dirty Bitmaps
-M: Fam Zheng <fam@euphon.net>
M: John Snow <jsnow@redhat.com>
+R: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
L: qemu-block@nongnu.org
S: Supported
F: util/hbitmap.c
@@ -1826,7 +1826,6 @@ F: include/qemu/hbitmap.h
F: include/block/dirty-bitmap.h
F: tests/test-hbitmap.c
F: docs/interop/bitmaps.rst
-T: git https://github.com/famz/qemu.git bitmaps
T: git https://github.com/jnsnow/qemu.git bitmaps
Character device backends
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PULL 19/19] dirty-bitmaps: remove deprecated autoload parameter
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (17 preceding siblings ...)
2019-10-11 21:25 ` [PULL 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps John Snow
@ 2019-10-11 21:25 ` John Snow
2019-10-14 16:11 ` [PULL 00/19] Bitmaps patches Peter Maydell
19 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-11 21:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, John Snow, Dr. David Alan Gilbert,
Max Reitz, Stefan Hajnoczi, Markus Armbruster
This parameter has been deprecated since 2.12.0 and is eligible for
removal. Remove this parameter as it is actually completely ignored;
let's not give false hope.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20191002232411.29968-1-jsnow@redhat.com
---
qemu-deprecated.texi | 20 +++++++++++++++-----
qapi/block-core.json | 6 +-----
blockdev.c | 6 ------
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 01245e0b1c4..7239e0959da 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -149,11 +149,6 @@ QEMU 4.1 has three options, please migrate to one of these three:
@section QEMU Machine Protocol (QMP) commands
-@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
-
-"autoload" parameter is now ignored. All bitmaps are automatically loaded
-from qcow2 images.
-
@subsection query-block result field dirty-bitmaps[i].status (since 4.0)
The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
@@ -356,3 +351,18 @@ existing CPU models. Management software that needs runnability
guarantees must resolve the CPU model aliases using te
``alias-of'' field returned by the ``query-cpu-definitions'' QMP
command.
+
+
+@node Recently removed features
+@appendix Recently removed features
+
+What follows is a record of recently removed, formerly deprecated
+features that serves as a record for users who have encountered
+trouble after a recent upgrade.
+
+@section QEMU Machine Protocol (QMP) commands
+
+@subsection block-dirty-bitmap-add "autoload" parameter (since 4.2.0)
+
+The "autoload" parameter has been ignored since 2.12.0. All bitmaps
+are automatically loaded from qcow2 images.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e6edd641f18..e4975ece4ab 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1987,10 +1987,6 @@
# Qcow2 disks support persistent bitmaps. Default is false for
# block-dirty-bitmap-add. (Since: 2.10)
#
-# @autoload: ignored and deprecated since 2.12.
-# Currently, all dirty tracking bitmaps are loaded from Qcow2 on
-# open.
-#
# @disabled: the bitmap is created in the disabled state, which means that
# it will not track drive changes. The bitmap may be enabled with
# block-dirty-bitmap-enable. Default is false. (Since: 4.0)
@@ -1999,7 +1995,7 @@
##
{ 'struct': 'BlockDirtyBitmapAdd',
'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
- '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
+ '*persistent': 'bool', '*disabled': 'bool' } }
##
# @BlockDirtyBitmapMergeSource:
diff --git a/blockdev.c b/blockdev.c
index 9b6eca66430..873aba3c27f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1966,7 +1966,6 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
- action->has_autoload, action->autoload,
action->has_disabled, action->disabled,
&local_err);
@@ -2858,7 +2857,6 @@ out:
void qmp_block_dirty_bitmap_add(const char *node, const char *name,
bool has_granularity, uint32_t granularity,
bool has_persistent, bool persistent,
- bool has_autoload, bool autoload,
bool has_disabled, bool disabled,
Error **errp)
{
@@ -2890,10 +2888,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
persistent = false;
}
- if (has_autoload) {
- warn_report("Autoload option is deprecated and its value is ignored");
- }
-
if (!has_disabled) {
disabled = false;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PULL 01/19] util/hbitmap: strict hbitmap_reset
2019-10-11 21:25 ` [PULL 01/19] util/hbitmap: strict hbitmap_reset John Snow
@ 2019-10-11 21:48 ` Eric Blake
2019-10-11 23:18 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Eric Blake @ 2019-10-11 21:48 UTC (permalink / raw)
To: John Snow, Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, Markus Armbruster,
Dr. David Alan Gilbert, Max Reitz, Stefan Hajnoczi
On 10/11/19 4:25 PM, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> hbitmap_reset has an unobvious property: it rounds requested region up.
> It may provoke bugs, like in recently fixed write-blocking mode of
> mirror: user calls reset on unaligned region, not keeping in mind that
> there are possible unrelated dirty bytes, covered by rounded-up region
> and information of this unrelated "dirtiness" will be lost.
>
> Make hbitmap_reset strict: assert that arguments are aligned, allowing
> only one exception when @start + @count == hb->orig_size. It's needed
> to comfort users of hbitmap_next_dirty_area, which cares about
> hb->orig_size.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Message-Id: <20190806152611.280389-1-vsementsov@virtuozzo.com>
> [Maintainer edit: Max's suggestions from on-list. --js]
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> include/qemu/hbitmap.h | 5 +++++
> tests/test-hbitmap.c | 2 +-
> util/hbitmap.c | 4 ++++
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> +++ b/util/hbitmap.c
> @@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
> /* Compute range in the last layer. */
> uint64_t first;
> uint64_t last = start + count - 1;
> + uint64_t gran = 1ULL << hb->granularity;
> +
> + assert(!(start & (gran - 1)));
> + assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
I know I'm replying a bit late (since this is now a pull request), but
would it be worth using the dedicated macro:
assert(QEMU_IS_ALIGNED(start, gran));
assert(QEMU_IS_ALIGNED(count, gran) || start + count == hb->orig_size);
instead of open-coding it? (I would also drop the extra () around the
right half of ||). If we want it, that would now be a followup patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PULL 01/19] util/hbitmap: strict hbitmap_reset
2019-10-11 21:48 ` Eric Blake
@ 2019-10-11 23:18 ` John Snow
2019-10-14 18:10 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2019-10-11 23:18 UTC (permalink / raw)
To: Eric Blake, Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, Markus Armbruster,
Dr. David Alan Gilbert, Max Reitz, Stefan Hajnoczi
On 10/11/19 5:48 PM, Eric Blake wrote:
> On 10/11/19 4:25 PM, John Snow wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> hbitmap_reset has an unobvious property: it rounds requested region up.
>> It may provoke bugs, like in recently fixed write-blocking mode of
>> mirror: user calls reset on unaligned region, not keeping in mind that
>> there are possible unrelated dirty bytes, covered by rounded-up region
>> and information of this unrelated "dirtiness" will be lost.
>>
>> Make hbitmap_reset strict: assert that arguments are aligned, allowing
>> only one exception when @start + @count == hb->orig_size. It's needed
>> to comfort users of hbitmap_next_dirty_area, which cares about
>> hb->orig_size.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> Message-Id: <20190806152611.280389-1-vsementsov@virtuozzo.com>
>> [Maintainer edit: Max's suggestions from on-list. --js]
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> include/qemu/hbitmap.h | 5 +++++
>> tests/test-hbitmap.c | 2 +-
>> util/hbitmap.c | 4 ++++
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>
>> +++ b/util/hbitmap.c
>> @@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start,
>> uint64_t count)
>> /* Compute range in the last layer. */
>> uint64_t first;
>> uint64_t last = start + count - 1;
>> + uint64_t gran = 1ULL << hb->granularity;
>> +
>> + assert(!(start & (gran - 1)));
>> + assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
>
> I know I'm replying a bit late (since this is now a pull request), but
> would it be worth using the dedicated macro:
>
> assert(QEMU_IS_ALIGNED(start, gran));
> assert(QEMU_IS_ALIGNED(count, gran) || start + count == hb->orig_size);
>
> instead of open-coding it? (I would also drop the extra () around the
> right half of ||). If we want it, that would now be a followup patch.
>
If the PR doesn't make it for some reason, I can amend a cleanup patch
for the next PR.
--js
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PULL 00/19] Bitmaps patches
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
` (18 preceding siblings ...)
2019-10-11 21:25 ` [PULL 19/19] dirty-bitmaps: remove deprecated autoload parameter John Snow
@ 2019-10-14 16:11 ` Peter Maydell
19 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2019-10-14 16:11 UTC (permalink / raw)
To: John Snow
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, Qemu-block,
Juan Quintela, Libvirt, Markus Armbruster, QEMU Developers,
Max Reitz, Stefan Hajnoczi, Dr. David Alan Gilbert
On Fri, 11 Oct 2019 at 22:26, John Snow <jsnow@redhat.com> wrote:
>
> The following changes since commit 98b2e3c9ab3abfe476a2b02f8f51813edb90e72d:
>
> Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into staging (2019-10-08 16:08:35 +0100)
>
> are available in the Git repository at:
>
> https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to b97d9a1014b61dd0980e7f4a0c9ca1e3b0aaa761:
>
> dirty-bitmaps: remove deprecated autoload parameter (2019-10-09 17:02:45 -0400)
>
> ----------------------------------------------------------------
> Pull request
>
> ----------------------------------------------------------------
>
Hi -- this has a conflict in block/backup.c -- could you fix up
and resend, please ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PULL 01/19] util/hbitmap: strict hbitmap_reset
2019-10-11 23:18 ` John Snow
@ 2019-10-14 18:10 ` John Snow
2019-10-15 8:44 ` Kevin Wolf
0 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2019-10-14 18:10 UTC (permalink / raw)
To: Eric Blake, Peter Maydell, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-block,
Juan Quintela, libvir-list, Markus Armbruster,
Dr. David Alan Gilbert, Max Reitz, Stefan Hajnoczi
On 10/11/19 7:18 PM, John Snow wrote:
>
>
> On 10/11/19 5:48 PM, Eric Blake wrote:
>> On 10/11/19 4:25 PM, John Snow wrote:
>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> hbitmap_reset has an unobvious property: it rounds requested region up.
>>> It may provoke bugs, like in recently fixed write-blocking mode of
>>> mirror: user calls reset on unaligned region, not keeping in mind that
>>> there are possible unrelated dirty bytes, covered by rounded-up region
>>> and information of this unrelated "dirtiness" will be lost.
>>>
>>> Make hbitmap_reset strict: assert that arguments are aligned, allowing
>>> only one exception when @start + @count == hb->orig_size. It's needed
>>> to comfort users of hbitmap_next_dirty_area, which cares about
>>> hb->orig_size.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> Message-Id: <20190806152611.280389-1-vsementsov@virtuozzo.com>
>>> [Maintainer edit: Max's suggestions from on-list. --js]
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> include/qemu/hbitmap.h | 5 +++++
>>> tests/test-hbitmap.c | 2 +-
>>> util/hbitmap.c | 4 ++++
>>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>
>>> +++ b/util/hbitmap.c
>>> @@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start,
>>> uint64_t count)
>>> /* Compute range in the last layer. */
>>> uint64_t first;
>>> uint64_t last = start + count - 1;
>>> + uint64_t gran = 1ULL << hb->granularity;
>>> +
>>> + assert(!(start & (gran - 1)));
>>> + assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
>>
>> I know I'm replying a bit late (since this is now a pull request), but
>> would it be worth using the dedicated macro:
>>
>> assert(QEMU_IS_ALIGNED(start, gran));
>> assert(QEMU_IS_ALIGNED(count, gran) || start + count == hb->orig_size);
>>
>> instead of open-coding it? (I would also drop the extra () around the
>> right half of ||). If we want it, that would now be a followup patch.
I've noticed that seasoned C programmers hate extra parentheses a lot.
I've noticed that I cannot remember operator precedence enough to ever
feel like this is actually an improvement.
Something about a nice weighted tree of ((expr1) || (expr2)) feels
soothing to my weary eyes. So, if it's not terribly important, I'd
prefer to leave it as-is.
(You may feel free to counter-educate me as desired.)
>>
>
> If the PR doesn't make it for some reason, I can amend a cleanup patch
> for the next PR.
>
by the way: GOOD NEWS! ...
--js
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PULL 01/19] util/hbitmap: strict hbitmap_reset
2019-10-14 18:10 ` John Snow
@ 2019-10-15 8:44 ` Kevin Wolf
2019-10-15 12:55 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2019-10-15 8:44 UTC (permalink / raw)
To: John Snow
Cc: Fam Zheng, Peter Maydell, Vladimir Sementsov-Ogievskiy,
qemu-block, Juan Quintela, libvir-list, Markus Armbruster,
qemu-devel, Max Reitz, Stefan Hajnoczi, Dr. David Alan Gilbert
Am 14.10.2019 um 20:10 hat John Snow geschrieben:
>
>
> On 10/11/19 7:18 PM, John Snow wrote:
> >
> >
> > On 10/11/19 5:48 PM, Eric Blake wrote:
> >> On 10/11/19 4:25 PM, John Snow wrote:
> >>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>
> >>> hbitmap_reset has an unobvious property: it rounds requested region up.
> >>> It may provoke bugs, like in recently fixed write-blocking mode of
> >>> mirror: user calls reset on unaligned region, not keeping in mind that
> >>> there are possible unrelated dirty bytes, covered by rounded-up region
> >>> and information of this unrelated "dirtiness" will be lost.
> >>>
> >>> Make hbitmap_reset strict: assert that arguments are aligned, allowing
> >>> only one exception when @start + @count == hb->orig_size. It's needed
> >>> to comfort users of hbitmap_next_dirty_area, which cares about
> >>> hb->orig_size.
> >>>
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>> Message-Id: <20190806152611.280389-1-vsementsov@virtuozzo.com>
> >>> [Maintainer edit: Max's suggestions from on-list. --js]
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>> ---
> >>> include/qemu/hbitmap.h | 5 +++++
> >>> tests/test-hbitmap.c | 2 +-
> >>> util/hbitmap.c | 4 ++++
> >>> 3 files changed, 10 insertions(+), 1 deletion(-)
> >>>
> >>
> >>> +++ b/util/hbitmap.c
> >>> @@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start,
> >>> uint64_t count)
> >>> /* Compute range in the last layer. */
> >>> uint64_t first;
> >>> uint64_t last = start + count - 1;
> >>> + uint64_t gran = 1ULL << hb->granularity;
> >>> +
> >>> + assert(!(start & (gran - 1)));
> >>> + assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
> >>
> >> I know I'm replying a bit late (since this is now a pull request), but
> >> would it be worth using the dedicated macro:
> >>
> >> assert(QEMU_IS_ALIGNED(start, gran));
> >> assert(QEMU_IS_ALIGNED(count, gran) || start + count == hb->orig_size);
> >>
> >> instead of open-coding it? (I would also drop the extra () around the
> >> right half of ||). If we want it, that would now be a followup patch.
>
> I've noticed that seasoned C programmers hate extra parentheses a lot.
> I've noticed that I cannot remember operator precedence enough to ever
> feel like this is actually an improvement.
>
> Something about a nice weighted tree of ((expr1) || (expr2)) feels
> soothing to my weary eyes. So, if it's not terribly important, I'd
> prefer to leave it as-is.
I don't mind the parentheses, but I do prefer QEMU_IS_ALIGNED() to the
open-coded version. Would that be a viable compromise?
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PULL 01/19] util/hbitmap: strict hbitmap_reset
2019-10-15 8:44 ` Kevin Wolf
@ 2019-10-15 12:55 ` John Snow
0 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2019-10-15 12:55 UTC (permalink / raw)
To: Kevin Wolf
Cc: Fam Zheng, Peter Maydell, Vladimir Sementsov-Ogievskiy,
qemu-block, Juan Quintela, libvir-list, Markus Armbruster,
qemu-devel, Max Reitz, Stefan Hajnoczi, Dr. David Alan Gilbert
On 10/15/19 4:44 AM, Kevin Wolf wrote:
> Am 14.10.2019 um 20:10 hat John Snow geschrieben:
>>
>>
>> On 10/11/19 7:18 PM, John Snow wrote:
>>>
>>>
>>> On 10/11/19 5:48 PM, Eric Blake wrote:
>>>> On 10/11/19 4:25 PM, John Snow wrote:
>>>>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>
>>>>> hbitmap_reset has an unobvious property: it rounds requested region up.
>>>>> It may provoke bugs, like in recently fixed write-blocking mode of
>>>>> mirror: user calls reset on unaligned region, not keeping in mind that
>>>>> there are possible unrelated dirty bytes, covered by rounded-up region
>>>>> and information of this unrelated "dirtiness" will be lost.
>>>>>
>>>>> Make hbitmap_reset strict: assert that arguments are aligned, allowing
>>>>> only one exception when @start + @count == hb->orig_size. It's needed
>>>>> to comfort users of hbitmap_next_dirty_area, which cares about
>>>>> hb->orig_size.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>> Message-Id: <20190806152611.280389-1-vsementsov@virtuozzo.com>
>>>>> [Maintainer edit: Max's suggestions from on-list. --js]
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> include/qemu/hbitmap.h | 5 +++++
>>>>> tests/test-hbitmap.c | 2 +-
>>>>> util/hbitmap.c | 4 ++++
>>>>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>>> +++ b/util/hbitmap.c
>>>>> @@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start,
>>>>> uint64_t count)
>>>>> /* Compute range in the last layer. */
>>>>> uint64_t first;
>>>>> uint64_t last = start + count - 1;
>>>>> + uint64_t gran = 1ULL << hb->granularity;
>>>>> +
>>>>> + assert(!(start & (gran - 1)));
>>>>> + assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
>>>>
>>>> I know I'm replying a bit late (since this is now a pull request), but
>>>> would it be worth using the dedicated macro:
>>>>
>>>> assert(QEMU_IS_ALIGNED(start, gran));
>>>> assert(QEMU_IS_ALIGNED(count, gran) || start + count == hb->orig_size);
>>>>
>>>> instead of open-coding it? (I would also drop the extra () around the
>>>> right half of ||). If we want it, that would now be a followup patch.
>>
>> I've noticed that seasoned C programmers hate extra parentheses a lot.
>> I've noticed that I cannot remember operator precedence enough to ever
>> feel like this is actually an improvement.
>>
>> Something about a nice weighted tree of ((expr1) || (expr2)) feels
>> soothing to my weary eyes. So, if it's not terribly important, I'd
>> prefer to leave it as-is.
>
> I don't mind the parentheses, but I do prefer QEMU_IS_ALIGNED() to the
> open-coded version. Would that be a viable compromise?
>
Oh, I'm sorry! I did change that. I didn't mean to appear any more
stubborn than I actually am.
--js
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-10-15 12:56 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11 21:25 [PULL 00/19] Bitmaps patches John Snow
2019-10-11 21:25 ` [PULL 01/19] util/hbitmap: strict hbitmap_reset John Snow
2019-10-11 21:48 ` Eric Blake
2019-10-11 23:18 ` John Snow
2019-10-14 18:10 ` John Snow
2019-10-15 8:44 ` Kevin Wolf
2019-10-15 12:55 ` John Snow
2019-10-11 21:25 ` [PULL 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c John Snow
2019-10-11 21:25 ` [PULL 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap John Snow
2019-10-11 21:25 ` [PULL 04/19] block/qcow2: proper locking on bitmap add/remove paths John Snow
2019-10-11 21:25 ` [PULL 05/19] block/dirty-bitmap: drop meta John Snow
2019-10-11 21:25 ` [PULL 06/19] block/dirty-bitmap: add bs link John Snow
2019-10-11 21:25 ` [PULL 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex John Snow
2019-10-11 21:25 ` [PULL 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next John Snow
2019-10-11 21:25 ` [PULL 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ John Snow
2019-10-11 21:25 ` [PULL 10/19] block: reverse order for reopen commits John Snow
2019-10-11 21:25 ` [PULL 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW John Snow
2019-10-11 21:25 ` [PULL 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps John Snow
2019-10-11 21:25 ` [PULL 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint() John Snow
2019-10-11 21:25 ` [PULL 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro John Snow
2019-10-11 21:25 ` [PULL 15/19] iotests: add test 260 to check bitmap life after snapshot + commit John Snow
2019-10-11 21:25 ` [PULL 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw John Snow
2019-10-11 21:25 ` [PULL 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit John Snow
2019-10-11 21:25 ` [PULL 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps John Snow
2019-10-11 21:25 ` [PULL 19/19] dirty-bitmaps: remove deprecated autoload parameter John Snow
2019-10-14 16:11 ` [PULL 00/19] Bitmaps patches Peter Maydell
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.