| Message ID | 1482503344-6424-3-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
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 <vsementsov@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 >
24.01.2017 10:09, Fam Zheng wrote: > 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 <vsementsov@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... We can't guess, what clusters should be copied finally in none mode. None mode is done by allowing only notifier handling and no linear copying. But notifier handling will copy only clusters marked in copy_bitmap, so I set it from 0 to end. > >> 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 >>
On Tue, 01/24 12:00, Vladimir Sementsov-Ogievskiy wrote: > > > 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... > > We can't guess, what clusters should be copied finally in none mode. None > mode is done by allowing only notifier handling and no linear copying. But > notifier handling will copy only clusters marked in copy_bitmap, so I set it > from 0 to end. Yes, that's how I understand it too, what I dislike is this bit of inconsistency with it: "allowed to copy bitmap" here versus "planned to copy" in other modes. I don't know how to improve that, but I think at least the specialty of none mode could be mentioned in the comment of copy_bitmap.
24.01.2017 12:46, Fam Zheng wrote: > On Tue, 01/24 12:00, Vladimir Sementsov-Ogievskiy wrote: >>>> 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... >> We can't guess, what clusters should be copied finally in none mode. None >> mode is done by allowing only notifier handling and no linear copying. But >> notifier handling will copy only clusters marked in copy_bitmap, so I set it >> from 0 to end. > Yes, that's how I understand it too, what I dislike is this bit of inconsistency > with it: "allowed to copy bitmap" here versus "planned to copy" in other modes. > I don't know how to improve that, but I think at least the specialty of none > mode could be mentioned in the comment of copy_bitmap. Ok, comment will be good. I'll add it.
On Fri, Dec 23, 2016 at 05:28:45PM +0300, Vladimir Sementsov-Ogievskiy wrote: > +/* 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); Simpler and clearer: bdrv_set_dirty_iter(dbi, next_sector);
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); 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); 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;
We should not copy non-dirty clusters in write notifiers. So, initialize copy_bitmap from sync_bitmap. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/backup.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)