Re: [Qemu-devel] [PATCH 18/42] job: Move coroutine and related code to Job

[John Snow]

On 05/09/2018 12:26 PM, Kevin Wolf wrote:
> This commit moves some core functions for dealing with the job coroutine
> from BlockJob to Job. This includes primarily entering the coroutine
> (both for the first and reentering) and yielding explicitly and at pause
> points.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  include/block/blockjob.h     |  40 ---------
>  include/block/blockjob_int.h |  26 ------
>  include/qemu/job.h           |  76 ++++++++++++++++
>  block/backup.c               |   2 +-
>  block/commit.c               |   4 +-
>  block/mirror.c               |  22 ++---
>  block/replication.c          |   2 +-
>  block/stream.c               |   4 +-
>  blockdev.c                   |   8 +-
>  blockjob.c                   | 201 
> +++++++------------------------------------
>  job.c                        | 137 +++++++++++++++++++++++++++++
>  tests/test-bdrv-drain.c      |  38 ++++----
>  tests/test-blockjob-txn.c    |  12 +--
>  tests/test-blockjob.c        |  14 +--
>  14 files changed, 296 insertions(+), 290 deletions(-)
> 

[...]

>  
>  /* Assumes the block_job_mutex is held */
> -static bool block_job_timer_pending(BlockJob *job)
> +static bool job_timer_pending(Job *job)

Is this intentionally left behind in blockjob.c, even once you've
changed the name (and moved the state to job.c?)

[...]

> +void job_start(Job *job)
> +{
> +    assert(job && !job_started(job) && job->paused &&
> +           job->driver && job->driver->start);
> +    job->co = qemu_coroutine_create(job_co_entry, job);
> +    job->pause_count--;
> +    job->busy = true;
> +    job->paused = false;
> +    job_state_transition(job, JOB_STATUS_RUNNING);
> +    aio_co_enter(job->aio_context, job->co);

I suppose patch 16 was the time to ask this, but are there any
detriments to setting the AIO Context at creation time and then using
that consistently instead of just fetching the AIO context of the BDS
whenever we need it?

I guess if the context changes then we've got it covered in
block_job_attached_aio_context.

Reviewed-by: John Snow <address@hidden>