| Message ID | 20180904170930.28619-7-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | jobs: Job Exit Refactoring Pt 2 | expand |
On 2018-09-04 19:09, John Snow wrote: > For purposes of minimum code movement, refactor the mirror_exit > callback to use the post-finalization callbacks in a trivial way. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/mirror.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> (Although I believe the ?: hunk from the previous patch should be here. Also note that we have a couple of places that make use of the GNU extension for "?:" as a binary operator (as in "x ?: y" returns x if x != 0). Just in case you find "s->to_replace ?: src" as appealing as I do.)
On 09/05/2018 06:43 AM, Max Reitz wrote: > On 2018-09-04 19:09, John Snow wrote: >> For purposes of minimum code movement, refactor the mirror_exit >> callback to use the post-finalization callbacks in a trivial way. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/mirror.c | 34 +++++++++++++++++++++++++++------- >> 1 file changed, 27 insertions(+), 7 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > (Although I believe the ?: hunk from the previous patch should be here. > Also note that we have a couple of places that make use of the GNU > extension for "?:" as a binary operator (as in "x ?: y" returns x if > x != 0). Just in case you find "s->to_replace ?: src" as appealing as I > do.) > Ah, I wasn't sure that was OK to use. Meh, since I goofed up the last patch I'll use that version.
On 09/05/2018 08:09 AM, John Snow wrote: > > > On 09/05/2018 06:43 AM, Max Reitz wrote: >> On 2018-09-04 19:09, John Snow wrote: >>> For purposes of minimum code movement, refactor the mirror_exit >>> callback to use the post-finalization callbacks in a trivial way. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> block/mirror.c | 34 +++++++++++++++++++++++++++------- >>> 1 file changed, 27 insertions(+), 7 deletions(-) >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> (Although I believe the ?: hunk from the previous patch should be here. >> Also note that we have a couple of places that make use of the GNU >> extension for "?:" as a binary operator (as in "x ?: y" returns x if >> x != 0). Just in case you find "s->to_replace ?: src" as appealing as I >> do.) >> > > Ah, I wasn't sure that was OK to use. Meh, since I goofed up the last > patch I'll use that version. My minor reason for liking 'a ?: b' - it's less typing, and makes avoiding long lines easier. My major reason for liking it: 'a() ?: b' only evaluates a() once, unlike 'a() ? a() : b' when taking the true branch, which can be particularly important if a() has side effects, but is still an efficiency issue even when side effects are not in play. Yes, you can always rewrite things to use a temporary variable to avoid the ?: operator (which in turn can inflate a single-line expression into multiple lines), but there are indeed enough places in the code base where we rely on this extension that it doesn't hurt to add more uses.
diff --git a/block/mirror.c b/block/mirror.c index 3365bcfdfb..26c30e9607 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob { int max_iov; bool initial_zeroing_ongoing; int in_active_write_counter; + bool prepared; } MirrorBlockJob; typedef struct MirrorBDSOpaque { @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s) } } -static void mirror_exit(Job *job) +static int mirror_exit_common(Job *job) { MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); BlockJob *bjob = &s->common; @@ -617,7 +618,13 @@ static void mirror_exit(Job *job) BlockDriverState *target_bs = blk_bs(s->target); BlockDriverState *mirror_top_bs = s->mirror_top_bs; Error *local_err = NULL; - int ret = job->ret; + bool abort = job->ret < 0; + int ret = 0; + + if (s->prepared) { + return 0; + } + s->prepared = true; bdrv_release_dirty_bitmap(src, s->dirty_bitmap); @@ -642,7 +649,7 @@ static void mirror_exit(Job *job) * required before it could become a backing file of target_bs. */ bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL, &error_abort); - if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { + if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) { BlockDriverState *backing = s->is_none_mode ? src : s->base; if (backing_bs(target_bs) != backing) { bdrv_set_backing_hd(target_bs, backing, &local_err); @@ -658,7 +665,7 @@ static void mirror_exit(Job *job) aio_context_acquire(replace_aio_context); } - if (s->should_complete && ret == 0) { + if (s->should_complete && !abort) { BlockDriverState *to_replace = s->to_replace ? s->to_replace : src; if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) { @@ -708,7 +715,18 @@ static void mirror_exit(Job *job) bdrv_unref(mirror_top_bs); bdrv_unref(src); - job->ret = ret; + return ret; +} + +static int mirror_prepare(Job *job) +{ + return mirror_exit_common(job); +} + +static void mirror_abort(Job *job) +{ + int ret = mirror_exit_common(job); + assert(ret == 0); } static void mirror_throttle(MirrorBlockJob *s) @@ -1129,7 +1147,8 @@ static const BlockJobDriver mirror_job_driver = { .user_resume = block_job_user_resume, .drain = block_job_drain, .run = mirror_run, - .exit = mirror_exit, + .prepare = mirror_prepare, + .abort = mirror_abort, .pause = mirror_pause, .complete = mirror_complete, }, @@ -1146,7 +1165,8 @@ static const BlockJobDriver commit_active_job_driver = { .user_resume = block_job_user_resume, .drain = block_job_drain, .run = mirror_run, - .exit = mirror_exit, + .prepare = mirror_prepare, + .abort = mirror_abort, .pause = mirror_pause, .complete = mirror_complete, },
For purposes of minimum code movement, refactor the mirror_exit callback to use the post-finalization callbacks in a trivial way. Signed-off-by: John Snow <jsnow@redhat.com> --- block/mirror.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)