| Message ID | 20180807043349.27196-17-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | jobs: defer graph changes until finalize | expand |
Am 07.08.2018 um 06:33 hat John Snow geschrieben: > Use the component callbacks; prepare, commit, abort, and clean. > > NB: prepare is only called when the job has not yet failed; > and abort can be called after prepare. > > complete -> prepare -> abort -> clean > complete -> abort -> clean I always found this confusing. After converting all jobs to use the infrastructure, do you think that this design is helpful? You seem to be calling *_common function from commit and abort, with an almost empty prepare, which looks like a hint that maybe we should reorganise things. > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/commit.c | 94 +++++++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 61 insertions(+), 33 deletions(-) > > diff --git a/block/commit.c b/block/commit.c > index 1a4a56795f..6673a0544e 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -35,7 +35,9 @@ typedef struct CommitBlockJob { > BlockJob common; > BlockDriverState *commit_top_bs; > BlockBackend *top; > + BlockDriverState *top_bs; > BlockBackend *base; > + BlockDriverState *base_bs; > BlockdevOnError on_error; > int base_flags; > char *backing_file_str; More state. :-( Can we at least move the new fields to the end of the struct with a comment that they are only valid after .exit()? > @@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base, > static void commit_exit(Job *job) > { > CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > - BlockJob *bjob = &s->common; > - BlockDriverState *top = blk_bs(s->top); > - BlockDriverState *base = blk_bs(s->base); > - BlockDriverState *commit_top_bs = s->commit_top_bs; > - int ret = job->ret; > - bool remove_commit_top_bs = false; > + > + s->top_bs = blk_bs(s->top); > + s->base_bs = blk_bs(s->base); > > /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */ > - bdrv_ref(top); > - bdrv_ref(commit_top_bs); > + bdrv_ref(s->top_bs); > + bdrv_ref(s->commit_top_bs); > +} > > - /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before > - * the normal backing chain can be restored. */ > - blk_unref(s->base); > +static int commit_prepare(Job *job) > +{ > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > - if (!job_is_cancelled(job) && ret == 0) { > - /* success */ > - ret = bdrv_drop_intermediate(s->commit_top_bs, base, > - s->backing_file_str); > - } else { > - /* XXX Can (or should) we somehow keep 'consistent read' blocked even > - * after the failed/cancelled commit job is gone? If we already wrote > - * something to base, the intermediate images aren't valid any more. */ > - remove_commit_top_bs = true; > - } > + /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before > + * the normal backing chain can be restored. */ > + blk_unref(s->base); > + s->base = NULL; > + > + return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs, > + s->backing_file_str); The indentation is off here (which is weird because the additional space seems to be the only change to most of the lines). > +} > + > +static void commit_exit_common(Job *job) > +{ > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > /* restore base open flags here if appropriate (e.g., change the base back > * to r/o). These reopens do not need to be atomic, since we won't abort > * even on failure here */ > - if (s->base_flags != bdrv_get_flags(base)) { > - bdrv_reopen(base, s->base_flags, NULL); > + if (s->base_flags != bdrv_get_flags(s->base_bs)) { > + bdrv_reopen(s->base_bs, s->base_flags, NULL); > } > g_free(s->backing_file_str); > blk_unref(s->top); Feels like general cleanup, so intuitively, I'd expect this in .clean. The only thing that could make this impossible is the ordering. bdrv_reopen() of s->base_bs should be safe to be deferred, the node is still referenced after the job is concluded and we don't rely on it being read-only again in any of the following completion code. g_free() is safe to be moved anyway. blk_unref(s->top) doesn't change the graph because we did a bdrv_ref(s->top_bs). The permissions of the BlockBackend could still be a problem. However, it doesn't take any permissions: s->top = blk_new(0, BLK_PERM_ALL); So I think moving this first part of commit_exit_common() to .clean should be safe. > @@ -110,21 +112,43 @@ static void commit_exit(Job *job) > * job_finish_sync()), job_completed() won't free it and therefore the > * blockers on the intermediate nodes remain. This would cause > * bdrv_set_backing_hd() to fail. */ > - block_job_remove_all_bdrv(bjob); > + block_job_remove_all_bdrv(&s->common); There hasn't been any bdrv_set_backing_hd() close to this comment for a while. Might be time to update it. I suppose the update would refer to bdrv_replace_node(), which only happens in .abort, so should block_job_remove_all_bdrv() move, too? With these changes, commit_exit_common() would be gone. > +} > + > +static void commit_commit(Job *job) > +{ > + commit_exit_common(job); > +} (I think I've answered this question now, by effectively proposing to do away with .commit, but I'll leave it there anyway.) Is there any reason for the split between .prepare and .commit as it is or is this mostly arbitrary? Probably the latter because commit isn't even transactionable? > +static void commit_abort(Job *job) > +{ > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > + > + if (s->base) { > + blk_unref(s->base); > + } > + > + commit_exit_common(job); > + > + /* XXX Can (or should) we somehow keep 'consistent read' blocked even > + * after the failed/cancelled commit job is gone? If we already wrote > + * something to base, the intermediate images aren't valid any more. */ > > /* If bdrv_drop_intermediate() didn't already do that, remove the commit > * filter driver from the backing chain. Do this as the final step so that > * the 'consistent read' permission can be granted. */ > - if (remove_commit_top_bs) { > - bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL, > - &error_abort); > - bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs), > - &error_abort); > - } > + bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL, > + &error_abort); > + bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs), > + &error_abort); > +} > > - bdrv_unref(commit_top_bs); > - bdrv_unref(top); > - job->ret = ret; > +static void commit_clean(Job *job) > +{ > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > + > + bdrv_unref(s->commit_top_bs); > + bdrv_unref(s->top_bs); > } > > static int coroutine_fn commit_run(Job *job) > @@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = { > .drain = block_job_drain, > .start = commit_run, > .exit = commit_exit, > + .prepare = commit_prepare, > + .commit = commit_commit, > + .abort = commit_abort, > + .clean = commit_clean > }, > }; Kevin
Noticed one other thing... Am 09.08.2018 um 17:12 hat Kevin Wolf geschrieben: > Am 07.08.2018 um 06:33 hat John Snow geschrieben: > > Use the component callbacks; prepare, commit, abort, and clean. > > > > NB: prepare is only called when the job has not yet failed; > > and abort can be called after prepare. > > > > complete -> prepare -> abort -> clean > > complete -> abort -> clean > > I always found this confusing. After converting all jobs to use the > infrastructure, do you think that this design is helpful? You seem to be > calling *_common function from commit and abort, with an almost empty > prepare, which looks like a hint that maybe we should reorganise things. > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > block/commit.c | 94 +++++++++++++++++++++++++++++++++++++--------------------- > > 1 file changed, 61 insertions(+), 33 deletions(-) > > > > diff --git a/block/commit.c b/block/commit.c > > index 1a4a56795f..6673a0544e 100644 > > --- a/block/commit.c > > +++ b/block/commit.c > > @@ -35,7 +35,9 @@ typedef struct CommitBlockJob { > > BlockJob common; > > BlockDriverState *commit_top_bs; > > BlockBackend *top; > > + BlockDriverState *top_bs; > > BlockBackend *base; > > + BlockDriverState *base_bs; > > BlockdevOnError on_error; > > int base_flags; > > char *backing_file_str; > > More state. :-( > > Can we at least move the new fields to the end of the struct with a > comment that they are only valid after .exit()? > > > @@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base, > > static void commit_exit(Job *job) > > { > > CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > - BlockJob *bjob = &s->common; > > - BlockDriverState *top = blk_bs(s->top); > > - BlockDriverState *base = blk_bs(s->base); > > - BlockDriverState *commit_top_bs = s->commit_top_bs; > > - int ret = job->ret; > > - bool remove_commit_top_bs = false; > > + > > + s->top_bs = blk_bs(s->top); > > + s->base_bs = blk_bs(s->base); > > > > /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */ > > - bdrv_ref(top); > > - bdrv_ref(commit_top_bs); > > + bdrv_ref(s->top_bs); > > + bdrv_ref(s->commit_top_bs); > > +} These bdrv_refs() protect against nodes going away... > > - /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before > > - * the normal backing chain can be restored. */ > > - blk_unref(s->base); > > +static int commit_prepare(Job *job) > > +{ > > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > > > - if (!job_is_cancelled(job) && ret == 0) { > > - /* success */ > > - ret = bdrv_drop_intermediate(s->commit_top_bs, base, > > - s->backing_file_str); > > - } else { > > - /* XXX Can (or should) we somehow keep 'consistent read' blocked even > > - * after the failed/cancelled commit job is gone? If we already wrote > > - * something to base, the intermediate images aren't valid any more. */ > > - remove_commit_top_bs = true; > > - } > > + /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before > > + * the normal backing chain can be restored. */ > > + blk_unref(s->base); > > + s->base = NULL; > > + > > + return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs, > > + s->backing_file_str); > > The indentation is off here (which is weird because the additional space > seems to be the only change to most of the lines). > > > +} > > + > > +static void commit_exit_common(Job *job) > > +{ > > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > > > /* restore base open flags here if appropriate (e.g., change the base back > > * to r/o). These reopens do not need to be atomic, since we won't abort > > * even on failure here */ > > - if (s->base_flags != bdrv_get_flags(base)) { > > - bdrv_reopen(base, s->base_flags, NULL); > > + if (s->base_flags != bdrv_get_flags(s->base_bs)) { > > + bdrv_reopen(s->base_bs, s->base_flags, NULL); > > } > > g_free(s->backing_file_str); > > blk_unref(s->top); > > Feels like general cleanup, so intuitively, I'd expect this in .clean. > The only thing that could make this impossible is the ordering. > > bdrv_reopen() of s->base_bs should be safe to be deferred, the node > is still referenced after the job is concluded and we don't rely on it > being read-only again in any of the following completion code. > > g_free() is safe to be moved anyway. > > blk_unref(s->top) doesn't change the graph because we did a > bdrv_ref(s->top_bs). The permissions of the BlockBackend could still be > a problem. However, it doesn't take any permissions: > > s->top = blk_new(0, BLK_PERM_ALL); > > So I think moving this first part of commit_exit_common() to .clean > should be safe. > > > @@ -110,21 +112,43 @@ static void commit_exit(Job *job) > > * job_finish_sync()), job_completed() won't free it and therefore the > > * blockers on the intermediate nodes remain. This would cause > > * bdrv_set_backing_hd() to fail. */ > > - block_job_remove_all_bdrv(bjob); > > + block_job_remove_all_bdrv(&s->common); ...during block_job_remove_all_bdrv(), I think. Before we do this, we have a reference to each node in the subchain anyway. > There hasn't been any bdrv_set_backing_hd() close to this comment for a > while. Might be time to update it. > > I suppose the update would refer to bdrv_replace_node(), which only > happens in .abort, so should block_job_remove_all_bdrv() move, too? > > With these changes, commit_exit_common() would be gone. So if we move block_job_remove_all_bdrv() to .abort, the bdrv_ref() could become local to .abort as well. This wouldn't only get us rid of the additional state in CommitBlockJob, but in fact it would make .exit empty. And as it happens, commit is the only job that still has an .exit after this series, so the callback can go away altogether. Which poses the question whether it's really worth introducing it in the first place. Does it simplify the conversion much? But anyway, introducing it just for a short time and removing it again in the same series would be okay with me, too, if it makes your life any easier. Kevin > > +} > > + > > +static void commit_commit(Job *job) > > +{ > > + commit_exit_common(job); > > +} > > (I think I've answered this question now, by effectively proposing to do > away with .commit, but I'll leave it there anyway.) > > Is there any reason for the split between .prepare and .commit as it is > or is this mostly arbitrary? Probably the latter because commit isn't > even transactionable? > > > +static void commit_abort(Job *job) > > +{ > > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > + > > + if (s->base) { > > + blk_unref(s->base); > > + } > > + > > + commit_exit_common(job); > > + > > + /* XXX Can (or should) we somehow keep 'consistent read' blocked even > > + * after the failed/cancelled commit job is gone? If we already wrote > > + * something to base, the intermediate images aren't valid any more. */ > > > > /* If bdrv_drop_intermediate() didn't already do that, remove the commit > > * filter driver from the backing chain. Do this as the final step so that > > * the 'consistent read' permission can be granted. */ > > - if (remove_commit_top_bs) { > > - bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL, > > - &error_abort); > > - bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs), > > - &error_abort); > > - } > > + bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL, > > + &error_abort); > > + bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs), > > + &error_abort); > > +} > > > > - bdrv_unref(commit_top_bs); > > - bdrv_unref(top); > > - job->ret = ret; > > +static void commit_clean(Job *job) > > +{ > > + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); > > + > > + bdrv_unref(s->commit_top_bs); > > + bdrv_unref(s->top_bs); > > } > > > > static int coroutine_fn commit_run(Job *job) > > @@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = { > > .drain = block_job_drain, > > .start = commit_run, > > .exit = commit_exit, > > + .prepare = commit_prepare, > > + .commit = commit_commit, > > + .abort = commit_abort, > > + .clean = commit_clean > > }, > > }; > > Kevin >
On 08/09/2018 11:12 AM, Kevin Wolf wrote: > Am 07.08.2018 um 06:33 hat John Snow geschrieben: >> Use the component callbacks; prepare, commit, abort, and clean. >> >> NB: prepare is only called when the job has not yet failed; >> and abort can be called after prepare. >> >> complete -> prepare -> abort -> clean >> complete -> abort -> clean > > I always found this confusing. After converting all jobs to use the > infrastructure, do you think that this design is helpful? You seem to be > calling *_common function from commit and abort, with an almost empty > prepare, which looks like a hint that maybe we should reorganise things. > After rewriting things a bit, I think it would be nicer to call prepare regardless of circumstances. The callbacks will be nicer for it. When I wrote it the first time, the thought process was something like: "Well, we won't need to prepare anything if we've already failed, we just need to speed along to abort." I wasn't considering so carefully the common cleanup case that needs to occur post-finalization which appears to actually happen in a good number of jobs. >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/commit.c | 94 +++++++++++++++++++++++++++++++++++++--------------------- >> 1 file changed, 61 insertions(+), 33 deletions(-) >> >> diff --git a/block/commit.c b/block/commit.c >> index 1a4a56795f..6673a0544e 100644 >> --- a/block/commit.c >> +++ b/block/commit.c >> @@ -35,7 +35,9 @@ typedef struct CommitBlockJob { >> BlockJob common; >> BlockDriverState *commit_top_bs; >> BlockBackend *top; >> + BlockDriverState *top_bs; >> BlockBackend *base; >> + BlockDriverState *base_bs; >> BlockdevOnError on_error; >> int base_flags; >> char *backing_file_str; > > More state. :-( > > Can we at least move the new fields to the end of the struct with a > comment that they are only valid after .exit()? > Sure ... admittedly this is just a crutch because I was not precisely sure exactly when these might change out from underneath me. If there are some clever ways to avoid needing the state, feel free to suggest them. >> @@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base, >> static void commit_exit(Job *job) >> { >> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); >> - BlockJob *bjob = &s->common; >> - BlockDriverState *top = blk_bs(s->top); >> - BlockDriverState *base = blk_bs(s->base); >> - BlockDriverState *commit_top_bs = s->commit_top_bs; >> - int ret = job->ret; >> - bool remove_commit_top_bs = false; >> + >> + s->top_bs = blk_bs(s->top); >> + s->base_bs = blk_bs(s->base); >> >> /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */ >> - bdrv_ref(top); >> - bdrv_ref(commit_top_bs); >> + bdrv_ref(s->top_bs); >> + bdrv_ref(s->commit_top_bs); >> +} >> >> - /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before >> - * the normal backing chain can be restored. */ >> - blk_unref(s->base); >> +static int commit_prepare(Job *job) >> +{ >> + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); >> >> - if (!job_is_cancelled(job) && ret == 0) { >> - /* success */ >> - ret = bdrv_drop_intermediate(s->commit_top_bs, base, >> - s->backing_file_str); >> - } else { >> - /* XXX Can (or should) we somehow keep 'consistent read' blocked even >> - * after the failed/cancelled commit job is gone? If we already wrote >> - * something to base, the intermediate images aren't valid any more. */ >> - remove_commit_top_bs = true; >> - } >> + /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before >> + * the normal backing chain can be restored. */ >> + blk_unref(s->base); >> + s->base = NULL; >> + >> + return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs, >> + s->backing_file_str); > > The indentation is off here (which is weird because the additional space > seems to be the only change to most of the lines). > >> +} >> + >> +static void commit_exit_common(Job *job) >> +{ >> + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); >> >> /* restore base open flags here if appropriate (e.g., change the base back >> * to r/o). These reopens do not need to be atomic, since we won't abort >> * even on failure here */ >> - if (s->base_flags != bdrv_get_flags(base)) { >> - bdrv_reopen(base, s->base_flags, NULL); >> + if (s->base_flags != bdrv_get_flags(s->base_bs)) { >> + bdrv_reopen(s->base_bs, s->base_flags, NULL); >> } >> g_free(s->backing_file_str); >> blk_unref(s->top); > > Feels like general cleanup, so intuitively, I'd expect this in .clean. > The only thing that could make this impossible is the ordering. > > bdrv_reopen() of s->base_bs should be safe to be deferred, the node > is still referenced after the job is concluded and we don't rely on it > being read-only again in any of the following completion code. > > g_free() is safe to be moved anyway. > > blk_unref(s->top) doesn't change the graph because we did a > bdrv_ref(s->top_bs). The permissions of the BlockBackend could still be > a problem. However, it doesn't take any permissions: > > s->top = blk_new(0, BLK_PERM_ALL); > > So I think moving this first part of commit_exit_common() to .clean > should be safe. > OK, I'll try to do this a little more intelligently. This is definitely a bit of a hack-and-slash job; I'll look at your comments in the second reply here too and try to eliminate .exit which I agree is likely not strictly needed especially if we make prepare something that's always called. >> @@ -110,21 +112,43 @@ static void commit_exit(Job *job) >> * job_finish_sync()), job_completed() won't free it and therefore the >> * blockers on the intermediate nodes remain. This would cause >> * bdrv_set_backing_hd() to fail. */ >> - block_job_remove_all_bdrv(bjob); >> + block_job_remove_all_bdrv(&s->common); > > There hasn't been any bdrv_set_backing_hd() close to this comment for a > while. Might be time to update it. > > I suppose the update would refer to bdrv_replace_node(), which only > happens in .abort, so should block_job_remove_all_bdrv() move, too? > > With these changes, commit_exit_common() would be gone. > >> +} >> + >> +static void commit_commit(Job *job) >> +{ >> + commit_exit_common(job); >> +} > > (I think I've answered this question now, by effectively proposing to do > away with .commit, but I'll leave it there anyway.) > > Is there any reason for the split between .prepare and .commit as it is > or is this mostly arbitrary? Probably the latter because commit isn't > even transactionable? > Just historical, yeah -- we only had commit and abort for a while, and prepare didn't join the party until we wanted finalize and it became apparent we needed a way to check the return code and still be able to fail the transaction in time to be able to do a final commit or still abort very, very late in the process. Probably there's no real meaningful reason that prepare and commit need to be separate callbacks. It is possible that: prepare --> [abort] --> clean would be entirely sufficient for all current cases. >> +static void commit_abort(Job *job) >> +{ >> + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); >> + >> + if (s->base) { >> + blk_unref(s->base); >> + } >> + >> + commit_exit_common(job); >> + >> + /* XXX Can (or should) we somehow keep 'consistent read' blocked even >> + * after the failed/cancelled commit job is gone? If we already wrote >> + * something to base, the intermediate images aren't valid any more. */ >> >> /* If bdrv_drop_intermediate() didn't already do that, remove the commit >> * filter driver from the backing chain. Do this as the final step so that >> * the 'consistent read' permission can be granted. */ >> - if (remove_commit_top_bs) { >> - bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL, >> - &error_abort); >> - bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs), >> - &error_abort); >> - } >> + bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL, >> + &error_abort); >> + bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs), >> + &error_abort); >> +} >> >> - bdrv_unref(commit_top_bs); >> - bdrv_unref(top); >> - job->ret = ret; >> +static void commit_clean(Job *job) >> +{ >> + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); >> + >> + bdrv_unref(s->commit_top_bs); >> + bdrv_unref(s->top_bs); >> } >> >> static int coroutine_fn commit_run(Job *job) >> @@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = { >> .drain = block_job_drain, >> .start = commit_run, >> .exit = commit_exit, >> + .prepare = commit_prepare, >> + .commit = commit_commit, >> + .abort = commit_abort, >> + .clean = commit_clean >> }, >> }; > > Kevin >
Am 15.08.2018 um 23:17 hat John Snow geschrieben: > On 08/09/2018 11:12 AM, Kevin Wolf wrote: > > Am 07.08.2018 um 06:33 hat John Snow geschrieben: > >> Use the component callbacks; prepare, commit, abort, and clean. > >> > >> NB: prepare is only called when the job has not yet failed; > >> and abort can be called after prepare. > >> > >> complete -> prepare -> abort -> clean > >> complete -> abort -> clean > > > > I always found this confusing. After converting all jobs to use the > > infrastructure, do you think that this design is helpful? You seem to be > > calling *_common function from commit and abort, with an almost empty > > prepare, which looks like a hint that maybe we should reorganise things. > > > > After rewriting things a bit, I think it would be nicer to call prepare > regardless of circumstances. The callbacks will be nicer for it. > > When I wrote it the first time, the thought process was something like: > > "Well, we won't need to prepare anything if we've already failed, we > just need to speed along to abort." > > I wasn't considering so carefully the common cleanup case that needs to > occur post-finalization which appears to actually happen in a good > number of jobs. Maybe let's see how things turn out when we actually make some more jobs transactionable. For now, it seems that the *_common function can go away at least for commit; and we didn't even try to properly split the completion of the other two jobs. > >> Signed-off-by: John Snow <jsnow@redhat.com> > >> --- > >> block/commit.c | 94 +++++++++++++++++++++++++++++++++++++--------------------- > >> 1 file changed, 61 insertions(+), 33 deletions(-) > >> > >> diff --git a/block/commit.c b/block/commit.c > >> index 1a4a56795f..6673a0544e 100644 > >> --- a/block/commit.c > >> +++ b/block/commit.c > >> @@ -35,7 +35,9 @@ typedef struct CommitBlockJob { > >> BlockJob common; > >> BlockDriverState *commit_top_bs; > >> BlockBackend *top; > >> + BlockDriverState *top_bs; > >> BlockBackend *base; > >> + BlockDriverState *base_bs; > >> BlockdevOnError on_error; > >> int base_flags; > >> char *backing_file_str; > > > > More state. :-( > > > > Can we at least move the new fields to the end of the struct with a > > comment that they are only valid after .exit()? > > > > Sure ... admittedly this is just a crutch because I was not precisely > sure exactly when these might change out from underneath me. If there > are some clever ways to avoid needing the state, feel free to suggest them. I did, in the reply to my own mail. Everything that would need the state can actually live in .abort, so it can be local. > >> +} > >> + > >> +static void commit_commit(Job *job) > >> +{ > >> + commit_exit_common(job); > >> +} > > > > (I think I've answered this question now, by effectively proposing to do > > away with .commit, but I'll leave it there anyway.) > > > > Is there any reason for the split between .prepare and .commit as it is > > or is this mostly arbitrary? Probably the latter because commit isn't > > even transactionable? > > > > Just historical, yeah -- we only had commit and abort for a while, and > prepare didn't join the party until we wanted finalize and it became > apparent we needed a way to check the return code and still be able to > fail the transaction in time to be able to do a final commit or still > abort very, very late in the process. > > Probably there's no real meaningful reason that prepare and commit need > to be separate callbacks. > > It is possible that: > > prepare --> [abort] --> clean > > would be entirely sufficient for all current cases. I think jobs that actually support transactions (i.e. backup currently) do in fact need commit. The other ones might not. Kevin
diff --git a/block/commit.c b/block/commit.c index 1a4a56795f..6673a0544e 100644 --- a/block/commit.c +++ b/block/commit.c @@ -35,7 +35,9 @@ typedef struct CommitBlockJob { BlockJob common; BlockDriverState *commit_top_bs; BlockBackend *top; + BlockDriverState *top_bs; BlockBackend *base; + BlockDriverState *base_bs; BlockdevOnError on_error; int base_flags; char *backing_file_str; @@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base, static void commit_exit(Job *job) { CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); - BlockJob *bjob = &s->common; - BlockDriverState *top = blk_bs(s->top); - BlockDriverState *base = blk_bs(s->base); - BlockDriverState *commit_top_bs = s->commit_top_bs; - int ret = job->ret; - bool remove_commit_top_bs = false; + + s->top_bs = blk_bs(s->top); + s->base_bs = blk_bs(s->base); /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */ - bdrv_ref(top); - bdrv_ref(commit_top_bs); + bdrv_ref(s->top_bs); + bdrv_ref(s->commit_top_bs); +} - /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before - * the normal backing chain can be restored. */ - blk_unref(s->base); +static int commit_prepare(Job *job) +{ + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); - if (!job_is_cancelled(job) && ret == 0) { - /* success */ - ret = bdrv_drop_intermediate(s->commit_top_bs, base, - s->backing_file_str); - } else { - /* XXX Can (or should) we somehow keep 'consistent read' blocked even - * after the failed/cancelled commit job is gone? If we already wrote - * something to base, the intermediate images aren't valid any more. */ - remove_commit_top_bs = true; - } + /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before + * the normal backing chain can be restored. */ + blk_unref(s->base); + s->base = NULL; + + return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs, + s->backing_file_str); +} + +static void commit_exit_common(Job *job) +{ + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); /* restore base open flags here if appropriate (e.g., change the base back * to r/o). These reopens do not need to be atomic, since we won't abort * even on failure here */ - if (s->base_flags != bdrv_get_flags(base)) { - bdrv_reopen(base, s->base_flags, NULL); + if (s->base_flags != bdrv_get_flags(s->base_bs)) { + bdrv_reopen(s->base_bs, s->base_flags, NULL); } g_free(s->backing_file_str); blk_unref(s->top); @@ -110,21 +112,43 @@ static void commit_exit(Job *job) * job_finish_sync()), job_completed() won't free it and therefore the * blockers on the intermediate nodes remain. This would cause * bdrv_set_backing_hd() to fail. */ - block_job_remove_all_bdrv(bjob); + block_job_remove_all_bdrv(&s->common); +} + +static void commit_commit(Job *job) +{ + commit_exit_common(job); +} + +static void commit_abort(Job *job) +{ + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); + + if (s->base) { + blk_unref(s->base); + } + + commit_exit_common(job); + + /* XXX Can (or should) we somehow keep 'consistent read' blocked even + * after the failed/cancelled commit job is gone? If we already wrote + * something to base, the intermediate images aren't valid any more. */ /* If bdrv_drop_intermediate() didn't already do that, remove the commit * filter driver from the backing chain. Do this as the final step so that * the 'consistent read' permission can be granted. */ - if (remove_commit_top_bs) { - bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL, - &error_abort); - bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs), - &error_abort); - } + bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL, + &error_abort); + bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs), + &error_abort); +} - bdrv_unref(commit_top_bs); - bdrv_unref(top); - job->ret = ret; +static void commit_clean(Job *job) +{ + CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); + + bdrv_unref(s->commit_top_bs); + bdrv_unref(s->top_bs); } static int coroutine_fn commit_run(Job *job) @@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = { .drain = block_job_drain, .start = commit_run, .exit = commit_exit, + .prepare = commit_prepare, + .commit = commit_commit, + .abort = commit_abort, + .clean = commit_clean }, };
Use the component callbacks; prepare, commit, abort, and clean. NB: prepare is only called when the job has not yet failed; and abort can be called after prepare. complete -> prepare -> abort -> clean complete -> abort -> clean Signed-off-by: John Snow <jsnow@redhat.com> --- block/commit.c | 94 +++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 61 insertions(+), 33 deletions(-)