Re: [Qemu-devel] [PATCH v3 06/23] block: Make BlockBackend own its BlockDriverState

[Kevin Wolf]

Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
> On BlockBackend destruction, unref its BlockDriverState.  Replaces the
> callers' unrefs.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/block-backend.c |  6 ++----
>  blockdev.c            |  8 ++------
>  hw/block/xen_disk.c   |  6 +++---
>  qemu-img.c            | 35 +----------------------------------
>  qemu-io.c             |  5 -----
>  qemu-nbd.c            |  1 -
>  6 files changed, 8 insertions(+), 53 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index be9acda..ed4873e 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -54,8 +54,6 @@ BlockBackend *blk_new(const char *name, Error **errp)
>  
>  /*
>   * Create a new BlockBackend with a new BlockDriverState attached.
> - * Both have a reference count of one.  Caller owns *both* references.
> - * TODO Let caller own only the BlockBackend reference
>   * Otherwise just like blk_new(), which see.
>   */
>  BlockBackend *blk_new_with_bs(const char *name, Error **errp)
> @@ -83,7 +81,9 @@ static void blk_delete(BlockBackend *blk)
>  {
>      assert(!blk->refcnt);
>      if (blk->bs) {
> +        assert(blk->bs->blk == blk);
>          blk->bs->blk = NULL;
> +        bdrv_unref(blk->bs);
>          blk->bs = NULL;
>      }
>      /* Avoid double-remove after blk_hide_on_behalf_of_do_drive_del() */
> @@ -121,8 +121,6 @@ void blk_ref(BlockBackend *blk)
>   * Decrement @blk's reference count.
>   * If this drops it to zero, destroy @blk.
>   * For convenience, do nothing if @blk is null.
> - * Does *not* touch the attached BlockDriverState's reference count.
> - * TODO Decrement it!
>   */
>  void blk_unref(BlockBackend *blk)
>  {
> diff --git a/blockdev.c b/blockdev.c
> index 1c97faa..3a6fd46 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -108,7 +108,7 @@ void blockdev_auto_del(BlockDriverState *bs)
>      DriveInfo *dinfo = blk_legacy_dinfo(blk);
>  
>      if (dinfo && dinfo->auto_del) {
> -        drive_del(dinfo);
> +        blk_unref(blk);
>      }
>  }
>  
> @@ -219,7 +219,6 @@ static void bdrv_format_print(void *opaque, const char 
> *name)
>  
>  void drive_del(DriveInfo *dinfo)
>  {
> -    bdrv_unref(dinfo->bdrv);
>      blk_unref(blk_by_legacy_dinfo(dinfo));
>  }
>  
> @@ -522,7 +521,6 @@ static BlockBackend *blockdev_init(const char *file, 
> QDict *bs_opts,
>      return blk;
>  
>  err:
> -    bdrv_unref(bs);
>      blk_unref(blk);
>  early_err:
>      qemu_opts_del(opts);
> @@ -1782,9 +1780,8 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
> QObject **ret_data)
>          /* Further I/O must not pause the guest */
>          bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
>                            BLOCKDEV_ON_ERROR_REPORT);
> -        /* FIXME bs->blk leaked when bs dies */

Which hunk of the patch fixes this? I can't see a new blk_unref()
anywhere.

Hm, rereading this before sending the mail out, I think what this
comment was intended for is already fixed in patch 4. I'll leave my
original thoughts here anyway because they are about an independent
problem:

I never completely understood where this anonymous BDS gets freed
eventually. It only occurs to me now that this is probably because it
doesn't? The original commit (d22b2f41) doesn't mention how this was
intended to work.

In some cases (and this may cover all of the original cases) the autodel
behaviour might actually happen to free it when the device is gone. But
if the BDS was created with blockdev-add, it certainly isn't (that would
be a bug introduced by my blockdev-add series). If there is any
non-hotpluggable device that doesn't call blockdev_mark_auto_del() in
its unrealize function, it also isn't freed for that device.

Can we make this whole mechanism more obvious eventually by getting a
user/monitor reference only for blockdev-add, but not for -drive, and
getting a second reference for devices that use the BB?

>      } else {
> -        drive_del(dinfo);
> +        blk_unref(blk);
>      }
>  
>      aio_context_release(aio_context);

Looks like I once again found questions that took me a lot of time to
investigate and perhaps even a bug, but it doesn't look like a bug in
this patch. Feel free to add:

Reviewed-by: Kevin Wolf <address@hidden>

Kevin