On Wed, Dec 20, 2017 at 05:02:18PM +0100, Marc-André Lureau wrote: > Hi > > On Wed, Dec 13, 2017 at 3:29 AM, Changpeng Liu <changpeng....@intel.com> > wrote: > > This commit introcudes a vhost-user-blk backend device, it uses UNIX > > domain socket to communicate with QEMU. The vhost-user-blk sample > > application should be used with QEMU vhost-user-blk-pci device. > > > > To use it, complie with: > > make vhost-user-blk > > > > and start like this: > > vhost-user-blk -b /dev/sdb -s /path/vhost.socket > > > > Signed-off-by: Changpeng Liu <changpeng....@intel.com> > > Looks ok overall, just a few code cleanups would be welcome,
I merged it by now, pls do cleanups by adding patches on top. > > --- > > .gitignore | 1 + > > Makefile | 3 + > > Makefile.objs | 1 + > > contrib/vhost-user-blk/Makefile.objs | 1 + > > contrib/vhost-user-blk/vhost-user-blk.c | 550 > > ++++++++++++++++++++++++++++++++ > > 5 files changed, 556 insertions(+) > > create mode 100644 contrib/vhost-user-blk/Makefile.objs > > create mode 100644 contrib/vhost-user-blk/vhost-user-blk.c > > > > diff --git a/.gitignore b/.gitignore > > index 588769b..495f854 100644 > > --- a/.gitignore > > +++ b/.gitignore > > @@ -55,6 +55,7 @@ > > /scsi/qemu-pr-helper > > /vscclient > > /vhost-user-scsi > > +/vhost-user-blk > > /fsdev/virtfs-proxy-helper > > *.tmp > > *.[1-9] > > diff --git a/Makefile b/Makefile > > index ab0354c..3aad271 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -327,6 +327,7 @@ dummy := $(call unnest-vars,, \ > > ivshmem-server-obj-y \ > > libvhost-user-obj-y \ > > vhost-user-scsi-obj-y \ > > + vhost-user-blk-obj-y \ > > qga-vss-dll-obj-y \ > > block-obj-y \ > > block-obj-m \ > > @@ -558,6 +559,8 @@ ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) > > $(COMMON_LDADDS) > > endif > > vhost-user-scsi$(EXESUF): $(vhost-user-scsi-obj-y) libvhost-user.a > > $(call LINK, $^) > > +vhost-user-blk$(EXESUF): $(vhost-user-blk-obj-y) libvhost-user.a > > + $(call LINK, $^) > > > > module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak > > $(call quiet-command,$(PYTHON) $< $@ \ > > diff --git a/Makefile.objs b/Makefile.objs > > index 285c6f3..ae9aef7 100644 > > --- a/Makefile.objs > > +++ b/Makefile.objs > > @@ -115,6 +115,7 @@ libvhost-user-obj-y = contrib/libvhost-user/ > > vhost-user-scsi.o-cflags := $(LIBISCSI_CFLAGS) > > vhost-user-scsi.o-libs := $(LIBISCSI_LIBS) > > vhost-user-scsi-obj-y = contrib/vhost-user-scsi/ > > +vhost-user-blk-obj-y = contrib/vhost-user-blk/ > > > > ###################################################################### > > trace-events-subdirs = > > diff --git a/contrib/vhost-user-blk/Makefile.objs > > b/contrib/vhost-user-blk/Makefile.objs > > new file mode 100644 > > index 0000000..72e2cdc > > --- /dev/null > > +++ b/contrib/vhost-user-blk/Makefile.objs > > @@ -0,0 +1 @@ > > +vhost-user-blk-obj-y = vhost-user-blk.o > > diff --git a/contrib/vhost-user-blk/vhost-user-blk.c > > b/contrib/vhost-user-blk/vhost-user-blk.c > > new file mode 100644 > > index 0000000..841c3c3 > > --- /dev/null > > +++ b/contrib/vhost-user-blk/vhost-user-blk.c > > @@ -0,0 +1,550 @@ > > +/* > > + * vhost-user-blk sample application > > + * > > + * Copyright (c) 2017 Intel Corporation. All rights reserved. > > + * > > + * Author: > > + * Changpeng Liu <changpeng....@intel.com> > > + * > > + * This work is based on the "vhost-user-scsi" sample and "virtio-blk" > > driver > > + * implemention by: > > + * Felipe Franciosi <fel...@nutanix.com> > > + * Anthony Liguori <aligu...@us.ibm.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2 only. > > + * See the COPYING file in the top-level directory. > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "standard-headers/linux/virtio_blk.h" > > +#include "contrib/libvhost-user/libvhost-user-glib.h" > > +#include "contrib/libvhost-user/libvhost-user.h" > > + > > +#include <glib.h> > > + > > +struct virtio_blk_inhdr { > > + unsigned char status; > > +}; > > + > > +/* vhost user block device */ > > +typedef struct VubDev { > > + VugDev parent; > > + int blk_fd; > > + struct virtio_blk_config blkcfg; > > + char *blk_name; > > + GMainLoop *loop; > > +} VubDev; > > + > > +typedef struct VubReq { > > + VuVirtqElement *elem; > > + int64_t sector_num; > > + size_t size; > > + struct virtio_blk_inhdr *in; > > + struct virtio_blk_outhdr *out; > > + VubDev *vdev_blk; > > + struct VuVirtq *vq; > > +} VubReq; > > + > > +/** refer util/iov.c **/ > > +static size_t vub_iov_size(const struct iovec *iov, > > + const unsigned int iov_cnt) > > +{ > > + size_t len; > > + unsigned int i; > > + > > + len = 0; > > + for (i = 0; i < iov_cnt; i++) { > > + len += iov[i].iov_len; > > + } > > + return len; > > +} > > + > > +static void vub_panic_cb(VuDev *vu_dev, const char *buf) > > +{ > > + VugDev *gdev; > > + VubDev *vdev_blk; > > + > > + assert(vu_dev); > > + > > + gdev = container_of(vu_dev, VugDev, parent); > > + vdev_blk = container_of(gdev, VubDev, parent); > > + if (buf) { > > + g_warning("vu_panic: %s", buf); > > + } > > + > > + g_main_loop_quit(vdev_blk->loop); > > +} > > + > > +static void vub_req_complete(VubReq *req) > > +{ > > + VugDev *gdev = &req->vdev_blk->parent; > > + VuDev *vu_dev = &gdev->parent; > > + > > + /* IO size with 1 extra status byte */ > > + vu_queue_push(vu_dev, req->vq, req->elem, > > + req->size + 1); > > + vu_queue_notify(vu_dev, req->vq); > > + > > + if (req->elem) { > > + free(req->elem); > > + } > > + g_free(req); > > +} > > + > > +static int vub_open(const char *file_name, bool wce) > > +{ > > + int fd; > > + int flags = O_RDWR; > > + > > + if (!wce) { > > + flags |= O_DIRECT; > > + } > > + > > + fd = open(file_name, flags); > > + if (fd < 0) { > > + fprintf(stderr, "Cannot open file %s, %s\n", file_name, > > + strerror(errno)); > > + return -1; > > + } > > + > > + return fd; > > +} > > + > > +static void vub_close(int fd) > > +{ > > + if (fd >= 0) { > > + close(fd); > > + } > > +} > > Not sure that function is worth it, see below: > > > + > > +static ssize_t > > +vub_readv(VubReq *req, struct iovec *iov, uint32_t iovcnt) > > +{ > > + VubDev *vdev_blk = req->vdev_blk; > > + ssize_t rc; > > + > > + if (!iovcnt) { > > + fprintf(stderr, "Invalid Read IOV count\n"); > > + return -1; > > + } > > + > > + req->size = vub_iov_size(iov, iovcnt); > > + rc = preadv(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512); > > + if (rc < 0) { > > + fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n", > > + vdev_blk->blk_name, req->sector_num, req->size, > > + strerror(errno)); > > + return -1; > > + } > > + > > + return rc; > > +} > > + > > +static ssize_t > > +vub_writev(VubReq *req, struct iovec *iov, uint32_t iovcnt) > > +{ > > + VubDev *vdev_blk = req->vdev_blk; > > + ssize_t rc; > > + > > + if (!iovcnt) { > > + fprintf(stderr, "Invalid Write IOV count\n"); > > + return -1; > > + } > > + > > + req->size = vub_iov_size(iov, iovcnt); > > + rc = pwritev(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512); > > + if (rc < 0) { > > + fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n", > > + vdev_blk->blk_name, req->sector_num, req->size, > > + strerror(errno)); > > + return -1; > > + } > > + > > + return rc; > > +} > > + > > +static void > > +vub_flush(VubReq *req) > > +{ > > + VubDev *vdev_blk = req->vdev_blk; > > + > > + fdatasync(vdev_blk->blk_fd); > > +} > > + > > + > > 2 lines? > > > +static int vub_virtio_process_req(VubDev *vdev_blk, > > + VuVirtq *vq) > > +{ > > + VugDev *gdev = &vdev_blk->parent; > > + VuDev *vu_dev = &gdev->parent; > > + VuVirtqElement *elem; > > + uint32_t type; > > + unsigned in_num; > > + unsigned out_num; > > + VubReq *req; > > + > > + elem = vu_queue_pop(vu_dev, vq, sizeof(VuVirtqElement)); > > + if (!elem) { > > + return -1; > > + } > > + > > + /* refer to hw/block/virtio_blk.c */ > > + if (elem->out_num < 1 || elem->in_num < 1) { > > + fprintf(stderr, "virtio-blk request missing headers\n"); > > + free(elem); > > + return -1; > > + } > > + > > + req = g_new0(VubReq, 1); > > + req->vdev_blk = vdev_blk; > > + req->vq = vq; > > + req->elem = elem; > > + > > + in_num = elem->in_num; > > + out_num = elem->out_num; > > + > > + /* don't support VIRTIO_F_ANY_LAYOUT and virtio 1.0 only */ > > + if (elem->out_sg[0].iov_len < sizeof(struct virtio_blk_outhdr)) { > > + fprintf(stderr, "Invalid outhdr size\n"); > > + goto err; > > + } > > + req->out = (struct virtio_blk_outhdr *)elem->out_sg[0].iov_base; > > + out_num--; > > + > > + if (elem->in_sg[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) > > { > > + fprintf(stderr, "Invalid inhdr size\n"); > > + goto err; > > + } > > + req->in = (struct virtio_blk_inhdr *)elem->in_sg[in_num - 1].iov_base; > > + in_num--; > > + > > + type = le32toh(req->out->type); > > + switch (type & ~(VIRTIO_BLK_T_OUT | VIRTIO_BLK_T_BARRIER)) { > > + case VIRTIO_BLK_T_IN: { > > + ssize_t ret = 0; > > + bool is_write = type & VIRTIO_BLK_T_OUT; > > + req->sector_num = le64toh(req->out->sector); > > + if (is_write) { > > + ret = vub_writev(req, &elem->out_sg[1], out_num); > > + } else { > > + ret = vub_readv(req, &elem->in_sg[0], in_num); > > + } > > + if (ret >= 0) { > > + req->in->status = VIRTIO_BLK_S_OK; > > + } else { > > + req->in->status = VIRTIO_BLK_S_IOERR; > > + } > > + vub_req_complete(req); > > + break; > > + } > > + case VIRTIO_BLK_T_FLUSH: { > > + vub_flush(req); > > + req->in->status = VIRTIO_BLK_S_OK; > > + vub_req_complete(req); > > + break; > > + } > > + case VIRTIO_BLK_T_GET_ID: { > > + size_t size = MIN(vub_iov_size(&elem->in_sg[0], in_num), > > + VIRTIO_BLK_ID_BYTES); > > + snprintf(elem->in_sg[0].iov_base, size, "%s", > > "vhost_user_blk"); > > + req->in->status = VIRTIO_BLK_S_OK; > > + req->size = elem->in_sg[0].iov_len; > > + vub_req_complete(req); > > + break; > > + } > > + default: { > > + req->in->status = VIRTIO_BLK_S_UNSUPP; > > + vub_req_complete(req); > > + break; > > + } > > + } > > + > > + return 0; > > + > > +err: > > + free(elem); > > + g_free(req); > > + return -1; > > +} > > + > > +static void vub_process_vq(VuDev *vu_dev, int idx) > > +{ > > + VugDev *gdev; > > + VubDev *vdev_blk; > > + VuVirtq *vq; > > + int ret; > > + > > + if ((idx < 0) || (idx >= VHOST_MAX_NR_VIRTQUEUE)) { > > + fprintf(stderr, "VQ Index out of range: %d\n", idx); > > + vub_panic_cb(vu_dev, NULL); > > + return; > > + } > > + > > + gdev = container_of(vu_dev, VugDev, parent); > > + vdev_blk = container_of(gdev, VubDev, parent); > > + assert(vdev_blk); > > + > > + vq = vu_get_queue(vu_dev, idx); > > + assert(vq); > > + > > + while (1) { > > + ret = vub_virtio_process_req(vdev_blk, vq); > > + if (ret) { > > + break; > > + } > > + } > > +} > > + > > +static void vub_queue_set_started(VuDev *vu_dev, int idx, bool started) > > +{ > > + VuVirtq *vq; > > + > > + assert(vu_dev); > > + > > + vq = vu_get_queue(vu_dev, idx); > > + vu_set_queue_handler(vu_dev, vq, started ? vub_process_vq : NULL); > > +} > > + > > +static uint64_t > > +vub_get_features(VuDev *dev) > > +{ > > + return 1ull << VIRTIO_BLK_F_SIZE_MAX | > > + 1ull << VIRTIO_BLK_F_SEG_MAX | > > + 1ull << VIRTIO_BLK_F_TOPOLOGY | > > + 1ull << VIRTIO_BLK_F_BLK_SIZE | > > + 1ull << VIRTIO_BLK_F_FLUSH | > > + 1ull << VIRTIO_BLK_F_CONFIG_WCE | > > + 1ull << VIRTIO_F_VERSION_1 | > > + 1ull << VHOST_USER_F_PROTOCOL_FEATURES; > > +} > > + > > +static int > > +vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len) > > +{ > > + VugDev *gdev; > > + VubDev *vdev_blk; > > + > > + gdev = container_of(vu_dev, VugDev, parent); > > + vdev_blk = container_of(gdev, VubDev, parent); > > + memcpy(config, &vdev_blk->blkcfg, len); > > + > > + return 0; > > +} > > + > > +static int > > +vub_set_config(VuDev *vu_dev, const uint8_t *data, > > + uint32_t offset, uint32_t size, uint32_t flags) > > +{ > > + VugDev *gdev; > > + VubDev *vdev_blk; > > + uint8_t wce; > > + int fd; > > + > > + /* don't support live migration */ > > + if (flags != VHOST_SET_CONFIG_TYPE_MASTER) { > > + return -1; > > + } > > + > > + gdev = container_of(vu_dev, VugDev, parent); > > + vdev_blk = container_of(gdev, VubDev, parent); > > + > > + if (offset != offsetof(struct virtio_blk_config, wce) || > > + size != 1) { > > + return -1; > > + } > > + > > + wce = *data; > > + if (wce == vdev_blk->blkcfg.wce) { > > + /* Do nothing as same with old configuration */ > > + return 0; > > + } > > + > > + vdev_blk->blkcfg.wce = wce; > > + fprintf(stdout, "Write Cache Policy Changed\n"); > > + vub_close(vdev_blk->blk_fd); > > if (vdev_blk->blk_fd >= 0) > close(vdev_blk->blk_fd); > vdev_blk->blk_fd = -1; > > > + > > + fd = vub_open(vdev_blk->blk_name, wce); > > + if (fd < 0) { > > + fprintf(stderr, "Error to open block device %s\n", > > vdev_blk->blk_name); > > + vdev_blk->blk_fd = -1; > > + return -1; > > + } > > + vdev_blk->blk_fd = fd; > > + > > + return 0; > > +} > > + > > +static const VuDevIface vub_iface = { > > + .get_features = vub_get_features, > > + .queue_set_started = vub_queue_set_started, > > + .get_config = vub_get_config, > > + .set_config = vub_set_config, > > +}; > > + > > +static int unix_sock_new(char *unix_fn) > > +{ > > + int sock; > > + struct sockaddr_un un; > > + size_t len; > > + > > + assert(unix_fn); > > + > > + sock = socket(AF_UNIX, SOCK_STREAM, 0); > > + if (sock <= 0) { > > + perror("socket"); > > + return -1; > > + } > > + > > + un.sun_family = AF_UNIX; > > + (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn); > > + len = sizeof(un.sun_family) + strlen(un.sun_path); > > + > > + (void)unlink(unix_fn); > > + if (bind(sock, (struct sockaddr *)&un, len) < 0) { > > + perror("bind"); > > + goto fail; > > + } > > + > > + if (listen(sock, 1) < 0) { > > + perror("listen"); > > + goto fail; > > + } > > + > > + return sock; > > + > > +fail: > > + (void)close(sock); > > + > > + return -1; > > +} > > + > > +static void vub_deinit(struct VubDev *vdev_blk) > > +{ > > + if (!vdev_blk) { > > + return; > > + } > > Make it vub_free() to remove the g_free() after the call below? > > > + > > + if (vdev_blk->blk_fd) { > > + vub_close(vdev_blk->blk_fd); > > if (vdev_blk->blk_fd >= 0) > close(vdev_blk->blk_fd) > > & remove vub_close() > > > + } > > +} > > + > > +static uint32_t > > +vub_get_blocksize(int fd) > > +{ > > + uint32_t blocksize = 512; > > + > > + #if defined(__linux__) && defined(BLKSSZGET) > > + if (ioctl(fd, BLKSSZGET, &blocksize) == 0) { > > + return blocklen; > > + } > > + #endif > > + > > + return blocksize; > > +} > > + > > +static void > > +vub_initialize_config(int fd, struct virtio_blk_config *config) > > +{ > > + off64_t capacity; > > + > > + capacity = lseek64(fd, 0, SEEK_END); > > + config->capacity = capacity >> 9; > > + config->blk_size = vub_get_blocksize(fd); > > + config->size_max = 65536; > > + config->seg_max = 128 - 2; > > + config->min_io_size = 1; > > + config->opt_io_size = 1; > > + config->num_queues = 1; > > +} > > + > > +static int > > +vub_new_block(VubDev *vdev_blk, char *blk_file) > > Make it vub_new() instead? > > > +{ > > + vdev_blk->blk_fd = vub_open(blk_file, 0); > > + if (vdev_blk->blk_fd < 0) { > > + fprintf(stderr, "Error to open block device %s\n", blk_file); > > + return -1; > > + } > > + vdev_blk->blkcfg.wce = 0; > > + vdev_blk->blk_name = blk_file; > > + > > + /* fill virtio_blk_config with block parameters */ > > + vub_initialize_config(vdev_blk->blk_fd, &vdev_blk->blkcfg); > > + > > + return 0; > > +} > > + > > +int main(int argc, char **argv) > > +{ > > + int opt; > > + char *unix_socket = NULL; > > + char *blk_file = NULL; > > + int lsock = -1, csock = -1, rc; > > + VubDev *vdev_blk = NULL; > > + > > + while ((opt = getopt(argc, argv, "b:h:s:")) != -1) { > > + switch (opt) { > > + case 'b': > > + blk_file = g_strdup(optarg); > > + break; > > + case 's': > > + unix_socket = g_strdup(optarg); > > + break; > > + case 'h': > > + default: > > + printf("Usage: %s [-b block device or file, -s UNIX domain > > socket]" > > + " | [ -h ]\n", argv[0]); > > + return -1; > > + } > > + } > > + > > + if (!unix_socket || !blk_file) { > > + printf("Usage: %s [-b block device or file, -s UNIX domain socket] > > |" > > + " [ -h ]\n", argv[0]); > > + return -1; > > + } > > + > > + lsock = unix_sock_new(unix_socket); > > + if (lsock < 0) { > > + goto err; > > + } > > + > > + csock = accept(lsock, (void *)0, (void *)0); > > + if (csock < 0) { > > + fprintf(stderr, "Accept error %s\n", strerror(errno)); > > + goto err; > > + } > > + > > + vdev_blk = g_new0(VubDev, 1); > > + vdev_blk->loop = g_main_loop_new(NULL, FALSE); > > + > > + rc = vub_new_block(vdev_blk, blk_file); > > The 3 lines above could be simplified to be: > > vub = vub_new(blk_file); > > > + if (rc) { > > + goto err; > > + } > > + > > + vug_init(&vdev_blk->parent, csock, vub_panic_cb, &vub_iface); > > + > > + g_main_loop_run(vdev_blk->loop); > > + > > + vug_deinit(&vdev_blk->parent); > > + > > +err: > > + if (vdev_blk) { > > + g_main_loop_unref(vdev_blk->loop); > > + vub_deinit(vdev_blk); > > + g_free(vdev_blk); > > + } > > And the above block could simply be: > > and this could simply be: > > vub_free(vub) > > > + if (csock >= 0) { > > + close(csock); > > + } > > + if (lsock >= 0) { > > + close(lsock); > > + } > > + g_free(unix_socket); > > + g_free(blk_file); > > + > > + return 0; > > +} > > + > > Please remove that extra empty line > > > > -- > Marc-André Lureau