[0/4] Tweaks around virtio-blk start/stop

Commit Message

Fam Zheng March 21, 2016, 10:57 a.m. UTC

On Thu, 03/17 19:03, tu bo wrote:
> 
> On 03/17/2016 08:39 AM, Fam Zheng wrote:
> >On Wed, 03/16 14:45, Paolo Bonzini wrote:
> >>
> >>
> >>On 16/03/2016 14:38, Christian Borntraeger wrote:
> >>>>If you just remove the calls to virtio_queue_host_notifier_read, here
> >>>>and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> >>>>(keeping patches 2-4 in)?
> >>>
> >>>With these changes and patch 2-4 it does no longer locks up.
> >>>I keep it running some hour to check if a crash happens.
> >>>
> >>>Tu Bo, your setup is currently better suited for reproducing. Can you also check?
> >>
> >>Great, I'll prepare a patch to virtio then sketching the solution that
> >>Conny agreed with.
> >>
> >>While Fam and I agreed that patch 1 is not required, I'm not sure if the
> >>mutex is necessary in the end.
> >
> >If we can fix this from the virtio_queue_host_notifier_read side, the mutex/BH
> >are not necessary; but OTOH the mutex does catch such bugs, so maybe it's good
> >to have it. I'm not sure about the BH.
> >
> >And on a hindsight I realize we don't want patches 2-3 too. Actually the
> >begin/end pair won't work as expected because of the blk_set_aio_context.
> >
> >Let's hold on this series.
> >
> >>
> >>So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
> >>and both with/without Fam's patches, it would be great.
> >
> >Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.
> >
> 1. without the virtio_queue_host_notifier_read calls,  without patch 4
> 
> crash happens very often,
> 
> (gdb) bt
> #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> #1  0x000002aa165da37e in coroutine_trampoline (i0=<optimized out>,
> i1=1812051552) at util/coroutine-ucontext.c:79
> #2  0x000003ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6
> 
> 
> 2. without the virtio_queue_host_notifier_read calls, with patch 4
> 
> crash happens very often,
> 
> (gdb) bt
> #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> #1  0x000002aa39dda43e in coroutine_trampoline (i0=<optimized out>,
> i1=-1677715600) at util/coroutine-ucontext.c:79
> #2  0x000003ffab6d150a in __makecontext_ret () from /lib64/libc.so.6
> 
> 

Tu Bo,

Could you help test this patch (on top of master, without patch 4)?

Comments

Cornelia Huck March 21, 2016, 11:15 a.m. UTC | #1

On Mon, 21 Mar 2016 18:57:18 +0800
Fam Zheng <[e-mail]> wrote:

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 08275a9..47f8043 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> 
>  void virtio_queue_notify(VirtIODevice *vdev, int n)
>  {
> -    virtio_queue_notify_vq(&vdev->vq[n]);
> +    VirtQueue *vq = &vdev->vq[n];
> +    EventNotifier *n;
> +    n = virtio_queue_get_host_notifier(vq);
> +    if (n) {

Isn't that always true, even if the notifier has not been setup?

> +        event_notifier_set(n);
> +    } else {
> +        virtio_queue_notify_vq(vq);
> +    }
>  }
> 
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> 
>

Fam Zheng March 21, 2016, 12:45 p.m. UTC | #2

On Mon, 03/21 12:15, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 18:57:18 +0800
> Fam Zheng <[e-mail]> wrote:
> 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 08275a9..47f8043 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > 
> >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> >  {
> > -    virtio_queue_notify_vq(&vdev->vq[n]);
> > +    VirtQueue *vq = &vdev->vq[n];
> > +    EventNotifier *n;
> > +    n = virtio_queue_get_host_notifier(vq);
> > +    if (n) {
> 
> Isn't that always true, even if the notifier has not been setup?

You are right, this doesn't make a correct fix. But we can still do a quick
test with this as the else branch should never be used with ioeventfd=on. Am I
right?

Fam

> 
> > +        event_notifier_set(n);
> > +    } else {
> > +        virtio_queue_notify_vq(vq);
> > +    }
> >  }
> > 
> >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > 
> > 
>

Cornelia Huck March 21, 2016, 1:02 p.m. UTC | #3

On Mon, 21 Mar 2016 20:45:27 +0800
Fam Zheng <[e-mail]> wrote:

> On Mon, 03/21 12:15, Cornelia Huck wrote:
> > On Mon, 21 Mar 2016 18:57:18 +0800
> > Fam Zheng <[e-mail]> wrote:
> > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 08275a9..47f8043 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > 
> > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > >  {
> > > -    virtio_queue_notify_vq(&vdev->vq[n]);
> > > +    VirtQueue *vq = &vdev->vq[n];
> > > +    EventNotifier *n;
> > > +    n = virtio_queue_get_host_notifier(vq);
> > > +    if (n) {
> > 
> > Isn't that always true, even if the notifier has not been setup?
> 
> You are right, this doesn't make a correct fix. But we can still do a quick
> test with this as the else branch should never be used with ioeventfd=on. Am I
> right?
> 
> Fam

Won't we come through here for the very first kick, when we haven't
registered the ioeventfd with the kernel yet?

> 
> > 
> > > +        event_notifier_set(n);
> > > +    } else {
> > > +        virtio_queue_notify_vq(vq);
> > > +    }
> > >  }
> > > 
> > >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > > 
> > > 
> > 
>

Fam Zheng March 21, 2016, 11:45 p.m. UTC | #4

On Mon, 03/21 14:02, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 20:45:27 +0800
> Fam Zheng <[e-mail]> wrote:
> 
> > On Mon, 03/21 12:15, Cornelia Huck wrote:
> > > On Mon, 21 Mar 2016 18:57:18 +0800
> > > Fam Zheng <[e-mail]> wrote:
> > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 08275a9..47f8043 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > > 
> > > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > > >  {
> > > > -    virtio_queue_notify_vq(&vdev->vq[n]);
> > > > +    VirtQueue *vq = &vdev->vq[n];
> > > > +    EventNotifier *n;
> > > > +    n = virtio_queue_get_host_notifier(vq);
> > > > +    if (n) {
> > > 
> > > Isn't that always true, even if the notifier has not been setup?
> > 
> > You are right, this doesn't make a correct fix. But we can still do a quick
> > test with this as the else branch should never be used with ioeventfd=on. Am I
> > right?
> > 
> > Fam
> 
> Won't we come through here for the very first kick, when we haven't
> registered the ioeventfd with the kernel yet?
> 

The ioeventfd in virtio-ccw is registered in the main loop when
VIRTIO_CONFIG_S_DRIVER_OK is set, so I think the first kick is okay.

Fam

Cornelia Huck March 22, 2016, 8:06 a.m. UTC | #5

On Tue, 22 Mar 2016 07:45:19 +0800
Fam Zheng <[e-mail]> wrote:

> On Mon, 03/21 14:02, Cornelia Huck wrote:
> > On Mon, 21 Mar 2016 20:45:27 +0800
> > Fam Zheng <[e-mail]> wrote:
> > 
> > > On Mon, 03/21 12:15, Cornelia Huck wrote:
> > > > On Mon, 21 Mar 2016 18:57:18 +0800
> > > > Fam Zheng <[e-mail]> wrote:
> > > > 
> > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > > index 08275a9..47f8043 100644
> > > > > --- a/hw/virtio/virtio.c
> > > > > +++ b/hw/virtio/virtio.c
> > > > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > > > 
> > > > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > > > >  {
> > > > > -    virtio_queue_notify_vq(&vdev->vq[n]);
> > > > > +    VirtQueue *vq = &vdev->vq[n];
> > > > > +    EventNotifier *n;
> > > > > +    n = virtio_queue_get_host_notifier(vq);
> > > > > +    if (n) {
> > > > 
> > > > Isn't that always true, even if the notifier has not been setup?
> > > 
> > > You are right, this doesn't make a correct fix. But we can still do a quick
> > > test with this as the else branch should never be used with ioeventfd=on. Am I
> > > right?
> > > 
> > > Fam
> > 
> > Won't we come through here for the very first kick, when we haven't
> > registered the ioeventfd with the kernel yet?
> > 
> 
> The ioeventfd in virtio-ccw is registered in the main loop when
> VIRTIO_CONFIG_S_DRIVER_OK is set, so I think the first kick is okay.

You're right, for well-behaved drivers this will be done from
set_status, so for testing that's fine.

Patch

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 08275a9..47f8043 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1098,7 +1098,14 @@  void virtio_queue_notify_vq(VirtQueue *vq)
 
 void virtio_queue_notify(VirtIODevice *vdev, int n)
 {
-    virtio_queue_notify_vq(&vdev->vq[n]);
+    VirtQueue *vq = &vdev->vq[n];
+    EventNotifier *n;
+    n = virtio_queue_get_host_notifier(vq);
+    if (n) {
+        event_notifier_set(n);
+    } else {
+        virtio_queue_notify_vq(vq);
+    }
 }
 
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)