On Fri, 12/23 17:28, Vladimir Sementsov-Ogievskiy wrote: > We should not copy non-dirty clusters in write notifiers. So, > initialize copy_bitmap from sync_bitmap. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > block/backup.c | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/block/backup.c b/block/backup.c > index 6b27e55..621b1c0 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -437,6 +437,34 @@ out: > return ret; > } > > +/* init copy_bitmap from sync_bitmap */ > +static void backup_incremental_init_copy_bitmap(BackupBlockJob *job) > +{ > + int64_t sector; > + BdrvDirtyBitmapIter *dbi; > + uint32_t sect_gran = > + bdrv_dirty_bitmap_granularity(job->sync_bitmap) >> BDRV_SECTOR_BITS; > + int64_t sz = bdrv_dirty_bitmap_size(job->sync_bitmap); > + int64_t sectors_per_cluster = cluster_size_sectors(job); > + uint32_t cl_gran = MAX(1, sect_gran / sectors_per_cluster); > + > + dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0); > + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { > + int64_t cluster = sector / sectors_per_cluster; > + int64_t next_sector = (cluster + cl_gran) * sectors_per_cluster; > + > + hbitmap_set(job->copy_bitmap, cluster, cl_gran); > + > + if (next_sector >= sz) { > + break; > + } > + > + bdrv_set_dirty_iter(dbi, (cluster + cl_gran) * sectors_per_cluster); > + } > + > + bdrv_dirty_iter_free(dbi); > +} > + > static void coroutine_fn backup_run(void *opaque) > { > BackupBlockJob *job = opaque; > @@ -453,20 +481,22 @@ static void coroutine_fn backup_run(void *opaque) > end = DIV_ROUND_UP(job->common.len, job->cluster_size); > > 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); > > if (job->sync_mode == MIRROR_SYNC_MODE_NONE) { > + hbitmap_set(job->copy_bitmap, 0, end);
This is confusing. It seems job->copy_bitmap is actually a superset of clusters to copy instead of the exact one? Because "none" mode doesn't need blanket copy - only overwritten clusters are copied... > while (!block_job_is_cancelled(&job->common)) { > /* Yield until the job is cancelled. We just let our > before_write > * notify callback service CoW requests. */ > block_job_yield(&job->common); > } > } else if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) { > + backup_incremental_init_copy_bitmap(job); ... But here you are only marking bits in sync_bitmap. Fam > ret = backup_run_incremental(job); > } else { > + hbitmap_set(job->copy_bitmap, 0, end); > /* Both FULL and TOP SYNC_MODE's require copying.. */ > for (; start < end; start++) { > bool error_is_read; > -- > 1.8.3.1 >