As for BlockCopyTask, add a lock to protect BlockCopyCallState
ret and sleep_state fields. Also move ret, finished and cancelled
in the OUT fields of BlockCopyCallState.
Here a QemuMutex is used to protect QemuCoSleep field, since it
can be concurrently invoked also from outside threads.
.finished, .cancelled and reads to .ret and .error_is_read will be
protected in the following patch.
.sleep state is handled in the series "coroutine: new sleep/wake API"
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/block-copy.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 3a949fab64..d5ed5932b0 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -55,13 +55,14 @@ typedef struct BlockCopyCallState {
QLIST_ENTRY(BlockCopyCallState) list;
/* State */
- int ret;
bool finished;
- QemuCoSleep sleep;
- bool cancelled;
+ QemuCoSleep sleep; /* TODO: protect API with a lock */
/* OUT parameters */
+ bool cancelled;
+ /* Fields protected by calls_lock in BlockCopyState */
bool error_is_read;
+ int ret;
} BlockCopyCallState;
typedef struct BlockCopyTask {
@@ -110,6 +111,7 @@ typedef struct BlockCopyState {
BlockCopyMethod method;
CoMutex tasks_lock;
QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls */
+ QemuMutex calls_lock;
QLIST_HEAD(, BlockCopyCallState) calls;
/* State fields that use a thread-safe API */
BdrvDirtyBitmap *copy_bitmap;
@@ -289,6 +291,7 @@ void block_copy_state_free(BlockCopyState *s)
}
ratelimit_destroy(&s->rate_limit);
+ qemu_mutex_destroy(&s->calls_lock);
bdrv_release_dirty_bitmap(s->copy_bitmap);
shres_destroy(s->mem);
g_free(s);
@@ -349,6 +352,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
ratelimit_init(&s->rate_limit);
qemu_co_mutex_init(&s->tasks_lock);
+ qemu_mutex_init(&s->calls_lock);
QLIST_INIT(&s->tasks);
QLIST_INIT(&s->calls);
@@ -492,11 +496,14 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
&error_is_read);
- if (ret < 0 && !t->call_state->ret) {
- t->call_state->ret = ret;
- t->call_state->error_is_read = error_is_read;
- } else {
- progress_work_done(t->s->progress, t->bytes);
+
+ WITH_QEMU_LOCK_GUARD(&t->s->calls_lock) {
+ if (ret < 0 && !t->call_state->ret) {
+ t->call_state->ret = ret;
+ t->call_state->error_is_read = error_is_read;
+ } else {
+ progress_work_done(t->s->progress, t->bytes);
+ }
}
co_put_to_shres(t->s->mem, t->bytes);
block_copy_task_end(t, ret);
@@ -740,7 +747,9 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
{
int ret;
+ qemu_mutex_lock(&call_state->s->calls_lock);
QLIST_INSERT_HEAD(&call_state->s->calls, call_state, list);
+ qemu_mutex_unlock(&call_state->s->calls_lock);
do {
ret = block_copy_dirty_clusters(call_state);
@@ -767,7 +776,9 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
call_state->cb(call_state->cb_opaque);
}
+ qemu_mutex_lock(&call_state->s->calls_lock);
QLIST_REMOVE(call_state, list);
+ qemu_mutex_unlock(&call_state->s->calls_lock);
return ret;
}
--
2.30.2
18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
> As for BlockCopyTask, add a lock to protect BlockCopyCallState
> ret and sleep_state fields. Also move ret, finished and cancelled
> in the OUT fields of BlockCopyCallState.
>
> Here a QemuMutex is used to protect QemuCoSleep field, since it
> can be concurrently invoked also from outside threads.
>
> .finished, .cancelled and reads to .ret and .error_is_read will be
> protected in the following patch.
>
> .sleep state is handled in the series "coroutine: new sleep/wake API"
Could we live with one mutex for all needs? Why to add one more?
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
> block/block-copy.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 3a949fab64..d5ed5932b0 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -55,13 +55,14 @@ typedef struct BlockCopyCallState {
> QLIST_ENTRY(BlockCopyCallState) list;
>
> /* State */
> - int ret;
> bool finished;
> - QemuCoSleep sleep;
> - bool cancelled;
> + QemuCoSleep sleep; /* TODO: protect API with a lock */
>
> /* OUT parameters */
> + bool cancelled;
> + /* Fields protected by calls_lock in BlockCopyState */
> bool error_is_read;
> + int ret;
> } BlockCopyCallState;
>
> typedef struct BlockCopyTask {
> @@ -110,6 +111,7 @@ typedef struct BlockCopyState {
> BlockCopyMethod method;
> CoMutex tasks_lock;
> QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls */
> + QemuMutex calls_lock;
> QLIST_HEAD(, BlockCopyCallState) calls;
> /* State fields that use a thread-safe API */
> BdrvDirtyBitmap *copy_bitmap;
> @@ -289,6 +291,7 @@ void block_copy_state_free(BlockCopyState *s)
> }
>
> ratelimit_destroy(&s->rate_limit);
> + qemu_mutex_destroy(&s->calls_lock);
> bdrv_release_dirty_bitmap(s->copy_bitmap);
> shres_destroy(s->mem);
> g_free(s);
> @@ -349,6 +352,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>
> ratelimit_init(&s->rate_limit);
> qemu_co_mutex_init(&s->tasks_lock);
> + qemu_mutex_init(&s->calls_lock);
> QLIST_INIT(&s->tasks);
> QLIST_INIT(&s->calls);
>
> @@ -492,11 +496,14 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
>
> ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,
> &error_is_read);
> - if (ret < 0 && !t->call_state->ret) {
> - t->call_state->ret = ret;
> - t->call_state->error_is_read = error_is_read;
> - } else {
> - progress_work_done(t->s->progress, t->bytes);
> +
> + WITH_QEMU_LOCK_GUARD(&t->s->calls_lock) {
> + if (ret < 0 && !t->call_state->ret) {
> + t->call_state->ret = ret;
> + t->call_state->error_is_read = error_is_read;
> + } else {
> + progress_work_done(t->s->progress, t->bytes);
> + }
> }
> co_put_to_shres(t->s->mem, t->bytes);
> block_copy_task_end(t, ret);
> @@ -740,7 +747,9 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
> {
> int ret;
>
> + qemu_mutex_lock(&call_state->s->calls_lock);
> QLIST_INSERT_HEAD(&call_state->s->calls, call_state, list);
> + qemu_mutex_unlock(&call_state->s->calls_lock);
>
> do {
> ret = block_copy_dirty_clusters(call_state);
> @@ -767,7 +776,9 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
> call_state->cb(call_state->cb_opaque);
> }
>
> + qemu_mutex_lock(&call_state->s->calls_lock);
> QLIST_REMOVE(call_state, list);
> + qemu_mutex_unlock(&call_state->s->calls_lock);
>
> return ret;
> }
>
--
Best regards,
Vladimir
On 20/05/21 17:30, Vladimir Sementsov-Ogievskiy wrote: > 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote: >> As for BlockCopyTask, add a lock to protect BlockCopyCallState >> ret and sleep_state fields. Also move ret, finished and cancelled >> in the OUT fields of BlockCopyCallState. >> >> Here a QemuMutex is used to protect QemuCoSleep field, since it >> can be concurrently invoked also from outside threads. >> >> .finished, .cancelled and reads to .ret and .error_is_read will be >> protected in the following patch. >> >> .sleep state is handled in the series "coroutine: new sleep/wake API" > > Could we live with one mutex for all needs? Why to add one more? This patch should just go away; the QemuMutex will not be needed once QemuCoSleep is thread safe, while right now it is still racy. Paolo
On 21/05/2021 17:01, Paolo Bonzini wrote: > On 20/05/21 17:30, Vladimir Sementsov-Ogievskiy wrote: >> 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote: >>> As for BlockCopyTask, add a lock to protect BlockCopyCallState >>> ret and sleep_state fields. Also move ret, finished and cancelled >>> in the OUT fields of BlockCopyCallState. >>> >>> Here a QemuMutex is used to protect QemuCoSleep field, since it >>> can be concurrently invoked also from outside threads. Actually I don't even protect it here, I should have deleted the above line. I left a TODO for the QemuCoSleep field. >>> >>> .finished, .cancelled and reads to .ret and .error_is_read will be >>> protected in the following patch. >>> >>> .sleep state is handled in the series "coroutine: new sleep/wake API" >> >> Could we live with one mutex for all needs? Why to add one more? > > This patch should just go away; the QemuMutex will not be needed once > QemuCoSleep is thread safe, while right now it is still racy. At this point, I would just rename the other lock (tasks_lock) in "lock" or "state_lock", and substitute it in the calls_lock usages of this patch. Depending on how it comes out, I may merge this with the previous patch. Thank you, Emanuele
On 25/05/21 12:58, Emanuele Giuseppe Esposito wrote: > > At this point, I would just rename the other lock (tasks_lock) in "lock" > or "state_lock", and substitute it in the calls_lock usages of this > patch. Depending on how it comes out, I may merge this with the previous > patch. Renaming the lock is a good idea indeed. Paolo
On 18/05/21 12:07, Emanuele Giuseppe Esposito wrote: > + qemu_mutex_lock(&call_state->s->calls_lock); > QLIST_INSERT_HEAD(&call_state->s->calls, call_state, list); > + qemu_mutex_unlock(&call_state->s->calls_lock); Let's just use tasks_lock here (maybe even rename it to just "lock"). Paolo
© 2016 - 2026 Red Hat, Inc.