* [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring
2019-04-29 9:08 [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 1/5] block/backup: simplify backup_incremental_init_copy_bitmap Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, den, vsementsov, jsnow, mreitz
Hi all!
Here some refactoring patches, as a first step for backup-top filter
introduction.
v7:
01,03,04: add Max's r-b
05: fix s/return NULL/goto error/ [Max]
v6:
01: - use end_cluster instead of last_cluster and fix bug in
calculation [Max]
02: only rebased on 01, keep r-b
03, 04: new
05: it's rewritten
"[PATCH v5 10/11] block/backup: tiny refactor backup_job_create",
to make it more useful in general than just code movement inside
one function
Vladimir Sementsov-Ogievskiy (5):
block/backup: simplify backup_incremental_init_copy_bitmap
block/backup: move to copy_bitmap with granularity
block/backup: refactor and tolerate unallocated cluster skipping
block/backup: unify different modes code path
block/backup: refactor: split out backup_calculate_cluster_size
block/backup.c | 243 +++++++++++++++++++++----------------------------
1 file changed, 105 insertions(+), 138 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v7 1/5] block/backup: simplify backup_incremental_init_copy_bitmap
2019-04-29 9:08 [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 2/5] block/backup: move to copy_bitmap with granularity Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jsnow, vsementsov, den
Simplify backup_incremental_init_copy_bitmap using the function
bdrv_dirty_bitmap_next_dirty_area.
Note: move to job->len instead of bitmap size: it should not matter but
less code.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 40 ++++++++++++----------------------------
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 9988753249..d9f5db18ac 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -403,43 +403,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
/* init copy_bitmap from sync_bitmap */
static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
{
- BdrvDirtyBitmapIter *dbi;
- int64_t offset;
- int64_t end = DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
- job->cluster_size);
-
- dbi = bdrv_dirty_iter_new(job->sync_bitmap);
- while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
- int64_t cluster = offset / job->cluster_size;
- int64_t next_cluster;
-
- offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
- if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
- hbitmap_set(job->copy_bitmap, cluster, end - cluster);
- break;
- }
+ uint64_t offset = 0;
+ uint64_t bytes = job->len;
- offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset,
- UINT64_MAX);
- if (offset == -1) {
- hbitmap_set(job->copy_bitmap, cluster, end - cluster);
- break;
- }
+ while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
+ &offset, &bytes))
+ {
+ uint64_t cluster = offset / job->cluster_size;
+ uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size);
- next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
- hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
- if (next_cluster >= end) {
+ hbitmap_set(job->copy_bitmap, cluster, end_cluster - cluster);
+
+ offset = end_cluster * job->cluster_size;
+ if (offset >= job->len) {
break;
}
-
- bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
+ bytes = job->len - offset;
}
/* TODO job_progress_set_remaining() would make more sense */
job_progress_update(&job->common.job,
job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
-
- bdrv_dirty_iter_free(dbi);
}
static int coroutine_fn backup_run(Job *job, Error **errp)
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v7 1/5] block/backup: simplify backup_incremental_init_copy_bitmap
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 1/5] block/backup: simplify backup_incremental_init_copy_bitmap Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, den, vsementsov, jsnow, mreitz
Simplify backup_incremental_init_copy_bitmap using the function
bdrv_dirty_bitmap_next_dirty_area.
Note: move to job->len instead of bitmap size: it should not matter but
less code.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 40 ++++++++++++----------------------------
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 9988753249..d9f5db18ac 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -403,43 +403,27 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
/* init copy_bitmap from sync_bitmap */
static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
{
- BdrvDirtyBitmapIter *dbi;
- int64_t offset;
- int64_t end = DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
- job->cluster_size);
-
- dbi = bdrv_dirty_iter_new(job->sync_bitmap);
- while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
- int64_t cluster = offset / job->cluster_size;
- int64_t next_cluster;
-
- offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
- if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
- hbitmap_set(job->copy_bitmap, cluster, end - cluster);
- break;
- }
+ uint64_t offset = 0;
+ uint64_t bytes = job->len;
- offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset,
- UINT64_MAX);
- if (offset == -1) {
- hbitmap_set(job->copy_bitmap, cluster, end - cluster);
- break;
- }
+ while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
+ &offset, &bytes))
+ {
+ uint64_t cluster = offset / job->cluster_size;
+ uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size);
- next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
- hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
- if (next_cluster >= end) {
+ hbitmap_set(job->copy_bitmap, cluster, end_cluster - cluster);
+
+ offset = end_cluster * job->cluster_size;
+ if (offset >= job->len) {
break;
}
-
- bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
+ bytes = job->len - offset;
}
/* TODO job_progress_set_remaining() would make more sense */
job_progress_update(&job->common.job,
job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
-
- bdrv_dirty_iter_free(dbi);
}
static int coroutine_fn backup_run(Job *job, Error **errp)
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v7 2/5] block/backup: move to copy_bitmap with granularity
2019-04-29 9:08 [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 1/5] block/backup: simplify backup_incremental_init_copy_bitmap Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 3/5] block/backup: refactor and tolerate unallocated cluster skipping Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jsnow, vsementsov, den
We are going to share this bitmap between backup and backup-top filter
driver, so let's share something more meaningful. It also simplifies
some calculations.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 48 +++++++++++++++++++++++-------------------------
1 file changed, 23 insertions(+), 25 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index d9f5db18ac..510fc54f98 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -113,7 +113,8 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
- hbitmap_reset(job->copy_bitmap, start / job->cluster_size, 1);
+ assert(QEMU_IS_ALIGNED(start, job->cluster_size));
+ hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
nbytes = MIN(job->cluster_size, job->len - start);
if (!*bounce_buffer) {
*bounce_buffer = blk_blockalign(blk, job->cluster_size);
@@ -147,7 +148,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
return nbytes;
fail:
- hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
+ hbitmap_set(job->copy_bitmap, start, job->cluster_size);
return ret;
}
@@ -167,16 +168,15 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+ assert(QEMU_IS_ALIGNED(start, job->cluster_size));
nbytes = MIN(job->copy_range_size, end - start);
nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
- hbitmap_reset(job->copy_bitmap, start / job->cluster_size,
- nr_clusters);
+ hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
read_flags, write_flags);
if (ret < 0) {
trace_backup_do_cow_copy_range_fail(job, start, ret);
- hbitmap_set(job->copy_bitmap, start / job->cluster_size,
- nr_clusters);
+ hbitmap_set(job->copy_bitmap, start, job->cluster_size * nr_clusters);
return ret;
}
@@ -204,7 +204,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
cow_request_begin(&cow_request, job, start, end);
while (start < end) {
- if (!hbitmap_get(job->copy_bitmap, start / job->cluster_size)) {
+ if (!hbitmap_get(job->copy_bitmap, start)) {
trace_backup_do_cow_skip(job, start);
start += job->cluster_size;
continue; /* already copied */
@@ -300,6 +300,11 @@ static void backup_clean(Job *job)
assert(s->target);
blk_unref(s->target);
s->target = NULL;
+
+ if (s->copy_bitmap) {
+ hbitmap_free(s->copy_bitmap);
+ s->copy_bitmap = NULL;
+ }
}
static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
@@ -312,7 +317,6 @@ static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
void backup_do_checkpoint(BlockJob *job, Error **errp)
{
BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
- int64_t len;
assert(block_job_driver(job) == &backup_job_driver);
@@ -322,8 +326,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
return;
}
- len = DIV_ROUND_UP(backup_job->len, backup_job->cluster_size);
- hbitmap_set(backup_job->copy_bitmap, 0, len);
+ hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
}
static void backup_drain(BlockJob *job)
@@ -378,16 +381,16 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
{
int ret;
bool error_is_read;
- int64_t cluster;
+ int64_t offset;
HBitmapIter hbi;
hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
- while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
+ while ((offset = hbitmap_iter_next(&hbi)) != -1) {
do {
if (yield_and_check(job)) {
return 0;
}
- ret = backup_do_cow(job, cluster * job->cluster_size,
+ ret = backup_do_cow(job, offset,
job->cluster_size, &error_is_read, false);
if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
BLOCK_ERROR_ACTION_REPORT)
@@ -409,12 +412,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
&offset, &bytes))
{
- uint64_t cluster = offset / job->cluster_size;
- uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size);
+ hbitmap_set(job->copy_bitmap, offset, bytes);
- hbitmap_set(job->copy_bitmap, cluster, end_cluster - cluster);
-
- offset = end_cluster * job->cluster_size;
+ offset += bytes;
if (offset >= job->len) {
break;
}
@@ -423,30 +423,27 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
/* TODO job_progress_set_remaining() would make more sense */
job_progress_update(&job->common.job,
- job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
+ job->len - hbitmap_count(job->copy_bitmap));
}
static int coroutine_fn backup_run(Job *job, Error **errp)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
BlockDriverState *bs = blk_bs(s->common.blk);
- int64_t offset, nb_clusters;
+ int64_t offset;
int ret = 0;
QLIST_INIT(&s->inflight_reqs);
qemu_co_rwlock_init(&s->flush_rwlock);
- nb_clusters = DIV_ROUND_UP(s->len, s->cluster_size);
job_progress_set_remaining(job, s->len);
- s->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
backup_incremental_init_copy_bitmap(s);
} else {
- hbitmap_set(s->copy_bitmap, 0, nb_clusters);
+ hbitmap_set(s->copy_bitmap, 0, s->len);
}
-
s->before_write.notify = backup_before_write_notify;
bdrv_add_before_write_notifier(bs, &s->before_write);
@@ -527,7 +524,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&s->flush_rwlock);
qemu_co_rwlock_unlock(&s->flush_rwlock);
- hbitmap_free(s->copy_bitmap);
return ret;
}
@@ -678,6 +674,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
} else {
job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
}
+
+ job->copy_bitmap = hbitmap_alloc(len, ctz32(job->cluster_size));
job->use_copy_range = true;
job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
blk_get_max_transfer(job->target));
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v7 2/5] block/backup: move to copy_bitmap with granularity
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 2/5] block/backup: move to copy_bitmap with granularity Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, den, vsementsov, jsnow, mreitz
We are going to share this bitmap between backup and backup-top filter
driver, so let's share something more meaningful. It also simplifies
some calculations.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 48 +++++++++++++++++++++++-------------------------
1 file changed, 23 insertions(+), 25 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index d9f5db18ac..510fc54f98 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -113,7 +113,8 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
- hbitmap_reset(job->copy_bitmap, start / job->cluster_size, 1);
+ assert(QEMU_IS_ALIGNED(start, job->cluster_size));
+ hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
nbytes = MIN(job->cluster_size, job->len - start);
if (!*bounce_buffer) {
*bounce_buffer = blk_blockalign(blk, job->cluster_size);
@@ -147,7 +148,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
return nbytes;
fail:
- hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
+ hbitmap_set(job->copy_bitmap, start, job->cluster_size);
return ret;
}
@@ -167,16 +168,15 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+ assert(QEMU_IS_ALIGNED(start, job->cluster_size));
nbytes = MIN(job->copy_range_size, end - start);
nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
- hbitmap_reset(job->copy_bitmap, start / job->cluster_size,
- nr_clusters);
+ hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
read_flags, write_flags);
if (ret < 0) {
trace_backup_do_cow_copy_range_fail(job, start, ret);
- hbitmap_set(job->copy_bitmap, start / job->cluster_size,
- nr_clusters);
+ hbitmap_set(job->copy_bitmap, start, job->cluster_size * nr_clusters);
return ret;
}
@@ -204,7 +204,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
cow_request_begin(&cow_request, job, start, end);
while (start < end) {
- if (!hbitmap_get(job->copy_bitmap, start / job->cluster_size)) {
+ if (!hbitmap_get(job->copy_bitmap, start)) {
trace_backup_do_cow_skip(job, start);
start += job->cluster_size;
continue; /* already copied */
@@ -300,6 +300,11 @@ static void backup_clean(Job *job)
assert(s->target);
blk_unref(s->target);
s->target = NULL;
+
+ if (s->copy_bitmap) {
+ hbitmap_free(s->copy_bitmap);
+ s->copy_bitmap = NULL;
+ }
}
static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
@@ -312,7 +317,6 @@ static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
void backup_do_checkpoint(BlockJob *job, Error **errp)
{
BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
- int64_t len;
assert(block_job_driver(job) == &backup_job_driver);
@@ -322,8 +326,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
return;
}
- len = DIV_ROUND_UP(backup_job->len, backup_job->cluster_size);
- hbitmap_set(backup_job->copy_bitmap, 0, len);
+ hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
}
static void backup_drain(BlockJob *job)
@@ -378,16 +381,16 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
{
int ret;
bool error_is_read;
- int64_t cluster;
+ int64_t offset;
HBitmapIter hbi;
hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
- while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
+ while ((offset = hbitmap_iter_next(&hbi)) != -1) {
do {
if (yield_and_check(job)) {
return 0;
}
- ret = backup_do_cow(job, cluster * job->cluster_size,
+ ret = backup_do_cow(job, offset,
job->cluster_size, &error_is_read, false);
if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
BLOCK_ERROR_ACTION_REPORT)
@@ -409,12 +412,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
&offset, &bytes))
{
- uint64_t cluster = offset / job->cluster_size;
- uint64_t end_cluster = DIV_ROUND_UP(offset + bytes, job->cluster_size);
+ hbitmap_set(job->copy_bitmap, offset, bytes);
- hbitmap_set(job->copy_bitmap, cluster, end_cluster - cluster);
-
- offset = end_cluster * job->cluster_size;
+ offset += bytes;
if (offset >= job->len) {
break;
}
@@ -423,30 +423,27 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
/* TODO job_progress_set_remaining() would make more sense */
job_progress_update(&job->common.job,
- job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
+ job->len - hbitmap_count(job->copy_bitmap));
}
static int coroutine_fn backup_run(Job *job, Error **errp)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
BlockDriverState *bs = blk_bs(s->common.blk);
- int64_t offset, nb_clusters;
+ int64_t offset;
int ret = 0;
QLIST_INIT(&s->inflight_reqs);
qemu_co_rwlock_init(&s->flush_rwlock);
- nb_clusters = DIV_ROUND_UP(s->len, s->cluster_size);
job_progress_set_remaining(job, s->len);
- s->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
backup_incremental_init_copy_bitmap(s);
} else {
- hbitmap_set(s->copy_bitmap, 0, nb_clusters);
+ hbitmap_set(s->copy_bitmap, 0, s->len);
}
-
s->before_write.notify = backup_before_write_notify;
bdrv_add_before_write_notifier(bs, &s->before_write);
@@ -527,7 +524,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&s->flush_rwlock);
qemu_co_rwlock_unlock(&s->flush_rwlock);
- hbitmap_free(s->copy_bitmap);
return ret;
}
@@ -678,6 +674,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
} else {
job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
}
+
+ job->copy_bitmap = hbitmap_alloc(len, ctz32(job->cluster_size));
job->use_copy_range = true;
job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
blk_get_max_transfer(job->target));
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v7 3/5] block/backup: refactor and tolerate unallocated cluster skipping
2019-04-29 9:08 [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 2/5] block/backup: move to copy_bitmap with granularity Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 4/5] block/backup: unify different modes code path Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jsnow, vsementsov, den
Split allocation checking to separate function and reduce nesting.
Consider bdrv_is_allocated() fail as allocated area, as copying more
than needed is not wrong (and we do it anyway) and seems better than
fail the whole job. And, most probably we will fail on the next read,
if there are real problem with source.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 60 +++++++++++++++++++-------------------------------
1 file changed, 23 insertions(+), 37 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 510fc54f98..298e85f1a9 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -377,6 +377,22 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job)
return false;
}
+static bool bdrv_is_unallocated_range(BlockDriverState *bs,
+ int64_t offset, int64_t bytes)
+{
+ int64_t end = offset + bytes;
+
+ while (offset < end && !bdrv_is_allocated(bs, offset, bytes, &bytes)) {
+ if (bytes == 0) {
+ return true;
+ }
+ offset += bytes;
+ bytes = end - offset;
+ }
+
+ return offset >= end;
+}
+
static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
{
int ret;
@@ -462,49 +478,19 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
for (offset = 0; offset < s->len;
offset += s->cluster_size) {
bool error_is_read;
- int alloced = 0;
if (yield_and_check(s)) {
break;
}
- if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
- int i;
- int64_t n;
-
- /* Check to see if these blocks are already in the
- * backing file. */
-
- for (i = 0; i < s->cluster_size;) {
- /* bdrv_is_allocated() only returns true/false based
- * on the first set of sectors it comes across that
- * are are all in the same state.
- * For that reason we must verify each sector in the
- * backup cluster length. We end up copying more than
- * needed but at some point that is always the case. */
- alloced =
- bdrv_is_allocated(bs, offset + i,
- s->cluster_size - i, &n);
- i += n;
-
- if (alloced || n == 0) {
- break;
- }
- }
-
- /* If the above loop never found any sectors that are in
- * the topmost image, skip this backup. */
- if (alloced == 0) {
- continue;
- }
- }
- /* FULL sync mode we copy the whole drive. */
- if (alloced < 0) {
- ret = alloced;
- } else {
- ret = backup_do_cow(s, offset, s->cluster_size,
- &error_is_read, false);
+ if (s->sync_mode == MIRROR_SYNC_MODE_TOP &&
+ bdrv_is_unallocated_range(bs, offset, s->cluster_size))
+ {
+ continue;
}
+
+ ret = backup_do_cow(s, offset, s->cluster_size,
+ &error_is_read, false);
if (ret < 0) {
/* Depending on error action, fail now or retry cluster */
BlockErrorAction action =
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v7 3/5] block/backup: refactor and tolerate unallocated cluster skipping
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 3/5] block/backup: refactor and tolerate unallocated cluster skipping Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, den, vsementsov, jsnow, mreitz
Split allocation checking to separate function and reduce nesting.
Consider bdrv_is_allocated() fail as allocated area, as copying more
than needed is not wrong (and we do it anyway) and seems better than
fail the whole job. And, most probably we will fail on the next read,
if there are real problem with source.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 60 +++++++++++++++++++-------------------------------
1 file changed, 23 insertions(+), 37 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 510fc54f98..298e85f1a9 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -377,6 +377,22 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job)
return false;
}
+static bool bdrv_is_unallocated_range(BlockDriverState *bs,
+ int64_t offset, int64_t bytes)
+{
+ int64_t end = offset + bytes;
+
+ while (offset < end && !bdrv_is_allocated(bs, offset, bytes, &bytes)) {
+ if (bytes == 0) {
+ return true;
+ }
+ offset += bytes;
+ bytes = end - offset;
+ }
+
+ return offset >= end;
+}
+
static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
{
int ret;
@@ -462,49 +478,19 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
for (offset = 0; offset < s->len;
offset += s->cluster_size) {
bool error_is_read;
- int alloced = 0;
if (yield_and_check(s)) {
break;
}
- if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
- int i;
- int64_t n;
-
- /* Check to see if these blocks are already in the
- * backing file. */
-
- for (i = 0; i < s->cluster_size;) {
- /* bdrv_is_allocated() only returns true/false based
- * on the first set of sectors it comes across that
- * are are all in the same state.
- * For that reason we must verify each sector in the
- * backup cluster length. We end up copying more than
- * needed but at some point that is always the case. */
- alloced =
- bdrv_is_allocated(bs, offset + i,
- s->cluster_size - i, &n);
- i += n;
-
- if (alloced || n == 0) {
- break;
- }
- }
-
- /* If the above loop never found any sectors that are in
- * the topmost image, skip this backup. */
- if (alloced == 0) {
- continue;
- }
- }
- /* FULL sync mode we copy the whole drive. */
- if (alloced < 0) {
- ret = alloced;
- } else {
- ret = backup_do_cow(s, offset, s->cluster_size,
- &error_is_read, false);
+ if (s->sync_mode == MIRROR_SYNC_MODE_TOP &&
+ bdrv_is_unallocated_range(bs, offset, s->cluster_size))
+ {
+ continue;
}
+
+ ret = backup_do_cow(s, offset, s->cluster_size,
+ &error_is_read, false);
if (ret < 0) {
/* Depending on error action, fail now or retry cluster */
BlockErrorAction action =
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v7 4/5] block/backup: unify different modes code path
2019-04-29 9:08 [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 3/5] block/backup: refactor and tolerate unallocated cluster skipping Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size Vladimir Sementsov-Ogievskiy
2019-05-13 13:41 ` [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Max Reitz
6 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jsnow, vsementsov, den
Do full, top and incremental mode copying all in one place. This
unifies the code path and helps further improvements.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 43 ++++++++++---------------------------------
1 file changed, 10 insertions(+), 33 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 298e85f1a9..b54386b699 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -393,15 +393,23 @@ static bool bdrv_is_unallocated_range(BlockDriverState *bs,
return offset >= end;
}
-static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+static int coroutine_fn backup_loop(BackupBlockJob *job)
{
int ret;
bool error_is_read;
int64_t offset;
HBitmapIter hbi;
+ BlockDriverState *bs = blk_bs(job->common.blk);
hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
while ((offset = hbitmap_iter_next(&hbi)) != -1) {
+ if (job->sync_mode == MIRROR_SYNC_MODE_TOP &&
+ bdrv_is_unallocated_range(bs, offset, job->cluster_size))
+ {
+ hbitmap_reset(job->copy_bitmap, offset, job->cluster_size);
+ continue;
+ }
+
do {
if (yield_and_check(job)) {
return 0;
@@ -446,7 +454,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
BlockDriverState *bs = blk_bs(s->common.blk);
- int64_t offset;
int ret = 0;
QLIST_INIT(&s->inflight_reqs);
@@ -471,38 +478,8 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
* notify callback service CoW requests. */
job_yield(job);
}
- } else if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
- ret = backup_run_incremental(s);
} else {
- /* Both FULL and TOP SYNC_MODE's require copying.. */
- for (offset = 0; offset < s->len;
- offset += s->cluster_size) {
- bool error_is_read;
-
- if (yield_and_check(s)) {
- break;
- }
-
- if (s->sync_mode == MIRROR_SYNC_MODE_TOP &&
- bdrv_is_unallocated_range(bs, offset, s->cluster_size))
- {
- continue;
- }
-
- ret = backup_do_cow(s, offset, s->cluster_size,
- &error_is_read, false);
- if (ret < 0) {
- /* Depending on error action, fail now or retry cluster */
- BlockErrorAction action =
- backup_error_action(s, error_is_read, -ret);
- if (action == BLOCK_ERROR_ACTION_REPORT) {
- break;
- } else {
- offset -= s->cluster_size;
- continue;
- }
- }
- }
+ ret = backup_loop(s);
}
notifier_with_return_remove(&s->before_write);
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v7 4/5] block/backup: unify different modes code path
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 4/5] block/backup: unify different modes code path Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, den, vsementsov, jsnow, mreitz
Do full, top and incremental mode copying all in one place. This
unifies the code path and helps further improvements.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 43 ++++++++++---------------------------------
1 file changed, 10 insertions(+), 33 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 298e85f1a9..b54386b699 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -393,15 +393,23 @@ static bool bdrv_is_unallocated_range(BlockDriverState *bs,
return offset >= end;
}
-static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
+static int coroutine_fn backup_loop(BackupBlockJob *job)
{
int ret;
bool error_is_read;
int64_t offset;
HBitmapIter hbi;
+ BlockDriverState *bs = blk_bs(job->common.blk);
hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
while ((offset = hbitmap_iter_next(&hbi)) != -1) {
+ if (job->sync_mode == MIRROR_SYNC_MODE_TOP &&
+ bdrv_is_unallocated_range(bs, offset, job->cluster_size))
+ {
+ hbitmap_reset(job->copy_bitmap, offset, job->cluster_size);
+ continue;
+ }
+
do {
if (yield_and_check(job)) {
return 0;
@@ -446,7 +454,6 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
BlockDriverState *bs = blk_bs(s->common.blk);
- int64_t offset;
int ret = 0;
QLIST_INIT(&s->inflight_reqs);
@@ -471,38 +478,8 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
* notify callback service CoW requests. */
job_yield(job);
}
- } else if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
- ret = backup_run_incremental(s);
} else {
- /* Both FULL and TOP SYNC_MODE's require copying.. */
- for (offset = 0; offset < s->len;
- offset += s->cluster_size) {
- bool error_is_read;
-
- if (yield_and_check(s)) {
- break;
- }
-
- if (s->sync_mode == MIRROR_SYNC_MODE_TOP &&
- bdrv_is_unallocated_range(bs, offset, s->cluster_size))
- {
- continue;
- }
-
- ret = backup_do_cow(s, offset, s->cluster_size,
- &error_is_read, false);
- if (ret < 0) {
- /* Depending on error action, fail now or retry cluster */
- BlockErrorAction action =
- backup_error_action(s, error_is_read, -ret);
- if (action == BLOCK_ERROR_ACTION_REPORT) {
- break;
- } else {
- offset -= s->cluster_size;
- continue;
- }
- }
- }
+ ret = backup_loop(s);
}
notifier_with_return_remove(&s->before_write);
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size
2019-04-29 9:08 [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 4/5] block/backup: unify different modes code path Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-05-09 16:12 ` Max Reitz
2019-05-13 13:41 ` [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Max Reitz
6 siblings, 2 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, jsnow, vsementsov, den
Split out cluster_size calculation. Move copy-bitmap creation above
block-job creation, as we are going to share it with upcoming
backup-top filter, which also should be created before actual block job
creation.
Also, while being here, drop unnecessary "goto error" from
bdrv_getlength error path.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/backup.c | 82 ++++++++++++++++++++++++++++++++------------------
1 file changed, 52 insertions(+), 30 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index b54386b699..d1b94a6dbe 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -507,6 +507,42 @@ static const BlockJobDriver backup_job_driver = {
.drain = backup_drain,
};
+static int64_t backup_calculate_cluster_size(BlockDriverState *target,
+ Error **errp)
+{
+ int ret;
+ BlockDriverInfo bdi;
+
+ /*
+ * If there is no backing file on the target, we cannot rely on COW if our
+ * backup cluster size is smaller than the target cluster size. Even for
+ * targets with a backing file, try to avoid COW if possible.
+ */
+ ret = bdrv_get_info(target, &bdi);
+ if (ret == -ENOTSUP && !target->backing) {
+ /* Cluster size is not defined */
+ warn_report("The target block device doesn't provide "
+ "information about the block size and it doesn't have a "
+ "backing file. The default block size of %u bytes is "
+ "used. If the actual block size of the target exceeds "
+ "this default, the backup may be unusable",
+ BACKUP_CLUSTER_SIZE_DEFAULT);
+ return BACKUP_CLUSTER_SIZE_DEFAULT;
+ } else if (ret < 0 && !target->backing) {
+ error_setg_errno(errp, -ret,
+ "Couldn't determine the cluster size of the target image, "
+ "which has no backing file");
+ error_append_hint(errp,
+ "Aborting, since this may create an unusable destination image\n");
+ return ret;
+ } else if (ret < 0 && target->backing) {
+ /* Not fatal; just trudge on ahead. */
+ return BACKUP_CLUSTER_SIZE_DEFAULT;
+ }
+
+ return MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+}
+
BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
BlockDriverState *target, int64_t speed,
MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
@@ -518,9 +554,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
JobTxn *txn, Error **errp)
{
int64_t len;
- BlockDriverInfo bdi;
BackupBlockJob *job = NULL;
int ret;
+ int64_t cluster_size;
+ HBitmap *copy_bitmap = NULL;
assert(bs);
assert(target);
@@ -582,6 +619,13 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
goto error;
}
+ cluster_size = backup_calculate_cluster_size(target, errp);
+ if (cluster_size < 0) {
+ goto error;
+ }
+
+ copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
+
/* job->len is fixed, so we can't allow resize */
job = block_job_create(job_id, &backup_job_driver, txn, bs,
BLK_PERM_CONSISTENT_READ,
@@ -610,35 +654,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
/* Detect image-fleecing (and similar) schemes */
job->serialize_target_writes = bdrv_chain_contains(target, bs);
-
- /* If there is no backing file on the target, we cannot rely on COW if our
- * backup cluster size is smaller than the target cluster size. Even for
- * targets with a backing file, try to avoid COW if possible. */
- ret = bdrv_get_info(target, &bdi);
- if (ret == -ENOTSUP && !target->backing) {
- /* Cluster size is not defined */
- warn_report("The target block device doesn't provide "
- "information about the block size and it doesn't have a "
- "backing file. The default block size of %u bytes is "
- "used. If the actual block size of the target exceeds "
- "this default, the backup may be unusable",
- BACKUP_CLUSTER_SIZE_DEFAULT);
- job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
- } else if (ret < 0 && !target->backing) {
- error_setg_errno(errp, -ret,
- "Couldn't determine the cluster size of the target image, "
- "which has no backing file");
- error_append_hint(errp,
- "Aborting, since this may create an unusable destination image\n");
- goto error;
- } else if (ret < 0 && target->backing) {
- /* Not fatal; just trudge on ahead. */
- job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
- } else {
- job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
- }
-
- job->copy_bitmap = hbitmap_alloc(len, ctz32(job->cluster_size));
+ job->cluster_size = cluster_size;
+ job->copy_bitmap = copy_bitmap;
+ copy_bitmap = NULL;
job->use_copy_range = true;
job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
blk_get_max_transfer(job->target));
@@ -654,6 +672,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
return &job->common;
error:
+ if (copy_bitmap) {
+ assert(!job || !job->copy_bitmap);
+ hbitmap_free(copy_bitmap);
+ }
if (sync_bitmap) {
bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size Vladimir Sementsov-Ogievskiy
@ 2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
2019-05-09 16:12 ` Max Reitz
1 sibling, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-04-29 9:08 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, den, vsementsov, jsnow, mreitz
Split out cluster_size calculation. Move copy-bitmap creation above
block-job creation, as we are going to share it with upcoming
backup-top filter, which also should be created before actual block job
creation.
Also, while being here, drop unnecessary "goto error" from
bdrv_getlength error path.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/backup.c | 82 ++++++++++++++++++++++++++++++++------------------
1 file changed, 52 insertions(+), 30 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index b54386b699..d1b94a6dbe 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -507,6 +507,42 @@ static const BlockJobDriver backup_job_driver = {
.drain = backup_drain,
};
+static int64_t backup_calculate_cluster_size(BlockDriverState *target,
+ Error **errp)
+{
+ int ret;
+ BlockDriverInfo bdi;
+
+ /*
+ * If there is no backing file on the target, we cannot rely on COW if our
+ * backup cluster size is smaller than the target cluster size. Even for
+ * targets with a backing file, try to avoid COW if possible.
+ */
+ ret = bdrv_get_info(target, &bdi);
+ if (ret == -ENOTSUP && !target->backing) {
+ /* Cluster size is not defined */
+ warn_report("The target block device doesn't provide "
+ "information about the block size and it doesn't have a "
+ "backing file. The default block size of %u bytes is "
+ "used. If the actual block size of the target exceeds "
+ "this default, the backup may be unusable",
+ BACKUP_CLUSTER_SIZE_DEFAULT);
+ return BACKUP_CLUSTER_SIZE_DEFAULT;
+ } else if (ret < 0 && !target->backing) {
+ error_setg_errno(errp, -ret,
+ "Couldn't determine the cluster size of the target image, "
+ "which has no backing file");
+ error_append_hint(errp,
+ "Aborting, since this may create an unusable destination image\n");
+ return ret;
+ } else if (ret < 0 && target->backing) {
+ /* Not fatal; just trudge on ahead. */
+ return BACKUP_CLUSTER_SIZE_DEFAULT;
+ }
+
+ return MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+}
+
BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
BlockDriverState *target, int64_t speed,
MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
@@ -518,9 +554,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
JobTxn *txn, Error **errp)
{
int64_t len;
- BlockDriverInfo bdi;
BackupBlockJob *job = NULL;
int ret;
+ int64_t cluster_size;
+ HBitmap *copy_bitmap = NULL;
assert(bs);
assert(target);
@@ -582,6 +619,13 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
goto error;
}
+ cluster_size = backup_calculate_cluster_size(target, errp);
+ if (cluster_size < 0) {
+ goto error;
+ }
+
+ copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
+
/* job->len is fixed, so we can't allow resize */
job = block_job_create(job_id, &backup_job_driver, txn, bs,
BLK_PERM_CONSISTENT_READ,
@@ -610,35 +654,9 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
/* Detect image-fleecing (and similar) schemes */
job->serialize_target_writes = bdrv_chain_contains(target, bs);
-
- /* If there is no backing file on the target, we cannot rely on COW if our
- * backup cluster size is smaller than the target cluster size. Even for
- * targets with a backing file, try to avoid COW if possible. */
- ret = bdrv_get_info(target, &bdi);
- if (ret == -ENOTSUP && !target->backing) {
- /* Cluster size is not defined */
- warn_report("The target block device doesn't provide "
- "information about the block size and it doesn't have a "
- "backing file. The default block size of %u bytes is "
- "used. If the actual block size of the target exceeds "
- "this default, the backup may be unusable",
- BACKUP_CLUSTER_SIZE_DEFAULT);
- job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
- } else if (ret < 0 && !target->backing) {
- error_setg_errno(errp, -ret,
- "Couldn't determine the cluster size of the target image, "
- "which has no backing file");
- error_append_hint(errp,
- "Aborting, since this may create an unusable destination image\n");
- goto error;
- } else if (ret < 0 && target->backing) {
- /* Not fatal; just trudge on ahead. */
- job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
- } else {
- job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
- }
-
- job->copy_bitmap = hbitmap_alloc(len, ctz32(job->cluster_size));
+ job->cluster_size = cluster_size;
+ job->copy_bitmap = copy_bitmap;
+ copy_bitmap = NULL;
job->use_copy_range = true;
job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
blk_get_max_transfer(job->target));
@@ -654,6 +672,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
return &job->common;
error:
+ if (copy_bitmap) {
+ assert(!job || !job->copy_bitmap);
+ hbitmap_free(copy_bitmap);
+ }
if (sync_bitmap) {
bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
}
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size Vladimir Sementsov-Ogievskiy
2019-04-29 9:08 ` Vladimir Sementsov-Ogievskiy
@ 2019-05-09 16:12 ` Max Reitz
2019-05-13 9:54 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 15+ messages in thread
From: Max Reitz @ 2019-05-09 16:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: kwolf, den, jsnow
[-- Attachment #1: Type: text/plain, Size: 664 bytes --]
On 29.04.19 11:08, Vladimir Sementsov-Ogievskiy wrote:
> Split out cluster_size calculation. Move copy-bitmap creation above
> block-job creation, as we are going to share it with upcoming
> backup-top filter, which also should be created before actual block job
> creation.
>
> Also, while being here, drop unnecessary "goto error" from
> bdrv_getlength error path.
This paragraph should be dropped now. Do you mind me doing it?
Max
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/backup.c | 82 ++++++++++++++++++++++++++++++++------------------
> 1 file changed, 52 insertions(+), 30 deletions(-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size
2019-05-09 16:12 ` Max Reitz
@ 2019-05-13 9:54 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-13 9:54 UTC (permalink / raw)
To: Max Reitz, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: kwolf@redhat.com, jsnow@redhat.com, Denis Lunev
09.05.2019 19:12, Max Reitz wrote:
> On 29.04.19 11:08, Vladimir Sementsov-Ogievskiy wrote:
>> Split out cluster_size calculation. Move copy-bitmap creation above
>> block-job creation, as we are going to share it with upcoming
>> backup-top filter, which also should be created before actual block job
>> creation.
>>
>> Also, while being here, drop unnecessary "goto error" from
>> bdrv_getlength error path.
>
> This paragraph should be dropped now. Do you mind me doing it?
>
Of course, don't mind)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring
2019-04-29 9:08 [Qemu-devel] [PATCH v7 0/5] backup-top: preparing refactoring Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2019-04-29 9:08 ` [Qemu-devel] [PATCH v7 5/5] block/backup: refactor: split out backup_calculate_cluster_size Vladimir Sementsov-Ogievskiy
@ 2019-05-13 13:41 ` Max Reitz
6 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2019-05-13 13:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: kwolf, den, jsnow
[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]
On 29.04.19 11:08, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here some refactoring patches, as a first step for backup-top filter
> introduction.
>
> v7:
> 01,03,04: add Max's r-b
> 05: fix s/return NULL/goto error/ [Max]
>
> v6:
> 01: - use end_cluster instead of last_cluster and fix bug in
> calculation [Max]
> 02: only rebased on 01, keep r-b
> 03, 04: new
> 05: it's rewritten
> "[PATCH v5 10/11] block/backup: tiny refactor backup_job_create",
> to make it more useful in general than just code movement inside
> one function
>
> Vladimir Sementsov-Ogievskiy (5):
> block/backup: simplify backup_incremental_init_copy_bitmap
> block/backup: move to copy_bitmap with granularity
> block/backup: refactor and tolerate unallocated cluster skipping
> block/backup: unify different modes code path
> block/backup: refactor: split out backup_calculate_cluster_size
>
> block/backup.c | 243 +++++++++++++++++++++----------------------------
> 1 file changed, 105 insertions(+), 138 deletions(-)
Thanks, I’ve fixed patch 5’s commit message and applied the series to my
block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread