Skip to content
  • Hanna Reitz's avatar
    08b83bff
    job: Add job_cancel_requested() · 08b83bff
    Hanna Reitz authored
    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_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:
    
    - block/mirror.c (mirror_run()):
      - The first invocation is a while loop that should loop until the job
        has been cancelled or scheduled for completion.  What kind of cancel
        does not matter, only the fact that the job is supposed to end.
    
      - The second invocation wants to know whether the job has been
        soft-cancelled.  Calling job_cancel_requested() is a bit too broad,
        but if the job were force-cancelled, we should leave the main loop
        as soon as possible anyway, so this should not matter here.
    
      - The last two invocations already check force_cancel, so they should
        continue to use job_is_cancelled().
    
    - block/backup.c, block/commit.c, block/stream.c, anything in tests/:
      These jobs know only force-cancel, so there is no difference between
      job_is_cancelled() and job_cancel_requested().  We can continue using
      job_is_cancelled().
    
    - job.c:
      - job_pause_point(), job_yield(), job_sleep_ns(): Only force-cancelled
        jobs should be prevented from being paused.  Continue using job_is_cancelled().
    
      - 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.)
    
      - job_completed_txn_abort(): All jobs other than @job have been
        force-cancelled.  job_is_cancelled() must be true for them.
        Regarding @job itself: job_completed_txn_abort() is mostly called
        when the job's return value is not 0.  A soft-cancelled mirror has a
        return value of 0, and so will not end up here then.
        However, job_cancel() invokes job_completed_txn_abort() if the job
        has been deferred to the main loop, which is mostly the case for
        completed jobs (which skip the assertion), but not for sure.
        To be safe, use job_cancel_requested() in this assertion.
    
      - job_complete(): This is function eventually invoked by the user
        (through qmp_block_job_complete() or qmp_job_complete(), or
        job_complete_sync(), which comes from qemu-img).  The intention here
        is to prevent a user from invoking job-complete after the job has
        been cancelled.  This should also apply to soft cancelling: After a
        mirror job has been soft-cancelled, the user should not be able to
        decide otherwise and have it complete as normal (i.e. pivoting to
        the target).
    
      - job_cancel(): Both functions are equivalent (see comment there), but
        we want to use job_is_cancelled(), because this shows that we call
        job_completed_txn_abort() only for force-cancelled jobs.  (As
        explained for job_update_rc(), soft-cancelled jobs should be treated
        as if they have completed as normal.)
    
    Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
    
    
    Signed-off-by: default avatarHanna Reitz <hreitz@redhat.com>
    Reviewed-by: default avatarEric Blake <eblake@redhat.com>
    Reviewed-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    Message-Id: <20211006151940.214590-9-hreitz@redhat.com>
    Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    08b83bff
    job: Add job_cancel_requested()
    Hanna Reitz authored
    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_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:
    
    - block/mirror.c (mirror_run()):
      - The first invocation is a while loop that should loop until the job
        has been cancelled or scheduled for completion.  What kind of cancel
        does not matter, only the fact that the job is supposed to end.
    
      - The second invocation wants to know whether the job has been
        soft-cancelled.  Calling job_cancel_requested() is a bit too broad,
        but if the job were force-cancelled, we should leave the main loop
        as soon as possible anyway, so this should not matter here.
    
      - The last two invocations already check force_cancel, so they should
        continue to use job_is_cancelled().
    
    - block/backup.c, block/commit.c, block/stream.c, anything in tests/:
      These jobs know only force-cancel, so there is no difference between
      job_is_cancelled() and job_cancel_requested().  We can continue using
      job_is_cancelled().
    
    - job.c:
      - job_pause_point(), job_yield(), job_sleep_ns(): Only force-cancelled
        jobs should be prevented from being paused.  Continue using job_is_cancelled().
    
      - 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.)
    
      - job_completed_txn_abort(): All jobs other than @job have been
        force-cancelled.  job_is_cancelled() must be true for them.
        Regarding @job itself: job_completed_txn_abort() is mostly called
        when the job's return value is not 0.  A soft-cancelled mirror has a
        return value of 0, and so will not end up here then.
        However, job_cancel() invokes job_completed_txn_abort() if the job
        has been deferred to the main loop, which is mostly the case for
        completed jobs (which skip the assertion), but not for sure.
        To be safe, use job_cancel_requested() in this assertion.
    
      - job_complete(): This is function eventually invoked by the user
        (through qmp_block_job_complete() or qmp_job_complete(), or
        job_complete_sync(), which comes from qemu-img).  The intention here
        is to prevent a user from invoking job-complete after the job has
        been cancelled.  This should also apply to soft cancelling: After a
        mirror job has been soft-cancelled, the user should not be able to
        decide otherwise and have it complete as normal (i.e. pivoting to
        the target).
    
      - job_cancel(): Both functions are equivalent (see comment there), but
        we want to use job_is_cancelled(), because this shows that we call
        job_completed_txn_abort() only for force-cancelled jobs.  (As
        explained for job_update_rc(), soft-cancelled jobs should be treated
        as if they have completed as normal.)
    
    Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
    
    
    Signed-off-by: default avatarHanna Reitz <hreitz@redhat.com>
    Reviewed-by: default avatarEric Blake <eblake@redhat.com>
    Reviewed-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    Message-Id: <20211006151940.214590-9-hreitz@redhat.com>
    Signed-off-by: default avatarVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Loading