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

Emanuele Giuseppe Esposito posted 6 patches 2 years, 10 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
block/aio_task.c               | 63 ++++++++++++++++++++--------------
block/block-copy.c             | 28 ++++++---------
blockjob.c                     | 45 +++++++++++++++---------
include/block/aio_task.h       |  2 +-
include/qemu/progress_meter.h  | 31 +++++++++++++++++
include/qemu/ratelimit.h       | 12 +++++--
job-qmp.c                      |  8 +++--
job.c                          |  3 ++
qemu-img.c                     |  9 +++--
util/qemu-co-shared-resource.c | 26 +++++++++++---
10 files changed, 155 insertions(+), 72 deletions(-)
[PATCH 0/6] block-copy: make helper APIs thread safe
Posted by Emanuele Giuseppe Esposito 2 years, 10 months ago
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 the global
AioContexlock in block-copy, by introducing smaller granularity
locks thus on making the block layer thread safe. 

This serie depends on Paolo's coroutine_sleep API.

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 (they are also based on the first
ratelimit patch sent by Paolo), 4 covers progressmeter,
5 co-shared-resources and 6 aiopool.

Based-on: <20210503112550.478521-1-pbonzini@redhat.com> [coroutine_sleep]
Based-on: <20210413125533.217440-1-pbonzini@redhat.com> [ratelimit]
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v1 -> v2:
* More field categorized as IN/State/OUT in the various struct
* Fix a couple of places where I missed locks [Vladimir, Paolo]

Emanuele Giuseppe Esposito (3):
  progressmeter: protect with a mutex
  co-shared-resource: protect with a mutex
  aiopool: 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/aio_task.c               | 63 ++++++++++++++++++++--------------
 block/block-copy.c             | 28 ++++++---------
 blockjob.c                     | 45 +++++++++++++++---------
 include/block/aio_task.h       |  2 +-
 include/qemu/progress_meter.h  | 31 +++++++++++++++++
 include/qemu/ratelimit.h       | 12 +++++--
 job-qmp.c                      |  8 +++--
 job.c                          |  3 ++
 qemu-img.c                     |  9 +++--
 util/qemu-co-shared-resource.c | 26 +++++++++++---
 10 files changed, 155 insertions(+), 72 deletions(-)

-- 
2.30.2


Re: [PATCH 0/6] block-copy: make helper APIs thread safe
Posted by Vladimir Sementsov-Ogievskiy 2 years, 10 months ago
10.05.2021 11:59, 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 the global
> AioContexlock in block-copy, by introducing smaller granularity
> locks thus on making the block layer thread safe.
> 
> This serie depends on Paolo's coroutine_sleep API.
> 
> 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 (they are also based on the first
> ratelimit patch sent by Paolo), 4 covers progressmeter,
> 5 co-shared-resources and 6 aiopool.
> 
> Based-on: <20210503112550.478521-1-pbonzini@redhat.com> [coroutine_sleep]
> Based-on: <20210413125533.217440-1-pbonzini@redhat.com> [ratelimit]

Seems, patchew failed to parse your "Based-on" tags: https://patchew.org/QEMU/20210510085941.22769-1-eesposit@redhat.com/
(and tries to apply series on master)
I think, it's because you placed "[...]" comments on the same lines.

Interesting, will patchew see, if I just duplicate tags here:

Based-on: <20210503112550.478521-1-pbonzini@redhat.com>
Based-on: <20210413125533.217440-1-pbonzini@redhat.com>


> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> v1 -> v2:
> * More field categorized as IN/State/OUT in the various struct
> * Fix a couple of places where I missed locks [Vladimir, Paolo]
> 
> Emanuele Giuseppe Esposito (3):
>    progressmeter: protect with a mutex
>    co-shared-resource: protect with a mutex
>    aiopool: 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/aio_task.c               | 63 ++++++++++++++++++++--------------
>   block/block-copy.c             | 28 ++++++---------
>   blockjob.c                     | 45 +++++++++++++++---------
>   include/block/aio_task.h       |  2 +-
>   include/qemu/progress_meter.h  | 31 +++++++++++++++++
>   include/qemu/ratelimit.h       | 12 +++++--
>   job-qmp.c                      |  8 +++--
>   job.c                          |  3 ++
>   qemu-img.c                     |  9 +++--
>   util/qemu-co-shared-resource.c | 26 +++++++++++---
>   10 files changed, 155 insertions(+), 72 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/6] block-copy: make helper APIs thread safe
Posted by Stefan Hajnoczi 2 years, 10 months ago
On Mon, May 10, 2021 at 10:59:35AM +0200, 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 the global
> AioContexlock in block-copy, by introducing smaller granularity
> locks thus on making the block layer thread safe. 

I'm not sure "global" is accurate. Block jobs can deal with arbitrary
AioContexts, not just a global AioContext. The lock is per-AioContext
rather than global. Or did I miss something?