diff mbox

[v9,06/14] block/mirror: Improve progress report

Message ID 1404582448-23840-7-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz July 5, 2014, 5:47 p.m. UTC
Instead of taking the total length of the block device as the block
job's length, use the number of dirty sectors. The progress is now the
number of sectors mirrored to the target block device. Note that this
may result in the job's length increasing during operation, which is
however in fact desirable.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Eric Blake July 7, 2014, 7:13 p.m. UTC | #1
On 07/05/2014 11:47 AM, Max Reitz wrote:
> Instead of taking the total length of the block device as the block
> job's length, use the number of dirty sectors. The progress is now the
> number of sectors mirrored to the target block device. Note that this
> may result in the job's length increasing during operation, which is
> however in fact desirable.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Paolo Bonzini July 9, 2014, 9:24 p.m. UTC | #2
Il 07/07/2014 21:13, Eric Blake ha scritto:
> On 07/05/2014 11:47 AM, Max Reitz wrote:
>> Instead of taking the total length of the block device as the block
>> job's length, use the number of dirty sectors. The progress is now the
>> number of sectors mirrored to the target block device. Note that this
>> may result in the job's length increasing during operation, which is
>> however in fact desirable.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/mirror.c | 34 ++++++++++++++++++++++------------
>>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

This is an API change... IIUC the length can become bigger than the 
underlying device's size.  Eric, how would libvirt expose this to 
clients and what are the chances that they get confused?

Paolo
Eric Blake July 9, 2014, 9:36 p.m. UTC | #3
On 07/09/2014 03:24 PM, Paolo Bonzini wrote:
> Il 07/07/2014 21:13, Eric Blake ha scritto:
>> On 07/05/2014 11:47 AM, Max Reitz wrote:
>>> Instead of taking the total length of the block device as the block
>>> job's length, use the number of dirty sectors. The progress is now the
>>> number of sectors mirrored to the target block device. Note that this
>>> may result in the job's length increasing during operation, which is
>>> however in fact desirable.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  block/mirror.c | 34 ++++++++++++++++++++++------------
>>>  1 file changed, 22 insertions(+), 12 deletions(-)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> This is an API change... IIUC the length can become bigger than the
> underlying device's size.  Eric, how would libvirt expose this to
> clients and what are the chances that they get confused?

Libvirt has already documented that a job size is unrelated to the block
device size, that it is only an approximation to completion, and that
the completion number may change during operation.  The only hard and
fast rule is that the job is finished when the two counters are equal.
I recommended this change precisely because the new semantics are better
than the old, including how libvirt exposes the numbers to the end user
- if the end parameter continues to grow more than the current
parameter, it is DESIRABLE to expose that as sign that the guest is
dirtying pages fast enough to cause problems in converging the block job.

http://libvirt.org/html/libvirt-libvirt.html#virDomainBlockJobInfo
"The following fields provide an indication of block job progress. @cur
indicates the current position and will be between 0 and @end. @end is
the final cursor position for this operation and represents completion.
To approximate progress, divide @cur by @end."
diff mbox

Patch

diff --git a/block/mirror.c b/block/mirror.c
index 6c3ee70..39c52c3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -45,6 +45,7 @@  typedef struct MirrorBlockJob {
     int64_t sector_num;
     int64_t granularity;
     size_t buf_size;
+    int64_t bdev_length;
     unsigned long *cow_bitmap;
     BdrvDirtyBitmap *dirty_bitmap;
     HBitmapIter hbi;
@@ -54,6 +55,7 @@  typedef struct MirrorBlockJob {
 
     unsigned long *in_flight_bitmap;
     int in_flight;
+    int sectors_in_flight;
     int ret;
 } MirrorBlockJob;
 
@@ -87,6 +89,7 @@  static void mirror_iteration_done(MirrorOp *op, int ret)
     trace_mirror_iteration_done(s, op->sector_num, op->nb_sectors, ret);
 
     s->in_flight--;
+    s->sectors_in_flight -= op->nb_sectors;
     iov = op->qiov.iov;
     for (i = 0; i < op->qiov.niov; i++) {
         MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
@@ -98,8 +101,11 @@  static void mirror_iteration_done(MirrorOp *op, int ret)
     chunk_num = op->sector_num / sectors_per_chunk;
     nb_chunks = op->nb_sectors / sectors_per_chunk;
     bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);
-    if (s->cow_bitmap && ret >= 0) {
-        bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
+    if (ret >= 0) {
+        if (s->cow_bitmap) {
+            bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
+        }
+        s->common.offset += (uint64_t)op->nb_sectors * BDRV_SECTOR_SIZE;
     }
 
     qemu_iovec_destroy(&op->qiov);
@@ -172,7 +178,7 @@  static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
     hbitmap_next_sector = s->sector_num;
     sector_num = s->sector_num;
     sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
-    end = s->common.len >> BDRV_SECTOR_BITS;
+    end = s->bdev_length / BDRV_SECTOR_SIZE;
 
     /* Extend the QEMUIOVector to include all adjacent blocks that will
      * be copied in this operation.
@@ -284,6 +290,7 @@  static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
 
     /* Copy the dirty cluster.  */
     s->in_flight++;
+    s->sectors_in_flight += nb_sectors;
     trace_mirror_one_iteration(s, sector_num, nb_sectors);
     bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
                    mirror_read_complete, op);
@@ -329,11 +336,11 @@  static void coroutine_fn mirror_run(void *opaque)
         goto immediate_exit;
     }
 
-    s->common.len = bdrv_getlength(bs);
-    if (s->common.len < 0) {
-        ret = s->common.len;
+    s->bdev_length = bdrv_getlength(bs);
+    if (s->bdev_length < 0) {
+        ret = s->bdev_length;
         goto immediate_exit;
-    } else if (s->common.len == 0) {
+    } else if (s->bdev_length == 0) {
         /* Report BLOCK_JOB_READY and wait for complete. */
         block_job_event_ready(&s->common);
         s->synced = true;
@@ -344,7 +351,7 @@  static void coroutine_fn mirror_run(void *opaque)
         goto immediate_exit;
     }
 
-    length = DIV_ROUND_UP(s->common.len, s->granularity);
+    length = DIV_ROUND_UP(s->bdev_length, s->granularity);
     s->in_flight_bitmap = bitmap_new(length);
 
     /* If we have no backing file yet in the destination, we cannot let
@@ -364,7 +371,7 @@  static void coroutine_fn mirror_run(void *opaque)
         }
     }
 
-    end = s->common.len >> BDRV_SECTOR_BITS;
+    end = s->bdev_length / BDRV_SECTOR_SIZE;
     s->buf = qemu_blockalign(bs, s->buf_size);
     sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
     mirror_free_init(s);
@@ -404,6 +411,12 @@  static void coroutine_fn mirror_run(void *opaque)
         }
 
         cnt = bdrv_get_dirty_count(bs, s->dirty_bitmap);
+        /* s->common.offset contains the number of bytes already processed so
+         * far, cnt is the number of dirty sectors remaining and
+         * s->sectors_in_flight is the number of sectors currently being
+         * processed; together those are the current total operation length */
+        s->common.len = s->common.offset +
+                        (cnt + s->sectors_in_flight) * BDRV_SECTOR_SIZE;
 
         /* Note that even when no rate limit is applied we need to yield
          * periodically with no pending I/O so that qemu_aio_flush() returns.
@@ -440,7 +453,6 @@  static void coroutine_fn mirror_run(void *opaque)
                  * report completion.  This way, block-job-cancel will leave
                  * the target in a consistent state.
                  */
-                s->common.offset = end * BDRV_SECTOR_SIZE;
                 if (!s->synced) {
                     block_job_event_ready(&s->common);
                     s->synced = true;
@@ -469,8 +481,6 @@  static void coroutine_fn mirror_run(void *opaque)
         ret = 0;
         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
         if (!s->synced) {
-            /* Publish progress */
-            s->common.offset = (end - cnt) * BDRV_SECTOR_SIZE;
             block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
             if (block_job_is_cancelled(&s->common)) {
                 break;