Re: [Qemu-block] [PATCH 17/42] job: Move defer_to_main_loop to Job

[Max Reitz]

On 2018-05-09 18:26, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  include/block/blockjob.h     |  5 ----
>  include/block/blockjob_int.h | 19 ---------------
>  include/qemu/job.h           | 20 ++++++++++++++++
>  block/backup.c               |  7 +++---
>  block/commit.c               | 11 +++++----
>  block/mirror.c               | 15 ++++++------
>  block/stream.c               | 14 +++++------
>  blockjob.c                   | 57 
> ++++----------------------------------------
>  job.c                        | 33 +++++++++++++++++++++++++
>  tests/test-bdrv-drain.c      |  7 +++---
>  tests/test-blockjob-txn.c    | 13 +++++-----
>  tests/test-blockjob.c        |  7 +++---
>  12 files changed, 98 insertions(+), 110 deletions(-)

[...]

> diff --git a/block/commit.c b/block/commit.c
> index 85baea8f92..d326766e4d 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -72,9 +72,10 @@ typedef struct {
>      int ret;
>  } CommitCompleteData;
>  
> -static void commit_complete(BlockJob *job, void *opaque)
> +static void commit_complete(Job *job, void *opaque)
>  {
> -    CommitBlockJob *s = container_of(job, CommitBlockJob, common);
> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);

Now this is just abuse.

...but it's not the first time someone packs two container_of() into
one, it appears.  So, whatever, I guess.

> +    BlockJob *bjob = &s->common;
>      CommitCompleteData *data = opaque;
>      BlockDriverState *top = blk_bs(s->top);
>      BlockDriverState *base = blk_bs(s->base);

[...]

> diff --git a/blockjob.c b/blockjob.c
> index d44f5c2e50..6021d885be 100644
> --- a/blockjob.c
> +++ b/blockjob.c

[...]

> @@ -1159,50 +1159,3 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
> BlockdevOnError on_err,
>      }
>      return action;
>  }
> -
> -typedef struct {
> -    BlockJob *job;
> -    AioContext *aio_context;
> -    BlockJobDeferToMainLoopFn *fn;
> -    void *opaque;
> -} BlockJobDeferToMainLoopData;
> -
> -static void block_job_defer_to_main_loop_bh(void *opaque)
> -{
> -    BlockJobDeferToMainLoopData *data = opaque;
> -    AioContext *aio_context;
> -
> -    /* Prevent race with block_job_defer_to_main_loop() */
> -    aio_context_acquire(data->aio_context);
> -
> -    /* Fetch BDS AioContext again, in case it has changed */
> -    aio_context = blk_get_aio_context(data->job->blk);
> -    if (aio_context != data->aio_context) {
> -        aio_context_acquire(aio_context);
> -    }
> -
> -    data->fn(data->job, data->opaque);
> -
> -    if (aio_context != data->aio_context) {
> -        aio_context_release(aio_context);
> -    }
> -
> -    aio_context_release(data->aio_context);
> -
> -    g_free(data);
> -}
> -
> -void block_job_defer_to_main_loop(BlockJob *job,
> -                                  BlockJobDeferToMainLoopFn *fn,
> -                                  void *opaque)
> -{
> -    BlockJobDeferToMainLoopData *data = g_malloc(sizeof(*data));
> -    data->job = job;
> -    data->aio_context = blk_get_aio_context(job->blk);
> -    data->fn = fn;
> -    data->opaque = opaque;
> -    job->deferred_to_main_loop = true;
> -
> -    aio_bh_schedule_oneshot(qemu_get_aio_context(),
> -                            block_job_defer_to_main_loop_bh, data);
> -}
> diff --git a/job.c b/job.c
> index 6f97a4317e..b074b3ffd7 100644
> --- a/job.c
> +++ b/job.c
> @@ -28,6 +28,7 @@
>  #include "qapi/error.h"
>  #include "qemu/job.h"
>  #include "qemu/id.h"
> +#include "qemu/main-loop.h"
>  #include "trace-root.h"
>  
>  static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs);
> @@ -170,3 +171,35 @@ void job_unref(Job *job)
>          g_free(job);
>      }
>  }
> +
> +typedef struct {
> +    Job *job;
> +    JobDeferToMainLoopFn *fn;
> +    void *opaque;
> +} JobDeferToMainLoopData;
> +
> +static void job_defer_to_main_loop_bh(void *opaque)
> +{
> +    JobDeferToMainLoopData *data = opaque;
> +    Job *job = data->job;
> +    AioContext *aio_context = job->aio_context;
> +
> +    /* Prevent race with job_defer_to_main_loop() */
> +    aio_context_acquire(aio_context);

I don't have a good feeling about this.  The original code had this
comment above an aio_context_acquire() for a context that might
decidedly not have anything to do with the BB's context;
block_job_defer_to_main_loop()'s description was that it would acquire
the latter, so why did it acquire the former at all?

We wouldn't need this comment here at all, because acquiring this
AioContext is part of the interface.  That's why I don't have a good
feeling.

The best explanation I can come up with is that the original code
acquired the AioContext both of the block device at the time of the BH
(because that needs to be done), and at the time of
block_job_defer_to_main_loop() -- because the latter is probably the
context the block_job_defer_to_main_loop() call came from, so it should
be (b)locked.

But if that's the case, then the same should be done here.  The context
of the job may change between scheduling the BH and the BH being
executed, so we might lock a different context here than the one
job_defer_to_main_loop() ran in (i.e., job->aio_context at the time of
job_defer_to_main_loop() running).  And maybe we should lock that old
context, too -- just like block_job_defer_to_main_loop_bh() did.

Max

> +    data->fn(data->job, data->opaque);
> +    aio_context_release(aio_context);
> +
> +    g_free(data);
> +}
> +
> +void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque)
> +{
> +    JobDeferToMainLoopData *data = g_malloc(sizeof(*data));
> +    data->job = job;
> +    data->fn = fn;
> +    data->opaque = opaque;
> +    job->deferred_to_main_loop = true;
> +
> +    aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +                            job_defer_to_main_loop_bh, data);
> +}

signature.asc
Description: OpenPGP digital signature