Commit b76e4458b1eb3c32e9824fe6aa51f67d2b251748 made the mirror block
job respect block-job-cancel's @force flag: With that flag set, it would
now always really cancel, even post-READY.

Unfortunately, it had a side effect: Without that flag set, it would now
never cancel, not even before READY.  Considering that is an
incompatible change and not noted anywhere in the commit or the
description of block-job-cancel's @force parameter, this seems
unintentional and we should revert to the previous behavior, which is to
immediately cancel the job when block-job-cancel is called before source
and target are in sync (i.e. before the READY event).

Cc: qemu-stable@nongnu.org
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1572856
Reported-by: Yanan Fu <yfu@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 7bfad6e844..003f957b12 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -874,7 +874,9 @@ static void coroutine_fn mirror_run(void *opaque)
         }
         trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
         block_job_sleep_ns(&s->common, delay_ns);
-        if (block_job_is_cancelled(&s->common) && s->common.force) {
+        if (block_job_is_cancelled(&s->common) &&
+            (!s->synced || s->common.force))
+        {
             break;
         }
         s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-- 
2.14.3

On 05/01/2018 05:05 PM, Max Reitz wrote:
> Commit b76e4458b1eb3c32e9824fe6aa51f67d2b251748 made the mirror block
> job respect block-job-cancel's @force flag: With that flag set, it would
> now always really cancel, even post-READY.
> 
> Unfortunately, it had a side effect: Without that flag set, it would now
> never cancel, not even before READY.  Considering that is an
> incompatible change and not noted anywhere in the commit or the
> description of block-job-cancel's @force parameter, this seems
> unintentional and we should revert to the previous behavior, which is to
> immediately cancel the job when block-job-cancel is called before source
> and target are in sync (i.e. before the READY event).
> 
> Cc: qemu-stable@nongnu.org

Actually adding that in cc on this reply.

> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1572856
> Reported-by: Yanan Fu <yfu@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/mirror.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 7bfad6e844..003f957b12 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -874,7 +874,9 @@ static void coroutine_fn mirror_run(void *opaque)
>           }
>           trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>           block_job_sleep_ns(&s->common, delay_ns);
> -        if (block_job_is_cancelled(&s->common) && s->common.force) {
> +        if (block_job_is_cancelled(&s->common) &&
> +            (!s->synced || s->common.force))
> +        {
>               break;
>           }
>           s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 2018-05-02 01:31, Eric Blake wrote:
> On 05/01/2018 05:05 PM, Max Reitz wrote:
>> Commit b76e4458b1eb3c32e9824fe6aa51f67d2b251748 made the mirror block
>> job respect block-job-cancel's @force flag: With that flag set, it would
>> now always really cancel, even post-READY.
>>
>> Unfortunately, it had a side effect: Without that flag set, it would now
>> never cancel, not even before READY.  Considering that is an
>> incompatible change and not noted anywhere in the commit or the
>> description of block-job-cancel's @force parameter, this seems
>> unintentional and we should revert to the previous behavior, which is to
>> immediately cancel the job when block-job-cancel is called before source
>> and target are in sync (i.e. before the READY event).
>>
>> Cc: qemu-stable@nongnu.org
> 
> Actually adding that in cc on this reply.

Thanks, I knew I was going to forget...

Max

>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1572856
>> Reported-by: Yanan Fu <yfu@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/mirror.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 7bfad6e844..003f957b12 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -874,7 +874,9 @@ static void coroutine_fn mirror_run(void *opaque)
>>           }
>>           trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
>>           block_job_sleep_ns(&s->common, delay_ns);
>> -        if (block_job_is_cancelled(&s->common) && s->common.force) {
>> +        if (block_job_is_cancelled(&s->common) &&
>> +            (!s->synced || s->common.force))
>> +        {
>>               break;
>>           }
>>           s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>>
>