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

[Paolo Bonzini <pbonzini@redhat.com>]

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

release_drive's call to blockdev_auto_del can then be removed completely.
This is of course okay in the case where the device has been unrealized
before and unrealize took care of calling blockdev_del_drive.  However,
it is also okay if the device has failed to be realized.  In that case,
blockdev_mark_auto_del was never called (because it is called during
unrealize) and thus release_drive's blockdev_auto_del call did nothing.
The drive-del-test qtest covers this case.

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 |  8 ++++++--
 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, 24 insertions(+), 24 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1f73478..2dfb2d8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -114,20 +114,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);
@@ -139,14 +135,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 c427698..0582787 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -945,7 +945,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 7bd5bde..39a72e4 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -1041,6 +1041,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 e10cede..469ba8a 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -101,9 +101,13 @@ static void release_drive(Object *obj, const char *name, void *opaque)
     Property *prop = opaque;
     BlockBackend **ptr = qdev_get_prop_ptr(dev, prop);
 
-    if (*ptr) {
+    if (*ptr && blk_get_attached_dev(*ptr) != NULL) {
+        /* Unrealize has already called blk_detach_dev and blockdev_del_drive
+         * if the device has been realized; in that case, blk_get_attached_dev
+         * returns NULL.  Thus, we get here if the device failed to realize,
+         * and the -drive must not be released.
+         */
         blk_detach_dev(*ptr, dev);
-        blockdev_auto_del(*ptr);
     }
 }
 
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index df46147..cf8fa58 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -182,6 +182,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 6dcdbc0..3b2b766 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -214,7 +214,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 5ae0424..1c00211 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -643,7 +643,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