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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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.

Yes, here:

@@ -218,6 +220,7 @@ 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));
 }
 
I'll amend PATCH 04 to drop the FIXME.

>                                                       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.

We discussed how this works in the context of your review of [PATCH v1
02].  Quoting myself:

Commit 9063f81 makes drive_del immediately destroy the root BDS.  Nice:
the device name becomes available for reuse immediately.  Not so nice:
the device model's pointer to the root BDS dangles, and we're prone to
crash when the memory gets reused.

Commit d22b2f4 fixed that by hiding the root BDS instead of destroying
it.  Destruction only happens on unplug.  "Hiding" means removing it
from bdrv_states and graph_bdrv_states; see bdrv_make_anon().

> 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.

Yes.

>                                                                     But
> if the BDS was created with blockdev-add, it certainly isn't (that would
> be a bug introduced by my blockdev-add series).

"Fixed" by my commit 48f364d "blockdev: Refuse to drive_del something
added with blockdev-add" :)

The plan is to have separate commands to revoke guest access to a
backend, and to delete an otherwise unused backend.  Then we deprecate
drive_del.

>                                                 If there is any
> non-hotpluggable device that doesn't call blockdev_mark_auto_del() in

You mean hot-pluggable.  Same as unpluggable.

> its unrealize function, it also isn't freed for that device.

Yes.

Commit 14bafc5 added blockdev_mark_auto_del(), and made all unpluggable
block device models call it: scsi-disk, scsi-generic, virtio-blk-pci.
It has since moved from virtio-blk-pci to virtio-blk-device (where it
also covers the other virtio-blk-FOO), and spread to related device
models scsi-block, scsi-cd, scsi-hd.

I searched for additional unpluggable block device models, and found
just one: nvme.  No blockdev_mark_auto_del(), thus doesn't auto-delete
its backend on unplug.

Could be delared a feature.  Mind, the whole auto-delete crap is just
for backward compatibility, which doesn't apply to new devices.  But
perhaps wewant to have new devices auto-delete anyway, for consistency's
sake.

If we do, then the need to call blockdev_mark_auto_del() is a trap.
Doing without isn't easy, because the backend is to be destroyed only
after unplug, *not* when device_add fails.  Implemented like this:
release_drive() before blockdev_mark_auto_del() must be device_add
failure (no delete), afterwards it must be unplug (delete unless created
by blockdev-add).

> 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?

Doing this with reference counts would certainly be much nicer.  One
reason for PATCH 23.

>>      } 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>

Thanks!

Take heart, the hairiest part of this series (in my reckoning) ends
here.