Re: [Qemu-devel] [PATCH v2 07/16] block: Convert bs->backing_hd to BdrvChild

[Fam Zheng <famz@redhat.com>]

On Thu, 10/01 15:13, Kevin Wolf wrote:
> This is the final step in converting all of the BlockDriverState
> pointers that block drivers use to BdrvChild.
> 
> After this patch, bs->children contains the full list of child nodes
> that are referenced by a given BDS, and these children are only
> referenced through BdrvChild, so that updating the pointer in there is
> enough for changing edges in the graph.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 116 +++++++++++++++++++++++++---------------------
>  block/io.c                |  24 +++++-----
>  block/mirror.c            |   6 +--
>  block/qapi.c              |   8 ++--
>  block/qcow.c              |   4 +-
>  block/qcow2-cluster.c     |   4 +-
>  block/qcow2.c             |   6 +--
>  block/qed.c               |  12 ++---
>  block/stream.c            |   8 ++--
>  block/vmdk.c              |  21 +++++----
>  block/vvfat.c             |   6 +--
>  blockdev.c                |   4 +-
>  include/block/block_int.h |  12 +++--
>  qemu-img.c                |   4 +-
>  14 files changed, 125 insertions(+), 110 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 33ecd93..20a3c5c 100644
> --- a/block.c
> +++ b/block.c
> @@ -721,7 +721,7 @@ const BdrvChildRole child_format = {
>  };
>  
>  /*
> - * Returns the flags that bs->backing_hd should get, based on the given flags
> + * Returns the flags that bs->backing should get, based on the given flags
>   * for the parent BDS
>   */
>  static int bdrv_backing_flags(int flags)
> @@ -1115,32 +1115,31 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
>  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>  {
>  
> -    if (bs->backing_hd) {
> +    if (bs->backing) {
>          assert(bs->backing_blocker);
> -        bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> -        bdrv_detach_child(bs->backing_child);
> +        bdrv_op_unblock_all(bs->backing->bs, bs->backing_blocker);
> +        bdrv_detach_child(bs->backing);
>      } else if (backing_hd) {
>          error_setg(&bs->backing_blocker,
>                     "node is used as backing hd of '%s'",
>                     bdrv_get_device_or_node_name(bs));
>      }
>  
> -    bs->backing_hd = backing_hd;
>      if (!backing_hd) {
>          error_free(bs->backing_blocker);
>          bs->backing_blocker = NULL;
> -        bs->backing_child = NULL;
> +        bs->backing = NULL;
>          goto out;
>      }
> -    bs->backing_child = bdrv_attach_child(bs, backing_hd, &child_backing);
> +    bs->backing = bdrv_attach_child(bs, backing_hd, &child_backing);
>      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);
> +    bdrv_op_block_all(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_TARGET,
> +    bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
>                      bs->backing_blocker);
>  out:
>      bdrv_refresh_limits(bs, NULL);
> @@ -1161,7 +1160,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>      BlockDriverState *backing_hd;
>      Error *local_err = NULL;
>  
> -    if (bs->backing_hd != NULL) {
> +    if (bs->backing != NULL) {
>          QDECREF(options);
>          goto free_exit;
>      }
> @@ -1201,7 +1200,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>          qdict_put(options, "driver", qstring_from_str(bs->backing_format));
>      }
>  
> -    assert(bs->backing_hd == NULL);
> +    assert(bs->backing == NULL);
>      ret = bdrv_open_inherit(&backing_hd,
>                              *backing_filename ? backing_filename : NULL,
>                              NULL, options, 0, bs, &child_backing, &local_err);
> @@ -1892,8 +1891,8 @@ void bdrv_close(BlockDriverState *bs)
>          bs->drv->bdrv_close(bs);
>          bs->drv = NULL;
>  
> -        if (bs->backing_hd) {
> -            BlockDriverState *backing_hd = bs->backing_hd;
> +        if (bs->backing) {
> +            BlockDriverState *backing_hd = bs->backing->bs;
>              bdrv_set_backing_hd(bs, NULL);
>              bdrv_unref(backing_hd);
>          }
> @@ -2205,20 +2204,20 @@ int bdrv_commit(BlockDriverState *bs)
>      if (!drv)
>          return -ENOMEDIUM;
>  
> -    if (!bs->backing_hd) {
> +    if (!bs->backing) {
>          return -ENOTSUP;
>      }
>  
>      if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT_SOURCE, NULL) ||
> -        bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET, NULL)) {
> +        bdrv_op_is_blocked(bs->backing->bs, BLOCK_OP_TYPE_COMMIT_TARGET, NULL)) {
>          return -EBUSY;
>      }
>  
> -    ro = bs->backing_hd->read_only;
> -    open_flags =  bs->backing_hd->open_flags;
> +    ro = bs->backing->bs->read_only;
> +    open_flags =  bs->backing->bs->open_flags;
>  
>      if (ro) {
> -        if (bdrv_reopen(bs->backing_hd, open_flags | BDRV_O_RDWR, NULL)) {
> +        if (bdrv_reopen(bs->backing->bs, open_flags | BDRV_O_RDWR, NULL)) {
>              return -EACCES;
>          }
>      }
> @@ -2229,7 +2228,7 @@ int bdrv_commit(BlockDriverState *bs)
>          goto ro_cleanup;
>      }
>  
> -    backing_length = bdrv_getlength(bs->backing_hd);
> +    backing_length = bdrv_getlength(bs->backing->bs);
>      if (backing_length < 0) {
>          ret = backing_length;
>          goto ro_cleanup;
> @@ -2239,7 +2238,7 @@ int bdrv_commit(BlockDriverState *bs)
>       * grow the backing file image if possible.  If not possible,
>       * we must return an error */
>      if (length > backing_length) {
> -        ret = bdrv_truncate(bs->backing_hd, length);
> +        ret = bdrv_truncate(bs->backing->bs, length);
>          if (ret < 0) {
>              goto ro_cleanup;
>          }
> @@ -2248,7 +2247,7 @@ int bdrv_commit(BlockDriverState *bs)
>      total_sectors = length >> BDRV_SECTOR_BITS;
>  
>      /* qemu_try_blockalign() for bs will choose an alignment that works for
> -     * bs->backing_hd as well, so no need to compare the alignment manually. */
> +     * bs->backing->bs as well, so no need to compare the alignment manually. */
>      buf = qemu_try_blockalign(bs, COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
>      if (buf == NULL) {
>          ret = -ENOMEM;
> @@ -2266,7 +2265,7 @@ int bdrv_commit(BlockDriverState *bs)
>                  goto ro_cleanup;
>              }
>  
> -            ret = bdrv_write(bs->backing_hd, sector, buf, n);
> +            ret = bdrv_write(bs->backing->bs, sector, buf, n);
>              if (ret < 0) {
>                  goto ro_cleanup;
>              }
> @@ -2285,8 +2284,8 @@ int bdrv_commit(BlockDriverState *bs)
>       * Make sure all data we wrote to the backing device is actually
>       * stable on disk.
>       */
> -    if (bs->backing_hd) {
> -        bdrv_flush(bs->backing_hd);
> +    if (bs->backing) {
> +        bdrv_flush(bs->backing->bs);
>      }
>  
>      ret = 0;
> @@ -2295,7 +2294,7 @@ ro_cleanup:
>  
>      if (ro) {
>          /* ignoring error return here */
> -        bdrv_reopen(bs->backing_hd, open_flags & ~BDRV_O_RDWR, NULL);
> +        bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
>      }
>  
>      return ret;
> @@ -2309,7 +2308,7 @@ int bdrv_commit_all(void)
>          AioContext *aio_context = bdrv_get_aio_context(bs);
>  
>          aio_context_acquire(aio_context);
> -        if (bs->drv && bs->backing_hd) {
> +        if (bs->drv && bs->backing) {
>              int ret = bdrv_commit(bs);
>              if (ret < 0) {
>                  aio_context_release(aio_context);
> @@ -2366,11 +2365,20 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>  BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>                                      BlockDriverState *bs)
>  {
> -    while (active && bs != active->backing_hd) {
> -        active = active->backing_hd;
> +    while (active) {
> +        if (active->backing) {
> +            if (bs == active->backing->bs) {
> +                return active;
> +            }
> +            active = active->backing->bs;
> +        } else if (bs == NULL) {
> +            return active;
> +        } else {
> +            return NULL;
> +        }

Why not just

    while (active && bs != backing_bs(active)) {
        active = backing_bs(active);
    }

    return active;

?

>      }
>  
> -    return active;
> +    return NULL;
>  }
>  
>  /* Given a BDS, searches for the base layer. */