Re: [Qemu-devel] [PATCH 03/10] blockjob: introduce block_job_fail

[John Snow <jsnow@redhat.com>]

On 03/23/2017 01:39 PM, Paolo Bonzini wrote:
> Outside blockjob.c, block_job_unref is only used when a block job fails
> to start, and block_job_ref is not used at all.  The reference counting
> thus is pretty well hidden.  Introduce a separate function to be used
> by block jobs; because block_job_ref and block_job_unref now become
> static, move them earlier in blockjob.c.
> 

Early into the series, "I guess so."

It makes it a lot less obvious and more vague as to what exactly is
happening here, so I like it less.

> Later on, block_job_fail will also have different locking than
> block_job_unref.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/backup.c               |  2 +-
>  block/commit.c               |  2 +-
>  block/mirror.c               |  2 +-
>  blockjob.c                   | 47 ++++++++++++++++++++++++++------------------
>  include/block/blockjob_int.h | 15 +++-----------
>  tests/test-blockjob.c        | 10 +++++-----
>  6 files changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index a4fb288..f284fd5 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -692,7 +692,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>      }
>      if (job) {
>          backup_clean(&job->common);
> -        block_job_unref(&job->common);
> +        block_job_fail(&job->common);
>      }
>  
>      return NULL;
> diff --git a/block/commit.c b/block/commit.c
> index 2832482..9b9ea39 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -424,7 +424,7 @@ fail:
>      if (commit_top_bs) {
>          bdrv_set_backing_hd(overlay_bs, top, &error_abort);
>      }
> -    block_job_unref(&s->common);
> +    block_job_fail(&s->common);
>  }
>  
>  
> diff --git a/block/mirror.c b/block/mirror.c
> index ca4baa5..5cb2134 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1243,7 +1243,7 @@ fail:
>      if (s) {
>          g_free(s->replaces);
>          blk_unref(s->target);
> -        block_job_unref(&s->common);
> +        block_job_fail(&s->common);
>      }
>  
>      bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> diff --git a/blockjob.c b/blockjob.c
> index 24d1e12..1c63d15 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -106,6 +106,32 @@ BlockJob *block_job_get(const char *id)
>      return NULL;
>  }
>  
> +static void block_job_ref(BlockJob *job)
> +{
> +    ++job->refcnt;
> +}
> +
> +static void block_job_attached_aio_context(AioContext *new_context,
> +                                           void *opaque);
> +static void block_job_detach_aio_context(void *opaque);
> +

^ Some tagalongs to the party?

> +static void block_job_unref(BlockJob *job)
> +{
> +    if (--job->refcnt == 0) {
> +        BlockDriverState *bs = blk_bs(job->blk);
> +        bs->job = NULL;
> +        block_job_remove_all_bdrv(job);
> +        blk_remove_aio_context_notifier(job->blk,
> +                                        block_job_attached_aio_context,
> +                                        block_job_detach_aio_context, job);
> +        blk_unref(job->blk);
> +        error_free(job->blocker);
> +        g_free(job->id);
> +        QLIST_REMOVE(job, job_list);
> +        g_free(job);
> +    }
> +}
> +
>  static void block_job_attached_aio_context(AioContext *new_context,
>                                             void *opaque)
>  {
> @@ -293,26 +319,9 @@ void block_job_start(BlockJob *job)
>      qemu_coroutine_enter(job->co);
>  }
>  
> -void block_job_ref(BlockJob *job)
> -{
> -    ++job->refcnt;
> -}
> -
> -void block_job_unref(BlockJob *job)
> +void block_job_fail(BlockJob *job)
>  {
> -    if (--job->refcnt == 0) {
> -        BlockDriverState *bs = blk_bs(job->blk);
> -        bs->job = NULL;
> -        block_job_remove_all_bdrv(job);
> -        blk_remove_aio_context_notifier(job->blk,
> -                                        block_job_attached_aio_context,
> -                                        block_job_detach_aio_context, job);
> -        blk_unref(job->blk);
> -        error_free(job->blocker);
> -        g_free(job->id);
> -        QLIST_REMOVE(job, job_list);
> -        g_free(job);
> -    }
> +    block_job_unref(job);
>  }
>  
>  static void block_job_completed_single(BlockJob *job)
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index bfcc5d1..97ffc43 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -156,21 +156,12 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
>  void block_job_yield(BlockJob *job);
>  
>  /**
> - * block_job_ref:
> + * block_job_fail:
>   * @bs: The block device.
>   *
> - * Grab a reference to the block job. Should be paired with block_job_unref.
> + * The block job could not be started, free it.
>   */
> -void block_job_ref(BlockJob *job);
> -
> -/**
> - * block_job_unref:
> - * @bs: The block device.
> - *
> - * Release reference to the block job and release resources if it is the last
> - * reference.
> - */
> -void block_job_unref(BlockJob *job);
> +void block_job_fail(BlockJob *job);
>  
>  /**
>   * block_job_completed:
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 740e740..35bbbbc 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -116,11 +116,11 @@ static void test_job_ids(void)
>      job[1] = do_test_id(blk[1], "id0", false);
>  
>      /* But once job[0] finishes we can reuse its ID */
> -    block_job_unref(job[0]);
> +    block_job_fail(job[0]);
>      job[1] = do_test_id(blk[1], "id0", true);
>  
>      /* No job ID specified, defaults to the backend name ('drive1') */
> -    block_job_unref(job[1]);
> +    block_job_fail(job[1]);
>      job[1] = do_test_id(blk[1], NULL, true);
>  
>      /* Duplicate job ID */
> @@ -133,9 +133,9 @@ static void test_job_ids(void)
>      /* This one is valid */
>      job[2] = do_test_id(blk[2], "id_2", true);
>  
> -    block_job_unref(job[0]);
> -    block_job_unref(job[1]);
> -    block_job_unref(job[2]);
> +    block_job_fail(job[0]);
> +    block_job_fail(job[1]);
> +    block_job_fail(job[2]);
>  
>      destroy_blk(blk[0]);
>      destroy_blk(blk[1]);
> 

Mechanically OK, of course. Allow me to discover my appetite for it
further into your series.