* [Virtio-fs] [PATCH] virtiofsd: During setup mapping, open file O_RDWR only if needed
@ 2019-07-24 21:10 Vivek Goyal
2019-07-24 23:29 ` Liu Bo
0 siblings, 1 reply; 5+ messages in thread
From: Vivek Goyal @ 2019-07-24 21:10 UTC (permalink / raw)
To: virtio-fs-list
As of now we always open file O_RDWR (even if writable mappings are not
required). This leads to copy up of file if file is backed by overlayfs
and hence nullying advantages of overlayfs.
So open file O_RDONLY if writable mappings are not required. Open O_RDWR
if writable mappings are needed.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
contrib/virtiofsd/passthrough_ll.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
Index: qemu/contrib/virtiofsd/passthrough_ll.c
===================================================================
--- qemu.orig/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:31:07.014871768 -0400
+++ qemu/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:32:10.064412722 -0400
@@ -2197,15 +2197,15 @@ static void lo_setupmapping(fuse_req_t r
VhostUserFSSlaveMsg msg = { 0 };
uint64_t vhu_flags;
char *buf;
+ bool writable = flags & FUSE_SETUPMAPPING_FLAG_WRITE;
if (lo_debug(req))
fuse_debug("lo_setupmapping(ino=%" PRIu64 ", fi=0x%p)\n", ino,
(void *)fi);
vhu_flags = VHOST_USER_FS_FLAG_MAP_R;
- if (flags & O_WRONLY) {
+ if (writable)
vhu_flags |= VHOST_USER_FS_FLAG_MAP_W;
- }
msg.fd_offset[0] = foffset;
msg.len[0] = len;
@@ -2223,7 +2223,10 @@ static void lo_setupmapping(fuse_req_t r
* TODO: O_RDWR might not be allowed if file is read only or
* write only. Fix it.
*/
- fd = openat(lo->proc_self_fd, buf, O_RDWR);
+ if (writable)
+ fd = openat(lo->proc_self_fd, buf, O_RDWR);
+ else
+ fd = openat(lo->proc_self_fd, buf, O_RDONLY);
free(buf);
if (fd == -1)
return (void) fuse_reply_err(req, errno);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: During setup mapping, open file O_RDWR only if needed
2019-07-24 21:10 [Virtio-fs] [PATCH] virtiofsd: During setup mapping, open file O_RDWR only if needed Vivek Goyal
@ 2019-07-24 23:29 ` Liu Bo
2019-07-25 13:27 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 5+ messages in thread
From: Liu Bo @ 2019-07-24 23:29 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs-list
On Wed, Jul 24, 2019 at 05:10:24PM -0400, Vivek Goyal wrote:
> As of now we always open file O_RDWR (even if writable mappings are not
> required). This leads to copy up of file if file is backed by overlayfs
> and hence nullying advantages of overlayfs.
>
> So open file O_RDONLY if writable mappings are not required. Open O_RDWR
> if writable mappings are needed.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> contrib/virtiofsd/passthrough_ll.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> Index: qemu/contrib/virtiofsd/passthrough_ll.c
> ===================================================================
> --- qemu.orig/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:31:07.014871768 -0400
> +++ qemu/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:32:10.064412722 -0400
> @@ -2197,15 +2197,15 @@ static void lo_setupmapping(fuse_req_t r
> VhostUserFSSlaveMsg msg = { 0 };
> uint64_t vhu_flags;
> char *buf;
> + bool writable = flags & FUSE_SETUPMAPPING_FLAG_WRITE;
typo? flags is set to O_WRONLY or 0 in do_lookup, although this may work as same.
thanks,
-liubo
>
> if (lo_debug(req))
> fuse_debug("lo_setupmapping(ino=%" PRIu64 ", fi=0x%p)\n", ino,
> (void *)fi);
>
> vhu_flags = VHOST_USER_FS_FLAG_MAP_R;
> - if (flags & O_WRONLY) {
> + if (writable)
> vhu_flags |= VHOST_USER_FS_FLAG_MAP_W;
> - }
>
> msg.fd_offset[0] = foffset;
> msg.len[0] = len;
> @@ -2223,7 +2223,10 @@ static void lo_setupmapping(fuse_req_t r
> * TODO: O_RDWR might not be allowed if file is read only or
> * write only. Fix it.
> */
> - fd = openat(lo->proc_self_fd, buf, O_RDWR);
> + if (writable)
> + fd = openat(lo->proc_self_fd, buf, O_RDWR);
> + else
> + fd = openat(lo->proc_self_fd, buf, O_RDONLY);
> free(buf);
> if (fd == -1)
> return (void) fuse_reply_err(req, errno);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: During setup mapping, open file O_RDWR only if needed
2019-07-24 23:29 ` Liu Bo
@ 2019-07-25 13:27 ` Dr. David Alan Gilbert
2019-07-25 15:21 ` Vivek Goyal
0 siblings, 1 reply; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2019-07-25 13:27 UTC (permalink / raw)
To: Liu Bo; +Cc: virtio-fs-list, Vivek Goyal
* Liu Bo (bo.liu@linux.alibaba.com) wrote:
> On Wed, Jul 24, 2019 at 05:10:24PM -0400, Vivek Goyal wrote:
> > As of now we always open file O_RDWR (even if writable mappings are not
> > required). This leads to copy up of file if file is backed by overlayfs
> > and hence nullying advantages of overlayfs.
> >
> > So open file O_RDONLY if writable mappings are not required. Open O_RDWR
> > if writable mappings are needed.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > contrib/virtiofsd/passthrough_ll.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > Index: qemu/contrib/virtiofsd/passthrough_ll.c
> > ===================================================================
> > --- qemu.orig/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:31:07.014871768 -0400
> > +++ qemu/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:32:10.064412722 -0400
> > @@ -2197,15 +2197,15 @@ static void lo_setupmapping(fuse_req_t r
> > VhostUserFSSlaveMsg msg = { 0 };
> > uint64_t vhu_flags;
> > char *buf;
> > + bool writable = flags & FUSE_SETUPMAPPING_FLAG_WRITE;
>
> typo? flags is set to O_WRONLY or 0 in do_lookup, although this may work as same.
Right; that should be O_WRONLY;
do_setupmapping does:
genflags = 0;
genflags |= (arg->flags & FUSE_SETUPMAPPING_FLAG_WRITE) ? O_WRONLY : 0;
to convert from the fuse definition on the wire to the O_ notation
before calling the filesstem code.
Dave
> thanks,
> -liubo
> >
> > if (lo_debug(req))
> > fuse_debug("lo_setupmapping(ino=%" PRIu64 ", fi=0x%p)\n", ino,
> > (void *)fi);
> >
> > vhu_flags = VHOST_USER_FS_FLAG_MAP_R;
> > - if (flags & O_WRONLY) {
> > + if (writable)
> > vhu_flags |= VHOST_USER_FS_FLAG_MAP_W;
> > - }
> >
> > msg.fd_offset[0] = foffset;
> > msg.len[0] = len;
> > @@ -2223,7 +2223,10 @@ static void lo_setupmapping(fuse_req_t r
> > * TODO: O_RDWR might not be allowed if file is read only or
> > * write only. Fix it.
> > */
> > - fd = openat(lo->proc_self_fd, buf, O_RDWR);
> > + if (writable)
> > + fd = openat(lo->proc_self_fd, buf, O_RDWR);
> > + else
> > + fd = openat(lo->proc_self_fd, buf, O_RDONLY);
> > free(buf);
> > if (fd == -1)
> > return (void) fuse_reply_err(req, errno);
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: During setup mapping, open file O_RDWR only if needed
2019-07-25 13:27 ` Dr. David Alan Gilbert
@ 2019-07-25 15:21 ` Vivek Goyal
2019-07-25 15:48 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 5+ messages in thread
From: Vivek Goyal @ 2019-07-25 15:21 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: virtio-fs-list
On Thu, Jul 25, 2019 at 02:27:43PM +0100, Dr. David Alan Gilbert wrote:
> * Liu Bo (bo.liu@linux.alibaba.com) wrote:
> > On Wed, Jul 24, 2019 at 05:10:24PM -0400, Vivek Goyal wrote:
> > > As of now we always open file O_RDWR (even if writable mappings are not
> > > required). This leads to copy up of file if file is backed by overlayfs
> > > and hence nullying advantages of overlayfs.
> > >
> > > So open file O_RDONLY if writable mappings are not required. Open O_RDWR
> > > if writable mappings are needed.
> > >
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > > contrib/virtiofsd/passthrough_ll.c | 9 ++++++---
> > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > Index: qemu/contrib/virtiofsd/passthrough_ll.c
> > > ===================================================================
> > > --- qemu.orig/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:31:07.014871768 -0400
> > > +++ qemu/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:32:10.064412722 -0400
> > > @@ -2197,15 +2197,15 @@ static void lo_setupmapping(fuse_req_t r
> > > VhostUserFSSlaveMsg msg = { 0 };
> > > uint64_t vhu_flags;
> > > char *buf;
> > > + bool writable = flags & FUSE_SETUPMAPPING_FLAG_WRITE;
> >
> > typo? flags is set to O_WRONLY or 0 in do_lookup, although this may work as same.
>
> Right; that should be O_WRONLY;
> do_setupmapping does:
>
> genflags = 0;
> genflags |= (arg->flags & FUSE_SETUPMAPPING_FLAG_WRITE) ? O_WRONLY : 0;
>
> to convert from the fuse definition on the wire to the O_ notation
> before calling the filesstem code.
So I have two questions.
- O_WRONLY does not sound like correct mapping. FUSE_SETUPMAPPING_FLAG_WRITE
only says that write behavior is required. It does not say *write only*.
- Is it must that we need to translate fuse message specific flag
to O_* notation. How about let filesystem code parse it?
Thanks
Vivek
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: During setup mapping, open file O_RDWR only if needed
2019-07-25 15:21 ` Vivek Goyal
@ 2019-07-25 15:48 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2019-07-25 15:48 UTC (permalink / raw)
To: Vivek Goyal; +Cc: virtio-fs-list
* Vivek Goyal (vgoyal@redhat.com) wrote:
> On Thu, Jul 25, 2019 at 02:27:43PM +0100, Dr. David Alan Gilbert wrote:
> > * Liu Bo (bo.liu@linux.alibaba.com) wrote:
> > > On Wed, Jul 24, 2019 at 05:10:24PM -0400, Vivek Goyal wrote:
> > > > As of now we always open file O_RDWR (even if writable mappings are not
> > > > required). This leads to copy up of file if file is backed by overlayfs
> > > > and hence nullying advantages of overlayfs.
> > > >
> > > > So open file O_RDONLY if writable mappings are not required. Open O_RDWR
> > > > if writable mappings are needed.
> > > >
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > ---
> > > > contrib/virtiofsd/passthrough_ll.c | 9 ++++++---
> > > > 1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > Index: qemu/contrib/virtiofsd/passthrough_ll.c
> > > > ===================================================================
> > > > --- qemu.orig/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:31:07.014871768 -0400
> > > > +++ qemu/contrib/virtiofsd/passthrough_ll.c 2019-07-24 16:32:10.064412722 -0400
> > > > @@ -2197,15 +2197,15 @@ static void lo_setupmapping(fuse_req_t r
> > > > VhostUserFSSlaveMsg msg = { 0 };
> > > > uint64_t vhu_flags;
> > > > char *buf;
> > > > + bool writable = flags & FUSE_SETUPMAPPING_FLAG_WRITE;
> > >
> > > typo? flags is set to O_WRONLY or 0 in do_lookup, although this may work as same.
> >
> > Right; that should be O_WRONLY;
> > do_setupmapping does:
> >
> > genflags = 0;
> > genflags |= (arg->flags & FUSE_SETUPMAPPING_FLAG_WRITE) ? O_WRONLY : 0;
> >
> > to convert from the fuse definition on the wire to the O_ notation
> > before calling the filesstem code.
>
> So I have two questions.
>
> - O_WRONLY does not sound like correct mapping. FUSE_SETUPMAPPING_FLAG_WRITE
> only says that write behavior is required. It does not say *write only*.
True, we should make it end up as O_RDONLY or O_RDWR.
> - Is it must that we need to translate fuse message specific flag
> to O_* notation. How about let filesystem code parse it?
I think all of the decoding of wire protocol is done in fuse_lowlevel,
it tries not to pass protocol down.
Dave
> Thanks
> Vivek
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-25 15:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-24 21:10 [Virtio-fs] [PATCH] virtiofsd: During setup mapping, open file O_RDWR only if needed Vivek Goyal
2019-07-24 23:29 ` Liu Bo
2019-07-25 13:27 ` Dr. David Alan Gilbert
2019-07-25 15:21 ` Vivek Goyal
2019-07-25 15:48 ` 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.