Am 26.07.2021 um 16:46 hat Max Reitz geschrieben: > 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 > cancellation request as a request for immediate termination), and add > job_cancel_request() 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. > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462 > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > include/qemu/job.h | 8 +++++++- > block/mirror.c | 10 ++++------ > job.c | 7 ++++++- > 3 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 8aa90f7395..032edf3c5f 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -436,9 +436,15 @@ const char *job_type_str(const Job *job); > /** Returns true if the job should not be visible to the management layer. */ > bool job_is_internal(Job *job); > > -/** Returns whether the job is scheduled for cancellation. */ > +/** Returns whether the job is being cancelled. */ > bool job_is_cancelled(Job *job); > > +/** > + * Returns whether the job is scheduled for cancellation (at an > + * indefinite point). > + */ > +bool job_cancel_requested(Job *job);
I don't think non-force blockdev-cancel for mirror should actually be considered cancellation, so what is the question that this function answers? "Is this a cancelled job, or a mirror block job that is supposed to complete soon, but only if it doesn't switch over the users to the target on completion"? Is this ever a reasonable question to ask, except maybe inside the mirror implementation itself? job_complete() is the only function outside of mirror that seems to use it. But even there, it feels wrong to make a difference. Either we accept redundant completion requests, or we don't. It doesn't really matter how the job reconfigures the graph on completion. (Also, I feel this should really have been part of the state machine, but I'm not sure if we want to touch it now...) Kevin