* [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance
@ 2019-08-07 7:10 piaojun
2019-08-07 9:21 ` Stefan Hajnoczi
2019-08-07 15:37 ` Dr. David Alan Gilbert
0 siblings, 2 replies; 6+ messages in thread
From: piaojun @ 2019-08-07 7:10 UTC (permalink / raw)
To: virtio-fs
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>
---
contrib/virtiofsd/passthrough_ll.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index cc9c175..c1bbc53 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2023,7 +2023,10 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
fuse_debug("lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu)\n",
ino, out_buf.buf[0].size, (unsigned long) off);
- res = fuse_buf_copy(req, &out_buf, in_buf, 0);
+ if (!(in_buf->buf[0].flags & FUSE_BUF_PHYS_ADDR))
+ res = fuse_buf_writev(req, &out_buf, in_buf, out_buf.buf[0].flags);
+ else
+ res = fuse_buf_copy(req, &out_buf, in_buf, 0);
if(res < 0) {
fuse_reply_err(req, -res);
} else {
--
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance
2019-08-07 7:10 [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance piaojun
@ 2019-08-07 9:21 ` Stefan Hajnoczi
2019-08-07 9:58 ` piaojun
2019-08-07 15:37 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2019-08-07 9:21 UTC (permalink / raw)
To: piaojun; +Cc: virtio-fs
On Wed, Aug 07, 2019 at 03:10:10PM +0800, piaojun 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>
> ---
> contrib/virtiofsd/passthrough_ll.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index cc9c175..c1bbc53 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -2023,7 +2023,10 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
> fuse_debug("lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu)\n",
> ino, out_buf.buf[0].size, (unsigned long) off);
>
> - res = fuse_buf_copy(req, &out_buf, in_buf, 0);
> + if (!(in_buf->buf[0].flags & FUSE_BUF_PHYS_ADDR))
> + res = fuse_buf_writev(req, &out_buf, in_buf, out_buf.buf[0].flags);
> + else
> + res = fuse_buf_copy(req, &out_buf, in_buf, 0);
Can you move the check inside fuse_buf_copy() and make fuse_buf_writev()
a static function? This way all fuse_buf_copy() callers automatically
benefit from the improvement (when possible).
By the way, this improvement can be sent upstream to libfuse so that
other FUSE file systems also get improved performance.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance
2019-08-07 9:21 ` Stefan Hajnoczi
@ 2019-08-07 9:58 ` piaojun
0 siblings, 0 replies; 6+ messages in thread
From: piaojun @ 2019-08-07 9:58 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: virtio-fs
Hi Stefan,
On 2019/8/7 17:21, Stefan Hajnoczi wrote:
> On Wed, Aug 07, 2019 at 03:10:10PM +0800, piaojun 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>
>> ---
>> contrib/virtiofsd/passthrough_ll.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
>> index cc9c175..c1bbc53 100644
>> --- a/contrib/virtiofsd/passthrough_ll.c
>> +++ b/contrib/virtiofsd/passthrough_ll.c
>> @@ -2023,7 +2023,10 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
>> fuse_debug("lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu)\n",
>> ino, out_buf.buf[0].size, (unsigned long) off);
>>
>> - res = fuse_buf_copy(req, &out_buf, in_buf, 0);
>> + if (!(in_buf->buf[0].flags & FUSE_BUF_PHYS_ADDR))
>> + res = fuse_buf_writev(req, &out_buf, in_buf, out_buf.buf[0].flags);
>> + else
>> + res = fuse_buf_copy(req, &out_buf, in_buf, 0);
>
> Can you move the check inside fuse_buf_copy() and make fuse_buf_writev()
> a static function? This way all fuse_buf_copy() callers automatically
> benefit from the improvement (when possible).
Good suggestion. I will try it in patch v3, and I'm also waiting other
developers's comments.
>
> By the way, this improvement can be sent upstream to libfuse so that
> other FUSE file systems also get improved performance.
Yes, I need some time looking into libfuse code and sync the fix.
Thanks,
Jun
>
> Stefan
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance
2019-08-07 7:10 [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance piaojun
2019-08-07 9:21 ` Stefan Hajnoczi
@ 2019-08-07 15:37 ` Dr. David Alan Gilbert
2019-08-08 1:26 ` piaojun
1 sibling, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2019-08-07 15:37 UTC (permalink / raw)
To: piaojun; +Cc: virtio-fs
* 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>
> ---
> contrib/virtiofsd/passthrough_ll.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index cc9c175..c1bbc53 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -2023,7 +2023,10 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
> fuse_debug("lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu)\n",
> ino, out_buf.buf[0].size, (unsigned long) off);
>
> - res = fuse_buf_copy(req, &out_buf, in_buf, 0);
> + if (!(in_buf->buf[0].flags & FUSE_BUF_PHYS_ADDR))
I don't think you can assume that the flags in in_buf[0] represent the
state of the entire vector; I'm pretty sure we had a case of an
application that did a writev() in the guest where the first element was
on the stack and the other was in an mmap, and it was only the 2nd one
that had the flag.
But also you're not checking any of the other flags either.
Dave
> + res = fuse_buf_writev(req, &out_buf, in_buf, out_buf.buf[0].flags);
> + else
> + res = fuse_buf_copy(req, &out_buf, in_buf, 0);
> if(res < 0) {
> fuse_reply_err(req, -res);
> } else {
> --
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance
2019-08-07 15:37 ` Dr. David Alan Gilbert
@ 2019-08-08 1:26 ` piaojun
2019-08-08 8:23 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 6+ messages in thread
From: piaojun @ 2019-08-08 1:26 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: virtio-fs
Hi Dave,
On 2019/8/7 23:37, Dr. David Alan Gilbert wrote:
> * 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>
>> ---
>> contrib/virtiofsd/passthrough_ll.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
>> index cc9c175..c1bbc53 100644
>> --- a/contrib/virtiofsd/passthrough_ll.c
>> +++ b/contrib/virtiofsd/passthrough_ll.c
>> @@ -2023,7 +2023,10 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
>> fuse_debug("lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu)\n",
>> ino, out_buf.buf[0].size, (unsigned long) off);
>>
>> - res = fuse_buf_copy(req, &out_buf, in_buf, 0);
>> + if (!(in_buf->buf[0].flags & FUSE_BUF_PHYS_ADDR))
>
> I don't think you can assume that the flags in in_buf[0] represent the
> state of the entire vector; I'm pretty sure we had a case of an
> application that did a writev() in the guest where the first element was
> on the stack and the other was in an mmap, and it was only the 2nd one
> that had the flag.
Yes, this assumption is idealized, and perhaps I need check all the
flags of entire vector before doing writev, right?
Thanks,
Jun
>
> But also you're not checking any of the other flags either.
>
> Dave
>
>> + res = fuse_buf_writev(req, &out_buf, in_buf, out_buf.buf[0].flags);
>> + else
>> + res = fuse_buf_copy(req, &out_buf, in_buf, 0);
>> if(res < 0) {
>> fuse_reply_err(req, -res);
>> } else {
>> --
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> .
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance
2019-08-08 1:26 ` piaojun
@ 2019-08-08 8:23 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2019-08-08 8:23 UTC (permalink / raw)
To: piaojun; +Cc: virtio-fs
* piaojun (piaojun@huawei.com) wrote:
> Hi Dave,
>
> On 2019/8/7 23:37, Dr. David Alan Gilbert wrote:
> > * 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>
> >> ---
> >> contrib/virtiofsd/passthrough_ll.c | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> >> index cc9c175..c1bbc53 100644
> >> --- a/contrib/virtiofsd/passthrough_ll.c
> >> +++ b/contrib/virtiofsd/passthrough_ll.c
> >> @@ -2023,7 +2023,10 @@ static void lo_write_buf(fuse_req_t req, fuse_ino_t ino,
> >> fuse_debug("lo_write_buf(ino=%" PRIu64 ", size=%zd, off=%lu)\n",
> >> ino, out_buf.buf[0].size, (unsigned long) off);
> >>
> >> - res = fuse_buf_copy(req, &out_buf, in_buf, 0);
> >> + if (!(in_buf->buf[0].flags & FUSE_BUF_PHYS_ADDR))
> >
> > I don't think you can assume that the flags in in_buf[0] represent the
> > state of the entire vector; I'm pretty sure we had a case of an
> > application that did a writev() in the guest where the first element was
> > on the stack and the other was in an mmap, and it was only the 2nd one
> > that had the flag.
>
> Yes, this assumption is idealized, and perhaps I need check all the
> flags of entire vector before doing writev, right?
Yes, you need to make sure that the vector fits the nice easy
sequential case for pwritev.
Dave
> Thanks,
> Jun
>
> >
> > But also you're not checking any of the other flags either.
> >
> > Dave
> >
> >> + res = fuse_buf_writev(req, &out_buf, in_buf, out_buf.buf[0].flags);
> >> + else
> >> + res = fuse_buf_copy(req, &out_buf, in_buf, 0);
> >> if(res < 0) {
> >> fuse_reply_err(req, -res);
> >> } else {
> >> --
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > .
> >
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-08 8:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-07 7:10 [Virtio-fs] [PATCH v2 2/2][RFC] use fuse_buf_writev to replace fuse_buf_write for better performance piaojun
2019-08-07 9:21 ` Stefan Hajnoczi
2019-08-07 9:58 ` piaojun
2019-08-07 15:37 ` Dr. David Alan Gilbert
2019-08-08 1:26 ` piaojun
2019-08-08 8:23 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.