block/block-copy.c | 234 +++++++++++++++++++++++++++++---------------- 1 file changed, 150 insertions(+), 84 deletions(-)
This serie of patches aims 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 and my previous
serie that brings thread safety to the smaller API used by block-copy,
like ratelimit, progressmeter abd co-shared-resource.
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.
Patch 1 introduces the .method field instead of .use_copy_range
and .copy_size, so that it can be later used as atomic.
Patch 2-3 provide comments and refactoring in preparation to
the locks added in patch 4 on BlockCopyTask, patch 5-6 on
BlockCopyCallState and 7 BlockCopyState.
Based-on: <20210517100548.28806-1-pbonzini@redhat.com>
Based-on: <20210518094058.25952-1-eesposit@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v1 -> v2:
* More field categorized as IN/State/OUT in the various struct, better
documentation in the structs
* Fix a couple of places where I missed locks [Vladimir, Paolo]
Emanuele Giuseppe Esposito (6):
block-copy: improve documentation of BlockCopyTask and BlockCopyState
types and functions
block-copy: move progress_set_remaining in block_copy_task_end
block-copy: add a CoMutex to the BlockCopyTask list
block-copy: add QemuMutex lock for BlockCopyCallState list
block-copy: atomic .cancelled and .finished fields in
BlockCopyCallState
block-copy: protect BlockCopyState .method fields
Paolo Bonzini (1):
block-copy: streamline choice of copy_range vs. read/write
block/block-copy.c | 234 +++++++++++++++++++++++++++++----------------
1 file changed, 150 insertions(+), 84 deletions(-)
--
2.30.2
18.05.2021 13:07, Emanuele Giuseppe Esposito wrote: > This serie of patches aims 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 and my previous > serie that brings thread safety to the smaller API used by block-copy, > like ratelimit, progressmeter abd co-shared-resource. > > 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. > > Patch 1 introduces the .method field instead of .use_copy_range > and .copy_size, so that it can be later used as atomic. > Patch 2-3 provide comments and refactoring in preparation to > the locks added in patch 4 on BlockCopyTask, patch 5-6 on > BlockCopyCallState and 7 BlockCopyState. > > Based-on: <20210517100548.28806-1-pbonzini@redhat.com> > Based-on: <20210518094058.25952-1-eesposit@redhat.com> Hi! I failed to apply this all. Could you please export your branch with your patches at some public git repo? > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > v1 -> v2: > * More field categorized as IN/State/OUT in the various struct, better > documentation in the structs > * Fix a couple of places where I missed locks [Vladimir, Paolo] > > > Emanuele Giuseppe Esposito (6): > block-copy: improve documentation of BlockCopyTask and BlockCopyState > types and functions > block-copy: move progress_set_remaining in block_copy_task_end > block-copy: add a CoMutex to the BlockCopyTask list > block-copy: add QemuMutex lock for BlockCopyCallState list > block-copy: atomic .cancelled and .finished fields in > BlockCopyCallState > block-copy: protect BlockCopyState .method fields > > Paolo Bonzini (1): > block-copy: streamline choice of copy_range vs. read/write > > block/block-copy.c | 234 +++++++++++++++++++++++++++++---------------- > 1 file changed, 150 insertions(+), 84 deletions(-) > -- Best regards, Vladimir
On 20/05/2021 15:47, Vladimir Sementsov-Ogievskiy wrote: > 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote: >> This serie of patches aims 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 and my previous >> serie that brings thread safety to the smaller API used by block-copy, >> like ratelimit, progressmeter abd co-shared-resource. >> >> 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. >> >> Patch 1 introduces the .method field instead of .use_copy_range >> and .copy_size, so that it can be later used as atomic. >> Patch 2-3 provide comments and refactoring in preparation to >> the locks added in patch 4 on BlockCopyTask, patch 5-6 on >> BlockCopyCallState and 7 BlockCopyState. >> >> Based-on: <20210517100548.28806-1-pbonzini@redhat.com> >> Based-on: <20210518094058.25952-1-eesposit@redhat.com> > > Hi! I failed to apply this all. Could you please export your branch with > your patches at some public git repo? Hi, thank you for applying the patches. My branch is here: https://gitlab.com/eesposit/qemu/-/commits/dataplane_new Emanuele > >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> v1 -> v2: >> * More field categorized as IN/State/OUT in the various struct, better >> documentation in the structs >> * Fix a couple of places where I missed locks [Vladimir, Paolo] >> >> >> Emanuele Giuseppe Esposito (6): >> block-copy: improve documentation of BlockCopyTask and BlockCopyState >> types and functions >> block-copy: move progress_set_remaining in block_copy_task_end >> block-copy: add a CoMutex to the BlockCopyTask list >> block-copy: add QemuMutex lock for BlockCopyCallState list >> block-copy: atomic .cancelled and .finished fields in >> BlockCopyCallState >> block-copy: protect BlockCopyState .method fields >> >> Paolo Bonzini (1): >> block-copy: streamline choice of copy_range vs. read/write >> >> block/block-copy.c | 234 +++++++++++++++++++++++++++++---------------- >> 1 file changed, 150 insertions(+), 84 deletions(-) >> > >
On Tue, May 18, 2021 at 12:07:50PM +0200, Emanuele Giuseppe Esposito wrote:
> This serie of patches aims 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 and my previous
> serie that brings thread safety to the smaller API used by block-copy,
> like ratelimit, progressmeter abd co-shared-resource.
>
> 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.
>
> Patch 1 introduces the .method field instead of .use_copy_range
> and .copy_size, so that it can be later used as atomic.
> Patch 2-3 provide comments and refactoring in preparation to
> the locks added in patch 4 on BlockCopyTask, patch 5-6 on
> BlockCopyCallState and 7 BlockCopyState.
>
> Based-on: <20210517100548.28806-1-pbonzini@redhat.com>
> Based-on: <20210518094058.25952-1-eesposit@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Here is my understanding of thread-safety in your
https://gitlab.com/eesposit/qemu.git dataplane_new branch:
Reading the code was much more satisfying than trying to review the
patches. That's probably because I'm not familiar with the block-copy.c
implementation and needed the context. I noticed you already addressed
some of Vladimir's comments in your git branch, so that may have helped
too.
The backup block job and the backup-top filter driver have a
BlockCopyState. QEMU threads that call bdrv_co_preadv/pwritev() invoke
the block_copy() coroutine function, so BlockCopyState needs to be
protected between threads. Additionally, the backup block job invokes
block_copy_async() to perform a background copy operation.
The relationships are as follows:
BlockCopyState - shared state that any thread can access
BlockCopyCallState - per-block_copy()/block_copy_async() state, not
accessed by other coroutines/threads
BlockCopyTask - per-aiotask state, find_conflicting_task_locked()
accesses this for a given BlockCopyState
What is the purpose of the BlockCopyState->calls list?
Existing issue: the various
block_copy_call_finished/succeeded/failed/cancelled/status() APIs are a
bit extreme. They all end up being called by block/backup.c in
succession when it seems like a single call should be enough to report
the status.
It's not immediately obvious to me that BlockCopyCallState needs to be
thread-safe. So I wondered why finished needs to be atomic. Then I
realized the set_speed/cancel code runs in the monitor, so at least
block_copy_call_cancel() and block_copy_kick() need to be thread-safe. I
guess the BlockCopyCallState status APIs were made thread-safe for
consistency even though it's not needed at the moment?
Please add doc comments to block-copy.h explaining the thread-safety of
the APIs (it might be as simple as "all APIs are thread-safe" at the top
of the file).
Summarizing everything, this series adds BlockCopyState->lock to protect
shared state and makes BlockCopyCallState's status atomic so it can be
queried from threads other than the one performing the copy operation.
Stefan
© 2016 - 2024 Red Hat, Inc.