svn commit: r345722 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs

[Alan Somers]

Author: asomers
Date: Sat Mar 30 00:57:07 2019
New Revision: 345722
URL: https://svnweb.freebsd.org/changeset/base/345722

Log:
  fusefs: don't force direct io for files opened O_WRONLY
  
  Previously fusefs would treat any file opened O_WRONLY as though the
  FOPEN_DIRECT_IO flag were set, in an attempt to avoid issuing reads as part
  of a RMW write operation on a cached part of the file.  However, the FUSE
  protocol explicitly allows reads of write-only files for precisely that
  reason.
  
  Sponsored by:	The FreeBSD Foundation

Modified:
  projects/fuse2/sys/fs/fuse/fuse_file.c
  projects/fuse2/sys/fs/fuse/fuse_io.c
  projects/fuse2/sys/fs/fuse/fuse_vnops.c
  projects/fuse2/tests/sys/fs/fusefs/write.cc

Modified: projects/fuse2/sys/fs/fuse/fuse_file.c
==============================================================================
--- projects/fuse2/sys/fs/fuse/fuse_file.c	Sat Mar 30 00:54:01 2019	(r345721)
+++ projects/fuse2/sys/fs/fuse/fuse_file.c	Sat Mar 30 00:57:07 2019	(r345722)
@@ -144,16 +144,7 @@ fuse_filehandle_open(struct vnode *vp, fufh_type_t fuf
 
 	fuse_filehandle_init(vp, fufh_type, fufhp, foo->fh);
 
-	/*
-	 * For WRONLY opens, force DIRECT_IO.  This is necessary
-	 * since writing a partial block through the buffer cache
-	 * will result in a read of the block and that read won't
-	 * be allowed by the WRONLY open.
-	 */
-	if (fufh_type == FUFH_WRONLY)
-		fuse_vnode_open(vp, foo->open_flags | FOPEN_DIRECT_IO, td);
-	else
-		fuse_vnode_open(vp, foo->open_flags, td);
+	fuse_vnode_open(vp, foo->open_flags, td);
 
 out:
 	fdisp_destroy(&fdi);

Modified: projects/fuse2/sys/fs/fuse/fuse_io.c
==============================================================================
--- projects/fuse2/sys/fs/fuse/fuse_io.c	Sat Mar 30 00:54:01 2019	(r345721)
+++ projects/fuse2/sys/fs/fuse/fuse_io.c	Sat Mar 30 00:57:07 2019	(r345722)
@@ -648,6 +648,15 @@ fuse_io_strategy(struct vnode *vp, struct buf *bp)
 
 	error = fuse_filehandle_getrw(vp,
 	    (bp->b_iocmd == BIO_READ) ? FUFH_RDONLY : FUFH_WRONLY, &fufh);
+	if (bp->b_iocmd == BIO_READ && error == EBADF) {
+		/* 
+		 * This may be a read-modify-write operation on a cached file
+		 * opened O_WRONLY.  The FUSE protocol allows this.
+		 *
+		 * TODO: eliminate this hacky check once the FUFH table is gone
+		 */
+		error = fuse_filehandle_get(vp, FUFH_WRONLY, &fufh);
+	}
 	if (error) {
 		printf("FUSE: strategy: filehandles are closed\n");
 		bp->b_ioflags |= BIO_ERROR;

Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c
==============================================================================
--- projects/fuse2/sys/fs/fuse/fuse_vnops.c	Sat Mar 30 00:54:01 2019	(r345721)
+++ projects/fuse2/sys/fs/fuse/fuse_vnops.c	Sat Mar 30 00:57:07 2019	(r345722)
@@ -1208,7 +1208,6 @@ fuse_vnop_open(struct vop_open_args *ap)
 	int mode = ap->a_mode;
 	struct thread *td = ap->a_td;
 	struct ucred *cred = ap->a_cred;
-	int32_t fuse_open_flags = 0;
 
 	fufh_type_t fufh_type;
 	struct fuse_vnode_data *fvdat;
@@ -1228,16 +1227,8 @@ fuse_vnop_open(struct vop_open_args *ap)
 		fufh_type = fuse_filehandle_xlate_from_fflags(mode);
 	}
 
-	/*
-	 * For WRONLY opens, force DIRECT_IO.  This is necessary since writing
-	 * a partial block through the buffer cache will result in a read of
-	 * the block and that read won't be allowed by the WRONLY open.
-	 */
-	if (fufh_type == FUFH_WRONLY || (fvdat->flag & FN_DIRECTIO) != 0)
-		fuse_open_flags = FOPEN_DIRECT_IO;
-
 	if (fuse_filehandle_validrw(vp, fufh_type) != FUFH_INVALID) {
-		fuse_vnode_open(vp, fuse_open_flags, td);
+		fuse_vnode_open(vp, 0, td);
 		return 0;
 	}
 

Modified: projects/fuse2/tests/sys/fs/fusefs/write.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/write.cc	Sat Mar 30 00:54:01 2019	(r345721)
+++ projects/fuse2/tests/sys/fs/fusefs/write.cc	Sat Mar 30 00:57:07 2019	(r345722)
@@ -391,7 +391,7 @@ TEST_F(Write, DISABLED_mmap)
 	free(zeros);
 }
 
-TEST_F(Write, pwrite)
+TEST_F(WriteThrough, pwrite)
 {
 	const char FULLPATH[] = "mountpoint/some_file.txt";
 	const char RELPATH[] = "some_file.txt";
@@ -519,6 +519,36 @@ TEST_F(WriteBack, close)
 
 	ASSERT_EQ(bufsize, write(fd, CONTENTS, bufsize)) << strerror(errno);
 	close(fd);
+}
+
+/*
+ * In writeback mode, writes to an O_WRONLY file could trigger reads from the
+ * server.  The FUSE protocol explicitly allows that.
+ */
+TEST_F(WriteBack, rmw)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const char *CONTENTS = "abcdefgh";
+	const char *INITIAL   = "XXXXXXXXXX";
+	uint64_t ino = 42;
+	uint64_t offset = 1;
+	off_t fsize = 10;
+	int fd;
+	ssize_t bufsize = strlen(CONTENTS);
+
+	expect_lookup(RELPATH, ino, 0);
+	expect_open(ino, 0, 1);
+	expect_getattr(ino, fsize);
+	expect_read(ino, 0, fsize, fsize, INITIAL);
+	expect_write(ino, offset, bufsize, bufsize, 0, CONTENTS);
+
+	fd = open(FULLPATH, O_WRONLY);
+	EXPECT_LE(0, fd) << strerror(errno);
+
+	ASSERT_EQ(bufsize, pwrite(fd, CONTENTS, bufsize, offset))
+		<< strerror(errno);
+	/* Deliberately leak fd.  close(2) will be tested in release.cc */
 }
 
 /*