diff mbox

[9/9] virtio: remove starting/stopping checks

Message ID 1459516794-23629-10-git-send-email-pbonzini@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paolo Bonzini April 1, 2016, 1:19 p.m. UTC 
Reentrancy cannot happen while the BQL is being held.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 12 ++----------
 hw/scsi/virtio-scsi-dataplane.c |  9 +--------
 include/hw/virtio/virtio-scsi.h |  2 --
 3 files changed, 3 insertions(+), 20 deletions(-)

Comments

Christian Borntraeger April 1, 2016, 2:14 p.m. UTC | #1 
On 04/01/2016 03:19 PM, Paolo Bonzini wrote:
> Reentrancy cannot happen while the BQL is being held.
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reverting this patch makes the segfaults go away.





> ---
>  hw/block/dataplane/virtio-blk.c | 12 ++----------
>  hw/scsi/virtio-scsi-dataplane.c |  9 +--------
>  include/hw/virtio/virtio-scsi.h |  2 --
>  3 files changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 74c6d37..cb00cdc 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -27,9 +27,6 @@
>  #include "qom/object_interfaces.h"
> 
>  struct VirtIOBlockDataPlane {
> -    bool starting;
> -    bool stopping;
> -
>      VirtIOBlkConf *conf;
> 
>      VirtIODevice *vdev;
> @@ -203,11 +200,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
>      int r;
> 
> -    if (vblk->dataplane_started || s->starting) {
> +    if (vblk->dataplane_started) {
>          return;
>      }
> 
> -    s->starting = true;
>      s->vq = virtio_get_queue(s->vdev, 0);
> 
>      /* Set up guest notifier (irq) */
> @@ -226,7 +222,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>          goto fail_host_notifier;
>      }
> 
> -    s->starting = false;
>      vblk->dataplane_started = true;
>      trace_virtio_blk_data_plane_start(s);
> 
> @@ -246,7 +241,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      k->set_guest_notifiers(qbus->parent, 1, false);
>    fail_guest_notifiers:
>      vblk->dataplane_disabled = true;
> -    s->starting = false;
>      vblk->dataplane_started = true;
>  }
> 
> @@ -257,7 +251,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> 
> -    if (!vblk->dataplane_started || s->stopping) {
> +    if (!vblk->dataplane_started) {
>          return;
>      }
> 
> @@ -267,7 +261,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>          vblk->dataplane_started = false;
>          return;
>      }
> -    s->stopping = true;
>      trace_virtio_blk_data_plane_stop(s);
> 
>      aio_context_acquire(s->ctx);
> @@ -286,5 +279,4 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>      k->set_guest_notifiers(qbus->parent, 1, false);
> 
>      vblk->dataplane_started = false;
> -    s->stopping = false;
>  }
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 5494dcc..7511447 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -115,14 +115,11 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> 
>      if (s->dataplane_started ||
> -        s->dataplane_starting ||
>          s->dataplane_fenced ||
>          s->ctx != iothread_get_aio_context(vs->conf.iothread)) {
>          return;
>      }
> 
> -    s->dataplane_starting = true;
> -
>      /* Set up guest notifier (irq) */
>      rc = k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, true);
>      if (rc != 0) {
> @@ -150,7 +147,6 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
>          }
>      }
> 
> -    s->dataplane_starting = false;
>      s->dataplane_started = true;
>      aio_context_release(s->ctx);
>      return;
> @@ -164,7 +160,6 @@ fail_vrings:
>      k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
>  fail_guest_notifiers:
>      s->dataplane_fenced = true;
> -    s->dataplane_starting = false;
>      s->dataplane_started = true;
>  }
> 
> @@ -176,7 +171,7 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
>      int i;
> 
> -    if (!s->dataplane_started || s->dataplane_stopping) {
> +    if (!s->dataplane_started) {
>          return;
>      }
> 
> @@ -186,7 +181,6 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
>          s->dataplane_started = false;
>          return;
>      }
> -    s->dataplane_stopping = true;
>      assert(s->ctx == iothread_get_aio_context(vs->conf.iothread));
> 
>      aio_context_acquire(s->ctx);
> @@ -203,6 +197,5 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
> 
>      /* Clean up guest notifier (irq) */
>      k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
> -    s->dataplane_stopping = false;
>      s->dataplane_started = false;
>  }
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index ba2f5ce..d5352d8 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -89,8 +89,6 @@ typedef struct VirtIOSCSI {
>      QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
> 
>      bool dataplane_started;
> -    bool dataplane_starting;
> -    bool dataplane_stopping;
>      bool dataplane_fenced;
>      Error *blocker;
>      uint32_t host_features;
>
Cornelia Huck April 1, 2016, 2:30 p.m. UTC | #2 
On Fri, 1 Apr 2016 16:14:22 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 04/01/2016 03:19 PM, Paolo Bonzini wrote:
> > Reentrancy cannot happen while the BQL is being held.
> > 
> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reverting this patch makes the segfaults go away.
> 
> 
> 
> 
> 
> > ---
> >  hw/block/dataplane/virtio-blk.c | 12 ++----------
> >  hw/scsi/virtio-scsi-dataplane.c |  9 +--------
> >  include/hw/virtio/virtio-scsi.h |  2 --
> >  3 files changed, 3 insertions(+), 20 deletions(-)

:(

On the one hand, I'm wondering what we're missing here.

On the other hand, let's just skip the cleanup patches and get the bug
fixed?
Michael S. Tsirkin April 3, 2016, 10:30 a.m. UTC | #3 
On Fri, Apr 01, 2016 at 04:30:44PM +0200, Cornelia Huck wrote:
> On Fri, 1 Apr 2016 16:14:22 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 04/01/2016 03:19 PM, Paolo Bonzini wrote:
> > > Reentrancy cannot happen while the BQL is being held.
> > > 
> > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Reverting this patch makes the segfaults go away.
> > 
> > 
> > 
> > 
> > 
> > > ---
> > >  hw/block/dataplane/virtio-blk.c | 12 ++----------
> > >  hw/scsi/virtio-scsi-dataplane.c |  9 +--------
> > >  include/hw/virtio/virtio-scsi.h |  2 --
> > >  3 files changed, 3 insertions(+), 20 deletions(-)
> 
> :(
> 
> On the one hand, I'm wondering what we're missing here.
> 
> On the other hand, let's just skip the cleanup patches and get the bug
> fixed?

For 2.6, absolutely. I would also drop 8/9.
For debugging (that we can keep up for now), instead of dropping the
checks, let's replace them with asserts.
diff mbox

Patch

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 74c6d37..cb00cdc 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -27,9 +27,6 @@ 
 #include "qom/object_interfaces.h"
 
 struct VirtIOBlockDataPlane {
-    bool starting;
-    bool stopping;
-
     VirtIOBlkConf *conf;
 
     VirtIODevice *vdev;
@@ -203,11 +200,10 @@  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
     int r;
 
-    if (vblk->dataplane_started || s->starting) {
+    if (vblk->dataplane_started) {
         return;
     }
 
-    s->starting = true;
     s->vq = virtio_get_queue(s->vdev, 0);
 
     /* Set up guest notifier (irq) */
@@ -226,7 +222,6 @@  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
         goto fail_host_notifier;
     }
 
-    s->starting = false;
     vblk->dataplane_started = true;
     trace_virtio_blk_data_plane_start(s);
 
@@ -246,7 +241,6 @@  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     k->set_guest_notifiers(qbus->parent, 1, false);
   fail_guest_notifiers:
     vblk->dataplane_disabled = true;
-    s->starting = false;
     vblk->dataplane_started = true;
 }
 
@@ -257,7 +251,7 @@  void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
 
-    if (!vblk->dataplane_started || s->stopping) {
+    if (!vblk->dataplane_started) {
         return;
     }
 
@@ -267,7 +261,6 @@  void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
         vblk->dataplane_started = false;
         return;
     }
-    s->stopping = true;
     trace_virtio_blk_data_plane_stop(s);
 
     aio_context_acquire(s->ctx);
@@ -286,5 +279,4 @@  void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     k->set_guest_notifiers(qbus->parent, 1, false);
 
     vblk->dataplane_started = false;
-    s->stopping = false;
 }
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 5494dcc..7511447 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -115,14 +115,11 @@  void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
 
     if (s->dataplane_started ||
-        s->dataplane_starting ||
         s->dataplane_fenced ||
         s->ctx != iothread_get_aio_context(vs->conf.iothread)) {
         return;
     }
 
-    s->dataplane_starting = true;
-
     /* Set up guest notifier (irq) */
     rc = k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, true);
     if (rc != 0) {
@@ -150,7 +147,6 @@  void virtio_scsi_dataplane_start(VirtIOSCSI *s)
         }
     }
 
-    s->dataplane_starting = false;
     s->dataplane_started = true;
     aio_context_release(s->ctx);
     return;
@@ -164,7 +160,6 @@  fail_vrings:
     k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
 fail_guest_notifiers:
     s->dataplane_fenced = true;
-    s->dataplane_starting = false;
     s->dataplane_started = true;
 }
 
@@ -176,7 +171,7 @@  void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
-    if (!s->dataplane_started || s->dataplane_stopping) {
+    if (!s->dataplane_started) {
         return;
     }
 
@@ -186,7 +181,6 @@  void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
         s->dataplane_started = false;
         return;
     }
-    s->dataplane_stopping = true;
     assert(s->ctx == iothread_get_aio_context(vs->conf.iothread));
 
     aio_context_acquire(s->ctx);
@@ -203,6 +197,5 @@  void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
 
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
-    s->dataplane_stopping = false;
     s->dataplane_started = false;
 }
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index ba2f5ce..d5352d8 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -89,8 +89,6 @@  typedef struct VirtIOSCSI {
     QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
 
     bool dataplane_started;
-    bool dataplane_starting;
-    bool dataplane_stopping;
     bool dataplane_fenced;
     Error *blocker;
     uint32_t host_features;