Re: [PATCH v2 08/20] block/block-copy: add block_copy_cancel

2020-11-04 Thread Max Reitz 
On 22.10.20 22:50, Vladimir Sementsov-Ogievskiy wrote:
> 22.07.2020 14:28, Max Reitz wrote:
>> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
>>> Add function to cancel running async block-copy call. It will be used
>>> in backup.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   include/block/block-copy.h |  7 +++
>>>   block/block-copy.c | 22 +++---
>>>   2 files changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>>> index d40e691123..370a194d3c 100644
>>> --- a/include/block/block-copy.h
>>> +++ b/include/block/block-copy.h
>>> @@ -67,6 +67,13 @@ BlockCopyCallState
>>> *block_copy_async(BlockCopyState *s,
>>>   void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState
>>> *call_state,
>>>     uint64_t speed);
>>>   +/*
>>> + * Cancel running block-copy call.
>>> + * Cancel leaves block-copy state valid: dirty bits are correct and
>>> you may use
>>> + * cancel +  to emulate
>>> pause/resume.
>>> + */
>>> +void block_copy_cancel(BlockCopyCallState *call_state);
>>> +
>>>   BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
>>>   void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
>>>   diff --git a/block/block-copy.c b/block/block-copy.c
>>> index 851d9c8aaf..b551feb6c2 100644
>>> --- a/block/block-copy.c
>>> +++ b/block/block-copy.c
>>> @@ -44,6 +44,8 @@ typedef struct BlockCopyCallState {
>>>   bool failed;
>>>   bool finished;
>>>   QemuCoSleepState *sleep_state;
>>> +    bool cancelled;
>>> +    Coroutine *canceller;
>>>     /* OUT parameters */
>>>   bool error_is_read;
>>> @@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState
>>> *call_state)
>>>   assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>>>   assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
>>>   -    while (bytes && aio_task_pool_status(aio) == 0) {
>>> +    while (bytes && aio_task_pool_status(aio) == 0 &&
>>> !call_state->cancelled) {
>>>   BlockCopyTask *task;
>>>   int64_t status_bytes;
>>>   @@ -693,7 +695,7 @@ static int coroutine_fn
>>> block_copy_common(BlockCopyCallState *call_state)
>>>   do {
>>>   ret = block_copy_dirty_clusters(call_state);
>>>   -    if (ret == 0) {
>>> +    if (ret == 0 && !call_state->cancelled) {
>>>   ret = block_copy_wait_one(call_state->s,
>>> call_state->offset,
>>>     call_state->bytes);
>>>   }
>>> @@ -707,13 +709,18 @@ static int coroutine_fn
>>> block_copy_common(BlockCopyCallState *call_state)
>>>    * 2. We have waited for some intersecting block-copy request
>>>    *    It may have failed and produced new dirty bits.
>>>    */
>>> -    } while (ret > 0);
>>> +    } while (ret > 0 && !call_state->cancelled);
>>
>> Would it be cleaner if block_copy_dirty_cluster() just returned
>> -ECANCELED?  Or would that pose a problem for its callers or the async
>> callback?
>>
> 
> I'd prefer not to merge io ret with block-copy logic: who knows what
> underlying operations may return.. Can't it be _another_ ECANCELED?
> And it would be just a sugar for block_copy_dirty_clusters() call, I'll
> have to check ->cancelled after block_copy_wait_one() anyway.
> Also, for the next version I try to make it more obvious that finished
> block-copy call is in one of thee states:
>  - success
>  - failed
>  - cancelled

OK.

Max

Re: [PATCH v2 08/20] block/block-copy: add block_copy_cancel

2020-10-23 Thread Vladimir Sementsov-Ogievskiy 

22.10.2020 23:50, Vladimir Sementsov-Ogievskiy wrote:

22.07.2020 14:28, Max Reitz wrote:

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:

Add function to cancel running async block-copy call. It will be used
in backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block-copy.h |  7 +++
  block/block-copy.c | 22 +++---
  2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index d40e691123..370a194d3c 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
  void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state,
    uint64_t speed);
+/*
+ * Cancel running block-copy call.
+ * Cancel leaves block-copy state valid: dirty bits are correct and you may use
+ * cancel +  to emulate pause/resume.
+ */
+void block_copy_cancel(BlockCopyCallState *call_state);
+
  BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
  void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
diff --git a/block/block-copy.c b/block/block-copy.c
index 851d9c8aaf..b551feb6c2 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -44,6 +44,8 @@ typedef struct BlockCopyCallState {
  bool failed;
  bool finished;
  QemuCoSleepState *sleep_state;
+    bool cancelled;
+    Coroutine *canceller;
  /* OUT parameters */
  bool error_is_read;
@@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
  assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
  assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
-    while (bytes && aio_task_pool_status(aio) == 0) {
+    while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {
  BlockCopyTask *task;
  int64_t status_bytes;
@@ -693,7 +695,7 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
  do {
  ret = block_copy_dirty_clusters(call_state);
-    if (ret == 0) {
+    if (ret == 0 && !call_state->cancelled) {
  ret = block_copy_wait_one(call_state->s, call_state->offset,
    call_state->bytes);
  }
@@ -707,13 +709,18 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
   * 2. We have waited for some intersecting block-copy request
   *    It may have failed and produced new dirty bits.
   */
-    } while (ret > 0);
+    } while (ret > 0 && !call_state->cancelled);


Would it be cleaner if block_copy_dirty_cluster() just returned
-ECANCELED?  Or would that pose a problem for its callers or the async
callback?



I'd prefer not to merge io ret with block-copy logic: who knows what underlying 
operations may return.. Can't it be _another_ ECANCELED?
And it would be just a sugar for block_copy_dirty_clusters() call, I'll have to 
check ->cancelled after block_copy_wait_one() anyway.
Also, for the next version I try to make it more obvious that finished 
block-copy call is in one of thee states:
  - success
  - failed
  - cancelled

Hmm. Also, cancelled should be OK for copy-on-write operations in filter, it 
just mean that we don't need to care anymore.


This is unrelated: actually only async block-copy call may be cancelled.




  if (call_state->cb) {
  call_state->cb(ret, call_state->error_is_read,
 call_state->s->progress_opaque);
  }
+    if (call_state->canceller) {
+    aio_co_wake(call_state->canceller);
+    call_state->canceller = NULL;
+    }
+
  call_state->finished = true;
  return ret;








--
Best regards,
Vladimir

Re: [PATCH v2 08/20] block/block-copy: add block_copy_cancel

2020-10-22 Thread Vladimir Sementsov-Ogievskiy 

22.07.2020 14:28, Max Reitz wrote:

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:

Add function to cancel running async block-copy call. It will be used
in backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block-copy.h |  7 +++
  block/block-copy.c | 22 +++---
  2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index d40e691123..370a194d3c 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
  void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state,
uint64_t speed);
  
+/*

+ * Cancel running block-copy call.
+ * Cancel leaves block-copy state valid: dirty bits are correct and you may use
+ * cancel +  to emulate pause/resume.
+ */
+void block_copy_cancel(BlockCopyCallState *call_state);
+
  BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
  void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
  
diff --git a/block/block-copy.c b/block/block-copy.c

index 851d9c8aaf..b551feb6c2 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -44,6 +44,8 @@ typedef struct BlockCopyCallState {
  bool failed;
  bool finished;
  QemuCoSleepState *sleep_state;
+bool cancelled;
+Coroutine *canceller;
  
  /* OUT parameters */

  bool error_is_read;
@@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
  assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
  assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
  
-while (bytes && aio_task_pool_status(aio) == 0) {

+while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {
  BlockCopyTask *task;
  int64_t status_bytes;
  
@@ -693,7 +695,7 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)

  do {
  ret = block_copy_dirty_clusters(call_state);
  
-if (ret == 0) {

+if (ret == 0 && !call_state->cancelled) {
  ret = block_copy_wait_one(call_state->s, call_state->offset,
call_state->bytes);
  }
@@ -707,13 +709,18 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
   * 2. We have waited for some intersecting block-copy request
   *It may have failed and produced new dirty bits.
   */
-} while (ret > 0);
+} while (ret > 0 && !call_state->cancelled);


Would it be cleaner if block_copy_dirty_cluster() just returned
-ECANCELED?  Or would that pose a problem for its callers or the async
callback?



I'd prefer not to merge io ret with block-copy logic: who knows what underlying 
operations may return.. Can't it be _another_ ECANCELED?
And it would be just a sugar for block_copy_dirty_clusters() call, I'll have to 
check ->cancelled after block_copy_wait_one() anyway.
Also, for the next version I try to make it more obvious that finished 
block-copy call is in one of thee states:
 - success
 - failed
 - cancelled

Hmm. Also, cancelled should be OK for copy-on-write operations in filter, it 
just mean that we don't need to care anymore.


  if (call_state->cb) {
  call_state->cb(ret, call_state->error_is_read,
 call_state->s->progress_opaque);
  }
  
+if (call_state->canceller) {

+aio_co_wake(call_state->canceller);
+call_state->canceller = NULL;
+}
+
  call_state->finished = true;
  
  return ret;





--
Best regards,
Vladimir

Re: [PATCH v2 08/20] block/block-copy: add block_copy_cancel

2020-07-22 Thread Max Reitz 
On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:
> Add function to cancel running async block-copy call. It will be used
> in backup.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/block-copy.h |  7 +++
>  block/block-copy.c | 22 +++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index d40e691123..370a194d3c 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
>  void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state,
>uint64_t speed);
>  
> +/*
> + * Cancel running block-copy call.
> + * Cancel leaves block-copy state valid: dirty bits are correct and you may 
> use
> + * cancel +  to emulate pause/resume.
> + */
> +void block_copy_cancel(BlockCopyCallState *call_state);
> +
>  BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
>  void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
>  
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 851d9c8aaf..b551feb6c2 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -44,6 +44,8 @@ typedef struct BlockCopyCallState {
>  bool failed;
>  bool finished;
>  QemuCoSleepState *sleep_state;
> +bool cancelled;
> +Coroutine *canceller;
>  
>  /* OUT parameters */
>  bool error_is_read;
> @@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
>  assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>  assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
>  
> -while (bytes && aio_task_pool_status(aio) == 0) {
> +while (bytes && aio_task_pool_status(aio) == 0 && 
> !call_state->cancelled) {
>  BlockCopyTask *task;
>  int64_t status_bytes;
>  
> @@ -693,7 +695,7 @@ static int coroutine_fn 
> block_copy_common(BlockCopyCallState *call_state)
>  do {
>  ret = block_copy_dirty_clusters(call_state);
>  
> -if (ret == 0) {
> +if (ret == 0 && !call_state->cancelled) {
>  ret = block_copy_wait_one(call_state->s, call_state->offset,
>call_state->bytes);
>  }
> @@ -707,13 +709,18 @@ static int coroutine_fn 
> block_copy_common(BlockCopyCallState *call_state)
>   * 2. We have waited for some intersecting block-copy request
>   *It may have failed and produced new dirty bits.
>   */
> -} while (ret > 0);
> +} while (ret > 0 && !call_state->cancelled);

Would it be cleaner if block_copy_dirty_cluster() just returned
-ECANCELED?  Or would that pose a problem for its callers or the async
callback?

>  if (call_state->cb) {
>  call_state->cb(ret, call_state->error_is_read,
> call_state->s->progress_opaque);
>  }
>  
> +if (call_state->canceller) {
> +aio_co_wake(call_state->canceller);
> +call_state->canceller = NULL;
> +}
> +
>  call_state->finished = true;
>  
>  return ret;



signature.asc
Description: OpenPGP digital signature

[PATCH v2 08/20] block/block-copy: add block_copy_cancel

2020-06-01 Thread Vladimir Sementsov-Ogievskiy 
Add function to cancel running async block-copy call. It will be used
in backup.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block-copy.h |  7 +++
 block/block-copy.c | 22 +++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index d40e691123..370a194d3c 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -67,6 +67,13 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
 void block_copy_set_speed(BlockCopyState *s, BlockCopyCallState *call_state,
   uint64_t speed);
 
+/*
+ * Cancel running block-copy call.
+ * Cancel leaves block-copy state valid: dirty bits are correct and you may use
+ * cancel +  to emulate pause/resume.
+ */
+void block_copy_cancel(BlockCopyCallState *call_state);
+
 BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s);
 void block_copy_set_skip_unallocated(BlockCopyState *s, bool skip);
 
diff --git a/block/block-copy.c b/block/block-copy.c
index 851d9c8aaf..b551feb6c2 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -44,6 +44,8 @@ typedef struct BlockCopyCallState {
 bool failed;
 bool finished;
 QemuCoSleepState *sleep_state;
+bool cancelled;
+Coroutine *canceller;
 
 /* OUT parameters */
 bool error_is_read;
@@ -582,7 +584,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
 assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
 assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
 
-while (bytes && aio_task_pool_status(aio) == 0) {
+while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {
 BlockCopyTask *task;
 int64_t status_bytes;
 
@@ -693,7 +695,7 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
 do {
 ret = block_copy_dirty_clusters(call_state);
 
-if (ret == 0) {
+if (ret == 0 && !call_state->cancelled) {
 ret = block_copy_wait_one(call_state->s, call_state->offset,
   call_state->bytes);
 }
@@ -707,13 +709,18 @@ static int coroutine_fn 
block_copy_common(BlockCopyCallState *call_state)
  * 2. We have waited for some intersecting block-copy request
  *It may have failed and produced new dirty bits.
  */
-} while (ret > 0);
+} while (ret > 0 && !call_state->cancelled);
 
 if (call_state->cb) {
 call_state->cb(ret, call_state->error_is_read,
call_state->s->progress_opaque);
 }
 
+if (call_state->canceller) {
+aio_co_wake(call_state->canceller);
+call_state->canceller = NULL;
+}
+
 call_state->finished = true;
 
 return ret;
@@ -772,6 +779,15 @@ BlockCopyCallState *block_copy_async(BlockCopyState *s,
 
 return call_state;
 }
+
+void block_copy_cancel(BlockCopyCallState *call_state)
+{
+call_state->cancelled = true;
+call_state->canceller = qemu_coroutine_self();
+block_copy_kick(call_state);
+qemu_coroutine_yield();
+}
+
 BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s)
 {
 return s->copy_bitmap;
-- 
2.21.0