Re: [Qemu-devel] [PATCH v2 05/13] block/mirror: conservative mirror_exit refactor

[Max Reitz]

On 2018-08-24 00:22, John Snow wrote:
> For purposes of minimum code movement, refactor the mirror_exit
> callback to use the post-finalization callbacks in a trivial way.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/mirror.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 837279e979..f5b504406d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
>      int max_iov;
>      bool initial_zeroing_ongoing;
>      int in_active_write_counter;
> +    bool prepared;
>  } MirrorBlockJob;
>  
>  typedef struct MirrorBDSOpaque {
> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
>      }
>  }
>  
> -static void mirror_exit(Job *job)
> +static int mirror_exit_common(Job *job)
>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>      BlockJob *bjob = &s->common;
> @@ -619,6 +620,11 @@ static void mirror_exit(Job *job)
>      Error *local_err = NULL;
>      int ret = job->ret;
>  
> +    if (s->prepared) {
> +        return ret;

I'd prefer "return 0".  We only get here from mirror_abort(), and
mirror_abort() cannot fail, so this function should not report failure then.

Also, the error has already been reported (as evidenced by the fact that
job->ret < 0), so there is no need to report it again.  The function is
supposed to be a no-op on its second call, and it was successful in
doing so.

> +    }
> +    s->prepared = true;
> +
>      bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>  
>      /* Make sure that the source BDS doesn't go away before we called
> @@ -711,7 +717,17 @@ static void mirror_exit(Job *job)
>      bdrv_unref(mirror_top_bs);
>      bdrv_unref(src);
>  
> -    job->ret = ret;
> +    return ret;
> +}
> +
> +static int mirror_prepare(Job *job)
> +{
> +    return mirror_exit_common(job);
> +}
> +
> +static void mirror_abort(Job *job)
> +{
> +    mirror_exit_common(job);

Hm.  If we cannot assert that this always returns 0 (given that job->ret
is negative before, which it is here), then something is off.

The first thing obviously is the "return ret" you add in this patch, but
that's easy to get rid off.

The second thing is the ret = -EPERM in the "if (s->backing_mode ==
MIRROR_SOURCE_BACKING_CHAIN)" path.  Now the first question is, can we
skip that on error?

The answer is "nobody would notice, but not really?".  First, we
probably usually want a working target node for mirror even on failure.
Now, this backing_mode here is used only for drive-mirror, so usually
when that job fails, the target just goes aways (and it wouldn't matter
what backing chain it had).

But you can give the target a node-name and then at least theoretically
reference it while the job is running.  Then the backing chain after
failure would matter.

Though OTOH, we do not share CONSISTENT_READ on the target, so if you do
that, it's not like we ever promised you you'd get the right backing
chain at any point...  (And not sharing CONSISTENT_READ means that you
really cannot attach the node to anything while the job is running.)

So we could just not attach the target backing chain if job->ret < 0.
At least we could ignore errors.   But it'd need an explanation (namely
"We didn't share CONSISTENT_READ, so we can ignore errors here").


The second idea would be: We only need a valid target when the job was
actually completed.  Looking it up unveils that cancelling a mirror
(post-READY) does not make it abort; job->ret is still 0.  So I suppose
when we ever get to mirror_abort() without mirror_prepare() having been
called before, that is not a point where the target needs to be made
consistent.  So that also means we can just ignore the error then.


All in all, I would say that if we are in .abort() (job->ret < 0), it
should ignore this specific error (i.e. not overwrite @ret, which is
already negative), because

(1) The target never shared CONSISTENT_READ anyway, so if there are
other parents already, they won't be surprised that the backing chain is
still missing; and if there are no other parents, it doesn't matter what
we do because the target is going to bdrv_delete()'d after the job anyway.
(2) While we can make an effort to get a half-consistent target even
when the job has failed, chances are that it will be inconsistent
anyway.  Also, nobody expects mirror to leave behind a consistent target
when failing.  (Note: Post-READY cancel (to complete) does not count as
failure.)


The basic chance looks good, but I'd really like a working assertion
here that mirror_exit_common() has returned 0.

Max

>  }
>  
>  static void mirror_throttle(MirrorBlockJob *s)
> @@ -1132,7 +1148,8 @@ static const BlockJobDriver mirror_job_driver = {
>          .user_resume            = block_job_user_resume,
>          .drain                  = block_job_drain,
>          .run                    = mirror_run,
> -        .exit                   = mirror_exit,
> +        .prepare                = mirror_prepare,
> +        .abort                  = mirror_abort,
>          .pause                  = mirror_pause,
>          .complete               = mirror_complete,
>      },
> @@ -1149,7 +1166,8 @@ static const BlockJobDriver commit_active_job_driver = {
>          .user_resume            = block_job_user_resume,
>          .drain                  = block_job_drain,
>          .run                    = mirror_run,
> -        .exit                   = mirror_exit,
> +        .prepare                = mirror_prepare,
> +        .abort                  = mirror_abort,
>          .pause                  = mirror_pause,
>          .complete               = mirror_complete,
>      },
>

signature.asc
Description: OpenPGP digital signature