Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks

[Kevin Wolf]

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 <address@hidden>
> >> ---
> >>  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