[PATCH 0/1] vhost: add vhost_blk driver

[PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskikh]

vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
driver allows VM to reach a near bare-metal disk performance. See IOPS
numbers below (fio --rw=randread --bs=4k).

This implementation uses kiocb interface. It is slightly slower than
going directly through bio, but is simpler and also works with disk
images placed on a file system.

# fio num-jobs
# A: bare metal over block
# B: bare metal over file
# C: virtio-blk over block
# D: virtio-blk over file
# E: vhost-blk bio over block
# F: vhost-blk kiocb over block
# G: vhost-blk kiocb over file
#
#  A     B     C    D    E     F     G

1  171k  151k  148k 151k 195k  187k  175k
2  328k  302k  249k 241k 349k  334k  296k
3  479k  437k  179k 174k 501k  464k  404k
4  622k  568k  143k 183k 620k  580k  492k
5  755k  697k  136k 128k 737k  693k  579k
6  887k  808k  131k 120k 830k  782k  640k
7  1004k 926k  126k 131k 926k  863k  693k
8  1099k 1015k 117k 115k 1001k 931k  712k
9  1194k 1119k 115k 111k 1055k 991k  711k
10 1278k 1207k 109k 114k 1130k 1046k 695k
11 1345k 1280k 110k 108k 1119k 1091k 663k
12 1411k 1356k 104k 106k 1201k 1142k 629k
13 1466k 1423k 106k 106k 1260k 1170k 607k
14 1517k 1486k 103k 106k 1296k 1179k 589k
15 1552k 1543k 102k 102k 1322k 1191k 571k
16 1480k 1506k 101k 102k 1346k 1202k 566k

Vitaly Mayatskikh (1):
  Add vhost_blk driver

 drivers/vhost/Kconfig  |  13 ++
 drivers/vhost/Makefile |   3 +
 drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 526 insertions(+)
 create mode 100644 drivers/vhost/blk.c

-- 
2.17.1

[PATCH 1/1] Add vhost_blk driver

[Vitaly Mayatskikh]

This driver accelerates host side of virtio-blk.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
---
 drivers/vhost/Kconfig  |  13 ++
 drivers/vhost/Makefile |   3 +
 drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 526 insertions(+)
 create mode 100644 drivers/vhost/blk.c

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index b580885243f7..c4980d6af0ea 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -53,3 +53,16 @@ config VHOST_CROSS_ENDIAN_LEGACY
 	  adds some overhead, it is disabled by default.
 
 	  If unsure, say "N".
+
+config VHOST_BLK
+	tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
+	depends on BLOCK && EVENTFD
+	select VHOST
+	default n
+	help
+	 This kernel module can be loaded in host kernel to accelerate
+	 guest block with virtio_blk. Not to be confused with virtio_blk
+	 module itself which needs to be loaded in guest kernel.
+
+	 To compile this driver as a module, choose M here: the module will
+	 be called vhost_blk.
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
index 6c6df24f770c..c8be36cd9214 100644
--- a/drivers/vhost/Makefile
+++ b/drivers/vhost/Makefile
@@ -8,6 +8,9 @@ vhost_scsi-y := scsi.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost_vsock.o
 vhost_vsock-y := vsock.o
 
+obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
+vhost_blk-y := blk.o
+
 obj-$(CONFIG_VHOST_RING) += vringh.o
 
 obj-$(CONFIG_VHOST)	+= vhost.o
diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
new file mode 100644
index 000000000000..aefb9a61fa0f
--- /dev/null
+++ b/drivers/vhost/blk.c
@@ -0,0 +1,510 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 IBM Corporation
+ * Author: Vitaly Mayatskikh <v.mayatskih@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *
+ * virtio-blk server in host kernel.
+ */
+
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/virtio_blk.h>
+#include <linux/vhost.h>
+#include <linux/fs.h>
+#include "vhost.h"
+
+enum {
+	VHOST_BLK_FEATURES =
+	VHOST_FEATURES |
+	(1ULL << VIRTIO_F_IOMMU_PLATFORM) |
+	(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
+	(1ULL << VIRTIO_RING_F_EVENT_IDX) |
+	(1ULL << VIRTIO_BLK_F_MQ)
+};
+
+#define VHOST_BLK_SET_BACKEND _IOW(VHOST_VIRTIO, 0x50, int)
+
+enum {
+	VHOST_BLK_VQ_MAX = 16,
+	VHOST_BLK_VQ_MAX_REQS = 128,
+};
+
+struct vhost_blk_req {
+	struct llist_node list;
+	int index;
+	struct vhost_blk_queue *q;
+	struct virtio_blk_outhdr hdr;
+	struct iovec *out_iov;
+	struct iovec *in_iov;
+	u8 out_num;
+	u8 in_num;
+	long len;
+	struct kiocb iocb;
+	struct iov_iter i;
+	int res;
+	void __user *status;
+};
+
+struct vhost_blk_queue {
+	int index;
+	struct vhost_blk *blk;
+	struct vhost_virtqueue vq;
+	struct vhost_work w;
+	struct llist_head wl;
+	struct vhost_blk_req req[VHOST_BLK_VQ_MAX_REQS];
+};
+
+struct vhost_blk {
+	struct vhost_dev dev;
+	struct file *backend;
+	int num_queues;
+	struct vhost_virtqueue *vqs[VHOST_BLK_VQ_MAX];
+	struct vhost_blk_queue queue[VHOST_BLK_VQ_MAX];
+};
+
+static void vhost_blk_flush(struct vhost_blk *blk)
+{
+	int i;
+
+	for (i = 0; i < blk->num_queues; i++)
+		vhost_poll_flush(&blk->queue[i].vq.poll);
+}
+
+
+static void vhost_blk_stop(struct vhost_blk *blk)
+{
+	struct vhost_virtqueue *vq;
+	int i;
+
+	for (i = 0; i < blk->num_queues; i++) {
+		vq = &blk->queue[i].vq;
+		mutex_lock(&vq->mutex);
+		rcu_assign_pointer(vq->private_data, NULL);
+		mutex_unlock(&vq->mutex);
+	}
+}
+
+static int vhost_blk_req_done(struct vhost_blk_req *req, unsigned char status)
+{
+	int ret;
+	int len = req->len;
+
+	pr_debug("%s vq[%d] req->index %d status %d len %d\n", __func__,
+		 req->q->index, req->index, status, len);
+	ret = put_user(status, (unsigned char __user *)req->status);
+
+	WARN(ret, "%s: vq[%d] req->index %d failed to write status\n", __func__,
+	     req->q->index, req->index);
+
+	vhost_add_used(&req->q->vq, req->index, len);
+
+	return ret;
+}
+
+static void vhost_blk_io_done_work(struct vhost_work *w)
+{
+	struct vhost_blk_queue *q = container_of(w, struct vhost_blk_queue, w);
+	struct llist_node *node;
+	struct vhost_blk_req *req, *tmp;
+
+	node = llist_del_all(&q->wl);
+	llist_for_each_entry_safe(req, tmp, node, list) {
+		vhost_blk_req_done(req, req->res);
+	}
+	vhost_signal(&q->blk->dev, &q->vq);
+}
+
+static void vhost_blk_iocb_complete(struct kiocb *iocb, long ret, long ret2)
+{
+	struct vhost_blk_req *req = container_of(iocb, struct vhost_blk_req,
+						 iocb);
+
+	pr_debug("%s vq[%d] req->index %d ret %ld ret2 %ld\n", __func__,
+		 req->q->index, req->index, ret, ret2);
+
+	req->res = (ret == req->len) ? VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR;
+	llist_add(&req->list, &req->q->wl);
+	vhost_vq_work_queue(&req->q->vq, &req->q->w);
+}
+
+static int vhost_blk_req_handle(struct vhost_blk_req *req)
+{
+	struct vhost_blk *blk = req->q->blk;
+	struct vhost_virtqueue *vq = &req->q->vq;
+	int type = le32_to_cpu(req->hdr.type);
+	int ret;
+	u8 status;
+
+	if ((type == VIRTIO_BLK_T_IN) || (type == VIRTIO_BLK_T_OUT)) {
+		bool write = (type == VIRTIO_BLK_T_OUT);
+		int nr_seg = (write ? req->out_num : req->in_num) - 1;
+		unsigned long sector = le64_to_cpu(req->hdr.sector);
+		ssize_t len, rem_len;
+
+		if (!req->q->blk->backend) {
+			vq_err(vq, "blk %p no backend!\n", req->q->blk);
+			ret = -EINVAL;
+			goto out_err;
+		}
+
+		len = iov_length(&vq->iov[1], nr_seg);
+		pr_debug("%s: [pid:%d %s] %s sector %lld, len %ld\n",
+			 __func__, current->pid, current->comm,
+			 write ? "WRITE" : "READ", req->hdr.sector, len);
+
+		req->len = len;
+		rem_len = len;
+		iov_iter_init(&req->i, (write ? WRITE : READ),
+			      write ? &req->out_iov[0] : &req->in_iov[0],
+			      nr_seg, len);
+
+		req->iocb.ki_pos = sector << 9;
+		req->iocb.ki_filp = blk->backend;
+		req->iocb.ki_complete = vhost_blk_iocb_complete;
+		req->iocb.ki_flags = IOCB_DIRECT;
+
+		if (write)
+			ret = call_write_iter(blk->backend, &req->iocb,
+					      &req->i);
+		else
+			ret = call_read_iter(blk->backend, &req->iocb,
+					     &req->i);
+
+		if (ret != -EIOCBQUEUED)
+			vhost_blk_iocb_complete(&req->iocb, ret, 0);
+
+		ret = 0;
+		goto out;
+	}
+
+	if (type == VIRTIO_BLK_T_GET_ID) {
+		char s[] = "vhost_blk";
+		size_t len = min_t(size_t, req->in_iov[0].iov_len,
+				   strlen(s));
+
+		ret = copy_to_user(req->in_iov[0].iov_base, s, len);
+		status = ret ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
+		if (put_user(status, (unsigned char __user *)req->status)) {
+			ret = -EFAULT;
+			goto out_err;
+		}
+		vhost_add_used_and_signal(&blk->dev, vq, req->index, 1);
+		ret = 0;
+		goto out;
+	} else {
+		pr_warn("Unsupported request type %d\n", type);
+		vhost_discard_vq_desc(vq, 1);
+		ret = -EINVAL;
+		return ret;
+	}
+out_err:
+	vhost_discard_vq_desc(vq, 1);
+out:
+	return ret;
+}
+
+static void vhost_blk_handle_guest_kick(struct vhost_work *work)
+{
+	struct vhost_virtqueue *vq;
+	struct vhost_blk_queue *q;
+	struct vhost_blk *blk;
+	struct vhost_blk_req *req;
+	int in, out;
+	int head;
+
+	vq = container_of(work, struct vhost_virtqueue, poll.work);
+	q = container_of(vq, struct vhost_blk_queue, vq);
+	blk = container_of(vq->dev, struct vhost_blk, dev);
+
+	vhost_disable_notify(&blk->dev, vq);
+	for (;;) {
+		in = out = -1;
+
+		head = vhost_get_vq_desc(vq, vq->iov,
+					 ARRAY_SIZE(vq->iov),
+					 &out, &in, NULL, NULL);
+
+		if (head < 0)
+			break;
+
+		if (head == vq->num) {
+			if (vhost_enable_notify(&blk->dev, vq)) {
+				vhost_disable_notify(&blk->dev, vq);
+				continue;
+			}
+			break;
+		}
+
+		req = &q->req[head];
+		req->index = head;
+		req->out_num = out;
+		req->in_num = in;
+		req->out_iov = &vq->iov[1];
+		req->in_iov = &vq->iov[out];
+		req->status = vq->iov[out + in - 1].iov_base;
+
+		if (copy_from_user(&req->hdr, vq->iov[0].iov_base,
+				   sizeof(req->hdr))) {
+			vq_err(vq, "Failed to get block header!\n");
+			vhost_discard_vq_desc(vq, 1);
+			continue;
+		}
+		if (vhost_blk_req_handle(req) < 0)
+			break;
+	}
+}
+
+static int vhost_blk_open(struct inode *inode, struct file *file)
+{
+	struct vhost_blk *blk;
+	struct vhost_blk_queue *q;
+	int i, j;
+
+	blk = kvzalloc(sizeof(*blk), GFP_KERNEL);
+	if (!blk)
+		return -ENOMEM;
+
+	for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
+		q = &blk->queue[i];
+		q->index = i;
+		q->blk = blk;
+		q->vq.handle_kick = vhost_blk_handle_guest_kick;
+		vhost_work_init(&q->w, vhost_blk_io_done_work);
+		blk->vqs[i] = &q->vq;
+		for (j = 0; j < VHOST_BLK_VQ_MAX_REQS; j++) {
+			q->req[j].index = j;
+			q->req[j].q = q;
+		}
+	}
+	vhost_dev_init(&blk->dev, (struct vhost_virtqueue **)&blk->vqs,
+		       VHOST_BLK_VQ_MAX);
+	file->private_data = blk;
+
+	return 0;
+}
+
+static int vhost_blk_release(struct inode *inode, struct file *f)
+{
+	struct vhost_blk *blk = f->private_data;
+
+	vhost_blk_stop(blk);
+	mutex_lock(&blk->dev.mutex);
+	vhost_blk_flush(blk);
+	vhost_dev_stop(&blk->dev);
+	vhost_dev_cleanup(&blk->dev);
+	vhost_blk_flush(blk);
+
+	if (blk->backend) {
+		fput(blk->backend);
+		blk->backend = NULL;
+	}
+
+	mutex_unlock(&blk->dev.mutex);
+	kvfree(blk);
+
+	return 0;
+}
+
+static int vhost_blk_set_features(struct vhost_blk *blk, u64 features)
+{
+	int i;
+	int ret = -EFAULT;
+
+	mutex_lock(&blk->dev.mutex);
+	if ((features & (1 << VHOST_F_LOG_ALL)) &&
+	    !vhost_log_access_ok(&blk->dev))
+		goto out_unlock;
+
+	if ((features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {
+		if (vhost_init_device_iotlb(&blk->dev, true))
+			goto out_unlock;
+	}
+
+	for (i = 0; i < VHOST_BLK_VQ_MAX; ++i) {
+		struct vhost_virtqueue *vq = blk->vqs[i];
+
+		mutex_lock(&vq->mutex);
+		vq->acked_features = features & VHOST_BLK_FEATURES;
+		mutex_unlock(&vq->mutex);
+	}
+	ret = 0;
+out_unlock:
+	mutex_unlock(&blk->dev.mutex);
+
+	return ret;
+}
+
+static long vhost_blk_reset_owner(struct vhost_blk *blk)
+{
+	long err;
+	struct vhost_umem *umem;
+
+	mutex_lock(&blk->dev.mutex);
+	err = vhost_dev_check_owner(&blk->dev);
+	if (err)
+		goto done;
+	umem = vhost_dev_reset_owner_prepare();
+	if (!umem) {
+		err = -ENOMEM;
+		goto done;
+	}
+	vhost_blk_stop(blk);
+	vhost_blk_flush(blk);
+	vhost_dev_reset_owner(&blk->dev, umem);
+done:
+	mutex_unlock(&blk->dev.mutex);
+	return err;
+}
+
+static long vhost_blk_set_backend(struct vhost_blk *blk, int fd)
+{
+	struct file *backend;
+	int ret, i;
+	struct vhost_virtqueue *vq;
+
+	mutex_lock(&blk->dev.mutex);
+	ret = vhost_dev_check_owner(&blk->dev);
+	if (ret)
+		goto out_dev;
+
+	backend = fget(fd);
+	if (IS_ERR(backend)) {
+		ret = PTR_ERR(backend);
+		goto out_dev;
+	}
+
+	if (backend == blk->backend) {
+		ret = 0;
+		goto out_file;
+	}
+
+	if (blk->backend)
+		fput(blk->backend);
+	blk->backend = backend;
+	for (i = 0; i < blk->num_queues; i++) {
+		vq = &blk->queue[i].vq;
+		if (!vhost_vq_access_ok(vq)) {
+			ret = -EFAULT;
+			goto out_file;
+		}
+		mutex_lock(&vq->mutex);
+		rcu_assign_pointer(vq->private_data, backend);
+		ret = vhost_vq_init_access(vq);
+		mutex_unlock(&vq->mutex);
+		if (ret) {
+			pr_err("vhost_vq_init_access failed: %d\n", ret);
+			goto out_file;
+		}
+
+	}
+	ret = 0;
+	goto out_dev;
+out_file:
+	fput(backend);
+	blk->backend = NULL;
+out_dev:
+	mutex_unlock(&blk->dev.mutex);
+	vhost_blk_flush(blk);
+	return ret;
+}
+
+static long vhost_blk_pass_ioctl(struct vhost_blk *blk, unsigned int ioctl,
+				 void __user *argp)
+{
+	long ret;
+
+	mutex_lock(&blk->dev.mutex);
+	ret = vhost_dev_ioctl(&blk->dev, ioctl, argp);
+	if (ret == -ENOIOCTLCMD)
+		ret = vhost_vring_ioctl(&blk->dev, ioctl, argp);
+	else
+		vhost_blk_flush(blk);
+	mutex_unlock(&blk->dev.mutex);
+	return ret;
+}
+
+static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
+			    unsigned long arg)
+{
+	struct vhost_blk *blk = f->private_data;
+	void __user *argp = (void __user *)arg;
+	int fd;
+	u64 __user *featurep = argp;
+	u64 features;
+	long ret;
+	struct vhost_vring_state s;
+
+	switch (ioctl) {
+	case VHOST_SET_MEM_TABLE:
+		vhost_blk_stop(blk);
+		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
+		break;
+	case VHOST_SET_VRING_NUM:
+		if (copy_from_user(&s, argp, sizeof(s)))
+			return -EFAULT;
+		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
+		if (!ret)
+			blk->num_queues = s.index + 1;
+		break;
+	case VHOST_BLK_SET_BACKEND:
+		if (copy_from_user(&fd, argp, sizeof(fd)))
+			return -EFAULT;
+		ret = vhost_blk_set_backend(blk, fd);
+		break;
+	case VHOST_GET_FEATURES:
+		features = VHOST_BLK_FEATURES;
+		if (copy_to_user(featurep, &features, sizeof(features)))
+			return -EFAULT;
+		ret = 0;
+		break;
+	case VHOST_SET_FEATURES:
+		if (copy_from_user(&features, featurep, sizeof(features)))
+			return -EFAULT;
+		if (features & ~VHOST_BLK_FEATURES)
+			return -EOPNOTSUPP;
+		ret = vhost_blk_set_features(blk, features);
+		break;
+	case VHOST_RESET_OWNER:
+		ret = vhost_blk_reset_owner(blk);
+		break;
+	default:
+		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
+		break;
+	}
+	return ret;
+}
+
+static const struct file_operations vhost_blk_fops = {
+	.owner          = THIS_MODULE,
+	.open           = vhost_blk_open,
+	.release        = vhost_blk_release,
+	.llseek		= noop_llseek,
+	.unlocked_ioctl = vhost_blk_ioctl,
+};
+
+static struct miscdevice vhost_blk_misc = {
+	MISC_DYNAMIC_MINOR,
+	"vhost-blk",
+	&vhost_blk_fops,
+};
+
+static int vhost_blk_init(void)
+{
+	return misc_register(&vhost_blk_misc);
+}
+module_init(vhost_blk_init);
+
+static void vhost_blk_exit(void)
+{
+	misc_deregister(&vhost_blk_misc);
+}
+
+module_exit(vhost_blk_exit);
+
+MODULE_VERSION("1.0");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vitaly Mayatskikh");
+MODULE_DESCRIPTION("Host kernel accelerator for virtio blk");
+MODULE_ALIAS("devname:vhost-blk");
-- 
2.17.1

Re: [PATCH 1/1] Add vhost_blk driver

[Michael S. Tsirkin]

On Fri, Nov 02, 2018 at 06:21:23PM +0000, Vitaly Mayatskikh wrote:
> This driver accelerates host side of virtio-blk.
> 
> Signed-off-by: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> ---
>  drivers/vhost/Kconfig  |  13 ++
>  drivers/vhost/Makefile |   3 +
>  drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 526 insertions(+)
>  create mode 100644 drivers/vhost/blk.c
> 
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index b580885243f7..c4980d6af0ea 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -53,3 +53,16 @@ config VHOST_CROSS_ENDIAN_LEGACY
>  	  adds some overhead, it is disabled by default.
>  
>  	  If unsure, say "N".
> +
> +config VHOST_BLK
> +	tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
> +	depends on BLOCK && EVENTFD
> +	select VHOST
> +	default n
> +	help
> +	 This kernel module can be loaded in host kernel to accelerate
> +	 guest block with virtio_blk. Not to be confused with virtio_blk
> +	 module itself which needs to be loaded in guest kernel.
> +
> +	 To compile this driver as a module, choose M here: the module will
> +	 be called vhost_blk.
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index 6c6df24f770c..c8be36cd9214 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -8,6 +8,9 @@ vhost_scsi-y := scsi.o
>  obj-$(CONFIG_VHOST_VSOCK) += vhost_vsock.o
>  vhost_vsock-y := vsock.o
>  
> +obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
> +vhost_blk-y := blk.o
> +
>  obj-$(CONFIG_VHOST_RING) += vringh.o
>  
>  obj-$(CONFIG_VHOST)	+= vhost.o
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> new file mode 100644
> index 000000000000..aefb9a61fa0f
> --- /dev/null
> +++ b/drivers/vhost/blk.c
> @@ -0,0 +1,510 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 IBM Corporation
> + * Author: Vitaly Mayatskikh <v.mayatskih@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + *
> + * virtio-blk server in host kernel.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/miscdevice.h>
> +#include <linux/virtio_blk.h>
> +#include <linux/vhost.h>
> +#include <linux/fs.h>
> +#include "vhost.h"
> +
> +enum {
> +	VHOST_BLK_FEATURES =
> +	VHOST_FEATURES |
> +	(1ULL << VIRTIO_F_IOMMU_PLATFORM) |
> +	(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> +	(1ULL << VIRTIO_RING_F_EVENT_IDX) |
> +	(1ULL << VIRTIO_BLK_F_MQ)
> +};
> +
> +#define VHOST_BLK_SET_BACKEND _IOW(VHOST_VIRTIO, 0x50, int)
> +
> +enum {
> +	VHOST_BLK_VQ_MAX = 16,
> +	VHOST_BLK_VQ_MAX_REQS = 128,
> +};
> +
> +struct vhost_blk_req {
> +	struct llist_node list;
> +	int index;
> +	struct vhost_blk_queue *q;
> +	struct virtio_blk_outhdr hdr;
> +	struct iovec *out_iov;
> +	struct iovec *in_iov;
> +	u8 out_num;
> +	u8 in_num;
> +	long len;
> +	struct kiocb iocb;
> +	struct iov_iter i;
> +	int res;
> +	void __user *status;
> +};
> +
> +struct vhost_blk_queue {
> +	int index;
> +	struct vhost_blk *blk;
> +	struct vhost_virtqueue vq;
> +	struct vhost_work w;
> +	struct llist_head wl;
> +	struct vhost_blk_req req[VHOST_BLK_VQ_MAX_REQS];
> +};
> +
> +struct vhost_blk {
> +	struct vhost_dev dev;
> +	struct file *backend;
> +	int num_queues;
> +	struct vhost_virtqueue *vqs[VHOST_BLK_VQ_MAX];
> +	struct vhost_blk_queue queue[VHOST_BLK_VQ_MAX];
> +};
> +
> +static void vhost_blk_flush(struct vhost_blk *blk)
> +{
> +	int i;
> +
> +	for (i = 0; i < blk->num_queues; i++)
> +		vhost_poll_flush(&blk->queue[i].vq.poll);
> +}
> +
> +
> +static void vhost_blk_stop(struct vhost_blk *blk)
> +{
> +	struct vhost_virtqueue *vq;
> +	int i;
> +
> +	for (i = 0; i < blk->num_queues; i++) {
> +		vq = &blk->queue[i].vq;
> +		mutex_lock(&vq->mutex);
> +		rcu_assign_pointer(vq->private_data, NULL);
> +		mutex_unlock(&vq->mutex);
> +	}
> +}
> +
> +static int vhost_blk_req_done(struct vhost_blk_req *req, unsigned char status)
> +{
> +	int ret;
> +	int len = req->len;
> +
> +	pr_debug("%s vq[%d] req->index %d status %d len %d\n", __func__,
> +		 req->q->index, req->index, status, len);
> +	ret = put_user(status, (unsigned char __user *)req->status);

I'd make it u8 and not unsigned char. Also why don't you change
req->status type so you don't need a cast?

> +
> +	WARN(ret, "%s: vq[%d] req->index %d failed to write status\n", __func__,
> +	     req->q->index, req->index);

kernel warnings and debug messages that are guest-triggerable lead to
disk full errors on the host. applies elsewhere too. you want traces
instead.

> +
> +	vhost_add_used(&req->q->vq, req->index, len);


this can fail too.

> +
> +	return ret;
> +}
> +
> +static void vhost_blk_io_done_work(struct vhost_work *w)
> +{
> +	struct vhost_blk_queue *q = container_of(w, struct vhost_blk_queue, w);
> +	struct llist_node *node;
> +	struct vhost_blk_req *req, *tmp;
> +
> +	node = llist_del_all(&q->wl);
> +	llist_for_each_entry_safe(req, tmp, node, list) {
> +		vhost_blk_req_done(req, req->res);
> +	}
> +	vhost_signal(&q->blk->dev, &q->vq);
> +}
> +
> +static void vhost_blk_iocb_complete(struct kiocb *iocb, long ret, long ret2)
> +{
> +	struct vhost_blk_req *req = container_of(iocb, struct vhost_blk_req,
> +						 iocb);
> +
> +	pr_debug("%s vq[%d] req->index %d ret %ld ret2 %ld\n", __func__,
> +		 req->q->index, req->index, ret, ret2);
> +
> +	req->res = (ret == req->len) ? VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR;
> +	llist_add(&req->list, &req->q->wl);
> +	vhost_vq_work_queue(&req->q->vq, &req->q->w);
> +}
> +
> +static int vhost_blk_req_handle(struct vhost_blk_req *req)
> +{
> +	struct vhost_blk *blk = req->q->blk;
> +	struct vhost_virtqueue *vq = &req->q->vq;
> +	int type = le32_to_cpu(req->hdr.type);
> +	int ret;
> +	u8 status;
> +
> +	if ((type == VIRTIO_BLK_T_IN) || (type == VIRTIO_BLK_T_OUT)) {
> +		bool write = (type == VIRTIO_BLK_T_OUT);
> +		int nr_seg = (write ? req->out_num : req->in_num) - 1;
> +		unsigned long sector = le64_to_cpu(req->hdr.sector);
> +		ssize_t len, rem_len;
> +
> +		if (!req->q->blk->backend) {
> +			vq_err(vq, "blk %p no backend!\n", req->q->blk);
> +			ret = -EINVAL;
> +			goto out_err;
> +		}
> +
> +		len = iov_length(&vq->iov[1], nr_seg);
> +		pr_debug("%s: [pid:%d %s] %s sector %lld, len %ld\n",
> +			 __func__, current->pid, current->comm,
> +			 write ? "WRITE" : "READ", req->hdr.sector, len);
> +
> +		req->len = len;
> +		rem_len = len;
> +		iov_iter_init(&req->i, (write ? WRITE : READ),
> +			      write ? &req->out_iov[0] : &req->in_iov[0],
> +			      nr_seg, len);
> +
> +		req->iocb.ki_pos = sector << 9;
> +		req->iocb.ki_filp = blk->backend;
> +		req->iocb.ki_complete = vhost_blk_iocb_complete;
> +		req->iocb.ki_flags = IOCB_DIRECT;
> +
> +		if (write)
> +			ret = call_write_iter(blk->backend, &req->iocb,
> +					      &req->i);
> +		else
> +			ret = call_read_iter(blk->backend, &req->iocb,
> +					     &req->i);
> +
> +		if (ret != -EIOCBQUEUED)
> +			vhost_blk_iocb_complete(&req->iocb, ret, 0);
> +
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	if (type == VIRTIO_BLK_T_GET_ID) {
> +		char s[] = "vhost_blk";

Isn't this supposed to return the serial #?

> +		size_t len = min_t(size_t, req->in_iov[0].iov_len,
> +				   strlen(s));
> +
> +		ret = copy_to_user(req->in_iov[0].iov_base, s, len);

I don't think we should assume there's no scatter list here.

> +		status = ret ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> +		if (put_user(status, (unsigned char __user *)req->status)) {
> +			ret = -EFAULT;
> +			goto out_err;
> +		}
> +		vhost_add_used_and_signal(&blk->dev, vq, req->index, 1);
> +		ret = 0;
> +		goto out;
> +	} else {
> +		pr_warn("Unsupported request type %d\n", type);
> +		vhost_discard_vq_desc(vq, 1);
> +		ret = -EINVAL;
> +		return ret;
> +	}
> +out_err:
> +	vhost_discard_vq_desc(vq, 1);
> +out:
> +	return ret;
> +}
> +
> +static void vhost_blk_handle_guest_kick(struct vhost_work *work)
> +{
> +	struct vhost_virtqueue *vq;
> +	struct vhost_blk_queue *q;
> +	struct vhost_blk *blk;
> +	struct vhost_blk_req *req;
> +	int in, out;
> +	int head;
> +
> +	vq = container_of(work, struct vhost_virtqueue, poll.work);
> +	q = container_of(vq, struct vhost_blk_queue, vq);
> +	blk = container_of(vq->dev, struct vhost_blk, dev);
> +
> +	vhost_disable_notify(&blk->dev, vq);
> +	for (;;) {
> +		in = out = -1;
> +
> +		head = vhost_get_vq_desc(vq, vq->iov,
> +					 ARRAY_SIZE(vq->iov),
> +					 &out, &in, NULL, NULL);
> +
> +		if (head < 0)
> +			break;
> +
> +		if (head == vq->num) {
> +			if (vhost_enable_notify(&blk->dev, vq)) {
> +				vhost_disable_notify(&blk->dev, vq);
> +				continue;
> +			}
> +			break;
> +		}
> +
> +		req = &q->req[head];
> +		req->index = head;
> +		req->out_num = out;
> +		req->in_num = in;
> +		req->out_iov = &vq->iov[1];
> +		req->in_iov = &vq->iov[out];
> +		req->status = vq->iov[out + in - 1].iov_base;

Shouldn't we validate that there's actually an in?

> +
> +		if (copy_from_user(&req->hdr, vq->iov[0].iov_base,
> +				   sizeof(req->hdr))) {
> +			vq_err(vq, "Failed to get block header!\n");
> +			vhost_discard_vq_desc(vq, 1);
> +			continue;
> +		}

It's better to avoid assuming that header is in a single iov entry,
use an iterator.

> +		if (vhost_blk_req_handle(req) < 0)
> +			break;
> +	}
> +}
> +
> +static int vhost_blk_open(struct inode *inode, struct file *file)
> +{
> +	struct vhost_blk *blk;
> +	struct vhost_blk_queue *q;
> +	int i, j;
> +
> +	blk = kvzalloc(sizeof(*blk), GFP_KERNEL);
> +	if (!blk)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
> +		q = &blk->queue[i];
> +		q->index = i;
> +		q->blk = blk;
> +		q->vq.handle_kick = vhost_blk_handle_guest_kick;
> +		vhost_work_init(&q->w, vhost_blk_io_done_work);
> +		blk->vqs[i] = &q->vq;
> +		for (j = 0; j < VHOST_BLK_VQ_MAX_REQS; j++) {
> +			q->req[j].index = j;
> +			q->req[j].q = q;
> +		}
> +	}
> +	vhost_dev_init(&blk->dev, (struct vhost_virtqueue **)&blk->vqs,
> +		       VHOST_BLK_VQ_MAX);
> +	file->private_data = blk;
> +
> +	return 0;
> +}
> +
> +static int vhost_blk_release(struct inode *inode, struct file *f)
> +{
> +	struct vhost_blk *blk = f->private_data;
> +
> +	vhost_blk_stop(blk);
> +	mutex_lock(&blk->dev.mutex);
> +	vhost_blk_flush(blk);
> +	vhost_dev_stop(&blk->dev);
> +	vhost_dev_cleanup(&blk->dev);
> +	vhost_blk_flush(blk);
> +
> +	if (blk->backend) {
> +		fput(blk->backend);
> +		blk->backend = NULL;
> +	}
> +
> +	mutex_unlock(&blk->dev.mutex);
> +	kvfree(blk);
> +
> +	return 0;
> +}
> +
> +static int vhost_blk_set_features(struct vhost_blk *blk, u64 features)
> +{
> +	int i;
> +	int ret = -EFAULT;
> +
> +	mutex_lock(&blk->dev.mutex);
> +	if ((features & (1 << VHOST_F_LOG_ALL)) &&
> +	    !vhost_log_access_ok(&blk->dev))
> +		goto out_unlock;
> +
> +	if ((features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) {
> +		if (vhost_init_device_iotlb(&blk->dev, true))
> +			goto out_unlock;
> +	}
> +
> +	for (i = 0; i < VHOST_BLK_VQ_MAX; ++i) {
> +		struct vhost_virtqueue *vq = blk->vqs[i];
> +
> +		mutex_lock(&vq->mutex);
> +		vq->acked_features = features & VHOST_BLK_FEATURES;
> +		mutex_unlock(&vq->mutex);
> +	}
> +	ret = 0;
> +out_unlock:
> +	mutex_unlock(&blk->dev.mutex);
> +
> +	return ret;
> +}
> +
> +static long vhost_blk_reset_owner(struct vhost_blk *blk)
> +{
> +	long err;
> +	struct vhost_umem *umem;
> +
> +	mutex_lock(&blk->dev.mutex);
> +	err = vhost_dev_check_owner(&blk->dev);
> +	if (err)
> +		goto done;
> +	umem = vhost_dev_reset_owner_prepare();
> +	if (!umem) {
> +		err = -ENOMEM;
> +		goto done;
> +	}
> +	vhost_blk_stop(blk);
> +	vhost_blk_flush(blk);
> +	vhost_dev_reset_owner(&blk->dev, umem);
> +done:
> +	mutex_unlock(&blk->dev.mutex);
> +	return err;
> +}
> +
> +static long vhost_blk_set_backend(struct vhost_blk *blk, int fd)
> +{
> +	struct file *backend;
> +	int ret, i;
> +	struct vhost_virtqueue *vq;
> +
> +	mutex_lock(&blk->dev.mutex);
> +	ret = vhost_dev_check_owner(&blk->dev);
> +	if (ret)
> +		goto out_dev;
> +
> +	backend = fget(fd);
> +	if (IS_ERR(backend)) {
> +		ret = PTR_ERR(backend);
> +		goto out_dev;
> +	}
> +
> +	if (backend == blk->backend) {
> +		ret = 0;
> +		goto out_file;
> +	}
> +
> +	if (blk->backend)
> +		fput(blk->backend);
> +	blk->backend = backend;
> +	for (i = 0; i < blk->num_queues; i++) {
> +		vq = &blk->queue[i].vq;
> +		if (!vhost_vq_access_ok(vq)) {
> +			ret = -EFAULT;
> +			goto out_file;
> +		}
> +		mutex_lock(&vq->mutex);
> +		rcu_assign_pointer(vq->private_data, backend);
> +		ret = vhost_vq_init_access(vq);
> +		mutex_unlock(&vq->mutex);
> +		if (ret) {
> +			pr_err("vhost_vq_init_access failed: %d\n", ret);
> +			goto out_file;
> +		}
> +
> +	}
> +	ret = 0;
> +	goto out_dev;
> +out_file:
> +	fput(backend);
> +	blk->backend = NULL;
> +out_dev:
> +	mutex_unlock(&blk->dev.mutex);
> +	vhost_blk_flush(blk);
> +	return ret;
> +}
> +
> +static long vhost_blk_pass_ioctl(struct vhost_blk *blk, unsigned int ioctl,
> +				 void __user *argp)
> +{
> +	long ret;
> +
> +	mutex_lock(&blk->dev.mutex);
> +	ret = vhost_dev_ioctl(&blk->dev, ioctl, argp);
> +	if (ret == -ENOIOCTLCMD)
> +		ret = vhost_vring_ioctl(&blk->dev, ioctl, argp);
> +	else
> +		vhost_blk_flush(blk);
> +	mutex_unlock(&blk->dev.mutex);
> +	return ret;
> +}
> +
> +static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
> +			    unsigned long arg)
> +{
> +	struct vhost_blk *blk = f->private_data;
> +	void __user *argp = (void __user *)arg;
> +	int fd;
> +	u64 __user *featurep = argp;
> +	u64 features;
> +	long ret;
> +	struct vhost_vring_state s;
> +
> +	switch (ioctl) {
> +	case VHOST_SET_MEM_TABLE:
> +		vhost_blk_stop(blk);
> +		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
> +		break;
> +	case VHOST_SET_VRING_NUM:
> +		if (copy_from_user(&s, argp, sizeof(s)))
> +			return -EFAULT;
> +		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
> +		if (!ret)
> +			blk->num_queues = s.index + 1;
> +		break;
> +	case VHOST_BLK_SET_BACKEND:
> +		if (copy_from_user(&fd, argp, sizeof(fd)))
> +			return -EFAULT;
> +		ret = vhost_blk_set_backend(blk, fd);
> +		break;
> +	case VHOST_GET_FEATURES:
> +		features = VHOST_BLK_FEATURES;
> +		if (copy_to_user(featurep, &features, sizeof(features)))
> +			return -EFAULT;
> +		ret = 0;
> +		break;
> +	case VHOST_SET_FEATURES:
> +		if (copy_from_user(&features, featurep, sizeof(features)))
> +			return -EFAULT;
> +		if (features & ~VHOST_BLK_FEATURES)
> +			return -EOPNOTSUPP;
> +		ret = vhost_blk_set_features(blk, features);
> +		break;
> +	case VHOST_RESET_OWNER:
> +		ret = vhost_blk_reset_owner(blk);
> +		break;
> +	default:
> +		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
> +		break;
> +	}
> +	return ret;
> +}
> +
> +static const struct file_operations vhost_blk_fops = {
> +	.owner          = THIS_MODULE,
> +	.open           = vhost_blk_open,
> +	.release        = vhost_blk_release,
> +	.llseek		= noop_llseek,
> +	.unlocked_ioctl = vhost_blk_ioctl,
> +};
> +
> +static struct miscdevice vhost_blk_misc = {
> +	MISC_DYNAMIC_MINOR,
> +	"vhost-blk",
> +	&vhost_blk_fops,
> +};
> +
> +static int vhost_blk_init(void)
> +{
> +	return misc_register(&vhost_blk_misc);
> +}
> +module_init(vhost_blk_init);
> +
> +static void vhost_blk_exit(void)
> +{
> +	misc_deregister(&vhost_blk_misc);
> +}
> +
> +module_exit(vhost_blk_exit);
> +
> +MODULE_VERSION("1.0");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Vitaly Mayatskikh");
> +MODULE_DESCRIPTION("Host kernel accelerator for virtio blk");
> +MODULE_ALIAS("devname:vhost-blk");
> -- 
> 2.17.1

Re: [PATCH 1/1] Add vhost_blk driver

[Vitaly Mayatskih]

On Fri, Nov 2, 2018 at 2:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> > +     if (type == VIRTIO_BLK_T_GET_ID) {
> > +             char s[] = "vhost_blk";
>
> Isn't this supposed to return the serial #?

Yes, that gets a bit tricky here... Disk serial number is specified in
QEMU command line parameters, so it needs to be passed to vhost_blk
when qemu attaches the disk backend. That can be done (now? in a
following incremental patch?).
Also other vhost work does the same with GET_ID, if that's matter.

I'll fix the rest, thanks for review!

-- 
wbr, Vitaly

Re: [PATCH 1/1] Add vhost_blk driver

[Michael S. Tsirkin]

On Fri, Nov 02, 2018 at 03:24:13PM -0400, Vitaly Mayatskih wrote:
> On Fri, Nov 2, 2018 at 2:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > > +     if (type == VIRTIO_BLK_T_GET_ID) {
> > > +             char s[] = "vhost_blk";
> >
> > Isn't this supposed to return the serial #?
> 
> Yes, that gets a bit tricky here... Disk serial number is specified in
> QEMU command line parameters, so it needs to be passed to vhost_blk
> when qemu attaches the disk backend. That can be done (now? in a
> following incremental patch?).

I'add an ioctl now.

> Also other vhost work does the same with GET_ID, if that's matter.
> 
> I'll fix the rest, thanks for review!
> 
> -- 
> wbr, Vitaly

Re: [PATCH 1/1] Add vhost_blk driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5345 bytes --]

Hi Vitaly,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on vhost/linux-next]
[also build test ERROR on v4.19 next-20181102]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vitaly-Mayatskikh/vhost-add-vhost_blk-driver/20181103-084141
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=mips 

All error/warnings (new ones prefixed by >>):

   drivers/vhost/blk.c: In function 'vhost_blk_iocb_complete':
>> drivers/vhost/blk.c:129:2: error: implicit declaration of function 'vhost_vq_work_queue'; did you mean 'vhost_work_queue'? [-Werror=implicit-function-declaration]
     vhost_vq_work_queue(&req->q->vq, &req->q->w);
     ^~~~~~~~~~~~~~~~~~~
     vhost_work_queue
   In file included from include/linux/kernel.h:14:0,
                    from include/linux/list.h:9,
                    from include/linux/module.h:9,
                    from drivers/vhost/blk.c:11:
   drivers/vhost/blk.c: In function 'vhost_blk_req_handle':
>> drivers/vhost/blk.c:153:12: warning: format '%ld' expects argument of type 'long int', but argument 8 has type 'ssize_t {aka int}' [-Wformat=]
      pr_debug("%s: [pid:%d %s] %s sector %lld, len %ld\n",
               ^
   include/linux/printk.h:292:21: note: in definition of macro 'pr_fmt'
    #define pr_fmt(fmt) fmt
                        ^~~
   include/linux/printk.h:340:2: note: in expansion of macro 'dynamic_pr_debug'
     dynamic_pr_debug(fmt, ##__VA_ARGS__)
     ^~~~~~~~~~~~~~~~
>> drivers/vhost/blk.c:153:3: note: in expansion of macro 'pr_debug'
      pr_debug("%s: [pid:%d %s] %s sector %lld, len %ld\n",
      ^~~~~~~~
   cc1: some warnings being treated as errors

vim +129 drivers/vhost/blk.c

   118	
   119	static void vhost_blk_iocb_complete(struct kiocb *iocb, long ret, long ret2)
   120	{
   121		struct vhost_blk_req *req = container_of(iocb, struct vhost_blk_req,
   122							 iocb);
   123	
   124		pr_debug("%s vq[%d] req->index %d ret %ld ret2 %ld\n", __func__,
   125			 req->q->index, req->index, ret, ret2);
   126	
   127		req->res = (ret == req->len) ? VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR;
   128		llist_add(&req->list, &req->q->wl);
 > 129		vhost_vq_work_queue(&req->q->vq, &req->q->w);
   130	}
   131	
   132	static int vhost_blk_req_handle(struct vhost_blk_req *req)
   133	{
   134		struct vhost_blk *blk = req->q->blk;
   135		struct vhost_virtqueue *vq = &req->q->vq;
   136		int type = le32_to_cpu(req->hdr.type);
   137		int ret;
   138		u8 status;
   139	
   140		if ((type == VIRTIO_BLK_T_IN) || (type == VIRTIO_BLK_T_OUT)) {
   141			bool write = (type == VIRTIO_BLK_T_OUT);
   142			int nr_seg = (write ? req->out_num : req->in_num) - 1;
   143			unsigned long sector = le64_to_cpu(req->hdr.sector);
   144			ssize_t len, rem_len;
   145	
   146			if (!req->q->blk->backend) {
   147				vq_err(vq, "blk %p no backend!\n", req->q->blk);
   148				ret = -EINVAL;
   149				goto out_err;
   150			}
   151	
   152			len = iov_length(&vq->iov[1], nr_seg);
 > 153			pr_debug("%s: [pid:%d %s] %s sector %lld, len %ld\n",
   154				 __func__, current->pid, current->comm,
   155				 write ? "WRITE" : "READ", req->hdr.sector, len);
   156	
   157			req->len = len;
   158			rem_len = len;
   159			iov_iter_init(&req->i, (write ? WRITE : READ),
   160				      write ? &req->out_iov[0] : &req->in_iov[0],
   161				      nr_seg, len);
   162	
   163			req->iocb.ki_pos = sector << 9;
   164			req->iocb.ki_filp = blk->backend;
   165			req->iocb.ki_complete = vhost_blk_iocb_complete;
   166			req->iocb.ki_flags = IOCB_DIRECT;
   167	
   168			if (write)
   169				ret = call_write_iter(blk->backend, &req->iocb,
   170						      &req->i);
   171			else
   172				ret = call_read_iter(blk->backend, &req->iocb,
   173						     &req->i);
   174	
   175			if (ret != -EIOCBQUEUED)
   176				vhost_blk_iocb_complete(&req->iocb, ret, 0);
   177	
   178			ret = 0;
   179			goto out;
   180		}
   181	
   182		if (type == VIRTIO_BLK_T_GET_ID) {
   183			char s[] = "vhost_blk";
   184			size_t len = min_t(size_t, req->in_iov[0].iov_len,
   185					   strlen(s));
   186	
   187			ret = copy_to_user(req->in_iov[0].iov_base, s, len);
   188			status = ret ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
   189			if (put_user(status, (unsigned char __user *)req->status)) {
   190				ret = -EFAULT;
   191				goto out_err;
   192			}
   193			vhost_add_used_and_signal(&blk->dev, vq, req->index, 1);
   194			ret = 0;
   195			goto out;
   196		} else {
   197			pr_warn("Unsupported request type %d\n", type);
   198			vhost_discard_vq_desc(vq, 1);
   199			ret = -EINVAL;
   200			return ret;
   201		}
   202	out_err:
   203		vhost_discard_vq_desc(vq, 1);
   204	out:
   205		return ret;
   206	}
   207	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57563 bytes --]

Re: [PATCH 1/1] Add vhost_blk driver

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3004 bytes --]

On Fri, Nov 02, 2018 at 06:21:23PM +0000, Vitaly Mayatskikh wrote:
> This driver accelerates host side of virtio-blk.

Did you look at vhost-user-blk?  It does things slightly differently:
more of the virtio-blk device model is handled by the vhost-user device
(e.g. config space).  That might be necessary to implement
virtio_blk_config.writeback properly.

> +#define VHOST_BLK_SET_BACKEND _IOW(VHOST_VIRTIO, 0x50, int)

Needs to be in the uapi header so userspace can use it.

> +
> +enum {
> +	VHOST_BLK_VQ_MAX = 16,
> +	VHOST_BLK_VQ_MAX_REQS = 128,
> +};

These limits seem arbitrary and probably too low.

> +
> +struct vhost_blk_req {
> +	struct llist_node list;
> +	int index;
> +	struct vhost_blk_queue *q;
> +	struct virtio_blk_outhdr hdr;
> +	struct iovec *out_iov;
> +	struct iovec *in_iov;
> +	u8 out_num;
> +	u8 in_num;
> +	long len;
> +	struct kiocb iocb;
> +	struct iov_iter i;
> +	int res;
> +	void __user *status;
> +};
> +
> +struct vhost_blk_queue {
> +	int index;
> +	struct vhost_blk *blk;
> +	struct vhost_virtqueue vq;
> +	struct vhost_work w;
> +	struct llist_head wl;

What is this?  Please use clear names and comments. :)

> +static int vhost_blk_req_handle(struct vhost_blk_req *req)
> +{
> +	struct vhost_blk *blk = req->q->blk;
> +	struct vhost_virtqueue *vq = &req->q->vq;
> +	int type = le32_to_cpu(req->hdr.type);
> +	int ret;
> +	u8 status;
> +
> +	if ((type == VIRTIO_BLK_T_IN) || (type == VIRTIO_BLK_T_OUT)) {
> +		bool write = (type == VIRTIO_BLK_T_OUT);
> +		int nr_seg = (write ? req->out_num : req->in_num) - 1;
> +		unsigned long sector = le64_to_cpu(req->hdr.sector);

Using little-endian instead of the virtio types means that only VIRTIO
1.0 modern devices are supported (older devices may not be
little-endian!).  In that case you'd need to put the VIRTIO_1 feature
bit into the features mask.

> +		req = &q->req[head];
> +		req->index = head;
> +		req->out_num = out;
> +		req->in_num = in;
> +		req->out_iov = &vq->iov[1];
> +		req->in_iov = &vq->iov[out];
> +		req->status = vq->iov[out + in - 1].iov_base;

The VIRTIO spec explicitly requires that devices support arbitrary
descriptor layouts, so you cannot assume a particular iov[] layout.

> +static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
> +			    unsigned long arg)
> +{
> +	struct vhost_blk *blk = f->private_data;
> +	void __user *argp = (void __user *)arg;
> +	int fd;
> +	u64 __user *featurep = argp;
> +	u64 features;
> +	long ret;
> +	struct vhost_vring_state s;
> +
> +	switch (ioctl) {
> +	case VHOST_SET_MEM_TABLE:
> +		vhost_blk_stop(blk);
> +		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
> +		break;

Why is this necessary?  Existing vhost devices pass the ioctl through
without an explicit case for it.

> +	case VHOST_SET_VRING_NUM:
> +		if (copy_from_user(&s, argp, sizeof(s)))
> +			return -EFAULT;
> +		ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
> +		if (!ret)
> +			blk->num_queues = s.index + 1;

Where is this input checked against ARRAY_SIZE(blk->queue)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 1/1] Add vhost_blk driver

[Vitaly Mayatskih]

On Tue, Nov 6, 2018 at 11:03 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> Did you look at vhost-user-blk?  It does things slightly differently:
> more of the virtio-blk device model is handled by the vhost-user device
> (e.g. config space).  That might be necessary to implement
> virtio_blk_config.writeback properly.

Yes, vhost-user-blk was used as a template.

> > +enum {
> > +     VHOST_BLK_VQ_MAX = 16,
> > +     VHOST_BLK_VQ_MAX_REQS = 128,
> > +};
>
> These limits seem arbitrary and probably too low.

It fits cache and TLB, since the data structures are statically
allocated. I saw a worse performance with bigger max-reqs. I'll make
it configurable.

> > +     if ((type == VIRTIO_BLK_T_IN) || (type == VIRTIO_BLK_T_OUT)) {
> > +             bool write = (type == VIRTIO_BLK_T_OUT);
> > +             int nr_seg = (write ? req->out_num : req->in_num) - 1;
> > +             unsigned long sector = le64_to_cpu(req->hdr.sector);
>
> Using little-endian instead of the virtio types means that only VIRTIO
> 1.0 modern devices are supported (older devices may not be
> little-endian!).  In that case you'd need to put the VIRTIO_1 feature
> bit into the features mask.

Yeah, I'm making first baby steps in virtio ;) Thanks!

> > +     switch (ioctl) {
> > +     case VHOST_SET_MEM_TABLE:
> > +             vhost_blk_stop(blk);
> > +             ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
> > +             break;
>
> Why is this necessary?  Existing vhost devices pass the ioctl through
> without an explicit case for it.

vq->private_data is populated, vhost_set_vring_num returns -EBUSY if
ioctl is passed as is. It can be a bug in vhost, too, but I don't have
enough knowledge.

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index aefb9a61fa0f..da3eb041a975 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -438,7 +438,7 @@ static long vhost_blk_ioctl(struct file *f,
unsigned int ioctl,

        switch (ioctl) {
        case VHOST_SET_MEM_TABLE:
-               vhost_blk_stop(blk);
+//             vhost_blk_stop(blk);
                ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
                break;
        case VHOST_SET_VRING_NUM:

./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -cpu host -smp 4 -m
2G -vnc 192.168.122.104:5 --drive
if=none,id=drive0,format=raw,file=/dev/vde -device
vhost-blk-pci,id=blk0,drive=drive0,num-queues=4
qemu-system-x86_64: vhost_set_vring_num failed: Device or resource busy (16)
qemu-system-x86_64: Error starting vhost: 16

> > +     case VHOST_SET_VRING_NUM:
> > +             if (copy_from_user(&s, argp, sizeof(s)))
> > +                     return -EFAULT;
> > +             ret = vhost_blk_pass_ioctl(blk, ioctl, argp);
> > +             if (!ret)
> > +                     blk->num_queues = s.index + 1;
>
> Where is this input checked against ARRAY_SIZE(blk->queue)?

In vhost itself. I capture the parameter only if vhost ioctl completes
without errors. Perhaps, worth a comment.

Thanks for review, this is exactly what I hoping for!

--
wbr, Vitaly

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Michael S. Tsirkin]

On Fri, Nov 02, 2018 at 06:21:22PM +0000, Vitaly Mayatskikh wrote:
> vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
> driver allows VM to reach a near bare-metal disk performance. See IOPS
> numbers below (fio --rw=randread --bs=4k).
> 
> This implementation uses kiocb interface. It is slightly slower than
> going directly through bio, but is simpler and also works with disk
> images placed on a file system.
> 
> # fio num-jobs
> # A: bare metal over block
> # B: bare metal over file
> # C: virtio-blk over block
> # D: virtio-blk over file
> # E: vhost-blk bio over block
> # F: vhost-blk kiocb over block
> # G: vhost-blk kiocb over file
> #
> #  A     B     C    D    E     F     G
> 
> 1  171k  151k  148k 151k 195k  187k  175k
> 2  328k  302k  249k 241k 349k  334k  296k
> 3  479k  437k  179k 174k 501k  464k  404k
> 4  622k  568k  143k 183k 620k  580k  492k
> 5  755k  697k  136k 128k 737k  693k  579k
> 6  887k  808k  131k 120k 830k  782k  640k
> 7  1004k 926k  126k 131k 926k  863k  693k
> 8  1099k 1015k 117k 115k 1001k 931k  712k
> 9  1194k 1119k 115k 111k 1055k 991k  711k
> 10 1278k 1207k 109k 114k 1130k 1046k 695k
> 11 1345k 1280k 110k 108k 1119k 1091k 663k
> 12 1411k 1356k 104k 106k 1201k 1142k 629k
> 13 1466k 1423k 106k 106k 1260k 1170k 607k
> 14 1517k 1486k 103k 106k 1296k 1179k 589k
> 15 1552k 1543k 102k 102k 1322k 1191k 571k
> 16 1480k 1506k 101k 102k 1346k 1202k 566k
> 
> Vitaly Mayatskikh (1):
>   Add vhost_blk driver


Thanks!
Before merging this, I'd like to get some acks from userspace that it's
actually going to be used - e.g. QEMU block maintainers.

>  drivers/vhost/Kconfig  |  13 ++
>  drivers/vhost/Makefile |   3 +
>  drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 526 insertions(+)
>  create mode 100644 drivers/vhost/blk.c
> 
> -- 
> 2.17.1

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskih]

Sure! It needs a new QEMU driver (vhost-blk-pci), I'm preparing it to
be sent out.
On Fri, Nov 2, 2018 at 2:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Nov 02, 2018 at 06:21:22PM +0000, Vitaly Mayatskikh wrote:
> > vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
> > driver allows VM to reach a near bare-metal disk performance. See IOPS
> > numbers below (fio --rw=randread --bs=4k).
> >
> > This implementation uses kiocb interface. It is slightly slower than
> > going directly through bio, but is simpler and also works with disk
> > images placed on a file system.
> >
> > # fio num-jobs
> > # A: bare metal over block
> > # B: bare metal over file
> > # C: virtio-blk over block
> > # D: virtio-blk over file
> > # E: vhost-blk bio over block
> > # F: vhost-blk kiocb over block
> > # G: vhost-blk kiocb over file
> > #
> > #  A     B     C    D    E     F     G
> >
> > 1  171k  151k  148k 151k 195k  187k  175k
> > 2  328k  302k  249k 241k 349k  334k  296k
> > 3  479k  437k  179k 174k 501k  464k  404k
> > 4  622k  568k  143k 183k 620k  580k  492k
> > 5  755k  697k  136k 128k 737k  693k  579k
> > 6  887k  808k  131k 120k 830k  782k  640k
> > 7  1004k 926k  126k 131k 926k  863k  693k
> > 8  1099k 1015k 117k 115k 1001k 931k  712k
> > 9  1194k 1119k 115k 111k 1055k 991k  711k
> > 10 1278k 1207k 109k 114k 1130k 1046k 695k
> > 11 1345k 1280k 110k 108k 1119k 1091k 663k
> > 12 1411k 1356k 104k 106k 1201k 1142k 629k
> > 13 1466k 1423k 106k 106k 1260k 1170k 607k
> > 14 1517k 1486k 103k 106k 1296k 1179k 589k
> > 15 1552k 1543k 102k 102k 1322k 1191k 571k
> > 16 1480k 1506k 101k 102k 1346k 1202k 566k
> >
> > Vitaly Mayatskikh (1):
> >   Add vhost_blk driver
>
>
> Thanks!
> Before merging this, I'd like to get some acks from userspace that it's
> actually going to be used - e.g. QEMU block maintainers.
>
> >  drivers/vhost/Kconfig  |  13 ++
> >  drivers/vhost/Makefile |   3 +
> >  drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 526 insertions(+)
> >  create mode 100644 drivers/vhost/blk.c
> >
> > --
> > 2.17.1



-- 
wbr, Vitaly

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Christian Borntraeger]

On 11/02/2018 07:26 PM, Michael S. Tsirkin wrote:
> On Fri, Nov 02, 2018 at 06:21:22PM +0000, Vitaly Mayatskikh wrote:
>> vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
>> driver allows VM to reach a near bare-metal disk performance. See IOPS
>> numbers below (fio --rw=randread --bs=4k).
>>
>> This implementation uses kiocb interface. It is slightly slower than
>> going directly through bio, but is simpler and also works with disk
>> images placed on a file system.


This should also work with other transports like virtio-ccw (instead of virtio-pci).
Correct?

>>
>> # fio num-jobs
>> # A: bare metal over block
>> # B: bare metal over file
>> # C: virtio-blk over block
>> # D: virtio-blk over file
>> # E: vhost-blk bio over block
>> # F: vhost-blk kiocb over block
>> # G: vhost-blk kiocb over file
>> #
>> #  A     B     C    D    E     F     G
>>
>> 1  171k  151k  148k 151k 195k  187k  175k
>> 2  328k  302k  249k 241k 349k  334k  296k
>> 3  479k  437k  179k 174k 501k  464k  404k
>> 4  622k  568k  143k 183k 620k  580k  492k
>> 5  755k  697k  136k 128k 737k  693k  579k
>> 6  887k  808k  131k 120k 830k  782k  640k
>> 7  1004k 926k  126k 131k 926k  863k  693k
>> 8  1099k 1015k 117k 115k 1001k 931k  712k
>> 9  1194k 1119k 115k 111k 1055k 991k  711k
>> 10 1278k 1207k 109k 114k 1130k 1046k 695k
>> 11 1345k 1280k 110k 108k 1119k 1091k 663k
>> 12 1411k 1356k 104k 106k 1201k 1142k 629k
>> 13 1466k 1423k 106k 106k 1260k 1170k 607k
>> 14 1517k 1486k 103k 106k 1296k 1179k 589k
>> 15 1552k 1543k 102k 102k 1322k 1191k 571k
>> 16 1480k 1506k 101k 102k 1346k 1202k 566k
>>
>> Vitaly Mayatskikh (1):
>>   Add vhost_blk driver
> 
> 
> Thanks!
> Before merging this, I'd like to get some acks from userspace that it's
> actually going to be used - e.g. QEMU block maintainers.
> 
>>  drivers/vhost/Kconfig  |  13 ++
>>  drivers/vhost/Makefile |   3 +
>>  drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 526 insertions(+)
>>  create mode 100644 drivers/vhost/blk.c
>>
>> -- 
>> 2.17.1
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskih]

> This should also work with other transports like virtio-ccw (instead of virtio-pci).
> Correct?

I guess it can. There's nothing in the code specifically asking for or
depending on PCI.

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

On Fri, Nov 02, 2018 at 02:26:00PM -0400, Michael S. Tsirkin wrote:
> On Fri, Nov 02, 2018 at 06:21:22PM +0000, Vitaly Mayatskikh wrote:
> > vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
> > driver allows VM to reach a near bare-metal disk performance. See IOPS
> > numbers below (fio --rw=randread --bs=4k).
> > 
> > This implementation uses kiocb interface. It is slightly slower than
> > going directly through bio, but is simpler and also works with disk
> > images placed on a file system.
> > 
> > # fio num-jobs
> > # A: bare metal over block
> > # B: bare metal over file
> > # C: virtio-blk over block
> > # D: virtio-blk over file
> > # E: vhost-blk bio over block
> > # F: vhost-blk kiocb over block
> > # G: vhost-blk kiocb over file
> > #
> > #  A     B     C    D    E     F     G
> > 
> > 1  171k  151k  148k 151k 195k  187k  175k
> > 2  328k  302k  249k 241k 349k  334k  296k
> > 3  479k  437k  179k 174k 501k  464k  404k
> > 4  622k  568k  143k 183k 620k  580k  492k
> > 5  755k  697k  136k 128k 737k  693k  579k
> > 6  887k  808k  131k 120k 830k  782k  640k
> > 7  1004k 926k  126k 131k 926k  863k  693k
> > 8  1099k 1015k 117k 115k 1001k 931k  712k
> > 9  1194k 1119k 115k 111k 1055k 991k  711k
> > 10 1278k 1207k 109k 114k 1130k 1046k 695k
> > 11 1345k 1280k 110k 108k 1119k 1091k 663k
> > 12 1411k 1356k 104k 106k 1201k 1142k 629k
> > 13 1466k 1423k 106k 106k 1260k 1170k 607k
> > 14 1517k 1486k 103k 106k 1296k 1179k 589k
> > 15 1552k 1543k 102k 102k 1322k 1191k 571k
> > 16 1480k 1506k 101k 102k 1346k 1202k 566k
> > 
> > Vitaly Mayatskikh (1):
> >   Add vhost_blk driver
> 
> 
> Thanks!
> Before merging this, I'd like to get some acks from userspace that it's
> actually going to be used - e.g. QEMU block maintainers.

I have CCed Kevin, who is the overall QEMU block layer maintainer.

Also CCing Denis since I think someone was working on a QEMU userspace
multiqueue virtio-blk device for maximum performance.

Previously vhost_blk.ko implementations were basically the same thing as
the QEMU x-data-plane=on (dedicated thread using Linux AIO), except they
were using a kernel thread and maybe submitted bios.

The performance differences weren't convincing enough that it seemed
worthwhile maintaining another code path which loses live migration, I/O
throttling, image file formats, etc (all the things that QEMU's block
layer supports).

Two changes since then:

1. x-data-plane=on has been replaced with a full trip down QEMU's block
layer (-object iothread,id=iothread0 -device
virtio-blk-pci,iothread=iothread0,...).  It's slower and not truly
multiqueue (yet!).

So from this perspective vhost_blk.ko might be more attractive again, at
least until further QEMU block layer work eliminates the multiqueue and
performance overheads.

2. SPDK has become available for users who want the best I/O performance
and are willing to sacrifice CPU cores for polling.

If you want better performance and don't care about QEMU block layer
features, could you use SPDK?  People who are the target market for
vhost_blk.ko would probably be willing to use SPDK and it already
exists...

From the QEMU userspace perspective, I think the best way to integrate
vhost_blk.ko is to transparently switch to it when possible.  If the
user enables QEMU block layer features that are incompatible with
vhost_blk.ko, then it should fall back to the QEMU block layer
transparently.

I'm not keen on yet another code path with it's own set of limitations
and having to educate users about how to make the choice.  But if it can
be integrated transparently as an "accelerator", then it could be
valuable.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Denis Lunev]

On 11/6/18 6:40 PM, Stefan Hajnoczi wrote:
> On Fri, Nov 02, 2018 at 02:26:00PM -0400, Michael S. Tsirkin wrote:
>> On Fri, Nov 02, 2018 at 06:21:22PM +0000, Vitaly Mayatskikh wrote:
>>> vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
>>> driver allows VM to reach a near bare-metal disk performance. See IOPS
>>> numbers below (fio --rw=randread --bs=4k).
>>>
>>> This implementation uses kiocb interface. It is slightly slower than
>>> going directly through bio, but is simpler and also works with disk
>>> images placed on a file system.
>>>
>>> # fio num-jobs
>>> # A: bare metal over block
>>> # B: bare metal over file
>>> # C: virtio-blk over block
>>> # D: virtio-blk over file
>>> # E: vhost-blk bio over block
>>> # F: vhost-blk kiocb over block
>>> # G: vhost-blk kiocb over file
>>> #
>>> #  A     B     C    D    E     F     G
>>>
>>> 1  171k  151k  148k 151k 195k  187k  175k
>>> 2  328k  302k  249k 241k 349k  334k  296k
>>> 3  479k  437k  179k 174k 501k  464k  404k
>>> 4  622k  568k  143k 183k 620k  580k  492k
>>> 5  755k  697k  136k 128k 737k  693k  579k
>>> 6  887k  808k  131k 120k 830k  782k  640k
>>> 7  1004k 926k  126k 131k 926k  863k  693k
>>> 8  1099k 1015k 117k 115k 1001k 931k  712k
>>> 9  1194k 1119k 115k 111k 1055k 991k  711k
>>> 10 1278k 1207k 109k 114k 1130k 1046k 695k
>>> 11 1345k 1280k 110k 108k 1119k 1091k 663k
>>> 12 1411k 1356k 104k 106k 1201k 1142k 629k
>>> 13 1466k 1423k 106k 106k 1260k 1170k 607k
>>> 14 1517k 1486k 103k 106k 1296k 1179k 589k
>>> 15 1552k 1543k 102k 102k 1322k 1191k 571k
>>> 16 1480k 1506k 101k 102k 1346k 1202k 566k
>>>
>>> Vitaly Mayatskikh (1):
>>>   Add vhost_blk driver
>>
>> Thanks!
>> Before merging this, I'd like to get some acks from userspace that it's
>> actually going to be used - e.g. QEMU block maintainers.
> I have CCed Kevin, who is the overall QEMU block layer maintainer.
>
> Also CCing Denis since I think someone was working on a QEMU userspace
> multiqueue virtio-blk device for maximum performance.
>
> Previously vhost_blk.ko implementations were basically the same thing as
> the QEMU x-data-plane=on (dedicated thread using Linux AIO), except they
> were using a kernel thread and maybe submitted bios.
>
> The performance differences weren't convincing enough that it seemed
> worthwhile maintaining another code path which loses live migration, I/O
> throttling, image file formats, etc (all the things that QEMU's block
> layer supports).
>
> Two changes since then:
>
> 1. x-data-plane=on has been replaced with a full trip down QEMU's block
> layer (-object iothread,id=iothread0 -device
> virtio-blk-pci,iothread=iothread0,...).  It's slower and not truly
> multiqueue (yet!).
>
> So from this perspective vhost_blk.ko might be more attractive again, at
> least until further QEMU block layer work eliminates the multiqueue and
> performance overheads.
>
> 2. SPDK has become available for users who want the best I/O performance
> and are willing to sacrifice CPU cores for polling.
>
> If you want better performance and don't care about QEMU block layer
> features, could you use SPDK?  People who are the target market for
> vhost_blk.ko would probably be willing to use SPDK and it already
> exists...
>
> From the QEMU userspace perspective, I think the best way to integrate
> vhost_blk.ko is to transparently switch to it when possible.  If the
> user enables QEMU block layer features that are incompatible with
> vhost_blk.ko, then it should fall back to the QEMU block layer
> transparently.
>
> I'm not keen on yet another code path with it's own set of limitations
> and having to educate users about how to make the choice.  But if it can
> be integrated transparently as an "accelerator", then it could be
> valuable.
>
> Stefan
Stefan,thank you very much for adding me into the loop :)
Patches themselves are very interesting and worth to
discuss.

First of all, I am not a very big fan of SPDK approach
and like kernel kernel implementation much more.
The reason for this is very simple. We are are not
discussing the overhead of the emulation. It would be
very interesting to compare CPU usage of each approach
and compare the overhead.

The problem is that we can not afford too costly
emulation for most scenarios.

I do think that kernel based approaches generates not
only more IOPSes but less overhead too.

Den

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskih]

On Tue, Nov 6, 2018 at 10:40 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:

> Previously vhost_blk.ko implementations were basically the same thing as
> the QEMU x-data-plane=on (dedicated thread using Linux AIO), except they
> were using a kernel thread and maybe submitted bios.
>
> The performance differences weren't convincing enough that it seemed
> worthwhile maintaining another code path which loses live migration, I/O
> throttling, image file formats, etc (all the things that QEMU's block
> layer supports).
>
> Two changes since then:
>
> 1. x-data-plane=on has been replaced with a full trip down QEMU's block
> layer (-object iothread,id=iothread0 -device
> virtio-blk-pci,iothread=iothread0,...).  It's slower and not truly
> multiqueue (yet!).
>
> So from this perspective vhost_blk.ko might be more attractive again, at
> least until further QEMU block layer work eliminates the multiqueue and
> performance overheads.

Yes, this work is a direct consequence of insufficient performance of
virtio-blk's host side. I'm working on a storage driver, but there's
no a good way to feed all these IOs into one disk of one VM. The
nature of storage design dictates the need of very high IOPS seen by
VM. This is only one tiny use case of course, but the vhost/QEMU
change is small enough to share.

> 2. SPDK has become available for users who want the best I/O performance
> and are willing to sacrifice CPU cores for polling.
>
> If you want better performance and don't care about QEMU block layer
> features, could you use SPDK?  People who are the target market for
> vhost_blk.ko would probably be willing to use SPDK and it already
> exists...

Yes. Though in my experience SPDK creates more problems most of times
than it solves ;) What I find very compelling in using a plain Linux
block device is that it is really fast these days (blk-mq) and the
device mapper can be used for even greater flexibility. Device mapper
is less than perfect performance-wise and at some point will need some
work for sure, but still can push few million IOPS through. And it's
all standard code with decades old user APIs.

In fact, Linux kernel is so good now that our pure-software solution
can push IO at rates up to the limits of fat hardware (x00 GbE, bunch
of NVMes) without an apparent need for hardware acceleration. And,
without hardware dependencies, it is much more flexible. Disk
interface between the host and VM was the only major bottleneck.

> From the QEMU userspace perspective, I think the best way to integrate
> vhost_blk.ko is to transparently switch to it when possible.  If the
> user enables QEMU block layer features that are incompatible with
> vhost_blk.ko, then it should fall back to the QEMU block layer
> transparently.

Sounds like an excellent idea! I'll do that. Most of vhost-blk support
in QEMU is a boilerplate code anyways.

> I'm not keen on yet another code path with it's own set of limitations
> and having to educate users about how to make the choice.  But if it can
> be integrated transparently as an "accelerator", then it could be
> valuable.

Understood. Agree.

Thanks!
-- 
wbr, Vitaly

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Maxim Levitsky]

On Fri, 2018-11-02 at 18:21 +0000, Vitaly Mayatskikh wrote:
> vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
> driver allows VM to reach a near bare-metal disk performance. See IOPS
> numbers below (fio --rw=randread --bs=4k).
> 
> This implementation uses kiocb interface. It is slightly slower than
> going directly through bio, but is simpler and also works with disk
> images placed on a file system.
> 
> # fio num-jobs
> # A: bare metal over block
> # B: bare metal over file
> # C: virtio-blk over block
> # D: virtio-blk over file
> # E: vhost-blk bio over block
> # F: vhost-blk kiocb over block
> # G: vhost-blk kiocb over file
> #
> #  A     B     C    D    E     F     G
> 

Hi!
I am also working in this area, and so I am very intersted in this driver.

> 1  171k  151k  148k 151k 195k  187k  175k
If I understand correctly this is fio --numjobs=1?
It looks like you are getting better that native performance over bare metal in
E,F,G (vhost-blk cases in fact). Is this correct?

Could you share the full fio command line you have used?
Which IO device did you use for the test? NVME?
Which system (cpu model/number of cores/etc) did you test on?

Best regards,
      Maxim Levitsky

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskih]

On Sun, Nov 4, 2018 at 6:57 AM Maxim Levitsky <mlevitsk@redhat.com> wrote:

> Hi!
> I am also working in this area, and so I am very intersted in this driver.
>
> > 1  171k  151k  148k 151k 195k  187k  175k
> If I understand correctly this is fio --numjobs=1?
> It looks like you are getting better that native performance over bare metal in
> E,F,G (vhost-blk cases in fact). Is this correct?

Yes. At such speeds it is a matter of how the workers are scheduled,
i.e. how good is batching. There are other factors why vhost-blk is on
par or slightly higher than fio running in userspace on bare metal,
but from my observation the right batching all through the stack is
more important.

> Could you share the full fio command line you have used?

sysctl -w vm.nr_hugepages=8300; numactl -p 1 -N 1 ./qemu-system-x86_64
-enable-kvm -cpu host -smp 16 -mem-prealloc -mem-path
/dev/hugepages/foo -m 8G -nographic -drive
if=none,id=drive0,format=raw,file=/dev/mapper/mirror-hello,cache=none
-device virtio-blk-pci,id=blk0,drive=drive0,num-queues=16 -drive
if=none,id=drive1,format=raw,file=/dev/mapper/mirror-volume,cache=none
-device vhost-blk-pci,id=blk1,drive=drive1,num-queues=16

for i in `seq 1 16`; do echo -n "$i "; ./fio --direct=1 --rw=randread
--ioengine=libaio --bs=4k --iodepth=128 --numjobs=$i --name=foo
--time_based --runtime=15 --group_reporting --filename=/dev/vda
--size=10g | grep -Po 'IOPS=[0-9\.]*k'; done

> Which IO device did you use for the test? NVME?

That was LVM mirror over 2 network disks. On the target side it was
LVM stripe over few NVMe's.

> Which system (cpu model/number of cores/etc) did you test on?

Dual socket: "model name    : Intel(R) Xeon(R) Gold 6142 CPU @
2.60GHz" with HT enabled, so 64 logical cores in total. The network
was something from Intel with 53 Gbps PHY and served by fm10k driver.

-- 
wbr, Vitaly

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Maxim Levitsky]

On Sun, 2018-11-04 at 11:40 -0500, Vitaly Mayatskih wrote:
> On Sun, Nov 4, 2018 at 6:57 AM Maxim Levitsky <mlevitsk@redhat.com> wrote:
> 
> > Hi!
> > I am also working in this area, and so I am very intersted in this driver.
> > 
> > > 1  171k  151k  148k 151k 195k  187k  175k
> > 
> > If I understand correctly this is fio --numjobs=1?
> > It looks like you are getting better that native performance over bare metal
> > in
> > E,F,G (vhost-blk cases in fact). Is this correct?
> 
> Yes. At such speeds it is a matter of how the workers are scheduled,
> i.e. how good is batching. There are other factors why vhost-blk is on
> par or slightly higher than fio running in userspace on bare metal,
> but from my observation the right batching all through the stack is
> more important.

I completely agree with you on that.
I currently learning profiling/tracing to understand the batching (or lack of)
in the tests I run

My focus currently is mostly on spdk + native nvme
While for multiple threads, the performance is very close to bare metal, on
single thread I see significant overhead which probably relate to batching as
well.

> 
> > Could you share the full fio command line you have used?
> 
> sysctl -w vm.nr_hugepages=8300; numactl -p 1 -N 1 ./qemu-system-x86_64
> -enable-kvm -cpu host -smp 16 -mem-prealloc -mem-path
> /dev/hugepages/foo -m 8G -nographic -drive
> if=none,id=drive0,format=raw,file=/dev/mapper/mirror-hello,cache=none
> -device virtio-blk-pci,id=blk0,drive=drive0,num-queues=16 -drive
> if=none,id=drive1,format=raw,file=/dev/mapper/mirror-volume,cache=none
> -device vhost-blk-pci,id=blk1,drive=drive1,num-queues=16
Thanks!

> 
> for i in `seq 1 16`; do echo -n "$i "; ./fio --direct=1 --rw=randread
> --ioengine=libaio --bs=4k --iodepth=128 --numjobs=$i --name=foo
> --time_based --runtime=15 --group_reporting --filename=/dev/vda
> --size=10g | grep -Po 'IOPS=[0-9\.]*k'; done
> 
> > Which IO device did you use for the test? NVME?
> 
> That was LVM mirror over 2 network disks. On the target side it was
> LVM stripe over few NVMe's.
Have you tried to test this over directly connected NVME device to?
The networking might naturally improve batching I think.

> 
> > Which system (cpu model/number of cores/etc) did you test on?
> 
> Dual socket: "model name    : Intel(R) Xeon(R) Gold 6142 CPU @
> 2.60GHz" with HT enabled, so 64 logical cores in total. The network
> was something from Intel with 53 Gbps PHY and served by fm10k driver.


All right, thanks!
I'll test your driver on my system where I tested most of the current solutions.

Best regards,
	Maxim Levitsky

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Jason Wang]

On 2018/11/3 上午2:21, Vitaly Mayatskikh wrote:
> vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
> driver allows VM to reach a near bare-metal disk performance. See IOPS
> numbers below (fio --rw=randread --bs=4k).
>
> This implementation uses kiocb interface. It is slightly slower than
> going directly through bio, but is simpler and also works with disk
> images placed on a file system.
>
> # fio num-jobs
> # A: bare metal over block
> # B: bare metal over file
> # C: virtio-blk over block
> # D: virtio-blk over file
> # E: vhost-blk bio over block
> # F: vhost-blk kiocb over block
> # G: vhost-blk kiocb over file
> #
> #  A     B     C    D    E     F     G
>
> 1  171k  151k  148k 151k 195k  187k  175k
> 2  328k  302k  249k 241k 349k  334k  296k
> 3  479k  437k  179k 174k 501k  464k  404k
> 4  622k  568k  143k 183k 620k  580k  492k
> 5  755k  697k  136k 128k 737k  693k  579k
> 6  887k  808k  131k 120k 830k  782k  640k
> 7  1004k 926k  126k 131k 926k  863k  693k
> 8  1099k 1015k 117k 115k 1001k 931k  712k
> 9  1194k 1119k 115k 111k 1055k 991k  711k
> 10 1278k 1207k 109k 114k 1130k 1046k 695k
> 11 1345k 1280k 110k 108k 1119k 1091k 663k
> 12 1411k 1356k 104k 106k 1201k 1142k 629k
> 13 1466k 1423k 106k 106k 1260k 1170k 607k
> 14 1517k 1486k 103k 106k 1296k 1179k 589k
> 15 1552k 1543k 102k 102k 1322k 1191k 571k
> 16 1480k 1506k 101k 102k 1346k 1202k 566k
>
> Vitaly Mayatskikh (1):
>    Add vhost_blk driver
>
>   drivers/vhost/Kconfig  |  13 ++
>   drivers/vhost/Makefile |   3 +
>   drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 526 insertions(+)
>   create mode 100644 drivers/vhost/blk.c
>

Hi:

Thanks for the patches.

This is not the first attempt for having vhost-blk:

- Badari's version: https://lwn.net/Articles/379864/

- Asias' version: https://lwn.net/Articles/519880/

It's better to describe the differences (kiocb vs bio? performance?). 
E.g if my memory is correct, Asias said it doesn't give much improvement 
compared with userspace qemu.

And what's more important, I believe we tend to use virtio-scsi nowdays. 
So what's the advantages of vhost-blk over vhost-scsi?

Thanks

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskih]

On Sun, Nov 4, 2018 at 10:00 PM Jason Wang <jasowang@redhat.com> wrote:

> > # fio num-jobs
> > # A: bare metal over block
> > # B: bare metal over file
> > # C: virtio-blk over block
> > # D: virtio-blk over file
> > # E: vhost-blk bio over block
> > # F: vhost-blk kiocb over block
> > # G: vhost-blk kiocb over file
> > #
> > #  A     B     C    D    E     F     G

> > 16 1480k 1506k 101k 102k 1346k 1202k 566k

> Hi:
>
> Thanks for the patches.
>
> This is not the first attempt for having vhost-blk:
>
> - Badari's version: https://lwn.net/Articles/379864/
>
> - Asias' version: https://lwn.net/Articles/519880/
>
> It's better to describe the differences (kiocb vs bio? performance?).
> E.g if my memory is correct, Asias said it doesn't give much improvement
> compared with userspace qemu.
>
> And what's more important, I believe we tend to use virtio-scsi nowdays.
> So what's the advantages of vhost-blk over vhost-scsi?

Hi,

Yes, I saw both. Frankly, my implementation is not that different,
because the whole thing has only twice more LOC that vhost/test.c.

I posted my numbers (see in quoted text above the 16 queues case),
IOPS goes from ~100k to 1.2M and almost reaches the physical
limitation of the backend.

submit_bio() is a bit faster, but can't be used for disk images placed
on a file system. I have that submit_bio implementation too.

Storage industry is shifting away from SCSI, which has a scaling
problem. I can compare vhost-scsi vs vhost-blk if you are curious.

Thanks!
-- 
wbr, Vitaly

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Jason Wang]

On 2018/11/5 上午11:23, Vitaly Mayatskih wrote:
> On Sun, Nov 4, 2018 at 10:00 PM Jason Wang <jasowang@redhat.com> wrote:
>
>>> # fio num-jobs
>>> # A: bare metal over block
>>> # B: bare metal over file
>>> # C: virtio-blk over block
>>> # D: virtio-blk over file
>>> # E: vhost-blk bio over block
>>> # F: vhost-blk kiocb over block
>>> # G: vhost-blk kiocb over file
>>> #
>>> #  A     B     C    D    E     F     G
>>> 16 1480k 1506k 101k 102k 1346k 1202k 566k
>> Hi:
>>
>> Thanks for the patches.
>>
>> This is not the first attempt for having vhost-blk:
>>
>> - Badari's version: https://lwn.net/Articles/379864/
>>
>> - Asias' version: https://lwn.net/Articles/519880/
>>
>> It's better to describe the differences (kiocb vs bio? performance?).
>> E.g if my memory is correct, Asias said it doesn't give much improvement
>> compared with userspace qemu.
>>
>> And what's more important, I believe we tend to use virtio-scsi nowdays.
>> So what's the advantages of vhost-blk over vhost-scsi?
> Hi,
>
> Yes, I saw both. Frankly, my implementation is not that different,
> because the whole thing has only twice more LOC that vhost/test.c.
>
> I posted my numbers (see in quoted text above the 16 queues case),
> IOPS goes from ~100k to 1.2M and almost reaches the physical
> limitation of the backend.
>
> submit_bio() is a bit faster, but can't be used for disk images placed
> on a file system. I have that submit_bio implementation too.
>
> Storage industry is shifting away from SCSI, which has a scaling
> problem.


Know little about storage. For scaling, do you mean SCSI protocol 
itself? If not, it's probably not a real issue for virtio-scsi itself.


>   I can compare vhost-scsi vs vhost-blk if you are curious.


It would be very helpful to see the performance comparison.


Thanks


> Thanks!

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskih]

On Mon, Nov 5, 2018 at 9:45 PM Jason Wang <jasowang@redhat.com> wrote:

> > Storage industry is shifting away from SCSI, which has a scaling
> > problem.
>
>
> Know little about storage. For scaling, do you mean SCSI protocol
> itself? If not, it's probably not a real issue for virtio-scsi itself.

Yes,  the protocol.

> >   I can compare vhost-scsi vs vhost-blk if you are curious.
>
>
> It would be very helpful to see the performance comparison.

I will try to find comparable hw with fast enough SAS SSDs. That one
was done with NVMe's sitting on the very bottom of storage topology.


-- 
wbr, Vitaly

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Christoph Hellwig]

On Tue, Nov 06, 2018 at 10:45:08AM +0800, Jason Wang wrote:
> > Storage industry is shifting away from SCSI, which has a scaling
> > problem.
> 
> 
> Know little about storage. For scaling, do you mean SCSI protocol itself? If
> not, it's probably not a real issue for virtio-scsi itself.

The above is utter bullshit.  There is a big NVMe hype, but it is not
because "SCSI has a scaling problem", but because the industry can sell
a new thing, and the standardization body seems easier to work with.

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Paolo Bonzini]

On 06/11/2018 08:13, Christoph Hellwig wrote:
> On Tue, Nov 06, 2018 at 10:45:08AM +0800, Jason Wang wrote:
>>> Storage industry is shifting away from SCSI, which has a scaling
>>> problem.
>>
>> Know little about storage. For scaling, do you mean SCSI protocol itself? If
>> not, it's probably not a real issue for virtio-scsi itself.
> 
> The above is utter bullshit.  There is a big NVMe hype, but it is not
> because "SCSI has a scaling problem", but because the industry can sell
> a new thing, and the standardization body seems easier to work with.

In the case of Linux, there is indeed some performance to be gained by
skipping the SCSI layer in the guest and to a lesser extent in QEMU,
which is why virtio-blk is still in use.  But there's no scaling
problem, it's just extra constant overhead.

Paolo

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Christian Borntraeger]

On 11/05/2018 04:00 AM, Jason Wang wrote:
> 
> On 2018/11/3 上午2:21, Vitaly Mayatskikh wrote:
>> vhost_blk is a host-side kernel mode accelerator for virtio-blk. The
>> driver allows VM to reach a near bare-metal disk performance. See IOPS
>> numbers below (fio --rw=randread --bs=4k).
>>
>> This implementation uses kiocb interface. It is slightly slower than
>> going directly through bio, but is simpler and also works with disk
>> images placed on a file system.
>>
>> # fio num-jobs
>> # A: bare metal over block
>> # B: bare metal over file
>> # C: virtio-blk over block
>> # D: virtio-blk over file
>> # E: vhost-blk bio over block
>> # F: vhost-blk kiocb over block
>> # G: vhost-blk kiocb over file
>> #
>> #  A     B     C    D    E     F     G
>>
>> 1  171k  151k  148k 151k 195k  187k  175k
>> 2  328k  302k  249k 241k 349k  334k  296k
>> 3  479k  437k  179k 174k 501k  464k  404k
>> 4  622k  568k  143k 183k 620k  580k  492k
>> 5  755k  697k  136k 128k 737k  693k  579k
>> 6  887k  808k  131k 120k 830k  782k  640k
>> 7  1004k 926k  126k 131k 926k  863k  693k
>> 8  1099k 1015k 117k 115k 1001k 931k  712k
>> 9  1194k 1119k 115k 111k 1055k 991k  711k
>> 10 1278k 1207k 109k 114k 1130k 1046k 695k
>> 11 1345k 1280k 110k 108k 1119k 1091k 663k
>> 12 1411k 1356k 104k 106k 1201k 1142k 629k
>> 13 1466k 1423k 106k 106k 1260k 1170k 607k
>> 14 1517k 1486k 103k 106k 1296k 1179k 589k
>> 15 1552k 1543k 102k 102k 1322k 1191k 571k
>> 16 1480k 1506k 101k 102k 1346k 1202k 566k
>>
>> Vitaly Mayatskikh (1):
>>    Add vhost_blk driver
>>
>>   drivers/vhost/Kconfig  |  13 ++
>>   drivers/vhost/Makefile |   3 +
>>   drivers/vhost/blk.c    | 510 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 526 insertions(+)
>>   create mode 100644 drivers/vhost/blk.c
>>
> 
> Hi:
> 
> Thanks for the patches.
> 
> This is not the first attempt for having vhost-blk:
> 
> - Badari's version: https://lwn.net/Articles/379864/
> 
> - Asias' version: https://lwn.net/Articles/519880/
> 
> It's better to describe the differences (kiocb vs bio? performance?). E.g if my memory is correct, Asias said it doesn't give much improvement compared with userspace qemu.
> 
> And what's more important, I believe we tend to use virtio-scsi nowdays. So what's the advantages of vhost-blk over vhost-scsi?


For the record, we still do use virtio-blk a lot. As we see new things like discard/write zero 
support it seems that others do as well.

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Vitaly Mayatskih]

On Mon, Nov 5, 2018 at 10:48 AM Christian Borntraeger
<borntraeger@de.ibm.com> wrote:

> For the record, we still do use virtio-blk a lot. As we see new things like discard/write zero
> support it seems that others do as well.

Yes, trim/discard and writesame support is planned, at least for
submit_bio path (not in this patch).
-- 
wbr, Vitaly

Re: [PATCH 0/1] vhost: add vhost_blk driver

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Mon, Nov 05, 2018 at 11:15:04AM -0500, Vitaly Mayatskih wrote:
> On Mon, Nov 5, 2018 at 10:48 AM Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
> 
> > For the record, we still do use virtio-blk a lot. As we see new things like discard/write zero
> > support it seems that others do as well.
> 
> Yes, trim/discard and writesame support is planned, at least for
> submit_bio path (not in this patch).

I agree, virtio_blk is quite popular and not deprecated/frozen.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]