| Message ID | 1453964571-23016-2-git-send-email-famz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 28, 2016 at 03:02:50PM +0800, Fam Zheng wrote: > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index d84ccd8..de59fc2 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -393,18 +393,20 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, > typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque); > > /** > - * block_job_defer_to_main_loop: > + * block_job_coroutine_complete: > * @job: The job > * @fn: The function to run in the main loop > * @opaque: The opaque value that is passed to @fn > * > - * Execute a given function in the main loop with the BlockDriverState > - * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and > - * anything that uses bdrv_drain_all() in the main loop. > + * Complete the block job coroutine and execute a given function in the main This function does not "complete the block job coroutine" so this seems confusing to me. How about "Call this function to schedule clean up code to run in the main loop when completing a block job"? That said, I'm not sure if changing the scope of this function is necessary at all. If the next patch adds a job->aio_context pointer then it could set the pointer back after calling the user's function from the main loop. Then this function could also be used in cases where the job/coroutine stays alive. > + * loop with the BlockDriverState AioContext acquired. Block jobs must call > + * bdrv_unref(), bdrv_close(), and anything that uses bdrv_drain_all() in the > + * main loop. After calling this, the block job coroutine should complete right > + * away, without doing any heavy operations such as I/O or block_job_yield(). "heavy operations" is too vague. What are the specific constraints here?
On Thu, 01/28 11:53, Stefan Hajnoczi wrote: > On Thu, Jan 28, 2016 at 03:02:50PM +0800, Fam Zheng wrote: > > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > > index d84ccd8..de59fc2 100644 > > --- a/include/block/blockjob.h > > +++ b/include/block/blockjob.h > > @@ -393,18 +393,20 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, > > typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque); > > > > /** > > - * block_job_defer_to_main_loop: > > + * block_job_coroutine_complete: > > * @job: The job > > * @fn: The function to run in the main loop > > * @opaque: The opaque value that is passed to @fn > > * > > - * Execute a given function in the main loop with the BlockDriverState > > - * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and > > - * anything that uses bdrv_drain_all() in the main loop. > > + * Complete the block job coroutine and execute a given function in the main > > This function does not "complete the block job coroutine" so this seems > confusing to me. How about "Call this function to schedule clean up > code to run in the main loop when completing a block job"? Sounds good. > > That said, I'm not sure if changing the scope of this function is > necessary at all. If the next patch adds a job->aio_context pointer > then it could set the pointer back after calling the user's function > from the main loop. Then this function could also be used in cases > where the job/coroutine stays alive. Yes, that can be done. Fam
diff --git a/block/backup.c b/block/backup.c index 00cafdb..b429666 100644 --- a/block/backup.c +++ b/block/backup.c @@ -482,7 +482,7 @@ static void coroutine_fn backup_run(void *opaque) data = g_malloc(sizeof(*data)); data->ret = ret; - block_job_defer_to_main_loop(&job->common, backup_complete, data); + block_job_coroutine_complete(&job->common, backup_complete, data); } void backup_start(BlockDriverState *bs, BlockDriverState *target, diff --git a/block/commit.c b/block/commit.c index 446a3ae..f6b93bd 100644 --- a/block/commit.c +++ b/block/commit.c @@ -181,7 +181,7 @@ out: data = g_malloc(sizeof(*data)); data->ret = ret; - block_job_defer_to_main_loop(&s->common, commit_complete, data); + block_job_coroutine_complete(&s->common, commit_complete, data); } static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp) diff --git a/block/mirror.c b/block/mirror.c index e9e151c..d665f2b 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -626,7 +626,7 @@ immediate_exit: /* Before we switch to target in mirror_exit, make sure data doesn't * change. */ bdrv_drained_begin(s->common.bs); - block_job_defer_to_main_loop(&s->common, mirror_exit, data); + block_job_coroutine_complete(&s->common, mirror_exit, data); } static void mirror_set_speed(BlockJob *job, int64_t speed, Error **errp) diff --git a/block/stream.c b/block/stream.c index cafaa07..9572ce3 100644 --- a/block/stream.c +++ b/block/stream.c @@ -194,7 +194,7 @@ wait: data = g_malloc(sizeof(*data)); data->ret = ret; data->reached_end = sector_num == end; - block_job_defer_to_main_loop(&s->common, stream_complete, data); + block_job_coroutine_complete(&s->common, stream_complete, data); } static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp) diff --git a/blockjob.c b/blockjob.c index 80adb9d..4b16720 100644 --- a/blockjob.c +++ b/blockjob.c @@ -471,7 +471,7 @@ static void block_job_defer_to_main_loop_bh(void *opaque) qemu_bh_delete(data->bh); - /* Prevent race with block_job_defer_to_main_loop() */ + /* Prevent race with block_job_coroutine_complete() */ aio_context_acquire(data->aio_context); /* Fetch BDS AioContext again, in case it has changed */ @@ -487,7 +487,7 @@ static void block_job_defer_to_main_loop_bh(void *opaque) g_free(data); } -void block_job_defer_to_main_loop(BlockJob *job, +void block_job_coroutine_complete(BlockJob *job, BlockJobDeferToMainLoopFn *fn, void *opaque) { diff --git a/include/block/blockjob.h b/include/block/blockjob.h index d84ccd8..de59fc2 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -393,18 +393,20 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque); /** - * block_job_defer_to_main_loop: + * block_job_coroutine_complete: * @job: The job * @fn: The function to run in the main loop * @opaque: The opaque value that is passed to @fn * - * Execute a given function in the main loop with the BlockDriverState - * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and - * anything that uses bdrv_drain_all() in the main loop. + * Complete the block job coroutine and execute a given function in the main + * loop with the BlockDriverState AioContext acquired. Block jobs must call + * bdrv_unref(), bdrv_close(), and anything that uses bdrv_drain_all() in the + * main loop. After calling this, the block job coroutine should complete right + * away, without doing any heavy operations such as I/O or block_job_yield(). * * The @job AioContext is held while @fn executes. */ -void block_job_defer_to_main_loop(BlockJob *job, +void block_job_coroutine_complete(BlockJob *job, BlockJobDeferToMainLoopFn *fn, void *opaque); diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c index 34747e9..56442f2 100644 --- a/tests/test-blockjob-txn.c +++ b/tests/test-blockjob-txn.c @@ -57,7 +57,7 @@ static void coroutine_fn test_block_job_run(void *opaque) } } - block_job_defer_to_main_loop(job, test_block_job_complete, + block_job_coroutine_complete(job, test_block_job_complete, (void *)(intptr_t)s->rc); }
The next patch will make this function more restrictive than it is now, rename it and update comment to reflect the change. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/backup.c | 2 +- block/commit.c | 2 +- block/mirror.c | 2 +- block/stream.c | 2 +- blockjob.c | 4 ++-- include/block/blockjob.h | 12 +++++++----- tests/test-blockjob-txn.c | 2 +- 7 files changed, 14 insertions(+), 12 deletions(-)