[Qemu-devel] [RFC PATCH 0/2] Early release of -drive QemuOpts

[Qemu-devel] [RFC PATCH 0/2] Early release of -drive QemuOpts

[Paolo Bonzini]

In short, this patch gets rid of blockdev_mark_auto_del and
blockdev_auto_del.

With these patches, it is possible to create a new -drive with the same
id as soon as the DEVICE_DELETED event is delivered (which equals to
unrealize).

I'm sorry I'm not able to explain the history (and probably do not
understand the full ramifications) of this.  That's why this is just
an RFC.

The idea here is that reference counting the BlockBackend is enough to
defer the deletion of the block device as much as necessary; anticipating
the demise of the DriveInfo is not a problem, and has the desired effect
of freeing the QemuOpts.

Paolo

Paolo Bonzini (2):
  block: detach devices from DriveInfo at unrealize time
  block: remove legacy_dinfo at blk_detach_dev time

 block/block-backend.c            | 14 ++++++++----
 blockdev.c                       | 26 ++++++++------------------
 hw/block/virtio-blk.c            |  4 +++-
 hw/block/xen_disk.c              |  1 +
 hw/core/qdev-properties-system.c |  2 +-
 hw/ide/piix.c                    |  3 +++
 hw/scsi/scsi-bus.c               |  4 +++-
 hw/usb/dev-storage.c             |  3 ++-
 include/sysemu/blockdev.h        |  5 ++---
 9 files changed, 33 insertions(+), 29 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/2] block: detach devices from DriveInfo at unrealize time

[Paolo Bonzini]

Instead of delaying blk_detach_dev and blockdev_auto_del until
the object is finalized and properties are released, do that
as soon as possible.

This patch replaces blockdev_mark_auto_del calls with blk_detach_dev
and blockdev_del_drive (the latter is a combination of the former
blockdev_mark_auto_del and blockdev_auto_del).  We cannot make
blk_detach_dev do both tasks because of the USB mass storage hack.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c                       | 21 +++++----------------
 hw/block/virtio-blk.c            |  4 +++-
 hw/block/xen_disk.c              |  1 +
 hw/core/qdev-properties-system.c |  2 +-
 hw/ide/piix.c                    |  3 +++
 hw/scsi/scsi-bus.c               |  4 +++-
 hw/usb/dev-storage.c             |  3 ++-
 include/sysemu/blockdev.h        |  4 +---
 8 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 917ae06..a8309fb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -110,20 +110,16 @@ void override_max_devs(BlockInterfaceType type, int max_devs)
 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.
- * Device models call blockdev_mark_auto_del() to schedule the
- * automatic deletion, and generic qdev code calls blockdev_auto_del()
- * when deletion is actually safe.
+ * Device models call blockdev_del_drive() to schedule the
+ * automatic deletion, and generic block layer code uses the
+ * refcount to do the deletion when it is actually safe.
  */
-void blockdev_mark_auto_del(BlockBackend *blk)
+void blockdev_del_drive(BlockBackend *blk)
 {
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
     BlockDriverState *bs = blk_bs(blk);
     AioContext *aio_context;
 
-    if (!dinfo) {
-        return;
-    }
-
     if (bs) {
         aio_context = bdrv_get_aio_context(bs);
         aio_context_acquire(aio_context);
@@ -135,14 +131,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
         aio_context_release(aio_context);
     }
 
-    dinfo->auto_del = 1;
-}
-
-void blockdev_auto_del(BlockBackend *blk)
-{
-    DriveInfo *dinfo = blk_legacy_dinfo(blk);
-
-    if (dinfo && dinfo->auto_del) {
+    if (dinfo) {
         blk_unref(blk);
     }
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 848f3fe..6e51246 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -960,7 +960,9 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
     s->dataplane = NULL;
     qemu_del_vm_change_state_handler(s->change);
     unregister_savevm(dev, "virtio-blk", s);
-    blockdev_mark_auto_del(s->blk);
+    blk_detach_dev(s->blk, dev);
+    blockdev_del_drive(s->blk);
+    s->blk = NULL;
     virtio_cleanup(vdev);
 }
 
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 02eda6e..a6a0b7c 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -1054,6 +1054,7 @@ static void blk_disconnect(struct XenDevice *xendev)
 
     if (blkdev->blk) {
         blk_detach_dev(blkdev->blk, blkdev);
+        blockdev_del_drive(blkdev->blk);
         blk_unref(blkdev->blk);
         blkdev->blk = NULL;
     }
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 921e799..cf147f4 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -102,7 +102,7 @@ static void release_drive(Object *obj, const char *name, void *opaque)
 
     if (*ptr) {
         blk_detach_dev(*ptr, dev);
-        blockdev_auto_del(*ptr);
+        blockdev_del_drive(*ptr);
     }
 }
 
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 5a26c86..2b2d043 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -181,6 +181,9 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev)
             if (ds) {
                 blk_detach_dev(blk, ds);
             }
+            if (pci_ide->bus[di->bus].ifs[di->unit].blk) {
+                blockdev_del_drive(blk);
+            }
             pci_ide->bus[di->bus].ifs[di->unit].blk = NULL;
             if (!(i % 2)) {
                 idedev = pci_ide->bus[di->bus].master;
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index fd1171e..a805fad 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -213,7 +213,9 @@ static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
     }
 
     scsi_device_purge_requests(dev, SENSE_CODE(NO_SENSE));
-    blockdev_mark_auto_del(dev->conf.blk);
+    blk_detach_dev(dev->conf.blk, qdev);
+    blockdev_del_drive(dev->conf.blk);
+    dev->conf.blk = NULL;
 }
 
 /* handle legacy '-drive if=scsi,...' cmd line args */
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 597d8fd..f191011 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -642,7 +642,8 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
      * blockdev, or else scsi_bus_legacy_add_drive() dies when it
      * attaches again.
      *
-     * The hack is probably a bad idea.
+     * The hack is probably a bad idea.  Anyway, this is why this does not
+     * call blockdev_del_drive.
      */
     blk_detach_dev(blk, &s->dev.qdev);
     s->conf.blk = NULL;
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index b06a060..ae7ad67 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -14,8 +14,7 @@
 #include "qapi/error.h"
 #include "qemu/queue.h"
 
-void blockdev_mark_auto_del(BlockBackend *blk);
-void blockdev_auto_del(BlockBackend *blk);
+void blockdev_del_drive(BlockBackend *blk);
 
 typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
@@ -34,7 +33,6 @@ struct DriveInfo {
     BlockInterfaceType type;
     int bus;
     int unit;
-    int auto_del;               /* see blockdev_mark_auto_del() */
     bool is_default;            /* Added by default_drive() ?  */
     int media_cd;
     int cyls, heads, secs, trans;
-- 
2.5.0

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

[Paolo Bonzini]

Currently, blockdev_del_drive does a blk_unref (and before it
blockdev_auto_del did the same) 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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/block-backend.c     | 14 ++++++++++----
 blockdev.c                |  7 ++++---
 include/sysemu/blockdev.h |  1 +
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 9889e81..a1db52b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -62,8 +62,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);
@@ -160,6 +158,7 @@ static void blk_delete(BlockBackend *blk)
 {
     assert(!blk->refcnt);
     assert(!blk->dev);
+    assert(!blk->legacy_dinfo);
     if (blk->bs) {
         assert(blk->bs->blk == blk);
         blk->bs->blk = NULL;
@@ -175,19 +174,26 @@ 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);
 }
 
-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);
 }
 
 int blk_get_refcnt(BlockBackend *blk)
diff --git a/blockdev.c b/blockdev.c
index a8309fb..d75be63 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -116,10 +116,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);
@@ -131,9 +131,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);
 }
 
 /**
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);
 
-- 
2.5.0

Re: [Qemu-devel] [RFC PATCH 0/2] Early release of -drive QemuOpts

[Paolo Bonzini]

On 08/01/2016 18:37, Paolo Bonzini wrote:
> In short, this patch gets rid of blockdev_mark_auto_del and
> blockdev_auto_del.
> 
> With these patches, it is possible to create a new -drive with the same
> id as soon as the DEVICE_DELETED event is delivered (which equals to
> unrealize).
> 
> I'm sorry I'm not able to explain the history (and probably do not
> understand the full ramifications) of this.  That's why this is just
> an RFC.
> 
> The idea here is that reference counting the BlockBackend is enough to
> defer the deletion of the block device as much as necessary; anticipating
> the demise of the DriveInfo is not a problem, and has the desired effect
> of freeing the QemuOpts.
> 
> Paolo
> 
> Paolo Bonzini (2):
>   block: detach devices from DriveInfo at unrealize time
>   block: remove legacy_dinfo at blk_detach_dev time
> 
>  block/block-backend.c            | 14 ++++++++----
>  blockdev.c                       | 26 ++++++++------------------
>  hw/block/virtio-blk.c            |  4 +++-
>  hw/block/xen_disk.c              |  1 +
>  hw/core/qdev-properties-system.c |  2 +-
>  hw/ide/piix.c                    |  3 +++
>  hw/scsi/scsi-bus.c               |  4 +++-
>  hw/usb/dev-storage.c             |  3 ++-
>  include/sysemu/blockdev.h        |  5 ++---
>  9 files changed, 33 insertions(+), 29 deletions(-)
> 

Ping?  Any comments or other kinds of review? :)

Paolo

Re: [Qemu-devel] [RFC PATCH 0/2] Early release of -drive QemuOpts

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On 22.01.2016 15:01, Paolo Bonzini wrote:
> 
> 
> On 08/01/2016 18:37, Paolo Bonzini wrote:
>> In short, this patch gets rid of blockdev_mark_auto_del and
>> blockdev_auto_del.
>>
>> With these patches, it is possible to create a new -drive with the same
>> id as soon as the DEVICE_DELETED event is delivered (which equals to
>> unrealize).
>>
>> I'm sorry I'm not able to explain the history (and probably do not
>> understand the full ramifications) of this.  That's why this is just
>> an RFC.
>>
>> The idea here is that reference counting the BlockBackend is enough to
>> defer the deletion of the block device as much as necessary; anticipating
>> the demise of the DriveInfo is not a problem, and has the desired effect
>> of freeing the QemuOpts.
>>
>> Paolo
>>
>> Paolo Bonzini (2):
>>   block: detach devices from DriveInfo at unrealize time
>>   block: remove legacy_dinfo at blk_detach_dev time
>>
>>  block/block-backend.c            | 14 ++++++++----
>>  blockdev.c                       | 26 ++++++++------------------
>>  hw/block/virtio-blk.c            |  4 +++-
>>  hw/block/xen_disk.c              |  1 +
>>  hw/core/qdev-properties-system.c |  2 +-
>>  hw/ide/piix.c                    |  3 +++
>>  hw/scsi/scsi-bus.c               |  4 +++-
>>  hw/usb/dev-storage.c             |  3 ++-
>>  include/sysemu/blockdev.h        |  5 ++---
>>  9 files changed, 33 insertions(+), 29 deletions(-)
>>
> 
> Ping?  Any comments or other kinds of review? :)

I skimmed it last week and I remember that I found the idea sound and
didn't have any objections; but that I didn't feel confident for a R-b
or explicit comment, because I don't think I understand the full
ramifications of it either. ;-)

Max


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

Re: [Qemu-devel] [RFC PATCH 0/2] Early release of -drive QemuOpts

[Paolo Bonzini]

On 22/01/2016 23:38, Max Reitz wrote:
> On 22.01.2016 15:01, Paolo Bonzini wrote:
>>
>>
>> On 08/01/2016 18:37, Paolo Bonzini wrote:
>>> In short, this patch gets rid of blockdev_mark_auto_del and
>>> blockdev_auto_del.
>>>
>>> With these patches, it is possible to create a new -drive with the same
>>> id as soon as the DEVICE_DELETED event is delivered (which equals to
>>> unrealize).
>>>
>>> I'm sorry I'm not able to explain the history (and probably do not
>>> understand the full ramifications) of this.  That's why this is just
>>> an RFC.
>>>
>>> The idea here is that reference counting the BlockBackend is enough to
>>> defer the deletion of the block device as much as necessary; anticipating
>>> the demise of the DriveInfo is not a problem, and has the desired effect
>>> of freeing the QemuOpts.
>>>
>>> Paolo
>>>
>>> Paolo Bonzini (2):
>>>   block: detach devices from DriveInfo at unrealize time
>>>   block: remove legacy_dinfo at blk_detach_dev time
>>>
>>>  block/block-backend.c            | 14 ++++++++----
>>>  blockdev.c                       | 26 ++++++++------------------
>>>  hw/block/virtio-blk.c            |  4 +++-
>>>  hw/block/xen_disk.c              |  1 +
>>>  hw/core/qdev-properties-system.c |  2 +-
>>>  hw/ide/piix.c                    |  3 +++
>>>  hw/scsi/scsi-bus.c               |  4 +++-
>>>  hw/usb/dev-storage.c             |  3 ++-
>>>  include/sysemu/blockdev.h        |  5 ++---
>>>  9 files changed, 33 insertions(+), 29 deletions(-)
>>>
>>
>> Ping?  Any comments or other kinds of review? :)
> 
> I skimmed it last week and I remember that I found the idea sound and
> didn't have any objections; but that I didn't feel confident for a R-b
> or explicit comment, because I don't think I understand the full
> ramifications of it either. ;-)

It has some failures after the latest block pull request.  I'll see
what's going on.

Paolo