* [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: allow bitmap_list_load to return an error code
2019-06-06 18:41 [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps John Snow
@ 2019-06-06 18:41 ` John Snow
2019-06-07 2:07 ` Eric Blake
2019-06-07 12:36 ` Vladimir Sementsov-Ogievskiy
2019-06-06 18:41 ` [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap John Snow
` (5 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: John Snow @ 2019-06-06 18:41 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz,
John Snow
This simply makes this function a little more convenient to call, and in
a forthcoming patch gives us a return code we can report to the
caller. (Which in turn makes THOSE functions easier to call.)
While we're here, remove the offset+size arguments which are only ever
called with the same values that can be determined inside this helper.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 64 +++++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 36 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..83aee1a42a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -547,30 +547,33 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
* Get bitmap list from qcow2 image. Actually reads bitmap directory,
* checks it and convert to bitmap list.
*/
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
- uint64_t size, Error **errp)
+static int bitmap_list_load(BlockDriverState *bs, Qcow2BitmapList **bm_list_out,
+ Error **errp)
{
- int ret;
+ int ret = 0;
BDRVQcow2State *s = bs->opaque;
uint8_t *dir, *dir_end;
Qcow2BitmapDirEntry *e;
uint32_t nb_dir_entries = 0;
Qcow2BitmapList *bm_list = NULL;
+ uint64_t offset = s->bitmap_directory_offset;
+ uint64_t size = s->bitmap_directory_size;
+
if (size == 0) {
- error_setg(errp, "Requested bitmap directory size is zero");
- return NULL;
+ error_setg(errp, "Cannot load a bitmap directory of size 0");
+ return -EINVAL;
}
if (size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
error_setg(errp, "Requested bitmap directory size is too big");
- return NULL;
+ return -EINVAL;
}
dir = g_try_malloc(size);
if (dir == NULL) {
error_setg(errp, "Failed to allocate space for bitmap directory");
- return NULL;
+ return -ENOMEM;
}
dir_end = dir + size;
@@ -594,6 +597,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
if (++nb_dir_entries > s->nb_bitmaps) {
error_setg(errp, "More bitmaps found than specified in header"
" extension");
+ ret = -EINVAL;
goto fail;
}
bitmap_dir_entry_to_cpu(e);
@@ -604,6 +608,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
if (e->extra_data_size != 0) {
error_setg(errp, "Bitmap extra data is not supported");
+ ret = -ENOTSUP;
goto fail;
}
@@ -626,6 +631,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
if (nb_dir_entries != s->nb_bitmaps) {
error_setg(errp, "Less bitmaps found than specified in header"
" extension");
+ ret = -EINVAL;
goto fail;
}
@@ -634,7 +640,8 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
}
g_free(dir);
- return bm_list;
+ *bm_list_out = bm_list;
+ return ret;
broken_dir:
ret = -EINVAL;
@@ -644,7 +651,7 @@ fail:
g_free(dir);
bitmap_list_free(bm_list);
- return NULL;
+ return ret;
}
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
@@ -667,11 +674,10 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
return ret;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, NULL);
- if (bm_list == NULL) {
+ ret = bitmap_list_load(bs, &bm_list, NULL);
+ if (ret) {
res->corruptions++;
- return -EINVAL;
+ return ret;
}
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -971,9 +977,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
return false;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
+ if (bitmap_list_load(bs, &bm_list, errp)) {
return false;
}
@@ -1080,9 +1084,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
return NULL;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
+ if (bitmap_list_load(bs, &bm_list, errp)) {
return NULL;
}
@@ -1125,10 +1127,8 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
return -EINVAL;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
- return -EINVAL;
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
+ return ret;
}
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1186,10 +1186,8 @@ int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
return 0;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
- return -EINVAL;
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
+ return ret;
}
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1419,9 +1417,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
return;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
+ if (bitmap_list_load(bs, &bm_list, errp)) {
return;
}
@@ -1472,9 +1468,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
if (s->nb_bitmaps == 0) {
bm_list = bitmap_list_new();
} else {
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
+ if (bitmap_list_load(bs, &bm_list, errp)) {
return;
}
}
@@ -1653,9 +1647,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
goto fail;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
+ if (bitmap_list_load(bs, &bm_list, errp)) {
goto fail;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: allow bitmap_list_load to return an error code
2019-06-06 18:41 ` [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: allow bitmap_list_load to return an error code John Snow
@ 2019-06-07 2:07 ` Eric Blake
2019-06-07 12:36 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 30+ messages in thread
From: Eric Blake @ 2019-06-07 2:07 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz
[-- Attachment #1.1: Type: text/plain, Size: 767 bytes --]
On 6/6/19 1:41 PM, John Snow wrote:
> This simply makes this function a little more convenient to call, and in
> a forthcoming patch gives us a return code we can report to the
> caller. (Which in turn makes THOSE functions easier to call.)
>
> While we're here, remove the offset+size arguments which are only ever
> called with the same values that can be determined inside this helper.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2-bitmap.c | 64 +++++++++++++++++++-------------------------
> 1 file changed, 28 insertions(+), 36 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: allow bitmap_list_load to return an error code
2019-06-06 18:41 ` [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: allow bitmap_list_load to return an error code John Snow
2019-06-07 2:07 ` Eric Blake
@ 2019-06-07 12:36 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-07 12:36 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
06.06.2019 21:41, John Snow wrote:
> This simply makes this function a little more convenient to call, and in
> a forthcoming patch gives us a return code we can report to the
> caller. (Which in turn makes THOSE functions easier to call.)
>
> While we're here, remove the offset+size arguments which are only ever
> called with the same values that can be determined inside this helper.
>
> Signed-off-by: John Snow<jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-06 18:41 [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps John Snow
2019-06-06 18:41 ` [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: allow bitmap_list_load to return an error code John Snow
@ 2019-06-06 18:41 ` John Snow
2019-06-07 2:16 ` Eric Blake
2019-06-07 14:29 ` Vladimir Sementsov-Ogievskiy
2019-06-06 18:41 ` [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap John Snow
` (4 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: John Snow @ 2019-06-06 18:41 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz,
John Snow
Instead of bdrv_can_store_new_bitmap, rework this as
bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
to modify the driver state because we know we ARE adding a bitmap
instead of simply being asked if we CAN store one.
As part of this symmetry, move this function next to
bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
To cement this semantic distinction, use a bitmap object instead of the
(name, granularity) pair as this allows us to set persistence as a
condition of the "add" succeeding.
Notably, the qcow2 implementation still does not actually store the new
bitmap at add time, but merely ensures that it will be able to at store
time.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 5 ++---
include/block/block.h | 2 --
include/block/block_int.h | 5 ++---
include/block/dirty-bitmap.h | 3 +++
block.c | 22 ---------------------
block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++
block/qcow2-bitmap.c | 24 ++++++++++++++---------
block/qcow2.c | 2 +-
blockdev.c | 19 +++++++-----------
9 files changed, 68 insertions(+), 52 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index fc1b0d3c1e..95d723d3c0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -742,9 +742,8 @@ 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,
+int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
Error **errp);
void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
diff --git a/include/block/block.h b/include/block/block.h
index f9415ed740..6d520f3b59 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
Error **errp);
void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp);
/**
*
* bdrv_register_buf/bdrv_unregister_buf:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06df2bda1b..93bbb66cd0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -537,9 +537,8 @@ 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,
+ int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
Error **errp);
void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
const char *name,
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8044ace63e..c37edbe05f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -38,6 +38,9 @@ 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);
+int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);
diff --git a/block.c b/block.c
index e3e77feee0..6aec36b7c9 100644
--- a/block.c
+++ b/block.c
@@ -6379,25 +6379,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 49646a30e6..615f8480b2 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
}
}
+int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp)
+{
+ BlockDriver *drv = bs->drv;
+ int ret;
+
+ if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+ error_setg(errp, "Bitmap '%s' is already persistent, "
+ "and cannot be re-added to node '%s'",
+ bdrv_dirty_bitmap_name(bitmap),
+ bdrv_get_device_or_node_name(bs));
+ return -EINVAL;
+ }
+
+ if (!drv) {
+ error_setg_errno(errp, ENOMEDIUM,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+ return -ENOMEDIUM;
+ }
+
+ if (!drv->bdrv_add_persistent_dirty_bitmap) {
+ error_setg_errno(errp, ENOTSUP,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+ return -ENOTSUP;
+ }
+
+ ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
+ if (ret == 0) {
+ bdrv_dirty_bitmap_set_persistence(bitmap, true);
+ }
+
+ return ret;
+}
+
+
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
bdrv_dirty_bitmap_lock(bitmap);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 83aee1a42a..c3a72ca782 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1607,14 +1607,16 @@ 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,
+int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
Error **errp)
{
BDRVQcow2State *s = bs->opaque;
bool found;
Qcow2BitmapList *bm_list;
+ const char *name = bdrv_dirty_bitmap_name(bitmap);
+ uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+ int ret = 0;
if (s->qcow_version < 3) {
/* Without autoclear_features, we would always have to assume
@@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
* have to drop all dirty bitmaps (defeating their purpose).
*/
error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
+ ret = -ENOTSUP;
goto fail;
}
- if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
+ ret = check_constraints_on_bitmap(bs, name, granularity, errp);
+ if (ret) {
goto fail;
}
if (s->nb_bitmaps == 0) {
- return true;
+ return 0;
}
if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
error_setg(errp,
"Maximum number of persistent bitmaps is already reached");
+ ret = -EOVERFLOW;
goto fail;
}
@@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
{
error_setg(errp, "Not enough space in the bitmap directory");
+ ret = -EOVERFLOW;
goto fail;
}
- if (bitmap_list_load(bs, &bm_list, errp)) {
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
goto fail;
}
found = find_bitmap_by_name(bm_list, name);
bitmap_list_free(bm_list);
if (found) {
+ ret = -EEXIST;
error_setg(errp, "Bitmap with the same name is already stored");
goto fail;
}
- return true;
-
+ return 0;
fail:
error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
name, bdrv_get_device_or_node_name(bs));
- return false;
+ return ret;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 9396d490d5..1c78eba71b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5229,7 +5229,7 @@ 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_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
.bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
};
diff --git a/blockdev.c b/blockdev.c
index 3f44b891eb..169a8da831 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
disabled = false;
}
- if (persistent) {
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
- if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
- goto out;
- }
- }
-
bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
if (bitmap == NULL) {
- goto out;
+ return;
}
if (disabled) {
bdrv_disable_dirty_bitmap(bitmap);
}
- bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
- out:
- if (aio_context) {
+ if (persistent) {
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+ if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
+ bdrv_release_dirty_bitmap(bs, bitmap);
+ }
aio_context_release(aio_context);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-06 18:41 ` [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap John Snow
@ 2019-06-07 2:16 ` Eric Blake
2019-06-07 14:29 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 30+ messages in thread
From: Eric Blake @ 2019-06-07 2:16 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz
[-- Attachment #1.1: Type: text/plain, Size: 1612 bytes --]
On 6/6/19 1:41 PM, John Snow wrote:
> Instead of bdrv_can_store_new_bitmap, rework this as
> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
> to modify the driver state because we know we ARE adding a bitmap
> instead of simply being asked if we CAN store one.
>
> As part of this symmetry, move this function next to
> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>
> To cement this semantic distinction, use a bitmap object instead of the
> (name, granularity) pair as this allows us to set persistence as a
> condition of the "add" succeeding.
>
> Notably, the qcow2 implementation still does not actually store the new
> bitmap at add time, but merely ensures that it will be able to at store
> time.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> +++ b/include/block/dirty-bitmap.h
> @@ -38,6 +38,9 @@ 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);
> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp);
Indentation looks off.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-06 18:41 ` [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap John Snow
2019-06-07 2:16 ` Eric Blake
@ 2019-06-07 14:29 ` Vladimir Sementsov-Ogievskiy
2019-06-07 18:10 ` John Snow
1 sibling, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-07 14:29 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
06.06.2019 21:41, John Snow wrote:
> Instead of bdrv_can_store_new_bitmap, rework this as
> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
> to modify the driver state because we know we ARE adding a bitmap
> instead of simply being asked if we CAN store one.
>
> As part of this symmetry, move this function next to
> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>
> To cement this semantic distinction, use a bitmap object instead of the
> (name, granularity) pair as this allows us to set persistence as a
> condition of the "add" succeeding.
>
> Notably, the qcow2 implementation still does not actually store the new
> bitmap at add time, but merely ensures that it will be able to at store
> time.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2.h | 5 ++---
> include/block/block.h | 2 --
> include/block/block_int.h | 5 ++---
> include/block/dirty-bitmap.h | 3 +++
> block.c | 22 ---------------------
> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++
> block/qcow2-bitmap.c | 24 ++++++++++++++---------
> block/qcow2.c | 2 +-
> blockdev.c | 19 +++++++-----------
> 9 files changed, 68 insertions(+), 52 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index fc1b0d3c1e..95d723d3c0 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -742,9 +742,8 @@ 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,
> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> Error **errp);
> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> const char *name,
> diff --git a/include/block/block.h b/include/block/block.h
> index f9415ed740..6d520f3b59 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
> Error **errp);
> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>
> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
> - uint32_t granularity, Error **errp);
> /**
> *
> * bdrv_register_buf/bdrv_unregister_buf:
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 06df2bda1b..93bbb66cd0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -537,9 +537,8 @@ 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,
> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> Error **errp);
> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> const char *name,
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8044ace63e..c37edbe05f 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -38,6 +38,9 @@ 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);
> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp);
> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> const char *name,
> Error **errp);
> diff --git a/block.c b/block.c
> index e3e77feee0..6aec36b7c9 100644
> --- a/block.c
> +++ b/block.c
> @@ -6379,25 +6379,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 49646a30e6..615f8480b2 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> }
> }
>
> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp)
strange thing for me: "bitmap" in function name is _not_ the same
thing as @bitmap argument.. as if it the same,
function adds "persistent bitmap", but @bitmap is not persistent yet,
so may be function, don't add persistent bitmap, but marks bitmap persistent?
Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
bitmap.persistent flag"
> +{
> + BlockDriver *drv = bs->drv;
> + int ret;
> +
> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
> + error_setg(errp, "Bitmap '%s' is already persistent, "
> + "and cannot be re-added to node '%s'",
> + bdrv_dirty_bitmap_name(bitmap),
> + bdrv_get_device_or_node_name(bs));
> + return -EINVAL;
> + }
> +
> + if (!drv) {
> + error_setg_errno(errp, ENOMEDIUM,
> + "Can't store persistent bitmaps to %s",
> + bdrv_get_device_or_node_name(bs));
> + return -ENOMEDIUM;
> + }
> +
> + if (!drv->bdrv_add_persistent_dirty_bitmap) {
> + error_setg_errno(errp, ENOTSUP,
> + "Can't store persistent bitmaps to %s",
> + bdrv_get_device_or_node_name(bs));
> + return -ENOTSUP;
> + }
> +
> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
> + if (ret == 0) {
> + bdrv_dirty_bitmap_set_persistence(bitmap, true);
> + }
> +
> + return ret;
> +}
> +
> +
> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> {
> bdrv_dirty_bitmap_lock(bitmap);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 83aee1a42a..c3a72ca782 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1607,14 +1607,16 @@ 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,
> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> Error **errp)
> {
> BDRVQcow2State *s = bs->opaque;
> bool found;
> Qcow2BitmapList *bm_list;
> + const char *name = bdrv_dirty_bitmap_name(bitmap);
> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> + int ret = 0;
dead assignment
>
> if (s->qcow_version < 3) {
> /* Without autoclear_features, we would always have to assume
> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
> * have to drop all dirty bitmaps (defeating their purpose).
> */
> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
> + ret = -ENOTSUP;
> goto fail;
> }
>
> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
> + ret = check_constraints_on_bitmap(bs, name, granularity, errp);
> + if (ret) {
hmm, in other places you prefere
if ((ret = ...)) {
syntax
> goto fail;
> }
>
> if (s->nb_bitmaps == 0) {
> - return true;
> + return 0;
> }
>
> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
> error_setg(errp,
> "Maximum number of persistent bitmaps is already reached");
> + ret = -EOVERFLOW;
> goto fail;
> }
>
> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
> QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
> {
> error_setg(errp, "Not enough space in the bitmap directory");
> + ret = -EOVERFLOW;
> goto fail;
> }
>
> - if (bitmap_list_load(bs, &bm_list, errp)) {
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
interesting, now we load all bitmaps, so, may be, we don't need to load list every time,
but may use bs.dirty_bitmaps to check such things.. But it seems unsafe
> goto fail;
> }
>
> found = find_bitmap_by_name(bm_list, name);
> bitmap_list_free(bm_list);
> if (found) {
> + ret = -EEXIST;
> error_setg(errp, "Bitmap with the same name is already stored");
> goto fail;
> }
>
> - return true;
> -
> + return 0;
> fail:
> error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
> name, bdrv_get_device_or_node_name(bs));
didn't you consider to move this error_prepend to caller and drop all these gotos?
> - return false;
> + return ret;
> }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 9396d490d5..1c78eba71b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5229,7 +5229,7 @@ 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_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
> .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
> };
>
> diff --git a/blockdev.c b/blockdev.c
> index 3f44b891eb..169a8da831 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
> disabled = false;
> }
>
> - if (persistent) {
> - aio_context = bdrv_get_aio_context(bs);
> - aio_context_acquire(aio_context);
> - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
> - goto out;
> - }
> - }
> -
> bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> if (bitmap == NULL) {
> - goto out;
> + return;
> }
>
> if (disabled) {
> bdrv_disable_dirty_bitmap(bitmap);
> }
>
> - bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
> - out:
> - if (aio_context) {
> + if (persistent) {
Good thing. I suggest to define aio_context in this block too.
> + aio_context = bdrv_get_aio_context(bs);
> + aio_context_acquire(aio_context);
> + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
> + bdrv_release_dirty_bitmap(bs, bitmap);
> + }
> aio_context_release(aio_context);
> }
> }
>
With some my suggestions or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-07 14:29 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-07 18:10 ` John Snow
2019-06-07 18:15 ` Eric Blake
2019-06-07 18:17 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 30+ messages in thread
From: John Snow @ 2019-06-07 18:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.06.2019 21:41, John Snow wrote:
>> Instead of bdrv_can_store_new_bitmap, rework this as
>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>> to modify the driver state because we know we ARE adding a bitmap
>> instead of simply being asked if we CAN store one.
>>
>> As part of this symmetry, move this function next to
>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>
>> To cement this semantic distinction, use a bitmap object instead of the
>> (name, granularity) pair as this allows us to set persistence as a
>> condition of the "add" succeeding.
>>
>> Notably, the qcow2 implementation still does not actually store the new
>> bitmap at add time, but merely ensures that it will be able to at store
>> time.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2.h | 5 ++---
>> include/block/block.h | 2 --
>> include/block/block_int.h | 5 ++---
>> include/block/dirty-bitmap.h | 3 +++
>> block.c | 22 ---------------------
>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++
>> block/qcow2-bitmap.c | 24 ++++++++++++++---------
>> block/qcow2.c | 2 +-
>> blockdev.c | 19 +++++++-----------
>> 9 files changed, 68 insertions(+), 52 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index fc1b0d3c1e..95d723d3c0 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -742,9 +742,8 @@ 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,
>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> Error **errp);
>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> const char *name,
>> diff --git a/include/block/block.h b/include/block/block.h
>> index f9415ed740..6d520f3b59 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>> Error **errp);
>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>>
>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>> - uint32_t granularity, Error **errp);
>> /**
>> *
>> * bdrv_register_buf/bdrv_unregister_buf:
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 06df2bda1b..93bbb66cd0 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -537,9 +537,8 @@ 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,
>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> Error **errp);
>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> const char *name,
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 8044ace63e..c37edbe05f 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -38,6 +38,9 @@ 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);
>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> + Error **errp);
>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> const char *name,
>> Error **errp);
>> diff --git a/block.c b/block.c
>> index e3e77feee0..6aec36b7c9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6379,25 +6379,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 49646a30e6..615f8480b2 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> }
>> }
>>
>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> + Error **errp)
>
> strange thing for me: "bitmap" in function name is _not_ the same
> thing as @bitmap argument.. as if it the same,
> function adds "persistent bitmap", but @bitmap is not persistent yet,
> so may be function, don't add persistent bitmap, but marks bitmap persistent?
>
>
> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
> bitmap.persistent flag"
>
Yeah, I meant "Add bitmap to file", where the persistence is implied
because it's part of the file. The bitmap is indeed not YET persistent,
but becomes so as a condition of this call succeeding.
Do you have a suggestion for a better name? I liked the symmetry with
'remove' so that there was an obvious "add bitmap" and "remove bitmap".
Of course, when you remove, it is indeed persistent, so
"remove_persistent_dirty_bitmap" makes sense there.
>
>> +{
>> + BlockDriver *drv = bs->drv;
>> + int ret;
>> +
>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>> + error_setg(errp, "Bitmap '%s' is already persistent, "
>> + "and cannot be re-added to node '%s'",
>> + bdrv_dirty_bitmap_name(bitmap),
>> + bdrv_get_device_or_node_name(bs));
>> + return -EINVAL;
>> + }
>> +
>> + if (!drv) {
>> + error_setg_errno(errp, ENOMEDIUM,
>> + "Can't store persistent bitmaps to %s",
>> + bdrv_get_device_or_node_name(bs));
>> + return -ENOMEDIUM;
>> + }
>> +
>> + if (!drv->bdrv_add_persistent_dirty_bitmap) {
>> + error_setg_errno(errp, ENOTSUP,
>> + "Can't store persistent bitmaps to %s",
>> + bdrv_get_device_or_node_name(bs));
>> + return -ENOTSUP;
>> + }
>> +
>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
>> + if (ret == 0) {
>> + bdrv_dirty_bitmap_set_persistence(bitmap, true);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +
>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> {
>> bdrv_dirty_bitmap_lock(bitmap);
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 83aee1a42a..c3a72ca782 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1607,14 +1607,16 @@ 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,
>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> Error **errp)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> bool found;
>> Qcow2BitmapList *bm_list;
>> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>> + int ret = 0;
>
> dead assignment
>
Will take care of.
>>
>> if (s->qcow_version < 3) {
>> /* Without autoclear_features, we would always have to assume
>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> * have to drop all dirty bitmaps (defeating their purpose).
>> */
>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
>> + ret = -ENOTSUP;
>> goto fail;
>> }
>>
>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp);
>> + if (ret) {
>
> hmm, in other places you prefere
> if ((ret = ...)) {
> syntax
>
I have to get rid of those anyway, they violate our coding style. I'll
be replacing them all with this full style.
>> goto fail;
>> }
>>
>> if (s->nb_bitmaps == 0) {
>> - return true;
>> + return 0;
>> }
>>
>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
>> error_setg(errp,
>> "Maximum number of persistent bitmaps is already reached");
>> + ret = -EOVERFLOW;
>> goto fail;
>> }
>>
>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
>> {
>> error_setg(errp, "Not enough space in the bitmap directory");
>> + ret = -EOVERFLOW;
>> goto fail;
>> }
>>
>> - if (bitmap_list_load(bs, &bm_list, errp)) {
>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> interesting, now we load all bitmaps, so, may be, we don't need to load list every time,
> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe
>
Yeah, I would like to cache this list eventually, but I ran into a lot
of hassle with it last time I tried and put it off for now.
I think we should definitely be able to.
And in fact, if we did, we could even add these bitmaps to the bm_list
as soon as _add_ is called and drop the need for the queued counters.
>> goto fail;
>> }
>>
>> found = find_bitmap_by_name(bm_list, name);
>> bitmap_list_free(bm_list);
>> if (found) {
>> + ret = -EEXIST;
>> error_setg(errp, "Bitmap with the same name is already stored");
>> goto fail;
>> }
>>
>> - return true;
>> -
>> + return 0;
>> fail:
>> error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
>> name, bdrv_get_device_or_node_name(bs));
>
> didn't you consider to move this error_prepend to caller and drop all these gotos?
>
Nope! But I'll do that.
>> - return false;
>> + return ret;
>> }
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 9396d490d5..1c78eba71b 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -5229,7 +5229,7 @@ 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_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
>> .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
>> };
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 3f44b891eb..169a8da831 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>> disabled = false;
>> }
>>
>> - if (persistent) {
>> - aio_context = bdrv_get_aio_context(bs);
>> - aio_context_acquire(aio_context);
>> - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
>> - goto out;
>> - }
>> - }
>> -
>> bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>> if (bitmap == NULL) {
>> - goto out;
>> + return;
>> }
>>
>> if (disabled) {
>> bdrv_disable_dirty_bitmap(bitmap);
>> }
>>
>> - bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
>> - out:
>> - if (aio_context) {
>> + if (persistent) {
>
> Good thing. I suggest to define aio_context in this block too.
>
Oh, sure.
>> + aio_context = bdrv_get_aio_context(bs);
>> + aio_context_acquire(aio_context);
>> + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
>> + bdrv_release_dirty_bitmap(bs, bitmap);
>> + }
>> aio_context_release(aio_context);
>> }
>> }
>>
>
> With some my suggestions or without:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
I will respin anyway, so I will take your suggestions.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-07 18:10 ` John Snow
@ 2019-06-07 18:15 ` Eric Blake
2019-06-07 18:17 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 30+ messages in thread
From: Eric Blake @ 2019-06-07 18:15 UTC (permalink / raw)
To: John Snow, Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
[-- Attachment #1.1: Type: text/plain, Size: 2376 bytes --]
On 6/7/19 1:10 PM, John Snow wrote:
>
>
> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 06.06.2019 21:41, John Snow wrote:
>>> Instead of bdrv_can_store_new_bitmap, rework this as
>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>>> to modify the driver state because we know we ARE adding a bitmap
>>> instead of simply being asked if we CAN store one.
>>>
>>> As part of this symmetry, move this function next to
>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>>
>>> To cement this semantic distinction, use a bitmap object instead of the
>>> (name, granularity) pair as this allows us to set persistence as a
>>> condition of the "add" succeeding.
>>>
>>> Notably, the qcow2 implementation still does not actually store the new
>>> bitmap at add time, but merely ensures that it will be able to at store
>>> time.
>>>
>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap,
>>> + Error **errp)
>>
>> strange thing for me: "bitmap" in function name is _not_ the same
>> thing as @bitmap argument.. as if it the same,
>> function adds "persistent bitmap", but @bitmap is not persistent yet,
>> so may be function, don't add persistent bitmap, but marks bitmap persistent?
>>
>>
>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
>> bitmap.persistent flag"
>>
>
> Yeah, I meant "Add bitmap to file", where the persistence is implied
> because it's part of the file. The bitmap is indeed not YET persistent,
> but becomes so as a condition of this call succeeding.
>
> Do you have a suggestion for a better name? I liked the symmetry with
> 'remove' so that there was an obvious "add bitmap" and "remove bitmap".
>
> Of course, when you remove, it is indeed persistent, so
> "remove_persistent_dirty_bitmap" makes sense there.
>
Or maybe even merely 'bdrv_add_dirty_bitmap' with doc comments stating
that it associates an existing non-persistent bitmap with the bdrv
storage and marks it persistent if successful.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-07 18:10 ` John Snow
2019-06-07 18:15 ` Eric Blake
@ 2019-06-07 18:17 ` Vladimir Sementsov-Ogievskiy
2019-06-07 18:23 ` Vladimir Sementsov-Ogievskiy
2019-06-07 22:08 ` John Snow
1 sibling, 2 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-07 18:17 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
07.06.2019 21:10, John Snow wrote:
>
>
> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 06.06.2019 21:41, John Snow wrote:
>>> Instead of bdrv_can_store_new_bitmap, rework this as
>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>>> to modify the driver state because we know we ARE adding a bitmap
>>> instead of simply being asked if we CAN store one.
>>>
>>> As part of this symmetry, move this function next to
>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>>
>>> To cement this semantic distinction, use a bitmap object instead of the
>>> (name, granularity) pair as this allows us to set persistence as a
>>> condition of the "add" succeeding.
>>>
>>> Notably, the qcow2 implementation still does not actually store the new
>>> bitmap at add time, but merely ensures that it will be able to at store
>>> time.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block/qcow2.h | 5 ++---
>>> include/block/block.h | 2 --
>>> include/block/block_int.h | 5 ++---
>>> include/block/dirty-bitmap.h | 3 +++
>>> block.c | 22 ---------------------
>>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++
>>> block/qcow2-bitmap.c | 24 ++++++++++++++---------
>>> block/qcow2.c | 2 +-
>>> blockdev.c | 19 +++++++-----------
>>> 9 files changed, 68 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index fc1b0d3c1e..95d723d3c0 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -742,9 +742,8 @@ 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,
>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap,
>>> Error **errp);
>>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>> const char *name,
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index f9415ed740..6d520f3b59 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>>> Error **errp);
>>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>>>
>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>>> - uint32_t granularity, Error **errp);
>>> /**
>>> *
>>> * bdrv_register_buf/bdrv_unregister_buf:
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 06df2bda1b..93bbb66cd0 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -537,9 +537,8 @@ 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,
>>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap,
>>> Error **errp);
>>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>>> const char *name,
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index 8044ace63e..c37edbe05f 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -38,6 +38,9 @@ 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);
>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap,
>>> + Error **errp);
>>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>> const char *name,
>>> Error **errp);
>>> diff --git a/block.c b/block.c
>>> index e3e77feee0..6aec36b7c9 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -6379,25 +6379,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 49646a30e6..615f8480b2 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>> }
>>> }
>>>
>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap,
>>> + Error **errp)
>>
>> strange thing for me: "bitmap" in function name is _not_ the same
>> thing as @bitmap argument.. as if it the same,
>> function adds "persistent bitmap", but @bitmap is not persistent yet,
>> so may be function, don't add persistent bitmap, but marks bitmap persistent?
>>
>>
>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
>> bitmap.persistent flag"
>>
>
> Yeah, I meant "Add bitmap to file", where the persistence is implied
> because it's part of the file. The bitmap is indeed not YET persistent,
> but becomes so as a condition of this call succeeding.
>
> Do you have a suggestion for a better name? I liked the symmetry with
> 'remove' so that there was an obvious "add bitmap" and "remove bitmap".
>
> Of course, when you remove, it is indeed persistent, so
> "remove_persistent_dirty_bitmap" makes sense there.
>
>>
>>> +{
>>> + BlockDriver *drv = bs->drv;
>>> + int ret;
>>> +
>>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>>> + error_setg(errp, "Bitmap '%s' is already persistent, "
>>> + "and cannot be re-added to node '%s'",
>>> + bdrv_dirty_bitmap_name(bitmap),
>>> + bdrv_get_device_or_node_name(bs));
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (!drv) {
>>> + error_setg_errno(errp, ENOMEDIUM,
>>> + "Can't store persistent bitmaps to %s",
>>> + bdrv_get_device_or_node_name(bs));
>>> + return -ENOMEDIUM;
>>> + }
>>> +
>>> + if (!drv->bdrv_add_persistent_dirty_bitmap) {
>>> + error_setg_errno(errp, ENOTSUP,
>>> + "Can't store persistent bitmaps to %s",
>>> + bdrv_get_device_or_node_name(bs));
>>> + return -ENOTSUP;
>>> + }
>>> +
>>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
>>> + if (ret == 0) {
>>> + bdrv_dirty_bitmap_set_persistence(bitmap, true);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +
>>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>> {
>>> bdrv_dirty_bitmap_lock(bitmap);
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 83aee1a42a..c3a72ca782 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -1607,14 +1607,16 @@ 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,
>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap,
>>> Error **errp)
>>> {
>>> BDRVQcow2State *s = bs->opaque;
>>> bool found;
>>> Qcow2BitmapList *bm_list;
>>> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>> + int ret = 0;
>>
>> dead assignment
>>
>
> Will take care of.
>
>>>
>>> if (s->qcow_version < 3) {
>>> /* Without autoclear_features, we would always have to assume
>>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>> * have to drop all dirty bitmaps (defeating their purpose).
>>> */
>>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
>>> + ret = -ENOTSUP;
>>> goto fail;
>>> }
>>>
>>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
>>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp);
>>> + if (ret) {
>>
>> hmm, in other places you prefere
>> if ((ret = ...)) {
>> syntax
>>
>
> I have to get rid of those anyway, they violate our coding style. I'll
> be replacing them all with this full style.
>
>>> goto fail;
>>> }
>>>
>>> if (s->nb_bitmaps == 0) {
>>> - return true;
>>> + return 0;
>>> }
>>>
>>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
>>> error_setg(errp,
>>> "Maximum number of persistent bitmaps is already reached");
>>> + ret = -EOVERFLOW;
>>> goto fail;
>>> }
>>>
>>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
>>> {
>>> error_setg(errp, "Not enough space in the bitmap directory");
>>> + ret = -EOVERFLOW;
>>> goto fail;
>>> }
>>>
>>> - if (bitmap_list_load(bs, &bm_list, errp)) {
>>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>>
>> interesting, now we load all bitmaps, so, may be, we don't need to load list every time,
>> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe
>>
>
> Yeah, I would like to cache this list eventually, but I ran into a lot
> of hassle with it last time I tried and put it off for now.
>
> I think we should definitely be able to.
>
> And in fact, if we did, we could even add these bitmaps to the bm_list
> as soon as _add_ is called and drop the need for the queued counters.
Personally I like the old _can_add_ :)
But you want to make this function really do something with driver, so "_can_" is not good
ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will
be in a bad relation with simple setter _set_persistence() which we already have and which
doesn't imply any complicated logic..
On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we
can calculate their number and needed size of directory in any moment, not extending the qcow2
state..
>
>>> goto fail;
>>> }
>>>
>>> found = find_bitmap_by_name(bm_list, name);
>>> bitmap_list_free(bm_list);
>>> if (found) {
>>> + ret = -EEXIST;
>>> error_setg(errp, "Bitmap with the same name is already stored");
>>> goto fail;
>>> }
>>>
>>> - return true;
>>> -
>>> + return 0;
>>> fail:
>>> error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
>>> name, bdrv_get_device_or_node_name(bs));
>>
>> didn't you consider to move this error_prepend to caller and drop all these gotos?
>>
>
> Nope! But I'll do that.
>
>>> - return false;
>>> + return ret;
>>> }
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 9396d490d5..1c78eba71b 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -5229,7 +5229,7 @@ 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_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
>>> .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
>>> };
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 3f44b891eb..169a8da831 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>>> disabled = false;
>>> }
>>>
>>> - if (persistent) {
>>> - aio_context = bdrv_get_aio_context(bs);
>>> - aio_context_acquire(aio_context);
>>> - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
>>> - goto out;
>>> - }
>>> - }
>>> -
>>> bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>>> if (bitmap == NULL) {
>>> - goto out;
>>> + return;
>>> }
>>>
>>> if (disabled) {
>>> bdrv_disable_dirty_bitmap(bitmap);
>>> }
>>>
>>> - bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
>>> - out:
>>> - if (aio_context) {
>>> + if (persistent) {
>>
>> Good thing. I suggest to define aio_context in this block too.
>>
>
> Oh, sure.
>
>>> + aio_context = bdrv_get_aio_context(bs);
>>> + aio_context_acquire(aio_context);
>>> + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
>>> + bdrv_release_dirty_bitmap(bs, bitmap);
>>> + }
>>> aio_context_release(aio_context);
>>> }
>>> }
>>>
>>
>> With some my suggestions or without:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>
> I will respin anyway, so I will take your suggestions.
>
> --js
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-07 18:17 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-07 18:23 ` Vladimir Sementsov-Ogievskiy
2019-06-07 22:08 ` John Snow
1 sibling, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-07 18:23 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
07.06.2019 21:17, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2019 21:10, John Snow wrote:
>>
>>
>> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 06.06.2019 21:41, John Snow wrote:
>>>> Instead of bdrv_can_store_new_bitmap, rework this as
>>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>>>> to modify the driver state because we know we ARE adding a bitmap
>>>> instead of simply being asked if we CAN store one.
>>>>
>>>> As part of this symmetry, move this function next to
>>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>>>
>>>> To cement this semantic distinction, use a bitmap object instead of the
>>>> (name, granularity) pair as this allows us to set persistence as a
>>>> condition of the "add" succeeding.
>>>>
>>>> Notably, the qcow2 implementation still does not actually store the new
>>>> bitmap at add time, but merely ensures that it will be able to at store
>>>> time.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> block/qcow2.h | 5 ++---
>>>> include/block/block.h | 2 --
>>>> include/block/block_int.h | 5 ++---
>>>> include/block/dirty-bitmap.h | 3 +++
>>>> block.c | 22 ---------------------
>>>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++
>>>> block/qcow2-bitmap.c | 24 ++++++++++++++---------
>>>> block/qcow2.c | 2 +-
>>>> blockdev.c | 19 +++++++-----------
>>>> 9 files changed, 68 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index fc1b0d3c1e..95d723d3c0 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -742,9 +742,8 @@ 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,
>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> Error **errp);
>>>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> const char *name,
>>>> diff --git a/include/block/block.h b/include/block/block.h
>>>> index f9415ed740..6d520f3b59 100644
>>>> --- a/include/block/block.h
>>>> +++ b/include/block/block.h
>>>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>>>> Error **errp);
>>>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>>>> - uint32_t granularity, Error **errp);
>>>> /**
>>>> *
>>>> * bdrv_register_buf/bdrv_unregister_buf:
>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>> index 06df2bda1b..93bbb66cd0 100644
>>>> --- a/include/block/block_int.h
>>>> +++ b/include/block/block_int.h
>>>> @@ -537,9 +537,8 @@ 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,
>>>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> Error **errp);
>>>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>>>> const char *name,
>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>>> index 8044ace63e..c37edbe05f 100644
>>>> --- a/include/block/dirty-bitmap.h
>>>> +++ b/include/block/dirty-bitmap.h
>>>> @@ -38,6 +38,9 @@ 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);
>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> + Error **errp);
>>>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> const char *name,
>>>> Error **errp);
>>>> diff --git a/block.c b/block.c
>>>> index e3e77feee0..6aec36b7c9 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -6379,25 +6379,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 49646a30e6..615f8480b2 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> }
>>>> }
>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> + Error **errp)
>>>
>>> strange thing for me: "bitmap" in function name is _not_ the same
>>> thing as @bitmap argument.. as if it the same,
>>> function adds "persistent bitmap", but @bitmap is not persistent yet,
>>> so may be function, don't add persistent bitmap, but marks bitmap persistent?
>>>
>>>
>>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
>>> bitmap.persistent flag"
>>>
>>
>> Yeah, I meant "Add bitmap to file", where the persistence is implied
>> because it's part of the file. The bitmap is indeed not YET persistent,
>> but becomes so as a condition of this call succeeding.
>>
>> Do you have a suggestion for a better name? I liked the symmetry with
>> 'remove' so that there was an obvious "add bitmap" and "remove bitmap".
>>
>> Of course, when you remove, it is indeed persistent, so
>> "remove_persistent_dirty_bitmap" makes sense there.
>>
>>>
>>>> +{
>>>> + BlockDriver *drv = bs->drv;
>>>> + int ret;
>>>> +
>>>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>>>> + error_setg(errp, "Bitmap '%s' is already persistent, "
>>>> + "and cannot be re-added to node '%s'",
>>>> + bdrv_dirty_bitmap_name(bitmap),
>>>> + bdrv_get_device_or_node_name(bs));
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (!drv) {
>>>> + error_setg_errno(errp, ENOMEDIUM,
>>>> + "Can't store persistent bitmaps to %s",
>>>> + bdrv_get_device_or_node_name(bs));
>>>> + return -ENOMEDIUM;
>>>> + }
>>>> +
>>>> + if (!drv->bdrv_add_persistent_dirty_bitmap) {
>>>> + error_setg_errno(errp, ENOTSUP,
>>>> + "Can't store persistent bitmaps to %s",
>>>> + bdrv_get_device_or_node_name(bs));
>>>> + return -ENOTSUP;
>>>> + }
>>>> +
>>>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
>>>> + if (ret == 0) {
>>>> + bdrv_dirty_bitmap_set_persistence(bitmap, true);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +
>>>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>>> {
>>>> bdrv_dirty_bitmap_lock(bitmap);
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index 83aee1a42a..c3a72ca782 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1607,14 +1607,16 @@ 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,
>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> Error **errp)
>>>> {
>>>> BDRVQcow2State *s = bs->opaque;
>>>> bool found;
>>>> Qcow2BitmapList *bm_list;
>>>> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>>> + int ret = 0;
>>>
>>> dead assignment
>>>
>>
>> Will take care of.
>>
>>>> if (s->qcow_version < 3) {
>>>> /* Without autoclear_features, we would always have to assume
>>>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>> * have to drop all dirty bitmaps (defeating their purpose).
>>>> */
>>>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
>>>> + ret = -ENOTSUP;
>>>> goto fail;
>>>> }
>>>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
>>>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp);
>>>> + if (ret) {
>>>
>>> hmm, in other places you prefere
>>> if ((ret = ...)) {
>>> syntax
>>>
>>
>> I have to get rid of those anyway, they violate our coding style. I'll
>> be replacing them all with this full style.
>>
>>>> goto fail;
>>>> }
>>>> if (s->nb_bitmaps == 0) {
>>>> - return true;
>>>> + return 0;
>>>> }
>>>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
>>>> error_setg(errp,
>>>> "Maximum number of persistent bitmaps is already reached");
>>>> + ret = -EOVERFLOW;
>>>> goto fail;
>>>> }
>>>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
>>>> {
>>>> error_setg(errp, "Not enough space in the bitmap directory");
>>>> + ret = -EOVERFLOW;
>>>> goto fail;
>>>> }
>>>> - if (bitmap_list_load(bs, &bm_list, errp)) {
>>>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>>>
>>> interesting, now we load all bitmaps, so, may be, we don't need to load list every time,
>>> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe
>>>
>>
>> Yeah, I would like to cache this list eventually, but I ran into a lot
>> of hassle with it last time I tried and put it off for now.
>>
>> I think we should definitely be able to.
>>
>> And in fact, if we did, we could even add these bitmaps to the bm_list
>> as soon as _add_ is called and drop the need for the queued counters.
>
> Personally I like the old _can_add_ :)
Haha, I already forget that it was _can_store_, which is anyway in a bad relation with the
fact that we want to count bitmaps that are "queued".
>
> But you want to make this function really do something with driver, so "_can_" is not good
> ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will
> be in a bad relation with simple setter _set_persistence() which we already have and which
> doesn't imply any complicated logic..
>
> On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we
> can calculate their number and needed size of directory in any moment, not extending the qcow2
> state..
>
>>
>>>> goto fail;
>>>> }
>>>> found = find_bitmap_by_name(bm_list, name);
>>>> bitmap_list_free(bm_list);
>>>> if (found) {
>>>> + ret = -EEXIST;
>>>> error_setg(errp, "Bitmap with the same name is already stored");
>>>> goto fail;
>>>> }
>>>> - return true;
>>>> -
>>>> + return 0;
>>>> fail:
>>>> error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
>>>> name, bdrv_get_device_or_node_name(bs));
>>>
>>> didn't you consider to move this error_prepend to caller and drop all these gotos?
>>>
>>
>> Nope! But I'll do that.
>>
>>>> - return false;
>>>> + return ret;
>>>> }
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 9396d490d5..1c78eba71b 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -5229,7 +5229,7 @@ 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_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
>>>> .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
>>>> };
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 3f44b891eb..169a8da831 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
>>>> disabled = false;
>>>> }
>>>> - if (persistent) {
>>>> - aio_context = bdrv_get_aio_context(bs);
>>>> - aio_context_acquire(aio_context);
>>>> - if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
>>>> - goto out;
>>>> - }
>>>> - }
>>>> -
>>>> bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>>>> if (bitmap == NULL) {
>>>> - goto out;
>>>> + return;
>>>> }
>>>> if (disabled) {
>>>> bdrv_disable_dirty_bitmap(bitmap);
>>>> }
>>>> - bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
>>>> - out:
>>>> - if (aio_context) {
>>>> + if (persistent) {
>>>
>>> Good thing. I suggest to define aio_context in this block too.
>>>
>>
>> Oh, sure.
>>
>>>> + aio_context = bdrv_get_aio_context(bs);
>>>> + aio_context_acquire(aio_context);
>>>> + if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
>>>> + bdrv_release_dirty_bitmap(bs, bitmap);
>>>> + }
>>>> aio_context_release(aio_context);
>>>> }
>>>> }
>>>>
>>>
>>> With some my suggestions or without:
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>
>> I will respin anyway, so I will take your suggestions.
>>
>> --js
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-07 18:17 ` Vladimir Sementsov-Ogievskiy
2019-06-07 18:23 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-07 22:08 ` John Snow
2019-06-10 9:29 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 30+ messages in thread
From: John Snow @ 2019-06-07 22:08 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
On 6/7/19 2:17 PM, Vladimir Sementsov-Ogievskiy wrote:
> 07.06.2019 21:10, John Snow wrote:
>>
>>
>> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 06.06.2019 21:41, John Snow wrote:
>>>> Instead of bdrv_can_store_new_bitmap, rework this as
>>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>>>> to modify the driver state because we know we ARE adding a bitmap
>>>> instead of simply being asked if we CAN store one.
>>>>
>>>> As part of this symmetry, move this function next to
>>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>>>
>>>> To cement this semantic distinction, use a bitmap object instead of the
>>>> (name, granularity) pair as this allows us to set persistence as a
>>>> condition of the "add" succeeding.
>>>>
>>>> Notably, the qcow2 implementation still does not actually store the new
>>>> bitmap at add time, but merely ensures that it will be able to at store
>>>> time.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> block/qcow2.h | 5 ++---
>>>> include/block/block.h | 2 --
>>>> include/block/block_int.h | 5 ++---
>>>> include/block/dirty-bitmap.h | 3 +++
>>>> block.c | 22 ---------------------
>>>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++
>>>> block/qcow2-bitmap.c | 24 ++++++++++++++---------
>>>> block/qcow2.c | 2 +-
>>>> blockdev.c | 19 +++++++-----------
>>>> 9 files changed, 68 insertions(+), 52 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index fc1b0d3c1e..95d723d3c0 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -742,9 +742,8 @@ 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,
>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> Error **errp);
>>>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> const char *name,
>>>> diff --git a/include/block/block.h b/include/block/block.h
>>>> index f9415ed740..6d520f3b59 100644
>>>> --- a/include/block/block.h
>>>> +++ b/include/block/block.h
>>>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>>>> Error **errp);
>>>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>>>>
>>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>>>> - uint32_t granularity, Error **errp);
>>>> /**
>>>> *
>>>> * bdrv_register_buf/bdrv_unregister_buf:
>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>> index 06df2bda1b..93bbb66cd0 100644
>>>> --- a/include/block/block_int.h
>>>> +++ b/include/block/block_int.h
>>>> @@ -537,9 +537,8 @@ 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,
>>>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> Error **errp);
>>>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>>>> const char *name,
>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>>> index 8044ace63e..c37edbe05f 100644
>>>> --- a/include/block/dirty-bitmap.h
>>>> +++ b/include/block/dirty-bitmap.h
>>>> @@ -38,6 +38,9 @@ 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);
>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> + Error **errp);
>>>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> const char *name,
>>>> Error **errp);
>>>> diff --git a/block.c b/block.c
>>>> index e3e77feee0..6aec36b7c9 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -6379,25 +6379,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 49646a30e6..615f8480b2 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> }
>>>> }
>>>>
>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> + Error **errp)
>>>
>>> strange thing for me: "bitmap" in function name is _not_ the same
>>> thing as @bitmap argument.. as if it the same,
>>> function adds "persistent bitmap", but @bitmap is not persistent yet,
>>> so may be function, don't add persistent bitmap, but marks bitmap persistent?
>>>
>>>
>>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
>>> bitmap.persistent flag"
>>>
>>
>> Yeah, I meant "Add bitmap to file", where the persistence is implied
>> because it's part of the file. The bitmap is indeed not YET persistent,
>> but becomes so as a condition of this call succeeding.
>>
>> Do you have a suggestion for a better name? I liked the symmetry with
>> 'remove' so that there was an obvious "add bitmap" and "remove bitmap".
>>
>> Of course, when you remove, it is indeed persistent, so
>> "remove_persistent_dirty_bitmap" makes sense there.
>>
>>>
>>>> +{
>>>> + BlockDriver *drv = bs->drv;
>>>> + int ret;
>>>> +
>>>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>>>> + error_setg(errp, "Bitmap '%s' is already persistent, "
>>>> + "and cannot be re-added to node '%s'",
>>>> + bdrv_dirty_bitmap_name(bitmap),
>>>> + bdrv_get_device_or_node_name(bs));
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (!drv) {
>>>> + error_setg_errno(errp, ENOMEDIUM,
>>>> + "Can't store persistent bitmaps to %s",
>>>> + bdrv_get_device_or_node_name(bs));
>>>> + return -ENOMEDIUM;
>>>> + }
>>>> +
>>>> + if (!drv->bdrv_add_persistent_dirty_bitmap) {
>>>> + error_setg_errno(errp, ENOTSUP,
>>>> + "Can't store persistent bitmaps to %s",
>>>> + bdrv_get_device_or_node_name(bs));
>>>> + return -ENOTSUP;
>>>> + }
>>>> +
>>>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
>>>> + if (ret == 0) {
>>>> + bdrv_dirty_bitmap_set_persistence(bitmap, true);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +
>>>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>>> {
>>>> bdrv_dirty_bitmap_lock(bitmap);
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index 83aee1a42a..c3a72ca782 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1607,14 +1607,16 @@ 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,
>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap,
>>>> Error **errp)
>>>> {
>>>> BDRVQcow2State *s = bs->opaque;
>>>> bool found;
>>>> Qcow2BitmapList *bm_list;
>>>> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>>> + int ret = 0;
>>>
>>> dead assignment
>>>
>>
>> Will take care of.
>>
>>>>
>>>> if (s->qcow_version < 3) {
>>>> /* Without autoclear_features, we would always have to assume
>>>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>> * have to drop all dirty bitmaps (defeating their purpose).
>>>> */
>>>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
>>>> + ret = -ENOTSUP;
>>>> goto fail;
>>>> }
>>>>
>>>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
>>>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp);
>>>> + if (ret) {
>>>
>>> hmm, in other places you prefere
>>> if ((ret = ...)) {
>>> syntax
>>>
>>
>> I have to get rid of those anyway, they violate our coding style. I'll
>> be replacing them all with this full style.
>>
>>>> goto fail;
>>>> }
>>>>
>>>> if (s->nb_bitmaps == 0) {
>>>> - return true;
>>>> + return 0;
>>>> }
>>>>
>>>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
>>>> error_setg(errp,
>>>> "Maximum number of persistent bitmaps is already reached");
>>>> + ret = -EOVERFLOW;
>>>> goto fail;
>>>> }
>>>>
>>>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
>>>> {
>>>> error_setg(errp, "Not enough space in the bitmap directory");
>>>> + ret = -EOVERFLOW;
>>>> goto fail;
>>>> }
>>>>
>>>> - if (bitmap_list_load(bs, &bm_list, errp)) {
>>>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>>>
>>> interesting, now we load all bitmaps, so, may be, we don't need to load list every time,
>>> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe
>>>
>>
>> Yeah, I would like to cache this list eventually, but I ran into a lot
>> of hassle with it last time I tried and put it off for now.
>>
>> I think we should definitely be able to.
>>
>> And in fact, if we did, we could even add these bitmaps to the bm_list
>> as soon as _add_ is called and drop the need for the queued counters.
>
> Personally I like the old _can_add_ :)
>
> But you want to make this function really do something with driver, so "_can_" is not good
> ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will
> be in a bad relation with simple setter _set_persistence() which we already have and which
> doesn't imply any complicated logic..
>
I think it's a simpler design to have "add" and "remove" callbacks and
be more direct about it. Of course, the qcow2 implementation is free to
avoid a write to disk on add if it wishes, but I don't like the idea of
having to call "can_store" and then later a "do_store" or any such thing.
You could say that can_store is the check and then mark persistent is
the actual action, but then why keep them separate?
I like the link between calling the driver and the later store to be
obvious. I feel like marking the bitmap persistent without the knowledge
or consent of the driver is bound to cause trouble sooner or later, so
I'd rather make the persistent call a condition of the store check
succeeding.
> On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we
> can calculate their number and needed size of directory in any moment, not extending the qcow2
> state..
>
Do we want to add an O(N) check because we don't want to spend 12 bytes?
--js
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
2019-06-07 22:08 ` John Snow
@ 2019-06-10 9:29 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-10 9:29 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
08.06.2019 1:08, John Snow wrote:
>
>
> On 6/7/19 2:17 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 07.06.2019 21:10, John Snow wrote:
>>>
>>>
>>> On 6/7/19 10:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> 06.06.2019 21:41, John Snow wrote:
>>>>> Instead of bdrv_can_store_new_bitmap, rework this as
>>>>> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
>>>>> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
>>>>> to modify the driver state because we know we ARE adding a bitmap
>>>>> instead of simply being asked if we CAN store one.
>>>>>
>>>>> As part of this symmetry, move this function next to
>>>>> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
>>>>>
>>>>> To cement this semantic distinction, use a bitmap object instead of the
>>>>> (name, granularity) pair as this allows us to set persistence as a
>>>>> condition of the "add" succeeding.
>>>>>
>>>>> Notably, the qcow2 implementation still does not actually store the new
>>>>> bitmap at add time, but merely ensures that it will be able to at store
>>>>> time.
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> block/qcow2.h | 5 ++---
>>>>> include/block/block.h | 2 --
>>>>> include/block/block_int.h | 5 ++---
>>>>> include/block/dirty-bitmap.h | 3 +++
>>>>> block.c | 22 ---------------------
>>>>> block/dirty-bitmap.c | 38 ++++++++++++++++++++++++++++++++++++
>>>>> block/qcow2-bitmap.c | 24 ++++++++++++++---------
>>>>> block/qcow2.c | 2 +-
>>>>> blockdev.c | 19 +++++++-----------
>>>>> 9 files changed, 68 insertions(+), 52 deletions(-)
>>>>>
>>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>>> index fc1b0d3c1e..95d723d3c0 100644
>>>>> --- a/block/qcow2.h
>>>>> +++ b/block/qcow2.h
>>>>> @@ -742,9 +742,8 @@ 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,
>>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>>> + BdrvDirtyBitmap *bitmap,
>>>>> Error **errp);
>>>>> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>>> const char *name,
>>>>> diff --git a/include/block/block.h b/include/block/block.h
>>>>> index f9415ed740..6d520f3b59 100644
>>>>> --- a/include/block/block.h
>>>>> +++ b/include/block/block.h
>>>>> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
>>>>> Error **errp);
>>>>> void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
>>>>>
>>>>> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>>>>> - uint32_t granularity, Error **errp);
>>>>> /**
>>>>> *
>>>>> * bdrv_register_buf/bdrv_unregister_buf:
>>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>>> index 06df2bda1b..93bbb66cd0 100644
>>>>> --- a/include/block/block_int.h
>>>>> +++ b/include/block/block_int.h
>>>>> @@ -537,9 +537,8 @@ 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,
>>>>> + int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
>>>>> + BdrvDirtyBitmap *bitmap,
>>>>> Error **errp);
>>>>> void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>>>>> const char *name,
>>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>>>> index 8044ace63e..c37edbe05f 100644
>>>>> --- a/include/block/dirty-bitmap.h
>>>>> +++ b/include/block/dirty-bitmap.h
>>>>> @@ -38,6 +38,9 @@ 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);
>>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>>> + BdrvDirtyBitmap *bitmap,
>>>>> + Error **errp);
>>>>> void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>>> const char *name,
>>>>> Error **errp);
>>>>> diff --git a/block.c b/block.c
>>>>> index e3e77feee0..6aec36b7c9 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -6379,25 +6379,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 49646a30e6..615f8480b2 100644
>>>>> --- a/block/dirty-bitmap.c
>>>>> +++ b/block/dirty-bitmap.c
>>>>> @@ -466,6 +466,44 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>>>> }
>>>>> }
>>>>>
>>>>> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>>> + BdrvDirtyBitmap *bitmap,
>>>>> + Error **errp)
>>>>
>>>> strange thing for me: "bitmap" in function name is _not_ the same
>>>> thing as @bitmap argument.. as if it the same,
>>>> function adds "persistent bitmap", but @bitmap is not persistent yet,
>>>> so may be function, don't add persistent bitmap, but marks bitmap persistent?
>>>>
>>>>
>>>> Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, set
>>>> bitmap.persistent flag"
>>>>
>>>
>>> Yeah, I meant "Add bitmap to file", where the persistence is implied
>>> because it's part of the file. The bitmap is indeed not YET persistent,
>>> but becomes so as a condition of this call succeeding.
>>>
>>> Do you have a suggestion for a better name? I liked the symmetry with
>>> 'remove' so that there was an obvious "add bitmap" and "remove bitmap".
>>>
>>> Of course, when you remove, it is indeed persistent, so
>>> "remove_persistent_dirty_bitmap" makes sense there.
>>>
>>>>
>>>>> +{
>>>>> + BlockDriver *drv = bs->drv;
>>>>> + int ret;
>>>>> +
>>>>> + if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>>>>> + error_setg(errp, "Bitmap '%s' is already persistent, "
>>>>> + "and cannot be re-added to node '%s'",
>>>>> + bdrv_dirty_bitmap_name(bitmap),
>>>>> + bdrv_get_device_or_node_name(bs));
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if (!drv) {
>>>>> + error_setg_errno(errp, ENOMEDIUM,
>>>>> + "Can't store persistent bitmaps to %s",
>>>>> + bdrv_get_device_or_node_name(bs));
>>>>> + return -ENOMEDIUM;
>>>>> + }
>>>>> +
>>>>> + if (!drv->bdrv_add_persistent_dirty_bitmap) {
>>>>> + error_setg_errno(errp, ENOTSUP,
>>>>> + "Can't store persistent bitmaps to %s",
>>>>> + bdrv_get_device_or_node_name(bs));
>>>>> + return -ENOTSUP;
>>>>> + }
>>>>> +
>>>>> + ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
>>>>> + if (ret == 0) {
>>>>> + bdrv_dirty_bitmap_set_persistence(bitmap, true);
>>>>> + }
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +
>>>>> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>>>> {
>>>>> bdrv_dirty_bitmap_lock(bitmap);
>>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>>> index 83aee1a42a..c3a72ca782 100644
>>>>> --- a/block/qcow2-bitmap.c
>>>>> +++ b/block/qcow2-bitmap.c
>>>>> @@ -1607,14 +1607,16 @@ 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,
>>>>> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>>>>> + BdrvDirtyBitmap *bitmap,
>>>>> Error **errp)
>>>>> {
>>>>> BDRVQcow2State *s = bs->opaque;
>>>>> bool found;
>>>>> Qcow2BitmapList *bm_list;
>>>>> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>>>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>>>>> + int ret = 0;
>>>>
>>>> dead assignment
>>>>
>>>
>>> Will take care of.
>>>
>>>>>
>>>>> if (s->qcow_version < 3) {
>>>>> /* Without autoclear_features, we would always have to assume
>>>>> @@ -1623,20 +1625,23 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>>> * have to drop all dirty bitmaps (defeating their purpose).
>>>>> */
>>>>> error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
>>>>> + ret = -ENOTSUP;
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> - if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
>>>>> + ret = check_constraints_on_bitmap(bs, name, granularity, errp);
>>>>> + if (ret) {
>>>>
>>>> hmm, in other places you prefere
>>>> if ((ret = ...)) {
>>>> syntax
>>>>
>>>
>>> I have to get rid of those anyway, they violate our coding style. I'll
>>> be replacing them all with this full style.
>>>
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> if (s->nb_bitmaps == 0) {
>>>>> - return true;
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
>>>>> error_setg(errp,
>>>>> "Maximum number of persistent bitmaps is already reached");
>>>>> + ret = -EOVERFLOW;
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> @@ -1644,24 +1649,25 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>>> QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
>>>>> {
>>>>> error_setg(errp, "Not enough space in the bitmap directory");
>>>>> + ret = -EOVERFLOW;
>>>>> goto fail;
>>>>> }
>>>>>
>>>>> - if (bitmap_list_load(bs, &bm_list, errp)) {
>>>>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>>>>
>>>> interesting, now we load all bitmaps, so, may be, we don't need to load list every time,
>>>> but may use bs.dirty_bitmaps to check such things.. But it seems unsafe
>>>>
>>>
>>> Yeah, I would like to cache this list eventually, but I ran into a lot
>>> of hassle with it last time I tried and put it off for now.
>>>
>>> I think we should definitely be able to.
>>>
>>> And in fact, if we did, we could even add these bitmaps to the bm_list
>>> as soon as _add_ is called and drop the need for the queued counters.
>>
>> Personally I like the old _can_add_ :)
>>
>> But you want to make this function really do something with driver, so "_can_" is not good
>> ofcourse and _add_ is better. And any kind of _mark_persistent_ or _make_persistent_ will
>> be in a bad relation with simple setter _set_persistence() which we already have and which
>> doesn't imply any complicated logic..
>>
>
> I think it's a simpler design to have "add" and "remove" callbacks and
> be more direct about it. Of course, the qcow2 implementation is free to
> avoid a write to disk on add if it wishes, but I don't like the idea of
> having to call "can_store" and then later a "do_store" or any such thing.
>
> You could say that can_store is the check and then mark persistent is
> the actual action, but then why keep them separate?
>
> I like the link between calling the driver and the later store to be
> obvious. I feel like marking the bitmap persistent without the knowledge
> or consent of the driver is bound to cause trouble sooner or later, so
> I'd rather make the persistent call a condition of the store check
> succeeding.
>
>> On the other hand I still not sure that we need track "queued" bitmaps in qcow2 driver, as we
>> can calculate their number and needed size of directory in any moment, not extending the qcow2
>> state..
>>
>
> Do we want to add an O(N) check because we don't want to spend 12 bytes?
We have O(N) check anyway, as we should check for existent bitmap name
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
2019-06-06 18:41 [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps John Snow
2019-06-06 18:41 ` [Qemu-devel] [PATCH 1/5] block/qcow2-bitmap: allow bitmap_list_load to return an error code John Snow
2019-06-06 18:41 ` [Qemu-devel] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap John Snow
@ 2019-06-06 18:41 ` John Snow
2019-06-07 2:24 ` Eric Blake
2019-06-07 14:41 ` Vladimir Sementsov-Ogievskiy
2019-06-06 18:41 ` [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps John Snow
` (3 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: John Snow @ 2019-06-06 18:41 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz,
John Snow
Allow propagating error code information from
bdrv_remove_persistent_dirty_bitmap as well.
Give it an interface that matches the newly revised
bdrv_add_persistent_dirty_bitmap, including removing the persistent flag
when the operation succeeds and refusing to operate on bitmaps that are
not persistent.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 6 +++---
include/block/block_int.h | 6 +++---
include/block/dirty-bitmap.h | 6 +++---
block/dirty-bitmap.c | 25 ++++++++++++++++++++-----
block/qcow2-bitmap.c | 16 +++++++++-------
blockdev.c | 15 ++++++---------
6 files changed, 44 insertions(+), 30 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 95d723d3c0..ce07f003f7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -745,9 +745,9 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp);
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ 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 93bbb66cd0..59f8cb9c12 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -540,9 +540,9 @@ struct BlockDriver {
int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
- void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
- const char *name,
- Error **errp);
+ int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ 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 c37edbe05f..88a9832ddc 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -41,9 +41,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp);
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ 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 615f8480b2..4667f9e65a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -455,15 +455,30 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
* BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
* stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
* not fail.
- * This function doesn't release corresponding BdrvDirtyBitmap.
+ * This function doesn't release the corresponding BdrvDirtyBitmap.
*/
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp)
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp)
{
+ int ret = 0;
+
+ if (!bdrv_dirty_bitmap_get_persistence(bitmap)) {
+ error_setg(errp, "Bitmap '%s' is not persistent, "
+ "so it cannot be removed from node '%s'",
+ bdrv_dirty_bitmap_name(bitmap),
+ bdrv_get_device_or_node_name(bs));
+ return -EINVAL;
+ }
+
if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
- bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+ ret = bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bitmap, errp);
}
+ if (!ret) {
+ bdrv_dirty_bitmap_set_persistence(bitmap, false);
+ }
+
+ return ret;
}
int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index c3a72ca782..930a6c91ff 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1402,23 +1402,24 @@ 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,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp)
{
- int ret;
+ int ret = 0;
BDRVQcow2State *s = bs->opaque;
Qcow2Bitmap *bm;
Qcow2BitmapList *bm_list;
+ const char *name = bdrv_dirty_bitmap_name(bitmap);
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;
}
- if (bitmap_list_load(bs, &bm_list, errp)) {
- return;
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
+ return ret;
}
bm = find_bitmap_by_name(bm_list, name);
@@ -1439,6 +1440,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
fail:
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 169a8da831..82f02d8e72 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2875,7 +2875,6 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
{
BlockDriverState *bs;
BdrvDirtyBitmap *bitmap;
- Error *local_err = NULL;
AioContext *aio_context = NULL;
bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
@@ -2889,20 +2888,18 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
}
if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+ int ret;
+
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
- goto out;
+ ret = bdrv_remove_persistent_dirty_bitmap(bs, bitmap, errp);
+ aio_context_release(aio_context);
+ if (ret) {
+ return;
}
}
bdrv_release_dirty_bitmap(bs, bitmap);
- out:
- if (aio_context) {
- aio_context_release(aio_context);
- }
}
/**
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
2019-06-06 18:41 ` [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap John Snow
@ 2019-06-07 2:24 ` Eric Blake
2019-06-07 14:41 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 30+ messages in thread
From: Eric Blake @ 2019-06-07 2:24 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz
[-- Attachment #1.1: Type: text/plain, Size: 1542 bytes --]
On 6/6/19 1:41 PM, John Snow wrote:
> Allow propagating error code information from
> bdrv_remove_persistent_dirty_bitmap as well.
>
> Give it an interface that matches the newly revised
> bdrv_add_persistent_dirty_bitmap, including removing the persistent flag
> when the operation succeeds and refusing to operate on bitmaps that are
> not persistent.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> +++ b/include/block/block_int.h
> @@ -540,9 +540,9 @@ struct BlockDriver {
> int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> - void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> - const char *name,
> - Error **errp);
> + int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp);
Would it hurt us (in patch 2 and again here) to add a comment about what
each callback is supposed to do? Just because we've been lousy at
callback interfaces in the past does not mean that we should continue to
omit them.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
2019-06-06 18:41 ` [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap John Snow
2019-06-07 2:24 ` Eric Blake
@ 2019-06-07 14:41 ` Vladimir Sementsov-Ogievskiy
2019-06-07 18:16 ` John Snow
1 sibling, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-07 14:41 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
06.06.2019 21:41, John Snow wrote:
> Allow propagating error code information from
> bdrv_remove_persistent_dirty_bitmap as well.
>
> Give it an interface that matches the newly revised
> bdrv_add_persistent_dirty_bitmap, including removing the persistent flag
> when the operation succeeds and refusing to operate on bitmaps that are
> not persistent.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2.h | 6 +++---
> include/block/block_int.h | 6 +++---
> include/block/dirty-bitmap.h | 6 +++---
> block/dirty-bitmap.c | 25 ++++++++++++++++++++-----
> block/qcow2-bitmap.c | 16 +++++++++-------
> blockdev.c | 15 ++++++---------
> 6 files changed, 44 insertions(+), 30 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 95d723d3c0..ce07f003f7 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -745,9 +745,9 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> - const char *name,
> - Error **errp);
> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + 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 93bbb66cd0..59f8cb9c12 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -540,9 +540,9 @@ struct BlockDriver {
> int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> - void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> - const char *name,
> - Error **errp);
> + int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + 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 c37edbe05f..88a9832ddc 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -41,9 +41,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
> int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> - const char *name,
> - Error **errp);
> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + 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 615f8480b2..4667f9e65a 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -455,15 +455,30 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
> * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
> * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
> * not fail.
> - * This function doesn't release corresponding BdrvDirtyBitmap.
> + * This function doesn't release the corresponding BdrvDirtyBitmap.
> */
> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> - const char *name,
> - Error **errp)
> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp)
> {
> + int ret = 0;
> +
> + if (!bdrv_dirty_bitmap_get_persistence(bitmap)) {
> + error_setg(errp, "Bitmap '%s' is not persistent, "
> + "so it cannot be removed from node '%s'",
> + bdrv_dirty_bitmap_name(bitmap),
> + bdrv_get_device_or_node_name(bs));
> + return -EINVAL;
> + }
> +
> if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
> - bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
> + ret = bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bitmap, errp);
> }
> + if (!ret) {
> + bdrv_dirty_bitmap_set_persistence(bitmap, false);
> + }
> +
> + return ret;
> }
>
> int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index c3a72ca782..930a6c91ff 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1402,23 +1402,24 @@ 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,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp)
> {
> - int ret;
> + int ret = 0;
dead assignment
> BDRVQcow2State *s = bs->opaque;
> Qcow2Bitmap *bm;
> Qcow2BitmapList *bm_list;
> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>
> 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;
> }
>
> - if (bitmap_list_load(bs, &bm_list, errp)) {
> - return;
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
> + return ret;
> }
>
> bm = find_bitmap_by_name(bm_list, name);
if (bm == NULL) {
goto fail;
}
You don't set ret, as it's considered success. Worth s/fail/out then (preexisting, of course)?
> @@ -1439,6 +1440,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> fail:
> 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 169a8da831..82f02d8e72 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2875,7 +2875,6 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> {
> BlockDriverState *bs;
> BdrvDirtyBitmap *bitmap;
> - Error *local_err = NULL;
Oh, that's cool. And it was my mistake to create interfaces provoking endless local_errors..
> AioContext *aio_context = NULL;
>
> bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
> @@ -2889,20 +2888,18 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> }
>
> if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
> + int ret;
> +
> aio_context = bdrv_get_aio_context(bs);
and here, could you define aio_context in this block?
> aio_context_acquire(aio_context);
> - bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
> - if (local_err != NULL) {
> - error_propagate(errp, local_err);
> - goto out;
> + ret = bdrv_remove_persistent_dirty_bitmap(bs, bitmap, errp);
> + aio_context_release(aio_context);
> + if (ret) {
> + return;
> }
> }
>
> bdrv_release_dirty_bitmap(bs, bitmap);
> - out:
> - if (aio_context) {
> - aio_context_release(aio_context);
> - }
> }
>
> /**
>
With some my suggestions or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
2019-06-07 14:41 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-07 18:16 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2019-06-07 18:16 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
On 6/7/19 10:41 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.06.2019 21:41, John Snow wrote:
>> Allow propagating error code information from
>> bdrv_remove_persistent_dirty_bitmap as well.
>>
>> Give it an interface that matches the newly revised
>> bdrv_add_persistent_dirty_bitmap, including removing the persistent flag
>> when the operation succeeds and refusing to operate on bitmaps that are
>> not persistent.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2.h | 6 +++---
>> include/block/block_int.h | 6 +++---
>> include/block/dirty-bitmap.h | 6 +++---
>> block/dirty-bitmap.c | 25 ++++++++++++++++++++-----
>> block/qcow2-bitmap.c | 16 +++++++++-------
>> blockdev.c | 15 ++++++---------
>> 6 files changed, 44 insertions(+), 30 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 95d723d3c0..ce07f003f7 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -745,9 +745,9 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>> int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap,
>> Error **errp);
>> -void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp);
>> +int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> + 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 93bbb66cd0..59f8cb9c12 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -540,9 +540,9 @@ struct BlockDriver {
>> int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap,
>> Error **errp);
>> - void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp);
>> + int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> + 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 c37edbe05f..88a9832ddc 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -41,9 +41,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>> int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap,
>> Error **errp);
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp);
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> + 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 615f8480b2..4667f9e65a 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -455,15 +455,30 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>> * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
>> * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
>> * not fail.
>> - * This function doesn't release corresponding BdrvDirtyBitmap.
>> + * This function doesn't release the corresponding BdrvDirtyBitmap.
>> */
>> -void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> - const char *name,
>> - Error **errp)
>> +int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> + Error **errp)
>> {
>> + int ret = 0;
>> +
>> + if (!bdrv_dirty_bitmap_get_persistence(bitmap)) {
>> + error_setg(errp, "Bitmap '%s' is not persistent, "
>> + "so it cannot be removed from node '%s'",
>> + bdrv_dirty_bitmap_name(bitmap),
>> + bdrv_get_device_or_node_name(bs));
>> + return -EINVAL;
>> + }
>> +
>> if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
>> - bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
>> + ret = bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bitmap, errp);
>> }
>> + if (!ret) {
>> + bdrv_dirty_bitmap_set_persistence(bitmap, false);
>> + }
>> +
>> + return ret;
>> }
>>
>> int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index c3a72ca782..930a6c91ff 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1402,23 +1402,24 @@ 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,
>> + BdrvDirtyBitmap *bitmap,
>> + Error **errp)
>> {
>> - int ret;
>> + int ret = 0;
>
> dead assignment
>
>> BDRVQcow2State *s = bs->opaque;
>> Qcow2Bitmap *bm;
>> Qcow2BitmapList *bm_list;
>> + const char *name = bdrv_dirty_bitmap_name(bitmap);
>>
>> 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;
>> }
>>
>> - if (bitmap_list_load(bs, &bm_list, errp)) {
>> - return;
>> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>> + return ret;
>> }
>>
>> bm = find_bitmap_by_name(bm_list, name);
> if (bm == NULL) {
> goto fail;
> }
>
> You don't set ret, as it's considered success. Worth s/fail/out then (preexisting, of course)?
>
This becomes an error in the next patch, so I'll omit this suggestion
because the problem goes away soon anyway.
>> @@ -1439,6 +1440,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> fail:
>> 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 169a8da831..82f02d8e72 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2875,7 +2875,6 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> {
>> BlockDriverState *bs;
>> BdrvDirtyBitmap *bitmap;
>> - Error *local_err = NULL;
>
> Oh, that's cool. And it was my mistake to create interfaces provoking endless local_errors..
>
I was worried these first three patches might look useless, but I'm glad
you like the idea of doing it this way.
>> AioContext *aio_context = NULL;
>>
>> bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>> @@ -2889,20 +2888,18 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>> }
>>
>> if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
>> + int ret;
>> +
>> aio_context = bdrv_get_aio_context(bs);
>
> and here, could you define aio_context in this block?
>
Indeed.
>> aio_context_acquire(aio_context);
>> - bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
>> - if (local_err != NULL) {
>> - error_propagate(errp, local_err);
>> - goto out;
>> + ret = bdrv_remove_persistent_dirty_bitmap(bs, bitmap, errp);
>> + aio_context_release(aio_context);
>> + if (ret) {
>> + return;
>> }
>> }
>>
>> bdrv_release_dirty_bitmap(bs, bitmap);
>> - out:
>> - if (aio_context) {
>> - aio_context_release(aio_context);
>> - }
>> }
>>
>> /**
>>
>
> With some my suggestions or without:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
Will take them, thanks for the review!
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
2019-06-06 18:41 [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps John Snow
` (2 preceding siblings ...)
2019-06-06 18:41 ` [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap John Snow
@ 2019-06-06 18:41 ` John Snow
2019-06-07 2:27 ` Eric Blake
2019-06-07 14:58 ` Vladimir Sementsov-Ogievskiy
2019-06-06 18:41 ` [Qemu-devel] [PATCH 5/5] block/qcow2-bitmap: Count queued bitmaps towards directory_size John Snow
` (2 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: John Snow @ 2019-06-06 18:41 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz,
John Snow
When we check to see if we can store a bitmap, we don't check how many
we've queued up. This can cause a problem saving bitmaps on close
instead of when we request them to be added. With the stricter add
interface, prohibit these bitmaps specifically.
To match, make the remove interface more strict as well; now rejecting
any requests to remove bitmaps that were never queued for storage.
We don't need to "find" the bitmap when the interface has been given the
bitmap explicitly, but this is done to make sure that the bitmap given
actually does belong to the bs we were passed as a paranoia check to
enforce consistency.
---
"What about directory size?" Please see the following patch.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 1 +
block/dirty-bitmap.c | 8 +++-----
block/qcow2-bitmap.c | 31 ++++++++++++++++++++++++++-----
3 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index ce07f003f7..ebf60ac236 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -317,6 +317,7 @@ typedef struct BDRVQcow2State {
QCowSnapshot *snapshots;
uint32_t nb_bitmaps;
+ uint32_t nb_queued_bitmaps;
uint64_t bitmap_directory_size;
uint64_t bitmap_directory_offset;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4667f9e65a..084c42af57 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -450,11 +450,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
}
/**
- * Remove persistent dirty bitmap from the storage if it exists.
- * Absence of bitmap is not an error, because we have the following scenario:
- * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
- * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
- * not fail.
+ * Remove a persistent dirty bitmap from storage,
+ * or dequeue it from being stored if it is enqueued.
+ *
* This function doesn't release the corresponding BdrvDirtyBitmap.
*/
int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 930a6c91ff..7193c66787 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
return NULL;
}
+static int qcow2_remove_queued_dirty_bitmap(
+ BlockDriverState *bs, const char *name, Error **errp)
+{
+ BDRVQcow2State *s = bs->opaque;
+ BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name);
+ if (!bitmap) {
+ error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'",
+ bdrv_get_node_name(bs), name);
+ return -ENOENT;
+ }
+ assert(s->nb_queued_bitmaps > 0);
+ assert(bdrv_dirty_bitmap_get_persistence(bitmap));
+ s->nb_queued_bitmaps -= 1;
+
+ return 0;
+}
+
int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp)
@@ -1413,9 +1430,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name = bdrv_dirty_bitmap_name(bitmap);
if (s->nb_bitmaps == 0) {
- /* Absence of the bitmap is not an error: see explanation above
- * bdrv_remove_persistent_dirty_bitmap() definition. */
- return 0;
+ return qcow2_remove_queued_dirty_bitmap(bs, name, errp);
}
if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
@@ -1424,6 +1439,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
bm = find_bitmap_by_name(bm_list, name);
if (bm == NULL) {
+ ret = qcow2_remove_queued_dirty_bitmap(bs, name, errp);
goto fail;
}
@@ -1544,6 +1560,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
error_setg_errno(errp, -ret, "Failed to update bitmap extension");
goto fail;
}
+ s->nb_queued_bitmaps = 0;
/* Bitmap directory was successfully updated, so, old data can be dropped.
* TODO it is better to reuse these clusters */
@@ -1618,6 +1635,7 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
Qcow2BitmapList *bm_list;
const char *name = bdrv_dirty_bitmap_name(bitmap);
uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+ uint32_t nb_bitmaps;
int ret = 0;
if (s->qcow_version < 3) {
@@ -1636,11 +1654,12 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
goto fail;
}
- if (s->nb_bitmaps == 0) {
+ nb_bitmaps = s->nb_bitmaps + s->nb_queued_bitmaps;
+ if (nb_bitmaps == 0) {
return 0;
}
- if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
+ if (nb_bitmaps >= QCOW2_MAX_BITMAPS) {
error_setg(errp,
"Maximum number of persistent bitmaps is already reached");
ret = -EOVERFLOW;
@@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
goto fail;
}
+ s->nb_queued_bitmaps += 1;
+
return 0;
fail:
error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
2019-06-06 18:41 ` [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps John Snow
@ 2019-06-07 2:27 ` Eric Blake
2019-06-07 18:04 ` John Snow
2019-06-07 14:58 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 30+ messages in thread
From: Eric Blake @ 2019-06-07 2:27 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz
[-- Attachment #1.1: Type: text/plain, Size: 1994 bytes --]
On 6/6/19 1:41 PM, John Snow wrote:
> When we check to see if we can store a bitmap, we don't check how many
> we've queued up. This can cause a problem saving bitmaps on close
> instead of when we request them to be added. With the stricter add
> interface, prohibit these bitmaps specifically.
>
> To match, make the remove interface more strict as well; now rejecting
> any requests to remove bitmaps that were never queued for storage.
>
> We don't need to "find" the bitmap when the interface has been given the
> bitmap explicitly, but this is done to make sure that the bitmap given
> actually does belong to the bs we were passed as a paranoia check to
> enforce consistency.
>
> ---
Oops - that marker...
>
> "What about directory size?" Please see the following patch.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
...renders the S-o-b invisible.
> +++ b/block/qcow2-bitmap.c
> @@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
> return NULL;
> }
>
> +static int qcow2_remove_queued_dirty_bitmap(
> + BlockDriverState *bs, const char *name, Error **errp)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name);
> + if (!bitmap) {
> + error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'",
> + bdrv_get_node_name(bs), name);
> + return -ENOENT;
> + }
> + assert(s->nb_queued_bitmaps > 0);
> + assert(bdrv_dirty_bitmap_get_persistence(bitmap));
> + s->nb_queued_bitmaps -= 1;
I tend to use -- over -= 1.
> @@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> goto fail;
> }
>
> + s->nb_queued_bitmaps += 1;
And again, for ++
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
2019-06-07 2:27 ` Eric Blake
@ 2019-06-07 18:04 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2019-06-07 18:04 UTC (permalink / raw)
To: Eric Blake, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz
On 6/6/19 10:27 PM, Eric Blake wrote:
> On 6/6/19 1:41 PM, John Snow wrote:
>> When we check to see if we can store a bitmap, we don't check how many
>> we've queued up. This can cause a problem saving bitmaps on close
>> instead of when we request them to be added. With the stricter add
>> interface, prohibit these bitmaps specifically.
>>
>> To match, make the remove interface more strict as well; now rejecting
>> any requests to remove bitmaps that were never queued for storage.
>>
>> We don't need to "find" the bitmap when the interface has been given the
>> bitmap explicitly, but this is done to make sure that the bitmap given
>> actually does belong to the bs we were passed as a paranoia check to
>> enforce consistency.
>>
>> ---
>
> Oops - that marker...
>
>>
>> "What about directory size?" Please see the following patch.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ...renders the S-o-b invisible.
>
>> +++ b/block/qcow2-bitmap.c
>> @@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
>> return NULL;
>> }
>>
>> +static int qcow2_remove_queued_dirty_bitmap(
>> + BlockDriverState *bs, const char *name, Error **errp)
>> +{
>> + BDRVQcow2State *s = bs->opaque;
>> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name);
>> + if (!bitmap) {
>> + error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'",
>> + bdrv_get_node_name(bs), name);
>> + return -ENOENT;
>> + }
>> + assert(s->nb_queued_bitmaps > 0);
>> + assert(bdrv_dirty_bitmap_get_persistence(bitmap));
>> + s->nb_queued_bitmaps -= 1;
>
> I tend to use -- over -= 1.
>
>> @@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
>> goto fail;
>> }
>>
>> + s->nb_queued_bitmaps += 1;
>
> And again, for ++
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Wow, sorry, lots of Python lately!
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
2019-06-06 18:41 ` [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps John Snow
2019-06-07 2:27 ` Eric Blake
@ 2019-06-07 14:58 ` Vladimir Sementsov-Ogievskiy
2019-06-07 18:24 ` John Snow
1 sibling, 1 reply; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-07 14:58 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
06.06.2019 21:41, John Snow wrote:
> When we check to see if we can store a bitmap, we don't check how many
> we've queued up. This can cause a problem saving bitmaps on close
> instead of when we request them to be added. With the stricter add
> interface, prohibit these bitmaps specifically.
>
> To match, make the remove interface more strict as well; now rejecting
> any requests to remove bitmaps that were never queued for storage.
>
> We don't need to "find" the bitmap when the interface has been given the
> bitmap explicitly, but this is done to make sure that the bitmap given
> actually does belong to the bs we were passed as a paranoia check to
> enforce consistency.
If you want to check it, I'd really prefer to do it explictly,
by adding "bool bdrv_has_dirty_bitmap(bs, bitmap) handler, or bitmap.bs field",
instead of hiding it under inconvenient interface of helper, so we actually do
name = bdrv_dirty_bitmap_name(bitmap);
bitmap = bdrv_find_dirty_bitmap(bs, name);
it really looks strange.
Hmmm, when I read series cover letter and this commit message, I thought
that you'll just calculate current number of persistent bitmaps on bs..
Do we really need to introduce additional counters on qcow2 state?
So, you want to check nb_queued + s->nb_bitmaps instead of just s->nb_bitmaps.
I think we can just count persistent dirty bitmaps and take that number
(as, there should not be in-image bitmaps, for which we don't have in-ram
version, but we can check it too and return an error on mismatch (or count
in-image-not-in-ram bitmaps and this count to number of in-ram persistent
bitmaps it seems an extra thing)..
>
> ---
>
> "What about directory size?" Please see the following patch.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2.h | 1 +
> block/dirty-bitmap.c | 8 +++-----
> block/qcow2-bitmap.c | 31 ++++++++++++++++++++++++++-----
> 3 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index ce07f003f7..ebf60ac236 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -317,6 +317,7 @@ typedef struct BDRVQcow2State {
> QCowSnapshot *snapshots;
>
> uint32_t nb_bitmaps;
> + uint32_t nb_queued_bitmaps;
> uint64_t bitmap_directory_size;
> uint64_t bitmap_directory_offset;
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4667f9e65a..084c42af57 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -450,11 +450,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
> }
>
> /**
> - * Remove persistent dirty bitmap from the storage if it exists.
> - * Absence of bitmap is not an error, because we have the following scenario:
> - * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
> - * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
> - * not fail.
> + * Remove a persistent dirty bitmap from storage,
> + * or dequeue it from being stored if it is enqueued.
> + *
> * This function doesn't release the corresponding BdrvDirtyBitmap.
> */
> int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 930a6c91ff..7193c66787 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
> return NULL;
> }
>
> +static int qcow2_remove_queued_dirty_bitmap(
> + BlockDriverState *bs, const char *name, Error **errp)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name);
> + if (!bitmap) {
> + error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'",
> + bdrv_get_node_name(bs), name);
> + return -ENOENT;
> + }
> + assert(s->nb_queued_bitmaps > 0);
> + assert(bdrv_dirty_bitmap_get_persistence(bitmap));
> + s->nb_queued_bitmaps -= 1;
> +
> + return 0;
> +}
> +
> int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp)
> @@ -1413,9 +1430,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> const char *name = bdrv_dirty_bitmap_name(bitmap);
>
> if (s->nb_bitmaps == 0) {
> - /* Absence of the bitmap is not an error: see explanation above
> - * bdrv_remove_persistent_dirty_bitmap() definition. */
> - return 0;
> + return qcow2_remove_queued_dirty_bitmap(bs, name, errp);
> }
>
> if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
> @@ -1424,6 +1439,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>
> bm = find_bitmap_by_name(bm_list, name);
> if (bm == NULL) {
> + ret = qcow2_remove_queued_dirty_bitmap(bs, name, errp);
> goto fail;
> }
>
> @@ -1544,6 +1560,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> error_setg_errno(errp, -ret, "Failed to update bitmap extension");
> goto fail;
> }
> + s->nb_queued_bitmaps = 0;
>
> /* Bitmap directory was successfully updated, so, old data can be dropped.
> * TODO it is better to reuse these clusters */
> @@ -1618,6 +1635,7 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> Qcow2BitmapList *bm_list;
> const char *name = bdrv_dirty_bitmap_name(bitmap);
> uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> + uint32_t nb_bitmaps;
> int ret = 0;
>
> if (s->qcow_version < 3) {
> @@ -1636,11 +1654,12 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> goto fail;
> }
>
> - if (s->nb_bitmaps == 0) {
> + nb_bitmaps = s->nb_bitmaps + s->nb_queued_bitmaps;
> + if (nb_bitmaps == 0) {
> return 0;
> }
>
> - if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
> + if (nb_bitmaps >= QCOW2_MAX_BITMAPS) {
> error_setg(errp,
> "Maximum number of persistent bitmaps is already reached");
> ret = -EOVERFLOW;
> @@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> goto fail;
> }
>
> + s->nb_queued_bitmaps += 1;
> +
> return 0;
> fail:
> error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
2019-06-07 14:58 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-07 18:24 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2019-06-07 18:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, Markus Armbruster, Max Reitz
On 6/7/19 10:58 AM, Vladimir Sementsov-Ogievskiy wrote:
> 06.06.2019 21:41, John Snow wrote:
>> When we check to see if we can store a bitmap, we don't check how many
>> we've queued up. This can cause a problem saving bitmaps on close
>> instead of when we request them to be added. With the stricter add
>> interface, prohibit these bitmaps specifically.
>>
>> To match, make the remove interface more strict as well; now rejecting
>> any requests to remove bitmaps that were never queued for storage.
>>
>> We don't need to "find" the bitmap when the interface has been given the
>> bitmap explicitly, but this is done to make sure that the bitmap given
>> actually does belong to the bs we were passed as a paranoia check to
>> enforce consistency.
>
> If you want to check it, I'd really prefer to do it explictly,
> by adding "bool bdrv_has_dirty_bitmap(bs, bitmap) handler, or bitmap.bs field",
> instead of hiding it under inconvenient interface of helper, so we actually do
>
> name = bdrv_dirty_bitmap_name(bitmap);
> bitmap = bdrv_find_dirty_bitmap(bs, name);
>
> it really looks strange.
>
You're right, it does look weird. It's not obvious when you read it what
the real purpose is. I'll try to rectify it with an explicit helper like
you suggest.
> Hmmm, when I read series cover letter and this commit message, I thought
> that you'll just calculate current number of persistent bitmaps on bs..
> Do we really need to introduce additional counters on qcow2 state?
>
I suppose I could do that, too -- that seems a bit heavier than just
incrementing a counter, but it has less risk of getting out of sync.
> So, you want to check nb_queued + s->nb_bitmaps instead of just s->nb_bitmaps.
>
> I think we can just count persistent dirty bitmaps and take that number
> (as, there should not be in-image bitmaps, for which we don't have in-ram
> version, but we can check it too and return an error on mismatch (or count
> in-image-not-in-ram bitmaps and this count to number of in-ram persistent
> bitmaps it seems an extra thing)..
>
If I cache the bm_list as discussed in patch 2, we can actually avoid
re-enumerating all of the bitmaps and just go off of that data without
having to add any new counters.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH 5/5] block/qcow2-bitmap: Count queued bitmaps towards directory_size
2019-06-06 18:41 [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps John Snow
` (3 preceding siblings ...)
2019-06-06 18:41 ` [Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps John Snow
@ 2019-06-06 18:41 ` John Snow
2019-06-07 2:30 ` Eric Blake
2019-06-06 21:54 ` [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps no-reply
2019-10-09 18:57 ` Eric Blake
6 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2019-06-06 18:41 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz,
John Snow
Similarly to the previous commit, we need to also keep a ledger of the
additional directory size burden that we've not yet committed so we can
reject new additions sooner instead of later.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2.h | 1 +
block/qcow2-bitmap.c | 13 ++++++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index ebf60ac236..5aff97eb9c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -318,6 +318,7 @@ typedef struct BDRVQcow2State {
uint32_t nb_bitmaps;
uint32_t nb_queued_bitmaps;
+ uint32_t queued_directory_size;
uint64_t bitmap_directory_size;
uint64_t bitmap_directory_offset;
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 7193c66787..b103fab362 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1405,16 +1405,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list,
static int qcow2_remove_queued_dirty_bitmap(
BlockDriverState *bs, const char *name, Error **errp)
{
+ uint32_t size_delta;
BDRVQcow2State *s = bs->opaque;
BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name);
+
if (!bitmap) {
error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'",
bdrv_get_node_name(bs), name);
return -ENOENT;
}
+
+ size_delta = calc_dir_entry_size(strlen(name), 0);
assert(s->nb_queued_bitmaps > 0);
assert(bdrv_dirty_bitmap_get_persistence(bitmap));
+ assert(s->queued_directory_size >= size_delta);
+
s->nb_queued_bitmaps -= 1;
+ s->queued_directory_size -= size_delta;
return 0;
}
@@ -1561,6 +1568,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
goto fail;
}
s->nb_queued_bitmaps = 0;
+ s->queued_directory_size = 0;
/* Bitmap directory was successfully updated, so, old data can be dropped.
* TODO it is better to reuse these clusters */
@@ -1636,6 +1644,7 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name = bdrv_dirty_bitmap_name(bitmap);
uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
uint32_t nb_bitmaps;
+ uint32_t size_delta;
int ret = 0;
if (s->qcow_version < 3) {
@@ -1666,7 +1675,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
goto fail;
}
- if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) >
+ size_delta = calc_dir_entry_size(strlen(name), 0);
+ if (s->bitmap_directory_size + s->queued_directory_size + size_delta >
QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
{
error_setg(errp, "Not enough space in the bitmap directory");
@@ -1687,6 +1697,7 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
}
s->nb_queued_bitmaps += 1;
+ s->queued_directory_size += size_delta;
return 0;
fail:
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 5/5] block/qcow2-bitmap: Count queued bitmaps towards directory_size
2019-06-06 18:41 ` [Qemu-devel] [PATCH 5/5] block/qcow2-bitmap: Count queued bitmaps towards directory_size John Snow
@ 2019-06-07 2:30 ` Eric Blake
2019-06-07 19:24 ` John Snow
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2019-06-07 2:30 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz
[-- Attachment #1.1: Type: text/plain, Size: 1009 bytes --]
On 6/6/19 1:41 PM, John Snow wrote:
> Similarly to the previous commit, we need to also keep a ledger of the
> additional directory size burden that we've not yet committed so we can
> reject new additions sooner instead of later.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2.h | 1 +
> block/qcow2-bitmap.c | 13 ++++++++++++-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index ebf60ac236..5aff97eb9c 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -318,6 +318,7 @@ typedef struct BDRVQcow2State {
>
> uint32_t nb_bitmaps;
> uint32_t nb_queued_bitmaps;
> + uint32_t queued_directory_size;
> uint64_t bitmap_directory_size;
Why can we get away with uint32_t for the queue size, but uint64_t for
the stored size? Something feels fishy.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 5/5] block/qcow2-bitmap: Count queued bitmaps towards directory_size
2019-06-07 2:30 ` Eric Blake
@ 2019-06-07 19:24 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2019-06-07 19:24 UTC (permalink / raw)
To: Eric Blake, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, Markus Armbruster, Max Reitz
On 6/6/19 10:30 PM, Eric Blake wrote:
> On 6/6/19 1:41 PM, John Snow wrote:
>> Similarly to the previous commit, we need to also keep a ledger of the
>> additional directory size burden that we've not yet committed so we can
>> reject new additions sooner instead of later.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2.h | 1 +
>> block/qcow2-bitmap.c | 13 ++++++++++++-
>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index ebf60ac236..5aff97eb9c 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -318,6 +318,7 @@ typedef struct BDRVQcow2State {
>>
>> uint32_t nb_bitmaps;
>> uint32_t nb_queued_bitmaps;
>> + uint32_t queued_directory_size;
>> uint64_t bitmap_directory_size;
>
> Why can we get away with uint32_t for the queue size, but uint64_t for
> the stored size? Something feels fishy.
>
That would be me using the wrong type.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
2019-06-06 18:41 [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps John Snow
` (4 preceding siblings ...)
2019-06-06 18:41 ` [Qemu-devel] [PATCH 5/5] block/qcow2-bitmap: Count queued bitmaps towards directory_size John Snow
@ 2019-06-06 21:54 ` no-reply
2019-06-06 22:26 ` John Snow
2019-10-09 18:57 ` Eric Blake
6 siblings, 1 reply; 30+ messages in thread
From: no-reply @ 2019-06-06 21:54 UTC (permalink / raw)
To: jsnow
Cc: fam, kwolf, vsementsov, aliang, qemu-block, qemu-devel, armbru,
mreitz, jsnow
Patchew URL: https://patchew.org/QEMU/20190606184159.979-1-jsnow@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
Type: series
Message-id: 20190606184159.979-1-jsnow@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
From https://github.com/patchew-project/qemu
* [new tag] patchew/20190606184159.979-1-jsnow@redhat.com -> patchew/20190606184159.979-1-jsnow@redhat.com
Switched to a new branch 'test'
6d00dc95dd block/qcow2-bitmap: Count queued bitmaps towards directory_size
aaf1723431 block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
475f5eef64 block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
0698e46266 block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
e94f44cb88 block/qcow2-bitmap: allow bitmap_list_load to return an error code
=== OUTPUT BEGIN ===
1/5 Checking commit e94f44cb88bb (block/qcow2-bitmap: allow bitmap_list_load to return an error code)
ERROR: do not use assignment in if condition
#151: FILE: block/qcow2-bitmap.c:1130:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
ERROR: do not use assignment in if condition
#164: FILE: block/qcow2-bitmap.c:1189:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
total: 2 errors, 0 warnings, 166 lines checked
Patch 1/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/5 Checking commit 0698e462661d (block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap)
ERROR: do not use assignment in if condition
#166: FILE: block/qcow2-bitmap.c:1656:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
total: 1 errors, 0 warnings, 200 lines checked
Patch 2/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/5 Checking commit 475f5eef64a7 (block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap)
ERROR: do not use assignment in if condition
#91: FILE: block/qcow2-bitmap.c:1421:
+ if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
total: 1 errors, 0 warnings, 143 lines checked
Patch 3/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/5 Checking commit aaf172343148 (block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps)
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 0 warnings, 97 lines checked
Patch 4/5 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/5 Checking commit 6d00dc95ddc0 (block/qcow2-bitmap: Count queued bitmaps towards directory_size)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190606184159.979-1-jsnow@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
2019-06-06 21:54 ` [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps no-reply
@ 2019-06-06 22:26 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2019-06-06 22:26 UTC (permalink / raw)
To: qemu-devel; +Cc: fam, kwolf, vsementsov, aliang, qemu-block, armbru, mreitz
On 6/6/19 5:54 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190606184159.979-1-jsnow@redhat.com/
>
>
>
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Subject: [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
> Type: series
> Message-id: 20190606184159.979-1-jsnow@redhat.com
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
>
> From https://github.com/patchew-project/qemu
> * [new tag] patchew/20190606184159.979-1-jsnow@redhat.com -> patchew/20190606184159.979-1-jsnow@redhat.com
> Switched to a new branch 'test'
> 6d00dc95dd block/qcow2-bitmap: Count queued bitmaps towards directory_size
> aaf1723431 block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
> 475f5eef64 block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
> 0698e46266 block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
> e94f44cb88 block/qcow2-bitmap: allow bitmap_list_load to return an error code
>
> === OUTPUT BEGIN ===
> 1/5 Checking commit e94f44cb88bb (block/qcow2-bitmap: allow bitmap_list_load to return an error code)
> ERROR: do not use assignment in if condition
> #151: FILE: block/qcow2-bitmap.c:1130:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> ERROR: do not use assignment in if condition
> #164: FILE: block/qcow2-bitmap.c:1189:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> total: 2 errors, 0 warnings, 166 lines checked
>
> Patch 1/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 2/5 Checking commit 0698e462661d (block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap)
> ERROR: do not use assignment in if condition
> #166: FILE: block/qcow2-bitmap.c:1656:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> total: 1 errors, 0 warnings, 200 lines checked
>
> Patch 2/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 3/5 Checking commit 475f5eef64a7 (block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap)
> ERROR: do not use assignment in if condition
> #91: FILE: block/qcow2-bitmap.c:1421:
> + if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
>
> total: 1 errors, 0 warnings, 143 lines checked
>
> Patch 3/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 4/5 Checking commit aaf172343148 (block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps)
> ERROR: Missing Signed-off-by: line(s)
>
> total: 1 errors, 0 warnings, 97 lines checked
>
> Patch 4/5 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 5/5 Checking commit 6d00dc95ddc0 (block/qcow2-bitmap: Count queued bitmaps towards directory_size)
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> The full log is available at
> http://patchew.org/logs/20190606184159.979-1-jsnow@redhat.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
>
Whoops, script keeps dropping SoB for some reason. I also forgot that we
don't like assignment conditionals in QEMU, sorry :(
I'll fix these up, but I'll wait for other critique first so I don't
clog mailboxes.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
2019-06-06 18:41 [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps John Snow
` (5 preceding siblings ...)
2019-06-06 21:54 ` [Qemu-devel] [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps no-reply
@ 2019-10-09 18:57 ` Eric Blake
2019-10-09 20:44 ` John Snow
6 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2019-10-09 18:57 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, aihua liang,
Markus Armbruster, Max Reitz
On 6/6/19 1:41 PM, John Snow wrote:
> When adding new persistent dirty bitmaps, we only check constraints
> against currently stored bitmaps, and ignore the pending number and size
> of any bitmaps yet to be stored.
>
> Rework the "can_store" and "remove" interface to explicit "add" and "remove",
> and begin keeping track of the queued burden when adding new bitmaps.
>
> Reported-by: aihua liang <aliang@redhat.com>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636
>
> John Snow (5):
> block/qcow2-bitmap: allow bitmap_list_load to return an error code
> block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
> block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
> block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
> block/qcow2-bitmap: Count queued bitmaps towards directory_size
Is this series worth reviving as a v2, as it solves a corner-case bug?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
2019-10-09 18:57 ` Eric Blake
@ 2019-10-09 20:44 ` John Snow
2019-10-10 6:23 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2019-10-09 20:44 UTC (permalink / raw)
To: Eric Blake, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, vsementsov, aihua liang,
Markus Armbruster, Max Reitz
On 10/9/19 2:57 PM, Eric Blake wrote:
> On 6/6/19 1:41 PM, John Snow wrote:
>> When adding new persistent dirty bitmaps, we only check constraints
>> against currently stored bitmaps, and ignore the pending number and size
>> of any bitmaps yet to be stored.
>>
>> Rework the "can_store" and "remove" interface to explicit "add" and
>> "remove",
>> and begin keeping track of the queued burden when adding new bitmaps.
>>
>> Reported-by: aihua liang <aliang@redhat.com>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636
>>
>> John Snow (5):
>> block/qcow2-bitmap: allow bitmap_list_load to return an error code
>> block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
>> block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
>> block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
>> block/qcow2-bitmap: Count queued bitmaps towards directory_size
>
> Is this series worth reviving as a v2, as it solves a corner-case bug?
>
>
Yes, but IIRC there were some disagreements about the methodology for
the fix, but can't recall exactly what right now.
The way I remember it is that I wanted to make our qcow2 functions more
like "do_store_bitmap" and "do_remove_bitmap" for direct addition and
removal as I find that conceptual model 'simpler'.
(I think it had something to do with additional complexities in the
different contexts that list_load is used in for when and if it performs
certain consistency checks...?)
I think Vladimir wanted to use a pending-flush style cache to check
requirements against instead of actual writes.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/5] block/dirty-bitmap: check number and size constraints against queued bitmaps
2019-10-09 20:44 ` John Snow
@ 2019-10-10 6:23 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 30+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-10 6:23 UTC (permalink / raw)
To: John Snow, Eric Blake, qemu-block, qemu-devel
Cc: Fam Zheng, Kevin Wolf, aihua liang, Markus Armbruster, Max Reitz
09.10.2019 23:44, John Snow wrote:
>
>
> On 10/9/19 2:57 PM, Eric Blake wrote:
>> On 6/6/19 1:41 PM, John Snow wrote:
>>> When adding new persistent dirty bitmaps, we only check constraints
>>> against currently stored bitmaps, and ignore the pending number and size
>>> of any bitmaps yet to be stored.
>>>
>>> Rework the "can_store" and "remove" interface to explicit "add" and
>>> "remove",
>>> and begin keeping track of the queued burden when adding new bitmaps.
>>>
>>> Reported-by: aihua liang <aliang@redhat.com>
>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636
>>>
>>> John Snow (5):
>>> block/qcow2-bitmap: allow bitmap_list_load to return an error code
>>> block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap
>>> block/dirty-bitmap: rework bdrv_remove_persistent_dirty_bitmap
>>> block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps
>>> block/qcow2-bitmap: Count queued bitmaps towards directory_size
>>
>> Is this series worth reviving as a v2, as it solves a corner-case bug?
>>
>>
>
> Yes, but IIRC there were some disagreements about the methodology for
> the fix, but can't recall exactly what right now.
>
> The way I remember it is that I wanted to make our qcow2 functions more
> like "do_store_bitmap" and "do_remove_bitmap" for direct addition and
> removal as I find that conceptual model 'simpler'.
>
> (I think it had something to do with additional complexities in the
> different contexts that list_load is used in for when and if it performs
> certain consistency checks...?)
>
> I think Vladimir wanted to use a pending-flush style cache to check
> requirements against instead of actual writes.
>
Actually, here is my counter-proposal:
[PATCH] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap
<20190607184802.100945-1-vsementsov@virtuozzo.com>
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg01696.html
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 30+ messages in thread