| Message ID | 1436500012-32593-10-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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!
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!
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); } @@ -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; AioContext *aio_context; - qemu_bh_delete(data->bh); + qemu_bh_delete(data.bh); /* Prevent race with block_job_defer_to_main_loop() */ - aio_context_acquire(data->aio_context); + 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 = bdrv_get_aio_context(job->bs); aio_context_acquire(aio_context); - data->fn(data->job, data->opaque); + data.fn(job, data.opaque); aio_context_release(aio_context); - aio_context_release(data->aio_context); - - g_free(data); + aio_context_release(data.aio_context); } 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); + BlockJobDeferToMainLoopData *data = &job->defer_to_main_loop_data; + assert(!data->bh); + data->bh = qemu_bh_new(block_job_defer_to_main_loop_bh, job); data->aio_context = bdrv_get_aio_context(job->bs); data->fn = fn; data->opaque = opaque; diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 40d0776..5bac2e2 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -64,6 +64,15 @@ typedef struct BlockJobDriver { void (*txn_abort)(BlockJob *job); } BlockJobDriver; +typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque); + +typedef struct { + QEMUBH *bh; + AioContext *aio_context; + BlockJobDeferToMainLoopFn *fn; + void *opaque; +} BlockJobDeferToMainLoopData; + /** * BlockJob: * @@ -83,6 +92,11 @@ struct BlockJob { Coroutine *co; /** + * The data used by block_job_defer_to_main_loop. + */ + BlockJobDeferToMainLoopData defer_to_main_loop_data; + + /** * Set to true if the job should cancel itself. The flag must * always be tested just before toggling the busy flag from false * to true. After a job has been cancelled, it should only yield @@ -359,8 +373,6 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, BlockdevOnError on_err, int is_read, int error); -typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque); - /** * block_job_defer_to_main_loop: * @job: The job
This will bind the BH data to block job data and is therefore easier to manage, for example during cancellation. Signed-off-by: Fam Zheng <famz@redhat.com> --- blockjob.c | 34 +++++++++++++++------------------- include/block/blockjob.h | 16 ++++++++++++++-- 2 files changed, 29 insertions(+), 21 deletions(-)