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 <[email protected]> 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*
