Re: [RFC] Implementing vhost-user-blk device backend

[Coiby Xu]

Hi Stefan,

Thank you for reviewing my work! All the improvements have been applied except for a small issue regarding object_add.

>  (qemu) object_add vhost-user-server,id=ID,chardev=CHARDEV,writable=on|off

Currently I implement object_add feature in the following syntax which use unix_socket directly instead of chardev,

  (qemu) object_add vhost-user-server,id=id=disk,unix_socket=/tmp/vhost-user-blk_vhost.socket,name=disk,writable=off

I know in QEMU we can create a socket server using chardev-add,

  (qemu) chardev-add socket,id=char1,path=/tmp/vhost-user-blk_vhost.socket

But it seems it's a bit cumbersome to utilize chardev. Take QMP over socket as an example, 

  $ x86_64-softmmu/qemu-system-x86_64 -drive file=dpdk.img,format=raw,if=none,id=disk -device ide-hd,drive=disk,bootindex=0 -m 128 -enable-kvm -chardev socket,id=mon1,path=/tmp/mon.sock,server,nowait -mon chardev=mon1,mode=control,pretty=on

It doesn't support multiple concurrent client connections because of the limitation of chardev/char-socket.c. 

On Thu, Dec 19, 2019 at 10:31 PM Stefan Hajnoczi <address@hidden> wrote:

On Mon, Nov 18, 2019 at 10:27:28PM +0800, Coiby Xu wrote:
> Hi all,
>
> This is an implementation of vhost-user-blk device backend by
> following https://wiki.qemu.org/Google_Summer_of_Code_2019#vhost-user-blk_device_backend.
> raw/qcow2 disk images can now be shared via vhost user protocol. In
> this way, it could provide better performance than QEMU's existing NBD
> support.

Thank you for working on this feature!

> +static size_t vub_iov_to_buf(const struct iovec *iov,
> +                             const unsigned int iov_cnt, void *buf)

Please take a look at utils/iov.c.  iov_to_buf_full() can be used
instead of defining this function.

> +{
> +    size_t len;
> +    unsigned int i;
> +
> +    len = 0;
> +    for (i = 0; i < iov_cnt; i++) {
> +        memcpy(buf + len,  iov[i].iov_base, iov[i].iov_len);
> +        len += iov[i].iov_len;
> +    }
> +    return len;
> +}
> +
> +static  VubDev *vub_device;

If you switch to -object (see below) then this global pointer will go
away so I won't comment on it throughout this patch.

> +static void vub_accept(QIONetListener *listener, QIOChannelSocket *sioc,
> +                       gpointer opaque)
> +{
> +    /* only one connection */
> +    if (vub_device->sioc) {
> +        return;
> +    }
> +
> +    vub_device->sioc = sioc;
> +    vub_device->listener = listener;
> +    /*
> +     * increase the object reference, so cioc will not freeed by
> +     * qio_net_listener_channel_func which will call object_unref(OBJECT(sioc))
> +     */
> +    object_ref(OBJECT(sioc));
> +
> +    qio_channel_set_name(QIO_CHANNEL(sioc), "vhost-server");
> +    if (!vug_init(&vub_device->parent, VHOST_USER_BLK_MAX_QUEUES, sioc->fd,
> +                  vub_panic_cb, &vub_iface)) {
> +        fprintf(stderr, "Failed to initialized libvhost-user-glib\n");
> +    }

vug_init() uses the default GMainContext, which is bad for performance
when there are many devices because it cannot take advantage of
multi-core CPUs.  vhost-user-server should support IOThread so that
devices can be run in dedicated threads.

The nbd/server.c:NBDExport->ctx field serves this purpose in the NBD
server.  It's a little trickier with libvhost-user-glib because the API
currently doesn't allow passing in a GMainContext and will need to be
extended.

> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index cfcc044ce4..d8de179747 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1614,6 +1614,33 @@ STEXI
>  @findex acl_reset
>  Remove all matches from the access control list, and set the default
>  policy back to @code{deny}.
> +ETEXI
> +
> +    {
> +        .name       = "vhost_user_server_stop",
> +        .args_type  = "",
> +        .params     = "vhost_user_server_stop",
> +        .help       = "stop vhost-user-blk device backend",
> +        .cmd        = hmp_vhost_user_server_stop,
> +    },
> +STEXI
> +@item vhost_user_server_stop
> +@findex vhost_user_server_stop
> +Stop the QEMU embedded vhost-user-blk device backend server.
> +ETEXI

The NBD server supports multiple client connections and exports
(drives).  A vhost-user socket only supports one connection and one
device.  I think it will be necessary to assign a unique identifier to
every vhost-user server.

By the way, I think the server should be a UserCreatable Object so the
following syntax works:

  $ qemu -object vhost-user-server,id=ID,chardev=CHARDEV,writable=on|off

And existing HMP/QMP commands can be used:

  (qemu) object_add vhost-user-server,id=ID,chardev=CHARDEV,writable=on|off
  (qemu) object_del ID

This way we don't need to define new HMP/QMP/command-line syntax for
vhost-user-server.

If you grep for UserCreatable you'll find examples like "iothread",
"secret", "throttle-group", etc.

--

Best regards,

Coiby