Re: [Qemu-devel] [PATCH for-4.0 0/6] vhost-user-blk: Add support for backend reconnecting

[Michael S. Tsirkin]

On Fri, Dec 14, 2018 at 12:36:01PM +0800, Jason Wang wrote:
> 
> On 2018/12/13 下午10:56, Michael S. Tsirkin wrote:
> > On Thu, Dec 13, 2018 at 11:41:06AM +0800, Yongji Xie wrote:
> > > On Thu, 13 Dec 2018 at 10:58, Jason Wang <address@hidden> wrote:
> > > > 
> > > > On 2018/12/12 下午5:18, Yongji Xie wrote:
> > > > > > > > Ok, then we can simply forbid increasing the avail_idx in this 
> > > > > > > > case?
> > > > > > > > 
> > > > > > > > Basically, it's a question of whether or not it's better to 
> > > > > > > > done it in
> > > > > > > > the level of virtio instead of vhost. I'm pretty sure if we 
> > > > > > > > expose
> > > > > > > > sufficient information, it could be done without touching 
> > > > > > > > vhost-user.
> > > > > > > > And we won't deal with e.g migration and other cases.
> > > > > > > > 
> > > > > > > OK, I get your point. That's indeed an alternative way. But this 
> > > > > > > feature seems
> > > > > > > to be only useful to vhost-user backend.
> > > > > > I admit I could not think of a use case other than vhost-user.
> > > > > > 
> > > > > > 
> > > > > > >     I'm not sure whether it make sense to
> > > > > > > touch virtio protocol for this feature.
> > > > > > Some possible advantages:
> > > > > > 
> > > > > > - Feature could be determined and noticed by user or management 
> > > > > > layer.
> > > > > > 
> > > > > > - There's no need to invent ring layout specific protocol to record 
> > > > > > in
> > > > > > flight descriptors. E.g if my understanding is correct, for this 
> > > > > > series
> > > > > > and for the example above, it still can not work for packed 
> > > > > > virtqueue
> > > > > > since descriptor id is not sufficient (descriptor could be 
> > > > > > overwritten
> > > > > > by used one). You probably need to have a (partial) copy of 
> > > > > > descriptor
> > > > > > ring for this.
> > > > > > 
> > > > > > - No need to deal with migration, all information was in guest 
> > > > > > memory.
> > > > > > 
> > > > > Yes, we have those advantages. But seems like handle this in 
> > > > > vhost-user
> > > > > level could be easier to be maintained in production environment. We 
> > > > > can
> > > > > support old guest. And the bug fix will not depend on guest kernel 
> > > > > updating.
> > > > 
> > > > Yes. But the my main concern is the layout specific data structure. If
> > > > it could be done through a generic structure (can it?), it would be
> > > > fine. Otherwise, I believe we don't want another negotiation about what
> > > > kind of layout that backend support for reconnect.
> > > > 
> > > Yes, the current layout in shared memory didn't support packed virtqueue 
> > > because
> > > the information of one descriptor in descriptor ring will not be
> > > available once device fetch it.
> > > 
> > > I also thought about a generic structure before. But I failed... So I
> > > tried another way
> > > to acheive that in this series. In QEMU side, we just provide a shared
> > > memory to backend
> > > and we didn't define anything for this memory. In backend side, they
> > > should know how to
> > > use those memory to record inflight I/O no matter what kind of
> > > virtqueue they used.
> > > Thus,  If we updates virtqueue for new virtio spec in the feature, we
> > > don't need to touch
> > > QEMU and guest. What do you think about it?
> > > 
> > > Thanks,
> > > Yongji
> > I think that's a good direction to take, yes.
> > Backends need to be very careful about the layout,
> > with versioning etc.
> 
> 
> I'm not sure this could be done 100% transparent to qemu. E.g you need to
> deal with reset I think and you need to carefully choose the size of the
> region. Which means you need negotiate the size, layout through backend.

I am not sure I follow. The point is all this state is internal to the
backend. QEMU does not care at all - it just helps a little by hanging
on to it.

> And
> need to deal with migration with them.

Good catch.
There definitely is an issue in that you can not migrate with backend
being disconnected: migration needs to flush the backend and we can't
when it's disconnected.  This needs to be addressed.
I think it's cleanest to just defer migration
until backend does reconnect.


Backend cross version migration is all messed up in vhost user, I agree.
There was a plan to fix it that was never executed unfortunately.
Maxime, do you still plan to look into it?

> This is another sin of splitting
> virtio dataplane from qemu anyway.
> 
> 
> Thanks

It wasn't split as such - dpdk was never a part of qemu.  We just
enabled it without fuse hacks.

-- 
MST