Re: [Qemu-devel] [PATCH 04/10] blockjob: introduce block_job_pause/resume_all

[John Snow]

On 03/23/2017 01:39 PM, Paolo Bonzini wrote:
> Remove use of block_job_pause/resume from outside blockjob.c, thus
> making them static.  Again, one reason to do this is that
> block_job_pause/resume will have different locking rules compared
> to everything else that block.c and block/io.c use.
> 

Ominous

> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/io.c               |  18 +-------
>  blockjob.c               | 114 
> ++++++++++++++++++++++++++++-------------------
>  include/block/blockjob.h |  14 +++---
>  3 files changed, 77 insertions(+), 69 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 2709a70..1cc9318 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -284,16 +284,9 @@ void bdrv_drain_all_begin(void)
>      bool waited = true;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
> -    BlockJob *job = NULL;
>      GSList *aio_ctxs = NULL, *ctx;
>  
> -    while ((job = block_job_next(job))) {
> -        AioContext *aio_context = blk_get_aio_context(job->blk);
> -
> -        aio_context_acquire(aio_context);
> -        block_job_pause(job);
> -        aio_context_release(aio_context);
> -    }
> +    block_job_pause_all();
>  

Cool. Well, maybe cool. I think it would be cool if we didn't have to
explicitly pause jobs here at all. This is better at least, though.

>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -337,7 +330,6 @@ void bdrv_drain_all_end(void)
>  {
>      BlockDriverState *bs;
>      BdrvNextIterator it;
> -    BlockJob *job = NULL;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -348,13 +340,7 @@ void bdrv_drain_all_end(void)
>          aio_context_release(aio_context);
>      }
>  
> -    while ((job = block_job_next(job))) {
> -        AioContext *aio_context = blk_get_aio_context(job->blk);
> -
> -        aio_context_acquire(aio_context);
> -        block_job_resume(job);
> -        aio_context_release(aio_context);
> -    }
> +    block_job_resume_all();
>  }
>  
>  void bdrv_drain_all(void)
> diff --git a/blockjob.c b/blockjob.c
> index 1c63d15..caca718 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -55,36 +55,6 @@ struct BlockJobTxn {
>  
>  static QLIST_HEAD(, BlockJob) block_jobs = 
> QLIST_HEAD_INITIALIZER(block_jobs);
>  
> -static char *child_job_get_parent_desc(BdrvChild *c)
> -{
> -    BlockJob *job = c->opaque;
> -    return g_strdup_printf("%s job '%s'",
> -                           BlockJobType_lookup[job->driver->job_type],
> -                           job->id);
> -}
> -
> -static const BdrvChildRole child_job = {
> -    .get_parent_desc    = child_job_get_parent_desc,
> -    .stay_at_node       = true,
> -};
> -
> -static void block_job_drained_begin(void *opaque)
> -{
> -    BlockJob *job = opaque;
> -    block_job_pause(job);
> -}
> -
> -static void block_job_drained_end(void *opaque)
> -{
> -    BlockJob *job = opaque;
> -    block_job_resume(job);
> -}
> -
> -static const BlockDevOps block_job_dev_ops = {
> -    .drained_begin = block_job_drained_begin,
> -    .drained_end = block_job_drained_end,
> -};
> -
>  BlockJob *block_job_next(BlockJob *job)
>  {
>      if (!job) {
> @@ -106,6 +76,21 @@ BlockJob *block_job_get(const char *id)
>      return NULL;
>  }
>  
> +static void block_job_pause(BlockJob *job)
> +{
> +    job->pause_count++;
> +}
> +
> +static void block_job_resume(BlockJob *job)
> +{
> +    assert(job->pause_count > 0);
> +    job->pause_count--;
> +    if (job->pause_count) {
> +        return;
> +    }
> +    block_job_enter(job);
> +}
> +
>  static void block_job_ref(BlockJob *job)
>  {
>      ++job->refcnt;
> @@ -171,6 +156,36 @@ static void block_job_detach_aio_context(void *opaque)
>      block_job_unref(job);
>  }
>  
> +static char *child_job_get_parent_desc(BdrvChild *c)
> +{
> +    BlockJob *job = c->opaque;
> +    return g_strdup_printf("%s job '%s'",
> +                           BlockJobType_lookup[job->driver->job_type],
> +                           job->id);
> +}
> +
> +static const BdrvChildRole child_job = {
> +    .get_parent_desc    = child_job_get_parent_desc,
> +    .stay_at_node       = true,
> +};
> +
> +static void block_job_drained_begin(void *opaque)
> +{
> +    BlockJob *job = opaque;
> +    block_job_pause(job);
> +}
> +
> +static void block_job_drained_end(void *opaque)
> +{
> +    BlockJob *job = opaque;
> +    block_job_resume(job);
> +}
> +
> +static const BlockDevOps block_job_dev_ops = {
> +    .drained_begin = block_job_drained_begin,
> +    .drained_end = block_job_drained_end,
> +};
> +

Movement should probably be its own patch, but I'm not a cop or nothin'

>  void block_job_remove_all_bdrv(BlockJob *job)
>  {
>      GSList *l;
> @@ -471,11 +486,6 @@ void block_job_complete(BlockJob *job, Error **errp)
>      job->driver->complete(job, errp);
>  }
>  
> -void block_job_pause(BlockJob *job)
> -{
> -    job->pause_count++;
> -}
> -
>  void block_job_user_pause(BlockJob *job)
>  {
>      job->user_paused = true;
> @@ -520,16 +530,6 @@ void coroutine_fn block_job_pause_point(BlockJob *job)
>      }
>  }
>  
> -void block_job_resume(BlockJob *job)
> -{
> -    assert(job->pause_count > 0);
> -    job->pause_count--;
> -    if (job->pause_count) {
> -        return;
> -    }
> -    block_job_enter(job);
> -}
> -
>  void block_job_user_resume(BlockJob *job)
>  {
>      if (job && job->user_paused && job->pause_count > 0) {
> @@ -723,6 +723,30 @@ static void block_job_event_completed(BlockJob *job, 
> const char *msg)
>                                          &error_abort);
>  }
>  
> +void block_job_pause_all(void)
> +{
> +    BlockJob *job = NULL;
> +    while ((job = block_job_next(job))) {
> +        AioContext *aio_context = blk_get_aio_context(job->blk);
> +
> +        aio_context_acquire(aio_context);
> +        block_job_pause(job);
> +        aio_context_release(aio_context);
> +    }
> +}
> +
> +void block_job_resume_all(void)
> +{
> +    BlockJob *job = NULL;
> +    while ((job = block_job_next(job))) {
> +        AioContext *aio_context = blk_get_aio_context(job->blk);
> +
> +        aio_context_acquire(aio_context);
> +        block_job_resume(job);
> +        aio_context_release(aio_context);
> +    }
> +}
> +
>  void block_job_event_ready(BlockJob *job)
>  {
>      job->ready = true;
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 9e906f7..aac87cd 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -235,12 +235,11 @@ void block_job_complete(BlockJob *job, Error **errp);
>  BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
>  
>  /**
> - * block_job_pause:
> - * @job: The job to be paused.
> + * block_job_pause_all:
>   *
> - * Asynchronously pause the specified job.
> + * Asynchronously pause all jobs.
>   */
> -void block_job_pause(BlockJob *job);
> +void block_job_pause_all(void);
>  
>  /**
>   * block_job_user_pause:
> @@ -260,12 +259,11 @@ void block_job_user_pause(BlockJob *job);
>  bool block_job_user_paused(BlockJob *job);
>  
>  /**
> - * block_job_resume:
> - * @job: The job to be resumed.
> + * block_job_resume_all:
>   *
> - * Resume the specified job.  Must be paired with a preceding 
> block_job_pause.
> + * Resume all block jobs.  Must be paired with a preceding 
> block_job_pause_all.
>   */
> -void block_job_resume(BlockJob *job);
> +void block_job_resume_all(void);
>  
>  /**
>   * block_job_user_resume:
> 

Reviewed-by: John Snow <address@hidden>