Re: [PATCH v3 4/5] block-copy: add a CoMutex
08.06.2021 10:33, Emanuele Giuseppe Esposito wrote:
Add a CoMutex to protect concurrent access of block-copy
data structures.
This mutex also protects .copy_bitmap, because its thread-safe
API does not prevent it from assigning two tasks to the same
bitmap region.
.finished, .cancelled and reads to .ret and .error_is_read will be
protected in the following patch, because are used also outside
coroutines.
Also set block_copy_task_create as coroutine_fn because:
1) it is static and only invoked by coroutine functions
2) this patch introduces and uses a CoMutex lock there
Signed-off-by: Emanuele Giuseppe Esposito
I missed, did you (where?) add a comment like "all APIs are thread-safe", or
what is thread-safe?
---
block/block-copy.c | 82 ++
1 file changed, 54 insertions(+), 28 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index e2adb5b2ea..56f62913e4 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -61,6 +61,7 @@ typedef struct BlockCopyCallState {
/* OUT parameters */
bool cancelled;
+/* Fields protected by lock in BlockCopyState */
bool error_is_read;
int ret;
} BlockCopyCallState;
@@ -78,7 +79,7 @@ typedef struct BlockCopyTask {
int64_t bytes; /* only re-set in task_shrink, before running the task */
BlockCopyMethod method; /* initialized in block_copy_dirty_clusters() */
-/* State */
+/* State. Protected by lock in BlockCopyState */
CoQueue wait_queue; /* coroutines blocked on this task */
/* To reference all call states from BlockCopyState */
@@ -99,7 +100,8 @@ typedef struct BlockCopyState {
BdrvChild *source;
BdrvChild *target;
-/* State */
+/* State. Protected by lock */
+CoMutex lock;
int64_t in_flight_bytes;
BlockCopyMethod method;
QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls
*/
@@ -139,8 +141,10 @@ typedef struct BlockCopyState {
bool skip_unallocated;
} BlockCopyState;
May be nitpicking, but if we want block_copy_set_progress_meter to be threadsafe
it should set s->progress under mutex. Or we should document that it's not
threadsafe and called once.
-static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
-int64_t offset, int64_t bytes)
+/* Called with lock held */
+static BlockCopyTask *find_conflicting_task_locked(BlockCopyState *s,
+ int64_t offset,
+ int64_t bytes)
{
BlockCopyTask *t;
@@ -160,18 +164,22 @@ static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t
offset,
int64_t bytes)
{
-BlockCopyTask *task = find_conflicting_task(s, offset, bytes);
+BlockCopyTask *task;
+
+QEMU_LOCK_GUARD(&s->lock);
+task = find_conflicting_task_locked(s, offset, bytes);
if (!task) {
return false;
}
-qemu_co_queue_wait(&task->wait_queue, NULL);
+qemu_co_queue_wait(&task->wait_queue, &s->lock);
return true;
}
-static int64_t block_copy_chunk_size(BlockCopyState *s)
+/* Called with lock held */
+static int64_t block_copy_chunk_size_locked(BlockCopyState *s)
{
switch (s->method) {
case COPY_READ_WRITE_CLUSTER:
@@ -193,14 +201,16 @@ static int64_t block_copy_chunk_size(BlockCopyState *s)
* Search for the first dirty area in offset/bytes range and create task at
* the beginning of it.
*/
-static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
- BlockCopyCallState *call_state,
- int64_t offset, int64_t bytes)
+static coroutine_fn BlockCopyTask *block_copy_task_create(BlockCopyState *s,
+BlockCopyCallState *call_state,
+int64_t offset, int64_t bytes)
{
BlockCopyTask *task;
-int64_t max_chunk = block_copy_chunk_size(s);
+int64_t max_chunk;
-max_chunk = MIN_NON_ZERO(max_chunk, call_state->max_chunk);
+QEMU_LOCK_GUARD(&s->lock);
+max_chunk = MIN_NON_ZERO(block_copy_chunk_size_locked(s),
+ call_state->max_chunk);
if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
offset, offset + bytes,
max_chunk, &offset, &bytes))
@@ -212,7 +222,7 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState
*s,
bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
/* region is dirty, so no existent tasks possible in it */
-assert(!find_conflicting_task(s, offset, bytes));
+assert(!find_conflicting_task_locked(s, offset, bytes));
Re: [PATCH v3 4/5] block-copy: add a CoMutex
On 09/06/2021 14:25, Vladimir Sementsov-Ogievskiy wrote:
08.06.2021 10:33, Emanuele Giuseppe Esposito wrote:
Add a CoMutex to protect concurrent access of block-copy
data structures.
This mutex also protects .copy_bitmap, because its thread-safe
API does not prevent it from assigning two tasks to the same
bitmap region.
.finished, .cancelled and reads to .ret and .error_is_read will be
protected in the following patch, because are used also outside
coroutines.
Also set block_copy_task_create as coroutine_fn because:
1) it is static and only invoked by coroutine functions
2) this patch introduces and uses a CoMutex lock there
Signed-off-by: Emanuele Giuseppe Esposito
I missed, did you (where?) add a comment like "all APIs are
thread-safe", or what is thread-safe?
You're right, I can't find that comment too. I will add it once more.
---
block/block-copy.c | 82 ++
1 file changed, 54 insertions(+), 28 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index e2adb5b2ea..56f62913e4 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -61,6 +61,7 @@ typedef struct BlockCopyCallState {
/* OUT parameters */
bool cancelled;
+ /* Fields protected by lock in BlockCopyState */
bool error_is_read;
int ret;
} BlockCopyCallState;
@@ -78,7 +79,7 @@ typedef struct BlockCopyTask {
int64_t bytes; /* only re-set in task_shrink, before running the
task */
BlockCopyMethod method; /* initialized in
block_copy_dirty_clusters() */
- /* State */
+ /* State. Protected by lock in BlockCopyState */
CoQueue wait_queue; /* coroutines blocked on this task */
/* To reference all call states from BlockCopyState */
@@ -99,7 +100,8 @@ typedef struct BlockCopyState {
BdrvChild *source;
BdrvChild *target;
- /* State */
+ /* State. Protected by lock */
+ CoMutex lock;
int64_t in_flight_bytes;
BlockCopyMethod method;
QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all
block-copy calls */
@@ -139,8 +141,10 @@ typedef struct BlockCopyState {
bool skip_unallocated;
} BlockCopyState;
May be nitpicking, but if we want block_copy_set_progress_meter to be
threadsafe it should set s->progress under mutex. Or we should document
that it's not threadsafe and called once.
Document it definitely. It is only called by the job before starting
block-copy, so it is safe to do as it is.
-static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
- int64_t offset, int64_t
bytes)
+/* Called with lock held */
+static BlockCopyTask *find_conflicting_task_locked(BlockCopyState *s,
+ int64_t offset,
+ int64_t bytes)
{
BlockCopyTask *t;
@@ -160,18 +164,22 @@ static BlockCopyTask
*find_conflicting_task(BlockCopyState *s,
static bool coroutine_fn block_copy_wait_one(BlockCopyState *s,
int64_t offset,
int64_t bytes)
{
- BlockCopyTask *task = find_conflicting_task(s, offset, bytes);
+ BlockCopyTask *task;
+
+ QEMU_LOCK_GUARD(&s->lock);
+ task = find_conflicting_task_locked(s, offset, bytes);
if (!task) {
return false;
}
- qemu_co_queue_wait(&task->wait_queue, NULL);
+ qemu_co_queue_wait(&task->wait_queue, &s->lock);
return true;
}
-static int64_t block_copy_chunk_size(BlockCopyState *s)
+/* Called with lock held */
+static int64_t block_copy_chunk_size_locked(BlockCopyState *s)
{
switch (s->method) {
case COPY_READ_WRITE_CLUSTER:
@@ -193,14 +201,16 @@ static int64_t
block_copy_chunk_size(BlockCopyState *s)
* Search for the first dirty area in offset/bytes range and create
task at
* the beginning of it.
*/
-static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
- BlockCopyCallState
*call_state,
- int64_t offset, int64_t
bytes)
+static coroutine_fn BlockCopyTask
*block_copy_task_create(BlockCopyState *s,
+ BlockCopyCallState
*call_state,
+ int64_t offset,
int64_t bytes)
{
BlockCopyTask *task;
- int64_t max_chunk = block_copy_chunk_size(s);
+ int64_t max_chunk;
- max_chunk = MIN_NON_ZERO(max_chunk, call_state->max_chunk);
+ QEMU_LOCK_GUARD(&s->lock);
+ max_chunk = MIN_NON_ZERO(block_copy_chunk_size_locked(s),
+ call_state->max_chunk);
if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap,
offset, offset + bytes,
max_chunk, &offset, &bytes))
@@ -212,7 +222,7 @@ static BlockCopyTask
*block_copy_task_create(BlockCopyState *s,
bytes = QEMU_AL
