[Qemu-devel] [PATCH v2 1/1] mirror: do not increase offset during initial zero_or_discard phase

[Qemu-devel] [PATCH v2 1/1] mirror: do not increase offset during initial zero_or_discard phase

[Denis V. Lunev]

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

Re: [Qemu-devel] [PATCH v2 1/1] mirror: do not increase offset during initial zero_or_discard phase

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

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>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] mirror: do not increase offset during initial zero_or_discard phase

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 1/1] mirror: do not increase offset during initial zero_or_discard phase

[Denis V. Lunev]

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

Re: [Qemu-devel] [PATCH v2 1/1] mirror: do not increase offset during initial zero_or_discard phase

[Jeff Cody]

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