Re: [PATCH v2 0/7] block-copy: protect block-copy internal structures

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]

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

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