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

[Fam Zheng <famz@redhat.com>]

On Tue, 07/14 11:03, Stefan Hajnoczi wrote:
> 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.

You're right, I agree this patch is superfluous and can be dropped.

Fam

> 
> > @@ -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!