* [PULL v2 01/19] util/hbitmap: strict hbitmap_reset
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c John Snow
` (18 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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]
[Maintainer edit: Eric's suggestion for aligned macro. --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 4afbe6292e..1bf944ca3d 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 eed5d288cb..e1f867085f 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 fd44c897ab..66db87c6ff 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(QEMU_IS_ALIGNED(start, gran));
+ assert(QEMU_IS_ALIGNED(count, gran) || (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] 22+ messages in thread* [PULL v2 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
2019-10-14 19:28 ` [PULL v2 01/19] util/hbitmap: strict hbitmap_reset John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap John Snow
` (17 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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 1946fc6f57..d19a4781a3 100644
--- a/block.c
+++ b/block.c
@@ -6582,25 +6582,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 134e0c9a0c..8f42015db9 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] 22+ messages in thread* [PULL v2 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
2019-10-14 19:28 ` [PULL v2 01/19] util/hbitmap: strict hbitmap_reset John Snow
2019-10-14 19:28 ` [PULL v2 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 04/19] block/qcow2: proper locking on bitmap add/remove paths John Snow
` (16 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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 f51f478e34..08b4c15dc4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -750,9 +750,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 05056b308a..6b511dd889 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -557,9 +557,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 4b4b731b46..07503b03b5 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 8f42015db9..d1ae2e1922 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 b2487101ed..9821c1628f 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 f89e48fc79..a45458a60a 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] 22+ messages in thread* [PULL v2 04/19] block/qcow2: proper locking on bitmap add/remove paths
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (2 preceding siblings ...)
2019-10-14 19:28 ` [PULL v2 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 05/19] block/dirty-bitmap: drop meta John Snow
` (15 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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 08b4c15dc4..0f3d9b088e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -746,12 +746,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 6b511dd889..32fb493cbb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -553,13 +553,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 d1ae2e1922..03e0872b97 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 9821c1628f..644837eb03 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 7961c05783..7062eccaee 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5407,8 +5407,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 a45458a60a..7e12919724 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] 22+ messages in thread* [PULL v2 05/19] block/dirty-bitmap: drop meta
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (3 preceding siblings ...)
2019-10-14 19:28 ` [PULL v2 04/19] block/qcow2: proper locking on bitmap add/remove paths John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 06/19] block/dirty-bitmap: add bs link John Snow
` (14 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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 07503b03b5..973056778a 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 03e0872b97..4ecf18d5df 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] 22+ messages in thread* [PULL v2 06/19] block/dirty-bitmap: add bs link
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (4 preceding siblings ...)
2019-10-14 19:28 ` [PULL v2 05/19] block/dirty-bitmap: drop meta John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex John Snow
` (13 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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
[Rebased on top of block-copy. --js]
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/dirty-bitmap.h | 14 +++++---------
block/backup.c | 8 ++++----
block/block-copy.c | 2 +-
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 ++--
9 files changed, 35 insertions(+), 40 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 973056778a..2f9b088e11 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 46978c1785..dddcf77f53 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -98,13 +98,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(job->source_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(job->source_bs, job->sync_bitmap, NULL);
+ bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
}
assert(bm);
@@ -402,7 +402,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;
}
}
@@ -472,7 +472,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
error:
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/block-copy.c b/block/block-copy.c
index 0f76ea1e63..066e3a7274 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -60,7 +60,7 @@ void block_copy_state_free(BlockCopyState *s)
return;
}
- bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
+ bdrv_release_dirty_bitmap(s->copy_bitmap);
g_free(s);
}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4ecf18d5df..44453ff824 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 fe984efb90..a6c50caea4 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 644837eb03..687087d2bc 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 7e12919724..d77e809623 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 5121f86d73..793f249aa5 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 8e49382070..c90288ed29 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] 22+ messages in thread* [PULL v2 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (5 preceding siblings ...)
2019-10-14 19:28 ` [PULL v2 06/19] block/dirty-bitmap: add bs link John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next John Snow
` (12 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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 44453ff824..4e5c87a907 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] 22+ messages in thread* [PULL v2 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (6 preceding siblings ...)
2019-10-14 19:28 ` [PULL v2 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:28 ` [PULL v2 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ John Snow
` (11 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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 2f9b088e11..257f0f6704 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 d19a4781a3..5721441697 100644
--- a/block.c
+++ b/block.c
@@ -5390,9 +5390,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 4e5c87a907..6065db8094 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 687087d2bc..99812b418b 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 793f249aa5..7eafface61 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] 22+ messages in thread* [PULL v2 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (7 preceding siblings ...)
2019-10-14 19:28 ` [PULL v2 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next John Snow
@ 2019-10-14 19:28 ` John Snow
2019-10-14 19:29 ` [PULL v2 10/19] block: reverse order for reopen commits John Snow
` (10 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:28 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 792bb826db..89606bd9f8 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 5721441697..0347632c6c 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] 22+ messages in thread* [PULL v2 10/19] block: reverse order for reopen commits
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (8 preceding siblings ...)
2019-10-14 19:28 ` [PULL v2 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW John Snow
` (9 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 0347632c6c..cf312258a9 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] 22+ messages in thread* [PULL v2 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (9 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 10/19] block: reverse order for reopen commits John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps John Snow
` (8 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 5650dc7c87..951ea011a2 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 ae1213e6f8..fbc63e62f8 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] 22+ messages in thread* [PULL v2 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (10 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint() John Snow
` (7 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 257f0f6704..958e7474fb 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 6065db8094..4bbb251b2c 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 99812b418b..6dfc083548 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] 22+ messages in thread* [PULL v2 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (11 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro John Snow
` (6 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 0f3d9b088e..23a9898a54 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -740,8 +740,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 6dfc083548..ebc1afccd3 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] 22+ messages in thread* [PULL v2 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (12 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint() John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 15/19] iotests: add test 260 to check bitmap life after snapshot + commit John Snow
` (5 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 23a9898a54..5cccd87162 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -742,7 +742,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 ebc1afccd3..f7dfb40256 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 7062eccaee..53a025703e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2503,7 +2503,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] 22+ messages in thread* [PULL v2 15/19] iotests: add test 260 to check bitmap life after snapshot + commit
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (13 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw John Snow
` (4 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 0000000000..4f6082c9d2
--- /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 0000000000..2f0d98d036
--- /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 5805a79d9e..0c1e5ef414 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] 22+ messages in thread* [PULL v2 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (14 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 15/19] iotests: add test 260 to check bitmap life after snapshot + commit John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit John Snow
` (3 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 f7dfb40256..98294a7696 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] 22+ messages in thread* [PULL v2 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (15 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps John Snow
` (2 subsequent siblings)
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 32fb493cbb..ca4ccac4c1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -547,12 +547,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 cf312258a9..dad5a3d8e0 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 53a025703e..8b05933565 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1835,6 +1835,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);
}
@@ -5406,7 +5420,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] 22+ messages in thread* [PULL v2 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (16 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-14 19:29 ` [PULL v2 19/19] dirty-bitmaps: remove deprecated autoload parameter John Snow
2019-10-17 11:07 ` [PULL v2 00/19] Bitmaps patches Peter Maydell
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 fe4dc51b08..250ce8e7a1 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] 22+ messages in thread* [PULL v2 19/19] dirty-bitmaps: remove deprecated autoload parameter
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (17 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps John Snow
@ 2019-10-14 19:29 ` John Snow
2019-10-17 11:07 ` [PULL v2 00/19] Bitmaps patches Peter Maydell
19 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-14 19:29 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 01245e0b1c..7239e0959d 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 f66553aac7..b274aef713 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2052,10 +2052,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)
@@ -2064,7 +2060,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 d77e809623..03c7cd7651 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] 22+ messages in thread* Re: [PULL v2 00/19] Bitmaps patches
2019-10-14 19:28 [PULL v2 00/19] Bitmaps patches John Snow
` (18 preceding siblings ...)
2019-10-14 19:29 ` [PULL v2 19/19] dirty-bitmaps: remove deprecated autoload parameter John Snow
@ 2019-10-17 11:07 ` Peter Maydell
2019-10-17 19:34 ` John Snow
19 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2019-10-17 11:07 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 Mon, 14 Oct 2019 at 20:29, John Snow <jsnow@redhat.com> wrote:
>
> The following changes since commit c760cb77e511eb05094df67c1b30029a952efa35:
>
> Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20191011a' into staging (2019-10-14 16:09:52 +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 b2ca29ee390743c42a6062d44ee3b10fb51f9fa6:
>
> dirty-bitmaps: remove deprecated autoload parameter (2019-10-14 15:28:17 -0400)
>
> ----------------------------------------------------------------
> Pull request
>
> ----------------------------------------------------------------
Hi; this pullreq fails on some hosts on the newly added iotest 260
with an "AF_UNIX path too long" error:
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg04063.html
Max tells me that this is a known problem (a fix is in the works)
but that for the moment we've chosen not to add any python based
tests to the 'auto' group, so that this AF_UNIX issue doesn't
affect "make check". Could you respin the pullreq with the
new iotest(s?) not in the 'auto' group, please?
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PULL v2 00/19] Bitmaps patches
2019-10-17 11:07 ` [PULL v2 00/19] Bitmaps patches Peter Maydell
@ 2019-10-17 19:34 ` John Snow
0 siblings, 0 replies; 22+ messages in thread
From: John Snow @ 2019-10-17 19:34 UTC (permalink / raw)
To: Peter Maydell
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 10/17/19 7:07 AM, Peter Maydell wrote:
> On Mon, 14 Oct 2019 at 20:29, John Snow <jsnow@redhat.com> wrote:
>>
>> The following changes since commit c760cb77e511eb05094df67c1b30029a952efa35:
>>
>> Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20191011a' into staging (2019-10-14 16:09:52 +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 b2ca29ee390743c42a6062d44ee3b10fb51f9fa6:
>>
>> dirty-bitmaps: remove deprecated autoload parameter (2019-10-14 15:28:17 -0400)
>>
>> ----------------------------------------------------------------
>> Pull request
>>
>> ----------------------------------------------------------------
>
> Hi; this pullreq fails on some hosts on the newly added iotest 260
> with an "AF_UNIX path too long" error:
> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg04063.html
>
> Max tells me that this is a known problem (a fix is in the works)
> but that for the moment we've chosen not to add any python based
> tests to the 'auto' group, so that this AF_UNIX issue doesn't
> affect "make check". Could you respin the pullreq with the
> new iotest(s?) not in the 'auto' group, please?
>
> thanks
> -- PMM
>
Anguish and turmoil.
Alright, V3 coming up.
^ permalink raw reply [flat|nested] 22+ messages in thread