[Top][All Lists]

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

Re: [PATCH for-6.1? 4/6] job: Add job_cancel_requested()

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH for-6.1? 4/6] job: Add job_cancel_requested()
Date: Thu, 22 Jul 2021 20:58:22 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0

22.07.2021 15:26, Max 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)

You have to repeat that this "cancel" is not "cancel".

So, the whole problem is that feature of mirror, on cancel in READY state do 
not cancel but do some specific kind of completion.

You try to make this thing correctly handled on generic layer..

Did you consider instead just drop the feature from generic layer? So that all 
*cancel* functions always do force-cancel. Then the internal implementation 
become a lot clearer.

But we have to support the qmp block-job-cancel of READY mirror (and commit) 
with force=false.

We can do it as an exclusion in qmp_block_job_cancel, something like:

if (job is mirror or commit AND it's ready AND force = false)

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 <mreitz@redhat.com>


--- a/job.c
+++ b/job.c
@@ -216,6 +216,11 @@ const char *job_type_str(const Job *job)
bool job_is_cancelled(Job *job)
+    return job->cancelled && job->force_cancel;
+bool job_cancel_requested(Job *job)
      return job->cancelled;
@@ -650,7 +655,7 @@ static void job_conclude(Job *job)
static void job_update_rc(Job *job)
-    if (!job->ret && job_is_cancelled(job)) {
+    if (!job->ret && job_cancel_requested(job)) {

Why not job_is_cancelled() here?

So in case of mirror other kind of completion we set ret to -ECANCELED?

          job->ret = -ECANCELED;
      if (job->ret) {
@@ -704,7 +709,7 @@ static int job_finalize_single(Job *job)
/* Emit events only if we actually started */
      if (job_started(job)) {
-        if (job_is_cancelled(job)) {
+        if (job_cancel_requested(job)) {

Same question here.. Shouldn't mirror report COMPLETED event in case of 
not-force cancelled in READY state?

          } else {
@@ -1015,7 +1020,7 @@ void job_complete(Job *job, Error **errp)
      if (job_apply_verb(job, JOB_VERB_COMPLETE, errp)) {
-    if (job_is_cancelled(job) || !job->driver->complete) {
+    if (job_cancel_requested(job) || !job->driver->complete) {
          error_setg(errp, "The active block job '%s' cannot be completed",
@@ -1043,7 +1048,7 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error 
**errp), Error **errp)
                     (job_enter(job), !job_is_completed(job)));
- ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
+    ret = (job_cancel_requested(job) && job->ret == 0) ? -ECANCELED : job->ret;
      return ret;

Best regards,

reply via email to

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