Re: [Qemu-devel] [PATCH v3 09/15] blockjob: Move BlockJobDeferToMainLoopData into BlockJob

[Stefan Hajnoczi]

On Fri, Jul 10, 2015 at 11:46:46AM +0800, Fam Zheng wrote:
> diff --git a/blockjob.c b/blockjob.c
> index 11b48f5..e057dd5 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -92,6 +92,10 @@ void block_job_completed(BlockJob *job, int ret)
>      assert(!job->completed);
>      job->completed = true;
>      job->ret = ret;
> +    if (job->defer_to_main_loop_data.bh) {
> +        qemu_bh_delete(job->defer_to_main_loop_data.bh);
> +        job->defer_to_main_loop_data.bh = NULL;
> +    }
>      job->cb(job->opaque, ret);
>      block_job_release(bs);
>  }

This doesn't make sense to me:

1. This function is called from a defer_to_main_loop BH so
   job->defer_to_main_loop_data.bh should already be running here.

   In fact, we've already called qemu_bh_delete() in
   block_job_defer_to_main_loop_bh().  This call is pointless but you
   could change it to an assertion and assign bh = NULL in
   block_job_defer_to_main_loop_bh().

2. If there was a BH scheduled, why is it safe to silently delete it?
   Wouldn't the caller rely on the BH code executing to clean up, for
   example?

If you drop this if statement, is it necessary to move
BlockJobDeferToMainLoopData into BlockJob at all?  Maybe you can just
drop this patch.

> @@ -344,44 +348,36 @@ 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;
> +    BlockJob *job = opaque;
> +    /* Copy the struct in case job get released in data.fn() */
> +    BlockJobDeferToMainLoopData data = job->defer_to_main_loop_data;

A comment is warranted since this access is only safe due to the
following:

The job may still be executing in another thread (the AioContext hasn't
been acquired by us yet), but job->defer_to_main_loop_data isn't
modified by the other thread after the qemu_bh_schedule() call.

Additionally, the atomic_xchg memory barriers in qemu_bh_schedule() and
aio_bh_poll() to ensure that this thread sees the latest
job->defer_to_main_loop_data data.

Access to other job fields would probably not be safe here!

pgpEi9NpV8OSg.pgp
Description: PGP signature