On Wed, Oct 01, 2014 at 09:06:36PM +0200, Max Reitz wrote:
> On 01.10.2014 19:01, Stefan Hajnoczi wrote:
> >+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.
Yes, I think you are correct.
The solution is a little tricky, we need to hold both AioContexts:
static void block_job_defer_to_main_loop_bh(void *opaque)
{
BlockJobDeferToMainLoopData *data = opaque;
AioContext aio_context;
qemu_bh_delete(data->bh);
/* 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 = bdrv_get_aio_context(data->job->bs);
aio_context_acquire(aio_context);
data->fn(data->job, data->opaque);
aio_context_release(aio_context);
aio_context_release(data->aio_context);
g_free(data);
}
Tada! Because this executes in the main loop it is safe to do this - no lock
ordering problems. And because aio_context_acquire() is recursive we can lock
twice without problems.
Stefan
pgp9YlF_UdiYL.pgp
Description: PGP signature
