Re: [Qemu-devel] [PATCH 03/23] block: Connect BlockBackend to BlockDriverState

[Kevin Wolf]

Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben:
> The pointer from BlockBackend to BlockDriverState is a strong
> reference, managed with bdrv_ref() / bdrv_unref(), the back-pointer is
> a weak one.
> 
> Convenience function blk_new_with_bs() creates a BlockBackend with its
> BlockDriverState.  Callers have to unref both.  The commit after next
> will relieve them of the need to unref the BlockDriverState.
> 
> Signed-off-by: Markus Armbruster <address@hidden>

I'm wondering whether this is the right direction to take. In creating a
block device, we have the following operations:

1. Create a BlockBackend
2. Create a BlockDriverState
3. Open the BlockDriverState

bdrv_open() can do 2 and 3, which is probably what we want to have in
the end - new/create and delete/close should be a single operation for
BDSes.

Combining 1 and 2 into a single function might make it harder to actually
use bdrv_open() in this way.

> diff --git a/blockdev.c b/blockdev.c
> index 86596bc..0a0b95e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -304,6 +304,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>      int bdrv_flags = 0;
>      int on_read_error, on_write_error;
>      BlockBackend *blk;
> +    BlockDriverState *bs;
>      DriveInfo *dinfo;
>      ThrottleConfig cfg;
>      int snapshot = 0;
> @@ -459,30 +460,28 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>      }
>  
>      /* init */
> -    blk = blk_new(qemu_opts_id(opts), errp);
> +    blk = blk_new_with_bs(qemu_opts_id(opts), errp);
>      if (!blk) {
>          goto early_err;
>      }
> +    bs = blk_bs(blk);
> +    bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> +    bs->read_only = ro;
> +    bs->detect_zeroes = detect_zeroes;
> +
> +    bdrv_set_on_error(bs, on_read_error, on_write_error);
> +
> +    /* disk I/O throttling */
> +    if (throttle_enabled(&cfg)) {
> +        bdrv_io_limits_enable(bs);
> +        bdrv_set_io_limits(bs, &cfg);
> +    }
> +
>      dinfo = g_malloc0(sizeof(*dinfo));
>      dinfo->id = g_strdup(qemu_opts_id(opts));
> -    dinfo->bdrv = bdrv_new_named(dinfo->id, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        goto bdrv_new_err;
> -    }
> -    dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> -    dinfo->bdrv->read_only = ro;
> -    dinfo->bdrv->detect_zeroes = detect_zeroes;
> +    dinfo->bdrv = bs;
>      QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>  
> -    bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
> -
> -    /* disk I/O throttling */
> -    if (throttle_enabled(&cfg)) {
> -        bdrv_io_limits_enable(dinfo->bdrv);
> -        bdrv_set_io_limits(dinfo->bdrv, &cfg);
> -    }
> -
>      if (!file || !*file) {
>          if (has_driver_specific_opts) {
>              file = NULL;

This looks correct, but it's really doing two things at once (on the one
hand changing code to use bs instead of dinfo->bdrv and reordering, on
the other hand introducing blk_new_with_bs()).

Might be worth doing in separate patches.

> @@ -509,7 +508,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
>      QINCREF(bs_opts);
> -    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, 
> &error);
> +    ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
> +    assert(bs == blk_bs(blk));
>  
>      if (ret < 0) {
>          error_setg(errp, "could not open disk image %s: %s",
> @@ -518,8 +518,9 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>          goto err;
>      }
>  
> -    if (bdrv_key_required(dinfo->bdrv))
> +    if (bdrv_key_required(bs)) {
>          autostart = 0;
> +    }
>  
>      QDECREF(bs_opts);
>      qemu_opts_del(opts);
> @@ -529,7 +530,6 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> *bs_opts,
>  err:
>      bdrv_unref(dinfo->bdrv);
>      QTAILQ_REMOVE(&drives, dinfo, next);
> -bdrv_new_err:
>      g_free(dinfo->id);
>      g_free(dinfo);
>      blk_unref(blk);
> @@ -1746,15 +1746,17 @@ void qmp_block_set_io_throttle(const char *device, 
> int64_t bps, int64_t bps_rd,
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>      const char *id = qdict_get_str(qdict, "id");
> +    BlockBackend *blk;
>      BlockDriverState *bs;
>      AioContext *aio_context;
>      Error *local_err = NULL;
>  
> -    bs = bdrv_find(id);
> -    if (!bs) {
> +    blk = blk_by_name(id);
> +    if (!blk) {
>          error_report("Device '%s' not found", id);
>          return -1;
>      }
> +    bs = blk_bs(blk);
>  
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
> @@ -1777,8 +1779,9 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>       * then we can just get rid of the block driver state right here.
>       */
>      if (bdrv_get_attached_dev(bs)) {
> +        blk_detach_bs(blk);
> +        blk_unref(blk);

We're not doing real refcounting yet, but if we did, and if the refcount
didn't drop to zero here, why would detaching the bs still be correct?

blk_delete() already takes care of detaching after this patch, so I
suppose removing the call here would be right.

>          bdrv_make_anon(bs);
> -        blk_unref(blk_by_name(id));
>          /* Further I/O must not pause the guest */
>          bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
>                            BLOCKDEV_ON_ERROR_REPORT);

Kevin