[v2,1/1] mirror: do not increase offset during initial zero_or_discard phase
diff mbox

Message ID 1486045515-8009-1-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Feb. 2, 2017, 2:25 p.m. UTC
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(-)

Comments

Eric Blake Feb. 2, 2017, 3:17 p.m. UTC | #1
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>
Stefan Hajnoczi Feb. 3, 2017, 2:52 p.m. UTC | #2
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>
Denis V. Lunev Feb. 3, 2017, 3:09 p.m. UTC | #3
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
Jeff Cody Feb. 7, 2017, 7:07 a.m. UTC | #4
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

Patch
diff mbox

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.  */