| Message ID | 1482503344-6424-2-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 23, 2016 at 05:28:44PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Use HBitmap copy_bitmap instead of done_bitmap. This is needed to unify > backup loop for full/incremental modes in future patches. > > We reset bit of the copy_bitmap immediately after checking it in > backup_do_cow(). It is safe, because all other intersecting requests > will wait for our request finish anyway. > > The other difference is that in case of error we will have zeroed bit in > copy_bitmap, when in done_bitmap we have not set bit. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/backup.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index ea38733..6b27e55 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -39,11 +39,12 @@ typedef struct BackupBlockJob { > BlockdevOnError on_target_error; > CoRwlock flush_rwlock; > uint64_t sectors_read; > - unsigned long *done_bitmap; > int64_t cluster_size; > bool compress; > NotifierWithReturn before_write; > QLIST_HEAD(, CowRequest) inflight_reqs; > + > + HBitmap *copy_bitmap; > } BackupBlockJob; > > /* Size of a cluster in sectors, instead of bytes. */ > @@ -115,10 +116,11 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > cow_request_begin(&cow_request, job, start, end); > > for (; start < end; start++) { > - if (test_bit(start, job->done_bitmap)) { > + if (!hbitmap_get(job->copy_bitmap, start)) { > trace_backup_do_cow_skip(job, start); > continue; /* already copied */ > } > + hbitmap_reset(job->copy_bitmap, start, 1); > > trace_backup_do_cow_process(job, start); > > @@ -141,6 +143,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > if (error_is_read) { > *error_is_read = true; > } > + hbitmap_set(job->copy_bitmap, start, 1); > goto out; > } > > @@ -157,11 +160,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > if (error_is_read) { > *error_is_read = false; > } > + hbitmap_set(job->copy_bitmap, start, 1); > goto out; > } > > - set_bit(start, job->done_bitmap); > - > /* Publish progress, guest I/O counts as progress too. Note that the > * offset field is an opaque progress value, it is not a disk offset. > */ > @@ -271,7 +273,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp) > } > > len = DIV_ROUND_UP(backup_job->common.len, backup_job->cluster_size); > - bitmap_zero(backup_job->done_bitmap, len); > + hbitmap_set(backup_job->copy_bitmap, 0, len); > } > > void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num, > @@ -450,7 +452,8 @@ static void coroutine_fn backup_run(void *opaque) > start = 0; > end = DIV_ROUND_UP(job->common.len, job->cluster_size); > > - job->done_bitmap = bitmap_new(end); > + job->copy_bitmap = hbitmap_alloc(end, 0); > + hbitmap_set(job->copy_bitmap, 0, end); > > job->before_write.notify = backup_before_write_notify; > bdrv_add_before_write_notifier(bs, &job->before_write); > @@ -524,7 +527,7 @@ static void coroutine_fn backup_run(void *opaque) > /* wait until pending backup_do_cow() calls have completed */ > qemu_co_rwlock_wrlock(&job->flush_rwlock); > qemu_co_rwlock_unlock(&job->flush_rwlock); > - g_free(job->done_bitmap); > + hbitmap_free(job->copy_bitmap); > > data = g_malloc(sizeof(*data)); > data->ret = ret; > -- > 1.8.3.1 > Reviewed-by: Jeff Cody <jcody@redhat.com>
23.12.2016 17:28, Vladimir Sementsov-Ogievskiy wrote: > Use HBitmap copy_bitmap instead of done_bitmap. This is needed to unify > backup loop for full/incremental modes in future patches. > > We reset bit of the copy_bitmap immediately after checking it in > backup_do_cow(). It is safe, because all other intersecting requests > will wait for our request finish anyway. > > The other difference is that in case of error we will have zeroed bit in > copy_bitmap, when in done_bitmap we have not set bit. This sentence is unrelated. It actually relates to old my mistake (not published versions), which is fixed here. Really, in case of error I set bit in copy_bitmap back, to correspond to old behavior with done_bitmap. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/backup.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index ea38733..6b27e55 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -39,11 +39,12 @@ typedef struct BackupBlockJob { > BlockdevOnError on_target_error; > CoRwlock flush_rwlock; > uint64_t sectors_read; > - unsigned long *done_bitmap; > int64_t cluster_size; > bool compress; > NotifierWithReturn before_write; > QLIST_HEAD(, CowRequest) inflight_reqs; > + > + HBitmap *copy_bitmap; > } BackupBlockJob; > > /* Size of a cluster in sectors, instead of bytes. */ > @@ -115,10 +116,11 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > cow_request_begin(&cow_request, job, start, end); > > for (; start < end; start++) { > - if (test_bit(start, job->done_bitmap)) { > + if (!hbitmap_get(job->copy_bitmap, start)) { > trace_backup_do_cow_skip(job, start); > continue; /* already copied */ > } > + hbitmap_reset(job->copy_bitmap, start, 1); > > trace_backup_do_cow_process(job, start); > > @@ -141,6 +143,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > if (error_is_read) { > *error_is_read = true; > } > + hbitmap_set(job->copy_bitmap, start, 1); > goto out; > } > > @@ -157,11 +160,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, > if (error_is_read) { > *error_is_read = false; > } > + hbitmap_set(job->copy_bitmap, start, 1); > goto out; > } > > - set_bit(start, job->done_bitmap); > - > /* Publish progress, guest I/O counts as progress too. Note that the > * offset field is an opaque progress value, it is not a disk offset. > */ > @@ -271,7 +273,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp) > } > > len = DIV_ROUND_UP(backup_job->common.len, backup_job->cluster_size); > - bitmap_zero(backup_job->done_bitmap, len); > + hbitmap_set(backup_job->copy_bitmap, 0, len); > } > > void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num, > @@ -450,7 +452,8 @@ static void coroutine_fn backup_run(void *opaque) > start = 0; > end = DIV_ROUND_UP(job->common.len, job->cluster_size); > > - job->done_bitmap = bitmap_new(end); > + job->copy_bitmap = hbitmap_alloc(end, 0); > + hbitmap_set(job->copy_bitmap, 0, end); > > job->before_write.notify = backup_before_write_notify; > bdrv_add_before_write_notifier(bs, &job->before_write); > @@ -524,7 +527,7 @@ static void coroutine_fn backup_run(void *opaque) > /* wait until pending backup_do_cow() calls have completed */ > qemu_co_rwlock_wrlock(&job->flush_rwlock); > qemu_co_rwlock_unlock(&job->flush_rwlock); > - g_free(job->done_bitmap); > + hbitmap_free(job->copy_bitmap); > > data = g_malloc(sizeof(*data)); > data->ret = ret;
On Fri, Dec 23, 2016 at 05:28:44PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Use HBitmap copy_bitmap instead of done_bitmap. This is needed to unify > backup loop for full/incremental modes in future patches. > > We reset bit of the copy_bitmap immediately after checking it in > backup_do_cow(). It is safe, because all other intersecting requests > will wait for our request finish anyway. > > The other difference is that in case of error we will have zeroed bit in > copy_bitmap, when in done_bitmap we have not set bit. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/backup.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/block/backup.c b/block/backup.c index ea38733..6b27e55 100644 --- a/block/backup.c +++ b/block/backup.c @@ -39,11 +39,12 @@ typedef struct BackupBlockJob { BlockdevOnError on_target_error; CoRwlock flush_rwlock; uint64_t sectors_read; - unsigned long *done_bitmap; int64_t cluster_size; bool compress; NotifierWithReturn before_write; QLIST_HEAD(, CowRequest) inflight_reqs; + + HBitmap *copy_bitmap; } BackupBlockJob; /* Size of a cluster in sectors, instead of bytes. */ @@ -115,10 +116,11 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, cow_request_begin(&cow_request, job, start, end); for (; start < end; start++) { - if (test_bit(start, job->done_bitmap)) { + if (!hbitmap_get(job->copy_bitmap, start)) { trace_backup_do_cow_skip(job, start); continue; /* already copied */ } + hbitmap_reset(job->copy_bitmap, start, 1); trace_backup_do_cow_process(job, start); @@ -141,6 +143,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, if (error_is_read) { *error_is_read = true; } + hbitmap_set(job->copy_bitmap, start, 1); goto out; } @@ -157,11 +160,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job, if (error_is_read) { *error_is_read = false; } + hbitmap_set(job->copy_bitmap, start, 1); goto out; } - set_bit(start, job->done_bitmap); - /* Publish progress, guest I/O counts as progress too. Note that the * offset field is an opaque progress value, it is not a disk offset. */ @@ -271,7 +273,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp) } len = DIV_ROUND_UP(backup_job->common.len, backup_job->cluster_size); - bitmap_zero(backup_job->done_bitmap, len); + hbitmap_set(backup_job->copy_bitmap, 0, len); } void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num, @@ -450,7 +452,8 @@ static void coroutine_fn backup_run(void *opaque) start = 0; end = DIV_ROUND_UP(job->common.len, job->cluster_size); - job->done_bitmap = bitmap_new(end); + job->copy_bitmap = hbitmap_alloc(end, 0); + hbitmap_set(job->copy_bitmap, 0, end); job->before_write.notify = backup_before_write_notify; bdrv_add_before_write_notifier(bs, &job->before_write); @@ -524,7 +527,7 @@ static void coroutine_fn backup_run(void *opaque) /* wait until pending backup_do_cow() calls have completed */ qemu_co_rwlock_wrlock(&job->flush_rwlock); qemu_co_rwlock_unlock(&job->flush_rwlock); - g_free(job->done_bitmap); + hbitmap_free(job->copy_bitmap); data = g_malloc(sizeof(*data)); data->ret = ret;
Use HBitmap copy_bitmap instead of done_bitmap. This is needed to unify backup loop for full/incremental modes in future patches. We reset bit of the copy_bitmap immediately after checking it in backup_do_cow(). It is safe, because all other intersecting requests will wait for our request finish anyway. The other difference is that in case of error we will have zeroed bit in copy_bitmap, when in done_bitmap we have not set bit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/backup.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)