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 <js...@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