Re: [PATCH v5 08/13] job: Add job_cancel_requested()

[Eric Blake]

On Wed, Oct 06, 2021 at 05:19:35PM +0200, Hanna Reitz wrote:
> Most callers of job_is_cancelled() actually want to know whether the job
> is on its way to immediate termination.  For example, we refuse to pause
> jobs that are cancelled; but this only makes sense for jobs that are
> really actually cancelled.
> 
> A mirror job that is cancelled during READY with force=false should
> absolutely be allowed to pause.  This "cancellation" (which is actually
> a kind of completion) may take an indefinite amount of time, and so
> should behave like any job during normal operation.  For example, with
> on-target-error=stop, the job should stop on write errors.  (In
> contrast, force-cancelled jobs should not get write errors, as they
> should just terminate and not do further I/O.)
> 
> Therefore, redefine job_is_cancelled() to only return true for jobs that
> are force-cancelled (which as of HEAD^ means any job that interprets the

Is this reference still accurate with the addition of new patch 7?

> cancellation request as a request for immediate termination), and add
> job_cancel_requested() as the general variant, which returns true for
> any jobs which have been requested to be cancelled, whether it be
> immediately or after an arbitrarily long completion phase.
> 
> Finally, here is a justification for how different job_is_cancelled()
> invocations are treated by this patch:
...
>   - job_update_rc(), job_finalize_single(), job_finish_sync(): These
>     functions are all called after the job has left its main loop.  The
>     mirror job (the only job that can be soft-cancelled) will clear
>     .cancelled before leaving the main loop if it has been
>     soft-cancelled.  Therefore, these functions will observe .cancelled
>     to be true only if the job has been force-cancelled.  We can
>     continue to use job_is_cancelled().
>     (Furthermore, conceptually, a soft-cancelled mirror job should not
>     report to have been cancelled.  It should report completion (see
>     also the block-job-cancel QAPI documentation).  Therefore, it makes
>     sense for these functions not to distinguish between a
>     soft-cancelled mirror job and a job that has completed as normal.)

Thanks for the careful explanation.

> 
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  include/qemu/job.h |  8 +++++++-
>  block/mirror.c     | 10 ++++------
>  job.c              | 14 ++++++++++++--
>  3 files changed, 23 insertions(+), 9 deletions(-)

The commit message is loads longer than the patch itself, but for good
reason.

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

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