Re: [Qemu-devel] [PATCH 05/11] blockjob: add block_job_defer_to_main_loop()

[Max Reitz <mreitz@redhat.com>]

On 01.10.2014 19:01, Stefan Hajnoczi wrote:
> Block jobs will run in the BlockDriverState's AioContext, which may not
> always be the QEMU main loop.
>
> There are some block layer APIs that are either not thread-safe or risk
> lock ordering problems.  This includes bdrv_unref(), bdrv_close(), and
> anything that calls bdrv_drain_all().
>
> The block_job_defer_to_main_loop() API allows a block job to schedule a
> function to run in the main loop with the BlockDriverState AioContext
> held.
>
> This function will be used to perform cleanup and backing chain
> manipulations in block jobs.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockjob.c               | 35 +++++++++++++++++++++++++++++++++++
>   include/block/blockjob.h | 19 +++++++++++++++++++
>   2 files changed, 54 insertions(+)
>
> diff --git a/blockjob.c b/blockjob.c
> index 0689fdd..24a64d8 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -313,3 +313,38 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
>       }
>       return action;
>   }
> +
> +typedef struct {
> +    BlockJob *job;
> +    QEMUBH *bh;
> +    AioContext *aio_context;
> +    BlockJobDeferToMainLoopFn *fn;
> +    void *opaque;
> +} BlockJobDeferToMainLoopData;
> +
> +static void block_job_defer_to_main_loop_bh(void *opaque)
> +{
> +    BlockJobDeferToMainLoopData *data = opaque;
> +
> +    qemu_bh_delete(data->bh);
> +
> +    aio_context_acquire(data->aio_context);
> +    data->fn(data->job, data->opaque);
> +    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->bh = qemu_bh_new(block_job_defer_to_main_loop_bh, data);
> +    data->aio_context = bdrv_get_aio_context(job->bs);
> +    data->fn = fn;
> +    data->opaque = opaque;
> +
> +    qemu_bh_schedule(data->bh);
> +}

I'm not so sure whether it'd be possible to change the BDS's AIO context 
(in another thread) after the call to bdrv_get_aio_context() in 
block_job_defer_to_main_loop() and before 
block_job_defer_to_main_loop_bh() is run. If so, this might create race 
conditions.

Other than that, this patch looks good.

Max