[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 16/21] block/commit: refactor commit to use job
From: |
John Snow |
Subject: |
Re: [Qemu-block] [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
- [Qemu-block] [PATCH 19/21] qapi/block-commit: expose new job properties, (continued)
- [Qemu-block] [PATCH 19/21] qapi/block-commit: expose new job properties, John Snow, 2018/08/07
- [Qemu-block] [PATCH 15/21] block/stream: add block job creation flags, John Snow, 2018/08/07
- [Qemu-block] [PATCH 11/21] jobs: remove job_defer_to_main_loop, John Snow, 2018/08/07
- [Qemu-block] [PATCH 14/21] block/mirror: add block job creation flags, John Snow, 2018/08/07
- [Qemu-block] [PATCH 16/21] block/commit: refactor commit to use job callbacks, John Snow, 2018/08/07
- [Qemu-block] [PATCH 18/21] block/commit: refactor stream to use job callbacks, John Snow, 2018/08/07
- [Qemu-block] [PATCH 21/21] qapi/block-stream: expose new job properties, John Snow, 2018/08/07
- Re: [Qemu-block] [PATCH 00/21] jobs: defer graph changes until finalize, Peter Krempa, 2018/08/15