[PATCH v3 0/5] block-copy: make helper APIs thread safe

[PATCH v3 0/5] block-copy: make helper APIs thread safe

[Emanuele Giuseppe Esposito]

This serie of patches bring thread safety to the smaller APIs used by
block-copy, namely ratelimit, progressmeter, co-shared-resource
and aiotask.
The end goal is to reduce the usage of AioContexlock in block-copy,
by introducing smaller granularity locks thus on making the block layer
thread safe. 

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it and will post the patches once his new
CoSleep API is accepted.

Patches 1-3 work on ratelimit, 4 covers progressmeter and
5 co-shared-resources.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v3:
* Rebase on current master (had conflicts in block-copy), remove based-on in
  cover letter

Emanuele Giuseppe Esposito (2):
  progressmeter: protect with a mutex
  co-shared-resource: protect with a mutex

Paolo Bonzini (3):
  ratelimit: treat zero speed as unlimited
  block-copy: let ratelimit handle a speed of 0
  blockjob: let ratelimit handle a speed of 0

 block/block-copy.c                | 28 ++++++--------
 block/meson.build                 |  1 +
 block/progress_meter.c            | 64 +++++++++++++++++++++++++++++++
 blockjob.c                        | 46 +++++++++++++---------
 include/qemu/co-shared-resource.h |  4 +-
 include/qemu/progress_meter.h     | 34 ++++++++--------
 include/qemu/ratelimit.h          | 12 +++++-
 job-qmp.c                         |  8 +++-
 job.c                             |  3 ++
 qemu-img.c                        |  9 +++--
 util/qemu-co-shared-resource.c    | 24 +++++++++---
 11 files changed, 168 insertions(+), 65 deletions(-)
 create mode 100644 block/progress_meter.c

-- 
2.31.1

[PATCH v3 1/5] ratelimit: treat zero speed as unlimited

[Emanuele Giuseppe Esposito]

From: Paolo Bonzini <pbonzini@redhat.com>

Both users of RateLimit, block-copy.c and blockjob.c, treat
a speed of zero as unlimited, while RateLimit treats it as
"as slow as possible".  The latter is nicer from the code
point of view but pretty useless, so disable rate limiting
if a speed of zero is provided.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/qemu/ratelimit.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/qemu/ratelimit.h b/include/qemu/ratelimit.h
index 003ea6d5a3..48bf59e857 100644
--- a/include/qemu/ratelimit.h
+++ b/include/qemu/ratelimit.h
@@ -43,7 +43,11 @@ static inline int64_t ratelimit_calculate_delay(RateLimit *limit, uint64_t n)
     double delay_slices;
 
     QEMU_LOCK_GUARD(&limit->lock);
-    assert(limit->slice_quota && limit->slice_ns);
+    if (!limit->slice_quota) {
+        /* Throttling disabled.  */
+        return 0;
+    }
+    assert(limit->slice_ns);
 
     if (limit->slice_end_time < now) {
         /* Previous, possibly extended, time slice finished; reset the
@@ -83,7 +87,11 @@ static inline void ratelimit_set_speed(RateLimit *limit, uint64_t speed,
 {
     QEMU_LOCK_GUARD(&limit->lock);
     limit->slice_ns = slice_ns;
-    limit->slice_quota = MAX(((double)speed * slice_ns) / 1000000000ULL, 1);
+    if (speed == 0) {
+        limit->slice_quota = 0;
+    } else {
+        limit->slice_quota = MAX(((double)speed * slice_ns) / 1000000000ULL, 1);
+    }
 }
 
 #endif
-- 
2.31.1

[PATCH v3 2/5] block-copy: let ratelimit handle a speed of 0

[Emanuele Giuseppe Esposito]

From: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-copy.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 5808cfe657..943e30b7e6 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -114,7 +114,6 @@ typedef struct BlockCopyState {
 
     SharedResource *mem;
 
-    uint64_t speed;
     RateLimit rate_limit;
 } BlockCopyState;
 
@@ -647,21 +646,19 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
             task->copy_range = false;
         }
 
-        if (s->speed) {
-            if (!call_state->ignore_ratelimit) {
-                uint64_t ns = ratelimit_calculate_delay(&s->rate_limit, 0);
-                if (ns > 0) {
-                    block_copy_task_end(task, -EAGAIN);
-                    g_free(task);
-                    qemu_co_sleep_ns_wakeable(&call_state->sleep,
-                                              QEMU_CLOCK_REALTIME, ns);
-                    continue;
-                }
+        if (!call_state->ignore_ratelimit) {
+            uint64_t ns = ratelimit_calculate_delay(&s->rate_limit, 0);
+            if (ns > 0) {
+                block_copy_task_end(task, -EAGAIN);
+                g_free(task);
+                qemu_co_sleep_ns_wakeable(&call_state->sleep,
+                                            QEMU_CLOCK_REALTIME, ns);
+                continue;
             }
-
-            ratelimit_calculate_delay(&s->rate_limit, task->bytes);
         }
 
+        ratelimit_calculate_delay(&s->rate_limit, task->bytes);
+
         trace_block_copy_process(s, task->offset);
 
         co_get_from_shres(s->mem, task->bytes);
@@ -853,10 +850,7 @@ void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip)
 
 void block_copy_set_speed(BlockCopyState *s, uint64_t speed)
 {
-    s->speed = speed;
-    if (speed > 0) {
-        ratelimit_set_speed(&s->rate_limit, speed, BLOCK_COPY_SLICE_TIME);
-    }
+    ratelimit_set_speed(&s->rate_limit, speed, BLOCK_COPY_SLICE_TIME);
 
     /*
      * Note: it's good to kick all call states from here, but it should be done
-- 
2.31.1

Re: [PATCH v3 0/5] block-copy: make helper APIs thread safe

[Emanuele Giuseppe Esposito]

Please discard this thread, I had an issue with git send-email and patch 
3-5 are missing.

Thank you,
Emanuele

On 14/06/2021 10:08, Emanuele Giuseppe Esposito wrote:
> This serie of patches bring thread safety to the smaller APIs used by
> block-copy, namely ratelimit, progressmeter, co-shared-resource
> and aiotask.
> The end goal is to reduce the usage of AioContexlock in block-copy,
> by introducing smaller granularity locks thus on making the block layer
> thread safe.
> 
> What's missing for block-copy to be fully thread-safe is fixing
> the CoSleep API to allow cross-thread sleep and wakeup.
> Paolo is working on it and will post the patches once his new
> CoSleep API is accepted.
> 
> Patches 1-3 work on ratelimit, 4 covers progressmeter and
> 5 co-shared-resources.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> v3:
> * Rebase on current master (had conflicts in block-copy), remove based-on in
>    cover letter
> 
> Emanuele Giuseppe Esposito (2):
>    progressmeter: protect with a mutex
>    co-shared-resource: protect with a mutex
> 
> Paolo Bonzini (3):
>    ratelimit: treat zero speed as unlimited
>    block-copy: let ratelimit handle a speed of 0
>    blockjob: let ratelimit handle a speed of 0
> 
>   block/block-copy.c                | 28 ++++++--------
>   block/meson.build                 |  1 +
>   block/progress_meter.c            | 64 +++++++++++++++++++++++++++++++
>   blockjob.c                        | 46 +++++++++++++---------
>   include/qemu/co-shared-resource.h |  4 +-
>   include/qemu/progress_meter.h     | 34 ++++++++--------
>   include/qemu/ratelimit.h          | 12 +++++-
>   job-qmp.c                         |  8 +++-
>   job.c                             |  3 ++
>   qemu-img.c                        |  9 +++--
>   util/qemu-co-shared-resource.c    | 24 +++++++++---
>   11 files changed, 168 insertions(+), 65 deletions(-)
>   create mode 100644 block/progress_meter.c
> 

[PATCH v3 0/5] block-copy: make helper APIs thread safe

[Emanuele Giuseppe Esposito]

This serie of patches bring thread safety to the smaller APIs used by
block-copy, namely ratelimit, progressmeter, co-shared-resource
and aiotask.
The end goal is to reduce the usage of AioContexlock in block-copy,
by introducing smaller granularity locks thus on making the block layer
thread safe. 

What's missing for block-copy to be fully thread-safe is fixing
the CoSleep API to allow cross-thread sleep and wakeup.
Paolo is working on it and will post the patches once his new
CoSleep API is accepted.

Patches 1-3 work on ratelimit, 4 covers progressmeter and
5 co-shared-resources.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v3:
* Rebase on current master (had conflicts in block-copy), remove based-on in
  cover letter

Emanuele Giuseppe Esposito (2):
  progressmeter: protect with a mutex
  co-shared-resource: protect with a mutex

Paolo Bonzini (3):
  ratelimit: treat zero speed as unlimited
  block-copy: let ratelimit handle a speed of 0
  blockjob: let ratelimit handle a speed of 0

 block/block-copy.c                | 28 ++++++--------
 block/meson.build                 |  1 +
 block/progress_meter.c            | 64 +++++++++++++++++++++++++++++++
 blockjob.c                        | 46 +++++++++++++---------
 include/qemu/co-shared-resource.h |  4 +-
 include/qemu/progress_meter.h     | 34 ++++++++--------
 include/qemu/ratelimit.h          | 12 +++++-
 job-qmp.c                         |  8 +++-
 job.c                             |  3 ++
 qemu-img.c                        |  9 +++--
 util/qemu-co-shared-resource.c    | 24 +++++++++---
 11 files changed, 168 insertions(+), 65 deletions(-)
 create mode 100644 block/progress_meter.c

-- 
2.31.1

Re: [PATCH v3 0/5] block-copy: make helper APIs thread safe

[Emanuele Giuseppe Esposito]

On 14/06/2021 10:11, Emanuele Giuseppe Esposito wrote:
> This serie of patches bring thread safety to the smaller APIs used by
> block-copy, namely ratelimit, progressmeter, co-shared-resource
> and aiotask.
> The end goal is to reduce the usage of AioContexlock in block-copy,
> by introducing smaller granularity locks thus on making the block layer
> thread safe.
> 
> What's missing for block-copy to be fully thread-safe is fixing
> the CoSleep API to allow cross-thread sleep and wakeup.
> Paolo is working on it and will post the patches once his new
> CoSleep API is accepted.
> 
> Patches 1-3 work on ratelimit, 4 covers progressmeter and
> 5 co-shared-resources.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> v3:
> * Rebase on current master (had conflicts in block-copy), remove based-on in
>    cover letter

Hi Kevin & Max,

I think this series ha been reviewed and I just rebased it to current 
master. Can you give it a look and let me know if it can be merged?

Thank you,
Emanuele

> 
> Emanuele Giuseppe Esposito (2):
>    progressmeter: protect with a mutex
>    co-shared-resource: protect with a mutex
> 
> Paolo Bonzini (3):
>    ratelimit: treat zero speed as unlimited
>    block-copy: let ratelimit handle a speed of 0
>    blockjob: let ratelimit handle a speed of 0
> 
>   block/block-copy.c                | 28 ++++++--------
>   block/meson.build                 |  1 +
>   block/progress_meter.c            | 64 +++++++++++++++++++++++++++++++
>   blockjob.c                        | 46 +++++++++++++---------
>   include/qemu/co-shared-resource.h |  4 +-
>   include/qemu/progress_meter.h     | 34 ++++++++--------
>   include/qemu/ratelimit.h          | 12 +++++-
>   job-qmp.c                         |  8 +++-
>   job.c                             |  3 ++
>   qemu-img.c                        |  9 +++--
>   util/qemu-co-shared-resource.c    | 24 +++++++++---
>   11 files changed, 168 insertions(+), 65 deletions(-)
>   create mode 100644 block/progress_meter.c
> 

Re: [PATCH v3 0/5] block-copy: make helper APIs thread safe

[Vladimir Sementsov-Ogievskiy]

14.06.2021 11:17, Emanuele Giuseppe Esposito wrote:
> 
> 
> On 14/06/2021 10:11, Emanuele Giuseppe Esposito wrote:
>> This serie of patches bring thread safety to the smaller APIs used by
>> block-copy, namely ratelimit, progressmeter, co-shared-resource
>> and aiotask.
>> The end goal is to reduce the usage of AioContexlock in block-copy,
>> by introducing smaller granularity locks thus on making the block layer
>> thread safe.
>>
>> What's missing for block-copy to be fully thread-safe is fixing
>> the CoSleep API to allow cross-thread sleep and wakeup.
>> Paolo is working on it and will post the patches once his new
>> CoSleep API is accepted.
>>
>> Patches 1-3 work on ratelimit, 4 covers progressmeter and
>> 5 co-shared-resources.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>> v3:
>> * Rebase on current master (had conflicts in block-copy), remove based-on in
>>    cover letter
> 
> Hi Kevin & Max,
> 
> I think this series ha been reviewed and I just rebased it to current master. Can you give it a look and let me know if it can be merged?
> 
> Thank you,
> Emanuele


I think, I can queue it myself as a block-job series. ratelimit and progressmeter are not mentioned in Block Jobs sections of MAINTAINERS, but actually these APIs used only by block-jobs.

I remember, Stefan had a complain against patch 5 and against general design of adding mutex to every structure.. Stefan, what do you think now? Paolo, is this v3 OK for you?

If everybody silent, I don't see a reason to slow down these series anymore and will make a pull request on Tuesday.

-- 
Best regards,
Vladimir

Re: [PATCH v3 0/5] block-copy: make helper APIs thread safe

[Vladimir Sementsov-Ogievskiy]

19.06.2021 15:21, Vladimir Sementsov-Ogievskiy wrote:
> 14.06.2021 11:17, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 14/06/2021 10:11, Emanuele Giuseppe Esposito wrote:
>>> This serie of patches bring thread safety to the smaller APIs used by
>>> block-copy, namely ratelimit, progressmeter, co-shared-resource
>>> and aiotask.
>>> The end goal is to reduce the usage of AioContexlock in block-copy,
>>> by introducing smaller granularity locks thus on making the block layer
>>> thread safe.
>>>
>>> What's missing for block-copy to be fully thread-safe is fixing
>>> the CoSleep API to allow cross-thread sleep and wakeup.
>>> Paolo is working on it and will post the patches once his new
>>> CoSleep API is accepted.
>>>
>>> Patches 1-3 work on ratelimit, 4 covers progressmeter and
>>> 5 co-shared-resources.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>> v3:
>>> * Rebase on current master (had conflicts in block-copy), remove based-on in
>>>    cover letter
>>
>> Hi Kevin & Max,
>>
>> I think this series ha been reviewed and I just rebased it to current master. Can you give it a look and let me know if it can be merged?
>>
>> Thank you,
>> Emanuele
> 
> 
> I think, I can queue it myself as a block-job series. ratelimit and progressmeter are not mentioned in Block Jobs sections of MAINTAINERS, but actually these APIs used only by block-jobs.
> 
> I remember, Stefan had a complain against patch 5 and against general design of adding mutex to every structure.. Stefan, what do you think now? Paolo, is this v3 OK for you?
> 
> If everybody silent, I don't see a reason to slow down these series anymore and will make a pull request on Tuesday.
> 

Hmm, actually, I'll wait for final version of "[PATCH v4 0/6] block-copy: protect block-copy internal structures" which seems to be close, to pull them together.


-- 
Best regards,
Vladimir

Re: [PATCH v3 0/5] block-copy: make helper APIs thread safe

[Vladimir Sementsov-Ogievskiy]

14.06.2021 11:11, Emanuele Giuseppe Esposito wrote:
> This serie of patches bring thread safety to the smaller APIs used by
> block-copy, namely ratelimit, progressmeter, co-shared-resource
> and aiotask.
> The end goal is to reduce the usage of AioContexlock in block-copy,
> by introducing smaller granularity locks thus on making the block layer
> thread safe.
> 
> What's missing for block-copy to be fully thread-safe is fixing
> the CoSleep API to allow cross-thread sleep and wakeup.
> Paolo is working on it and will post the patches once his new
> CoSleep API is accepted.
> 
> Patches 1-3 work on ratelimit, 4 covers progressmeter and
> 5 co-shared-resources.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Thanks, applied to the jobs branch

-- 
Best regards,
Vladimir