On Tue, Jan 30, 2018 at 08:20:03AM -0600, Eric Blake wrote:
> On 01/30/2018 02:38 AM, Liang Li wrote:
>> When doing drive mirror to a low speed shared storage, if there was heavy
>> BLK IO write workload in VM after the 'ready' event, drive mirror block job
>> can't be canceled immediately, it would keep running until the heavy BLK IO
>> workload stopped in the VM.
>
> So far so good. But the grammar and explanation in the rest of the
> commit is a bit hard to read; let me give a shot at an alternative wording:
>
> Libvirt depends on the current block-job-cancel semantics, which is that
> when used without a flag after the 'ready' event, the command blocks
> until data is in sync. However, these semantics are awkward in other
> situations, for example, people may use drive mirror for realtime
> backups while still wanting to use block live migration. Libvirt cannot
> start a block live migration while another drive mirror is in progress,
> but the user would rather abandon the backup attempt as broken and
> proceed with the live migration than be stuck waiting for the current
> drive mirror backup to finish.
>
> The drive-mirror command already includes a 'force' flag, which libvirt
> does not use, although it documented the flag as only being useful to
> quit a job which is paused. However, since quitting a paused job has
> the same effect as abandoning a backup in a non-paused job (namely, the
> destination file is not in sync, and the command completes immediately),
> we can just improve the documentation to make the force flag obviously
> useful.
>
much better, will include in the v2. Thanks!
>>
>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>> Cc: Jeff Cody <jc...@redhat.com>
>> Cc: Kevin Wolf <kw...@redhat.com>
>> Cc: Max Reitz <mre...@redhat.com>
>> Cc: Eric Blake <ebl...@redhat.com>
>> Cc: John Snow <js...@redhat.com>
>> Reported-by: Huaitong Han <huanhuait...@didichuxing.com>
>> Signed-off-by: Huaitong Han <huanhuait...@didichuxing.com>
>> Signed-off-by: Liang Li <liliang...@didichuxing.com>
>> ---
>
>
>> +++ b/hmp-commands.hx
>> @@ -106,7 +106,8 @@ ETEXI
>> .args_type = "force:-f,device:B",
>> .params = "[-f] device",
>> .help = "stop an active background block operation (use -f"
>> - "\n\t\t\t if the operation is currently paused)",
>> + "\n\t\t\t if you want to abort the operation
>> immediately"
>> + "\n\t\t\t instead of keep running until data is in
>> sync )",
>
> s/sync )/sync)/
>
done
>> .cmd = hmp_block_job_cancel,
>> },
>>
>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>> index 00403d9..4a96c42 100644
>> --- a/include/block/blockjob.h
>> +++ b/include/block/blockjob.h
>> @@ -63,6 +63,12 @@ typedef struct BlockJob {
>> bool cancelled;
>>
>> /**
>> + * Set to true if the job should be abort immediately without waiting
>
> s/be //
done
>
>> + * for data is in sync.
>
> s/is/to be/
>
done
>> + */
>> + bool force;
>> +
>> + /**
>> * Counter for pause request. If non-zero, the block job is either
>> paused,
>> * or if busy == true will pause itself as soon as possible.
>> */
>> @@ -218,10 +224,11 @@ void block_job_start(BlockJob *job);
>> /**
>> * block_job_cancel:
>> * @job: The job to be canceled.
>> + * @force: Quit a job without waiting data is in sync.
>
> s/data is/for data to be/
>
done
>> +++ b/qapi/block-core.json
>> @@ -2098,8 +2098,10 @@
>> # the name of the parameter), but since QEMU 2.7 it can have
>> # other values.
>> #
>> -# @force: whether to allow cancellation of a paused job (default
>> -# false). Since 1.3.
>> +# @force: #optional whether to allow cancellation a job without waiting
>> data is
>
> The '#optional' tag should no longer be added.
>
>> +# in sync, please not that since 2.12 it's semantic is not exactly
>> the
>> +# same as before, from 1.3 to 2.11 it means whether to allow
>> cancellation
>> +# of a paused job (default false). Since 1.3.
>
> Reads awkwardly. I suggest:
>
> @force: If true, and the job has already emitted the event
> BLOCK_JOB_READY, abandon the job immediately (even if it is paused)
> instead of waiting for the destination to complete its final
> synchronization (since 1.3)
>
much more clear
>
>> +++ b/tests/test-blockjob-txn.c
>> @@ -125,7 +125,7 @@ static void test_single_job(int expected)
>> block_job_start(job);
>>
>> if (expected == -ECANCELED) {
>> - block_job_cancel(job);
>> + block_job_cancel(job, false);
>> }
>>
>> while (result == -EINPROGRESS) {
>> @@ -173,10 +173,10 @@ static void test_pair_jobs(int expected1, int
>> expected2)
>> block_job_txn_unref(txn);
>>
>> if (expected1 == -ECANCELED) {
>> - block_job_cancel(job1);
>> + block_job_cancel(job1, false);
>> }
>> if (expected2 == -ECANCELED) {
>> - block_job_cancel(job2);
>> + block_job_cancel(job2, false);
>> }
>>
>> while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
>> @@ -231,7 +231,7 @@ static void test_pair_jobs_fail_cancel_race(void)
>> block_job_start(job1);
>> block_job_start(job2);
>>
>> - block_job_cancel(job1);
>> + block_job_cancel(job1, false);
>>
>> /* Now make job2 finish before the main loop kicks jobs. This simulates
>> * the race between a pending kick and another job completing.
>>
>
> The testsuite should probably also test block_job_cancel(..., true).
>
will change in v2, thanks!
Liang
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
