Re: [PATCH for-6.1? v2 4/7] jobs: Give Job.force_cancel more meaning

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

26.07.2021 17:46, Max Reitz wrote:
> We largely have two cancel modes for jobs:
> 
> First, there is actual cancelling.  The job is terminated as soon as
> possible, without trying to reach a consistent result.
> 
> Second, we have mirror in the READY state.  Technically, the job is not
> really cancelled, but it just is a different completion mode.  The job
> can still run for an indefinite amount of time while it tries to reach a
> consistent result.
> 
> We want to be able to clearly distinguish which cancel mode a job is in
> (when it has been cancelled).  We can use Job.force_cancel for this, but
> right now it only reflects cancel requests from the user with
> force=true, but clearly, jobs that do not even distinguish between
> force=false and force=true are effectively always force-cancelled.
> 
> So this patch has Job.force_cancel signify whether the job will
> terminate as soon as possible (force_cancel=true) or whether it will
> effectively remain running despite being "cancelled"
> (force_cancel=false).
> 
> To this end, we let jobs that provide JobDriver.cancel() tell the
> generic job code whether they will terminate as soon as possible or not,
> and for jobs that do not provide that method we assume they will.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/qemu/job.h | 11 ++++++++++-
>   block/backup.c     |  3 ++-
>   block/mirror.c     | 24 ++++++++++++++++++------
>   job.c              |  6 +++++-
>   4 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 5e8edbc2c8..8aa90f7395 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -253,8 +253,17 @@ struct JobDriver {
>   
>       /**
>        * If the callback is not NULL, it will be invoked in job_cancel_async
> +     *
> +     * This function must return true if the job will be cancelled
> +     * immediately without any further I/O (mandatory if @force is
> +     * true), and false otherwise.  This lets the generic job layer
> +     * know whether a job has been truly (force-)cancelled, or whether
> +     * it is just in a special completion mode (like mirror after
> +     * READY).
> +     * (If the callback is NULL, the job is assumed to terminate
> +     * without I/O.)
>        */
> -    void (*cancel)(Job *job, bool force);
> +    bool (*cancel)(Job *job, bool force);
>   
>   
>       /** Called when the job is freed */
> diff --git a/block/backup.c b/block/backup.c
> index bd3614ce70..513e1c8a0b 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -331,11 +331,12 @@ static void coroutine_fn backup_set_speed(BlockJob *job, int64_t speed)
>       }
>   }
>   
> -static void backup_cancel(Job *job, bool force)
> +static bool backup_cancel(Job *job, bool force)
>   {
>       BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>   
>       bdrv_cancel_in_flight(s->target_bs);
> +    return true;
>   }
>   
>   static const BlockJobDriver backup_job_driver = {
> diff --git a/block/mirror.c b/block/mirror.c
> index fcb7b65f93..e93631a9f6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1087,9 +1087,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>           trace_mirror_before_sleep(s, cnt, job_is_ready(&s->common.job),
>                                     delay_ns);
>           job_sleep_ns(&s->common.job, delay_ns);
> -        if (job_is_cancelled(&s->common.job) &&
> -            (!job_is_ready(&s->common.job) || s->common.job.force_cancel))
> -        {
> +        if (job_is_cancelled(&s->common.job) && s->common.job.force_cancel) {

Seems, it could it be reduced to

if (s->common.job.force_cancel) {


>               break;
>           }
>           s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> @@ -1102,7 +1100,7 @@ immediate_exit:
>            * the target is a copy of the source.
>            */
>           assert(ret < 0 ||
> -               ((s->common.job.force_cancel || !job_is_ready(&s->common.job)) &&
> +               (s->common.job.force_cancel &&

and here

>                   job_is_cancelled(&s->common.job)));
>           assert(need_drain);
>           mirror_wait_for_all_io(s);
> @@ -1188,14 +1186,27 @@ static bool mirror_drained_poll(BlockJob *job)
>       return !!s->in_flight;
>   }
>   

anyway:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir