qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH] block/mirror: change the semantic of 'force' of


From: Liang Li
Subject: Re: [Qemu-block] [PATCH] block/mirror: change the semantic of 'force' of block-job-cancel
Date: Thu, 1 Feb 2018 10:14:05 +0800

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 <address@hidden>
>> Cc: Jeff Cody <address@hidden>
>> Cc: Kevin Wolf <address@hidden>
>> Cc: Max Reitz <address@hidden>
>> Cc: Eric Blake <address@hidden>
>> Cc: John Snow <address@hidden>
>> Reported-by: Huaitong Han <address@hidden>
>> Signed-off-by: Huaitong Han <address@hidden>
>> Signed-off-by: Liang Li <address@hidden>
>> ---
> 
> 
>> +++ 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]