Re: [PATCH v4 0/5] vhost-user block device backend implementation

[Marc-André Lureau]

Hi

On Thu, Feb 27, 2020 at 12:39 PM Daniel P. Berrangé <address@hidden> wrote:
>
> On Thu, Feb 27, 2020 at 12:19:58PM +0100, Kevin Wolf wrote:
> > Am 27.02.2020 um 12:07 hat Marc-André Lureau geschrieben:
> > > On Thu, Feb 27, 2020 at 11:55 AM Kevin Wolf <address@hidden> wrote:
> > > > Am 27.02.2020 um 11:28 hat Coiby Xu geschrieben:
> > > > > > > we still need customized vu_message_read because libvhost-user 
> > > > > > > assumes
> > > > > > > we will always get a full-size VhostUserMsg and hasn't taken care 
> > > > > > > of
> > > > > > > this short read case. I will improve libvhost-user's 
> > > > > > > vu_message_read
> > > > > > > by making it keep reading from socket util getting enough bytes. I
> > > > > > > assume short read is a rare case thus introduced performance 
> > > > > > > penalty
> > > > > > > would be negligible.
> > > > >
> > > > > > In any case, please make sure that we use the QIOChannel functions
> > > > > > called from a coroutine in QEMU so that it will never block, but the
> > > > > > coroutine can just yield while it's waiting for more bytes.
> > > > >
> > > > > But if I am not wrong, libvhost-user is supposed to be indepdent from
> > > > > the main QEMU code. So it can't use the QIOChannel functions if we
> > > > > simply modify exiting vu_message_read to address the short read issue.
> > > > > In v3 & v4, I extended libvhost-user to allow vu_message_read to be
> > > > > replaced by one which will depend on the main QEMU code. I'm not sure
> > > > > which way is better.
> > > >
> > > > The way your latest patches have it, with a separate read function,
> > > > works for me.
> > >
> > > Done right, I am not against it, fwiw
> > >
> > > > You could probably change libvhost-user to reimplement the same
> > > > functionality, and it might be an improvement for other users of the
> > > > library, but it's also code duplication and doesn't provide more value
> > > > in the context of the vhost-user export in QEMU.
> > > >
> > > > The point that's really important to me is just that we never block when
> > > > we run inside QEMU because that would actually stall the guest. This
> > > > means busy waiting in a tight loop until read() returns enough bytes is
> > > > not acceptable in QEMU.
> > >
> > > In the context of vhost-user, local unix sockets with short messages
> > > (do we have >1k messages?), I am not sure if this is really a problem.
> >
> > I'm not sure how much of a problem it is in practice, and whether we
> > can consider the vhost-user client trusted. But using QIOChannel from
> > within a coroutine just avoids the problem completely, so it feels like
> > a natural choice to just do that.
>
> It isn't clear to me that we have a consitent plan for how we intend
> libvhost-user to develop & what it is permitted to use.  What information
> I see in the source code and in this thread are contradictory.
>
> For example, in the text quoted above:
>
>   "libvhost-user is supposed to be indepdent from the main QEMU code."
>
> which did match my overall understanding too. At the top of libvhost-user.c
> there is a comment
>
>    /* this code avoids GLib dependency */
>
> but a few lines later it does
>
>   #include "qemu/atomic.h"
>   #include "qemu/osdep.h"
>   #include "qemu/memfd.h"
>
> and in the Makefile we link it to much of QEMU util code:
>
>   libvhost-user.a: $(libvhost-user-obj-y) $(util-obj-y) $(stub-obj-y)
>
> this in turn pulls in GLib code, and looking at symbols we can see
> over 100 GLib functions used:
>
>   $ nm ./libvhost-user.a | grep 'U g_' | sort | uniq | wc -l
>   128
>
> And over 200 QEMU source object files included:
>
>   $ nm ./libvhost-user.a | grep '.o:' | sort | wc -l
>   224
>
> So unless I'm missing something, we have lost any independance from
> both QEMU and GLib code that we might have had in the past.

Yep, I think this is mostly due to commit 5f9ff1eff38 ("libvhost-user:
Support tracking inflight I/O in shared memory")

It may not be that hard to bring back glib independence. Is it worth it though?

>
> Note this also has licensing implications, as I expect this means that
> via the QEMU source objects it pulls in, libvhost-user.a has become
> a GPLv2-only combined work, not a GPLv2-or-later combined work.
>

libvhost-user.c is GPLv2-or-later because tests/vhost-user-bridge.c
was and is still as well. Do we need to change that?

> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>


-- 
Marc-André Lureau