qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks
Date: Wed, 15 Aug 2018 17:17:10 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0


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

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

-- 
—js



reply via email to

[Prev in Thread] Current Thread [Next in Thread]