[05/24] vhost: change some assert() for error_report() or silent fail

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Calling a vhost operation may fail, especially with disconnectable
backends. Treat some that look harmless as recoverable errors (print
error, or ignore on error code path).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

On Tue, Jun 21, 2016 at 12:02:33PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Calling a vhost operation may fail, especially with disconnectable
> backends. Treat some that look harmless as recoverable errors (print
> error, or ignore on error code path).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

If it's ok and we can recover, then why should we print errors?

> ---
>  hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 81cc5b0..e2d1614 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -398,7 +398,10 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>      /* inform backend of log switching, this must be done before
>         releasing the current log, to ensure no logging is lost */
>      r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> -    assert(r >= 0);
> +    if (r < 0) {
> +        error_report("Failed to change backend log");
> +    }
> +
>      vhost_log_put(dev, true);
>      dev->log = log;
>      dev->log_size = size;
> @@ -565,7 +568,9 @@ static void vhost_commit(MemoryListener *listener)
>  
>      if (!dev->log_enabled) {
>          r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> -        assert(r >= 0);
> +        if (r < 0) {
> +            error_report("Failed to set mem table");
> +        }
>          dev->memory_changed = false;
>          return;
>      }
> @@ -578,7 +583,9 @@ static void vhost_commit(MemoryListener *listener)
>          vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
>      }
>      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> -    assert(r >= 0);
> +    if (r < 0) {
> +        error_report("Failed to set mem table");
> +    }
>      /* To log less, can only decrease log size after table update. */
>      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
>          vhost_dev_log_resize(dev, log_size);
> @@ -647,6 +654,7 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
>      };
>      int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
>      if (r < 0) {
> +        error_report("Failed to set vring addr");
>          return -errno;
>      }
>      return 0;
> @@ -660,12 +668,15 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
>          features |= 0x1ULL << VHOST_F_LOG_ALL;
>      }
>      r = dev->vhost_ops->vhost_set_features(dev, features);
> +    if (r < 0) {
> +        error_report("Failed to set features");
> +    }
>      return r < 0 ? -errno : 0;
>  }
>  
>  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>  {
> -    int r, t, i, idx;
> +    int r, i, idx;
>      r = vhost_dev_set_features(dev, enable_log);
>      if (r < 0) {
>          goto err_features;
> @@ -682,12 +693,10 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
>  err_vq:
>      for (; i >= 0; --i) {
>          idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> -                                     dev->log_enabled);
> -        assert(t >= 0);
> +        vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> +                                 dev->log_enabled);
>      }
> -    t = vhost_dev_set_features(dev, dev->log_enabled);
> -    assert(t >= 0);
> +    vhost_dev_set_features(dev, dev->log_enabled);
>  err_features:
>      return r;
>  }
> @@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
>          }
>      }
>  
> -    assert (r >= 0);
>      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
>                                0, virtio_queue_get_ring_size(vdev, idx));
>      cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev, idx),
> @@ -1187,7 +1195,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
>  
>      file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
>      r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
> -    assert(r >= 0);
> +    if (r < 0) {
> +        error_report("Failed to set vring call");
> +    }
>  }
>  
>  uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> -- 
> 2.7.4

Hi

----- Original Message -----
> On Tue, Jun 21, 2016 at 12:02:33PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Calling a vhost operation may fail, especially with disconnectable
> > backends. Treat some that look harmless as recoverable errors (print
> > error, or ignore on error code path).
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> If it's ok and we can recover, then why should we print errors?

To me, the current disconnect handling is not handled cleanly. There is not much/nothing in the protocol that tells when and how you can disconnect. If qemu vhost-user reconnection works today, it is mostly by luck. Imho, we should print errors if any call to vhost user fails to help further analysis of broken behaviour. And we shoud have a clean mechanism to handle shutdown/disconnect/reconnect.

> 
> > ---
> >  hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
> >  1 file changed, 21 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 81cc5b0..e2d1614 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -398,7 +398,10 @@ static inline void vhost_dev_log_resize(struct
> > vhost_dev *dev, uint64_t size)
> >      /* inform backend of log switching, this must be done before
> >         releasing the current log, to ensure no logging is lost */
> >      r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > -    assert(r >= 0);
> > +    if (r < 0) {
> > +        error_report("Failed to change backend log");
> > +    }
> > +
> >      vhost_log_put(dev, true);
> >      dev->log = log;
> >      dev->log_size = size;
> > @@ -565,7 +568,9 @@ static void vhost_commit(MemoryListener *listener)
> >  
> >      if (!dev->log_enabled) {
> >          r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > -        assert(r >= 0);
> > +        if (r < 0) {
> > +            error_report("Failed to set mem table");
> > +        }
> >          dev->memory_changed = false;
> >          return;
> >      }
> > @@ -578,7 +583,9 @@ static void vhost_commit(MemoryListener *listener)
> >          vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> >      }
> >      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > -    assert(r >= 0);
> > +    if (r < 0) {
> > +        error_report("Failed to set mem table");
> > +    }
> >      /* To log less, can only decrease log size after table update. */
> >      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> >          vhost_dev_log_resize(dev, log_size);
> > @@ -647,6 +654,7 @@ static int vhost_virtqueue_set_addr(struct vhost_dev
> > *dev,
> >      };
> >      int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
> >      if (r < 0) {
> > +        error_report("Failed to set vring addr");
> >          return -errno;
> >      }
> >      return 0;
> > @@ -660,12 +668,15 @@ static int vhost_dev_set_features(struct vhost_dev
> > *dev, bool enable_log)
> >          features |= 0x1ULL << VHOST_F_LOG_ALL;
> >      }
> >      r = dev->vhost_ops->vhost_set_features(dev, features);
> > +    if (r < 0) {
> > +        error_report("Failed to set features");
> > +    }
> >      return r < 0 ? -errno : 0;
> >  }
> >  
> >  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >  {
> > -    int r, t, i, idx;
> > +    int r, i, idx;
> >      r = vhost_dev_set_features(dev, enable_log);
> >      if (r < 0) {
> >          goto err_features;
> > @@ -682,12 +693,10 @@ static int vhost_dev_set_log(struct vhost_dev *dev,
> > bool enable_log)
> >  err_vq:
> >      for (; i >= 0; --i) {
> >          idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> > -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > -                                     dev->log_enabled);
> > -        assert(t >= 0);
> > +        vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > +                                 dev->log_enabled);
> >      }
> > -    t = vhost_dev_set_features(dev, dev->log_enabled);
> > -    assert(t >= 0);
> > +    vhost_dev_set_features(dev, dev->log_enabled);
> >  err_features:
> >      return r;
> >  }
> > @@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
> >          }
> >      }
> >  
> > -    assert (r >= 0);
> >      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev,
> >      idx),
> >                                0, virtio_queue_get_ring_size(vdev, idx));
> >      cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev,
> >      idx),
> > @@ -1187,7 +1195,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> > VirtIODevice *vdev, int n,
> >  
> >      file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
> >      r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
> > -    assert(r >= 0);
> > +    if (r < 0) {
> > +        error_report("Failed to set vring call");
> > +    }
> >  }
> >  
> >  uint64_t vhost_get_features(struct vhost_dev *hdev, const int
> >  *feature_bits,
> > --
> > 2.7.4
>

On Thu, Jun 23, 2016 at 05:19:55AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Tue, Jun 21, 2016 at 12:02:33PM +0200, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > Calling a vhost operation may fail, especially with disconnectable
> > > backends. Treat some that look harmless as recoverable errors (print
> > > error, or ignore on error code path).
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > If it's ok and we can recover, then why should we print errors?
> 
> To me, the current disconnect handling is not handled cleanly. There
> is not much/nothing in the protocol that tells when and how you can
> disconnect. If qemu vhost-user reconnection works today, it is mostly
> by luck. Imho, we should print errors if any call to vhost user fails
> to help further analysis of broken behaviour.

But we decided disconnect is OK, did we not? So now a failure like this
is just expected. It's not broken behaviour anymore ...

> And we shoud have a
> clean mechanism to handle shutdown/disconnect/reconnect.


At some level this is something that's missing here.

How about we patch docs/specs/vhost-user.txt
and describe how is reconnection supposed to work?

All we have is:
	If Master is unable to send the full message or receives a wrong reply
	it will close the connection. An optional reconnection mechanism can be
	implemented.


> > 
> > > ---
> > >  hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
> > >  1 file changed, 21 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 81cc5b0..e2d1614 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -398,7 +398,10 @@ static inline void vhost_dev_log_resize(struct
> > > vhost_dev *dev, uint64_t size)
> > >      /* inform backend of log switching, this must be done before
> > >         releasing the current log, to ensure no logging is lost */
> > >      r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > > -    assert(r >= 0);
> > > +    if (r < 0) {
> > > +        error_report("Failed to change backend log");
> > > +    }
> > > +
> > >      vhost_log_put(dev, true);
> > >      dev->log = log;
> > >      dev->log_size = size;
> > > @@ -565,7 +568,9 @@ static void vhost_commit(MemoryListener *listener)
> > >  
> > >      if (!dev->log_enabled) {
> > >          r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > > -        assert(r >= 0);
> > > +        if (r < 0) {
> > > +            error_report("Failed to set mem table");
> > > +        }
> > >          dev->memory_changed = false;
> > >          return;
> > >      }
> > > @@ -578,7 +583,9 @@ static void vhost_commit(MemoryListener *listener)
> > >          vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> > >      }
> > >      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > > -    assert(r >= 0);
> > > +    if (r < 0) {
> > > +        error_report("Failed to set mem table");
> > > +    }
> > >      /* To log less, can only decrease log size after table update. */
> > >      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> > >          vhost_dev_log_resize(dev, log_size);
> > > @@ -647,6 +654,7 @@ static int vhost_virtqueue_set_addr(struct vhost_dev
> > > *dev,
> > >      };
> > >      int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
> > >      if (r < 0) {
> > > +        error_report("Failed to set vring addr");
> > >          return -errno;
> > >      }
> > >      return 0;
> > > @@ -660,12 +668,15 @@ static int vhost_dev_set_features(struct vhost_dev
> > > *dev, bool enable_log)
> > >          features |= 0x1ULL << VHOST_F_LOG_ALL;
> > >      }
> > >      r = dev->vhost_ops->vhost_set_features(dev, features);
> > > +    if (r < 0) {
> > > +        error_report("Failed to set features");
> > > +    }
> > >      return r < 0 ? -errno : 0;
> > >  }
> > >  
> > >  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> > >  {
> > > -    int r, t, i, idx;
> > > +    int r, i, idx;
> > >      r = vhost_dev_set_features(dev, enable_log);
> > >      if (r < 0) {
> > >          goto err_features;
> > > @@ -682,12 +693,10 @@ static int vhost_dev_set_log(struct vhost_dev *dev,
> > > bool enable_log)
> > >  err_vq:
> > >      for (; i >= 0; --i) {
> > >          idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> > > -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > > -                                     dev->log_enabled);
> > > -        assert(t >= 0);
> > > +        vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > > +                                 dev->log_enabled);
> > >      }
> > > -    t = vhost_dev_set_features(dev, dev->log_enabled);
> > > -    assert(t >= 0);
> > > +    vhost_dev_set_features(dev, dev->log_enabled);
> > >  err_features:
> > >      return r;
> > >  }
> > > @@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
> > >          }
> > >      }
> > >  
> > > -    assert (r >= 0);
> > >      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev,
> > >      idx),
> > >                                0, virtio_queue_get_ring_size(vdev, idx));
> > >      cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev,
> > >      idx),
> > > @@ -1187,7 +1195,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> > > VirtIODevice *vdev, int n,
> > >  
> > >      file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
> > >      r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
> > > -    assert(r >= 0);
> > > +    if (r < 0) {
> > > +        error_report("Failed to set vring call");
> > > +    }
> > >  }
> > >  
> > >  uint64_t vhost_get_features(struct vhost_dev *hdev, const int
> > >  *feature_bits,
> > > --
> > > 2.7.4
> >

On Thu, Jun 23, 2016 at 7:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > If it's ok and we can recover, then why should we print errors?
>>
>> To me, the current disconnect handling is not handled cleanly. There
>> is not much/nothing in the protocol that tells when and how you can
>> disconnect. If qemu vhost-user reconnection works today, it is mostly
>> by luck. Imho, we should print errors if any call to vhost user fails
>> to help further analysis of broken behaviour.
>
> But we decided disconnect is OK, did we not? So now a failure like this
> is just expected. It's not broken behaviour anymore ...

As you know, there are many ways qemu or the vm can break when the
backend is disconnected.  For now, it would help a lot if we had a bit
of error messages in this case. But do we really want to support
spurious disconnect/reconnect at any time? It's going to be
challenging, to maintain, to test... Is it really worthwhile? I doubt
it. I think it would be easier and more future-proof to have a
dedicated vhost-user request for that and only do a best effort for
the ungraceful disconnect.

>> And we shoud have a
>> clean mechanism to handle shutdown/disconnect/reconnect.
>
>
> At some level this is something that's missing here.
>
> How about we patch docs/specs/vhost-user.txt
> and describe how is reconnection supposed to work?
>
> All we have is:
>         If Master is unable to send the full message or receives a wrong reply
>         it will close the connection. An optional reconnection mechanism can be
>         implemented.

This text is there from the original commit but it doesn't say how to
reconnect and or how to recover from the ring. I don't have enough
experience with virtio in general to say when it is acceptable for
backend to be gone (not processing the ring), I can imagine the
implementation vary widely, and the requirements depend on the kind of
device.

If we limit the spec to vhost-user protocol only and leave it open on
how each virtio device should support ungraceful disconnect/reconnect,
then it sounds reasonable to document that after such disconnect, on
reconnect:
- the server assumes the backend has no knowledge of previous connection
- the state can be changed between reconnections (new ring state, mem table etc)
- the server will check feature compatibility with previous backend
and reject incompatible backends
- backend must restore ring processing from used->idx and ignore
VHOST_USER_SET_VRING_BASE (or should we change and document that in
this case the ring base is updated from used->idx?)

(it's probably not a complete list, I am not good at imagining all
possible cases)

On Fri, Jun 24, 2016 at 03:25:30PM +0200, Marc-André Lureau wrote:
> On Thu, Jun 23, 2016 at 7:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > If it's ok and we can recover, then why should we print errors?
> >>
> >> To me, the current disconnect handling is not handled cleanly. There
> >> is not much/nothing in the protocol that tells when and how you can
> >> disconnect. If qemu vhost-user reconnection works today, it is mostly
> >> by luck. Imho, we should print errors if any call to vhost user fails
> >> to help further analysis of broken behaviour.
> >
> > But we decided disconnect is OK, did we not? So now a failure like this
> > is just expected. It's not broken behaviour anymore ...
> 
> As you know, there are many ways qemu or the vm can break when the
> backend is disconnected.  For now, it would help a lot if we had a bit
> of error messages in this case. But do we really want to support
> spurious disconnect/reconnect at any time? It's going to be
> challenging, to maintain, to test... Is it really worthwhile? I doubt
> it. I think it would be easier and more future-proof to have a
> dedicated vhost-user request for that and only do a best effort for
> the ungraceful disconnect.

ungraceful might take a while. But I don't see a need for
message exchanges for shutdown. Do you?

> >> And we shoud have a
> >> clean mechanism to handle shutdown/disconnect/reconnect.
> >
> >
> > At some level this is something that's missing here.
> >
> > How about we patch docs/specs/vhost-user.txt
> > and describe how is reconnection supposed to work?
> >
> > All we have is:
> >         If Master is unable to send the full message or receives a wrong reply
> >         it will close the connection. An optional reconnection mechanism can be
> >         implemented.
> 
> This text is there from the original commit but it doesn't say how to
> reconnect and or how to recover from the ring. I don't have enough
> experience with virtio in general to say when it is acceptable for
> backend to be gone (not processing the ring), I can imagine the
> implementation vary widely, and the requirements depend on the kind of
> device.
> 
> If we limit the spec to vhost-user protocol only and leave it open on
> how each virtio device should support ungraceful disconnect/reconnect,
> then it sounds reasonable to document that after such disconnect, on
> reconnect:
> - the server assumes the backend has no knowledge of previous connection
> - the state can be changed between reconnections (new ring state, mem table etc)
> - the server will check feature compatibility with previous backend
> and reject incompatible backends
> - backend must restore ring processing from used->idx and ignore
> VHOST_USER_SET_VRING_BASE (or should we change and document that in
> this case the ring base is updated from used->idx?)

I think it's cleaner to change VHOST_USER_SET_VRING_BASE to get stuff
from used->idx.

> (it's probably not a complete list, I am not good at imagining all
> possible cases)

used ring must be flushed before disconnect (so all entries
before used->idx have been processed, and no entries
after used->idx have been processed).

If enabled, dirty log must be flushed before disconnect.


> 
> -- 
> Marc-André Lureau

Hi

On Sat, Jun 25, 2016 at 12:38 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jun 24, 2016 at 03:25:30PM +0200, Marc-André Lureau wrote:
>> On Thu, Jun 23, 2016 at 7:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > If it's ok and we can recover, then why should we print errors?
>> >>
>> >> To me, the current disconnect handling is not handled cleanly. There
>> >> is not much/nothing in the protocol that tells when and how you can
>> >> disconnect. If qemu vhost-user reconnection works today, it is mostly
>> >> by luck. Imho, we should print errors if any call to vhost user fails
>> >> to help further analysis of broken behaviour.
>> >
>> > But we decided disconnect is OK, did we not? So now a failure like this
>> > is just expected. It's not broken behaviour anymore ...
>>
>> As you know, there are many ways qemu or the vm can break when the
>> backend is disconnected.  For now, it would help a lot if we had a bit
>> of error messages in this case. But do we really want to support
>> spurious disconnect/reconnect at any time? It's going to be
>> challenging, to maintain, to test... Is it really worthwhile? I doubt
>> it. I think it would be easier and more future-proof to have a
>> dedicated vhost-user request for that and only do a best effort for
>> the ungraceful disconnect.
>
> ungraceful might take a while. But I don't see a need for
> message exchanges for shutdown. Do you?

A message exchange would allow the server to do something before
actually shuting down.: to check if shutdown is allowed/clean, to
flush some pending operations, to change device state, to request
something before shutdown (such as ring base etc),...

>> >> And we shoud have a
>> >> clean mechanism to handle shutdown/disconnect/reconnect.
>> >
>> >
>> > At some level this is something that's missing here.
>> >
>> > How about we patch docs/specs/vhost-user.txt
>> > and describe how is reconnection supposed to work?
>> >
>> > All we have is:
>> >         If Master is unable to send the full message or receives a wrong reply
>> >         it will close the connection. An optional reconnection mechanism can be
>> >         implemented.
>>
>> This text is there from the original commit but it doesn't say how to
>> reconnect and or how to recover from the ring. I don't have enough
>> experience with virtio in general to say when it is acceptable for
>> backend to be gone (not processing the ring), I can imagine the
>> implementation vary widely, and the requirements depend on the kind of
>> device.
>>
>> If we limit the spec to vhost-user protocol only and leave it open on
>> how each virtio device should support ungraceful disconnect/reconnect,
>> then it sounds reasonable to document that after such disconnect, on
>> reconnect:
>> - the server assumes the backend has no knowledge of previous connection
>> - the state can be changed between reconnections (new ring state, mem table etc)
>> - the server will check feature compatibility with previous backend
>> and reject incompatible backends
>> - backend must restore ring processing from used->idx and ignore
>> VHOST_USER_SET_VRING_BASE (or should we change and document that in
>> this case the ring base is updated from used->idx?)
>
> I think it's cleaner to change VHOST_USER_SET_VRING_BASE to get stuff
> from used->idx.

The problem with this approach is that it depends on how the backend
process the rings. Processing packets in order doesn't seem to be
required in general, or is it?

>> (it's probably not a complete list, I am not good at imagining all
>> possible cases)
>
> used ring must be flushed before disconnect (so all entries
> before used->idx have been processed, and no entries
> after used->idx have been processed).
> If enabled, dirty log must be flushed before disconnect.

Ok that sounds like a good requirement for "non-explicit graceful
shutdown" (how would you name it?) and seem to solve the processing
order problem. Yet, at this point, I think it would be easy for the
backend to do a proper shutdown request with all the benefits I
listed.

On Mon, Jun 27, 2016 at 11:01:42AM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Sat, Jun 25, 2016 at 12:38 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Jun 24, 2016 at 03:25:30PM +0200, Marc-André Lureau wrote:
> >> On Thu, Jun 23, 2016 at 7:13 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> > If it's ok and we can recover, then why should we print errors?
> >> >>
> >> >> To me, the current disconnect handling is not handled cleanly. There
> >> >> is not much/nothing in the protocol that tells when and how you can
> >> >> disconnect. If qemu vhost-user reconnection works today, it is mostly
> >> >> by luck. Imho, we should print errors if any call to vhost user fails
> >> >> to help further analysis of broken behaviour.
> >> >
> >> > But we decided disconnect is OK, did we not? So now a failure like this
> >> > is just expected. It's not broken behaviour anymore ...
> >>
> >> As you know, there are many ways qemu or the vm can break when the
> >> backend is disconnected.  For now, it would help a lot if we had a bit
> >> of error messages in this case. But do we really want to support
> >> spurious disconnect/reconnect at any time? It's going to be
> >> challenging, to maintain, to test... Is it really worthwhile? I doubt
> >> it. I think it would be easier and more future-proof to have a
> >> dedicated vhost-user request for that and only do a best effort for
> >> the ungraceful disconnect.
> >
> > ungraceful might take a while. But I don't see a need for
> > message exchanges for shutdown. Do you?
> 
> A message exchange would allow the server to do something before
> actually shuting down.: to check if shutdown is allowed/clean, to
> flush some pending operations, to change device state, to request
> something before shutdown (such as ring base etc),...

Who's the server here? It makes sense for backend to flush things,
but qemu doesn't seem to maintain too much state.

Could you be a bit more explicit?


I think it's ok to add a new message if it actually brings some
benefit, but I'm not sure why it makes sense to do it just in case.

> 
> >> >> And we shoud have a
> >> >> clean mechanism to handle shutdown/disconnect/reconnect.
> >> >
> >> >
> >> > At some level this is something that's missing here.
> >> >
> >> > How about we patch docs/specs/vhost-user.txt
> >> > and describe how is reconnection supposed to work?
> >> >
> >> > All we have is:
> >> >         If Master is unable to send the full message or receives a wrong reply
> >> >         it will close the connection. An optional reconnection mechanism can be
> >> >         implemented.
> >>
> >> This text is there from the original commit but it doesn't say how to
> >> reconnect and or how to recover from the ring. I don't have enough
> >> experience with virtio in general to say when it is acceptable for
> >> backend to be gone (not processing the ring), I can imagine the
> >> implementation vary widely, and the requirements depend on the kind of
> >> device.
> >>
> >> If we limit the spec to vhost-user protocol only and leave it open on
> >> how each virtio device should support ungraceful disconnect/reconnect,
> >> then it sounds reasonable to document that after such disconnect, on
> >> reconnect:
> >> - the server assumes the backend has no knowledge of previous connection
> >> - the state can be changed between reconnections (new ring state, mem table etc)
> >> - the server will check feature compatibility with previous backend
> >> and reject incompatible backends
> >> - backend must restore ring processing from used->idx and ignore
> >> VHOST_USER_SET_VRING_BASE (or should we change and document that in
> >> this case the ring base is updated from used->idx?)
> >
> > I think it's cleaner to change VHOST_USER_SET_VRING_BASE to get stuff
> > from used->idx.
> 
> The problem with this approach is that it depends on how the backend
> process the rings. Processing packets in order doesn't seem to be
> required in general, or is it?


The requirement is that packets are flushed before socket
is closed.

If they are in order then you can close at any point.

With out of order, you need to flush them before closing socket.




> >> (it's probably not a complete list, I am not good at imagining all
> >> possible cases)
> >
> > used ring must be flushed before disconnect (so all entries
> > before used->idx have been processed, and no entries
> > after used->idx have been processed).
> > If enabled, dirty log must be flushed before disconnect.
> 
> Ok that sounds like a good requirement for "non-explicit graceful
> shutdown" (how would you name it?) and seem to solve the processing
> order problem. Yet, at this point, I think it would be easy for the
> backend to do a proper shutdown request with all the benefits I
> listed.
> 
> 
> -- 
> Marc-André Lureau

On Mon, Jul 4, 2016 at 5:43 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> I think it's ok to add a new message if it actually brings some
> benefit, but I'm not sure why it makes sense to do it just in case.

The main benefit today would be to have a single code path to handle
disconnection, not dozens. The experimental branch I have (I have not
touched it for a while, it's unfinished etc), does also check that the
backend support a stop state, changing the link status state for ex.
Anyway, it's not part of this series, but I believe it would be way
easier to support this method in the long run than any time backend
disconnect...