Re: [Qemu-devel] [PATCH v17 06/14] block: Add backing_blocker in BlockDriverState

[Fam Zheng <famz@redhat.com>]

On Sun, 04/06 20:31, Jeff Cody wrote:
> On Mon, Mar 10, 2014 at 03:26:02PM +0800, Fam Zheng wrote:
> > This makes use of op_blocker and blocks all the operations except for
> > commit target, on each BlockDriverState->backing_hd.
> > 
> > The asserts for op_blocker in bdrv_swap are removed because with this
> > change, the target of block commit has at least the backing blocker of
> > its child, so the assertion is not true. Callers should do their check.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c                   | 24 ++++++++++++++++++++----
> >  block/mirror.c            |  1 +
> >  include/block/block_int.h |  3 +++
> >  3 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 64738dc..95247c8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1050,16 +1050,33 @@ fail:
> >  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
> >  {
> >  
> > +    if (bs->backing_hd) {
> > +        assert(error_is_set(&bs->backing_blocker));
> > +        bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> > +    } else if (backing_hd) {
> > +        error_setg(&bs->backing_blocker,
> > +                   "device is used as backing hd of '%s'",
> > +                   bs->device_name);
> > +    }
> > +
> >      bs->backing_hd = backing_hd;
> >      if (!backing_hd) {
> >          bs->backing_file[0] = '\0';
> >          bs->backing_format[0] = '\0';
> > +        if (error_is_set(&bs->backing_blocker)) {
> > +            error_free(bs->backing_blocker);
> > +        }
> >          goto out;
> >      }
> >      bs->open_flags &= ~BDRV_O_NO_BACKING;
> >      pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
> >      pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> >              backing_hd->drv ? backing_hd->drv->format_name : "");
> > +
> > +    bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);
> > +    /* Otherwise we won't be able to commit due to check in bdrv_commit */
> > +    bdrv_op_unblock(bs->backing_hd, BLOCK_OP_TYPE_COMMIT,
> > +                    bs->backing_blocker);
> >  out:
> >      bdrv_refresh_limits(bs);
> >  }
> > @@ -1699,8 +1716,9 @@ void bdrv_close(BlockDriverState *bs)
> >  
> >      if (bs->drv) {
> >          if (bs->backing_hd) {
> > -            bdrv_unref(bs->backing_hd);
> > -            bs->backing_hd = NULL;
> > +            BlockDriverState *backing_hd = bs->backing_hd;
> > +            bdrv_set_backing_hd(bs, NULL);
> > +            bdrv_unref(backing_hd);
> >          }
> >          bs->drv->bdrv_close(bs);
> >          g_free(bs->opaque);
> > @@ -1908,7 +1926,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
> >      assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
> >      assert(bs_new->job == NULL);
> >      assert(bs_new->dev == NULL);
> > -    assert(bdrv_op_blocker_is_empty(bs_new));
> >      assert(bs_new->io_limits_enabled == false);
> >      assert(!throttle_have_timer(&bs_new->throttle_state));
> >  
> > @@ -1927,7 +1944,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
> >      /* Check a few fields that should remain attached to the device */
> >      assert(bs_new->dev == NULL);
> >      assert(bs_new->job == NULL);
> > -    assert(bdrv_op_blocker_is_empty(bs_new));
> 
> Do we want to unswap the blocker field inside bdrv_move_feature_fields()
> now?

Conceptually a BDS's blocker is a "feature field" (whatever it is) that
shouldn't be swapped by bdrv_swap(), just as .device_name and .refcnt. So it is
added to bdrv_move_feature_fields() since introduced, in patch 02.

Are you seeing any issue with this?

Fam

> 
> >      assert(bs_new->io_limits_enabled == false);
> >      assert(!throttle_have_timer(&bs_new->throttle_state));
> >  
> > diff --git a/block/mirror.c b/block/mirror.c
> > index dd5ee05..6dc84e8 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -487,6 +487,7 @@ immediate_exit:
> >               * trigger the unref from the top one */
> >              BlockDriverState *p = s->base->backing_hd;
> >              s->base->backing_hd = NULL;
> > +            bdrv_op_unblock_all(p, s->base->backing_blocker);
> >              bdrv_unref(p);
> >          }
> >      }
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 1d3f76f..1f4f78b 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -369,6 +369,9 @@ struct BlockDriverState {
> >      BlockJob *job;
> >  
> >      QDict *options;
> > +
> > +    /* The error object in use for blocking operations on backing_hd */
> > +    Error *backing_blocker;
> >  };
> >  
> >  int get_tmp_filename(char *filename, int size);
> > -- 
> > 1.9.0
> > 
> > 
>