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

[Jeff Cody <jcody@redhat.com>]

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));
>      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);

FYI, this is what I changed in my testing, to try out the active layer
case in bdrv_drop_intermediate().  Since you'll need to respin anyway,
might as well clean this up to use the updated
bdrv_drop_intermediate():


--- a/block/mirror.c
+++ b/block/mirror.c
@@ -490,15 +490,8 @@ immediate_exit:
         if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
             bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
         }
-        bdrv_swap(s->target, s->common.bs);
-        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 */
-            BlockDriverState *p = s->base->backing_hd;
-            s->base->backing_hd = NULL;
-            bdrv_op_unblock_all(p, s->base->backing_blocker);
-            bdrv_unref(p);
-        }
+        bdrv_op_unblock_all(s->common.bs->backing_hd, s->common.bs->backing_blocker);
+        bdrv_drop_intermediate(s->common.bs, s->common.bs, s->target);
     }
     bdrv_unref(s->target);
     block_job_completed(&s->common, ret);