Re: [Qemu-devel] [PATCH 3/3] block: remove legacy_dinfo at blk_detach_dev time

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Currently, blockdev_del_drive (and before it blk_auto_del) does a blk_unref
> that will cause blk_delete to be called and the DriveInfo to be freed.
> But really, we want to free the drive info as soon as the device is
> detached, even if there are other references for whatever reason, so
> that the QemuOpts are freed as well and the id can be reused.

Let me write up how I understand this, so you can correct my
misunderstandings.

Management applications expect that on receipt of event DEVICE_DELETED

* the frontend is fully gone, and

* any of its warty block backends are fully gone.

A frontend's block backend is warty if it gets automatically deleted
along with the frontend.

"Fully gone" implies the ID can safely be reused.

The whole series is about warty automatic block backend deletion.

Non-warty block backends need to be deleted explicitly with
x-blockdev-del, which fails when there are other references.

We can't do the same for warty deletion, because the *frontend* got
deleted just fine (thus DEVICE_DELETED must be sent), even when
something else is keeping the backend alive.  Back when the wart was
born, this wasn't possible.

So, what to do?  The commit message sounds like the patch hides the
backend then, so it's "fully gone" from the management application's
view.

But isn't that an impossible mission if the "something else" is
something the management application can see?  For instance, what if
there's a block job tied to the backend?  The management application can
see that tie.  If we succeed in hiding the backend, we have a dangling
tie.  If we keep the tie, we failed at hiding.

We avoid the case of a block job the hamfisted way: we cancel it.  Okay,
but that begs the question what else could hold a reference, whether
it's similarly exposed to the management appliction, and whether it
needs to be "cancelled" as well.

I'm sure Shrek would love this swamp.

> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/block-backend.c     | 13 +++++++++----
>  blockdev.c                |  9 +++++----
>  include/sysemu/blockdev.h |  1 +
>  3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index ebdf78a..0a85c6a 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -65,8 +65,6 @@ static const AIOCBInfo block_backend_aiocb_info = {
>      .aiocb_size = sizeof(BlockBackendAIOCB),
>  };
>  
> -static void drive_info_del(DriveInfo *dinfo);
> -
>  /* All the BlockBackends (except for hidden ones) */
>  static QTAILQ_HEAD(, BlockBackend) blk_backends =
>      QTAILQ_HEAD_INITIALIZER(blk_backends);
> @@ -165,6 +163,7 @@ static void blk_delete(BlockBackend *blk)
>  {
>      assert(!blk->refcnt);
>      assert(!blk->dev);
> +    assert(!blk->legacy_dinfo);
>      if (blk->bs) {
>          blk_remove_bs(blk);
>      }
> @@ -179,19 +178,25 @@ static void blk_delete(BlockBackend *blk)
>          QTAILQ_REMOVE(&blk_backends, blk, link);
>      }
>      g_free(blk->name);
> -    drive_info_del(blk->legacy_dinfo);
>      block_acct_cleanup(&blk->stats);
>      g_free(blk);
>  }

This and the assertion above effectively say "you must delete a
BlockBackend's legacy_dinfo before you drop its last reference."
We'll see below why that works.

>  
> -static void drive_info_del(DriveInfo *dinfo)
> +void blk_release_legacy_dinfo(BlockBackend *blk)
>  {
> +    DriveInfo *dinfo = blk->legacy_dinfo;
> +
>      if (!dinfo) {
>          return;
>      }
>      qemu_opts_del(dinfo->opts);
>      g_free(dinfo->serial);
>      g_free(dinfo);
> +    blk->legacy_dinfo = NULL;
> +    /* We are not interested anymore in retrieving the BlockBackend
> +     * via blk_by_legacy_dinfo, so let it die.
> +     */
> +    blk_unref(blk);
>  }
>  

This looks like DriveInfo now owns a reference to BlockBackend, even
though the pointer still goes in the other direction.

>  int blk_get_refcnt(BlockBackend *blk)
> diff --git a/blockdev.c b/blockdev.c
> index 2dfb2d8..85f0cb5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -120,10 +120,10 @@ void override_max_devs(BlockInterfaceType type, int 
> max_devs)
>   */
>  void blockdev_del_drive(BlockBackend *blk)
>  {
> -    DriveInfo *dinfo = blk_legacy_dinfo(blk);
>      BlockDriverState *bs = blk_bs(blk);
>      AioContext *aio_context;
>  
> +    blk_ref(blk);
>      if (bs) {
>          aio_context = bdrv_get_aio_context(bs);
>          aio_context_acquire(aio_context);
> @@ -135,9 +135,10 @@ void blockdev_del_drive(BlockBackend *blk)
>          aio_context_release(aio_context);
>      }
>  
> -    if (dinfo) {
> -        blk_unref(blk);
> +    if (blk_legacy_dinfo(blk)) {
> +        blk_release_legacy_dinfo(blk);
>      }
> +    blk_unref(blk);
>  }

Before: we drop a reference here if we have a DriveInfo.

After: we drop a reference in blk_release_legacy_dinfo() if we have a
DriveInfo.  We also take a temporary reference here.  I guess the only
reason is to avoid tripping the assertion we just discussed.

>  
>  /**
> @@ -2811,7 +2812,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
       /* if we have a device attached to this BlockDriverState
        * then we need to make the drive anonymous until the device
        * can be removed.  If this is a drive with no device backing
        * then we can just get rid of the block driver state right here.
        */
       if (blk_get_attached_dev(blk)) {
           blk_hide_on_behalf_of_hmp_drive_del(blk);
           /* Further I/O must not pause the guest */
>          blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
>                           BLOCKDEV_ON_ERROR_REPORT);
>      } else {
> -        blk_unref(blk);
> +        blockdev_del_drive(blk);

Now we come to the spot I've dreaded all along...

If blk_get_attached_dev(), we can't delete the backend, so we must hide
it.  Not changed by your patch.

Else, we delete the backend.

Before your patch, we delete the obvious way: drop the reference.  If
something else holds another reference, we fail to delete.  If this can
happen, it's a bug.  I'm not sure it can't happen.

After your patch, we can't just drop the reference, because that would
trip the assertion we just discussed.  So we call blockdev_del_drive()
instead.  In addition to dropping the reference, this

* Cancels block jobs.  Could well be a fix for the bug I just described,
  but it needs to be featured in the commit message then.

* blk_release_legacy_dinfo().  Same addition as in blockdev_del_drive()
  above.

Now back to why "you must delete a BlockBackend's legacy_dinfo before
you drop its last reference" works.  A BlockBackend has a legacy_dinfo
exactly when it was created with -drive or drive_add.  The block layer
holds a reference for lookup by name then.  This reference goes away
only when the backend is deleted by the user, in one of the following
ways:

* Explicitly with drive_del

  Deletes right away if no frontend is attached.  Your patch has the
  necessary replacement of blk_unref(blk) by blockdev_del_drive(blk), in
  hmp_drive_del().

  Else, deletion is delayed until the frontend detaches.  Where is the
  legacy_dinfo released then?

* Explicitly with x-blockdev-del

  Fails unless no other reference exists.  Where is the legacy_dinfo
  released?

* Implicitly via warty automatic deletion

  Your PATCH 01 has the necessary replacement of blk_unref(blk) by
  blkdev_del_drive(blk) for some devices (virtio-blk.c, scsi-bus.c,
  xen_disk,c, piix.c), butas far as I can see not for others such as
  nvme.c.

>      }
>  
>      aio_context_release(aio_context);
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index ae7ad67..5722b9f 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -44,6 +44,7 @@ struct DriveInfo {
>  DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
>  DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo);
>  BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
> +void blk_release_legacy_dinfo(BlockBackend *blk);
>  
>  void override_max_devs(BlockInterfaceType type, int max_devs);