[PATCH v2 0/6] block-copy: use aio-task-pool

[PATCH v2 0/6] block-copy: use aio-task-pool

[Vladimir Sementsov-Ogievskiy]

Hi all!

This is the next step of improving block-copy: use aio task pool.

Async copying loop has better performance than linear, which is shown
in original series (was
"[RFC 00/24] backup performance: block_status + async", so this is
called v2)

Vladimir Sementsov-Ogievskiy (6):
  block/block-copy: rename in-flight requests to tasks
  block/block-copy: alloc task on each iteration
  block/block-copy: add state pointer to BlockCopyTask
  block/block-copy: move task size initial calculation to _task_create
  block/block-copy: move block_copy_task_create down
  block/block-copy: use aio-task-pool API

 block/block-copy.c | 250 ++++++++++++++++++++++++++++++---------------
 1 file changed, 168 insertions(+), 82 deletions(-)

-- 
2.21.0

[PATCH v2 1/6] block/block-copy: rename in-flight requests to tasks

[Vladimir Sementsov-Ogievskiy]

We are going to use aio-task-pool API and extend in-flight request
structure to be a successor of AioTask, so rename things appropriately.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 99 +++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 50 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 05227e18bf..61d1d26991 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -24,12 +24,12 @@
 #define BLOCK_COPY_MAX_BUFFER (1 * MiB)
 #define BLOCK_COPY_MAX_MEM (128 * MiB)
 
-typedef struct BlockCopyInFlightReq {
+typedef struct BlockCopyTask {
     int64_t offset;
     int64_t bytes;
-    QLIST_ENTRY(BlockCopyInFlightReq) list;
-    CoQueue wait_queue; /* coroutines blocked on this request */
-} BlockCopyInFlightReq;
+    QLIST_ENTRY(BlockCopyTask) list;
+    CoQueue wait_queue; /* coroutines blocked on this task */
+} BlockCopyTask;
 
 typedef struct BlockCopyState {
     /*
@@ -45,7 +45,7 @@ typedef struct BlockCopyState {
     bool use_copy_range;
     int64_t copy_size;
     uint64_t len;
-    QLIST_HEAD(, BlockCopyInFlightReq) inflight_reqs;
+    QLIST_HEAD(, BlockCopyTask) tasks;
 
     BdrvRequestFlags write_flags;
 
@@ -73,15 +73,14 @@ typedef struct BlockCopyState {
     SharedResource *mem;
 } BlockCopyState;
 
-static BlockCopyInFlightReq *find_conflicting_inflight_req(BlockCopyState *s,
-                                                           int64_t offset,
-                                                           int64_t bytes)
+static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
+                                            int64_t offset, int64_t bytes)
 {
-    BlockCopyInFlightReq *req;
+    BlockCopyTask *t;
 
-    QLIST_FOREACH(req, &s->inflight_reqs, list) {
-        if (offset + bytes > req->offset && offset < req->offset + req->bytes) {
-            return req;
+    QLIST_FOREACH(t, &s->tasks, list) {
+        if (offset + bytes > t->offset && offset < t->offset + t->bytes) {
+            return t;
         }
     }
 
@@ -89,73 +88,73 @@ static BlockCopyInFlightReq *find_conflicting_inflight_req(BlockCopyState *s,
 }
 
 /*
- * If there are no intersecting requests return false. Otherwise, wait for the
- * first found intersecting request to finish and return true.
+ * If there are no intersecting tasks return false. Otherwise, wait for the
+ * first found intersecting tasks to finish and return true.
  */
 static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
                                              int64_t bytes)
 {
-    BlockCopyInFlightReq *req = find_conflicting_inflight_req(s, offset, bytes);
+    BlockCopyTask *task = find_conflicting_task(s, offset, bytes);
 
-    if (!req) {
+    if (!task) {
         return false;
     }
 
-    qemu_co_queue_wait(&req->wait_queue, NULL);
+    qemu_co_queue_wait(&task->wait_queue, NULL);
 
     return true;
 }
 
 /* Called only on full-dirty region */
-static void block_copy_inflight_req_begin(BlockCopyState *s,
-                                          BlockCopyInFlightReq *req,
-                                          int64_t offset, int64_t bytes)
+static void block_copy_task_begin(BlockCopyState *s, BlockCopyTask *task,
+                                  int64_t offset, int64_t bytes)
 {
-    assert(!find_conflicting_inflight_req(s, offset, bytes));
+    assert(!find_conflicting_task(s, offset, bytes));
 
     bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
     s->in_flight_bytes += bytes;
 
-    req->offset = offset;
-    req->bytes = bytes;
-    qemu_co_queue_init(&req->wait_queue);
-    QLIST_INSERT_HEAD(&s->inflight_reqs, req, list);
+    task->offset = offset;
+    task->bytes = bytes;
+    qemu_co_queue_init(&task->wait_queue);
+    QLIST_INSERT_HEAD(&s->tasks, task, list);
 }
 
 /*
- * block_copy_inflight_req_shrink
+ * block_copy_task_shrink
  *
- * Drop the tail of the request to be handled later. Set dirty bits back and
- * wake up all requests waiting for us (may be some of them are not intersecting
- * with shrunk request)
+ * Drop the tail of the task to be handled later. Set dirty bits back and
+ * wake up all tasks waiting for us (may be some of them are not intersecting
+ * with shrunk task)
  */
-static void coroutine_fn block_copy_inflight_req_shrink(BlockCopyState *s,
-        BlockCopyInFlightReq *req, int64_t new_bytes)
+static void coroutine_fn block_copy_task_shrink(BlockCopyState *s,
+                                                BlockCopyTask *task,
+                                                int64_t new_bytes)
 {
-    if (new_bytes == req->bytes) {
+    if (new_bytes == task->bytes) {
         return;
     }
 
-    assert(new_bytes > 0 && new_bytes < req->bytes);
+    assert(new_bytes > 0 && new_bytes < task->bytes);
 
-    s->in_flight_bytes -= req->bytes - new_bytes;
+    s->in_flight_bytes -= task->bytes - new_bytes;
     bdrv_set_dirty_bitmap(s->copy_bitmap,
-                          req->offset + new_bytes, req->bytes - new_bytes);
+                          task->offset + new_bytes, task->bytes - new_bytes);
+    s->in_flight_bytes -= task->bytes - new_bytes;
 
-    req->bytes = new_bytes;
-    qemu_co_queue_restart_all(&req->wait_queue);
+    task->bytes = new_bytes;
+    qemu_co_queue_restart_all(&task->wait_queue);
 }
 
-static void coroutine_fn block_copy_inflight_req_end(BlockCopyState *s,
-                                                     BlockCopyInFlightReq *req,
-                                                     int ret)
+static void coroutine_fn block_copy_task_end(BlockCopyState *s,
+                                             BlockCopyTask *task, int ret)
 {
-    s->in_flight_bytes -= req->bytes;
+    s->in_flight_bytes -= task->bytes;
     if (ret < 0) {
-        bdrv_set_dirty_bitmap(s->copy_bitmap, req->offset, req->bytes);
+        bdrv_set_dirty_bitmap(s->copy_bitmap, task->offset, task->bytes);
     }
-    QLIST_REMOVE(req, list);
-    qemu_co_queue_restart_all(&req->wait_queue);
+    QLIST_REMOVE(task, list);
+    qemu_co_queue_restart_all(&task->wait_queue);
 }
 
 void block_copy_state_free(BlockCopyState *s)
@@ -223,7 +222,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
         s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
     }
 
-    QLIST_INIT(&s->inflight_reqs);
+    QLIST_INIT(&s->tasks);
 
     return s;
 }
@@ -474,7 +473,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
     assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
 
     while (bytes) {
-        BlockCopyInFlightReq req;
+        BlockCopyTask task;
         int64_t next_zero, cur_bytes, status_bytes;
 
         if (!bdrv_dirty_bitmap_get(s->copy_bitmap, offset)) {
@@ -495,14 +494,14 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
             assert(next_zero < offset + cur_bytes); /* no need to do MIN() */
             cur_bytes = next_zero - offset;
         }
-        block_copy_inflight_req_begin(s, &req, offset, cur_bytes);
+        block_copy_task_begin(s, &task, offset, cur_bytes);
 
         ret = block_copy_block_status(s, offset, cur_bytes, &status_bytes);
         assert(ret >= 0); /* never fail */
         cur_bytes = MIN(cur_bytes, status_bytes);
-        block_copy_inflight_req_shrink(s, &req, cur_bytes);
+        block_copy_task_shrink(s, &task, cur_bytes);
         if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
-            block_copy_inflight_req_end(s, &req, 0);
+            block_copy_task_end(s, &task, 0);
             progress_set_remaining(s->progress,
                                    bdrv_get_dirty_count(s->copy_bitmap) +
                                    s->in_flight_bytes);
@@ -518,7 +517,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
         ret = block_copy_do_copy(s, offset, cur_bytes, ret & BDRV_BLOCK_ZERO,
                                  error_is_read);
         co_put_to_shres(s->mem, cur_bytes);
-        block_copy_inflight_req_end(s, &req, ret);
+        block_copy_task_end(s, &task, ret);
         if (ret < 0) {
             return ret;
         }
-- 
2.21.0

Re: [PATCH v2 1/6] block/block-copy: rename in-flight requests to tasks

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1607 bytes --]

On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
> We are going to use aio-task-pool API and extend in-flight request
> structure to be a successor of AioTask, so rename things appropriately.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 99 +++++++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 50 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 05227e18bf..61d1d26991 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c

[...]

> -static void coroutine_fn block_copy_inflight_req_shrink(BlockCopyState *s,
> -        BlockCopyInFlightReq *req, int64_t new_bytes)
> +static void coroutine_fn block_copy_task_shrink(BlockCopyState *s,
> +                                                BlockCopyTask *task,
> +                                                int64_t new_bytes)
>  {
> -    if (new_bytes == req->bytes) {
> +    if (new_bytes == task->bytes) {
>          return;
>      }
>  
> -    assert(new_bytes > 0 && new_bytes < req->bytes);
> +    assert(new_bytes > 0 && new_bytes < task->bytes);
>  
> -    s->in_flight_bytes -= req->bytes - new_bytes;
> +    s->in_flight_bytes -= task->bytes - new_bytes;
>      bdrv_set_dirty_bitmap(s->copy_bitmap,
> -                          req->offset + new_bytes, req->bytes - new_bytes);
> +                          task->offset + new_bytes, task->bytes - new_bytes);
> +    s->in_flight_bytes -= task->bytes - new_bytes;

This line doesn’t seem right.

(The rest does.)

Max


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

Re: [PATCH v2 1/6] block/block-copy: rename in-flight requests to tasks

[Vladimir Sementsov-Ogievskiy]

28.04.2020 10:30, Max Reitz wrote:
> On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
>> We are going to use aio-task-pool API and extend in-flight request
>> structure to be a successor of AioTask, so rename things appropriately.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/block-copy.c | 99 +++++++++++++++++++++++-----------------------
>>   1 file changed, 49 insertions(+), 50 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index 05227e18bf..61d1d26991 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
> 
> [...]
> 
>> -static void coroutine_fn block_copy_inflight_req_shrink(BlockCopyState *s,
>> -        BlockCopyInFlightReq *req, int64_t new_bytes)
>> +static void coroutine_fn block_copy_task_shrink(BlockCopyState *s,
>> +                                                BlockCopyTask *task,
>> +                                                int64_t new_bytes)
>>   {
>> -    if (new_bytes == req->bytes) {
>> +    if (new_bytes == task->bytes) {
>>           return;
>>       }
>>   
>> -    assert(new_bytes > 0 && new_bytes < req->bytes);
>> +    assert(new_bytes > 0 && new_bytes < task->bytes);
>>   
>> -    s->in_flight_bytes -= req->bytes - new_bytes;
>> +    s->in_flight_bytes -= task->bytes - new_bytes;
>>       bdrv_set_dirty_bitmap(s->copy_bitmap,
>> -                          req->offset + new_bytes, req->bytes - new_bytes);
>> +                          task->offset + new_bytes, task->bytes - new_bytes);
>> +    s->in_flight_bytes -= task->bytes - new_bytes;
> 
> This line doesn’t seem right.
> 

Hmm yes.. A kind of copy-paste or rebase artifact.


-- 
Best regards,
Vladimir

[PATCH v2 2/6] block/block-copy: alloc task on each iteration

[Vladimir Sementsov-Ogievskiy]

We are going to use aio-task-pool API, so tasks will be handled in
parallel. We need therefore separate allocated task on each iteration.
Introduce this logic now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 61d1d26991..0daaaa9ba0 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -106,9 +106,11 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
 }
 
 /* Called only on full-dirty region */
-static void block_copy_task_begin(BlockCopyState *s, BlockCopyTask *task,
-                                  int64_t offset, int64_t bytes)
+static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
+                                             int64_t offset, int64_t bytes)
 {
+    BlockCopyTask *task = g_new(BlockCopyTask, 1);
+
     assert(!find_conflicting_task(s, offset, bytes));
 
     bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
@@ -118,6 +120,8 @@ static void block_copy_task_begin(BlockCopyState *s, BlockCopyTask *task,
     task->bytes = bytes;
     qemu_co_queue_init(&task->wait_queue);
     QLIST_INSERT_HEAD(&s->tasks, task, list);
+
+    return task;
 }
 
 /*
@@ -473,7 +477,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
     assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
 
     while (bytes) {
-        BlockCopyTask task;
+        g_autofree BlockCopyTask *task = NULL;
         int64_t next_zero, cur_bytes, status_bytes;
 
         if (!bdrv_dirty_bitmap_get(s->copy_bitmap, offset)) {
@@ -494,14 +498,14 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
             assert(next_zero < offset + cur_bytes); /* no need to do MIN() */
             cur_bytes = next_zero - offset;
         }
-        block_copy_task_begin(s, &task, offset, cur_bytes);
+        task = block_copy_task_create(s, offset, cur_bytes);
 
         ret = block_copy_block_status(s, offset, cur_bytes, &status_bytes);
         assert(ret >= 0); /* never fail */
         cur_bytes = MIN(cur_bytes, status_bytes);
-        block_copy_task_shrink(s, &task, cur_bytes);
+        block_copy_task_shrink(s, task, cur_bytes);
         if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
-            block_copy_task_end(s, &task, 0);
+            block_copy_task_end(s, task, 0);
             progress_set_remaining(s->progress,
                                    bdrv_get_dirty_count(s->copy_bitmap) +
                                    s->in_flight_bytes);
@@ -517,7 +521,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
         ret = block_copy_do_copy(s, offset, cur_bytes, ret & BDRV_BLOCK_ZERO,
                                  error_is_read);
         co_put_to_shres(s->mem, cur_bytes);
-        block_copy_task_end(s, &task, ret);
+        block_copy_task_end(s, task, ret);
         if (ret < 0) {
             return ret;
         }
-- 
2.21.0

Re: [PATCH v2 2/6] block/block-copy: alloc task on each iteration

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 462 bytes --]

On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
> We are going to use aio-task-pool API, so tasks will be handled in
> parallel. We need therefore separate allocated task on each iteration.
> Introduce this logic now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)

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


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

[PATCH v2 3/6] block/block-copy: add state pointer to BlockCopyTask

[Vladimir Sementsov-Ogievskiy]

We are going to use aio-task-pool API, so we'll need state pointer in
BlockCopyTask anyway. Add it now and use where possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 0daaaa9ba0..63d8468b27 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -25,6 +25,7 @@
 #define BLOCK_COPY_MAX_MEM (128 * MiB)
 
 typedef struct BlockCopyTask {
+    BlockCopyState *s;
     int64_t offset;
     int64_t bytes;
     QLIST_ENTRY(BlockCopyTask) list;
@@ -116,8 +117,11 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
     bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
     s->in_flight_bytes += bytes;
 
-    task->offset = offset;
-    task->bytes = bytes;
+    *task = (BlockCopyTask) {
+        .s = s,
+        .offset = offset,
+        .bytes = bytes,
+    };
     qemu_co_queue_init(&task->wait_queue);
     QLIST_INSERT_HEAD(&s->tasks, task, list);
 
@@ -131,8 +135,7 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
  * wake up all tasks waiting for us (may be some of them are not intersecting
  * with shrunk task)
  */
-static void coroutine_fn block_copy_task_shrink(BlockCopyState *s,
-                                                BlockCopyTask *task,
+static void coroutine_fn block_copy_task_shrink(BlockCopyTask *task,
                                                 int64_t new_bytes)
 {
     if (new_bytes == task->bytes) {
@@ -141,21 +144,20 @@ static void coroutine_fn block_copy_task_shrink(BlockCopyState *s,
 
     assert(new_bytes > 0 && new_bytes < task->bytes);
 
-    s->in_flight_bytes -= task->bytes - new_bytes;
-    bdrv_set_dirty_bitmap(s->copy_bitmap,
+    task->s->in_flight_bytes -= task->bytes - new_bytes;
+    bdrv_set_dirty_bitmap(task->s->copy_bitmap,
                           task->offset + new_bytes, task->bytes - new_bytes);
-    s->in_flight_bytes -= task->bytes - new_bytes;
+    task->s->in_flight_bytes -= task->bytes - new_bytes;
 
     task->bytes = new_bytes;
     qemu_co_queue_restart_all(&task->wait_queue);
 }
 
-static void coroutine_fn block_copy_task_end(BlockCopyState *s,
-                                             BlockCopyTask *task, int ret)
+static void coroutine_fn block_copy_task_end(BlockCopyTask *task, int ret)
 {
-    s->in_flight_bytes -= task->bytes;
+    task->s->in_flight_bytes -= task->bytes;
     if (ret < 0) {
-        bdrv_set_dirty_bitmap(s->copy_bitmap, task->offset, task->bytes);
+        bdrv_set_dirty_bitmap(task->s->copy_bitmap, task->offset, task->bytes);
     }
     QLIST_REMOVE(task, list);
     qemu_co_queue_restart_all(&task->wait_queue);
@@ -503,9 +505,9 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
         ret = block_copy_block_status(s, offset, cur_bytes, &status_bytes);
         assert(ret >= 0); /* never fail */
         cur_bytes = MIN(cur_bytes, status_bytes);
-        block_copy_task_shrink(s, task, cur_bytes);
+        block_copy_task_shrink(task, cur_bytes);
         if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
-            block_copy_task_end(s, task, 0);
+            block_copy_task_end(task, 0);
             progress_set_remaining(s->progress,
                                    bdrv_get_dirty_count(s->copy_bitmap) +
                                    s->in_flight_bytes);
@@ -521,7 +523,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
         ret = block_copy_do_copy(s, offset, cur_bytes, ret & BDRV_BLOCK_ZERO,
                                  error_is_read);
         co_put_to_shres(s->mem, cur_bytes);
-        block_copy_task_end(s, task, ret);
+        block_copy_task_end(task, ret);
         if (ret < 0) {
             return ret;
         }
-- 
2.21.0

Re: [PATCH v2 3/6] block/block-copy: add state pointer to BlockCopyTask

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 435 bytes --]

On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
> We are going to use aio-task-pool API, so we'll need state pointer in
> BlockCopyTask anyway. Add it now and use where possible.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)

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


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

[PATCH v2 4/6] block/block-copy: move task size initial calculation to _task_create

[Vladimir Sementsov-Ogievskiy]

Comment "Called only on full-dirty region" without corresponding
assertion is a very unsafe thing. Adding assertion means call
bdrv_dirty_bitmap_next_zero twice. Instead, let's move
bdrv_dirty_bitmap_next_zero call to block_copy_task_create. It also
allows to drop cur_bytes variable which partly duplicate task->bytes.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 47 ++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 63d8468b27..dd406eb4bb 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -106,12 +106,23 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
     return true;
 }
 
-/* Called only on full-dirty region */
 static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
                                              int64_t offset, int64_t bytes)
 {
+    int64_t next_zero;
     BlockCopyTask *task = g_new(BlockCopyTask, 1);
 
+    assert(bdrv_dirty_bitmap_get(s->copy_bitmap, offset));
+
+    bytes = MIN(bytes, s->copy_size);
+    next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, offset, bytes);
+    if (next_zero >= 0) {
+        assert(next_zero > offset); /* offset is dirty */
+        assert(next_zero < offset + bytes); /* no need to do MIN() */
+        bytes = next_zero - offset;
+    }
+
+    /* region is dirty, so no existent tasks possible in it */
     assert(!find_conflicting_task(s, offset, bytes));
 
     bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
@@ -480,7 +491,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
 
     while (bytes) {
         g_autofree BlockCopyTask *task = NULL;
-        int64_t next_zero, cur_bytes, status_bytes;
+        int64_t status_bytes;
 
         if (!bdrv_dirty_bitmap_get(s->copy_bitmap, offset)) {
             trace_block_copy_skip(s, offset);
@@ -491,21 +502,13 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
 
         found_dirty = true;
 
-        cur_bytes = MIN(bytes, s->copy_size);
+        task = block_copy_task_create(s, offset, bytes);
 
-        next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, offset,
-                                                cur_bytes);
-        if (next_zero >= 0) {
-            assert(next_zero > offset); /* offset is dirty */
-            assert(next_zero < offset + cur_bytes); /* no need to do MIN() */
-            cur_bytes = next_zero - offset;
-        }
-        task = block_copy_task_create(s, offset, cur_bytes);
-
-        ret = block_copy_block_status(s, offset, cur_bytes, &status_bytes);
+        ret = block_copy_block_status(s, offset, task->bytes, &status_bytes);
         assert(ret >= 0); /* never fail */
-        cur_bytes = MIN(cur_bytes, status_bytes);
-        block_copy_task_shrink(task, cur_bytes);
+        if (status_bytes < task->bytes) {
+            block_copy_task_shrink(task, status_bytes);
+        }
         if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
             block_copy_task_end(task, 0);
             progress_set_remaining(s->progress,
@@ -519,19 +522,19 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
 
         trace_block_copy_process(s, offset);
 
-        co_get_from_shres(s->mem, cur_bytes);
-        ret = block_copy_do_copy(s, offset, cur_bytes, ret & BDRV_BLOCK_ZERO,
+        co_get_from_shres(s->mem, task->bytes);
+        ret = block_copy_do_copy(s, offset, task->bytes, ret & BDRV_BLOCK_ZERO,
                                  error_is_read);
-        co_put_to_shres(s->mem, cur_bytes);
+        co_put_to_shres(s->mem, task->bytes);
         block_copy_task_end(task, ret);
         if (ret < 0) {
             return ret;
         }
 
-        progress_work_done(s->progress, cur_bytes);
-        s->progress_bytes_callback(cur_bytes, s->progress_opaque);
-        offset += cur_bytes;
-        bytes -= cur_bytes;
+        progress_work_done(s->progress, task->bytes);
+        s->progress_bytes_callback(task->bytes, s->progress_opaque);
+        offset += task->bytes;
+        bytes -= task->bytes;
     }
 
     return found_dirty;
-- 
2.21.0

Re: [PATCH v2 4/6] block/block-copy: move task size initial calculation to _task_create

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2834 bytes --]

On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
> Comment "Called only on full-dirty region" without corresponding
> assertion is a very unsafe thing.

Not sure whether it’s that unsafe for a static function with a single
caller, but, well.

> Adding assertion means call
> bdrv_dirty_bitmap_next_zero twice. Instead, let's move
> bdrv_dirty_bitmap_next_zero call to block_copy_task_create. It also
> allows to drop cur_bytes variable which partly duplicate task->bytes.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 47 ++++++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 63d8468b27..dd406eb4bb 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -106,12 +106,23 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
>      return true;
>  }
>  
> -/* Called only on full-dirty region */
>  static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>                                               int64_t offset, int64_t bytes)

A bit of documentation on the new interface might be nice.  For one
thing, that @offset must be dirty, although there is an assertion that,
well, checks it.  (An assertion doesn’t really check anything, it rather
verifies a contract.  And violation is fatal.)

For another, what the range [offset, offset + bytes) is; namely
basically the whole range of data that we might potentially copy, only
that the head must be dirty, but the tail may be clean.

Which makes me think that the interface is maybe less than intuitive.
It would make more sense if we could just call this function on the
whole region and it would figure out whether @offset is dirty by itself
(and return NULL if it isn’t).

OTOH I suppose the interface how it is here is more useful for
task-ification.  But maybe that should be documented.

>  {
> +    int64_t next_zero;
>      BlockCopyTask *task = g_new(BlockCopyTask, 1);
>  
> +    assert(bdrv_dirty_bitmap_get(s->copy_bitmap, offset));
> +
> +    bytes = MIN(bytes, s->copy_size);
> +    next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, offset, bytes);
> +    if (next_zero >= 0) {
> +        assert(next_zero > offset); /* offset is dirty */
> +        assert(next_zero < offset + bytes); /* no need to do MIN() */
> +        bytes = next_zero - offset;
> +    }
> +
> +    /* region is dirty, so no existent tasks possible in it */

s/existent/existing/?

(The code movement and how you replaced cur_bytes by task->bytes looks
good.)

Max

>      assert(!find_conflicting_task(s, offset, bytes));
>  
>      bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);


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

Re: [PATCH v2 4/6] block/block-copy: move task size initial calculation to _task_create

[Vladimir Sementsov-Ogievskiy]

28.04.2020 11:52, Max Reitz wrote:
> On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
>> Comment "Called only on full-dirty region" without corresponding
>> assertion is a very unsafe thing.
> 
> Not sure whether it’s that unsafe for a static function with a single
> caller, but, well.
> 
>> Adding assertion means call
>> bdrv_dirty_bitmap_next_zero twice. Instead, let's move
>> bdrv_dirty_bitmap_next_zero call to block_copy_task_create. It also
>> allows to drop cur_bytes variable which partly duplicate task->bytes.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/block-copy.c | 47 ++++++++++++++++++++++++----------------------
>>   1 file changed, 25 insertions(+), 22 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index 63d8468b27..dd406eb4bb 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
>> @@ -106,12 +106,23 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
>>       return true;
>>   }
>>   
>> -/* Called only on full-dirty region */
>>   static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>>                                                int64_t offset, int64_t bytes)
> 
> A bit of documentation on the new interface might be nice.  For one
> thing, that @offset must be dirty, although there is an assertion that,
> well, checks it.  (An assertion doesn’t really check anything, it rather
> verifies a contract.  And violation is fatal.)

Still, good to document that.

> 
> For another, what the range [offset, offset + bytes) is; namely
> basically the whole range of data that we might potentially copy, only
> that the head must be dirty, but the tail may be clean.

Right

> 
> Which makes me think that the interface is maybe less than intuitive.
> It would make more sense if we could just call this function on the
> whole region and it would figure out whether @offset is dirty by itself
> (and return NULL if it isn’t).

Hmm. Actually, I didn't touch the very inefficient "if (!bdrv_dirty_bitmap_get(s->copy_bitmap, offset)) { continue }" construct, because I was waiting for my series about refactoring bdrv_dirty_bitmap_next_dirty_area to merge in. But now it is merged, and I should refactor this thing. And may be you are right, that it may be done inside block_copy_task_create.

> 
> OTOH I suppose the interface how it is here is more useful for
> task-ification.  But maybe that should be documented.

On the first glance, it should not really matter.

OK, I'll try to improve it somehow for v3

-- 
Best regards,
Vladimir

[PATCH v2 5/6] block/block-copy: move block_copy_task_create down

[Vladimir Sementsov-Ogievskiy]

Simple movement without any change. It's needed for the following
patch, as this function will need to use some staff which is currently
below it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 66 +++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index dd406eb4bb..910947cb43 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -106,39 +106,6 @@ static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
     return true;
 }
 
-static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
-                                             int64_t offset, int64_t bytes)
-{
-    int64_t next_zero;
-    BlockCopyTask *task = g_new(BlockCopyTask, 1);
-
-    assert(bdrv_dirty_bitmap_get(s->copy_bitmap, offset));
-
-    bytes = MIN(bytes, s->copy_size);
-    next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, offset, bytes);
-    if (next_zero >= 0) {
-        assert(next_zero > offset); /* offset is dirty */
-        assert(next_zero < offset + bytes); /* no need to do MIN() */
-        bytes = next_zero - offset;
-    }
-
-    /* region is dirty, so no existent tasks possible in it */
-    assert(!find_conflicting_task(s, offset, bytes));
-
-    bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
-    s->in_flight_bytes += bytes;
-
-    *task = (BlockCopyTask) {
-        .s = s,
-        .offset = offset,
-        .bytes = bytes,
-    };
-    qemu_co_queue_init(&task->wait_queue);
-    QLIST_INSERT_HEAD(&s->tasks, task, list);
-
-    return task;
-}
-
 /*
  * block_copy_task_shrink
  *
@@ -361,6 +328,39 @@ out:
     return ret;
 }
 
+static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
+                                             int64_t offset, int64_t bytes)
+{
+    int64_t next_zero;
+    BlockCopyTask *task = g_new(BlockCopyTask, 1);
+
+    assert(bdrv_dirty_bitmap_get(s->copy_bitmap, offset));
+
+    bytes = MIN(bytes, s->copy_size);
+    next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, offset, bytes);
+    if (next_zero >= 0) {
+        assert(next_zero > offset); /* offset is dirty */
+        assert(next_zero < offset + bytes); /* no need to do MIN() */
+        bytes = next_zero - offset;
+    }
+
+    /* region is dirty, so no existent tasks possible in it */
+    assert(!find_conflicting_task(s, offset, bytes));
+
+    bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
+    s->in_flight_bytes += bytes;
+
+    *task = (BlockCopyTask) {
+        .s = s,
+        .offset = offset,
+        .bytes = bytes,
+    };
+    qemu_co_queue_init(&task->wait_queue);
+    QLIST_INSERT_HEAD(&s->tasks, task, list);
+
+    return task;
+}
+
 static int block_copy_block_status(BlockCopyState *s, int64_t offset,
                                    int64_t bytes, int64_t *pnum)
 {
-- 
2.21.0

Re: [PATCH v2 5/6] block/block-copy: move block_copy_task_create down

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 512 bytes --]

On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
> Simple movement without any change. It's needed for the following
> patch, as this function will need to use some staff which is currently

*stuff

> below it.

Wouldn’t it be simpler to just declare block_copy_task_entry()?

Max

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 66 +++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)


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

Re: [PATCH v2 5/6] block/block-copy: move block_copy_task_create down

[Vladimir Sementsov-Ogievskiy]

28.04.2020 12:06, Max Reitz wrote:
> On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
>> Simple movement without any change. It's needed for the following
>> patch, as this function will need to use some staff which is currently
> 
> *stuff
> 
>> below it.
> 
> Wouldn’t it be simpler to just declare block_copy_task_entry()?
> 

I just think, that it's good to keep native order of functions and avoid extra declarations. Still, may be I care too much. No actual difference, if you prefer declaration, I can drop this patch.

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/block-copy.c | 66 +++++++++++++++++++++++-----------------------
>>   1 file changed, 33 insertions(+), 33 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 5/6] block/block-copy: move block_copy_task_create down

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 988 bytes --]

On 28.04.20 11:17, Vladimir Sementsov-Ogievskiy wrote:
> 28.04.2020 12:06, Max Reitz wrote:
>> On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
>>> Simple movement without any change. It's needed for the following
>>> patch, as this function will need to use some staff which is currently
>>
>> *stuff
>>
>>> below it.
>>
>> Wouldn’t it be simpler to just declare block_copy_task_entry()?
>>
> 
> I just think, that it's good to keep native order of functions and avoid
> extra declarations. Still, may be I care too much. No actual difference,
> if you prefer declaration, I can drop this patch.

Personally, the native order doesn’t do me any good (cscope doesn’t
really care where the definition is), and also having functions in order
seems just like a C artifact.

I just prefer declarations because otherwise we end up moving functions
all the time with no real benefit.  Furthermore, moving functions has
the drawback of polluting git blame.

Max


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

[PATCH v2 6/6] block/block-copy: use aio-task-pool API

[Vladimir Sementsov-Ogievskiy]

Run block_copy iterations in parallel in aio tasks.

Changes:
  - BlockCopyTask becomes aio task structure. Add zeroes field to pass
    it to block_copy_do_copy
  - add call state - it's a state of one call of block_copy(), shared
    between parallel tasks. For now used only to keep information about
    first error: is it read or not.
  - convert block_copy_dirty_clusters to aio-task loop.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 104 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 91 insertions(+), 13 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 910947cb43..9994598eb7 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -19,15 +19,27 @@
 #include "block/block-copy.h"
 #include "sysemu/block-backend.h"
 #include "qemu/units.h"
+#include "qemu/coroutine.h"
+#include "block/aio_task.h"
 
 #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
 #define BLOCK_COPY_MAX_BUFFER (1 * MiB)
 #define BLOCK_COPY_MAX_MEM (128 * MiB)
+#define BLOCK_COPY_MAX_WORKERS 64
+
+typedef struct BlockCopyCallState {
+    bool failed;
+    bool error_is_read;
+} BlockCopyCallState;
 
 typedef struct BlockCopyTask {
+    AioTask task;
+
     BlockCopyState *s;
+    BlockCopyCallState *call_state;
     int64_t offset;
     int64_t bytes;
+    bool zeroes;
     QLIST_ENTRY(BlockCopyTask) list;
     CoQueue wait_queue; /* coroutines blocked on this task */
 } BlockCopyTask;
@@ -225,6 +237,30 @@ void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm)
     s->progress = pm;
 }
 
+/* Takes ownership on @task */
+static coroutine_fn int block_copy_task_run(AioTaskPool *pool,
+                                            BlockCopyTask *task)
+{
+    if (!pool) {
+        int ret = task->task.func(&task->task);
+
+        g_free(task);
+        return ret;
+    }
+
+    aio_task_pool_wait_slot(pool);
+    if (aio_task_pool_status(pool) < 0) {
+        co_put_to_shres(task->s->mem, task->bytes);
+        block_copy_task_end(task, -EAGAIN);
+        g_free(task);
+        return aio_task_pool_status(pool);
+    }
+
+    aio_task_pool_start_task(pool, &task->task);
+
+    return 0;
+}
+
 /*
  * block_copy_do_copy
  *
@@ -328,8 +364,32 @@ out:
     return ret;
 }
 
+static coroutine_fn int block_copy_task_entry(AioTask *task)
+{
+    BlockCopyTask *t = container_of(task, BlockCopyTask, task);
+    bool error_is_read;
+    int ret;
+
+    ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
+                             &error_is_read);
+    if (ret < 0 && !t->call_state->failed) {
+        t->call_state->failed = true;
+        t->call_state->error_is_read = error_is_read;
+    } else {
+        progress_work_done(t->s->progress, t->bytes);
+        t->s->progress_bytes_callback(t->bytes, t->s->progress_opaque);
+    }
+    co_put_to_shres(t->s->mem, t->bytes);
+    block_copy_task_end(t, ret);
+
+    return ret;
+}
+
+/* Called only on full-dirty region */
 static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
-                                             int64_t offset, int64_t bytes)
+                                             BlockCopyCallState *call_state,
+                                             int64_t offset,
+                                             int64_t bytes)
 {
     int64_t next_zero;
     BlockCopyTask *task = g_new(BlockCopyTask, 1);
@@ -351,7 +411,9 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
     s->in_flight_bytes += bytes;
 
     *task = (BlockCopyTask) {
+        .task.func = block_copy_task_entry,
         .s = s,
+        .call_state = call_state,
         .offset = offset,
         .bytes = bytes,
     };
@@ -478,6 +540,8 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
 {
     int ret = 0;
     bool found_dirty = false;
+    AioTaskPool *aio = NULL;
+    BlockCopyCallState call_state = {false, false};
 
     /*
      * block_copy() user is responsible for keeping source and target in same
@@ -489,8 +553,8 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
     assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
     assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
 
-    while (bytes) {
-        g_autofree BlockCopyTask *task = NULL;
+    while (bytes && aio_task_pool_status(aio) == 0) {
+        BlockCopyTask *task;
         int64_t status_bytes;
 
         if (!bdrv_dirty_bitmap_get(s->copy_bitmap, offset)) {
@@ -502,7 +566,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
 
         found_dirty = true;
 
-        task = block_copy_task_create(s, offset, bytes);
+        task = block_copy_task_create(s, &call_state, offset, bytes);
 
         ret = block_copy_block_status(s, offset, task->bytes, &status_bytes);
         assert(ret >= 0); /* never fail */
@@ -511,6 +575,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
         }
         if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
             block_copy_task_end(task, 0);
+            g_free(task);
             progress_set_remaining(s->progress,
                                    bdrv_get_dirty_count(s->copy_bitmap) +
                                    s->in_flight_bytes);
@@ -519,25 +584,38 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,
             bytes -= status_bytes;
             continue;
         }
+        task->zeroes = ret & BDRV_BLOCK_ZERO;
 
         trace_block_copy_process(s, offset);
 
         co_get_from_shres(s->mem, task->bytes);
-        ret = block_copy_do_copy(s, offset, task->bytes, ret & BDRV_BLOCK_ZERO,
-                                 error_is_read);
-        co_put_to_shres(s->mem, task->bytes);
-        block_copy_task_end(task, ret);
-        if (ret < 0) {
-            return ret;
+
+        if (!aio && task->bytes != bytes) {
+            aio = aio_task_pool_new(BLOCK_COPY_MAX_WORKERS);
         }
 
-        progress_work_done(s->progress, task->bytes);
-        s->progress_bytes_callback(task->bytes, s->progress_opaque);
         offset += task->bytes;
         bytes -= task->bytes;
+
+        ret = block_copy_task_run(aio, task);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+
+out:
+    if (aio) {
+        aio_task_pool_wait_all(aio);
+        if (ret == 0) {
+            ret = aio_task_pool_status(aio);
+        }
+        g_free(aio);
+    }
+    if (error_is_read && ret < 0) {
+        *error_is_read = call_state.error_is_read;
     }
 
-    return found_dirty;
+    return ret < 0 ? ret : found_dirty;
 }
 
 /*
-- 
2.21.0

Re: [PATCH v2 6/6] block/block-copy: use aio-task-pool API

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2348 bytes --]

On 25.03.20 14:46, Vladimir Sementsov-Ogievskiy wrote:
> Run block_copy iterations in parallel in aio tasks.
> 
> Changes:
>   - BlockCopyTask becomes aio task structure. Add zeroes field to pass
>     it to block_copy_do_copy
>   - add call state - it's a state of one call of block_copy(), shared
>     between parallel tasks. For now used only to keep information about
>     first error: is it read or not.
>   - convert block_copy_dirty_clusters to aio-task loop.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 104 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 91 insertions(+), 13 deletions(-)

Looks good, just some nits:

> diff --git a/block/block-copy.c b/block/block-copy.c
> index 910947cb43..9994598eb7 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c

[...]

> @@ -225,6 +237,30 @@ void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm)
>      s->progress = pm;
>  }
>  
> +/* Takes ownership on @task */

*of

> +static coroutine_fn int block_copy_task_run(AioTaskPool *pool,
> +                                            BlockCopyTask *task)
> +{
> +    if (!pool) {
> +        int ret = task->task.func(&task->task);
> +
> +        g_free(task);
> +        return ret;
> +    }
> +
> +    aio_task_pool_wait_slot(pool);
> +    if (aio_task_pool_status(pool) < 0) {
> +        co_put_to_shres(task->s->mem, task->bytes);
> +        block_copy_task_end(task, -EAGAIN);

Hm, I think -ECANCELED might be better.  Not that it really matters...

> +        g_free(task);
> +        return aio_task_pool_status(pool);

Here, too.  (Here, there’s also the fact that this task doesn’t really
fail because of the same reason that the other task failed, so we should
have our own error code here.)

> +    }
> +
> +    aio_task_pool_start_task(pool, &task->task);
> +
> +    return 0;
> +}
> +
>  /*
>   * block_copy_do_copy
>   *

[...]

> @@ -519,25 +584,38 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s,

[...]

> +out:
> +    if (aio) {
> +        aio_task_pool_wait_all(aio);
> +        if (ret == 0) {
> +            ret = aio_task_pool_status(aio);
> +        }
> +        g_free(aio);

aio_task_pool_free()?

Max


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

Re: [PATCH v2 0/6] block-copy: use aio-task-pool

[Vladimir Sementsov-Ogievskiy]

ping :)

25.03.2020 16:46, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> This is the next step of improving block-copy: use aio task pool.
> 
> Async copying loop has better performance than linear, which is shown
> in original series (was
> "[RFC 00/24] backup performance: block_status + async", so this is
> called v2)
> 
> Vladimir Sementsov-Ogievskiy (6):
>    block/block-copy: rename in-flight requests to tasks
>    block/block-copy: alloc task on each iteration
>    block/block-copy: add state pointer to BlockCopyTask
>    block/block-copy: move task size initial calculation to _task_create
>    block/block-copy: move block_copy_task_create down
>    block/block-copy: use aio-task-pool API
> 
>   block/block-copy.c | 250 ++++++++++++++++++++++++++++++---------------
>   1 file changed, 168 insertions(+), 82 deletions(-)
> 


-- 
Best regards,
Vladimir