Re: [Qemu-devel] [PATCH 1/9] block: Use BdrvChild callbacks for change_media/resize

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6019 bytes --]

On 22.03.2016 20:36, Kevin Wolf wrote:
> We want to get rid of BlockDriverState.blk in order to allow multiple
> BlockBackends per BDS. Converting the device callbacks in block.c (which
> assume a single BlockBackend) to per-child callbacks gets us rid of the
> first few instances.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 38 +++++++++++++++++++++++++-------------
>  block/block-backend.c     | 15 ++++++++++++++-
>  include/block/block_int.h |  4 +++-
>  3 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fb37b91..1fb5dac 100644
> --- a/block.c
> +++ b/block.c
> @@ -1216,6 +1216,27 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
>      bdrv_root_unref_child(child);
>  }
>  
> +
> +static void bdrv_parent_cb_change_media(BlockDriverState *bs, bool load)
> +{
> +    BdrvChild *c;
> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
> +        if (c->role->change_media) {
> +            c->role->change_media(c, load);
> +        }
> +    }
> +}
> +
> +static void bdrv_parent_cb_resize(BlockDriverState *bs)
> +{
> +    BdrvChild *c;
> +    QLIST_FOREACH(c, &bs->parents, next_parent) {
> +        if (c->role->resize) {
> +            c->role->resize(c);
> +        }
> +    }
> +}
> +
>  /*
>   * Sets the backing file link of a BDS. A new reference is created; callers
>   * which don't need their own reference any more must call bdrv_unref().
> @@ -1674,9 +1695,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>      }
>  
>      if (!bdrv_key_required(bs)) {
> -        if (bs->blk) {
> -            blk_dev_change_media_cb(bs->blk, true);
> -        }
> +        bdrv_parent_cb_change_media(bs, true);
>      } else if (!runstate_check(RUN_STATE_PRELAUNCH)
>                 && !runstate_check(RUN_STATE_INMIGRATE)
>                 && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
> @@ -2122,9 +2141,7 @@ static void bdrv_close(BlockDriverState *bs)
>      bdrv_release_named_dirty_bitmaps(bs);
>      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>  
> -    if (bs->blk) {
> -        blk_dev_change_media_cb(bs->blk, false);
> -    }
> +    bdrv_parent_cb_change_media(bs, false);

This is probably unnecessary altogether, but I'm certain about it after
http://lists.nongnu.org/archive/html/qemu-block/2015-11/msg00349.html.
So I'll take a note of removing this then.

>      if (bs->drv) {
>          BdrvChild *child, *next;
> @@ -2605,9 +2622,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>      if (ret == 0) {
>          ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>          bdrv_dirty_bitmap_truncate(bs);
> -        if (bs->blk) {
> -            blk_dev_resize_cb(bs->blk);
> -        }
> +        bdrv_parent_cb_resize(bs);
>      }
>      return ret;
>  }
> @@ -2718,10 +2733,7 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
>          bs->valid_key = 0;
>      } else if (!bs->valid_key) {
>          bs->valid_key = 1;
> -        if (bs->blk) {
> -            /* call the change callback now, we skipped it on open */
> -            blk_dev_change_media_cb(bs->blk, true);
> -        }
> +        bdrv_parent_cb_change_media(bs, true);

Not sure why you removed the comment here. In case you send a v2, I'd
keep it.

Whichever way you decide:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>      }
>      return ret;
>  }
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 8b4eb1a..9d973ba 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -92,9 +92,15 @@ static void blk_root_inherit_options(int *child_flags, QDict *child_options,
>  }
>  static bool blk_drain_throttling_queue(BdrvChild *child);
>  
> +static void blk_root_change_media(BdrvChild *child, bool load);
> +static void blk_root_resize(BdrvChild *child);
> +
>  static const BdrvChildRole child_root = {
>      .inherit_options    = blk_root_inherit_options,
>  
> +    .change_media       = blk_root_change_media,
> +    .resize             = blk_root_resize,
> +
>      .drain_queue        = blk_drain_throttling_queue,
>  };
>  
> @@ -553,6 +559,11 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool load)
>      }
>  }
>  
> +static void blk_root_change_media(BdrvChild *child, bool load)
> +{
> +    blk_dev_change_media_cb(child->opaque, load);
> +}
> +
>  /*
>   * Does @blk's attached device model have removable media?
>   * %true if no device model is attached.
> @@ -607,8 +618,10 @@ bool blk_dev_is_medium_locked(BlockBackend *blk)
>  /*
>   * Notify @blk's attached device model of a backend size change.
>   */
> -void blk_dev_resize_cb(BlockBackend *blk)
> +static void blk_root_resize(BdrvChild *child)
>  {
> +    BlockBackend *blk = child->opaque;
> +
>      if (blk->dev_ops && blk->dev_ops->resize_cb) {
>          blk->dev_ops->resize_cb(blk->dev_opaque);
>      }
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 000d2c0..f60cb7c 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -356,6 +356,9 @@ struct BdrvChildRole {
>      void (*inherit_options)(int *child_flags, QDict *child_options,
>                              int parent_flags, QDict *parent_options);
>  
> +    void (*change_media)(BdrvChild *child, bool load);
> +    void (*resize)(BdrvChild *child);
> +
>      bool (*drain_queue)(BdrvChild *child);
>  };
>  
> @@ -695,7 +698,6 @@ bool blk_dev_has_tray(BlockBackend *blk);
>  void blk_dev_eject_request(BlockBackend *blk, bool force);
>  bool blk_dev_is_tray_open(BlockBackend *blk);
>  bool blk_dev_is_medium_locked(BlockBackend *blk);
> -void blk_dev_resize_cb(BlockBackend *blk);
>  
>  void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
>  bool bdrv_requests_pending(BlockDriverState *bs);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]