[PATCH 0/6] block-copy: memory limit

[PATCH 0/6] block-copy: memory limit

[Vladimir Sementsov-Ogievskiy]

Hi all!

I'm going to bring block-status driven, async copying process to
block-copy, to make it fast. The first step is to limit memory usage of
backup, here is it.

Based on my "[PATCH v15 0/5] backup-top filter driver for backup":
Based-on: <20191001131409.14202-1-vsementsov@virtuozzo.com>

Vladimir Sementsov-Ogievskiy (6):
  block/block-copy: allocate buffer in block_copy_with_bounce_buffer
  block/block-copy: limit copy_range_size to 16 MiB
  block/block-copy: refactor copying
  util: introduce co-shared-amount
  block/block-copy: add memory limit
  block/block-copy: increase buffered copy request

 include/block/block-copy.h      |   5 +-
 include/qemu/co-shared-amount.h |  66 ++++++++++++
 block/block-copy.c              | 179 +++++++++++++++++---------------
 util/qemu-co-shared-amount.c    |  77 ++++++++++++++
 block/trace-events              |   6 +-
 util/Makefile.objs              |   1 +
 6 files changed, 246 insertions(+), 88 deletions(-)
 create mode 100644 include/qemu/co-shared-amount.h
 create mode 100644 util/qemu-co-shared-amount.c

-- 
2.21.0

[PATCH 1/6] block/block-copy: allocate buffer in block_copy_with_bounce_buffer

[Vladimir Sementsov-Ogievskiy]

Move bounce_buffer allocation block_copy_with_bounce_buffer. This
commit simplifies further work on implementing copying by larger chunks
(of different size) and further asynchronous handling of block_copy
iterations (with help of block/aio_task API).

Allocation works fast, a lot faster than disk io, so it's not a problem
that we now allocate/free bounce_buffer more times. And we anyway will
have to allocate several bounce_buffers for parallel execution of loop
iterations in future.

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

diff --git a/block/block-copy.c b/block/block-copy.c
index 5404bc921d..aca0f893d7 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -126,20 +126,17 @@ void block_copy_set_callbacks(
 static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
                                                       int64_t start,
                                                       int64_t end,
-                                                      bool *error_is_read,
-                                                      void **bounce_buffer)
+                                                      bool *error_is_read)
 {
     int ret;
     int nbytes;
+    void *bounce_buffer = qemu_blockalign(s->source->bs, s->cluster_size);
 
     assert(QEMU_IS_ALIGNED(start, s->cluster_size));
     bdrv_reset_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
     nbytes = MIN(s->cluster_size, s->len - start);
-    if (!*bounce_buffer) {
-        *bounce_buffer = qemu_blockalign(s->source->bs, s->cluster_size);
-    }
 
-    ret = bdrv_co_pread(s->source, start, nbytes, *bounce_buffer, 0);
+    ret = bdrv_co_pread(s->source, start, nbytes, bounce_buffer, 0);
     if (ret < 0) {
         trace_block_copy_with_bounce_buffer_read_fail(s, start, ret);
         if (error_is_read) {
@@ -148,7 +145,7 @@ static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
         goto fail;
     }
 
-    ret = bdrv_co_pwrite(s->target, start, nbytes, *bounce_buffer,
+    ret = bdrv_co_pwrite(s->target, start, nbytes, bounce_buffer,
                          s->write_flags);
     if (ret < 0) {
         trace_block_copy_with_bounce_buffer_write_fail(s, start, ret);
@@ -158,8 +155,11 @@ static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
         goto fail;
     }
 
+    qemu_vfree(bounce_buffer);
+
     return nbytes;
 fail:
+    qemu_vfree(bounce_buffer);
     bdrv_set_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
     return ret;
 
@@ -271,7 +271,6 @@ int coroutine_fn block_copy(BlockCopyState *s,
 {
     int ret = 0;
     int64_t end = bytes + start; /* bytes */
-    void *bounce_buffer = NULL;
     int64_t status_bytes;
     BlockCopyInFlightReq req;
 
@@ -324,7 +323,7 @@ int coroutine_fn block_copy(BlockCopyState *s,
         }
         if (!s->use_copy_range) {
             ret = block_copy_with_bounce_buffer(s, start, dirty_end,
-                                                error_is_read, &bounce_buffer);
+                                                error_is_read);
         }
         if (ret < 0) {
             break;
@@ -335,10 +334,6 @@ int coroutine_fn block_copy(BlockCopyState *s,
         ret = 0;
     }
 
-    if (bounce_buffer) {
-        qemu_vfree(bounce_buffer);
-    }
-
     block_copy_inflight_req_end(&req);
 
     return ret;
-- 
2.21.0

Re: [PATCH 1/6] block/block-copy: allocate buffer in block_copy_with_bounce_buffer

[Max Reitz]

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

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> Move bounce_buffer allocation block_copy_with_bounce_buffer. This
> commit simplifies further work on implementing copying by larger chunks
> (of different size) and further asynchronous handling of block_copy
> iterations (with help of block/aio_task API).
> 
> Allocation works fast, a lot faster than disk io, so it's not a problem
> that we now allocate/free bounce_buffer more times. And we anyway will
> have to allocate several bounce_buffers for parallel execution of loop
> iterations in future.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)

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


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

[PATCH 2/6] block/block-copy: limit copy_range_size to 16 MiB

[Vladimir Sementsov-Ogievskiy]

Large copy range may imply memory allocation and large io effort, so
using 2G copy range request may be bad idea. Let's limit it to 16 MiB.
It also helps the following patch to refactor copy-with-offload
fallback to copy-with-bounce-buffer.

Note, that total memory usage of backup is still not limited, it will
be fixed in further commit.

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

diff --git a/block/block-copy.c b/block/block-copy.c
index aca0f893d7..75287ce24d 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -18,6 +18,9 @@
 #include "qapi/error.h"
 #include "block/block-copy.h"
 #include "sysemu/block-backend.h"
+#include "qemu/units.h"
+
+#define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
 
 static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
                                                        int64_t start,
@@ -70,9 +73,12 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
 {
     BlockCopyState *s;
     BdrvDirtyBitmap *copy_bitmap;
+
+    /* Ignore BLOCK_COPY_MAX_COPY_RANGE if requested cluster_size is larger */
     uint32_t max_transfer =
-            MIN_NON_ZERO(INT_MAX, MIN_NON_ZERO(source->bs->bl.max_transfer,
-                                               target->bs->bl.max_transfer));
+            MIN_NON_ZERO(MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+                         MIN_NON_ZERO(source->bs->bl.max_transfer,
+                                      target->bs->bl.max_transfer));
 
     copy_bitmap = bdrv_create_dirty_bitmap(source->bs, cluster_size, NULL,
                                            errp);
-- 
2.21.0

Re: [PATCH 2/6] block/block-copy: limit copy_range_size to 16 MiB

[Max Reitz]

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

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> Large copy range may imply memory allocation and large io effort, so
> using 2G copy range request may be bad idea. Let's limit it to 16 MiB.
> It also helps the following patch to refactor copy-with-offload
> fallback to copy-with-bounce-buffer.
> 
> Note, that total memory usage of backup is still not limited, it will
> be fixed in further commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

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


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

[PATCH 3/6] block/block-copy: refactor copying

[Vladimir Sementsov-Ogievskiy]

Merge copying code into one function block_copy_do_copy, which only
calls bdrv_ io functions and don't do any synchronization (like dirty
bitmap set/reset).

Refactor block_copy() function so that it takes full decision about
size of chunk to be copied and does all the synchronization (checking
intersecting requests, set/reset dirty bitmaps).

It will help:
 - introduce parallel processing of block_copy iterations: we need to
   calculate chunk size, start async chunk copying and go to the next
   iteration
 - simplify synchronization improvement (like memory limiting in
   further commit and reducing critical section (now we lock the whole
   requested range, when actually we need to lock only dirty region
   which we handle at the moment))

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/block-copy.c | 113 ++++++++++++++++++++-------------------------
 block/trace-events |   6 +--
 2 files changed, 53 insertions(+), 66 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 75287ce24d..cc49d2345d 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -126,25 +126,43 @@ void block_copy_set_callbacks(
 }
 
 /*
- * Copy range to target with a bounce buffer and return the bytes copied. If
- * error occurred, return a negative error number
+ * block_copy_do_copy
+ *
+ * Do copy of cluser-aligned chunk. @end is allowed to exceed s->len only to
+ * cover last cluster when s->len is not aligned to clusters.
+ *
+ * No sync here: nor bitmap neighter intersecting requests handling, only copy.
+ *
+ * Returns 0 on success.
  */
-static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
-                                                      int64_t start,
-                                                      int64_t end,
-                                                      bool *error_is_read)
+static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
+                                           int64_t start, int64_t end,
+                                           bool *error_is_read)
 {
     int ret;
-    int nbytes;
-    void *bounce_buffer = qemu_blockalign(s->source->bs, s->cluster_size);
+    int nbytes = MIN(end, s->len) - start;
+    void *bounce_buffer = NULL;
 
     assert(QEMU_IS_ALIGNED(start, s->cluster_size));
-    bdrv_reset_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
-    nbytes = MIN(s->cluster_size, s->len - start);
+    assert(QEMU_IS_ALIGNED(end, s->cluster_size));
+    assert(end < s->len || end == QEMU_ALIGN_UP(s->len, s->cluster_size));
+
+    if (s->use_copy_range) {
+        ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
+                                 0, s->write_flags);
+        if (ret < 0) {
+            trace_block_copy_copy_range_fail(s, start, ret);
+            s->use_copy_range = false;
+        } else {
+            return ret;
+        }
+    }
+
+    bounce_buffer = qemu_blockalign(s->source->bs, nbytes);
 
     ret = bdrv_co_pread(s->source, start, nbytes, bounce_buffer, 0);
     if (ret < 0) {
-        trace_block_copy_with_bounce_buffer_read_fail(s, start, ret);
+        trace_block_copy_read_fail(s, start, ret);
         if (error_is_read) {
             *error_is_read = true;
         }
@@ -154,7 +172,7 @@ static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
     ret = bdrv_co_pwrite(s->target, start, nbytes, bounce_buffer,
                          s->write_flags);
     if (ret < 0) {
-        trace_block_copy_with_bounce_buffer_write_fail(s, start, ret);
+        trace_block_copy_write_fail(s, start, ret);
         if (error_is_read) {
             *error_is_read = false;
         }
@@ -163,42 +181,12 @@ static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
 
     qemu_vfree(bounce_buffer);
 
-    return nbytes;
+    return 0;
+
 fail:
     qemu_vfree(bounce_buffer);
-    bdrv_set_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
-    return ret;
-
-}
-
-/*
- * Copy range to target and return the bytes copied. If error occurred, return a
- * negative error number.
- */
-static int coroutine_fn block_copy_with_offload(BlockCopyState *s,
-                                                int64_t start,
-                                                int64_t end)
-{
-    int ret;
-    int nr_clusters;
-    int nbytes;
 
-    assert(QEMU_IS_ALIGNED(s->copy_range_size, s->cluster_size));
-    assert(QEMU_IS_ALIGNED(start, s->cluster_size));
-    nbytes = MIN(s->copy_range_size, MIN(end, s->len) - start);
-    nr_clusters = DIV_ROUND_UP(nbytes, s->cluster_size);
-    bdrv_reset_dirty_bitmap(s->copy_bitmap, start,
-                            s->cluster_size * nr_clusters);
-    ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
-                            0, s->write_flags);
-    if (ret < 0) {
-        trace_block_copy_with_offload_fail(s, start, ret);
-        bdrv_set_dirty_bitmap(s->copy_bitmap, start,
-                              s->cluster_size * nr_clusters);
-        return ret;
-    }
-
-    return nbytes;
+    return ret;
 }
 
 /*
@@ -294,7 +282,7 @@ int coroutine_fn block_copy(BlockCopyState *s,
     block_copy_inflight_req_begin(s, &req, start, end);
 
     while (start < end) {
-        int64_t dirty_end;
+        int64_t next_zero, chunk_end;
 
         if (!bdrv_dirty_bitmap_get(s->copy_bitmap, start)) {
             trace_block_copy_skip(s, start);
@@ -302,10 +290,15 @@ int coroutine_fn block_copy(BlockCopyState *s,
             continue; /* already copied */
         }
 
-        dirty_end = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, start,
-                                                (end - start));
-        if (dirty_end < 0) {
-            dirty_end = end;
+        chunk_end = MIN(end, start + (s->use_copy_range ?
+                                      s->copy_range_size : s->cluster_size));
+
+        next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, start,
+                                                chunk_end - start);
+        if (next_zero >= 0) {
+            assert(next_zero > start); /* start is dirty */
+            assert(next_zero < chunk_end); /* no need to do MIN() */
+            chunk_end = next_zero;
         }
 
         if (s->skip_unallocated) {
@@ -316,27 +309,21 @@ int coroutine_fn block_copy(BlockCopyState *s,
                 continue;
             }
             /* Clamp to known allocated region */
-            dirty_end = MIN(dirty_end, start + status_bytes);
+            chunk_end = MIN(chunk_end, start + status_bytes);
         }
 
         trace_block_copy_process(s, start);
 
-        if (s->use_copy_range) {
-            ret = block_copy_with_offload(s, start, dirty_end);
-            if (ret < 0) {
-                s->use_copy_range = false;
-            }
-        }
-        if (!s->use_copy_range) {
-            ret = block_copy_with_bounce_buffer(s, start, dirty_end,
-                                                error_is_read);
-        }
+        bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
+
+        ret = block_copy_do_copy(s, start, chunk_end, error_is_read);
         if (ret < 0) {
+            bdrv_set_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
             break;
         }
 
-        start += ret;
-        s->progress_bytes_callback(ret, s->progress_opaque);
+        s->progress_bytes_callback(chunk_end - start, s->progress_opaque);
+        start = chunk_end;
         ret = 0;
     }
 
diff --git a/block/trace-events b/block/trace-events
index b8d70f5242..ccde15a14c 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -45,9 +45,9 @@ backup_do_cow_return(void *job, int64_t offset, uint64_t bytes, int ret) "job %p
 block_copy_skip(void *bcs, int64_t start) "bcs %p start %"PRId64
 block_copy_skip_range(void *bcs, int64_t start, uint64_t bytes) "bcs %p start %"PRId64" bytes %"PRId64
 block_copy_process(void *bcs, int64_t start) "bcs %p start %"PRId64
-block_copy_with_bounce_buffer_read_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
-block_copy_with_bounce_buffer_write_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
-block_copy_with_offload_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
+block_copy_copy_range_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
+block_copy_read_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
+block_copy_write_fail(void *bcs, int64_t start, int ret) "bcs %p start %"PRId64" ret %d"
 
 # ../blockdev.c
 qmp_block_job_cancel(void *job) "job %p"
-- 
2.21.0

Re: [PATCH 3/6] block/block-copy: refactor copying

[Max Reitz]

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

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> Merge copying code into one function block_copy_do_copy, which only
> calls bdrv_ io functions and don't do any synchronization (like dirty
> bitmap set/reset).
> 
> Refactor block_copy() function so that it takes full decision about
> size of chunk to be copied and does all the synchronization (checking
> intersecting requests, set/reset dirty bitmaps).
> 
> It will help:
>  - introduce parallel processing of block_copy iterations: we need to
>    calculate chunk size, start async chunk copying and go to the next
>    iteration
>  - simplify synchronization improvement (like memory limiting in
>    further commit and reducing critical section (now we lock the whole
>    requested range, when actually we need to lock only dirty region
>    which we handle at the moment))
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/block-copy.c | 113 ++++++++++++++++++++-------------------------
>  block/trace-events |   6 +--
>  2 files changed, 53 insertions(+), 66 deletions(-)

Looks good to me, just some clean-up path nit picks below.

> diff --git a/block/block-copy.c b/block/block-copy.c
> index 75287ce24d..cc49d2345d 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -126,25 +126,43 @@ void block_copy_set_callbacks(
>  }
>  
>  /*
> - * Copy range to target with a bounce buffer and return the bytes copied. If
> - * error occurred, return a negative error number
> + * block_copy_do_copy
> + *
> + * Do copy of cluser-aligned chunk. @end is allowed to exceed s->len only to
> + * cover last cluster when s->len is not aligned to clusters.
> + *
> + * No sync here: nor bitmap neighter intersecting requests handling, only copy.
> + *
> + * Returns 0 on success.
>   */
> -static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
> -                                                      int64_t start,
> -                                                      int64_t end,
> -                                                      bool *error_is_read)
> +static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
> +                                           int64_t start, int64_t end,
> +                                           bool *error_is_read)
>  {
>      int ret;
> -    int nbytes;
> -    void *bounce_buffer = qemu_blockalign(s->source->bs, s->cluster_size);
> +    int nbytes = MIN(end, s->len) - start;
> +    void *bounce_buffer = NULL;
>  
>      assert(QEMU_IS_ALIGNED(start, s->cluster_size));
> -    bdrv_reset_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
> -    nbytes = MIN(s->cluster_size, s->len - start);
> +    assert(QEMU_IS_ALIGNED(end, s->cluster_size));
> +    assert(end < s->len || end == QEMU_ALIGN_UP(s->len, s->cluster_size));
> +
> +    if (s->use_copy_range) {
> +        ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
> +                                 0, s->write_flags);
> +        if (ret < 0) {
> +            trace_block_copy_copy_range_fail(s, start, ret);
> +            s->use_copy_range = false;
> +        } else {
> +            return ret;

Maybe the “fail” label should be called ”out” and then we could go there
from here.  Doesn’t make much of a difference here (1 LoC either way),
but maybe it’s a bit cleaner to always use the clean-up path in this
function (even when there’s nothing to clean up).

*shrug*

> +        }
> +    }
> +
> +    bounce_buffer = qemu_blockalign(s->source->bs, nbytes);
>  
>      ret = bdrv_co_pread(s->source, start, nbytes, bounce_buffer, 0);
>      if (ret < 0) {
> -        trace_block_copy_with_bounce_buffer_read_fail(s, start, ret);
> +        trace_block_copy_read_fail(s, start, ret);
>          if (error_is_read) {
>              *error_is_read = true;
>          }

[...]

> @@ -163,42 +181,12 @@ static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
>  
>      qemu_vfree(bounce_buffer);
>  
> -    return nbytes;
> +    return 0;
> +
>  fail:
>      qemu_vfree(bounce_buffer);
> -    bdrv_set_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
> -    return ret;
> -
> -}

Wouldn’t it be simpler to drop the “qemu_vfree(bounce_buffer); return
0;” above the fail label and just fall through?

In any case:

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


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

Re: [PATCH 3/6] block/block-copy: refactor copying

[Vladimir Sementsov-Ogievskiy]

07.10.2019 17:17, Max Reitz wrote:
> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>> Merge copying code into one function block_copy_do_copy, which only
>> calls bdrv_ io functions and don't do any synchronization (like dirty
>> bitmap set/reset).
>>
>> Refactor block_copy() function so that it takes full decision about
>> size of chunk to be copied and does all the synchronization (checking
>> intersecting requests, set/reset dirty bitmaps).
>>
>> It will help:
>>   - introduce parallel processing of block_copy iterations: we need to
>>     calculate chunk size, start async chunk copying and go to the next
>>     iteration
>>   - simplify synchronization improvement (like memory limiting in
>>     further commit and reducing critical section (now we lock the whole
>>     requested range, when actually we need to lock only dirty region
>>     which we handle at the moment))
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/block-copy.c | 113 ++++++++++++++++++++-------------------------
>>   block/trace-events |   6 +--
>>   2 files changed, 53 insertions(+), 66 deletions(-)
> 
> Looks good to me, just some clean-up path nit picks below.
> 
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index 75287ce24d..cc49d2345d 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
>> @@ -126,25 +126,43 @@ void block_copy_set_callbacks(
>>   }
>>   
>>   /*
>> - * Copy range to target with a bounce buffer and return the bytes copied. If
>> - * error occurred, return a negative error number
>> + * block_copy_do_copy
>> + *
>> + * Do copy of cluser-aligned chunk. @end is allowed to exceed s->len only to
>> + * cover last cluster when s->len is not aligned to clusters.
>> + *
>> + * No sync here: nor bitmap neighter intersecting requests handling, only copy.
>> + *
>> + * Returns 0 on success.
>>    */
>> -static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
>> -                                                      int64_t start,
>> -                                                      int64_t end,
>> -                                                      bool *error_is_read)
>> +static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
>> +                                           int64_t start, int64_t end,
>> +                                           bool *error_is_read)
>>   {
>>       int ret;
>> -    int nbytes;
>> -    void *bounce_buffer = qemu_blockalign(s->source->bs, s->cluster_size);
>> +    int nbytes = MIN(end, s->len) - start;
>> +    void *bounce_buffer = NULL;
>>   
>>       assert(QEMU_IS_ALIGNED(start, s->cluster_size));
>> -    bdrv_reset_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
>> -    nbytes = MIN(s->cluster_size, s->len - start);
>> +    assert(QEMU_IS_ALIGNED(end, s->cluster_size));
>> +    assert(end < s->len || end == QEMU_ALIGN_UP(s->len, s->cluster_size));
>> +
>> +    if (s->use_copy_range) {
>> +        ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
>> +                                 0, s->write_flags);
>> +        if (ret < 0) {
>> +            trace_block_copy_copy_range_fail(s, start, ret);
>> +            s->use_copy_range = false;
>> +        } else {
>> +            return ret;
> 
> Maybe the “fail” label should be called ”out” and then we could go there
> from here.  Doesn’t make much of a difference here (1 LoC either way),
> but maybe it’s a bit cleaner to always use the clean-up path in this
> function (even when there’s nothing to clean up).

Hmm, I always do immediate return if possible (things which needs cleanup not
yet happened). A kind of taste of course, you are maintainer, so if you like
another way, I'll change it to goto.

> 
> *shrug*
> 
>> +        }
>> +    }
>> +
>> +    bounce_buffer = qemu_blockalign(s->source->bs, nbytes);
>>   
>>       ret = bdrv_co_pread(s->source, start, nbytes, bounce_buffer, 0);
>>       if (ret < 0) {
>> -        trace_block_copy_with_bounce_buffer_read_fail(s, start, ret);
>> +        trace_block_copy_read_fail(s, start, ret);
>>           if (error_is_read) {
>>               *error_is_read = true;
>>           }
> 
> [...]
> 
>> @@ -163,42 +181,12 @@ static int coroutine_fn block_copy_with_bounce_buffer(BlockCopyState *s,
>>   
>>       qemu_vfree(bounce_buffer);
>>   
>> -    return nbytes;
>> +    return 0;
>> +
>>   fail:
>>       qemu_vfree(bounce_buffer);
>> -    bdrv_set_dirty_bitmap(s->copy_bitmap, start, s->cluster_size);
>> -    return ret;
>> -
>> -}
> 
> Wouldn’t it be simpler to drop the “qemu_vfree(bounce_buffer); return
> 0;” above the fail label and just fall through?

Hmm yes that's better and more usual pattern. Will do.

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


-- 
Best regards,
Vladimir

[PATCH 4/6] util: introduce co-shared-amount

[Vladimir Sementsov-Ogievskiy]

Introduce an API for some shared splitable resource, like memory.
It's going to be used by backup. Backup uses both read/write io and
copy_range. copy_range may consume memory implictly, so new API is
abstract: it don't allocate any real memory but only handling out
tickets.

The idea is that we have some total amount of something and callers
should wait in coroutine queue if there is not enough of the resource
at the moment.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/co-shared-amount.h | 66 ++++++++++++++++++++++++++++
 util/qemu-co-shared-amount.c    | 77 +++++++++++++++++++++++++++++++++
 util/Makefile.objs              |  1 +
 3 files changed, 144 insertions(+)
 create mode 100644 include/qemu/co-shared-amount.h
 create mode 100644 util/qemu-co-shared-amount.c

diff --git a/include/qemu/co-shared-amount.h b/include/qemu/co-shared-amount.h
new file mode 100644
index 0000000000..e2dbc43dfd
--- /dev/null
+++ b/include/qemu/co-shared-amount.h
@@ -0,0 +1,66 @@
+/*
+ * Generic shared amount for coroutines
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef QEMU_CO_SHARED_AMOUNT_H
+#define QEMU_CO_SHARED_AMOUNT_H
+
+
+typedef struct QemuCoSharedAmount QemuCoSharedAmount;
+
+/*
+ * Create QemuCoSharedAmount structure
+ *
+ * @total: total amount to be shared between clients
+ *
+ * Note: this API is not thread-safe as it originally designed to be used in
+ * backup block-job and called from one aio context. If multiple threads support
+ * is needed it should be implemented (for ex., protect QemuCoSharedAmount by
+ * mutex).
+ */
+QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total);
+
+/*
+ * Release QemuCoSharedAmount structure
+ */
+void qemu_co_shared_amount_free(QemuCoSharedAmount *s);
+
+/*
+ * Try to get n peaces. If not enough free peaces returns false, otherwise true.
+ */
+bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n);
+
+/*
+ * Get n peaces. If not enough yields. Return on success.
+ */
+void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n);
+
+/*
+ * Put n peaces. Client must not put more than it gets, still it may put in
+ * split: for example, get(5) and then put(3), put(2). All peaces must be put
+ * back before qemu_co_shared_amount_free call.
+ */
+void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n);
+
+
+#endif /* QEMU_CO_SHARED_AMOUNT_H */
diff --git a/util/qemu-co-shared-amount.c b/util/qemu-co-shared-amount.c
new file mode 100644
index 0000000000..8855ce5705
--- /dev/null
+++ b/util/qemu-co-shared-amount.c
@@ -0,0 +1,77 @@
+/*
+ * Generic shared amount for coroutines
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/coroutine.h"
+#include "qemu/co-shared-amount.h"
+
+struct QemuCoSharedAmount {
+    uint64_t total;
+    uint64_t taken;
+
+    CoQueue queue;
+};
+
+QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total)
+{
+    QemuCoSharedAmount *s = g_new0(QemuCoSharedAmount, 1);
+
+    s->total = total;
+    qemu_co_queue_init(&s->queue);
+
+    return s;
+}
+
+void qemu_co_shared_amount_free(QemuCoSharedAmount *s)
+{
+    assert(s->taken == 0);
+    g_free(s);
+}
+
+bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n)
+{
+    if (n < s->total && s->total - n >= s->taken) {
+        s->taken += n;
+        return true;
+    }
+
+    return false;
+}
+
+void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n)
+{
+    assert(n < s->total);
+    while (s->total - n < s->taken) {
+        qemu_co_queue_wait(&s->queue, NULL);
+    }
+
+    assert(qemu_co_try_get_amount(s, n));
+}
+
+void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n)
+{
+    assert(n <= s->taken);
+    s->taken -= n;
+    qemu_co_queue_restart_all(&s->queue);
+}
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 41bf59d127..65ae18993a 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -37,6 +37,7 @@ util-obj-y += rcu.o
 util-obj-$(CONFIG_MEMBARRIER) += sys_membarrier.o
 util-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
 util-obj-y += qemu-coroutine-sleep.o
+util-obj-y += qemu-co-shared-amount.o
 util-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
 util-obj-y += buffer.o
 util-obj-y += timed-average.o
-- 
2.21.0

Re: [PATCH 4/6] util: introduce co-shared-amount

[Max Reitz]

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

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> Introduce an API for some shared splitable resource, like memory.

*splittable, I suppose

> It's going to be used by backup. Backup uses both read/write io and
> copy_range. copy_range may consume memory implictly, so new API is

*the new API

> abstract: it don't allocate any real memory but only handling out

*It doesn’t allocate any real memory but only hands out

> tickets.

Note that allocation doesn’t necessarily mean you get a handle to the
allocated object.  It just means that some resource (or part of a
resource) is now under your exclusive control.  At least that’s how I
understand the term.

So this is indeed some form of allocation.

> The idea is that we have some total amount of something and callers
> should wait in coroutine queue if there is not enough of the resource
> at the moment.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/qemu/co-shared-amount.h | 66 ++++++++++++++++++++++++++++
>  util/qemu-co-shared-amount.c    | 77 +++++++++++++++++++++++++++++++++
>  util/Makefile.objs              |  1 +
>  3 files changed, 144 insertions(+)
>  create mode 100644 include/qemu/co-shared-amount.h
>  create mode 100644 util/qemu-co-shared-amount.c
> 
> diff --git a/include/qemu/co-shared-amount.h b/include/qemu/co-shared-amount.h
> new file mode 100644
> index 0000000000..e2dbc43dfd
> --- /dev/null
> +++ b/include/qemu/co-shared-amount.h
> @@ -0,0 +1,66 @@
> +/*
> + * Generic shared amount for coroutines

“amount” always begs the question “amount of what”.  So maybe something
more verbose like “Helper functionality for distributing a fixed total
amount of an abstract resource among multiple coroutines” would answer
that question.

(I can’t come up with a better short name than shared-amount, though.
It does get the point across once the concept’s clear.)

> + *
> + * Copyright (c) 2019 Virtuozzo International GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef QEMU_CO_SHARED_AMOUNT_H
> +#define QEMU_CO_SHARED_AMOUNT_H
> +
> +
> +typedef struct QemuCoSharedAmount QemuCoSharedAmount;
> +
> +/*
> + * Create QemuCoSharedAmount structure
> + *
> + * @total: total amount to be shared between clients
> + *
> + * Note: this API is not thread-safe as it originally designed to be used in
> + * backup block-job and called from one aio context. If multiple threads support
> + * is needed it should be implemented (for ex., protect QemuCoSharedAmount by
> + * mutex).

I think the simple note “This API is not thread-safe” will suffice.  The
rest seems more confusing to me than that it really helps.

> + */
> +QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total);
> +
> +/*
> + * Release QemuCoSharedAmount structure

I’d add the note from qemu_co_put_amount():

“This function may only be called once everything allocated by all
clients has been deallocated/released.”

> + */
> +void qemu_co_shared_amount_free(QemuCoSharedAmount *s);
> +
> +/*
> + * Try to get n peaces. If not enough free peaces returns false, otherwise true.

*pieces

But I’d prefer “Try to allocate an amount of @n.  Return true on
success, and false if there is too little left of the collective
resource to fulfill the request.”

> + */
> +bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n);
> +
> +/*
> + * Get n peaces. If not enough yields. Return on success.

I’d prefer “Allocate an amount of $n, and, if necessary, yield until
that becomes possible.”

> + */
> +void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n);
> +
> +/*
> + * Put n peaces. Client must not put more than it gets, still it may put in
> + * split: for example, get(5) and then put(3), put(2). All peaces must be put
> + * back before qemu_co_shared_amount_free call.

I’d prefer “Deallocate/Release an amount of @n.  The total amount
allocated by a caller does not need to be deallocated/released with a
single call, but may be split over several calls.  For example, get(4),
get(3), and then put(5), put(2).”

(And drop the qemu_co_shared_amount_free() reference, because it should
so say there.)

> + */
> +void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n);
> +
> +
> +#endif /* QEMU_CO_SHARED_AMOUNT_H */
> diff --git a/util/qemu-co-shared-amount.c b/util/qemu-co-shared-amount.c
> new file mode 100644
> index 0000000000..8855ce5705
> --- /dev/null
> +++ b/util/qemu-co-shared-amount.c
> @@ -0,0 +1,77 @@
> +/*
> + * Generic shared amount for coroutines
> + *
> + * Copyright (c) 2019 Virtuozzo International GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/coroutine.h"
> +#include "qemu/co-shared-amount.h"
> +
> +struct QemuCoSharedAmount {
> +    uint64_t total;
> +    uint64_t taken;

I’d reverse the “taken” to be “available”.  Then the only purpose of
“total” would be as part of assertions.

> +
> +    CoQueue queue;
> +};
> +
> +QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total)
> +{
> +    QemuCoSharedAmount *s = g_new0(QemuCoSharedAmount, 1);
> +
> +    s->total = total;
> +    qemu_co_queue_init(&s->queue);
> +
> +    return s;
> +}
> +
> +void qemu_co_shared_amount_free(QemuCoSharedAmount *s)
> +{
> +    assert(s->taken == 0);
> +    g_free(s);
> +}
> +
> +bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n)
> +{
> +    if (n < s->total && s->total - n >= s->taken) {

(This’d become simply “s->available >= n”)

(And to be honest I have a hard time parsing that second condition.  To
me, the natural order would appear to be “s->total - s->taken >= n”.  I
mean, I can see that it matches by rearranging the inequation, but...
And in this order you could drop the “n < s->total” part because it’s
guaranteed that s->taken <= s->total.)

> +        s->taken += n;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n)
> +{
> +    assert(n < s->total);
> +    while (s->total - n < s->taken) {
> +        qemu_co_queue_wait(&s->queue, NULL);
> +    }
> +
> +    assert(qemu_co_try_get_amount(s, n));

I’d refrain from putting things that should do something in assertions
because assert() is not guaranteed to be compiled.

It is in practice in qemu, but I still don’t like it too much.

Furthermore, it appears to me that the following would be simpler:

while (!qemu_co_try_get_amount(s, n)) {
    qemu_co_queue_wait(&s->queue, NULL);
}

> +}
> +
> +void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n)
> +{
> +    assert(n <= s->taken);
> +    s->taken -= n;
> +    qemu_co_queue_restart_all(&s->queue);

It itches me to ask for a better allocation strategy (like maybe
smallest-allocation-first), but I suppose I should just scratch myself.

Max

> +}
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 41bf59d127..65ae18993a 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -37,6 +37,7 @@ util-obj-y += rcu.o
>  util-obj-$(CONFIG_MEMBARRIER) += sys_membarrier.o
>  util-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
>  util-obj-y += qemu-coroutine-sleep.o
> +util-obj-y += qemu-co-shared-amount.o
>  util-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
>  util-obj-y += buffer.o
>  util-obj-y += timed-average.o
> 



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

Re: [PATCH 4/6] util: introduce co-shared-amount

[Vladimir Sementsov-Ogievskiy]

07.10.2019 18:22, Max Reitz wrote:
> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>> Introduce an API for some shared splitable resource, like memory.
> 
> *splittable, I suppose
> 
>> It's going to be used by backup. Backup uses both read/write io and
>> copy_range. copy_range may consume memory implictly, so new API is
> 
> *the new API
> 
>> abstract: it don't allocate any real memory but only handling out
> 
> *It doesn’t allocate any real memory but only hands out
> 
>> tickets.
> 
> Note that allocation doesn’t necessarily mean you get a handle to the
> allocated object.  It just means that some resource (or part of a
> resource) is now under your exclusive control.  At least that’s how I
> understand the term.
> 
> So this is indeed some form of allocation.
> 
>> The idea is that we have some total amount of something and callers
>> should wait in coroutine queue if there is not enough of the resource
>> at the moment.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/qemu/co-shared-amount.h | 66 ++++++++++++++++++++++++++++
>>   util/qemu-co-shared-amount.c    | 77 +++++++++++++++++++++++++++++++++
>>   util/Makefile.objs              |  1 +
>>   3 files changed, 144 insertions(+)
>>   create mode 100644 include/qemu/co-shared-amount.h
>>   create mode 100644 util/qemu-co-shared-amount.c
>>
>> diff --git a/include/qemu/co-shared-amount.h b/include/qemu/co-shared-amount.h
>> new file mode 100644
>> index 0000000000..e2dbc43dfd
>> --- /dev/null
>> +++ b/include/qemu/co-shared-amount.h
>> @@ -0,0 +1,66 @@
>> +/*
>> + * Generic shared amount for coroutines
> 
> “amount” always begs the question “amount of what”.  So maybe something
> more verbose like “Helper functionality for distributing a fixed total
> amount of an abstract resource among multiple coroutines” would answer
> that question.
> 
> (I can’t come up with a better short name than shared-amount, though.
> It does get the point across once the concept’s clear.)
> 
>> + *
>> + * Copyright (c) 2019 Virtuozzo International GmbH
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#ifndef QEMU_CO_SHARED_AMOUNT_H
>> +#define QEMU_CO_SHARED_AMOUNT_H
>> +
>> +
>> +typedef struct QemuCoSharedAmount QemuCoSharedAmount;
>> +
>> +/*
>> + * Create QemuCoSharedAmount structure
>> + *
>> + * @total: total amount to be shared between clients
>> + *
>> + * Note: this API is not thread-safe as it originally designed to be used in
>> + * backup block-job and called from one aio context. If multiple threads support
>> + * is needed it should be implemented (for ex., protect QemuCoSharedAmount by
>> + * mutex).
> 
> I think the simple note “This API is not thread-safe” will suffice.  The
> rest seems more confusing to me than that it really helps.
> 
>> + */
>> +QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total);
>> +
>> +/*
>> + * Release QemuCoSharedAmount structure
> 
> I’d add the note from qemu_co_put_amount():
> 
> “This function may only be called once everything allocated by all
> clients has been deallocated/released.”
> 
>> + */
>> +void qemu_co_shared_amount_free(QemuCoSharedAmount *s);
>> +
>> +/*
>> + * Try to get n peaces. If not enough free peaces returns false, otherwise true.
> 
> *pieces
> 
> But I’d prefer “Try to allocate an amount of @n.  Return true on
> success, and false if there is too little left of the collective
> resource to fulfill the request.”
> 
>> + */
>> +bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n);
>> +
>> +/*
>> + * Get n peaces. If not enough yields. Return on success.
> 
> I’d prefer “Allocate an amount of $n, and, if necessary, yield until
> that becomes possible.”
> 
>> + */
>> +void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n);
>> +
>> +/*
>> + * Put n peaces. Client must not put more than it gets, still it may put in
>> + * split: for example, get(5) and then put(3), put(2). All peaces must be put
>> + * back before qemu_co_shared_amount_free call.
> 
> I’d prefer “Deallocate/Release an amount of @n.  The total amount
> allocated by a caller does not need to be deallocated/released with a
> single call, but may be split over several calls.  For example, get(4),
> get(3), and then put(5), put(2).”
> 
> (And drop the qemu_co_shared_amount_free() reference, because it should
> so say there.)
> 
>> + */
>> +void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n);
>> +
>> +
>> +#endif /* QEMU_CO_SHARED_AMOUNT_H */
>> diff --git a/util/qemu-co-shared-amount.c b/util/qemu-co-shared-amount.c
>> new file mode 100644
>> index 0000000000..8855ce5705
>> --- /dev/null
>> +++ b/util/qemu-co-shared-amount.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * Generic shared amount for coroutines
>> + *
>> + * Copyright (c) 2019 Virtuozzo International GmbH
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/coroutine.h"
>> +#include "qemu/co-shared-amount.h"
>> +
>> +struct QemuCoSharedAmount {
>> +    uint64_t total;
>> +    uint64_t taken;
> 
> I’d reverse the “taken” to be “available”.  Then the only purpose of
> “total” would be as part of assertions.
> 
>> +
>> +    CoQueue queue;
>> +};
>> +
>> +QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total)
>> +{
>> +    QemuCoSharedAmount *s = g_new0(QemuCoSharedAmount, 1);
>> +
>> +    s->total = total;
>> +    qemu_co_queue_init(&s->queue);
>> +
>> +    return s;
>> +}
>> +
>> +void qemu_co_shared_amount_free(QemuCoSharedAmount *s)
>> +{
>> +    assert(s->taken == 0);
>> +    g_free(s);
>> +}
>> +
>> +bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n)
>> +{
>> +    if (n < s->total && s->total - n >= s->taken) {
> 
> (This’d become simply “s->available >= n”)
> 
> (And to be honest I have a hard time parsing that second condition.  To
> me, the natural order would appear to be “s->total - s->taken >= n”.  I
> mean, I can see that it matches by rearranging the inequation, but...
> And in this order you could drop the “n < s->total” part because it’s
> guaranteed that s->taken <= s->total.)
> 
>> +        s->taken += n;
>> +        return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n)
>> +{
>> +    assert(n < s->total);
>> +    while (s->total - n < s->taken) {
>> +        qemu_co_queue_wait(&s->queue, NULL);
>> +    }
>> +
>> +    assert(qemu_co_try_get_amount(s, n));
> 
> I’d refrain from putting things that should do something in assertions
> because assert() is not guaranteed to be compiled.
> 
> It is in practice in qemu, but I still don’t like it too much.
> 
> Furthermore, it appears to me that the following would be simpler:
> 
> while (!qemu_co_try_get_amount(s, n)) {
>      qemu_co_queue_wait(&s->queue, NULL);
> }

A lot more nicer, thanks. Thanks for your suggestions, I'll take them!

> 
>> +}
>> +
>> +void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n)
>> +{
>> +    assert(n <= s->taken);
>> +    s->taken -= n;
>> +    qemu_co_queue_restart_all(&s->queue);
> 
> It itches me to ask for a better allocation strategy (like maybe
> smallest-allocation-first), but I suppose I should just scratch myself.
> 

And, then it would be named and customizable "--object allocator,..." to
be used for different things like jobs or block drivers.. But I'd postpone
it for future, my goal is async requests in backup and I'm moving towards it
since 2016 (we have downstream version of it since 2016) :(.. And now, with
separated block-copy and aio-task API, I feel really close.

>> +}
>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>> index 41bf59d127..65ae18993a 100644
>> --- a/util/Makefile.objs
>> +++ b/util/Makefile.objs
>> @@ -37,6 +37,7 @@ util-obj-y += rcu.o
>>   util-obj-$(CONFIG_MEMBARRIER) += sys_membarrier.o
>>   util-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
>>   util-obj-y += qemu-coroutine-sleep.o
>> +util-obj-y += qemu-co-shared-amount.o
>>   util-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
>>   util-obj-y += buffer.o
>>   util-obj-y += timed-average.o
>>
> 
> 


-- 
Best regards,
Vladimir

[PATCH 5/6] block/block-copy: add memory limit

[Vladimir Sementsov-Ogievskiy]

Currently total allocation for parallel requests to block-copy instance
is unlimited. Let's limit it to 128 MiB.

For now block-copy is used only in backup, so actually we limit total
allocation for backup job.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block-copy.h | 3 +++
 block/block-copy.c         | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index e2e135ff1b..bb666e7068 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -16,6 +16,7 @@
 #define BLOCK_COPY_H
 
 #include "block/block.h"
+#include "qemu/co-shared-amount.h"
 
 typedef struct BlockCopyInFlightReq {
     int64_t start_byte;
@@ -69,6 +70,8 @@ typedef struct BlockCopyState {
      */
     ProgressResetCallbackFunc progress_reset_callback;
     void *progress_opaque;
+
+    QemuCoSharedAmount *mem;
 } BlockCopyState;
 
 BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
diff --git a/block/block-copy.c b/block/block-copy.c
index cc49d2345d..e700c20d0f 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -21,6 +21,7 @@
 #include "qemu/units.h"
 
 #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
+#define BLOCK_COPY_MAX_MEM (128 * MiB)
 
 static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
                                                        int64_t start,
@@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
     }
 
     bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
+    qemu_co_shared_amount_free(s->mem);
     g_free(s);
 }
 
@@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
         .cluster_size = cluster_size,
         .len = bdrv_dirty_bitmap_size(copy_bitmap),
         .write_flags = write_flags,
+        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
     };
 
     s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
@@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
 
         bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
 
+        qemu_co_get_amount(s->mem, chunk_end - start);
         ret = block_copy_do_copy(s, start, chunk_end, error_is_read);
+        qemu_co_put_amount(s->mem, chunk_end - start);
         if (ret < 0) {
             bdrv_set_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
             break;
-- 
2.21.0

Re: [PATCH 5/6] block/block-copy: add memory limit

[Max Reitz]

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

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> Currently total allocation for parallel requests to block-copy instance
> is unlimited. Let's limit it to 128 MiB.
> 
> For now block-copy is used only in backup, so actually we limit total
> allocation for backup job.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block-copy.h | 3 +++
>  block/block-copy.c         | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index e2e135ff1b..bb666e7068 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -16,6 +16,7 @@
>  #define BLOCK_COPY_H
>  
>  #include "block/block.h"
> +#include "qemu/co-shared-amount.h"
>  
>  typedef struct BlockCopyInFlightReq {
>      int64_t start_byte;
> @@ -69,6 +70,8 @@ typedef struct BlockCopyState {
>       */
>      ProgressResetCallbackFunc progress_reset_callback;
>      void *progress_opaque;
> +
> +    QemuCoSharedAmount *mem;
>  } BlockCopyState;
>  
>  BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
> diff --git a/block/block-copy.c b/block/block-copy.c
> index cc49d2345d..e700c20d0f 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -21,6 +21,7 @@
>  #include "qemu/units.h"
>  
>  #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
> +#define BLOCK_COPY_MAX_MEM (128 * MiB)
>  
>  static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>                                                         int64_t start,
> @@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
>      }
>  
>      bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
> +    qemu_co_shared_amount_free(s->mem);
>      g_free(s);
>  }
>  
> @@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>          .cluster_size = cluster_size,
>          .len = bdrv_dirty_bitmap_size(copy_bitmap),
>          .write_flags = write_flags,
> +        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>      };
>  
>      s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
> @@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
>  
>          bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>  
> +        qemu_co_get_amount(s->mem, chunk_end - start);

Now that I see it like this, maybe the name is too short.  This sounds
like it was trying to get some amount of coroutines.

Would “qemu_co_get_from_shared_amount” be too long?  (Something like
qemu_co_sham_alloc() would be funny, but maybe not.  :-)  Or maybe
exactly because it”s funny.)

Max

>          ret = block_copy_do_copy(s, start, chunk_end, error_is_read);
> +        qemu_co_put_amount(s->mem, chunk_end - start);
>          if (ret < 0) {
>              bdrv_set_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>              break;
> 



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

Re: [PATCH 5/6] block/block-copy: add memory limit

[Vladimir Sementsov-Ogievskiy]

07.10.2019 18:27, Max Reitz wrote:
> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>> Currently total allocation for parallel requests to block-copy instance
>> is unlimited. Let's limit it to 128 MiB.
>>
>> For now block-copy is used only in backup, so actually we limit total
>> allocation for backup job.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block-copy.h | 3 +++
>>   block/block-copy.c         | 5 +++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>> index e2e135ff1b..bb666e7068 100644
>> --- a/include/block/block-copy.h
>> +++ b/include/block/block-copy.h
>> @@ -16,6 +16,7 @@
>>   #define BLOCK_COPY_H
>>   
>>   #include "block/block.h"
>> +#include "qemu/co-shared-amount.h"
>>   
>>   typedef struct BlockCopyInFlightReq {
>>       int64_t start_byte;
>> @@ -69,6 +70,8 @@ typedef struct BlockCopyState {
>>        */
>>       ProgressResetCallbackFunc progress_reset_callback;
>>       void *progress_opaque;
>> +
>> +    QemuCoSharedAmount *mem;
>>   } BlockCopyState;
>>   
>>   BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index cc49d2345d..e700c20d0f 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
>> @@ -21,6 +21,7 @@
>>   #include "qemu/units.h"
>>   
>>   #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
>> +#define BLOCK_COPY_MAX_MEM (128 * MiB)
>>   
>>   static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>>                                                          int64_t start,
>> @@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
>>       }
>>   
>>       bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
>> +    qemu_co_shared_amount_free(s->mem);
>>       g_free(s);
>>   }
>>   
>> @@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>           .cluster_size = cluster_size,
>>           .len = bdrv_dirty_bitmap_size(copy_bitmap),
>>           .write_flags = write_flags,
>> +        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>       };
>>   
>>       s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>> @@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
>>   
>>           bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>   
>> +        qemu_co_get_amount(s->mem, chunk_end - start);
> 
> Now that I see it like this, maybe the name is too short.  This sounds
> like it was trying to get some amount of coroutines.
> 
> Would “qemu_co_get_from_shared_amount” be too long?  (Something like
> qemu_co_sham_alloc() would be funny, but maybe not.  :-)  Or maybe
> exactly because it”s funny.)
> 

hmm sham may be interpreted as shared memory, not only like shame..

And if we call it _alloc, the opposite should be _free, but how to
distinguish it from freeing the whole object? Hmm, use create/destroy for
the whole object maybe.

May be, drop "qemu_" ? It's not very informative. Or may be drop "co_"?.

I don't like shaming my shared amount :)

May be, we should imagine, what are we allocating? May be balls?

struct BallAllocator

ball_allocator_create
ball_allocator_destroy

co_try_alloc_balls
co_alloc_balls
co_free_balls

Or bars? Or which thing may be used for funny naming and to not intersect
with existing concepts like memory?

> 
>>           ret = block_copy_do_copy(s, start, chunk_end, error_is_read);
>> +        qemu_co_put_amount(s->mem, chunk_end - start);
>>           if (ret < 0) {
>>               bdrv_set_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>               break;
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 5/6] block/block-copy: add memory limit

[Max Reitz]

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

On 07.10.19 19:10, Vladimir Sementsov-Ogievskiy wrote:
> 07.10.2019 18:27, Max Reitz wrote:
>> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>>> Currently total allocation for parallel requests to block-copy instance
>>> is unlimited. Let's limit it to 128 MiB.
>>>
>>> For now block-copy is used only in backup, so actually we limit total
>>> allocation for backup job.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/block-copy.h | 3 +++
>>>   block/block-copy.c         | 5 +++++
>>>   2 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>>> index e2e135ff1b..bb666e7068 100644
>>> --- a/include/block/block-copy.h
>>> +++ b/include/block/block-copy.h
>>> @@ -16,6 +16,7 @@
>>>   #define BLOCK_COPY_H
>>>   
>>>   #include "block/block.h"
>>> +#include "qemu/co-shared-amount.h"
>>>   
>>>   typedef struct BlockCopyInFlightReq {
>>>       int64_t start_byte;
>>> @@ -69,6 +70,8 @@ typedef struct BlockCopyState {
>>>        */
>>>       ProgressResetCallbackFunc progress_reset_callback;
>>>       void *progress_opaque;
>>> +
>>> +    QemuCoSharedAmount *mem;
>>>   } BlockCopyState;
>>>   
>>>   BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>> index cc49d2345d..e700c20d0f 100644
>>> --- a/block/block-copy.c
>>> +++ b/block/block-copy.c
>>> @@ -21,6 +21,7 @@
>>>   #include "qemu/units.h"
>>>   
>>>   #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
>>> +#define BLOCK_COPY_MAX_MEM (128 * MiB)
>>>   
>>>   static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>>>                                                          int64_t start,
>>> @@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
>>>       }
>>>   
>>>       bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
>>> +    qemu_co_shared_amount_free(s->mem);
>>>       g_free(s);
>>>   }
>>>   
>>> @@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>           .cluster_size = cluster_size,
>>>           .len = bdrv_dirty_bitmap_size(copy_bitmap),
>>>           .write_flags = write_flags,
>>> +        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>>       };
>>>   
>>>       s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>>> @@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
>>>   
>>>           bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>>   
>>> +        qemu_co_get_amount(s->mem, chunk_end - start);
>>
>> Now that I see it like this, maybe the name is too short.  This sounds
>> like it was trying to get some amount of coroutines.
>>
>> Would “qemu_co_get_from_shared_amount” be too long?  (Something like
>> qemu_co_sham_alloc() would be funny, but maybe not.  :-)  Or maybe
>> exactly because it”s funny.)
>>
> 
> hmm sham may be interpreted as shared memory, not only like shame..

“sham” is also a word by itself. :-)

> And if we call it _alloc, the opposite should be _free, but how to
> distinguish it from freeing the whole object? Hmm, use create/destroy for
> the whole object maybe.
> 
> May be, drop "qemu_" ? It's not very informative. Or may be drop "co_"?.
> 
> I don't like shaming my shared amount :)

It’s worse calling it all a sham.

> May be, we should imagine, what are we allocating? May be balls?
> 
> struct BallAllocator
> 
> ball_allocator_create
> ball_allocator_destroy
> 
> co_try_alloc_balls
> co_alloc_balls
> co_free_balls
> 
> Or bars? Or which thing may be used for funny naming and to not intersect
> with existing concepts like memory?

I love it (thanks for making my morning), but I fear it may be
interpreted as risqué.

Maybe just shres for shared resource?  So alloc_from_shres?

Max

>>
>>>           ret = block_copy_do_copy(s, start, chunk_end, error_is_read);
>>> +        qemu_co_put_amount(s->mem, chunk_end - start);
>>>           if (ret < 0) {
>>>               bdrv_set_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>>               break;
>>>
>>
>>
> 
> 



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

Re: [PATCH 5/6] block/block-copy: add memory limit

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:03, Max Reitz wrote:
> On 07.10.19 19:10, Vladimir Sementsov-Ogievskiy wrote:
>> 07.10.2019 18:27, Max Reitz wrote:
>>> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>>>> Currently total allocation for parallel requests to block-copy instance
>>>> is unlimited. Let's limit it to 128 MiB.
>>>>
>>>> For now block-copy is used only in backup, so actually we limit total
>>>> allocation for backup job.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    include/block/block-copy.h | 3 +++
>>>>    block/block-copy.c         | 5 +++++
>>>>    2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>>>> index e2e135ff1b..bb666e7068 100644
>>>> --- a/include/block/block-copy.h
>>>> +++ b/include/block/block-copy.h
>>>> @@ -16,6 +16,7 @@
>>>>    #define BLOCK_COPY_H
>>>>    
>>>>    #include "block/block.h"
>>>> +#include "qemu/co-shared-amount.h"
>>>>    
>>>>    typedef struct BlockCopyInFlightReq {
>>>>        int64_t start_byte;
>>>> @@ -69,6 +70,8 @@ typedef struct BlockCopyState {
>>>>         */
>>>>        ProgressResetCallbackFunc progress_reset_callback;
>>>>        void *progress_opaque;
>>>> +
>>>> +    QemuCoSharedAmount *mem;
>>>>    } BlockCopyState;
>>>>    
>>>>    BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>> index cc49d2345d..e700c20d0f 100644
>>>> --- a/block/block-copy.c
>>>> +++ b/block/block-copy.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include "qemu/units.h"
>>>>    
>>>>    #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
>>>> +#define BLOCK_COPY_MAX_MEM (128 * MiB)
>>>>    
>>>>    static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>>>>                                                           int64_t start,
>>>> @@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
>>>>        }
>>>>    
>>>>        bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
>>>> +    qemu_co_shared_amount_free(s->mem);
>>>>        g_free(s);
>>>>    }
>>>>    
>>>> @@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>>            .cluster_size = cluster_size,
>>>>            .len = bdrv_dirty_bitmap_size(copy_bitmap),
>>>>            .write_flags = write_flags,
>>>> +        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>>>        };
>>>>    
>>>>        s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>>>> @@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
>>>>    
>>>>            bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>>>    
>>>> +        qemu_co_get_amount(s->mem, chunk_end - start);
>>>
>>> Now that I see it like this, maybe the name is too short.  This sounds
>>> like it was trying to get some amount of coroutines.
>>>
>>> Would “qemu_co_get_from_shared_amount” be too long?  (Something like
>>> qemu_co_sham_alloc() would be funny, but maybe not.  :-)  Or maybe
>>> exactly because it”s funny.)
>>>
>>
>> hmm sham may be interpreted as shared memory, not only like shame..
> 
> “sham” is also a word by itself. :-)

Hmm didn't know, me go to google translate. OK, sham looks a lot nicer than shame)

> 
>> And if we call it _alloc, the opposite should be _free, but how to
>> distinguish it from freeing the whole object? Hmm, use create/destroy for
>> the whole object maybe.
>>
>> May be, drop "qemu_" ? It's not very informative. Or may be drop "co_"?.
>>
>> I don't like shaming my shared amount :)
> 
> It’s worse calling it all a sham.
> 
>> May be, we should imagine, what are we allocating? May be balls?
>>
>> struct BallAllocator
>>
>> ball_allocator_create
>> ball_allocator_destroy
>>
>> co_try_alloc_balls
>> co_alloc_balls
>> co_free_balls
>>
>> Or bars? Or which thing may be used for funny naming and to not intersect
>> with existing concepts like memory?
> 
> I love it (thanks for making my morning), but I fear it may be
> interpreted as risqué.
> 
> Maybe just shres for shared resource?  So alloc_from_shres?
> 

OK for me. But.. How to name _free function than?

struct SharedResource

shres_create
shres_destroy

co_try_alloc_from_shres
co_alloc_from_shres
co_free_???

co_free_res_alloced_from_shres ? :)

or

co_try_get_from_shres
co_get_from_shres
co_put_to_shres

>>>
>>>>            ret = block_copy_do_copy(s, start, chunk_end, error_is_read);
>>>> +        qemu_co_put_amount(s->mem, chunk_end - start);
>>>>            if (ret < 0) {
>>>>                bdrv_set_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>>>                break;
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 5/6] block/block-copy: add memory limit

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:15, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:03, Max Reitz wrote:
>> On 07.10.19 19:10, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.10.2019 18:27, Max Reitz wrote:
>>>> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Currently total allocation for parallel requests to block-copy instance
>>>>> is unlimited. Let's limit it to 128 MiB.
>>>>>
>>>>> For now block-copy is used only in backup, so actually we limit total
>>>>> allocation for backup job.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    include/block/block-copy.h | 3 +++
>>>>>    block/block-copy.c         | 5 +++++
>>>>>    2 files changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>>>>> index e2e135ff1b..bb666e7068 100644
>>>>> --- a/include/block/block-copy.h
>>>>> +++ b/include/block/block-copy.h
>>>>> @@ -16,6 +16,7 @@
>>>>>    #define BLOCK_COPY_H
>>>>>    #include "block/block.h"
>>>>> +#include "qemu/co-shared-amount.h"
>>>>>    typedef struct BlockCopyInFlightReq {
>>>>>        int64_t start_byte;
>>>>> @@ -69,6 +70,8 @@ typedef struct BlockCopyState {
>>>>>         */
>>>>>        ProgressResetCallbackFunc progress_reset_callback;
>>>>>        void *progress_opaque;
>>>>> +
>>>>> +    QemuCoSharedAmount *mem;
>>>>>    } BlockCopyState;
>>>>>    BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>>> index cc49d2345d..e700c20d0f 100644
>>>>> --- a/block/block-copy.c
>>>>> +++ b/block/block-copy.c
>>>>> @@ -21,6 +21,7 @@
>>>>>    #include "qemu/units.h"
>>>>>    #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
>>>>> +#define BLOCK_COPY_MAX_MEM (128 * MiB)
>>>>>    static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>>>>>                                                           int64_t start,
>>>>> @@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
>>>>>        }
>>>>>        bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
>>>>> +    qemu_co_shared_amount_free(s->mem);
>>>>>        g_free(s);
>>>>>    }
>>>>> @@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>>>            .cluster_size = cluster_size,
>>>>>            .len = bdrv_dirty_bitmap_size(copy_bitmap),
>>>>>            .write_flags = write_flags,
>>>>> +        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>>>>        };
>>>>>        s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>>>>> @@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
>>>>>            bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>>>> +        qemu_co_get_amount(s->mem, chunk_end - start);
>>>>
>>>> Now that I see it like this, maybe the name is too short.  This sounds
>>>> like it was trying to get some amount of coroutines.
>>>>
>>>> Would “qemu_co_get_from_shared_amount” be too long?  (Something like
>>>> qemu_co_sham_alloc() would be funny, but maybe not.  :-)  Or maybe
>>>> exactly because it”s funny.)
>>>>
>>>
>>> hmm sham may be interpreted as shared memory, not only like shame..
>>
>> “sham” is also a word by itself. :-)
> 
> Hmm didn't know, me go to google translate. OK, sham looks a lot nicer than shame)
> 
>>
>>> And if we call it _alloc, the opposite should be _free, but how to
>>> distinguish it from freeing the whole object? Hmm, use create/destroy for
>>> the whole object maybe.
>>>
>>> May be, drop "qemu_" ? It's not very informative. Or may be drop "co_"?.
>>>
>>> I don't like shaming my shared amount :)
>>
>> It’s worse calling it all a sham.
>>
>>> May be, we should imagine, what are we allocating? May be balls?
>>>
>>> struct BallAllocator
>>>
>>> ball_allocator_create
>>> ball_allocator_destroy
>>>
>>> co_try_alloc_balls
>>> co_alloc_balls
>>> co_free_balls
>>>
>>> Or bars? Or which thing may be used for funny naming and to not intersect
>>> with existing concepts like memory?
>>
>> I love it (thanks for making my morning), but I fear it may be
>> interpreted as risqué.
>>
>> Maybe just shres for shared resource?  So alloc_from_shres?
>>
> 
> OK for me. But.. How to name _free function than?
> 
> struct SharedResource
> 
> shres_create
> shres_destroy
> 
> co_try_alloc_from_shres
> co_alloc_from_shres
> co_free_???
> 
> co_free_res_alloced_from_shres ? :)
> 
> or
> 
> co_try_get_from_shres
> co_get_from_shres
> co_put_to_shres
> 


Another proposal from Roma: use "budget" word.


-- 
Best regards,
Vladimir

Re: [PATCH 5/6] block/block-copy: add memory limit

[Max Reitz]

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

On 08.10.19 11:20, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:15, Vladimir Sementsov-Ogievskiy wrote:
>> 08.10.2019 12:03, Max Reitz wrote:
>>> On 07.10.19 19:10, Vladimir Sementsov-Ogievskiy wrote:
>>>> 07.10.2019 18:27, Max Reitz wrote:
>>>>> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Currently total allocation for parallel requests to block-copy instance
>>>>>> is unlimited. Let's limit it to 128 MiB.
>>>>>>
>>>>>> For now block-copy is used only in backup, so actually we limit total
>>>>>> allocation for backup job.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>    include/block/block-copy.h | 3 +++
>>>>>>    block/block-copy.c         | 5 +++++
>>>>>>    2 files changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>>>>>> index e2e135ff1b..bb666e7068 100644
>>>>>> --- a/include/block/block-copy.h
>>>>>> +++ b/include/block/block-copy.h
>>>>>> @@ -16,6 +16,7 @@
>>>>>>    #define BLOCK_COPY_H
>>>>>>    #include "block/block.h"
>>>>>> +#include "qemu/co-shared-amount.h"
>>>>>>    typedef struct BlockCopyInFlightReq {
>>>>>>        int64_t start_byte;
>>>>>> @@ -69,6 +70,8 @@ typedef struct BlockCopyState {
>>>>>>         */
>>>>>>        ProgressResetCallbackFunc progress_reset_callback;
>>>>>>        void *progress_opaque;
>>>>>> +
>>>>>> +    QemuCoSharedAmount *mem;
>>>>>>    } BlockCopyState;
>>>>>>    BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>>>> index cc49d2345d..e700c20d0f 100644
>>>>>> --- a/block/block-copy.c
>>>>>> +++ b/block/block-copy.c
>>>>>> @@ -21,6 +21,7 @@
>>>>>>    #include "qemu/units.h"
>>>>>>    #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
>>>>>> +#define BLOCK_COPY_MAX_MEM (128 * MiB)
>>>>>>    static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>>>>>>                                                           int64_t start,
>>>>>> @@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
>>>>>>        }
>>>>>>        bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
>>>>>> +    qemu_co_shared_amount_free(s->mem);
>>>>>>        g_free(s);
>>>>>>    }
>>>>>> @@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>>>>            .cluster_size = cluster_size,
>>>>>>            .len = bdrv_dirty_bitmap_size(copy_bitmap),
>>>>>>            .write_flags = write_flags,
>>>>>> +        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>>>>>        };
>>>>>>        s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>>>>>> @@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
>>>>>>            bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>>>>> +        qemu_co_get_amount(s->mem, chunk_end - start);
>>>>>
>>>>> Now that I see it like this, maybe the name is too short.  This sounds
>>>>> like it was trying to get some amount of coroutines.
>>>>>
>>>>> Would “qemu_co_get_from_shared_amount” be too long?  (Something like
>>>>> qemu_co_sham_alloc() would be funny, but maybe not.  :-)  Or maybe
>>>>> exactly because it”s funny.)
>>>>>
>>>>
>>>> hmm sham may be interpreted as shared memory, not only like shame..
>>>
>>> “sham” is also a word by itself. :-)
>>
>> Hmm didn't know, me go to google translate. OK, sham looks a lot nicer than shame)
>>
>>>
>>>> And if we call it _alloc, the opposite should be _free, but how to
>>>> distinguish it from freeing the whole object? Hmm, use create/destroy for
>>>> the whole object maybe.
>>>>
>>>> May be, drop "qemu_" ? It's not very informative. Or may be drop "co_"?.
>>>>
>>>> I don't like shaming my shared amount :)
>>>
>>> It’s worse calling it all a sham.
>>>
>>>> May be, we should imagine, what are we allocating? May be balls?
>>>>
>>>> struct BallAllocator
>>>>
>>>> ball_allocator_create
>>>> ball_allocator_destroy
>>>>
>>>> co_try_alloc_balls
>>>> co_alloc_balls
>>>> co_free_balls
>>>>
>>>> Or bars? Or which thing may be used for funny naming and to not intersect
>>>> with existing concepts like memory?
>>>
>>> I love it (thanks for making my morning), but I fear it may be
>>> interpreted as risqué.
>>>
>>> Maybe just shres for shared resource?  So alloc_from_shres?
>>>
>>
>> OK for me. But.. How to name _free function than?
>>
>> struct SharedResource
>>
>> shres_create
>> shres_destroy
>>
>> co_try_alloc_from_shres
>> co_alloc_from_shres
>> co_free_???
>>
>> co_free_res_alloced_from_shres ? :)
>>
>> or
>>
>> co_try_get_from_shres
>> co_get_from_shres
>> co_put_to_shres
>>
> 
> 
> Another proposal from Roma: use "budget" word.

Instead of shres?  Why not.

Max


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

Re: [PATCH 5/6] block/block-copy: add memory limit

[Max Reitz]

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

On 08.10.19 11:15, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:03, Max Reitz wrote:
>> On 07.10.19 19:10, Vladimir Sementsov-Ogievskiy wrote:
>>> 07.10.2019 18:27, Max Reitz wrote:
>>>> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Currently total allocation for parallel requests to block-copy instance
>>>>> is unlimited. Let's limit it to 128 MiB.
>>>>>
>>>>> For now block-copy is used only in backup, so actually we limit total
>>>>> allocation for backup job.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    include/block/block-copy.h | 3 +++
>>>>>    block/block-copy.c         | 5 +++++
>>>>>    2 files changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>>>>> index e2e135ff1b..bb666e7068 100644
>>>>> --- a/include/block/block-copy.h
>>>>> +++ b/include/block/block-copy.h
>>>>> @@ -16,6 +16,7 @@
>>>>>    #define BLOCK_COPY_H
>>>>>    
>>>>>    #include "block/block.h"
>>>>> +#include "qemu/co-shared-amount.h"
>>>>>    
>>>>>    typedef struct BlockCopyInFlightReq {
>>>>>        int64_t start_byte;
>>>>> @@ -69,6 +70,8 @@ typedef struct BlockCopyState {
>>>>>         */
>>>>>        ProgressResetCallbackFunc progress_reset_callback;
>>>>>        void *progress_opaque;
>>>>> +
>>>>> +    QemuCoSharedAmount *mem;
>>>>>    } BlockCopyState;
>>>>>    
>>>>>    BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>>> index cc49d2345d..e700c20d0f 100644
>>>>> --- a/block/block-copy.c
>>>>> +++ b/block/block-copy.c
>>>>> @@ -21,6 +21,7 @@
>>>>>    #include "qemu/units.h"
>>>>>    
>>>>>    #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
>>>>> +#define BLOCK_COPY_MAX_MEM (128 * MiB)
>>>>>    
>>>>>    static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
>>>>>                                                           int64_t start,
>>>>> @@ -64,6 +65,7 @@ void block_copy_state_free(BlockCopyState *s)
>>>>>        }
>>>>>    
>>>>>        bdrv_release_dirty_bitmap(s->source->bs, s->copy_bitmap);
>>>>> +    qemu_co_shared_amount_free(s->mem);
>>>>>        g_free(s);
>>>>>    }
>>>>>    
>>>>> @@ -95,6 +97,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>>>>            .cluster_size = cluster_size,
>>>>>            .len = bdrv_dirty_bitmap_size(copy_bitmap),
>>>>>            .write_flags = write_flags,
>>>>> +        .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>>>>        };
>>>>>    
>>>>>        s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>>>>> @@ -316,7 +319,9 @@ int coroutine_fn block_copy(BlockCopyState *s,
>>>>>    
>>>>>            bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>>>>>    
>>>>> +        qemu_co_get_amount(s->mem, chunk_end - start);
>>>>
>>>> Now that I see it like this, maybe the name is too short.  This sounds
>>>> like it was trying to get some amount of coroutines.
>>>>
>>>> Would “qemu_co_get_from_shared_amount” be too long?  (Something like
>>>> qemu_co_sham_alloc() would be funny, but maybe not.  :-)  Or maybe
>>>> exactly because it”s funny.)
>>>>
>>>
>>> hmm sham may be interpreted as shared memory, not only like shame..
>>
>> “sham” is also a word by itself. :-)
> 
> Hmm didn't know, me go to google translate. OK, sham looks a lot nicer than shame)
> 
>>
>>> And if we call it _alloc, the opposite should be _free, but how to
>>> distinguish it from freeing the whole object? Hmm, use create/destroy for
>>> the whole object maybe.
>>>
>>> May be, drop "qemu_" ? It's not very informative. Or may be drop "co_"?.
>>>
>>> I don't like shaming my shared amount :)
>>
>> It’s worse calling it all a sham.
>>
>>> May be, we should imagine, what are we allocating? May be balls?
>>>
>>> struct BallAllocator
>>>
>>> ball_allocator_create
>>> ball_allocator_destroy
>>>
>>> co_try_alloc_balls
>>> co_alloc_balls
>>> co_free_balls
>>>
>>> Or bars? Or which thing may be used for funny naming and to not intersect
>>> with existing concepts like memory?
>>
>> I love it (thanks for making my morning), but I fear it may be
>> interpreted as risqué.
>>
>> Maybe just shres for shared resource?  So alloc_from_shres?
>>
> 
> OK for me. But.. How to name _free function than?
> 
> struct SharedResource
> 
> shres_create
> shres_destroy
> 
> co_try_alloc_from_shres
> co_alloc_from_shres
> co_free_???
> 
> co_free_res_alloced_from_shres ? :)
> 
> or
> 
> co_try_get_from_shres
> co_get_from_shres
> co_put_to_shres

Sounds good to me.

Max


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

[PATCH 6/6] block/block-copy: increase buffered copy request

[Vladimir Sementsov-Ogievskiy]

No reason to limit buffered copy to one cluster. Let's allow up to 1
MiB.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block-copy.h |  2 +-
 block/block-copy.c         | 44 +++++++++++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index bb666e7068..6a6f5ce73a 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -38,7 +38,7 @@ typedef struct BlockCopyState {
     BdrvDirtyBitmap *copy_bitmap;
     int64_t cluster_size;
     bool use_copy_range;
-    int64_t copy_range_size;
+    int64_t copy_size;
     uint64_t len;
     QLIST_HEAD(, BlockCopyInFlightReq) inflight_reqs;
 
diff --git a/block/block-copy.c b/block/block-copy.c
index e700c20d0f..12cb540494 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -21,6 +21,7 @@
 #include "qemu/units.h"
 
 #define BLOCK_COPY_MAX_COPY_RANGE (16 * MiB)
+#define BLOCK_COPY_MAX_BUFFER (1 * MiB)
 #define BLOCK_COPY_MAX_MEM (128 * MiB)
 
 static void coroutine_fn block_copy_wait_inflight_reqs(BlockCopyState *s,
@@ -100,17 +101,28 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
         .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
     };
 
-    s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
-    /*
-     * Set use_copy_range, consider the following:
-     * 1. Compression is not supported for copy_range.
-     * 2. copy_range does not respect max_transfer (it's a TODO), so we factor
-     *    that in here. If max_transfer is smaller than the job->cluster_size,
-     *    we do not use copy_range (in that case it's zero after aligning down
-     *    above).
-     */
-    s->use_copy_range =
-        !(write_flags & BDRV_REQ_WRITE_COMPRESSED) && s->copy_range_size > 0;
+    if (max_transfer < cluster_size) {
+        /*
+         * copy_range does not respect max_transfer. We don't want to bother
+         * with requests smaller than block-copy cluster size, so fallback to
+         * buffered copying (read and write respect max_transfer on their
+         * behalf).
+         */
+        s->use_copy_range = false;
+        s->copy_size = cluster_size;
+    } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
+        /* Compression is not supported for copy_range */
+        s->use_copy_range = false;
+        s->copy_size = MAX(cluster_size, BLOCK_COPY_MAX_BUFFER);
+    } else {
+        /*
+         * copy_range does not respect max_transfer (it's a TODO), so we factor
+         * that in here.
+         */
+        s->use_copy_range = true;
+        s->copy_size = MIN(MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
+                           QEMU_ALIGN_DOWN(max_transfer, cluster_size));
+    }
 
     QLIST_INIT(&s->inflight_reqs);
 
@@ -156,11 +168,18 @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
         if (ret < 0) {
             trace_block_copy_copy_range_fail(s, start, ret);
             s->use_copy_range = false;
+            s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
         } else {
             return ret;
         }
     }
 
+    /*
+     * In case of failed copy_range request above, we may proceed with buffered
+     * request larger than BLOCK_COPY_MAX_BUFFER. Still, further requests will
+     * be properly limited, so don't care too much.
+     */
+
     bounce_buffer = qemu_blockalign(s->source->bs, nbytes);
 
     ret = bdrv_co_pread(s->source, start, nbytes, bounce_buffer, 0);
@@ -293,8 +312,7 @@ int coroutine_fn block_copy(BlockCopyState *s,
             continue; /* already copied */
         }
 
-        chunk_end = MIN(end, start + (s->use_copy_range ?
-                                      s->copy_range_size : s->cluster_size));
+        chunk_end = MIN(end, start + s->copy_size);
 
         next_zero = bdrv_dirty_bitmap_next_zero(s->copy_bitmap, start,
                                                 chunk_end - start);
-- 
2.21.0

Re: [PATCH 6/6] block/block-copy: increase buffered copy request

[Max Reitz]

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

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> No reason to limit buffered copy to one cluster. Let's allow up to 1
> MiB.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block-copy.h |  2 +-
>  block/block-copy.c         | 44 +++++++++++++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 14 deletions(-)

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


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

Re: [PATCH 6/6] block/block-copy: increase buffered copy request

[Max Reitz]

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

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> No reason to limit buffered copy to one cluster. Let's allow up to 1
> MiB.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block-copy.h |  2 +-
>  block/block-copy.c         | 44 +++++++++++++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 14 deletions(-)


Er, oops, looks like I was a bit quick there...

> @@ -100,17 +101,28 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>          .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>      };
>  
> -    s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
> -    /*
> -     * Set use_copy_range, consider the following:
> -     * 1. Compression is not supported for copy_range.
> -     * 2. copy_range does not respect max_transfer (it's a TODO), so we factor
> -     *    that in here. If max_transfer is smaller than the job->cluster_size,
> -     *    we do not use copy_range (in that case it's zero after aligning down
> -     *    above).
> -     */
> -    s->use_copy_range =
> -        !(write_flags & BDRV_REQ_WRITE_COMPRESSED) && s->copy_range_size > 0;
> +    if (max_transfer < cluster_size) {
> +        /*
> +         * copy_range does not respect max_transfer. We don't want to bother
> +         * with requests smaller than block-copy cluster size, so fallback to
> +         * buffered copying (read and write respect max_transfer on their
> +         * behalf).
> +         */
> +        s->use_copy_range = false;
> +        s->copy_size = cluster_size;
> +    } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
> +        /* Compression is not supported for copy_range */
> +        s->use_copy_range = false;
> +        s->copy_size = MAX(cluster_size, BLOCK_COPY_MAX_BUFFER);
> +    } else {
> +        /*
> +         * copy_range does not respect max_transfer (it's a TODO), so we factor
> +         * that in here.
> +         */
> +        s->use_copy_range = true;
> +        s->copy_size = MIN(MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE),

This is already part of max_transfer, isn’t it?

(That doesn’t make it wrong, but I think max_transfer will always be
less than or equal to MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE) anyway.)

Max

> +                           QEMU_ALIGN_DOWN(max_transfer, cluster_size));


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

Re: [PATCH 6/6] block/block-copy: increase buffered copy request

[Vladimir Sementsov-Ogievskiy]

07.10.2019 18:48, Max Reitz wrote:
> On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
>> No reason to limit buffered copy to one cluster. Let's allow up to 1
>> MiB.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block-copy.h |  2 +-
>>   block/block-copy.c         | 44 +++++++++++++++++++++++++++-----------
>>   2 files changed, 32 insertions(+), 14 deletions(-)
> 
> 
> Er, oops, looks like I was a bit quick there...
> 
>> @@ -100,17 +101,28 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>           .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>>       };
>>   
>> -    s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
>> -    /*
>> -     * Set use_copy_range, consider the following:
>> -     * 1. Compression is not supported for copy_range.
>> -     * 2. copy_range does not respect max_transfer (it's a TODO), so we factor
>> -     *    that in here. If max_transfer is smaller than the job->cluster_size,
>> -     *    we do not use copy_range (in that case it's zero after aligning down
>> -     *    above).
>> -     */
>> -    s->use_copy_range =
>> -        !(write_flags & BDRV_REQ_WRITE_COMPRESSED) && s->copy_range_size > 0;
>> +    if (max_transfer < cluster_size) {
>> +        /*
>> +         * copy_range does not respect max_transfer. We don't want to bother
>> +         * with requests smaller than block-copy cluster size, so fallback to
>> +         * buffered copying (read and write respect max_transfer on their
>> +         * behalf).
>> +         */
>> +        s->use_copy_range = false;
>> +        s->copy_size = cluster_size;
>> +    } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
>> +        /* Compression is not supported for copy_range */
>> +        s->use_copy_range = false;
>> +        s->copy_size = MAX(cluster_size, BLOCK_COPY_MAX_BUFFER);
>> +    } else {
>> +        /*
>> +         * copy_range does not respect max_transfer (it's a TODO), so we factor
>> +         * that in here.
>> +         */
>> +        s->use_copy_range = true;
>> +        s->copy_size = MIN(MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE),
> 
> This is already part of max_transfer, isn’t it?
> 
> (That doesn’t make it wrong, but I think max_transfer will always be
> less than or equal to MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE) anyway.)
> 
> Max

Hmm, oops, yes. It'd better be just QEMU_ALIGN_DOWN(max_transfer, cluster_size)

> 
>> +                           QEMU_ALIGN_DOWN(max_transfer, cluster_size));
> 


-- 
Best regards,
Vladimir