jcs.org / qemu
qemu with hax to log dma reads & writes jcs.org/2018/11/12/vfio
fork atom

block/mirror: change the semantic of 'force' of block-job-cancel

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.

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.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Jeff Cody <jcody@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Reported-by: Huaitong Han <huanhuaitong@didichuxing.com>
Signed-off-by: Huaitong Han <huanhuaitong@didichuxing.com>
Signed-off-by: Liang Li <liliangleo@didichuxing.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

authored by

Liang Li and committed by
Kevin Wolf b76e4458 1cfeaf38

+34 -24
+4 -6
block/mirror.c
··· 869 869 870 870 ret = 0; 871 871 trace_mirror_before_sleep(s, cnt, s->synced, delay_ns); 872 - if (!s->synced) { 873 - block_job_sleep_ns(&s->common, delay_ns); 874 - if (block_job_is_cancelled(&s->common)) { 875 - break; 876 - } 872 + if (block_job_is_cancelled(&s->common) && s->common.force) { 873 + break; 877 874 } else if (!should_complete) { 878 875 delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0); 879 876 block_job_sleep_ns(&s->common, delay_ns); ··· 887 884 * or it was cancelled prematurely so that we do not guarantee that 888 885 * the target is a copy of the source. 889 886 */ 890 - assert(ret < 0 || (!s->synced && block_job_is_cancelled(&s->common))); 887 + assert(ret < 0 || ((s->common.force || !s->synced) && 888 + block_job_is_cancelled(&s->common))); 891 889 assert(need_drain); 892 890 mirror_wait_for_all_io(s); 893 891 }
+2 -2
blockdev.c
··· 150 150 aio_context_acquire(aio_context); 151 151 152 152 if (bs->job) { 153 - block_job_cancel(bs->job); 153 + block_job_cancel(bs->job, false); 154 154 } 155 155 156 156 aio_context_release(aio_context); ··· 3850 3850 } 3851 3851 3852 3852 trace_qmp_block_job_cancel(job); 3853 - block_job_user_cancel(job, errp); 3853 + block_job_user_cancel(job, force, errp); 3854 3854 out: 3855 3855 aio_context_release(aio_context); 3856 3856 }
+9 -7
blockjob.c
··· 487 487 return 0; 488 488 } 489 489 490 - static void block_job_cancel_async(BlockJob *job) 490 + static void block_job_cancel_async(BlockJob *job, bool force) 491 491 { 492 492 if (job->iostatus != BLOCK_DEVICE_IO_STATUS_OK) { 493 493 block_job_iostatus_reset(job); ··· 498 498 job->pause_count--; 499 499 } 500 500 job->cancelled = true; 501 + /* To prevent 'force == false' overriding a previous 'force == true' */ 502 + job->force |= force; 501 503 } 502 504 503 505 static int block_job_txn_apply(BlockJobTxn *txn, int fn(BlockJob *), bool lock) ··· 581 583 * on the caller, so leave it. */ 582 584 QLIST_FOREACH(other_job, &txn->jobs, txn_list) { 583 585 if (other_job != job) { 584 - block_job_cancel_async(other_job); 586 + block_job_cancel_async(other_job, false); 585 587 } 586 588 } 587 589 while (!QLIST_EMPTY(&txn->jobs)) { ··· 747 749 block_job_resume(job); 748 750 } 749 751 750 - void block_job_cancel(BlockJob *job) 752 + void block_job_cancel(BlockJob *job, bool force) 751 753 { 752 754 if (job->status == BLOCK_JOB_STATUS_CONCLUDED) { 753 755 block_job_do_dismiss(job); 754 756 return; 755 757 } 756 - block_job_cancel_async(job); 758 + block_job_cancel_async(job, force); 757 759 if (!block_job_started(job)) { 758 760 block_job_completed(job, -ECANCELED); 759 761 } else if (job->deferred_to_main_loop) { ··· 763 765 } 764 766 } 765 767 766 - void block_job_user_cancel(BlockJob *job, Error **errp) 768 + void block_job_user_cancel(BlockJob *job, bool force, Error **errp) 767 769 { 768 770 if (block_job_apply_verb(job, BLOCK_JOB_VERB_CANCEL, errp)) { 769 771 return; 770 772 } 771 - block_job_cancel(job); 773 + block_job_cancel(job, force); 772 774 } 773 775 774 776 /* A wrapper around block_job_cancel() taking an Error ** parameter so it may be ··· 776 778 * function pointer casts there. */ 777 779 static void block_job_cancel_err(BlockJob *job, Error **errp) 778 780 { 779 - block_job_cancel(job); 781 + block_job_cancel(job, false); 780 782 } 781 783 782 784 int block_job_cancel_sync(BlockJob *job)
+2 -1
hmp-commands.hx
··· 106 106 .args_type = "force:-f,device:B", 107 107 .params = "[-f] device", 108 108 .help = "stop an active background block operation (use -f" 109 - "\n\t\t\t if the operation is currently paused)", 109 + "\n\t\t\t if you want to abort the operation immediately" 110 + "\n\t\t\t instead of keep running until data is in sync)", 110 111 .cmd = hmp_block_job_cancel, 111 112 }, 112 113
+10 -2
include/block/blockjob.h
··· 63 63 bool cancelled; 64 64 65 65 /** 66 + * Set to true if the job should abort immediately without waiting 67 + * for data to be in sync. 68 + */ 69 + bool force; 70 + 71 + /** 66 72 * Counter for pause request. If non-zero, the block job is either paused, 67 73 * or if busy == true will pause itself as soon as possible. 68 74 */ ··· 230 236 /** 231 237 * block_job_cancel: 232 238 * @job: The job to be canceled. 239 + * @force: Quit a job without waiting for data to be in sync. 233 240 * 234 241 * Asynchronously cancel the specified job. 235 242 */ 236 - void block_job_cancel(BlockJob *job); 243 + void block_job_cancel(BlockJob *job, bool force); 237 244 238 245 /** 239 246 * block_job_complete: ··· 307 314 /** 308 315 * block_job_user_cancel: 309 316 * @job: The job to be cancelled. 317 + * @force: Quit a job without waiting for data to be in sync. 310 318 * 311 319 * Cancels the specified job, but may refuse to do so if the 312 320 * operation isn't currently meaningful. 313 321 */ 314 - void block_job_user_cancel(BlockJob *job, Error **errp); 322 + void block_job_user_cancel(BlockJob *job, bool force, Error **errp); 315 323 316 324 /** 317 325 * block_job_cancel_sync:
+3 -2
qapi/block-core.json
··· 2207 2207 # the name of the parameter), but since QEMU 2.7 it can have 2208 2208 # other values. 2209 2209 # 2210 - # @force: whether to allow cancellation of a paused job (default 2211 - # false). Since 1.3. 2210 + # @force: If true, and the job has already emitted the event BLOCK_JOB_READY, 2211 + # abandon the job immediately (even if it is paused) instead of waiting 2212 + # for the destination to complete its final synchronization (since 1.3) 2212 2213 # 2213 2214 # Returns: Nothing on success 2214 2215 # If no background operation is active on this device, DeviceNotActive
+4 -4
tests/test-blockjob-txn.c
··· 124 124 block_job_start(job); 125 125 126 126 if (expected == -ECANCELED) { 127 - block_job_cancel(job); 127 + block_job_cancel(job, false); 128 128 } 129 129 130 130 while (result == -EINPROGRESS) { ··· 170 170 block_job_txn_unref(txn); 171 171 172 172 if (expected1 == -ECANCELED) { 173 - block_job_cancel(job1); 173 + block_job_cancel(job1, false); 174 174 } 175 175 if (expected2 == -ECANCELED) { 176 - block_job_cancel(job2); 176 + block_job_cancel(job2, false); 177 177 } 178 178 179 179 while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) { ··· 226 226 block_job_start(job1); 227 227 block_job_start(job2); 228 228 229 - block_job_cancel(job1); 229 + block_job_cancel(job1, false); 230 230 231 231 /* Now make job2 finish before the main loop kicks jobs. This simulates 232 232 * the race between a pending kick and another job completing.