Re: [PATCH v2 5/7] block-copy: add QemuMutex lock for BlockCopyCallState list

2021-05-28 Thread Paolo Bonzini 

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

Re: [PATCH v2 5/7] block-copy: add QemuMutex lock for BlockCopyCallState list

2021-05-26 Thread Paolo Bonzini 

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

Re: [PATCH v2 5/7] block-copy: add QemuMutex lock for BlockCopyCallState list

2021-05-25 Thread Emanuele Giuseppe Esposito 




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

Re: [PATCH v2 5/7] block-copy: add QemuMutex lock for BlockCopyCallState list

2021-05-21 Thread Paolo Bonzini 

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

Re: [PATCH v2 5/7] block-copy: add QemuMutex lock for BlockCopyCallState list

2021-05-20 Thread Vladimir Sementsov-Ogievskiy 

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 
---
  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

[PATCH v2 5/7] block-copy: add QemuMutex lock for BlockCopyCallState list

2021-05-18 Thread Emanuele Giuseppe Esposito 
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 
---
 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