18.05.2021 17:22, Max Reitz wrote:
On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
block-copy has a bit inconvenient interface around dirty bitmap: user
should get pointer to it and than set by hand. We do need a possibility
to share the bitmap with backup job.
But default of empty bitmap is strange.
I don’t know, I don’t find it strange. It expects its user to specify what
data to copy, so clearly it gives said user a blank slate.
Switch to full-set bitmap by
default. This way we will not care about setting dirty bitmap in
copy-before-write filter when publish it so that it can be used in
separate of backup job.
That’s a valid reason, though, so:
Reviewed-by: Max Reitz <[email protected]>
Still, I find it stranger this way, because I’m more used to “initialization to
0 by default”.
When I started to test this series it didn't work, because bitmap was not
initialized. I fixed it and call original behavior strange :)
Probably better to keep block-copy as is and set bitmap in cbw_open. Backup
code should be modified anyway, as it will get the bitmap after initialization
in cbw_open.
Next step would be support for bitmap in copy-before-write filter anyway, and
this place will be changed again.. So, I don't know, but it's not important
thing.
Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
---
block/backup.c | 16 +++++++---------
block/block-copy.c | 1 +
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 90cca1ca30..706c54fea1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -233,18 +233,16 @@ static void backup_init_bcs_bitmap(BackupBlockJob *job)
BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs);
if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
+ bdrv_clear_dirty_bitmap(bcs_bitmap, NULL);
ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap,
NULL, true);
assert(ret);
- } else {
- if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
- /*
- * We can't hog the coroutine to initialize this thoroughly.
- * Set a flag and resume work when we are able to yield safely.
- */
- block_copy_set_skip_unallocated(job->bcs, true);
- }
- bdrv_set_dirty_bitmap(bcs_bitmap, 0, job->len);
+ } else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
+ /*
+ * We can't hog the coroutine to initialize this thoroughly.
+ * Set a flag and resume work when we are able to yield safely.
+ */
+ block_copy_set_skip_unallocated(job->bcs, true);
}
estimate = bdrv_get_dirty_count(bcs_bitmap);
diff --git a/block/block-copy.c b/block/block-copy.c
index 9020234c6e..4126f7e8cc 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -296,6 +296,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source,
BdrvChild *target,
return NULL;
}
bdrv_disable_dirty_bitmap(copy_bitmap);
+ bdrv_set_dirty_bitmap(copy_bitmap, 0, bdrv_dirty_bitmap_size(copy_bitmap));
/*
* Why we always set BDRV_REQ_SERIALISING write flag:
--
Best regards,
Vladimir
