Message ID | 20210518100757.31243-1-eesposit@redhat.com |
---|---|
Headers | show |
Series | block-copy: protect block-copy internal structures | expand |
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(-) >
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
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(-)