diff mbox

[v7,13/24] virtio-scsi: Catch BDS-BB removal/insertion

Message ID 1447108773-6836-14-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Nov. 9, 2015, 10:39 p.m. UTC 
Make use of the BDS-BB removal and insertion notifiers to remove or set
up, respectively, virtio-scsi's op blockers.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 hw/scsi/virtio-scsi.c           | 59 +++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-scsi.h | 10 +++++++
 2 files changed, 69 insertions(+)

Comments

Kevin Wolf Nov. 25, 2015, 4:03 p.m. UTC | #1 
Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
> Make use of the BDS-BB removal and insertion notifiers to remove or set
> up, respectively, virtio-scsi's op blockers.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      if (s->ctx) {
>          blk_op_unblock_all(sd->conf.blk, s->blocker);
>      }
> +
> +    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
> +        if (insert_notifier->sd == sd) {
> +            break;
> +        }
> +    }
> +    if (insert_notifier) {
> +        notifier_remove(&insert_notifier->n);
> +        QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
> +        g_free(insert_notifier);
> +    }

Why a separate if block instead of just doing that inside the loop?

> +    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
> +        if (remove_notifier->sd == sd) {
> +            break;
> +        }
> +    }
> +    if (remove_notifier) {
> +        notifier_remove(&remove_notifier->n);
> +        QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
> +        g_free(remove_notifier);
> +    }
> +
>      qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>  }

Kevin
Max Reitz Nov. 25, 2015, 4:08 p.m. UTC | #2 
On 25.11.2015 17:03, Kevin Wolf wrote:
> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>> Make use of the BDS-BB removal and insertion notifiers to remove or set
>> up, respectively, virtio-scsi's op blockers.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>> @@ -797,6 +830,29 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>      if (s->ctx) {
>>          blk_op_unblock_all(sd->conf.blk, s->blocker);
>>      }
>> +
>> +    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
>> +        if (insert_notifier->sd == sd) {
>> +            break;
>> +        }
>> +    }
>> +    if (insert_notifier) {
>> +        notifier_remove(&insert_notifier->n);
>> +        QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
>> +        g_free(insert_notifier);
>> +    }
> 
> Why a separate if block instead of just doing that inside the loop?

Because I unconsciously try to reduce indentation.

Also, because when I wrote the code, to me it was "Find the relevant
notifier -- destroy that notifier", and that's how this pattern came about.

I personally still like it more the way I did it, but I can very well
imagine that I'm the only one who thinks so. Therefore, I wouldn't mind
changing it (besides the effort involved to change it).

Max

>> +    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
>> +        if (remove_notifier->sd == sd) {
>> +            break;
>> +        }
>> +    }
>> +    if (remove_notifier) {
>> +        notifier_remove(&remove_notifier->n);
>> +        QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
>> +        g_free(remove_notifier);
>> +    }
>> +
>>      qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
>>  }
> 
> Kevin
>
diff mbox

Patch

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 7655401..02606e6 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -757,6 +757,22 @@  static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
     }
 }
 
+static void virtio_scsi_blk_insert_notifier(Notifier *n, void *data)
+{
+    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
+                                                n, n);
+    assert(cn->sd->conf.blk == data);
+    blk_op_block_all(cn->sd->conf.blk, cn->s->blocker);
+}
+
+static void virtio_scsi_blk_remove_notifier(Notifier *n, void *data)
+{
+    VirtIOSCSIBlkChangeNotifier *cn = DO_UPCAST(VirtIOSCSIBlkChangeNotifier,
+                                                n, n);
+    assert(cn->sd->conf.blk == data);
+    blk_op_unblock_all(cn->sd->conf.blk, cn->s->blocker);
+}
+
 static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                 Error **errp)
 {
@@ -765,6 +781,22 @@  static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     SCSIDevice *sd = SCSI_DEVICE(dev);
 
     if (s->ctx && !s->dataplane_disabled) {
+        VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
+
+        insert_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
+        insert_notifier->n.notify = virtio_scsi_blk_insert_notifier;
+        insert_notifier->s = s;
+        insert_notifier->sd = sd;
+        blk_add_insert_bs_notifier(sd->conf.blk, &insert_notifier->n);
+        QTAILQ_INSERT_TAIL(&s->insert_notifiers, insert_notifier, next);
+
+        remove_notifier = g_new0(VirtIOSCSIBlkChangeNotifier, 1);
+        remove_notifier->n.notify = virtio_scsi_blk_remove_notifier;
+        remove_notifier->s = s;
+        remove_notifier->sd = sd;
+        blk_add_remove_bs_notifier(sd->conf.blk, &remove_notifier->n);
+        QTAILQ_INSERT_TAIL(&s->remove_notifiers, remove_notifier, next);
+
         if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
             return;
         }
@@ -787,6 +819,7 @@  static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     VirtIODevice *vdev = VIRTIO_DEVICE(hotplug_dev);
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     SCSIDevice *sd = SCSI_DEVICE(dev);
+    VirtIOSCSIBlkChangeNotifier *insert_notifier, *remove_notifier;
 
     if (virtio_vdev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) {
         virtio_scsi_push_event(s, sd,
@@ -797,6 +830,29 @@  static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     if (s->ctx) {
         blk_op_unblock_all(sd->conf.blk, s->blocker);
     }
+
+    QTAILQ_FOREACH(insert_notifier, &s->insert_notifiers, next) {
+        if (insert_notifier->sd == sd) {
+            break;
+        }
+    }
+    if (insert_notifier) {
+        notifier_remove(&insert_notifier->n);
+        QTAILQ_REMOVE(&s->insert_notifiers, insert_notifier, next);
+        g_free(insert_notifier);
+    }
+
+    QTAILQ_FOREACH(remove_notifier, &s->remove_notifiers, next) {
+        if (remove_notifier->sd == sd) {
+            break;
+        }
+    }
+    if (remove_notifier) {
+        notifier_remove(&remove_notifier->n);
+        QTAILQ_REMOVE(&s->remove_notifiers, remove_notifier, next);
+        g_free(remove_notifier);
+    }
+
     qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
@@ -911,6 +967,9 @@  static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
     add_migration_state_change_notifier(&s->migration_state_notifier);
 
     error_setg(&s->blocker, "block device is in use by data plane");
+
+    QTAILQ_INIT(&s->insert_notifiers);
+    QTAILQ_INIT(&s->remove_notifiers);
 }
 
 static void virtio_scsi_instance_init(Object *obj)
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 088fe9f..0394eb2 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -76,6 +76,13 @@  typedef struct VirtIOSCSICommon {
     VirtQueue **cmd_vqs;
 } VirtIOSCSICommon;
 
+typedef struct VirtIOSCSIBlkChangeNotifier {
+    Notifier n;
+    struct VirtIOSCSI *s;
+    SCSIDevice *sd;
+    QTAILQ_ENTRY(VirtIOSCSIBlkChangeNotifier) next;
+} VirtIOSCSIBlkChangeNotifier;
+
 typedef struct VirtIOSCSI {
     VirtIOSCSICommon parent_obj;
 
@@ -86,6 +93,9 @@  typedef struct VirtIOSCSI {
     /* Fields for dataplane below */
     AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
 
+    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
+    QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
+
     /* Vring is used instead of vq in dataplane code, because of the underlying
      * memory layer thread safety */
     VirtIOSCSIVring *ctrl_vring;