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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2435 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]