ZFS Reads may result in unneccesary calls to zil_commit

ZFS supports O_RSYNC for read operations and when specified will ensure the same level of data integrity that O_DSYNC and O_SYNC provides for writes. O_RSYNC by itself has no effect so it must be combined with either O_DSYNC or O_SYNC. However, many platforms don't support O_RSYNC and have mapped O_SYNC to mean O_RSYNC within ZFS. This is incorrect and causes unnecessary calls to zil_commit. Only platforms which support O_RSYNC should implement the zil_commit functionality in the read code path. Reviewed-by: Matt Ahrens <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: George Wilson <[email protected]> Closes openzfs#8523
datto · Mar 22, 2019 · 2efea7c · 2efea7c
1 parent 060f022
commit 2efea7c
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 3 deletions.
diff --git a/include/spl/sys/vnode.h b/include/spl/sys/vnode.h
@@ -58,7 +58,6 @@
 #define	FOFFMAX		O_LARGEFILE
 #define	FSYNC		O_SYNC
 #define	FDSYNC		O_DSYNC
-#define	FRSYNC		O_SYNC
 #define	FEXCL		O_EXCL
 #define	FDIRECT		O_DIRECT
 #define	FAPPEND		O_APPEND

diff --git a/lib/libspl/include/sys/file.h b/lib/libspl/include/sys/file.h
@@ -40,7 +40,6 @@
 #define	FOFFMAX	O_LARGEFILE
 #define	FSYNC	O_SYNC
 #define	FDSYNC	O_DSYNC
-#define	FRSYNC	O_RSYNC
 #define	FEXCL	O_EXCL
 
 #define	FNODSYNC	0x10000	/* fsync pseudo flag */

diff --git a/module/zfs/zfs_vnops.c b/module/zfs/zfs_vnops.c
@@ -439,6 +439,7 @@ int
 zfs_read(struct inode *ip, uio_t *uio, int ioflag, cred_t *cr)
 {
 	int error = 0;
+	boolean_t frsync = B_FALSE;
 
 	znode_t *zp = ITOZ(ip);
 	zfsvfs_t *zfsvfs = ITOZSB(ip);
@@ -466,12 +467,19 @@ zfs_read(struct inode *ip, uio_t *uio, int ioflag, cred_t *cr)
 		return (0);
 	}
 
+#ifdef FRSYNC
 	/*
 	 * If we're in FRSYNC mode, sync out this znode before reading it.
 	 * Only do this for non-snapshots.
+	 *
+	 * Some platforms do not support FRSYNC and instead map it
+	 * to FSYNC, which results in unnecessary calls to zil_commit. We
+	 * only honor FRSYNC requests on platforms which support it.
 	 */
+	frsync = !!(ioflag & FRSYNC);
+#endif
 	if (zfsvfs->z_log &&
-	    (ioflag & FRSYNC || zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS))
+	    (frsync || zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS))
 		zil_commit(zfsvfs->z_log, zp->z_id);
 
 	/*