Re: [Qemu-devel] [PATCH 05/24] vhost: change some assert() for error_report() or silent fail

[Michael S. Tsirkin]

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 <address@hidden> 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 <address@hidden> 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