[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 09/15] blockjob: Move BlockJobDeferToMainLoop
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v3 09/15] blockjob: Move BlockJobDeferToMainLoopData into BlockJob |
Date: |
Tue, 14 Jul 2015 18:36:26 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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!
- [Qemu-devel] [PATCH v3 06/15] blockjob: Add .txn_commit and .txn_abort transaction actions, (continued)
- [Qemu-devel] [PATCH v3 06/15] blockjob: Add .txn_commit and .txn_abort transaction actions, Fam Zheng, 2015/07/09
- [Qemu-devel] [PATCH v3 07/15] blockjob: Add "completed" and "ret" in BlockJob, Fam Zheng, 2015/07/09
- [Qemu-devel] [PATCH v3 08/15] blockjob: Simplify block_job_finish_sync, Fam Zheng, 2015/07/09
- [Qemu-devel] [PATCH v3 09/15] blockjob: Move BlockJobDeferToMainLoopData into BlockJob, Fam Zheng, 2015/07/09
- [Qemu-devel] [PATCH v3 10/15] block: add block job transactions, Fam Zheng, 2015/07/09
- Re: [Qemu-devel] [PATCH v3 10/15] block: add block job transactions, Stefan Hajnoczi, 2015/07/14
- [Qemu-devel] [PATCH v3 11/15] blockdev: make BlockJobTxn available to qmp 'transaction', Fam Zheng, 2015/07/09
- [Qemu-devel] [PATCH v3 12/15] block/backup: support block job transactions, Fam Zheng, 2015/07/09