[Virtio-fs] [PATCH 0/2][V2] virtiofsd: Do not setup mapping read-write always

[Virtio-fs] [PATCH 0/2][V2] virtiofsd: Do not setup mapping read-write always

[Vivek Goyal]

Setup mapping either read-only or read-write depending on what client
has asked for. Also open file O_RDONLY for read-only mapping (and not
O_RDWR). Otherwise files always get copied up on overlayfs.

Vivek Goyal (2):
  virtiofsd: During setup mapping, open file O_RDWR only if needed
  virtiofsd: No need to promote O_WRONLY to O_RDWR

 contrib/virtiofsd/fuse_lowlevel.c  |  5 +++--
 contrib/virtiofsd/passthrough_ll.c | 29 +++++++----------------------
 2 files changed, 10 insertions(+), 24 deletions(-)

-- 
2.17.2

[Virtio-fs] [PATCH 1/2] virtiofsd: During setup mapping, open file O_RDWR only if needed

[Vivek Goyal]

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/fuse_lowlevel.c  |  5 +++--
 contrib/virtiofsd/passthrough_ll.c | 17 +++++++----------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
index 8adc4b1ab8..2ed894563b 100644
--- a/contrib/virtiofsd/fuse_lowlevel.c
+++ b/contrib/virtiofsd/fuse_lowlevel.c
@@ -1891,8 +1891,9 @@ static void do_setupmapping(fuse_req_t req, fuse_ino_t nodeid,
         // for now just use O_ flags
         uint64_t genflags;
 
-        genflags = 0;
-        genflags |= (arg->flags & FUSE_SETUPMAPPING_FLAG_WRITE) ? O_WRONLY : 0;
+	genflags = O_RDONLY;
+	if (arg->flags & FUSE_SETUPMAPPING_FLAG_WRITE)
+		genflags = O_RDWR;
 
 	if (req->se->op.setupmapping) {
 		/*
diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index 9ae1381618..c09c46a7e5 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -2197,15 +2197,17 @@ static void lo_setupmapping(fuse_req_t req, fuse_ino_t ino, uint64_t foffset,
         VhostUserFSSlaveMsg msg = { 0 };
         uint64_t vhu_flags;
 	char *buf;
+	bool writable = flags & O_RDWR;
 
 	if (lo_debug(req))
-		fuse_debug("lo_setupmapping(ino=%" PRIu64 ", fi=0x%p)\n", ino,
-			   (void *)fi);
+		fuse_debug("lo_setupmapping(ino=%" PRIu64 ", fi=0x%p,"
+			   " foffset=%" PRIu64 ", len=%" PRIu64
+			   ", moffset=%" PRIu64 ", flags=%" PRIu64 ")\n", ino,
+			   (void *)fi, foffset, len, moffset, flags);
 
         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;
@@ -2219,15 +2221,10 @@ static void lo_setupmapping(fuse_req_t req, fuse_ino_t ino, uint64_t foffset,
 		if (res == -1)
 			return (void) fuse_reply_err(req, errno);
 
-		/*
-		 * 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);
+		fd = openat(lo->proc_self_fd, buf, flags);
 		free(buf);
 		if (fd == -1)
 			return (void) fuse_reply_err(req, errno);
-
 	}
 
         if (fuse_virtio_map(req, &msg, fd)) {
-- 
2.17.2

[Virtio-fs] [PATCH 2/2] virtiofsd: No need to promote O_WRONLY to O_RDWR

[Vivek Goyal]

I am not sure why do we still need this logic. mmap() needs file to be
opened for O_RDONLY atleast. And we open file either as O_RDONLY or
O_RDWR in lo_setupmapping() depending on if read-only or read-write
mapping is required.

So it should not matter how fd is opended during lo_create() or lo_open()
because we are not going to use that fd anyway for mmap().

So for now drop this logic. I ran pjdfstests and blogbench and both
ran successfully.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 contrib/virtiofsd/passthrough_ll.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
index c09c46a7e5..502ce50178 100644
--- a/contrib/virtiofsd/passthrough_ll.c
+++ b/contrib/virtiofsd/passthrough_ll.c
@@ -1513,12 +1513,6 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
 	if (err)
 		goto out;
 
-	/* Promote O_WRONLY to O_RDWR. Otherwise later mmap(PROT_WRITE) fails */
-	if ((fi->flags & O_ACCMODE) == O_WRONLY) {
-		fi->flags &= ~O_ACCMODE;
-		fi->flags |= O_RDWR;
-	}
-
 	fd = openat(lo_fd(req, parent), name,
 		    (fi->flags | O_CREAT) & ~O_NOFOLLOW, mode);
 	err = fd == -1 ? errno : 0;
@@ -1710,12 +1704,6 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
 		fuse_debug("lo_open(ino=%" PRIu64 ", flags=%d)\n",
 			   ino, fi->flags);
 
-	/* Promote O_WRONLY to O_RDWR. Otherwise later mmap(PROT_WRITE) fails */
-	if ((fi->flags & O_ACCMODE) == O_WRONLY) {
-		fi->flags &= ~O_ACCMODE;
-		fi->flags |= O_RDWR;
-	}
-
 	/* With writeback cache, kernel may send read requests even
 	   when userspace opened write-only */
 	if (lo->writeback && (fi->flags & O_ACCMODE) == O_WRONLY) {
-- 
2.17.2

Re: [Virtio-fs] [PATCH 2/2] virtiofsd: No need to promote O_WRONLY to O_RDWR

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> I am not sure why do we still need this logic. mmap() needs file to be
> opened for O_RDONLY atleast. And we open file either as O_RDONLY or
> O_RDWR in lo_setupmapping() depending on if read-only or read-write
> mapping is required.
> 
> So it should not matter how fd is opended during lo_create() or lo_open()
> because we are not going to use that fd anyway for mmap().
> 
> So for now drop this logic. I ran pjdfstests and blogbench and both
> ran successfully.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

So this is just removing your two commits:
988e23c0fb06be2636b4 virtiofsd: Promote O_WRONLY to O_RDWR in lo_create()
7ebaa054e7aba33e1239 virtiofsd: Open O_WRONLY files as O_RDWR

which I've done; and applied your other patch.

Dave


> ---
>  contrib/virtiofsd/passthrough_ll.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c b/contrib/virtiofsd/passthrough_ll.c
> index c09c46a7e5..502ce50178 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1513,12 +1513,6 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name,
>  	if (err)
>  		goto out;
>  
> -	/* Promote O_WRONLY to O_RDWR. Otherwise later mmap(PROT_WRITE) fails */
> -	if ((fi->flags & O_ACCMODE) == O_WRONLY) {
> -		fi->flags &= ~O_ACCMODE;
> -		fi->flags |= O_RDWR;
> -	}
> -
>  	fd = openat(lo_fd(req, parent), name,
>  		    (fi->flags | O_CREAT) & ~O_NOFOLLOW, mode);
>  	err = fd == -1 ? errno : 0;
> @@ -1710,12 +1704,6 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>  		fuse_debug("lo_open(ino=%" PRIu64 ", flags=%d)\n",
>  			   ino, fi->flags);
>  
> -	/* Promote O_WRONLY to O_RDWR. Otherwise later mmap(PROT_WRITE) fails */
> -	if ((fi->flags & O_ACCMODE) == O_WRONLY) {
> -		fi->flags &= ~O_ACCMODE;
> -		fi->flags |= O_RDWR;
> -	}
> -
>  	/* With writeback cache, kernel may send read requests even
>  	   when userspace opened write-only */
>  	if (lo->writeback && (fi->flags & O_ACCMODE) == O_WRONLY) {
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK