[v3,4/8] mirror: create mirror_dirty_init helper for mirror_run

Commit Message

Denis V. Lunev July 14, 2016, 1:33 p.m. UTC

The code inside the helper will be extended in the next patch. mirror_run
itself is overbloated at the moment.

Signed-off-by: Denis V. Lunev <[e-mail]>
Reviewed-by: Vladimir Sementsov-Ogievskiy<[e-mail]>
Reviewed-by: Eric Blake <[e-mail]>
Reviewed-by: Fam Zheng <[e-mail]>
CC: Stefan Hajnoczi <[e-mail]>
CC: Kevin Wolf <[e-mail]>
CC: Max Reitz <[e-mail]>
CC: Jeff Cody <[e-mail]>
CC: Eric Blake <[e-mail]>
---
 block/mirror.c | 70 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 29 deletions(-)

Comments

Eric Blake July 14, 2016, 4:19 p.m. UTC | #1

On 07/14/2016 07:33 AM, Denis V. Lunev wrote:
> The code inside the helper will be extended in the next patch. mirror_run
> itself is overbloated at the moment.
> 
> Signed-off-by: Denis V. Lunev <[e-mail]>
> Reviewed-by: Vladimir Sementsov-Ogievskiy<[e-mail]>
> Reviewed-by: Eric Blake <[e-mail]>

I did NOT give R-b on v2 3/7.  In particular, this patch has semantic
changes that I requested on v2, and so you want to make sure I re-review
it (I often skip re-reviewing a patch that has my R-b listed, on the
grounds that I'm trusting your judgment that it hasn't substantially
changed since my last time through it; but here, you have changed it
since last time).

> Reviewed-by: Fam Zheng <[e-mail]>
> CC: Stefan Hajnoczi <[e-mail]>
> CC: Kevin Wolf <[e-mail]>
> CC: Max Reitz <[e-mail]>
> CC: Jeff Cody <[e-mail]>
> CC: Eric Blake <[e-mail]>
> ---
>  block/mirror.c | 70 ++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 41 insertions(+), 29 deletions(-)

That said, I've looked through the changes (the rebase on top of moving
last_pause_ns to being a member of s, so that a single timestamp is
shared across both functions), and can now safely state:

Reviewed-by: Eric Blake <[e-mail]>

Denis V. Lunev July 14, 2016, 4:48 p.m. UTC | #2

On 07/14/2016 07:19 PM, Eric Blake wrote:
> On 07/14/2016 07:33 AM, Denis V. Lunev wrote:
>> The code inside the helper will be extended in the next patch. mirror_run
>> itself is overbloated at the moment.
>>
>> Signed-off-by: Denis V. Lunev <[e-mail]>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy<[e-mail]>
>> Reviewed-by: Eric Blake <[e-mail]>
> I did NOT give R-b on v2 3/7.  In particular, this patch has semantic
> changes that I requested on v2, and so you want to make sure I re-review
> it (I often skip re-reviewing a patch that has my R-b listed, on the
> grounds that I'm trusting your judgment that it hasn't substantially
> changed since my last time through it; but here, you have changed it
> since last time).
>
sorry :(

Patch

diff --git a/block/mirror.c b/block/mirror.c
index 93e2896..3185ccb 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -525,18 +525,54 @@  static void mirror_throttle(MirrorBlockJob *s)
     }
 }
 
+static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
+{
+    int64_t sector_num, end;
+    BlockDriverState *base = s->base;
+    BlockDriverState *bs = blk_bs(s->common.blk);
+    BlockDriverState *target_bs = blk_bs(s->target);
+    bool mark_all_dirty = base == NULL && !bdrv_has_zero_init(target_bs);
+    int ret, n;
+
+    end = s->bdev_length / BDRV_SECTOR_SIZE;
+
+    /* First part, loop on the sectors and initialize the dirty bitmap.  */
+    for (sector_num = 0; sector_num < end; ) {
+        /* Just to make sure we are not exceeding int limit. */
+        int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
+                             end - sector_num);
+
+        mirror_throttle(s);
+
+        if (block_job_is_cancelled(&s->common)) {
+            return 0;
+        }
+
+        ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
+        if (ret < 0) {
+            return ret;
+        }
+
+        assert(n > 0);
+        if (ret == 1 || mark_all_dirty) {
+            bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
+        }
+        sector_num += n;
+    }
+    return 0;
+}
+
 static void coroutine_fn mirror_run(void *opaque)
 {
     MirrorBlockJob *s = opaque;
     MirrorExitData *data;
     BlockDriverState *bs = blk_bs(s->common.blk);
     BlockDriverState *target_bs = blk_bs(s->target);
-    int64_t sector_num, end, length;
+    int64_t length;
     BlockDriverInfo bdi;
     char backing_filename[2]; /* we only need 2 characters because we are only
                                  checking for a NULL string */
     int ret = 0;
-    int n;
     int target_cluster_size = BDRV_SECTOR_SIZE;
 
     if (block_job_is_cancelled(&s->common)) {
@@ -578,7 +614,6 @@  static void coroutine_fn mirror_run(void *opaque)
     s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
     s->max_iov = MIN(bs->bl.max_iov, target_bs->bl.max_iov);
 
-    end = s->bdev_length / BDRV_SECTOR_SIZE;
     s->buf = qemu_try_blockalign(bs, s->buf_size);
     if (s->buf == NULL) {
         ret = -ENOMEM;
@@ -589,32 +624,9 @@  static void coroutine_fn mirror_run(void *opaque)
 
     s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     if (!s->is_none_mode) {
-        /* First part, loop on the sectors and initialize the dirty bitmap.  */
-        BlockDriverState *base = s->base;
-        bool mark_all_dirty = s->base == NULL && !bdrv_has_zero_init(target_bs);
-
-        for (sector_num = 0; sector_num < end; ) {
-            /* Just to make sure we are not exceeding int limit. */
-            int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
-                                 end - sector_num);
-
-            mirror_throttle(s);
-
-            if (block_job_is_cancelled(&s->common)) {
-                goto immediate_exit;
-            }
-
-            ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
-
-            if (ret < 0) {
-                goto immediate_exit;
-            }
-
-            assert(n > 0);
-            if (ret == 1 || mark_all_dirty) {
-                bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
-            }
-            sector_num += n;
+        ret = mirror_dirty_init(s);
+        if (ret < 0 || block_job_is_cancelled(&s->common)) {
+            goto immediate_exit;
         }
     }