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

[Virtio-fs] [PATCH v3 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(). I tried my
best not doing harm to the origin code construct, and there will be
some *useless* branches in fuse_buf_copy_one() which are hard to judge
if they will be useful in the future. So I just leave them alone
safely. If the cleanup work is necessary, please let me know.

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

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>
Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 buffer.c      |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 fuse_common.h |   14 ++++++++++++++
 seccomp.c     |    2 ++
 3 files changed, 64 insertions(+)
--

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

[piaojun]

Define fuse_buf_writev() which use pwritev and writev to improve io
bandwidth.

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      | 37 +++++++++++++++++++++++++++++++++++++
 contrib/virtiofsd/fuse_common.h | 14 ++++++++++++++
 contrib/virtiofsd/seccomp.c     |  2 ++
 3 files changed, 53 insertions(+)

diff --git a/contrib/virtiofsd/buffer.c b/contrib/virtiofsd/buffer.c
index 655be137..3920cbf 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)
 {
@@ -71,6 +72,42 @@ static ssize_t fuse_buf_write(const struct fuse_buf *dst, size_t dst_off,
 	return copied;
 }

+ssize_t fuse_buf_writev(fuse_req_t req,
+			     struct fuse_bufvec *out_buf,
+			     struct fuse_bufvec *in_buf,
+			     enum fuse_buf_copy_flags flags)
+{
+	ssize_t res, i, buf_index, iovcnt;
+	struct iovec * iov;
+	int fd = out_buf->buf[0].fd;
+	off_t pos = out_buf->buf[0].pos;
+
+	if (in_buf->count > 2)
+		iovcnt = in_buf->count - 1;
+	else
+		iovcnt = 1;
+
+	iov = calloc(iovcnt, sizeof(struct iovec));
+	if (!iov)
+		return -ENOMEM;
+
+	for (i = 0, buf_index = 1; i < iovcnt; i++, buf_index++) {
+		iov[i].iov_base = in_buf->buf[buf_index].mem;
+		iov[i].iov_len = in_buf->buf[buf_index].size;
+	}
+
+	if (flags & FUSE_BUF_FD_SEEK)
+		res = pwritev(fd, iov, iovcnt, pos);
+	else
+		res = writev(fd, iov, iovcnt);
+
+	if (res == -1)
+		res = -errno;
+
+	free(iov);
+	return res;
+}
+
 static ssize_t fuse_buf_read(const struct fuse_buf *dst, size_t dst_off,
 			     const struct fuse_buf *src, size_t src_off,
 			     size_t len)
diff --git a/contrib/virtiofsd/fuse_common.h b/contrib/virtiofsd/fuse_common.h
index 001e6d6..7c07da7 100644
--- a/contrib/virtiofsd/fuse_common.h
+++ b/contrib/virtiofsd/fuse_common.h
@@ -730,6 +730,20 @@ ssize_t fuse_buf_copy(fuse_req_t req,
 		      enum fuse_buf_copy_flags flags);

 /**
+ * Write data from one buffer vector to another
+ *
+ * @param req The request this copy is part of
+ * @param out_buf destination buffer vector
+ * @param in_buf source buffer vector
+ * @param flags flags controlling the copy
+ * @return actual number of bytes written or -errno on error
+ */
+ssize_t fuse_buf_writev(fuse_req_t req,
+		      struct fuse_bufvec *out_buf,
+		      struct fuse_bufvec *in_buf,
+		      enum fuse_buf_copy_flags flags);
+
+/**
  * Memory buffer iterator
  *
  */
diff --git a/contrib/virtiofsd/seccomp.c b/contrib/virtiofsd/seccomp.c
index 5f1c873..4bba30b 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),
 };

 /* Syscalls used when --syslog is enabled */
-- 

[Virtio-fs] [PATCH v3 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/contrib/virtiofsd/buffer.c b/contrib/virtiofsd/buffer.c
index 3920cbf..f09d43c 100644
--- a/contrib/virtiofsd/buffer.c
+++ b/contrib/virtiofsd/buffer.c
@@ -324,11 +324,22 @@ 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 i;
 	size_t copied = 0;

 	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)
+			break;
+	}
+	if (i == srcv->count)
+		return fuse_buf_writev(req, dstv, srcv, dstv->buf[0].flags);
+
 	for (;;) {
 		const struct fuse_buf *src = fuse_bufvec_current(srcv);
 		const struct fuse_buf *dst = fuse_bufvec_current(dstv);
-- 

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

[Stefan Hajnoczi]

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

On Thu, Aug 08, 2019 at 02:15:27PM +0800, piaojun wrote:
> @@ -71,6 +72,42 @@ static ssize_t fuse_buf_write(const struct fuse_buf *dst, size_t dst_off,
>  	return copied;
>  }
> 
> +ssize_t fuse_buf_writev(fuse_req_t req,
> +			     struct fuse_bufvec *out_buf,
> +			     struct fuse_bufvec *in_buf,
> +			     enum fuse_buf_copy_flags flags)

FUSE_BUF_FD_SEEK is defined in enum fuse_buf_flags, not enum
fuse_buf_copy_flags.  This argument can be removed and the check below
can be changed from:

  if (flags & FUSE_BUF_FD_SEEK)

to

  if (out_buf->buf[0].flags & FUSE_BUF_FD_SEEK)

> +{
> +	ssize_t res, i, buf_index, iovcnt;
> +	struct iovec * iov;
> +	int fd = out_buf->buf[0].fd;
> +	off_t pos = out_buf->buf[0].pos;

A struct fuse_bufvec may have multiple elements but this function
assumes it only has 1.  Please use struct fuse_buf *out_buf instead.
This way it's clear that only 1 fuse_buf will be written.

> +
> +	if (in_buf->count > 2)
> +		iovcnt = in_buf->count - 1;
> +	else
> +		iovcnt = 1;
> +
> +	iov = calloc(iovcnt, sizeof(struct iovec));
> +	if (!iov)
> +		return -ENOMEM;
> +
> +	for (i = 0, buf_index = 1; i < iovcnt; i++, buf_index++) {
> +		iov[i].iov_base = in_buf->buf[buf_index].mem;
> +		iov[i].iov_len = in_buf->buf[buf_index].size;
> +	}

Why is in_buf->buf[0] is skipped?

> +
> +	if (flags & FUSE_BUF_FD_SEEK)
> +		res = pwritev(fd, iov, iovcnt, pos);

Please move off_t pos = out_buf->buf[0].pos into this if statement body
to avoid a possible uninitialized memory access when !(flags &
FUSE_BUF_FD_SEEK).  This makes valgrind and other tools happy.

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

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

[piaojun]

Hi Stefan,

On 2019/8/8 17:38, Stefan Hajnoczi wrote:
> On Thu, Aug 08, 2019 at 02:15:27PM +0800, piaojun wrote:
>> @@ -71,6 +72,42 @@ static ssize_t fuse_buf_write(const struct fuse_buf *dst, size_t dst_off,
>>  	return copied;
>>  }
>>
>> +ssize_t fuse_buf_writev(fuse_req_t req,
>> +			     struct fuse_bufvec *out_buf,
>> +			     struct fuse_bufvec *in_buf,
>> +			     enum fuse_buf_copy_flags flags)
> 
> FUSE_BUF_FD_SEEK is defined in enum fuse_buf_flags, not enum
> fuse_buf_copy_flags.  This argument can be removed and the check below
> can be changed from:
> 
>   if (flags & FUSE_BUF_FD_SEEK)
> 
> to
> 
>   if (out_buf->buf[0].flags & FUSE_BUF_FD_SEEK)

Accepted, and will be fixed in PATCH v4.

> 
>> +{
>> +	ssize_t res, i, buf_index, iovcnt;
>> +	struct iovec * iov;
>> +	int fd = out_buf->buf[0].fd;
>> +	off_t pos = out_buf->buf[0].pos;
> 
> A struct fuse_bufvec may have multiple elements but this function
> assumes it only has 1.  Please use struct fuse_buf *out_buf instead.
> This way it's clear that only 1 fuse_buf will be written.

Do you mean that assign *out_buf with &out_buf->buf[0] in
fuse_buf_writev() and then operate out_buf in the following process?

> 
>> +
>> +	if (in_buf->count > 2)
>> +		iovcnt = in_buf->count - 1;
>> +	else
>> +		iovcnt = 1;
>> +
>> +	iov = calloc(iovcnt, sizeof(struct iovec));
>> +	if (!iov)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0, buf_index = 1; i < iovcnt; i++, buf_index++) {
>> +		iov[i].iov_base = in_buf->buf[buf_index].mem;
>> +		iov[i].iov_len = in_buf->buf[buf_index].size;
>> +	}
> 
> Why is in_buf->buf[0] is skipped?

This part of code seems a little complex and in_buf->buf[0].count just
tricks me. Maybe I need handle two situations:
1. Iterate from in_buf->buf[0].mem when in_buf->buf[0].count is 1;
2. buf[0].size is set 0 in do_write_buf() when in_buf->buf[0].count > 1,
and it's better to skip it. Perhaps it's also ok to iterate from buf[0],
as writev skip the iov with zero len. This looks like a hack.

> 
>> +
>> +	if (flags & FUSE_BUF_FD_SEEK)
>> +		res = pwritev(fd, iov, iovcnt, pos);
> 
> Please move off_t pos = out_buf->buf[0].pos into this if statement body
> to avoid a possible uninitialized memory access when !(flags &
> FUSE_BUF_FD_SEEK).  This makes valgrind and other tools happy.

Accepted.

Thanks,
Jun

> 

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

[Stefan Hajnoczi]

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

On Thu, Aug 08, 2019 at 10:28:26PM +0800, piaojun wrote:
> On 2019/8/8 17:38, Stefan Hajnoczi wrote:
> > On Thu, Aug 08, 2019 at 02:15:27PM +0800, piaojun wrote:
> >> +{
> >> +	ssize_t res, i, buf_index, iovcnt;
> >> +	struct iovec * iov;
> >> +	int fd = out_buf->buf[0].fd;
> >> +	off_t pos = out_buf->buf[0].pos;
> > 
> > A struct fuse_bufvec may have multiple elements but this function
> > assumes it only has 1.  Please use struct fuse_buf *out_buf instead.
> > This way it's clear that only 1 fuse_buf will be written.
> 
> Do you mean that assign *out_buf with &out_buf->buf[0] in
> fuse_buf_writev() and then operate out_buf in the following process?

Since this function assumes that out_buf is only a single struct
fuse_buf, please change the argument to struct fuse_buf instead of
struct fuse_bufvec.  This way it's clear what the function expects.

> > 
> >> +
> >> +	if (in_buf->count > 2)
> >> +		iovcnt = in_buf->count - 1;
> >> +	else
> >> +		iovcnt = 1;
> >> +
> >> +	iov = calloc(iovcnt, sizeof(struct iovec));
> >> +	if (!iov)
> >> +		return -ENOMEM;
> >> +
> >> +	for (i = 0, buf_index = 1; i < iovcnt; i++, buf_index++) {
> >> +		iov[i].iov_base = in_buf->buf[buf_index].mem;
> >> +		iov[i].iov_len = in_buf->buf[buf_index].size;
> >> +	}
> > 
> > Why is in_buf->buf[0] is skipped?
> 
> This part of code seems a little complex and in_buf->buf[0].count just
> tricks me. Maybe I need handle two situations:
> 1. Iterate from in_buf->buf[0].mem when in_buf->buf[0].count is 1;
> 2. buf[0].size is set 0 in do_write_buf() when in_buf->buf[0].count > 1,
> and it's better to skip it. Perhaps it's also ok to iterate from buf[0],
> as writev skip the iov with zero len. This looks like a hack.

I would perform writev() with all of in_buf's buffers, even if some of
them have 0 length.  That way this function does not make any
assumptions about in_buf's layout.

The only requirement is that in_buf buffers are all memory buffers (not
file descriptors).

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

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

[piaojun]

Hi Stefan,

On 2019/8/9 16:14, Stefan Hajnoczi wrote:
> On Thu, Aug 08, 2019 at 10:28:26PM +0800, piaojun wrote:
>> On 2019/8/8 17:38, Stefan Hajnoczi wrote:
>>> On Thu, Aug 08, 2019 at 02:15:27PM +0800, piaojun wrote:
>>>> +{
>>>> +	ssize_t res, i, buf_index, iovcnt;
>>>> +	struct iovec * iov;
>>>> +	int fd = out_buf->buf[0].fd;
>>>> +	off_t pos = out_buf->buf[0].pos;
>>>
>>> A struct fuse_bufvec may have multiple elements but this function
>>> assumes it only has 1.  Please use struct fuse_buf *out_buf instead.
>>> This way it's clear that only 1 fuse_buf will be written.
>>
>> Do you mean that assign *out_buf with &out_buf->buf[0] in
>> fuse_buf_writev() and then operate out_buf in the following process?
> 
> Since this function assumes that out_buf is only a single struct
> fuse_buf, please change the argument to struct fuse_buf instead of
> struct fuse_bufvec.  This way it's clear what the function expects.

Agreed, already fixed in PATCH v4.

> 
>>>
>>>> +
>>>> +	if (in_buf->count > 2)
>>>> +		iovcnt = in_buf->count - 1;
>>>> +	else
>>>> +		iovcnt = 1;
>>>> +
>>>> +	iov = calloc(iovcnt, sizeof(struct iovec));
>>>> +	if (!iov)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	for (i = 0, buf_index = 1; i < iovcnt; i++, buf_index++) {
>>>> +		iov[i].iov_base = in_buf->buf[buf_index].mem;
>>>> +		iov[i].iov_len = in_buf->buf[buf_index].size;
>>>> +	}
>>>
>>> Why is in_buf->buf[0] is skipped?
>>
>> This part of code seems a little complex and in_buf->buf[0].count just
>> tricks me. Maybe I need handle two situations:
>> 1. Iterate from in_buf->buf[0].mem when in_buf->buf[0].count is 1;
>> 2. buf[0].size is set 0 in do_write_buf() when in_buf->buf[0].count > 1,
>> and it's better to skip it. Perhaps it's also ok to iterate from buf[0],
>> as writev skip the iov with zero len. This looks like a hack.
> 
> I would perform writev() with all of in_buf's buffers, even if some of
> them have 0 length.  That way this function does not make any
> assumptions about in_buf's layout.

Agreed, already fixed in PATCH v4.

Jun

> 
> The only requirement is that in_buf buffers are all memory buffers (not
> file descriptors).
>