Message ID | 20170323173928.14439-4-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
On 03/23/2017 01:39 PM, Paolo Bonzini wrote: > Outside blockjob.c, block_job_unref is only used when a block job fails > to start, and block_job_ref is not used at all. The reference counting > thus is pretty well hidden. Introduce a separate function to be used > by block jobs; because block_job_ref and block_job_unref now become > static, move them earlier in blockjob.c. > Early into the series, "I guess so." It makes it a lot less obvious and more vague as to what exactly is happening here, so I like it less. > Later on, block_job_fail will also have different locking than > block_job_unref. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/backup.c | 2 +- > block/commit.c | 2 +- > block/mirror.c | 2 +- > blockjob.c | 47 ++++++++++++++++++++++++++------------------ > include/block/blockjob_int.h | 15 +++----------- > tests/test-blockjob.c | 10 +++++----- > 6 files changed, 39 insertions(+), 39 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index a4fb288..f284fd5 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -692,7 +692,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, > } > if (job) { > backup_clean(&job->common); > - block_job_unref(&job->common); > + block_job_fail(&job->common); > } > > return NULL; > diff --git a/block/commit.c b/block/commit.c > index 2832482..9b9ea39 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -424,7 +424,7 @@ fail: > if (commit_top_bs) { > bdrv_set_backing_hd(overlay_bs, top, &error_abort); > } > - block_job_unref(&s->common); > + block_job_fail(&s->common); > } > > > diff --git a/block/mirror.c b/block/mirror.c > index ca4baa5..5cb2134 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1243,7 +1243,7 @@ fail: > if (s) { > g_free(s->replaces); > blk_unref(s->target); > - block_job_unref(&s->common); > + block_job_fail(&s->common); > } > > bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL, > diff --git a/blockjob.c b/blockjob.c > index 24d1e12..1c63d15 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -106,6 +106,32 @@ BlockJob *block_job_get(const char *id) > return NULL; > } > > +static void block_job_ref(BlockJob *job) > +{ > + ++job->refcnt; > +} > + > +static void block_job_attached_aio_context(AioContext *new_context, > + void *opaque); > +static void block_job_detach_aio_context(void *opaque); > + ^ Some tagalongs to the party? > +static void block_job_unref(BlockJob *job) > +{ > + if (--job->refcnt == 0) { > + BlockDriverState *bs = blk_bs(job->blk); > + bs->job = NULL; > + block_job_remove_all_bdrv(job); > + blk_remove_aio_context_notifier(job->blk, > + block_job_attached_aio_context, > + block_job_detach_aio_context, job); > + blk_unref(job->blk); > + error_free(job->blocker); > + g_free(job->id); > + QLIST_REMOVE(job, job_list); > + g_free(job); > + } > +} > + > static void block_job_attached_aio_context(AioContext *new_context, > void *opaque) > { > @@ -293,26 +319,9 @@ void block_job_start(BlockJob *job) > qemu_coroutine_enter(job->co); > } > > -void block_job_ref(BlockJob *job) > -{ > - ++job->refcnt; > -} > - > -void block_job_unref(BlockJob *job) > +void block_job_fail(BlockJob *job) > { > - if (--job->refcnt == 0) { > - BlockDriverState *bs = blk_bs(job->blk); > - bs->job = NULL; > - block_job_remove_all_bdrv(job); > - blk_remove_aio_context_notifier(job->blk, > - block_job_attached_aio_context, > - block_job_detach_aio_context, job); > - blk_unref(job->blk); > - error_free(job->blocker); > - g_free(job->id); > - QLIST_REMOVE(job, job_list); > - g_free(job); > - } > + block_job_unref(job); > } > > static void block_job_completed_single(BlockJob *job) > diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h > index bfcc5d1..97ffc43 100644 > --- a/include/block/blockjob_int.h > +++ b/include/block/blockjob_int.h > @@ -156,21 +156,12 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns); > void block_job_yield(BlockJob *job); > > /** > - * block_job_ref: > + * block_job_fail: > * @bs: The block device. > * > - * Grab a reference to the block job. Should be paired with block_job_unref. > + * The block job could not be started, free it. > */ > -void block_job_ref(BlockJob *job); > - > -/** > - * block_job_unref: > - * @bs: The block device. > - * > - * Release reference to the block job and release resources if it is the last > - * reference. > - */ > -void block_job_unref(BlockJob *job); > +void block_job_fail(BlockJob *job); > > /** > * block_job_completed: > diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c > index 740e740..35bbbbc 100644 > --- a/tests/test-blockjob.c > +++ b/tests/test-blockjob.c > @@ -116,11 +116,11 @@ static void test_job_ids(void) > job[1] = do_test_id(blk[1], "id0", false); > > /* But once job[0] finishes we can reuse its ID */ > - block_job_unref(job[0]); > + block_job_fail(job[0]); > job[1] = do_test_id(blk[1], "id0", true); > > /* No job ID specified, defaults to the backend name ('drive1') */ > - block_job_unref(job[1]); > + block_job_fail(job[1]); > job[1] = do_test_id(blk[1], NULL, true); > > /* Duplicate job ID */ > @@ -133,9 +133,9 @@ static void test_job_ids(void) > /* This one is valid */ > job[2] = do_test_id(blk[2], "id_2", true); > > - block_job_unref(job[0]); > - block_job_unref(job[1]); > - block_job_unref(job[2]); > + block_job_fail(job[0]); > + block_job_fail(job[1]); > + block_job_fail(job[2]); > > destroy_blk(blk[0]); > destroy_blk(blk[1]); > Mechanically OK, of course. Allow me to discover my appetite for it further into your series.
On Thu, Mar 23, 2017 at 06:39:21PM +0100, Paolo Bonzini wrote: > Outside blockjob.c, block_job_unref is only used when a block job fails > to start, and block_job_ref is not used at all. The reference counting > thus is pretty well hidden. Introduce a separate function to be used > by block jobs; because block_job_ref and block_job_unref now become > static, move them earlier in blockjob.c. > > Later on, block_job_fail will also have different locking than > block_job_unref. block_job_fail() sounds like it's *the* job failure API. How about block_job_fail_early()? It indicates the API is only for the beginning of the job's lifecycle. Stefan
On 10/04/2017 17:22, Stefan Hajnoczi wrote: >> >> Later on, block_job_fail will also have different locking than >> block_job_unref. > block_job_fail() sounds like it's *the* job failure API. > How about block_job_fail_early()? > > It indicates the API is only for the beginning of the job's lifecycle. Can't disagree and I'll apply your suggestion. Thanks, Paolo
diff --git a/block/backup.c b/block/backup.c index a4fb288..f284fd5 100644 --- a/block/backup.c +++ b/block/backup.c @@ -692,7 +692,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, } if (job) { backup_clean(&job->common); - block_job_unref(&job->common); + block_job_fail(&job->common); } return NULL; diff --git a/block/commit.c b/block/commit.c index 2832482..9b9ea39 100644 --- a/block/commit.c +++ b/block/commit.c @@ -424,7 +424,7 @@ fail: if (commit_top_bs) { bdrv_set_backing_hd(overlay_bs, top, &error_abort); } - block_job_unref(&s->common); + block_job_fail(&s->common); } diff --git a/block/mirror.c b/block/mirror.c index ca4baa5..5cb2134 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1243,7 +1243,7 @@ fail: if (s) { g_free(s->replaces); blk_unref(s->target); - block_job_unref(&s->common); + block_job_fail(&s->common); } bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL, diff --git a/blockjob.c b/blockjob.c index 24d1e12..1c63d15 100644 --- a/blockjob.c +++ b/blockjob.c @@ -106,6 +106,32 @@ BlockJob *block_job_get(const char *id) return NULL; } +static void block_job_ref(BlockJob *job) +{ + ++job->refcnt; +} + +static void block_job_attached_aio_context(AioContext *new_context, + void *opaque); +static void block_job_detach_aio_context(void *opaque); + +static void block_job_unref(BlockJob *job) +{ + if (--job->refcnt == 0) { + BlockDriverState *bs = blk_bs(job->blk); + bs->job = NULL; + block_job_remove_all_bdrv(job); + blk_remove_aio_context_notifier(job->blk, + block_job_attached_aio_context, + block_job_detach_aio_context, job); + blk_unref(job->blk); + error_free(job->blocker); + g_free(job->id); + QLIST_REMOVE(job, job_list); + g_free(job); + } +} + static void block_job_attached_aio_context(AioContext *new_context, void *opaque) { @@ -293,26 +319,9 @@ void block_job_start(BlockJob *job) qemu_coroutine_enter(job->co); } -void block_job_ref(BlockJob *job) -{ - ++job->refcnt; -} - -void block_job_unref(BlockJob *job) +void block_job_fail(BlockJob *job) { - if (--job->refcnt == 0) { - BlockDriverState *bs = blk_bs(job->blk); - bs->job = NULL; - block_job_remove_all_bdrv(job); - blk_remove_aio_context_notifier(job->blk, - block_job_attached_aio_context, - block_job_detach_aio_context, job); - blk_unref(job->blk); - error_free(job->blocker); - g_free(job->id); - QLIST_REMOVE(job, job_list); - g_free(job); - } + block_job_unref(job); } static void block_job_completed_single(BlockJob *job) diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index bfcc5d1..97ffc43 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -156,21 +156,12 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns); void block_job_yield(BlockJob *job); /** - * block_job_ref: + * block_job_fail: * @bs: The block device. * - * Grab a reference to the block job. Should be paired with block_job_unref. + * The block job could not be started, free it. */ -void block_job_ref(BlockJob *job); - -/** - * block_job_unref: - * @bs: The block device. - * - * Release reference to the block job and release resources if it is the last - * reference. - */ -void block_job_unref(BlockJob *job); +void block_job_fail(BlockJob *job); /** * block_job_completed: diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c index 740e740..35bbbbc 100644 --- a/tests/test-blockjob.c +++ b/tests/test-blockjob.c @@ -116,11 +116,11 @@ static void test_job_ids(void) job[1] = do_test_id(blk[1], "id0", false); /* But once job[0] finishes we can reuse its ID */ - block_job_unref(job[0]); + block_job_fail(job[0]); job[1] = do_test_id(blk[1], "id0", true); /* No job ID specified, defaults to the backend name ('drive1') */ - block_job_unref(job[1]); + block_job_fail(job[1]); job[1] = do_test_id(blk[1], NULL, true); /* Duplicate job ID */ @@ -133,9 +133,9 @@ static void test_job_ids(void) /* This one is valid */ job[2] = do_test_id(blk[2], "id_2", true); - block_job_unref(job[0]); - block_job_unref(job[1]); - block_job_unref(job[2]); + block_job_fail(job[0]); + block_job_fail(job[1]); + block_job_fail(job[2]); destroy_blk(blk[0]); destroy_blk(blk[1]);
Outside blockjob.c, block_job_unref is only used when a block job fails to start, and block_job_ref is not used at all. The reference counting thus is pretty well hidden. Introduce a separate function to be used by block jobs; because block_job_ref and block_job_unref now become static, move them earlier in blockjob.c. Later on, block_job_fail will also have different locking than block_job_unref. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/backup.c | 2 +- block/commit.c | 2 +- block/mirror.c | 2 +- blockjob.c | 47 ++++++++++++++++++++++++++------------------ include/block/blockjob_int.h | 15 +++----------- tests/test-blockjob.c | 10 +++++----- 6 files changed, 39 insertions(+), 39 deletions(-)