Re: [Qemu-devel] [PATCH v3 3/6] blockjob: add .start field

From: Jeff Cody <jcody@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: qemu-block@nongnu.org, kwolf@redhat.com,
	vsementsov@virtuozzo.com, stefanha@redhat.com,
	pbonzini@redhat.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v3 3/6] blockjob: add .start field
Date: Mon, 7 Nov 2016 21:58:05 -0500	[thread overview]
Message-ID: <20161108025804.GE23932@localhost.localdomain> (raw)
In-Reply-To: <1478109056-25198-4-git-send-email-jsnow@redhat.com>

On Wed, Nov 02, 2016 at 01:50:53PM -0400, John Snow wrote:
> Add an explicit start field to specify the entrypoint. We already have
> ownership of the coroutine itself AND managing the lifetime of the
> coroutine, let's take control of creation of the coroutine, too.
> 
> This will allow us to delay creation of the actual coroutine until we
> know we'll actually start a BlockJob in block_job_start. This avoids
> the sticky question of how to "un-create" a Coroutine that hasn't been
> started yet.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c               | 25 +++++++++++++------------
>  block/commit.c               |  3 ++-
>  block/mirror.c               |  4 +++-
>  block/stream.c               |  3 ++-
>  include/block/blockjob_int.h |  3 +++
>  5 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 734a24c..4ed4494 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -323,17 +323,6 @@ static void backup_drain(BlockJob *job)
>      }
>  }
>  
> -static const BlockJobDriver backup_job_driver = {
> -    .instance_size          = sizeof(BackupBlockJob),
> -    .job_type               = BLOCK_JOB_TYPE_BACKUP,
> -    .set_speed              = backup_set_speed,
> -    .commit                 = backup_commit,
> -    .abort                  = backup_abort,
> -    .clean                  = backup_clean,
> -    .attached_aio_context   = backup_attached_aio_context,
> -    .drain                  = backup_drain,
> -};
> -
>  static BlockErrorAction backup_error_action(BackupBlockJob *job,
>                                              bool read, int error)
>  {
> @@ -542,6 +531,18 @@ static void coroutine_fn backup_run(void *opaque)
>      block_job_defer_to_main_loop(&job->common, backup_complete, data);
>  }
>  
> +static const BlockJobDriver backup_job_driver = {
> +    .instance_size          = sizeof(BackupBlockJob),
> +    .job_type               = BLOCK_JOB_TYPE_BACKUP,
> +    .start                  = backup_run,
> +    .set_speed              = backup_set_speed,
> +    .commit                 = backup_commit,
> +    .abort                  = backup_abort,
> +    .clean                  = backup_clean,
> +    .attached_aio_context   = backup_attached_aio_context,
> +    .drain                  = backup_drain,
> +};
> +

Some code movement here in addition to the .start addition, but to a better
place (I am guessing that is intentional).

>  void backup_start(const char *job_id, BlockDriverState *bs,
>                    BlockDriverState *target, int64_t speed,
>                    MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
> @@ -653,7 +654,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
>  
>      block_job_add_bdrv(&job->common, target);
>      job->common.len = len;
> -    job->common.co = qemu_coroutine_create(backup_run, job);
> +    job->common.co = qemu_coroutine_create(job->common.driver->start, job);
>      block_job_txn_add_job(txn, &job->common);
>      qemu_coroutine_enter(job->common.co);
>      return;
> diff --git a/block/commit.c b/block/commit.c
> index e1eda89..20d27e2 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -205,6 +205,7 @@ static const BlockJobDriver commit_job_driver = {
>      .instance_size = sizeof(CommitBlockJob),
>      .job_type      = BLOCK_JOB_TYPE_COMMIT,
>      .set_speed     = commit_set_speed,
> +    .start         = commit_run,
>  };
>  
>  void commit_start(const char *job_id, BlockDriverState *bs,
> @@ -288,7 +289,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
>      s->backing_file_str = g_strdup(backing_file_str);
>  
>      s->on_error = on_error;
> -    s->common.co = qemu_coroutine_create(commit_run, s);
> +    s->common.co = qemu_coroutine_create(s->common.driver->start, s);
>  
>      trace_commit_start(bs, base, top, s, s->common.co);
>      qemu_coroutine_enter(s->common.co);
> diff --git a/block/mirror.c b/block/mirror.c
> index b2c1fb8..659e09c 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -920,6 +920,7 @@ static const BlockJobDriver mirror_job_driver = {
>      .instance_size          = sizeof(MirrorBlockJob),
>      .job_type               = BLOCK_JOB_TYPE_MIRROR,
>      .set_speed              = mirror_set_speed,
> +    .start                  = mirror_run,
>      .complete               = mirror_complete,
>      .pause                  = mirror_pause,
>      .attached_aio_context   = mirror_attached_aio_context,
> @@ -930,6 +931,7 @@ static const BlockJobDriver commit_active_job_driver = {
>      .instance_size          = sizeof(MirrorBlockJob),
>      .job_type               = BLOCK_JOB_TYPE_COMMIT,
>      .set_speed              = mirror_set_speed,
> +    .start                  = mirror_run,
>      .complete               = mirror_complete,
>      .pause                  = mirror_pause,
>      .attached_aio_context   = mirror_attached_aio_context,
> @@ -1007,7 +1009,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>          }
>      }
>  
> -    s->common.co = qemu_coroutine_create(mirror_run, s);
> +    s->common.co = qemu_coroutine_create(s->common.driver->start, s);
>      trace_mirror_start(bs, s, s->common.co, opaque);
>      qemu_coroutine_enter(s->common.co);
>  }
> diff --git a/block/stream.c b/block/stream.c
> index b05856b..92309ff 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -218,6 +218,7 @@ static const BlockJobDriver stream_job_driver = {
>      .instance_size = sizeof(StreamBlockJob),
>      .job_type      = BLOCK_JOB_TYPE_STREAM,
>      .set_speed     = stream_set_speed,
> +    .start         = stream_run,
>  };
>  
>  void stream_start(const char *job_id, BlockDriverState *bs,
> @@ -254,7 +255,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>      s->bs_flags = orig_bs_flags;
>  
>      s->on_error = on_error;
> -    s->common.co = qemu_coroutine_create(stream_run, s);
> +    s->common.co = qemu_coroutine_create(s->common.driver->start, s);
>      trace_stream_start(bs, base, s, s->common.co);
>      qemu_coroutine_enter(s->common.co);
>  }
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 60d91a0..8223822 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -47,6 +47,9 @@ struct BlockJobDriver {
>      /** Optional callback for job types that need to forward I/O status reset */
>      void (*iostatus_reset)(BlockJob *job);
>  
> +    /** Mandatory: Entrypoint for the Coroutine. */
> +    CoroutineEntry *start;
> +
>      /**
>       * Optional callback for job types whose completion must be triggered
>       * manually.
> -- 
> 2.7.4
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>