| Message ID | 1486045515-8009-1-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/02/2017 08:25 AM, Denis V. Lunev wrote: > From: Anton Nefedov <anton.nefedov@virtuozzo.com> > > If explicit zeroing out before mirroring is required for the target image, > it moves the block job offset counter to EOF, then offset and len counters > count the image size twice. There is no harm but stats are confusing, > specifically the progress of the operation is always reported as 99% by > management tools. > > The patch skips offset increase for the first "technical" pass over the > image. This should not cause any further harm. > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Jeff Cody <jcody@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > + bool initial_zeroing_ongoing; Long name. With a bit of bikeshedding, I might have used 'init_pass' for a shorter name (particularly if some later patch introduces another aspect of initialization that is not zeroing but is worth ignoring with respects to progress reporting). > } MirrorBlockJob; > > typedef struct MirrorOp { > @@ -117,9 +118,10 @@ static void mirror_iteration_done(MirrorOp *op, int ret) > if (s->cow_bitmap) { > bitmap_set(s->cow_bitmap, chunk_num, nb_chunks); > } > - s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE; > + if (!s->initial_zeroing_ongoing) { > + s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE; > + } > } > - > qemu_iovec_destroy(&op->qiov); Why are you deleting the blank line? Other than naming, the patch looks reasonable. If you spin a v3 with only the name changed, you can add: Reviewed-by: Eric Blake <eblake@redhat.com>
On Thu, Feb 02, 2017 at 05:25:15PM +0300, Denis V. Lunev wrote: > From: Anton Nefedov <anton.nefedov@virtuozzo.com> > > If explicit zeroing out before mirroring is required for the target image, > it moves the block job offset counter to EOF, then offset and len counters > count the image size twice. There is no harm but stats are confusing, > specifically the progress of the operation is always reported as 99% by > management tools. > > The patch skips offset increase for the first "technical" pass over the > image. This should not cause any further harm. > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Jeff Cody <jcody@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > Changes from v1: > - changed the approach - we do not allow to increase the offset rather then > to move it back > - description rewritten > - kludges to tests are removed as not actually needed with this approach > > block/mirror.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Another option is to put the flag in MirrorOp instead of MirrorBlockJob. That reduces the scope of the variable, but this is okay too: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 02/03/2017 05:52 PM, Stefan Hajnoczi wrote: > On Thu, Feb 02, 2017 at 05:25:15PM +0300, Denis V. Lunev wrote: >> From: Anton Nefedov <anton.nefedov@virtuozzo.com> >> >> If explicit zeroing out before mirroring is required for the target image, >> it moves the block job offset counter to EOF, then offset and len counters >> count the image size twice. There is no harm but stats are confusing, >> specifically the progress of the operation is always reported as 99% by >> management tools. >> >> The patch skips offset increase for the first "technical" pass over the >> image. This should not cause any further harm. >> >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Jeff Cody <jcody@redhat.com> >> CC: Kevin Wolf <kwolf@redhat.com> >> CC: Max Reitz <mreitz@redhat.com> >> CC: Eric Blake <eblake@redhat.com> >> --- >> Changes from v1: >> - changed the approach - we do not allow to increase the offset rather then >> to move it back >> - description rewritten >> - kludges to tests are removed as not actually needed with this approach >> >> block/mirror.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) > Another option is to put the flag in MirrorOp instead of MirrorBlockJob. > That reduces the scope of the variable, but this is okay too: > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> In this case we will have to pass argument through several layers on request creation path. Current approach is better. Thank you for the review :) Den
On Thu, Feb 02, 2017 at 05:25:15PM +0300, Denis V. Lunev wrote: > From: Anton Nefedov <anton.nefedov@virtuozzo.com> > > If explicit zeroing out before mirroring is required for the target image, > it moves the block job offset counter to EOF, then offset and len counters > count the image size twice. There is no harm but stats are confusing, > specifically the progress of the operation is always reported as 99% by > management tools. > > The patch skips offset increase for the first "technical" pass over the > image. This should not cause any further harm. > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Jeff Cody <jcody@redhat.com> > CC: Kevin Wolf <kwolf@redhat.com> > CC: Max Reitz <mreitz@redhat.com> > CC: Eric Blake <eblake@redhat.com> > --- > Changes from v1: > - changed the approach - we do not allow to increase the offset rather then > to move it back > - description rewritten > - kludges to tests are removed as not actually needed with this approach > > block/mirror.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index 301ba92..f100f5d 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -69,6 +69,7 @@ typedef struct MirrorBlockJob { > bool waiting_for_io; > int target_cluster_sectors; > int max_iov; > + bool initial_zeroing_ongoing; > } MirrorBlockJob; > > typedef struct MirrorOp { > @@ -117,9 +118,10 @@ static void mirror_iteration_done(MirrorOp *op, int ret) > if (s->cow_bitmap) { > bitmap_set(s->cow_bitmap, chunk_num, nb_chunks); > } > - s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE; > + if (!s->initial_zeroing_ongoing) { > + s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE; > + } > } > - > qemu_iovec_destroy(&op->qiov); > g_free(op); > > @@ -566,6 +568,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) > return 0; > } > > + s->initial_zeroing_ongoing = true; > for (sector_num = 0; sector_num < end; ) { > int nb_sectors = MIN(end - sector_num, > QEMU_ALIGN_DOWN(INT_MAX, s->granularity) >> BDRV_SECTOR_BITS); > @@ -573,6 +576,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) > mirror_throttle(s); > > if (block_job_is_cancelled(&s->common)) { > + s->initial_zeroing_ongoing = false; > return 0; > } > > @@ -587,6 +591,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) > } > > mirror_wait_for_all_io(s); > + s->initial_zeroing_ongoing = false; > } > > /* First part, loop on the sectors and initialize the dirty bitmap. */ > -- > 2.7.4 > Thanks, Applied to my block branch: git://github.com/codyprime/qemu-kvm-jtc.git block -Jeff
diff --git a/block/mirror.c b/block/mirror.c index 301ba92..f100f5d 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -69,6 +69,7 @@ typedef struct MirrorBlockJob { bool waiting_for_io; int target_cluster_sectors; int max_iov; + bool initial_zeroing_ongoing; } MirrorBlockJob; typedef struct MirrorOp { @@ -117,9 +118,10 @@ static void mirror_iteration_done(MirrorOp *op, int ret) if (s->cow_bitmap) { bitmap_set(s->cow_bitmap, chunk_num, nb_chunks); } - s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE; + if (!s->initial_zeroing_ongoing) { + s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE; + } } - qemu_iovec_destroy(&op->qiov); g_free(op); @@ -566,6 +568,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) return 0; } + s->initial_zeroing_ongoing = true; for (sector_num = 0; sector_num < end; ) { int nb_sectors = MIN(end - sector_num, QEMU_ALIGN_DOWN(INT_MAX, s->granularity) >> BDRV_SECTOR_BITS); @@ -573,6 +576,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) mirror_throttle(s); if (block_job_is_cancelled(&s->common)) { + s->initial_zeroing_ongoing = false; return 0; } @@ -587,6 +591,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) } mirror_wait_for_all_io(s); + s->initial_zeroing_ongoing = false; } /* First part, loop on the sectors and initialize the dirty bitmap. */