Re: [Qemu-devel] [PATCH v10 3/4] block: Add replaces argument to drive-mirror

[Benoît Canet]

The Friday 27 Jun 2014 à 15:37:00 (+0200), Kevin Wolf wrote :
> Am 27.06.2014 um 14:53 hat Benoît Canet geschrieben:
> > The Friday 27 Jun 2014 à 13:57:02 (+0200), Kevin Wolf wrote :
> > > Am 16.06.2014 um 12:00 hat Benoît Canet geschrieben:
> > > > drive-mirror will bdrv_swap the new BDS named node-name with the one
> > > > pointed by replaces when the mirroring is finished.
> > > > 
> > > > Signed-off-by: Benoit Canet <address@hidden>
> > > > Reviewed-by: Max Reitz <address@hidden>
> > > > ---
> > > >  block.c                   | 17 ++++++++++++++
> > > >  block/mirror.c            | 60 
> > > > +++++++++++++++++++++++++++++++++++++----------
> > > >  blockdev.c                | 30 +++++++++++++++++++++++-
> > > >  hmp.c                     |  2 +-
> > > >  include/block/block.h     |  4 ++++
> > > >  include/block/block_int.h |  3 +++
> > > >  qapi/block-core.json      |  6 ++++-
> > > >  qmp-commands.hx           |  4 +++-
> > > >  8 files changed, 109 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index 17f763d..318f1e6 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -5795,3 +5795,20 @@ bool bdrv_is_first_non_filter(BlockDriverState 
> > > > *candidate)
> > > >  
> > > >      return false;
> > > >  }
> > > > +
> > > > +BlockDriverState *check_to_replace_node(const char *node_name, Error 
> > > > **errp)
> > > > +{
> > > > +    BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
> > > > +    if (!to_replace_bs) {
> > > > +        error_setg(errp, "Node name '%s' not found",
> > > > +                   node_name);
> > > 
> > > Unnecessary line break.
> > > 
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, 
> > > > errp)) {
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    return to_replace_bs;
> > > > +}
> > > > +
> > > 
> > > Empty line before EOF.
> > > 
> > > > diff --git a/block/mirror.c b/block/mirror.c
> > > > index 94c8661..151167e 100644
> > > > --- a/block/mirror.c
> > > > +++ b/block/mirror.c
> > > > @@ -32,6 +32,12 @@ typedef struct MirrorBlockJob {
> > > >      RateLimit limit;
> > > >      BlockDriverState *target;
> > > >      BlockDriverState *base;
> > > > +    /* The name of the graph node to replace */
> > > > +    char *replaces;
> > > > +    /* The BDS to replace */
> > > > +    BlockDriverState *to_replace;
> > > > +    /* Used to block operations on the drive-mirror-replace target */
> > > > +    Error *replace_blocker;
> > > >      bool is_none_mode;
> > > >      BlockdevOnError on_source_error, on_target_error;
> > > >      bool synced;
> > > > @@ -490,10 +496,14 @@ immediate_exit:
> > > >      bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
> > > >      bdrv_iostatus_disable(s->target);
> > > >      if (s->should_complete && ret == 0) {
> > > > -        if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) 
> > > > {
> > > > -            bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
> > > > +        BlockDriverState *to_replace = s->common.bs;
> > > > +        if (s->to_replace) {
> > > > +            to_replace = s->to_replace;
> > > >          }
> > > > -        bdrv_swap(s->target, s->common.bs);
> > > > +        if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
> > > > +            bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
> > > > +        }
> > > > +        bdrv_swap(s->target, to_replace);
> > > >          if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
> > > >              /* drop the bs loop chain formed by the swap: break the 
> > > > loop then
> > > >               * trigger the unref from the top one */
> > > > @@ -502,6 +512,12 @@ immediate_exit:
> > > >              bdrv_unref(p);
> > > >          }
> > > >      }
> > > > +    if (s->to_replace) {
> > > > +        bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
> > > > +        error_free(s->replace_blocker);
> > > > +        bdrv_unref(s->to_replace);
> > > > +    }
> > > > +    g_free(s->replaces);
> > > >      bdrv_unref(s->target);
> > > >      block_job_completed(&s->common, ret);
> > > >  }
> > > > @@ -540,6 +556,20 @@ static void mirror_complete(BlockJob *job, Error 
> > > > **errp)
> > > >          return;
> > > >      }
> > > >  
> > > > +    /* check the target bs is not blocked and block all operations on 
> > > > it */
> > > > +    if (s->replaces) {
> > > > +        s->to_replace = check_to_replace_node(s->replaces, errp);
> > > > +
> > > 
> > > This empty line looks unusual.
> > > 
> > > > +        if (!s->to_replace) {
> > > > +            return;
> > > > +        }
> > > 
> > > So here is the thing that I really wanted to comment on. In the case of
> > > a REPLACE blocker being set, this is a silent failure.
> > 
> > Why would it be silent ?
> > errp is directly passed to check_to_replace_node.
> 
> Ah, sorry, my bad. I missed that bdrv_op_is_blocked() sets errp. Which
> is a bit odd for a function with this name. I had expected that it
> simply returns true if the op is blocked and sets an error only if it
> can't answer the question.

I passed &local_err and propagated it to errp to make it clearer in the
new version.

Best regards

Benoît

> 
> Not a problem of your patch, though.
> 
> Kevin
> 
> > > The completion
> > > command will return success, but s->should_complete won't actually be
> > > set, so the completion doesn't happen. The only thing that actually
> > > happens is the bdrv_open_backing_file(s->target) (which looks somewhat
> > > questionable, too...)
> > > 
> > > Now I would expect that the REPLACE blocker is actually set for any
> > > backing file, because that is what bdrv_set_backing_hd() does. For
> > > quorum it does work as expected because quorum children don't get any
> > > backing_blocker (we need to check whether they should get something
> > > similar from the quorum BDS), so this is probably why it escaped your
> > > testing. We'll need a test case that tries replacing some ordinary
> > > backing file.
> > > 
> > > Now I think the (accidental?) restriction to only replacing quorum nodes
> > > actually makes this patch pretty safe, so maybe it would be nice to keep
> > > this behaviour; but we need to fix it to not fail silently but return an
> > > explicit error.
>