Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks
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
> >> ---
> >> 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
Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks
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
>> ---
>> 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 = >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.
>
>
Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks
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
> ---
> 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 = >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);
> +
