[Qemu-devel] [PATCH 00/17] mirror: Mainly coroutine refinements

[Qemu-devel] [PATCH 00/17] mirror: Mainly coroutine refinements

[Max Reitz]

This series is based on v2 of my "block: Deal with filters" series:

Based-on: <20180809223117.7846-1-mreitz@redhat.com>


The bulk of this series (14 of the patches here, in fact) makes more of
the coroutined mirror I started with my active mirror series.  For this,
mirror_perform() is translated into a coroutine version that actually
looks like coroutines are a nice thing to have (a single AioContext
lock, a common clean-up path, and a mirror_co_copy() that does both read
and write in a single function (like it is supposed to)).

Another thing I have wanted to implement is a read-write-blocking mode.
write-blocking (active mirror) copies data to the target whenever you
write data to the mirror node -- so once the mirror bitmap is clean, it
will stay clean, and completing the job is guaranteed to be basically
instantenous.  read-write-blocking would also copy data whenever you
read from the mirror node, because we have already read the data right
now, so we might as well copy it to the target now.

Instead of implementing it actually as a new copy-mode (which is how I
used to do, but I decided that was boring), I opted for implementing an
interesting .bdrv_co_block_status() function that allows using COR for
the purpose: You simply put a COR node on top of your write-blocking
mirror node, and then all the data read will be copied to the target
through COR (as long as it is not already cleanly mirrored).

The benefit I see with this approach is that is just looks nice.  Why
implement an actual new copy-mode when we can just an existing block
layer function for the purpose?

The drawbacks I see are these:
(1) mirror in write-blocking mode has to interpret
    BDRV_REQ_WRITE_UNCHANGED in a special manner, or this doesn't really
    work.  With patch 16 of this series, such requests are not written
    to the source, but only to the target.  You might find that weird.

(2) We currently don't have a good way of inserting a COR node at
    runtime...

(3) You may break your data if you do funny stuff.  Now this is of
    course the interesting point.
    Mirror works (by design) even when you attach a parent to the source
    directly, i.e., not going through the mirror node.  That is because
    mirror uses the source's dirty bitmap functionality and does not
    build its own.  But if you do this with COR/write-blocking, then the
    COR driver may issue a request while some other parent is issuing
    another request immediately to the source, which may cause source
    and target to go out of sync.
    But...  Technically this is already an issue with write-blocking
    alone.  We reset the bitmap only in do_sync_target_write(), after we
    have already written to the source.  So if another of source's
    parents writes to source between our write and the bitmap reset, we
    lose that information.
    So I suppose when using write-blocking, the mirror driver may not
    share the WRITE permission?

(4) The same issue exists with COR itself, actually...  It can only
    share the WRITE permission because COR requests are serializing, but
    that only works on a single layer.  With a filter between COR and
    the allocating node, you get the same issue of someone being able to
    slip in a request between the COR read and the COR allocation.
    Maybe we actually want COR not to share WRITE either?

So in my opinion the main drawback is that this design reveals current
weaknesses and bugs of the block layer.


Max Reitz (17):
  iotests: Try reading while mirroring in 156
  mirror: Make wait_for_any_operation() coroutine_fn
  mirror: Pull *_align_for_copy() from *_co_read()
  mirror: Remove bytes_handled, part 1
  mirror: Remove bytes_handled, part 2
  mirror: Create mirror_co_perform()
  mirror: Make mirror_co_zero() nicer
  mirror: Make mirror_co_discard() nicer
  mirror: Lock AioContext in mirror_co_perform()
  mirror: Create mirror_co_alloc_qiov()
  mirror: Inline mirror_write_complete(), part 1
  mirror: Put QIOV locally into mirror_co_read
  mirror: Linearize mirror_co_read()
  mirror: Inline mirror_iteration_done()
  mirror: Release AioCtx before queue_restart_all()
  mirror: Support COR with write-blocking
  iotests: Add test for active mirror with COR

 block/mirror.c             | 483 ++++++++++++++++++++-----------------
 tests/qemu-iotests/151     |  56 +++++
 tests/qemu-iotests/151.out |   4 +-
 tests/qemu-iotests/156     |   7 +
 tests/qemu-iotests/156.out |   3 +
 5 files changed, 334 insertions(+), 219 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 01/17] iotests: Try reading while mirroring in 156

[Max Reitz]

Currently, we never test whether we can read from the source while
mirroring (that means, whether we can read from the mirror BDS).  Add
such a test to 156 because it fits well.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/156     | 7 +++++++
 tests/qemu-iotests/156.out | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
index 0a9a09802e..99f7326158 100755
--- a/tests/qemu-iotests/156
+++ b/tests/qemu-iotests/156
@@ -106,6 +106,13 @@ _send_qemu_cmd $QEMU_HANDLE \
                       'qemu-io source \"write -P 4 192k 64k\"' } }" \
     'return'
 
+# Read it back (proving that we can read while mirroring)
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io source \"read -P 4 192k 64k\"' } }" \
+    'return'
+
 # Copy source backing chain to the target before completing the job
 cp "$TEST_IMG.backing" "$TEST_IMG.target.backing"
 cp "$TEST_IMG" "$TEST_IMG.target"
diff --git a/tests/qemu-iotests/156.out b/tests/qemu-iotests/156.out
index 34c057b626..a591bd3a1e 100644
--- a/tests/qemu-iotests/156.out
+++ b/tests/qemu-iotests/156.out
@@ -20,6 +20,9 @@ Formatting 'TEST_DIR/t.IMGFMT.target.overlay', fmt=IMGFMT size=1048576 backing_f
 wrote 65536/65536 bytes at offset 196608
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": ""}
+read 65536/65536 bytes at offset 196608
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "source"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "source"}}
-- 
2.17.1

Re: [Qemu-devel] [PATCH 01/17] iotests: Try reading while mirroring in 156

[Jeff Cody]

On Mon, Aug 13, 2018 at 04:19:50AM +0200, Max Reitz wrote:
> Currently, we never test whether we can read from the source while
> mirroring (that means, whether we can read from the mirror BDS).  Add
> such a test to 156 because it fits well.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  tests/qemu-iotests/156     | 7 +++++++
>  tests/qemu-iotests/156.out | 3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
> index 0a9a09802e..99f7326158 100755
> --- a/tests/qemu-iotests/156
> +++ b/tests/qemu-iotests/156
> @@ -106,6 +106,13 @@ _send_qemu_cmd $QEMU_HANDLE \
>                        'qemu-io source \"write -P 4 192k 64k\"' } }" \
>      'return'
>  
> +# Read it back (proving that we can read while mirroring)
> +_send_qemu_cmd $QEMU_HANDLE \
> +    "{ 'execute': 'human-monitor-command',
> +       'arguments': { 'command-line':
> +                      'qemu-io source \"read -P 4 192k 64k\"' } }" \
> +    'return'
> +
>  # Copy source backing chain to the target before completing the job
>  cp "$TEST_IMG.backing" "$TEST_IMG.target.backing"
>  cp "$TEST_IMG" "$TEST_IMG.target"
> diff --git a/tests/qemu-iotests/156.out b/tests/qemu-iotests/156.out
> index 34c057b626..a591bd3a1e 100644
> --- a/tests/qemu-iotests/156.out
> +++ b/tests/qemu-iotests/156.out
> @@ -20,6 +20,9 @@ Formatting 'TEST_DIR/t.IMGFMT.target.overlay', fmt=IMGFMT size=1048576 backing_f
>  wrote 65536/65536 bytes at offset 196608
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  {"return": ""}
> +read 65536/65536 bytes at offset 196608
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"return": ""}
>  {"return": {}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "source"}}
>  {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "source"}}
> -- 
> 2.17.1
> 

[Qemu-devel] [PATCH 02/17] mirror: Make wait_for_any_operation() coroutine_fn

[Max Reitz]

mirror_wait_for_any_operation() calls qemu_co_queue_wait(), which is a
coroutine_fn (technically it is a macro which resolves to a
coroutine_fn).  Therefore, this function needs to be a coroutine_fn as
well.

This patch makes it and all of its callers coroutine_fns.

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

diff --git a/block/mirror.c b/block/mirror.c
index 85f5742eae..c28b6159d5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -279,7 +279,8 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
     return ret;
 }
 
-static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
+static inline void coroutine_fn
+    mirror_co_wait_for_any_operation(MirrorBlockJob *s, bool active)
 {
     MirrorOp *op;
 
@@ -297,10 +298,11 @@ static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
     abort();
 }
 
-static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
+static inline void coroutine_fn
+    mirror_co_wait_for_free_in_flight_slot(MirrorBlockJob *s)
 {
     /* Only non-active operations use up in-flight slots */
-    mirror_wait_for_any_operation(s, false);
+    mirror_co_wait_for_any_operation(s, false);
 }
 
 /* Perform a mirror copy operation.
@@ -343,7 +345,7 @@ static void coroutine_fn mirror_co_read(void *opaque)
 
     while (s->buf_free_count < nb_chunks) {
         trace_mirror_yield_in_flight(s, op->offset, s->in_flight);
-        mirror_wait_for_free_in_flight_slot(s);
+        mirror_co_wait_for_free_in_flight_slot(s);
     }
 
     /* Now make a QEMUIOVector taking enough granularity-sized chunks
@@ -549,7 +551,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
 
         while (s->in_flight >= MAX_IN_FLIGHT) {
             trace_mirror_yield_in_flight(s, offset, s->in_flight);
-            mirror_wait_for_free_in_flight_slot(s);
+            mirror_co_wait_for_free_in_flight_slot(s);
         }
 
         if (s->ret < 0) {
@@ -600,10 +602,10 @@ static void mirror_free_init(MirrorBlockJob *s)
  * mirror_resume() because mirror_run() will begin iterating again
  * when the job is resumed.
  */
-static void mirror_wait_for_all_io(MirrorBlockJob *s)
+static void coroutine_fn mirror_co_wait_for_all_io(MirrorBlockJob *s)
 {
     while (s->in_flight > 0) {
-        mirror_wait_for_free_in_flight_slot(s);
+        mirror_co_wait_for_free_in_flight_slot(s);
     }
 }
 
@@ -762,7 +764,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             if (s->in_flight >= MAX_IN_FLIGHT) {
                 trace_mirror_yield(s, UINT64_MAX, s->buf_free_count,
                                    s->in_flight);
-                mirror_wait_for_free_in_flight_slot(s);
+                mirror_co_wait_for_free_in_flight_slot(s);
                 continue;
             }
 
@@ -770,7 +772,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             offset += bytes;
         }
 
-        mirror_wait_for_all_io(s);
+        mirror_co_wait_for_all_io(s);
         s->initial_zeroing_ongoing = false;
     }
 
@@ -916,7 +918,7 @@ static void coroutine_fn mirror_run(void *opaque)
         /* Do not start passive operations while there are active
          * writes in progress */
         while (s->in_active_write_counter) {
-            mirror_wait_for_any_operation(s, true);
+            mirror_co_wait_for_any_operation(s, true);
         }
 
         if (s->ret < 0) {
@@ -942,7 +944,7 @@ static void coroutine_fn mirror_run(void *opaque)
             if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
                 (cnt == 0 && s->in_flight > 0)) {
                 trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
-                mirror_wait_for_free_in_flight_slot(s);
+                mirror_co_wait_for_free_in_flight_slot(s);
                 continue;
             } else if (cnt != 0) {
                 delay_ns = mirror_iteration(s);
@@ -1028,7 +1030,7 @@ immediate_exit:
         assert(ret < 0 || ((s->common.job.force_cancel || !s->synced) &&
                job_is_cancelled(&s->common.job)));
         assert(need_drain);
-        mirror_wait_for_all_io(s);
+        mirror_co_wait_for_all_io(s);
     }
 
     assert(s->in_flight == 0);
@@ -1098,11 +1100,11 @@ static void mirror_complete(Job *job, Error **errp)
     job_enter(job);
 }
 
-static void mirror_pause(Job *job)
+static void coroutine_fn mirror_pause(Job *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
 
-    mirror_wait_for_all_io(s);
+    mirror_co_wait_for_all_io(s);
 }
 
 static bool mirror_drained_poll(BlockJob *job)
-- 
2.17.1

Re: [Qemu-devel] [PATCH 02/17] mirror: Make wait_for_any_operation() coroutine_fn

[Murilo Opsfelder Araujo]

Hi, Max.

On Mon, Aug 13, 2018 at 04:19:51AM +0200, Max Reitz wrote:
> mirror_wait_for_any_operation() calls qemu_co_queue_wait(), which is a
> coroutine_fn (technically it is a macro which resolves to a
> coroutine_fn).  Therefore, this function needs to be a coroutine_fn as
> well.
>
> This patch makes it and all of its callers coroutine_fns.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 85f5742eae..c28b6159d5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -279,7 +279,8 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
>      return ret;
>  }
>
> -static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
> +static inline void coroutine_fn
> +    mirror_co_wait_for_any_operation(MirrorBlockJob *s, bool active)
>  {
>      MirrorOp *op;
>
> @@ -297,10 +298,11 @@ static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
>      abort();
>  }
>
> -static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
> +static inline void coroutine_fn
> +    mirror_co_wait_for_free_in_flight_slot(MirrorBlockJob *s)
>  {
>      /* Only non-active operations use up in-flight slots */
> -    mirror_wait_for_any_operation(s, false);
> +    mirror_co_wait_for_any_operation(s, false);
>  }
>
>  /* Perform a mirror copy operation.
> @@ -343,7 +345,7 @@ static void coroutine_fn mirror_co_read(void *opaque)
>
>      while (s->buf_free_count < nb_chunks) {
>          trace_mirror_yield_in_flight(s, op->offset, s->in_flight);
> -        mirror_wait_for_free_in_flight_slot(s);
> +        mirror_co_wait_for_free_in_flight_slot(s);
>      }
>
>      /* Now make a QEMUIOVector taking enough granularity-sized chunks
> @@ -549,7 +551,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>
>          while (s->in_flight >= MAX_IN_FLIGHT) {
>              trace_mirror_yield_in_flight(s, offset, s->in_flight);
> -            mirror_wait_for_free_in_flight_slot(s);
> +            mirror_co_wait_for_free_in_flight_slot(s);
>          }
>
>          if (s->ret < 0) {
> @@ -600,10 +602,10 @@ static void mirror_free_init(MirrorBlockJob *s)
>   * mirror_resume() because mirror_run() will begin iterating again
>   * when the job is resumed.
>   */
> -static void mirror_wait_for_all_io(MirrorBlockJob *s)
> +static void coroutine_fn mirror_co_wait_for_all_io(MirrorBlockJob *s)
>  {
>      while (s->in_flight > 0) {
> -        mirror_wait_for_free_in_flight_slot(s);
> +        mirror_co_wait_for_free_in_flight_slot(s);
>      }
>  }
>
> @@ -762,7 +764,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              if (s->in_flight >= MAX_IN_FLIGHT) {
>                  trace_mirror_yield(s, UINT64_MAX, s->buf_free_count,
>                                     s->in_flight);
> -                mirror_wait_for_free_in_flight_slot(s);
> +                mirror_co_wait_for_free_in_flight_slot(s);
>                  continue;
>              }
>
> @@ -770,7 +772,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              offset += bytes;
>          }
>
> -        mirror_wait_for_all_io(s);
> +        mirror_co_wait_for_all_io(s);
>          s->initial_zeroing_ongoing = false;
>      }
>
> @@ -916,7 +918,7 @@ static void coroutine_fn mirror_run(void *opaque)
>          /* Do not start passive operations while there are active
>           * writes in progress */
>          while (s->in_active_write_counter) {
> -            mirror_wait_for_any_operation(s, true);
> +            mirror_co_wait_for_any_operation(s, true);
>          }
>
>          if (s->ret < 0) {
> @@ -942,7 +944,7 @@ static void coroutine_fn mirror_run(void *opaque)
>              if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>                  (cnt == 0 && s->in_flight > 0)) {
>                  trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
> -                mirror_wait_for_free_in_flight_slot(s);
> +                mirror_co_wait_for_free_in_flight_slot(s);
>                  continue;
>              } else if (cnt != 0) {
>                  delay_ns = mirror_iteration(s);
> @@ -1028,7 +1030,7 @@ immediate_exit:
>          assert(ret < 0 || ((s->common.job.force_cancel || !s->synced) &&
>                 job_is_cancelled(&s->common.job)));
>          assert(need_drain);
> -        mirror_wait_for_all_io(s);
> +        mirror_co_wait_for_all_io(s);
>      }
>
>      assert(s->in_flight == 0);
> @@ -1098,11 +1100,11 @@ static void mirror_complete(Job *job, Error **errp)
>      job_enter(job);
>  }
>
> -static void mirror_pause(Job *job)
> +static void coroutine_fn mirror_pause(Job *job)

Other functions in this patch were renamed to mirror_co_*.  I'm not sure if
mirror_pause() should follow that, i.e. be renamed to mirror_co_pause().

>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>
> -    mirror_wait_for_all_io(s);
> +    mirror_co_wait_for_all_io(s);
>  }
>
>  static bool mirror_drained_poll(BlockJob *job)
> --
> 2.17.1
>
>

--
Murilo

Re: [Qemu-devel] [PATCH 02/17] mirror: Make wait_for_any_operation() coroutine_fn

[Max Reitz]

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

On 2018-08-13 14:54, Murilo Opsfelder Araujo wrote:
> Hi, Max.

Hi, Murilo!

> On Mon, Aug 13, 2018 at 04:19:51AM +0200, Max Reitz wrote:
>> mirror_wait_for_any_operation() calls qemu_co_queue_wait(), which is a
>> coroutine_fn (technically it is a macro which resolves to a
>> coroutine_fn).  Therefore, this function needs to be a coroutine_fn as
>> well.
>>
>> This patch makes it and all of its callers coroutine_fns.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/mirror.c | 30 ++++++++++++++++--------------
>>  1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 85f5742eae..c28b6159d5 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c

[...]

>> @@ -1098,11 +1100,11 @@ static void mirror_complete(Job *job, Error **errp)
>>      job_enter(job);
>>  }
>>
>> -static void mirror_pause(Job *job)
>> +static void coroutine_fn mirror_pause(Job *job)
> 
> Other functions in this patch were renamed to mirror_co_*.  I'm not sure if
> mirror_pause() should follow that, i.e. be renamed to mirror_co_pause().

Hm.  I don't know why I didn't do that.  Makes sense, why not.

(Well, JobDriver.pause() is not called .co_pause(), but then again,
JobDriver.start is set to mirror_run(), so it's not like our function
names match so far...)

Max


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

Re: [Qemu-devel] [PATCH 02/17] mirror: Make wait_for_any_operation() coroutine_fn

[Jeff Cody]

On Mon, Aug 13, 2018 at 04:19:51AM +0200, Max Reitz wrote:
> mirror_wait_for_any_operation() calls qemu_co_queue_wait(), which is a
> coroutine_fn (technically it is a macro which resolves to a
> coroutine_fn).  Therefore, this function needs to be a coroutine_fn as
> well.
> 
> This patch makes it and all of its callers coroutine_fns.
>

It'd be nice if someday coroutine_fn was actually functional during
compile.  We can dream :)

> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/mirror.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 85f5742eae..c28b6159d5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -279,7 +279,8 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
>      return ret;
>  }
>  
> -static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
> +static inline void coroutine_fn
> +    mirror_co_wait_for_any_operation(MirrorBlockJob *s, bool active)
>  {
>      MirrorOp *op;
>  
> @@ -297,10 +298,11 @@ static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
>      abort();
>  }
>  
> -static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
> +static inline void coroutine_fn
> +    mirror_co_wait_for_free_in_flight_slot(MirrorBlockJob *s)
>  {
>      /* Only non-active operations use up in-flight slots */
> -    mirror_wait_for_any_operation(s, false);
> +    mirror_co_wait_for_any_operation(s, false);
>  }
>  
>  /* Perform a mirror copy operation.
> @@ -343,7 +345,7 @@ static void coroutine_fn mirror_co_read(void *opaque)
>  
>      while (s->buf_free_count < nb_chunks) {
>          trace_mirror_yield_in_flight(s, op->offset, s->in_flight);
> -        mirror_wait_for_free_in_flight_slot(s);
> +        mirror_co_wait_for_free_in_flight_slot(s);
>      }
>  
>      /* Now make a QEMUIOVector taking enough granularity-sized chunks
> @@ -549,7 +551,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>  
>          while (s->in_flight >= MAX_IN_FLIGHT) {
>              trace_mirror_yield_in_flight(s, offset, s->in_flight);
> -            mirror_wait_for_free_in_flight_slot(s);
> +            mirror_co_wait_for_free_in_flight_slot(s);
>          }
>  
>          if (s->ret < 0) {
> @@ -600,10 +602,10 @@ static void mirror_free_init(MirrorBlockJob *s)
>   * mirror_resume() because mirror_run() will begin iterating again
>   * when the job is resumed.
>   */
> -static void mirror_wait_for_all_io(MirrorBlockJob *s)
> +static void coroutine_fn mirror_co_wait_for_all_io(MirrorBlockJob *s)
>  {
>      while (s->in_flight > 0) {
> -        mirror_wait_for_free_in_flight_slot(s);
> +        mirror_co_wait_for_free_in_flight_slot(s);
>      }
>  }
>  
> @@ -762,7 +764,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              if (s->in_flight >= MAX_IN_FLIGHT) {
>                  trace_mirror_yield(s, UINT64_MAX, s->buf_free_count,
>                                     s->in_flight);
> -                mirror_wait_for_free_in_flight_slot(s);
> +                mirror_co_wait_for_free_in_flight_slot(s);
>                  continue;
>              }
>  
> @@ -770,7 +772,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              offset += bytes;
>          }
>  
> -        mirror_wait_for_all_io(s);
> +        mirror_co_wait_for_all_io(s);
>          s->initial_zeroing_ongoing = false;
>      }
>  
> @@ -916,7 +918,7 @@ static void coroutine_fn mirror_run(void *opaque)
>          /* Do not start passive operations while there are active
>           * writes in progress */
>          while (s->in_active_write_counter) {
> -            mirror_wait_for_any_operation(s, true);
> +            mirror_co_wait_for_any_operation(s, true);
>          }
>  
>          if (s->ret < 0) {
> @@ -942,7 +944,7 @@ static void coroutine_fn mirror_run(void *opaque)
>              if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>                  (cnt == 0 && s->in_flight > 0)) {
>                  trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
> -                mirror_wait_for_free_in_flight_slot(s);
> +                mirror_co_wait_for_free_in_flight_slot(s);
>                  continue;
>              } else if (cnt != 0) {
>                  delay_ns = mirror_iteration(s);
> @@ -1028,7 +1030,7 @@ immediate_exit:
>          assert(ret < 0 || ((s->common.job.force_cancel || !s->synced) &&
>                 job_is_cancelled(&s->common.job)));
>          assert(need_drain);
> -        mirror_wait_for_all_io(s);
> +        mirror_co_wait_for_all_io(s);
>      }
>  
>      assert(s->in_flight == 0);
> @@ -1098,11 +1100,11 @@ static void mirror_complete(Job *job, Error **errp)
>      job_enter(job);
>  }
>  
> -static void mirror_pause(Job *job)
> +static void coroutine_fn mirror_pause(Job *job)
>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>  
> -    mirror_wait_for_all_io(s);
> +    mirror_co_wait_for_all_io(s);
>  }
>  
>  static bool mirror_drained_poll(BlockJob *job)
> -- 
> 2.17.1
> 

[Qemu-devel] [PATCH 03/17] mirror: Pull *_align_for_copy() from *_co_read()

[Max Reitz]

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

diff --git a/block/mirror.c b/block/mirror.c
index c28b6159d5..34cb8293b2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -305,42 +305,60 @@ static inline void coroutine_fn
     mirror_co_wait_for_any_operation(s, false);
 }
 
-/* Perform a mirror copy operation.
+/*
+ * Restrict *bytes to how much we can actually handle, and align the
+ * [*offset, *bytes] range to clusters if COW is needed.
  *
- * *op->bytes_handled is set to the number of bytes copied after and
+ * *bytes_handled is set to the number of bytes copied after and
  * including offset, excluding any bytes copied prior to offset due
- * to alignment.  This will be op->bytes if no alignment is necessary,
- * or (new_end - op->offset) if the tail is rounded up or down due to
+ * to alignment.  This will be *bytes if no alignment is necessary, or
+ * (new_end - *offset) if the tail is rounded up or down due to
  * alignment or buffer limit.
  */
-static void coroutine_fn mirror_co_read(void *opaque)
+static void mirror_align_for_copy(MirrorBlockJob *s,
+                                  int64_t *offset, uint64_t *bytes,
+                                  int64_t *bytes_handled)
 {
-    MirrorOp *op = opaque;
-    MirrorBlockJob *s = op->s;
-    int nb_chunks;
-    uint64_t ret;
     uint64_t max_bytes;
 
     max_bytes = s->granularity * s->max_iov;
 
     /* We can only handle as much as buf_size at a time. */
-    op->bytes = MIN(s->buf_size, MIN(max_bytes, op->bytes));
-    assert(op->bytes);
-    assert(op->bytes < BDRV_REQUEST_MAX_BYTES);
-    *op->bytes_handled = op->bytes;
+    *bytes = MIN(s->buf_size, MIN(max_bytes, *bytes));
+    assert(*bytes);
+    assert(*bytes < BDRV_REQUEST_MAX_BYTES);
+    *bytes_handled = *bytes;
 
     if (s->cow_bitmap) {
-        *op->bytes_handled += mirror_cow_align(s, &op->offset, &op->bytes);
+        *bytes_handled += mirror_cow_align(s, offset, bytes);
     }
     /* Cannot exceed BDRV_REQUEST_MAX_BYTES + INT_MAX */
-    assert(*op->bytes_handled <= UINT_MAX);
-    assert(op->bytes <= s->buf_size);
+    assert(*bytes_handled <= UINT_MAX);
+    assert(*bytes <= s->buf_size);
     /* The offset is granularity-aligned because:
      * 1) Caller passes in aligned values;
      * 2) mirror_cow_align is used only when target cluster is larger. */
-    assert(QEMU_IS_ALIGNED(op->offset, s->granularity));
+    assert(QEMU_IS_ALIGNED(*offset, s->granularity));
     /* The range is sector-aligned, since bdrv_getlength() rounds up. */
-    assert(QEMU_IS_ALIGNED(op->bytes, BDRV_SECTOR_SIZE));
+    assert(QEMU_IS_ALIGNED(*bytes, BDRV_SECTOR_SIZE));
+}
+
+/* Perform a mirror copy operation.
+ *
+ * *op->bytes_handled is set to the number of bytes copied after and
+ * including offset, excluding any bytes copied prior to offset due
+ * to alignment.  This will be op->bytes if no alignment is necessary,
+ * or (new_end - op->offset) if the tail is rounded up or down due to
+ * alignment or buffer limit.
+ */
+static void coroutine_fn mirror_co_read(void *opaque)
+{
+    MirrorOp *op = opaque;
+    MirrorBlockJob *s = op->s;
+    int nb_chunks;
+    uint64_t ret;
+
+    mirror_align_for_copy(s, &op->offset, &op->bytes, op->bytes_handled);
     nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
 
     while (s->buf_free_count < nb_chunks) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 03/17] mirror: Pull *_align_for_copy() from *_co_read()

[Jeff Cody]

On Mon, Aug 13, 2018 at 04:19:52AM +0200, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/mirror.c | 54 +++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index c28b6159d5..34cb8293b2 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -305,42 +305,60 @@ static inline void coroutine_fn
>      mirror_co_wait_for_any_operation(s, false);
>  }
>  
> -/* Perform a mirror copy operation.
> +/*
> + * Restrict *bytes to how much we can actually handle, and align the
> + * [*offset, *bytes] range to clusters if COW is needed.
>   *
> - * *op->bytes_handled is set to the number of bytes copied after and
> + * *bytes_handled is set to the number of bytes copied after and
>   * including offset, excluding any bytes copied prior to offset due
> - * to alignment.  This will be op->bytes if no alignment is necessary,
> - * or (new_end - op->offset) if the tail is rounded up or down due to
> + * to alignment.  This will be *bytes if no alignment is necessary, or
> + * (new_end - *offset) if the tail is rounded up or down due to
>   * alignment or buffer limit.
>   */
> -static void coroutine_fn mirror_co_read(void *opaque)
> +static void mirror_align_for_copy(MirrorBlockJob *s,
> +                                  int64_t *offset, uint64_t *bytes,
> +                                  int64_t *bytes_handled)
>  {
> -    MirrorOp *op = opaque;
> -    MirrorBlockJob *s = op->s;
> -    int nb_chunks;
> -    uint64_t ret;
>      uint64_t max_bytes;
>  
>      max_bytes = s->granularity * s->max_iov;
>  
>      /* We can only handle as much as buf_size at a time. */
> -    op->bytes = MIN(s->buf_size, MIN(max_bytes, op->bytes));
> -    assert(op->bytes);
> -    assert(op->bytes < BDRV_REQUEST_MAX_BYTES);
> -    *op->bytes_handled = op->bytes;
> +    *bytes = MIN(s->buf_size, MIN(max_bytes, *bytes));
> +    assert(*bytes);
> +    assert(*bytes < BDRV_REQUEST_MAX_BYTES);
> +    *bytes_handled = *bytes;
>  
>      if (s->cow_bitmap) {
> -        *op->bytes_handled += mirror_cow_align(s, &op->offset, &op->bytes);
> +        *bytes_handled += mirror_cow_align(s, offset, bytes);
>      }
>      /* Cannot exceed BDRV_REQUEST_MAX_BYTES + INT_MAX */
> -    assert(*op->bytes_handled <= UINT_MAX);
> -    assert(op->bytes <= s->buf_size);
> +    assert(*bytes_handled <= UINT_MAX);
> +    assert(*bytes <= s->buf_size);
>      /* The offset is granularity-aligned because:
>       * 1) Caller passes in aligned values;
>       * 2) mirror_cow_align is used only when target cluster is larger. */
> -    assert(QEMU_IS_ALIGNED(op->offset, s->granularity));
> +    assert(QEMU_IS_ALIGNED(*offset, s->granularity));
>      /* The range is sector-aligned, since bdrv_getlength() rounds up. */
> -    assert(QEMU_IS_ALIGNED(op->bytes, BDRV_SECTOR_SIZE));
> +    assert(QEMU_IS_ALIGNED(*bytes, BDRV_SECTOR_SIZE));
> +}
> +
> +/* Perform a mirror copy operation.
> + *
> + * *op->bytes_handled is set to the number of bytes copied after and
> + * including offset, excluding any bytes copied prior to offset due
> + * to alignment.  This will be op->bytes if no alignment is necessary,
> + * or (new_end - op->offset) if the tail is rounded up or down due to
> + * alignment or buffer limit.
> + */
> +static void coroutine_fn mirror_co_read(void *opaque)
> +{
> +    MirrorOp *op = opaque;
> +    MirrorBlockJob *s = op->s;
> +    int nb_chunks;
> +    uint64_t ret;
> +
> +    mirror_align_for_copy(s, &op->offset, &op->bytes, op->bytes_handled);
>      nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
>  
>      while (s->buf_free_count < nb_chunks) {
> -- 
> 2.17.1
> 

[Qemu-devel] [PATCH 04/17] mirror: Remove bytes_handled, part 1

[Max Reitz]

By moving the mirror_align_for_copy() call from mirror_co_read() to
mirror_perform(), we can drop bytes_handled from MirrorOp.

mirror_align_for_copy() takes a uint64_t * for @bytes, so this commit
changes mirror_perform()'s @bytes parameter to uint64_t, too; but it
still asserts that its value does not exceed UINT_MAX (necessary because
it may be assigned to bytes_handled, which is returned and may thus not
exceed UINT_MAX).  This assertion is removed in a later patch.

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

diff --git a/block/mirror.c b/block/mirror.c
index 34cb8293b2..3234b8b687 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -91,10 +91,6 @@ struct MirrorOp {
     int64_t offset;
     uint64_t bytes;
 
-    /* The pointee is set by mirror_co_read(), mirror_co_zero(), and
-     * mirror_co_discard() before yielding for the first time */
-    int64_t *bytes_handled;
-
     bool is_pseudo_op;
     bool is_active_write;
     CoQueue waiting_requests;
@@ -343,14 +339,7 @@ static void mirror_align_for_copy(MirrorBlockJob *s,
     assert(QEMU_IS_ALIGNED(*bytes, BDRV_SECTOR_SIZE));
 }
 
-/* Perform a mirror copy operation.
- *
- * *op->bytes_handled is set to the number of bytes copied after and
- * including offset, excluding any bytes copied prior to offset due
- * to alignment.  This will be op->bytes if no alignment is necessary,
- * or (new_end - op->offset) if the tail is rounded up or down due to
- * alignment or buffer limit.
- */
+/* Perform a mirror copy operation. */
 static void coroutine_fn mirror_co_read(void *opaque)
 {
     MirrorOp *op = opaque;
@@ -358,7 +347,6 @@ static void coroutine_fn mirror_co_read(void *opaque)
     int nb_chunks;
     uint64_t ret;
 
-    mirror_align_for_copy(s, &op->offset, &op->bytes, op->bytes_handled);
     nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
 
     while (s->buf_free_count < nb_chunks) {
@@ -396,7 +384,6 @@ static void coroutine_fn mirror_co_zero(void *opaque)
 
     op->s->in_flight++;
     op->s->bytes_in_flight += op->bytes;
-    *op->bytes_handled = op->bytes;
 
     ret = blk_co_pwrite_zeroes(op->s->target, op->offset, op->bytes,
                                op->s->unmap ? BDRV_REQ_MAY_UNMAP : 0);
@@ -410,25 +397,33 @@ static void coroutine_fn mirror_co_discard(void *opaque)
 
     op->s->in_flight++;
     op->s->bytes_in_flight += op->bytes;
-    *op->bytes_handled = op->bytes;
 
     ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes);
     mirror_write_complete(op, ret);
 }
 
 static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
-                               unsigned bytes, MirrorMethod mirror_method)
+                               uint64_t bytes, MirrorMethod mirror_method)
 {
     MirrorOp *op;
     Coroutine *co;
-    int64_t bytes_handled = -1;
+    int64_t bytes_handled;
+
+    /* FIXME: Drop this assertion */
+    assert(bytes <= UINT_MAX);
+
+    if (mirror_method == MIRROR_METHOD_COPY) {
+        mirror_align_for_copy(s, &offset, &bytes, &bytes_handled);
+        assert(bytes_handled <= UINT_MAX);
+    } else {
+        bytes_handled = bytes;
+    }
 
     op = g_new(MirrorOp, 1);
     *op = (MirrorOp){
         .s              = s,
         .offset         = offset,
         .bytes          = bytes,
-        .bytes_handled  = &bytes_handled,
     };
     qemu_co_queue_init(&op->waiting_requests);
 
@@ -448,16 +443,7 @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
 
     QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next);
     qemu_coroutine_enter(co);
-    /* At this point, ownership of op has been moved to the coroutine
-     * and the object may already be freed */
-
-    /* Assert that this value has been set */
-    assert(bytes_handled >= 0);
 
-    /* Same assertion as in mirror_co_read() (and for mirror_co_read()
-     * and mirror_co_discard(), bytes_handled == op->bytes, which
-     * is the @bytes parameter given to this function) */
-    assert(bytes_handled <= UINT_MAX);
     return bytes_handled;
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 05/17] mirror: Remove bytes_handled, part 2

[Max Reitz]

Remove mirror_perform()'s return value, instead aligning the @offset and
@bytes it receives.  We needed the return value for two reasons:

(1) So that "offset += io_bytes" would result in an aligned offset, and
(2) so that io_bytes_acct is kind of aligned (the tail is aligned, but
    the head is not).

(2) does not make sense.  If we want to account for the bytes we have to
copy, we should not have used the returned io_bytes as the basis but its
original value before the mirror_perform() call.  If we want to account
for the bytes we have actually copied, we should not disregard the
potential alignment head, but bytes_handled only includes the tail, so
that was wrong, too.
Let's go for the fully aligned value (the number of bytes actually
copied), because apparently we do want some alignment (or we would have
just added io_bytes before this patch instead of bytes_handled).

(1) can be achieved by simply letting mirror_perform() return the
aligned values to its callers, which is what this patch does.

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

diff --git a/block/mirror.c b/block/mirror.c
index 3234b8b687..f05404e557 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -89,7 +89,7 @@ struct MirrorOp {
     MirrorBlockJob *s;
     QEMUIOVector qiov;
     int64_t offset;
-    uint64_t bytes;
+    int64_t bytes;
 
     bool is_pseudo_op;
     bool is_active_write;
@@ -239,13 +239,10 @@ static inline int64_t mirror_clip_bytes(MirrorBlockJob *s,
     return MIN(bytes, s->bdev_length - offset);
 }
 
-/* Round offset and/or bytes to target cluster if COW is needed, and
- * return the offset of the adjusted tail against original. */
-static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
-                            uint64_t *bytes)
+/* Round offset and/or bytes to target cluster if COW is needed */
+static void mirror_cow_align(MirrorBlockJob *s, int64_t *offset, int64_t *bytes)
 {
     bool need_cow;
-    int ret = 0;
     int64_t align_offset = *offset;
     int64_t align_bytes = *bytes;
     int max_bytes = s->granularity * s->max_iov;
@@ -268,11 +265,8 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
      * that doesn't matter because it's already the end of source image. */
     align_bytes = mirror_clip_bytes(s, align_offset, align_bytes);
 
-    ret = align_offset + align_bytes - (*offset + *bytes);
     *offset = align_offset;
     *bytes = align_bytes;
-    assert(ret >= 0);
-    return ret;
 }
 
 static inline void coroutine_fn
@@ -304,16 +298,9 @@ static inline void coroutine_fn
 /*
  * Restrict *bytes to how much we can actually handle, and align the
  * [*offset, *bytes] range to clusters if COW is needed.
- *
- * *bytes_handled is set to the number of bytes copied after and
- * including offset, excluding any bytes copied prior to offset due
- * to alignment.  This will be *bytes if no alignment is necessary, or
- * (new_end - *offset) if the tail is rounded up or down due to
- * alignment or buffer limit.
  */
 static void mirror_align_for_copy(MirrorBlockJob *s,
-                                  int64_t *offset, uint64_t *bytes,
-                                  int64_t *bytes_handled)
+                                  int64_t *offset, int64_t *bytes)
 {
     uint64_t max_bytes;
 
@@ -323,13 +310,11 @@ static void mirror_align_for_copy(MirrorBlockJob *s,
     *bytes = MIN(s->buf_size, MIN(max_bytes, *bytes));
     assert(*bytes);
     assert(*bytes < BDRV_REQUEST_MAX_BYTES);
-    *bytes_handled = *bytes;
 
     if (s->cow_bitmap) {
-        *bytes_handled += mirror_cow_align(s, offset, bytes);
+        mirror_cow_align(s, offset, bytes);
     }
-    /* Cannot exceed BDRV_REQUEST_MAX_BYTES + INT_MAX */
-    assert(*bytes_handled <= UINT_MAX);
+
     assert(*bytes <= s->buf_size);
     /* The offset is granularity-aligned because:
      * 1) Caller passes in aligned values;
@@ -402,28 +387,23 @@ static void coroutine_fn mirror_co_discard(void *opaque)
     mirror_write_complete(op, ret);
 }
 
-static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
-                               uint64_t bytes, MirrorMethod mirror_method)
+/* If mirror_method == MIRROR_METHOD_COPY, *offset and *bytes will be
+ * aligned as necessary */
+static void mirror_perform(MirrorBlockJob *s, int64_t *offset, int64_t *bytes,
+                           MirrorMethod mirror_method)
 {
     MirrorOp *op;
     Coroutine *co;
-    int64_t bytes_handled;
-
-    /* FIXME: Drop this assertion */
-    assert(bytes <= UINT_MAX);
 
     if (mirror_method == MIRROR_METHOD_COPY) {
-        mirror_align_for_copy(s, &offset, &bytes, &bytes_handled);
-        assert(bytes_handled <= UINT_MAX);
-    } else {
-        bytes_handled = bytes;
+        mirror_align_for_copy(s, offset, bytes);
     }
 
     op = g_new(MirrorOp, 1);
     *op = (MirrorOp){
         .s              = s,
-        .offset         = offset,
-        .bytes          = bytes,
+        .offset         = *offset,
+        .bytes          = *bytes,
     };
     qemu_co_queue_init(&op->waiting_requests);
 
@@ -443,8 +423,6 @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
 
     QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next);
     qemu_coroutine_enter(co);
-
-    return bytes_handled;
 }
 
 static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
@@ -564,7 +542,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         }
 
         io_bytes = mirror_clip_bytes(s, offset, io_bytes);
-        io_bytes = mirror_perform(s, offset, io_bytes, mirror_method);
+        mirror_perform(s, &offset, &io_bytes, mirror_method);
         if (mirror_method != MIRROR_METHOD_COPY && write_zeroes_ok) {
             io_bytes_acct = 0;
         } else {
@@ -755,8 +733,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 
         s->initial_zeroing_ongoing = true;
         for (offset = 0; offset < s->bdev_length; ) {
-            int bytes = MIN(s->bdev_length - offset,
-                            QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
+            int64_t bytes = MIN(s->bdev_length - offset,
+                                QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
             mirror_throttle(s);
 
@@ -772,7 +750,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
                 continue;
             }
 
-            mirror_perform(s, offset, bytes, MIRROR_METHOD_ZERO);
+            mirror_perform(s, &offset, &bytes, MIRROR_METHOD_ZERO);
             offset += bytes;
         }
 
-- 
2.17.1

[Qemu-devel] [PATCH 06/17] mirror: Create mirror_co_perform()

[Max Reitz]

While very simple now, this function will be fattened in future patches.

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

diff --git a/block/mirror.c b/block/mirror.c
index f05404e557..89452ad371 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -85,8 +85,16 @@ typedef struct MirrorBDSOpaque {
     MirrorBlockJob *job;
 } MirrorBDSOpaque;
 
+typedef enum MirrorMethod {
+    MIRROR_METHOD_COPY,
+    MIRROR_METHOD_ZERO,
+    MIRROR_METHOD_DISCARD,
+} MirrorMethod;
+
 struct MirrorOp {
     MirrorBlockJob *s;
+    MirrorMethod mirror_method;
+
     QEMUIOVector qiov;
     int64_t offset;
     int64_t bytes;
@@ -98,12 +106,6 @@ struct MirrorOp {
     QTAILQ_ENTRY(MirrorOp) next;
 };
 
-typedef enum MirrorMethod {
-    MIRROR_METHOD_COPY,
-    MIRROR_METHOD_ZERO,
-    MIRROR_METHOD_DISCARD,
-} MirrorMethod;
-
 static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
                                             int error)
 {
@@ -387,6 +389,25 @@ static void coroutine_fn mirror_co_discard(void *opaque)
     mirror_write_complete(op, ret);
 }
 
+static void coroutine_fn mirror_co_perform(void *opaque)
+{
+    MirrorOp *op = opaque;
+
+    switch (op->mirror_method) {
+    case MIRROR_METHOD_COPY:
+        mirror_co_read(opaque);
+        return;
+    case MIRROR_METHOD_ZERO:
+        mirror_co_zero(opaque);
+        return;
+    case MIRROR_METHOD_DISCARD:
+        mirror_co_discard(opaque);
+        return;
+    default:
+        abort();
+    }
+}
+
 /* If mirror_method == MIRROR_METHOD_COPY, *offset and *bytes will be
  * aligned as necessary */
 static void mirror_perform(MirrorBlockJob *s, int64_t *offset, int64_t *bytes,
@@ -402,24 +423,13 @@ static void mirror_perform(MirrorBlockJob *s, int64_t *offset, int64_t *bytes,
     op = g_new(MirrorOp, 1);
     *op = (MirrorOp){
         .s              = s,
+        .mirror_method  = mirror_method,
         .offset         = *offset,
         .bytes          = *bytes,
     };
     qemu_co_queue_init(&op->waiting_requests);
 
-    switch (mirror_method) {
-    case MIRROR_METHOD_COPY:
-        co = qemu_coroutine_create(mirror_co_read, op);
-        break;
-    case MIRROR_METHOD_ZERO:
-        co = qemu_coroutine_create(mirror_co_zero, op);
-        break;
-    case MIRROR_METHOD_DISCARD:
-        co = qemu_coroutine_create(mirror_co_discard, op);
-        break;
-    default:
-        abort();
-    }
+    co = qemu_coroutine_create(mirror_co_perform, op);
 
     QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next);
     qemu_coroutine_enter(co);
-- 
2.17.1

[Qemu-devel] [PATCH 07/17] mirror: Make mirror_co_zero() nicer

[Max Reitz]

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

diff --git a/block/mirror.c b/block/mirror.c
index 89452ad371..df8e0242dc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -364,17 +364,14 @@ static void coroutine_fn mirror_co_read(void *opaque)
     mirror_read_complete(op, ret);
 }
 
-static void coroutine_fn mirror_co_zero(void *opaque)
+static int coroutine_fn mirror_co_zero(MirrorBlockJob *s,
+                                       int64_t offset, int64_t bytes)
 {
-    MirrorOp *op = opaque;
-    int ret;
-
-    op->s->in_flight++;
-    op->s->bytes_in_flight += op->bytes;
+    s->in_flight++;
+    s->bytes_in_flight += bytes;
 
-    ret = blk_co_pwrite_zeroes(op->s->target, op->offset, op->bytes,
-                               op->s->unmap ? BDRV_REQ_MAY_UNMAP : 0);
-    mirror_write_complete(op, ret);
+    return blk_co_pwrite_zeroes(s->target, offset, bytes,
+                                s->unmap ? BDRV_REQ_MAY_UNMAP : 0);
 }
 
 static void coroutine_fn mirror_co_discard(void *opaque)
@@ -392,20 +389,24 @@ static void coroutine_fn mirror_co_discard(void *opaque)
 static void coroutine_fn mirror_co_perform(void *opaque)
 {
     MirrorOp *op = opaque;
+    MirrorBlockJob *s = op->s;
+    int ret;
 
     switch (op->mirror_method) {
     case MIRROR_METHOD_COPY:
         mirror_co_read(opaque);
         return;
     case MIRROR_METHOD_ZERO:
-        mirror_co_zero(opaque);
-        return;
+        ret = mirror_co_zero(s, op->offset, op->bytes);
+        break;
     case MIRROR_METHOD_DISCARD:
         mirror_co_discard(opaque);
         return;
     default:
         abort();
     }
+
+    mirror_write_complete(op, ret);
 }
 
 /* If mirror_method == MIRROR_METHOD_COPY, *offset and *bytes will be
-- 
2.17.1

[Qemu-devel] [PATCH 08/17] mirror: Make mirror_co_discard() nicer

[Max Reitz]

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

diff --git a/block/mirror.c b/block/mirror.c
index df8e0242dc..85b08086cc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -374,16 +374,13 @@ static int coroutine_fn mirror_co_zero(MirrorBlockJob *s,
                                 s->unmap ? BDRV_REQ_MAY_UNMAP : 0);
 }
 
-static void coroutine_fn mirror_co_discard(void *opaque)
+static int coroutine_fn mirror_co_discard(MirrorBlockJob *s,
+                                          int64_t offset, int64_t bytes)
 {
-    MirrorOp *op = opaque;
-    int ret;
-
-    op->s->in_flight++;
-    op->s->bytes_in_flight += op->bytes;
+    s->in_flight++;
+    s->bytes_in_flight += bytes;
 
-    ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes);
-    mirror_write_complete(op, ret);
+    return blk_co_pdiscard(s->target, offset, bytes);
 }
 
 static void coroutine_fn mirror_co_perform(void *opaque)
@@ -400,8 +397,8 @@ static void coroutine_fn mirror_co_perform(void *opaque)
         ret = mirror_co_zero(s, op->offset, op->bytes);
         break;
     case MIRROR_METHOD_DISCARD:
-        mirror_co_discard(opaque);
-        return;
+        ret = mirror_co_discard(s, op->offset, op->bytes);
+        break;
     default:
         abort();
     }
-- 
2.17.1

[Qemu-devel] [PATCH 09/17] mirror: Lock AioContext in mirror_co_perform()

[Max Reitz]

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

diff --git a/block/mirror.c b/block/mirror.c
index 85b08086cc..6330269156 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -196,7 +196,6 @@ static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
 {
     MirrorBlockJob *s = op->s;
 
-    aio_context_acquire(blk_get_aio_context(s->common.blk));
     if (ret < 0) {
         BlockErrorAction action;
 
@@ -207,14 +206,12 @@ static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
         }
     }
     mirror_iteration_done(op, ret);
-    aio_context_release(blk_get_aio_context(s->common.blk));
 }
 
 static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
 {
     MirrorBlockJob *s = op->s;
 
-    aio_context_acquire(blk_get_aio_context(s->common.blk));
     if (ret < 0) {
         BlockErrorAction action;
 
@@ -230,7 +227,6 @@ static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
                              op->qiov.size, &op->qiov, 0);
         mirror_write_complete(op, ret);
     }
-    aio_context_release(blk_get_aio_context(s->common.blk));
 }
 
 /* Clip bytes relative to offset to not exceed end-of-file */
@@ -387,12 +383,16 @@ static void coroutine_fn mirror_co_perform(void *opaque)
 {
     MirrorOp *op = opaque;
     MirrorBlockJob *s = op->s;
+    AioContext *aio_context;
     int ret;
 
+    aio_context = blk_get_aio_context(s->common.blk);
+    aio_context_acquire(aio_context);
+
     switch (op->mirror_method) {
     case MIRROR_METHOD_COPY:
         mirror_co_read(opaque);
-        return;
+        goto done;
     case MIRROR_METHOD_ZERO:
         ret = mirror_co_zero(s, op->offset, op->bytes);
         break;
@@ -404,6 +404,9 @@ static void coroutine_fn mirror_co_perform(void *opaque)
     }
 
     mirror_write_complete(op, ret);
+
+done:
+    aio_context_release(aio_context);
 }
 
 /* If mirror_method == MIRROR_METHOD_COPY, *offset and *bytes will be
-- 
2.17.1

Re: [Qemu-devel] [Qemu-block] [PATCH 09/17] mirror: Lock AioContext in mirror_co_perform()

[Paolo Bonzini]

On 13/08/2018 04:19, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Locking AioContext should not be needed anywhere here.  mirror_run is
called via aio_co_enter or aio_co_wake, so the lock is actually already
taken every time you call aio_context_acquire.

It was needed only because AIO callbacks do *not* take the lock, but
it's not needed anymore since the conversion to coroutines.

Paolo

>  block/mirror.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 85b08086cc..6330269156 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -196,7 +196,6 @@ static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
>  {
>      MirrorBlockJob *s = op->s;
>  
> -    aio_context_acquire(blk_get_aio_context(s->common.blk));
>      if (ret < 0) {
>          BlockErrorAction action;
>  
> @@ -207,14 +206,12 @@ static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
>          }
>      }
>      mirror_iteration_done(op, ret);
> -    aio_context_release(blk_get_aio_context(s->common.blk));
>  }
>  
>  static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
>  {
>      MirrorBlockJob *s = op->s;
>  
> -    aio_context_acquire(blk_get_aio_context(s->common.blk));
>      if (ret < 0) {
>          BlockErrorAction action;
>  
> @@ -230,7 +227,6 @@ static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
>                               op->qiov.size, &op->qiov, 0);
>          mirror_write_complete(op, ret);
>      }
> -    aio_context_release(blk_get_aio_context(s->common.blk));
>  }
>  
>  /* Clip bytes relative to offset to not exceed end-of-file */
> @@ -387,12 +383,16 @@ static void coroutine_fn mirror_co_perform(void *opaque)
>  {
>      MirrorOp *op = opaque;
>      MirrorBlockJob *s = op->s;
> +    AioContext *aio_context;
>      int ret;
>  
> +    aio_context = blk_get_aio_context(s->common.blk);
> +    aio_context_acquire(aio_context);
> +
>      switch (op->mirror_method) {
>      case MIRROR_METHOD_COPY:
>          mirror_co_read(opaque);
> -        return;
> +        goto done;
>      case MIRROR_METHOD_ZERO:
>          ret = mirror_co_zero(s, op->offset, op->bytes);
>          break;
> @@ -404,6 +404,9 @@ static void coroutine_fn mirror_co_perform(void *opaque)
>      }
>  
>      mirror_write_complete(op, ret);
> +
> +done:
> +    aio_context_release(aio_context);
>  }
>  
>  /* If mirror_method == MIRROR_METHOD_COPY, *offset and *bytes will be
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 09/17] mirror: Lock AioContext in mirror_co_perform()

[Max Reitz]

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

On 2018-08-13 16:43, Paolo Bonzini wrote:
> On 13/08/2018 04:19, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Locking AioContext should not be needed anywhere here.  mirror_run is
> called via aio_co_enter or aio_co_wake, so the lock is actually already
> taken every time you call aio_context_acquire.
> 
> It was needed only because AIO callbacks do *not* take the lock, but
> it's not needed anymore since the conversion to coroutines.

Uh, nice.  Thanks for letting me know! :-)

I'll drop the locks in v2.

Max


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

[Qemu-devel] [PATCH 10/17] mirror: Create mirror_co_alloc_qiov()

[Max Reitz]

By pulling this function out of mirror_co_read(), the latter becomes
simpler.  This is important so the following patches can make it more
complicated again.

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

diff --git a/block/mirror.c b/block/mirror.c
index 6330269156..2a131d8b99 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -293,6 +293,29 @@ static inline void coroutine_fn
     mirror_co_wait_for_any_operation(s, false);
 }
 
+/* Allocate a QEMUIOVector from the mirror buffer pool (s->buf_free) */
+static void coroutine_fn mirror_co_alloc_qiov(MirrorBlockJob *s,
+                                              QEMUIOVector *qiov,
+                                              int64_t offset, uint64_t bytes)
+{
+    int nb_chunks = DIV_ROUND_UP(bytes, s->granularity);
+
+    while (s->buf_free_count < nb_chunks) {
+        trace_mirror_yield_in_flight(s, offset, s->in_flight);
+        mirror_co_wait_for_free_in_flight_slot(s);
+    }
+
+    qemu_iovec_init(qiov, nb_chunks);
+    while (nb_chunks-- > 0) {
+        MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);
+        size_t remaining = bytes - qiov->size;
+
+        QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);
+        s->buf_free_count--;
+        qemu_iovec_add(qiov, buf, MIN(s->granularity, remaining));
+    }
+}
+
 /*
  * Restrict *bytes to how much we can actually handle, and align the
  * [*offset, *bytes] range to clusters if COW is needed.
@@ -327,28 +350,9 @@ static void coroutine_fn mirror_co_read(void *opaque)
 {
     MirrorOp *op = opaque;
     MirrorBlockJob *s = op->s;
-    int nb_chunks;
     uint64_t ret;
 
-    nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
-
-    while (s->buf_free_count < nb_chunks) {
-        trace_mirror_yield_in_flight(s, op->offset, s->in_flight);
-        mirror_co_wait_for_free_in_flight_slot(s);
-    }
-
-    /* Now make a QEMUIOVector taking enough granularity-sized chunks
-     * from s->buf_free.
-     */
-    qemu_iovec_init(&op->qiov, nb_chunks);
-    while (nb_chunks-- > 0) {
-        MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);
-        size_t remaining = op->bytes - op->qiov.size;
-
-        QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);
-        s->buf_free_count--;
-        qemu_iovec_add(&op->qiov, buf, MIN(s->granularity, remaining));
-    }
+    mirror_co_alloc_qiov(s, &op->qiov, op->offset, op->bytes);
 
     /* Copy the dirty cluster.  */
     s->in_flight++;
-- 
2.17.1

[Qemu-devel] [PATCH 11/17] mirror: Inline mirror_write_complete(), part 1

[Max Reitz]

Eventually, we want to inline mirror_write_complete() fully into
mirror_co_perform().  This patch does the inlining, but we cannot remove
the function yet, as it is still required by mirror_co_read().

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

diff --git a/block/mirror.c b/block/mirror.c
index 2a131d8b99..66746cf075 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -407,7 +407,17 @@ static void coroutine_fn mirror_co_perform(void *opaque)
         abort();
     }
 
-    mirror_write_complete(op, ret);
+    if (ret < 0) {
+        BlockErrorAction action;
+
+        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
+        action = mirror_error_action(s, false, -ret);
+        if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
+            s->ret = ret;
+        }
+    }
+
+    mirror_iteration_done(op, ret);
 
 done:
     aio_context_release(aio_context);
-- 
2.17.1

[Qemu-devel] [PATCH 12/17] mirror: Put QIOV locally into mirror_co_read

[Max Reitz]

We only need the I/O vector for data copying operations, so we do not
need to put it into the MirrorOp structure and can keep it locally in
mirror_co_read().

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

diff --git a/block/mirror.c b/block/mirror.c
index 66746cf075..f3df7dd984 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -95,7 +95,6 @@ struct MirrorOp {
     MirrorBlockJob *s;
     MirrorMethod mirror_method;
 
-    QEMUIOVector qiov;
     int64_t offset;
     int64_t bytes;
 
@@ -155,7 +154,8 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
     }
 }
 
-static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
+static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret,
+                                               QEMUIOVector *qiov)
 {
     MirrorBlockJob *s = op->s;
     struct iovec *iov;
@@ -166,11 +166,13 @@ static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
 
     s->in_flight--;
     s->bytes_in_flight -= op->bytes;
-    iov = op->qiov.iov;
-    for (i = 0; i < op->qiov.niov; i++) {
-        MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
-        QSIMPLEQ_INSERT_TAIL(&s->buf_free, buf, next);
-        s->buf_free_count++;
+    if (qiov) {
+        iov = qiov->iov;
+        for (i = 0; i < qiov->niov; i++) {
+            MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
+            QSIMPLEQ_INSERT_TAIL(&s->buf_free, buf, next);
+            s->buf_free_count++;
+        }
     }
 
     chunk_num = op->offset / s->granularity;
@@ -186,13 +188,16 @@ static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
             job_progress_update(&s->common.job, op->bytes);
         }
     }
-    qemu_iovec_destroy(&op->qiov);
+    if (qiov) {
+        qemu_iovec_destroy(qiov);
+    }
 
     qemu_co_queue_restart_all(&op->waiting_requests);
     g_free(op);
 }
 
-static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
+static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret,
+                                               QEMUIOVector *qiov)
 {
     MirrorBlockJob *s = op->s;
 
@@ -205,10 +210,11 @@ static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
             s->ret = ret;
         }
     }
-    mirror_iteration_done(op, ret);
+    mirror_iteration_done(op, ret, qiov);
 }
 
-static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
+static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret,
+                                              QEMUIOVector *qiov)
 {
     MirrorBlockJob *s = op->s;
 
@@ -221,11 +227,11 @@ static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
             s->ret = ret;
         }
 
-        mirror_iteration_done(op, ret);
+        mirror_iteration_done(op, ret, qiov);
     } else {
         ret = blk_co_pwritev(s->target, op->offset,
-                             op->qiov.size, &op->qiov, 0);
-        mirror_write_complete(op, ret);
+                             qiov->size, qiov, 0);
+        mirror_write_complete(op, ret, qiov);
     }
 }
 
@@ -350,9 +356,10 @@ static void coroutine_fn mirror_co_read(void *opaque)
 {
     MirrorOp *op = opaque;
     MirrorBlockJob *s = op->s;
+    QEMUIOVector qiov;
     uint64_t ret;
 
-    mirror_co_alloc_qiov(s, &op->qiov, op->offset, op->bytes);
+    mirror_co_alloc_qiov(s, &qiov, op->offset, op->bytes);
 
     /* Copy the dirty cluster.  */
     s->in_flight++;
@@ -360,8 +367,8 @@ static void coroutine_fn mirror_co_read(void *opaque)
     trace_mirror_one_iteration(s, op->offset, op->bytes);
 
     ret = bdrv_co_preadv(s->mirror_top_bs->backing, op->offset, op->bytes,
-                         &op->qiov, 0);
-    mirror_read_complete(op, ret);
+                         &qiov, 0);
+    mirror_read_complete(op, ret, &qiov);
 }
 
 static int coroutine_fn mirror_co_zero(MirrorBlockJob *s,
@@ -417,7 +424,7 @@ static void coroutine_fn mirror_co_perform(void *opaque)
         }
     }
 
-    mirror_iteration_done(op, ret);
+    mirror_iteration_done(op, ret, NULL);
 
 done:
     aio_context_release(aio_context);
-- 
2.17.1

[Qemu-devel] [PATCH 13/17] mirror: Linearize mirror_co_read()

[Max Reitz]

This function inlines mirror_read_complete() and mirror_write_complete()
into mirror_co_read() (which is thus renamed to mirror_co_copy()).  In
addition, freeing of the I/O vector is removed from
mirror_iteration_done() and put into an own function mirror_free_qiov()
(which is called by mirror_co_copy()).

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

diff --git a/block/mirror.c b/block/mirror.c
index f3df7dd984..62fd499799 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -154,26 +154,16 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
     }
 }
 
-static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret,
-                                               QEMUIOVector *qiov)
+static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
 {
     MirrorBlockJob *s = op->s;
-    struct iovec *iov;
     int64_t chunk_num;
-    int i, nb_chunks;
+    int nb_chunks;
 
     trace_mirror_iteration_done(s, op->offset, op->bytes, ret);
 
     s->in_flight--;
     s->bytes_in_flight -= op->bytes;
-    if (qiov) {
-        iov = qiov->iov;
-        for (i = 0; i < qiov->niov; i++) {
-            MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
-            QSIMPLEQ_INSERT_TAIL(&s->buf_free, buf, next);
-            s->buf_free_count++;
-        }
-    }
 
     chunk_num = op->offset / s->granularity;
     nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
@@ -188,53 +178,11 @@ static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret,
             job_progress_update(&s->common.job, op->bytes);
         }
     }
-    if (qiov) {
-        qemu_iovec_destroy(qiov);
-    }
 
     qemu_co_queue_restart_all(&op->waiting_requests);
     g_free(op);
 }
 
-static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret,
-                                               QEMUIOVector *qiov)
-{
-    MirrorBlockJob *s = op->s;
-
-    if (ret < 0) {
-        BlockErrorAction action;
-
-        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
-        action = mirror_error_action(s, false, -ret);
-        if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
-            s->ret = ret;
-        }
-    }
-    mirror_iteration_done(op, ret, qiov);
-}
-
-static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret,
-                                              QEMUIOVector *qiov)
-{
-    MirrorBlockJob *s = op->s;
-
-    if (ret < 0) {
-        BlockErrorAction action;
-
-        bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
-        action = mirror_error_action(s, true, -ret);
-        if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
-            s->ret = ret;
-        }
-
-        mirror_iteration_done(op, ret, qiov);
-    } else {
-        ret = blk_co_pwritev(s->target, op->offset,
-                             qiov->size, qiov, 0);
-        mirror_write_complete(op, ret, qiov);
-    }
-}
-
 /* Clip bytes relative to offset to not exceed end-of-file */
 static inline int64_t mirror_clip_bytes(MirrorBlockJob *s,
                                         int64_t offset,
@@ -322,6 +270,19 @@ static void coroutine_fn mirror_co_alloc_qiov(MirrorBlockJob *s,
     }
 }
 
+static void mirror_free_qiov(MirrorBlockJob *s, QEMUIOVector *qiov)
+{
+    struct iovec *iov = qiov->iov;
+    int i;
+
+    for (i = 0; i < qiov->niov; i++) {
+        MirrorBuffer *buf = (MirrorBuffer *) iov[i].iov_base;
+        QSIMPLEQ_INSERT_TAIL(&s->buf_free, buf, next);
+        s->buf_free_count++;
+    }
+    qemu_iovec_destroy(qiov);
+}
+
 /*
  * Restrict *bytes to how much we can actually handle, and align the
  * [*offset, *bytes] range to clusters if COW is needed.
@@ -351,24 +312,41 @@ static void mirror_align_for_copy(MirrorBlockJob *s,
     assert(QEMU_IS_ALIGNED(*bytes, BDRV_SECTOR_SIZE));
 }
 
-/* Perform a mirror copy operation. */
-static void coroutine_fn mirror_co_read(void *opaque)
+/*
+ * Perform a mirror copy operation.
+ *
+ * On error, -errno is returned and *failed_on_read is set to the
+ * appropriate value.
+ */
+static int coroutine_fn mirror_co_copy(MirrorBlockJob *s,
+                                       int64_t offset, int64_t bytes,
+                                       bool *failed_on_read)
 {
-    MirrorOp *op = opaque;
-    MirrorBlockJob *s = op->s;
     QEMUIOVector qiov;
-    uint64_t ret;
+    int ret;
 
-    mirror_co_alloc_qiov(s, &qiov, op->offset, op->bytes);
+    mirror_co_alloc_qiov(s, &qiov, offset, bytes);
 
     /* Copy the dirty cluster.  */
     s->in_flight++;
-    s->bytes_in_flight += op->bytes;
-    trace_mirror_one_iteration(s, op->offset, op->bytes);
+    s->bytes_in_flight += bytes;
+    trace_mirror_one_iteration(s, offset, bytes);
+
+    ret = bdrv_co_preadv(s->mirror_top_bs->backing, offset, bytes, &qiov, 0);
+    if (ret < 0) {
+        *failed_on_read = true;
+        goto fail;
+    }
 
-    ret = bdrv_co_preadv(s->mirror_top_bs->backing, op->offset, op->bytes,
-                         &qiov, 0);
-    mirror_read_complete(op, ret, &qiov);
+    ret = blk_co_pwritev(s->target, offset, bytes, &qiov, 0);
+    if (ret < 0) {
+        *failed_on_read = false;
+        goto fail;
+    }
+
+fail:
+    mirror_free_qiov(s, &qiov);
+    return ret;
 }
 
 static int coroutine_fn mirror_co_zero(MirrorBlockJob *s,
@@ -395,6 +373,7 @@ static void coroutine_fn mirror_co_perform(void *opaque)
     MirrorOp *op = opaque;
     MirrorBlockJob *s = op->s;
     AioContext *aio_context;
+    bool failed_on_read = false;
     int ret;
 
     aio_context = blk_get_aio_context(s->common.blk);
@@ -402,8 +381,8 @@ static void coroutine_fn mirror_co_perform(void *opaque)
 
     switch (op->mirror_method) {
     case MIRROR_METHOD_COPY:
-        mirror_co_read(opaque);
-        goto done;
+        ret = mirror_co_copy(s, op->offset, op->bytes, &failed_on_read);
+        break;
     case MIRROR_METHOD_ZERO:
         ret = mirror_co_zero(s, op->offset, op->bytes);
         break;
@@ -418,15 +397,14 @@ static void coroutine_fn mirror_co_perform(void *opaque)
         BlockErrorAction action;
 
         bdrv_set_dirty_bitmap(s->dirty_bitmap, op->offset, op->bytes);
-        action = mirror_error_action(s, false, -ret);
+        action = mirror_error_action(s, failed_on_read, -ret);
         if (action == BLOCK_ERROR_ACTION_REPORT && s->ret >= 0) {
             s->ret = ret;
         }
     }
 
-    mirror_iteration_done(op, ret, NULL);
+    mirror_iteration_done(op, ret);
 
-done:
     aio_context_release(aio_context);
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 14/17] mirror: Inline mirror_iteration_done()

[Max Reitz]

mirror_co_perform() is the sole user of that function, and it looks a
bit weird now.  This patch inlines it into mirror_co_perform().

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

diff --git a/block/mirror.c b/block/mirror.c
index 62fd499799..053c37b6a6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -154,35 +154,6 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
     }
 }
 
-static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
-{
-    MirrorBlockJob *s = op->s;
-    int64_t chunk_num;
-    int nb_chunks;
-
-    trace_mirror_iteration_done(s, op->offset, op->bytes, ret);
-
-    s->in_flight--;
-    s->bytes_in_flight -= op->bytes;
-
-    chunk_num = op->offset / s->granularity;
-    nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
-
-    bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);
-    QTAILQ_REMOVE(&s->ops_in_flight, op, next);
-    if (ret >= 0) {
-        if (s->cow_bitmap) {
-            bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
-        }
-        if (!s->initial_zeroing_ongoing) {
-            job_progress_update(&s->common.job, op->bytes);
-        }
-    }
-
-    qemu_co_queue_restart_all(&op->waiting_requests);
-    g_free(op);
-}
-
 /* Clip bytes relative to offset to not exceed end-of-file */
 static inline int64_t mirror_clip_bytes(MirrorBlockJob *s,
                                         int64_t offset,
@@ -373,6 +344,8 @@ static void coroutine_fn mirror_co_perform(void *opaque)
     MirrorOp *op = opaque;
     MirrorBlockJob *s = op->s;
     AioContext *aio_context;
+    int64_t chunk_num;
+    int nb_chunks;
     bool failed_on_read = false;
     int ret;
 
@@ -403,7 +376,27 @@ static void coroutine_fn mirror_co_perform(void *opaque)
         }
     }
 
-    mirror_iteration_done(op, ret);
+    trace_mirror_iteration_done(s, op->offset, op->bytes, ret);
+
+    s->in_flight--;
+    s->bytes_in_flight -= op->bytes;
+
+    chunk_num = op->offset / s->granularity;
+    nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
+
+    bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);
+    QTAILQ_REMOVE(&s->ops_in_flight, op, next);
+    if (ret >= 0) {
+        if (s->cow_bitmap) {
+            bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
+        }
+        if (!s->initial_zeroing_ongoing) {
+            job_progress_update(&s->common.job, op->bytes);
+        }
+    }
+
+    qemu_co_queue_restart_all(&op->waiting_requests);
+    g_free(op);
 
     aio_context_release(aio_context);
 }
-- 
2.17.1

[Qemu-devel] [PATCH 15/17] mirror: Release AioCtx before queue_restart_all()

[Max Reitz]

Calling qemu_co_queue_restart_all() with a held AioContext looks a bit
strange.  There is no reason why we would hold the context any longer
(as this coroutine is not going to perform any further operations that
would necessitate it), so release it before restarting the waiting
requests.

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

diff --git a/block/mirror.c b/block/mirror.c
index 053c37b6a6..cba7de610e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -395,10 +395,10 @@ static void coroutine_fn mirror_co_perform(void *opaque)
         }
     }
 
+    aio_context_release(aio_context);
+
     qemu_co_queue_restart_all(&op->waiting_requests);
     g_free(op);
-
-    aio_context_release(aio_context);
 }
 
 /* If mirror_method == MIRROR_METHOD_COPY, *offset and *bytes will be
-- 
2.17.1

[Qemu-devel] [PATCH 16/17] mirror: Support COR with write-blocking

[Max Reitz]

This patch adds a .bdrv_co_block_status() implementation for the mirror
block job that reports an area as allocated iff source and target are
in sync.  This allows putting a copy-on-read node on top of a mirror
node which automatically copies all data read from the source to the
target.

To make this perform better, bdrv_mirror_top_do_write() is modified to
ignore BDRV_REQ_WRITE_UNCHANGED requests regarding the source, and only
write them to the target (in write-blocking mode).  Otherwise, using COR
would result in all data read from the source that is not in sync with
the target to be re-written to the source (which is not the intention of
COR).

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

diff --git a/block/mirror.c b/block/mirror.c
index cba7de610e..625297fec1 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1304,29 +1304,40 @@ static int coroutine_fn bdrv_mirror_top_do_write(BlockDriverState *bs,
     MirrorBDSOpaque *s = bs->opaque;
     int ret = 0;
     bool copy_to_target;
+    bool write_to_source;
 
     copy_to_target = s->job->ret >= 0 &&
                      s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
 
+    /* WRITE_UNCHANGED requests are allocating writes, which in this
+     * case means that we should ensure the target is in sync with the
+     * source (by writing the data to the target).  Therefore, if we
+     * do write to the target (only in write-blocking mode), skip
+     * writing the (unchanged) data to the source. */
+    write_to_source = s->job->copy_mode != MIRROR_COPY_MODE_WRITE_BLOCKING ||
+                      !(flags & BDRV_REQ_WRITE_UNCHANGED);
+
     if (copy_to_target) {
         op = active_write_prepare(s->job, offset, bytes);
     }
 
-    switch (method) {
-    case MIRROR_METHOD_COPY:
-        ret = bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
-        break;
+    if (write_to_source) {
+        switch (method) {
+        case MIRROR_METHOD_COPY:
+            ret = bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
+            break;
 
-    case MIRROR_METHOD_ZERO:
-        ret = bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
-        break;
+        case MIRROR_METHOD_ZERO:
+            ret = bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
+            break;
 
-    case MIRROR_METHOD_DISCARD:
-        ret = bdrv_co_pdiscard(bs->backing, offset, bytes);
-        break;
+        case MIRROR_METHOD_DISCARD:
+            ret = bdrv_co_pdiscard(bs->backing, offset, bytes);
+            break;
 
-    default:
-        abort();
+        default:
+            abort();
+        }
     }
 
     if (ret < 0) {
@@ -1403,6 +1414,57 @@ static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
                                     NULL, 0);
 }
 
+/**
+ * Allocation status is determined by whether source and target are in
+ * sync:
+ * - If they are (dirty bitmap is clean), the data is considered to be
+ *   allocated in this layer.  Then, return BDRV_BLOCK_RAW so the
+ *   request is forwarded to the source.
+ * - Dirty (unsynced) areas are considered unallocated.  For those,
+ *   return 0.
+ */
+static int coroutine_fn bdrv_mirror_top_block_status(BlockDriverState *bs,
+                                                     bool want_zero,
+                                                     int64_t offset,
+                                                     int64_t bytes,
+                                                     int64_t *pnum,
+                                                     int64_t *map,
+                                                     BlockDriverState **file)
+{
+    MirrorBDSOpaque *s = bs->opaque;
+    BdrvDirtyBitmapIter *iter;
+    uint64_t dirty_offset, clean_offset;
+    int ret;
+
+    *map = offset;
+    *file = bs->backing->bs;
+
+    iter = bdrv_dirty_iter_new(s->job->dirty_bitmap);
+    bdrv_set_dirty_iter(iter, offset);
+
+    bdrv_dirty_bitmap_lock(s->job->dirty_bitmap);
+    dirty_offset = bdrv_dirty_iter_next(iter);
+    bdrv_dirty_iter_free(iter);
+    if (dirty_offset > offset) {
+        /* Clean area */
+        *pnum = MIN(dirty_offset - offset, bytes);
+        ret = BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
+        goto out;
+    }
+
+    /* Dirty area, find next clean area */
+    clean_offset = bdrv_dirty_bitmap_next_zero(s->job->dirty_bitmap, offset);
+    bdrv_dirty_bitmap_unlock(s->job->dirty_bitmap);
+
+    assert(clean_offset > offset);
+    *pnum = MIN(clean_offset - offset, bytes);
+    ret = 0;
+
+out:
+    bdrv_dirty_bitmap_unlock(s->job->dirty_bitmap);
+    return ret;
+}
+
 static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
 {
     if (bs->backing == NULL) {
@@ -1442,7 +1504,7 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
     .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
     .bdrv_co_flush              = bdrv_mirror_top_flush,
-    .bdrv_co_block_status       = bdrv_co_block_status_from_backing,
+    .bdrv_co_block_status       = bdrv_mirror_top_block_status,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_close                 = bdrv_mirror_top_close,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
-- 
2.17.1

[Qemu-devel] [PATCH 17/17] iotests: Add test for active mirror with COR

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/151     | 56 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/151.out |  4 +--
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
index fe53b9f446..e5515c2d37 100755
--- a/tests/qemu-iotests/151
+++ b/tests/qemu-iotests/151
@@ -114,6 +114,62 @@ class TestActiveMirror(iotests.QMPTestCase):
     def testActiveIOFlushed(self):
         self.doActiveIO(True)
 
+    def testCOR(self):
+        # Fill the source image
+        self.vm.hmp_qemu_io('source',
+                            'write -P 1 0 %i' % self.image_len);
+
+        # Start some background requests
+        for offset in range(0, self.image_len / 2, 1024 * 1024):
+            self.vm.hmp_qemu_io('source', 'aio_write -P 2 %i 1M' % offset)
+
+        # Start the block job
+        result = self.vm.qmp('blockdev-mirror',
+                             job_id='mirror',
+                             filter_node_name='mirror-node',
+                             device='source-node',
+                             target='target-node',
+                             sync='full',
+                             copy_mode='write-blocking')
+        self.assert_qmp(result, 'return', {})
+
+        # Stop background request processing
+        result = self.vm.qmp('job-pause', id='mirror')
+
+        result = self.vm.qmp('blockdev-add',
+                             node_name='cor-node',
+                             driver='copy-on-read',
+                             file='mirror-node')
+        self.assert_qmp(result, 'return', {})
+
+        # Now read everything, thus synchronously copying it to the
+        # target
+        for offset in range(0, self.image_len, 1024 * 1024):
+            self.vm.hmp_qemu_io('cor-node', 'read %i 1M' % offset)
+
+        # Resuming the job should make it READY immediately, because
+        # the read requests from cor-node above should have copied
+        # everything to target already.
+        # Set the job speed to 1 before resuming it, so that we can
+        # see that the READY event really comes immediately.
+
+        result = self.vm.qmp('block-job-set-speed',
+                             device='mirror',
+                             speed=1)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('job-resume', id='mirror')
+        self.assert_qmp(result, 'return', {})
+
+        # Wait for the READY event; if we get a timeout here, reading
+        # from cor-node has failed to copy something to target
+        self.vm.event_wait(name='BLOCK_JOB_READY', timeout=5.0)
+
+        self.vm.hmp_qemu_io('source', 'aio_flush')
+        self.potential_writes_in_flight = False
+
+        self.complete_and_wait(drive='mirror', wait_ready=False)
+
 
 
 if __name__ == '__main__':
diff --git a/tests/qemu-iotests/151.out b/tests/qemu-iotests/151.out
index fbc63e62f8..8d7e996700 100644
--- a/tests/qemu-iotests/151.out
+++ b/tests/qemu-iotests/151.out
@@ -1,5 +1,5 @@
-..
+...
 ----------------------------------------------------------------------
-Ran 2 tests
+Ran 3 tests
 
 OK
-- 
2.17.1