[Virtio-fs] [PATCH v6 0/2] virtiofsd: Improve io bandwidth by replacing pwrite with pwritev

[Virtio-fs] [PATCH v6 0/2] virtiofsd: Improve io bandwidth by replacing pwrite with pwritev

[piaojun]

>From my test, write bandwidth will be improved greatly by replacing
pwrite with pwritev, and the test result as below:

---
pwrite:
# fio -direct=1 -time_based -iodepth=64 -rw=randwrite -ioengine=libaio -bs=1M -size=1G -numjob=16 -runtime=30 -group_reporting -name=file -filename=/mnt/virtiofs/file
file: (g=0): rw=randwrite, bs=1M-1M/1M-1M/1M-1M, ioengine=libaio, iodepth=64
...
fio-2.13
Starting 16 processes
Jobs: 16 (f=16): [w(16)] [100.0% done] [0KB/886.0MB/0KB /s] [0/886/0 iops] [eta 00m:00s]
file: (groupid=0, jobs=16): err= 0: pid=5799: Tue Aug 6 18:48:26 2019
write: io=26881MB, bw=916988KB/s, iops=895, runt= 30018msec

pwritev:
# fio -direct=1 -time_based -iodepth=64 -rw=randwrite -ioengine=libaio -bs=1M -size=1G -numjob=16 -runtime=30 -group_reporting -name=file -filename=/mnt/virtiofs/file
file: (g=0): rw=randwrite, bs=1M-1M/1M-1M/1M-1M, ioengine=libaio, iodepth=64
...
fio-2.13
Starting 16 processes
Jobs: 16 (f=16): [w(16)] [100.0% done] [0KB/1793MB/0KB /s] [0/1793/0 iops] [eta 00m:00s]
file: (groupid=0, jobs=16): err= 0: pid=6328: Tue Aug 6 18:22:17 2019
write: io=52775MB, bw=1758.7MB/s, iops=1758, runt= 30009msec
---

This patch introduces writev and pwritev for lo_write_buf().

v2
  - Split into two patches
  - Add the lost flags support, such as FUSE_BUF_PHYS_ADDR

v3
  - use git send-email to make the patch set in one thread
  - move fuse_buf_writev() into fuse_buf_copy()
  - use writev for the src buffers when they're alread already mapped by the daemon process
  - use calloc to replace malloc
  - set res 0 if writev() returns 0

v4
  - iterate from in_buf->buf[0] rather than buf[1]
  - optimize the code to make it more elegant

v5
  - add some check for preconditions of the buffers

v6
  - add more preconditions before fuse_buf_writev() to make it generic
  - skip the empty bufs to avoid writev failed in direct io mode

Jun Piao (2):
  add definition of fuse_buf_writev().
  use fuse_buf_writev to replace fuse_buf_write for better performance

Signed-off-by: Jun Piao <piaojun@huawei.com>
---
 buffer.c  |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 seccomp.c |    2 ++
 2 files changed, 53 insertions(+), 1 deletion(-)
--

[Virtio-fs] [PATCH v6 1/2] virtiofsd: add definition of fuse_buf_writev()

[piaojun]

Define fuse_buf_writev() which use pwritev and writev to improve io
bandwidth. Especially, the src bufs with 0 size should be skipped as
their mems are not *block_size* aligned which will cause writev failed
in direct io mode.

Signed-off-by: Jun Piao <piaojun@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/buffer.c  | 35 +++++++++++++++++++++++++++++++++++
 contrib/virtiofsd/seccomp.c |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/contrib/virtiofsd/buffer.c b/contrib/virtiofsd/buffer.c
index 655be137..0ed284f 100644
--- a/contrib/virtiofsd/buffer.c
+++ b/contrib/virtiofsd/buffer.c
@@ -15,6 +15,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <assert.h>
+#include <stdlib.h>

 size_t fuse_buf_size(const struct fuse_bufvec *bufv)
 {
@@ -31,6 +32,40 @@ size_t fuse_buf_size(const struct fuse_bufvec *bufv)
 	return size;
 }

+static ssize_t fuse_buf_writev(fuse_req_t req,
+			     struct fuse_buf *out_buf,
+			     struct fuse_bufvec *in_buf)
+{
+	ssize_t res, i, j;
+	size_t iovcnt = in_buf->count;
+	struct iovec * iov;
+	int fd = out_buf->fd;
+
+	iov = calloc(iovcnt, sizeof(struct iovec));
+	if (!iov)
+		return -ENOMEM;
+
+	for (i = 0, j = 0; i < iovcnt; i++) {
+		/* Skip the buf with 0 size */
+		if (in_buf->buf[i].size) {
+			iov[j].iov_base = in_buf->buf[i].mem;
+			iov[j].iov_len = in_buf->buf[i].size;
+			j++;
+		}
+	}
+
+	if (out_buf->flags & FUSE_BUF_FD_SEEK)
+		res = pwritev(fd, iov, iovcnt, out_buf->pos);
+	else
+		res = writev(fd, iov, iovcnt);
+
+	if (res == -1)
+		res = -errno;
+
+	free(iov);
+	return res;
+}
+
 static size_t min_size(size_t s1, size_t s2)
 {
 	return s1 < s2 ? s1 : s2;
diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
index 7384ebe..3b92c6e 100644
--- a/contrib/virtiofsd/seccomp.c
+++ b/contrib/virtiofsd/seccomp.c
@@ -60,6 +60,7 @@ static const int syscall_whitelist[] = {
 	SCMP_SYS(ppoll),
 	SCMP_SYS(prctl), /* TODO restrict to just PR_SET_NAME? */
 	SCMP_SYS(preadv),
+	SCMP_SYS(pwritev),
 	SCMP_SYS(pwrite64),
 	SCMP_SYS(read),
 	SCMP_SYS(readlinkat),
@@ -78,6 +79,7 @@ static const int syscall_whitelist[] = {
 	SCMP_SYS(unlinkat),
 	SCMP_SYS(utimensat),
 	SCMP_SYS(write),
+	SCMP_SYS(writev),
 	SCMP_SYS(capget),
 	SCMP_SYS(capset),
 };
-- 

[Virtio-fs] [PATCH v6 2/2] virtiofsd: use fuse_buf_writev to replace fuse_buf_write for better performance

[piaojun]

fuse_buf_writev() only handles the normal write in which src is buffer
and dest is fd. Specially if src buffer represents guest physical
address that can't be mapped by the daemon process, IO must be bounced
back to the VMM to do it by fuse_buf_copy().

Signed-off-by: Jun Piao <piaojun@huawei.com>
Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 contrib/virtiofsd/buffer.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/contrib/virtiofsd/buffer.c b/contrib/virtiofsd/buffer.c
index 0ed284f..072b5dc 100644
--- a/contrib/virtiofsd/buffer.c
+++ b/contrib/virtiofsd/buffer.c
@@ -322,11 +322,26 @@ static int fuse_bufvec_advance(struct fuse_bufvec *bufv, size_t len)
 ssize_t fuse_buf_copy(fuse_req_t req, struct fuse_bufvec *dstv, struct fuse_bufvec *srcv,
 		      enum fuse_buf_copy_flags flags)
 {
-	size_t copied = 0;
+	size_t copied = 0, i;

 	if (dstv == srcv)
 		return fuse_buf_size(dstv);

+	/* use writev to improve bandwidth when all the
+	 * src buffers already mapped by the daemon
+	 * process */
+	for (i = 0; i < srcv->count; i++) {
+		if ((srcv->buf[i].flags & FUSE_BUF_PHYS_ADDR) ||
+		    (srcv->buf[i].flags & FUSE_BUF_IS_FD))
+			break;
+	}
+	if ((i == srcv->count) && (dstv->count == 1) &&
+	    (dstv->idx == 0) &&
+	    (dstv->buf[0].flags & FUSE_BUF_IS_FD)) {
+		dstv->buf[0].pos += dstv->off;
+		return fuse_buf_writev(req, &dstv->buf[0], srcv);
+	}
+
 	for (;;) {
 		const struct fuse_buf *src = fuse_bufvec_current(srcv);
 		const struct fuse_buf *dst = fuse_bufvec_current(dstv);
-- 

Re: [Virtio-fs] [PATCH v6 2/2] virtiofsd: use fuse_buf_writev to replace fuse_buf_write for better performance

[Dr. David Alan Gilbert]

* piaojun (piaojun@huawei.com) wrote:
> fuse_buf_writev() only handles the normal write in which src is buffer
> and dest is fd. Specially if src buffer represents guest physical
> address that can't be mapped by the daemon process, IO must be bounced
> back to the VMM to do it by fuse_buf_copy().
> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  contrib/virtiofsd/buffer.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/virtiofsd/buffer.c b/contrib/virtiofsd/buffer.c
> index 0ed284f..072b5dc 100644
> --- a/contrib/virtiofsd/buffer.c
> +++ b/contrib/virtiofsd/buffer.c
> @@ -322,11 +322,26 @@ static int fuse_bufvec_advance(struct fuse_bufvec *bufv, size_t len)
>  ssize_t fuse_buf_copy(fuse_req_t req, struct fuse_bufvec *dstv, struct fuse_bufvec *srcv,
>  		      enum fuse_buf_copy_flags flags)
>  {
> -	size_t copied = 0;
> +	size_t copied = 0, i;
> 
>  	if (dstv == srcv)
>  		return fuse_buf_size(dstv);
> 
> +	/* use writev to improve bandwidth when all the
> +	 * src buffers already mapped by the daemon
> +	 * process */
> +	for (i = 0; i < srcv->count; i++) {
> +		if ((srcv->buf[i].flags & FUSE_BUF_PHYS_ADDR) ||
> +		    (srcv->buf[i].flags & FUSE_BUF_IS_FD))
> +			break;
> +	}
> +	if ((i == srcv->count) && (dstv->count == 1) &&
> +	    (dstv->idx == 0) &&
> +	    (dstv->buf[0].flags & FUSE_BUF_IS_FD)) {
> +		dstv->buf[0].pos += dstv->off;
> +		return fuse_buf_writev(req, &dstv->buf[0], srcv);
> +	}
> +

OK, thanks, I think that's actually a strong enough check, so I'll merge
these now.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  	for (;;) {
>  		const struct fuse_buf *src = fuse_bufvec_current(srcv);
>  		const struct fuse_buf *dst = fuse_bufvec_current(dstv);
> -- 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK