vfs: Non-blockling buffered fs read (page cache only)

New syscalls that take an flag argument. This change does not add any specific flags.

Signed-off-by Milosz Tanski <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/read_write.c                   | 176 ++++++++++++++++++++++++++++++--------
 include/linux/compat.h            |   6 ++
 include/linux/syscalls.h          |   6 ++
 include/uapi/asm-generic/unistd.h |   6 +-
 mm/filemap.c                      |   5 +-
 5 files changed, 158 insertions(+), 41 deletions(-)

fs/read_write.c +2 -0

index 94b2d34..907735c 100644
@@ -866,6 +866,8 @@ ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
 		return -EBADF;
 	if (!(file->f_mode & FMODE_CAN_READ))
 		return -EINVAL;
+	if (flags & ~0)
+		return -EINVAL;
 
 	return do_readv_writev(READ, file, vec, vlen, pos, flags);
 }

Quoted 27 lines · Milosz Tanski

New syscalls that take an flag argument. This change does not add any specific flags.

Signed-off-by Milosz Tanski <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/read_write.c                   | 176 ++++++++++++++++++++++++++++++--------
 include/linux/compat.h            |   6 ++
 include/linux/syscalls.h          |   6 ++
 include/uapi/asm-generic/unistd.h |   6 +-
 mm/filemap.c                      |   5 +-
 5 files changed, 158 insertions(+), 41 deletions(-)

fs/read_write.c +2 -0

index 94b2d34..907735c 100644
@@ -866,6 +866,8 @@ ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
              return -EBADF;
      if (!(file->f_mode & FMODE_CAN_READ))
              return -EINVAL;
+     if (flags & ~0)
+             return -EINVAL;
 
      return do_readv_writev(READ, file, vec, vlen, pos, flags);
 }

fs/read_write.c ssize_t vfs_writev(struct file *file, const struct iovec __user *vec, · line 888

Quoted 8 lines

fs/read_write.c +2 -0

@@ -879,21 +881,23 @@ ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
              return -EBADF;
      if (!(file->f_mode & FMODE_CAN_WRITE))
              return -EINVAL;
+     if (flags & ~0)
+             return -EINVAL;
 
      return do_readv_writev(WRITE, file, vec, vlen, pos, flags);
 }

Hi, Milosz,

You've checked for invalid flags for the normal system calls, but not for the compat variants. Can you add that in, please?

Thanks!
Jeff

generic_file_read_iter() supports a new flag RWF_NONBLOCK which says that we only want to read the data if it's already in the page cache.

Additionally, there are a few filesystems that we have to specifically bail early if RWF_NONBLOCK because the op would block. Christoph Hellwig contributed this code.

Signed-off-by Milosz Tanski <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Reviewed-by Jeff Moyer <[email protected]>

---
 fs/ceph/file.c     |  2 ++
 fs/cifs/file.c     |  6 ++++++
 fs/nfs/file.c      |  5 ++++-
 fs/ocfs2/file.c    |  6 ++++++
 fs/pipe.c          |  3 ++-
 fs/read_write.c    | 38 +++++++++++++++++++++++++-------------
 fs/xfs/xfs_file.c  |  4 ++++
 include/linux/fs.h |  3 +++
 mm/filemap.c       | 18 ++++++++++++++++++
 mm/shmem.c         |  4 ++++
 10 files changed, 74 insertions(+), 15 deletions(-)

fs/ceph/file.c +2 -0

index d7e0da8..b798b5c 100644
@@ -822,6 +822,8 @@ again:
 	if ((got & (CEPH_CAP_FILE_CACHE|CEPH_CAP_FILE_LAZYIO)) == 0 ||
 	    (iocb->ki_filp->f_flags & O_DIRECT) ||
 	    (fi->flags & CEPH_F_SYNC)) {
+		if (iocb->ki_rwflags & O_NONBLOCK)
+			return -EAGAIN;
 
 		dout("aio_sync_read %p %llx.%llx %llu~%u got cap refs on %s\n",
 		     inode, ceph_vinop(inode), iocb->ki_pos, (unsigned)len,

fs/cifs/file.c +6 -0

index 3e4d00a..c485afa 100644
@@ -3005,6 +3005,9 @@ ssize_t cifs_user_readv(struct kiocb *iocb, struct iov_iter *to)
 	struct cifs_readdata *rdata, *tmp;
 	struct list_head rdata_list;
 
+	if (iocb->ki_rwflags & RWF_NONBLOCK)
+		return -EAGAIN;
+
 	len = iov_iter_count(to);
 	if (!len)
 		return 0;

@@ -3123,6 +3126,9 @@ cifs_strict_readv(struct kiocb *iocb, struct iov_iter *to)
 	    ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NOPOSIXBRL) == 0))
 		return generic_file_read_iter(iocb, to);
 
+	if (iocb->ki_rwflags & RWF_NONBLOCK)
+		return -EAGAIN;
+
 	/*
 	 * We need to hold the sem to be sure nobody modifies lock list
 	 * with a brlock that prevents reading.

fs/nfs/file.c +4 -1

index 2ab6f00..aa9046f 100644
@@ -171,8 +171,11 @@ nfs_file_read(struct kiocb *iocb, struct iov_iter *to)
 	struct inode *inode = file_inode(iocb->ki_filp);
 	ssize_t result;
 
-	if (iocb->ki_filp->f_flags & O_DIRECT)
+	if (iocb->ki_filp->f_flags & O_DIRECT) {
+		if (iocb->ki_rwflags & O_NONBLOCK)
+			return -EAGAIN;
 		return nfs_file_direct_read(iocb, to, iocb->ki_pos);
+	}
 
 	dprintk("NFS: read(%pD2, %zu@%lu)\n",
 		iocb->ki_filp,

fs/ocfs2/file.c +6 -0

index 324dc93..bb66ca4 100644
@@ -2472,6 +2472,12 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
 			filp->f_path.dentry->d_name.name,
 			to->nr_segs);	/* GRRRRR */
 
+	/*
+	 * No non-blocking reads for ocfs2 for now.  Might be doable with
+	 * non-blocking cluster lock helpers.
+	 */
+	if (iocb->ki_rwflags & RWF_NONBLOCK)
+		return -EAGAIN;
 
 	if (!inode) {
 		ret = -EINVAL;

fs/pipe.c +2 -1

index 21981e5..212bf68 100644
@@ -302,7 +302,8 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to)
 			 */
 			if (ret)
 				break;
-			if (filp->f_flags & O_NONBLOCK) {
+			if ((filp->f_flags & O_NONBLOCK) ||
+			    (iocb->ki_rwflags & RWF_NONBLOCK)) {
 				ret = -EAGAIN;
 				break;
 			}

fs/read_write.c +25 -13

index 907735c..cba7d4c 100644
@@ -835,14 +835,19 @@ static ssize_t do_readv_writev(int type, struct file *file,
 		file_start_write(file);
 	}
 
-	if (iter_fn)
+	if (iter_fn) {
 		ret = do_iter_readv_writev(file, type, iov, nr_segs, tot_len,
 						pos, iter_fn, flags);
-	else if (fnv)
-		ret = do_sync_readv_writev(file, iov, nr_segs, tot_len,
-						pos, fnv);
-	else
-		ret = do_loop_readv_writev(file, iov, nr_segs, pos, fn);
+	} else {
+		if (type == READ && (flags & RWF_NONBLOCK))
+			return -EAGAIN;
+
+		if (fnv)
+			ret = do_sync_readv_writev(file, iov, nr_segs, tot_len,
+							pos, fnv);
+		else
+			ret = do_loop_readv_writev(file, iov, nr_segs, pos, fn);
+	}
 
 	if (type != READ)
 		file_end_write(file);

@@ -866,8 +871,10 @@ ssize_t vfs_readv(struct file *file, const struct iovec __user *vec,
 		return -EBADF;
 	if (!(file->f_mode & FMODE_CAN_READ))
 		return -EINVAL;
-	if (flags & ~0)
+	if (flags & ~RWF_NONBLOCK)
 		return -EINVAL;
+	if ((file->f_flags & O_DIRECT) && (flags & RWF_NONBLOCK))
+		return -EAGAIN;
 
 	return do_readv_writev(READ, file, vec, vlen, pos, flags);
 }

@@ -1069,14 +1076,19 @@ static ssize_t compat_do_readv_writev(int type, struct file *file,
 		file_start_write(file);
 	}
 
-	if (iter_fn)
+	if (iter_fn) {
 		ret = do_iter_readv_writev(file, type, iov, nr_segs, tot_len,
 						pos, iter_fn, flags);
-	else if (fnv)
-		ret = do_sync_readv_writev(file, iov, nr_segs, tot_len,
-						pos, fnv);
-	else
-		ret = do_loop_readv_writev(file, iov, nr_segs, pos, fn);
+	} else {
+		if (type == READ && (flags & RWF_NONBLOCK))
+			return -EAGAIN;
+
+		if (fnv)
+			ret = do_sync_readv_writev(file, iov, nr_segs, tot_len,
+							pos, fnv);
+		else
+			ret = do_loop_readv_writev(file, iov, nr_segs, pos, fn);
+	}
 
 	if (type != READ)
 		file_end_write(file);

fs/xfs/xfs_file.c +4 -0

index eb596b4..b1f6334 100644
@@ -246,6 +246,10 @@ xfs_file_read_iter(
 
 	XFS_STATS_INC(xs_read_calls);
 
+	/* XXX: need a non-blocking iolock helper, shouldn't be too hard */
+	if (iocb->ki_rwflags & RWF_NONBLOCK)
+		return -EAGAIN;
+
 	if (unlikely(file->f_flags & O_DIRECT))
 		ioflags |= XFS_IO_ISDIRECT;
 	if (file->f_mode & FMODE_NOCMTIME)

include/linux/fs.h +3 -0

index 9ed5711..eaebd99 100644
@@ -1459,6 +1459,9 @@ struct block_device_operations;
 #define HAVE_COMPAT_IOCTL 1
 #define HAVE_UNLOCKED_IOCTL 1
 
+/* These flags are used for the readv/writev syscalls with flags. */
+#define RWF_NONBLOCK 0x00000001
+
 struct iov_iter;
 
 struct file_operations {

mm/filemap.c +18 -0

index 530c263..09d3af3 100644
@@ -1494,6 +1494,8 @@ static ssize_t do_generic_file_read(struct file *filp, loff_t *ppos,
 find_page:
 		page = find_get_page(mapping, index);
 		if (!page) {
+			if (flags & RWF_NONBLOCK)
+				goto would_block;
 			page_cache_sync_readahead(mapping,
 					ra, filp,
 					index, last_index - index);

@@ -1585,6 +1587,11 @@ page_ok:
 		continue;
 
 page_not_up_to_date:
+		if (flags & RWF_NONBLOCK) {
+			page_cache_release(page);
+			goto would_block;
+		}
+
 		/* Get exclusive access to the page ... */
 		error = lock_page_killable(page);
 		if (unlikely(error))

@@ -1604,6 +1611,12 @@ page_not_up_to_date_locked:
 			goto page_ok;
 		}
 
+		if (flags & RWF_NONBLOCK) {
+			unlock_page(page);
+			page_cache_release(page);
+			goto would_block;
+		}
+
 readpage:
 		/*
 		 * A previous I/O error may have been due to temporary

@@ -1674,6 +1687,8 @@ no_cached_page:
 		goto readpage;
 	}
 
+would_block:
+	error = -EAGAIN;
 out:
 	ra->prev_pos = prev_index;
 	ra->prev_pos <<= PAGE_CACHE_SHIFT;

@@ -1707,6 +1722,9 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		size_t count = iov_iter_count(iter);
 		loff_t size;
 
+		if (iocb->ki_rwflags & RWF_NONBLOCK)
+			return -EAGAIN;
+
 		if (!count)
 			goto out; /* skip atime */
 		size = i_size_read(inode);

mm/shmem.c +4 -0

index cd6fc75..5c30f04 100644
@@ -1531,6 +1531,10 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	ssize_t retval = 0;
 	loff_t *ppos = &iocb->ki_pos;
 
+	/* XXX: should be easily supportable */
+	if (iocb->ki_rwflags & RWF_NONBLOCK)
+		return -EAGAIN;
+
 	/*
 	 * Might this read be for a stacking filesystem?  Then when reading
 	 * holes of a sparse file, we actually need to allocate those pages,

Signature · 6 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Christoph Hellwig <[email protected]>

Clean up the generic_write_sync by just passing an iocb and a bytes written / negative errno argument. In addition to simplifying the callers this also prepares for passing a per-operation O_DSYNC flag. Two callers didn't quite fit that scheme:

- dio_complete didn't both to update ki_pos as we don't need it on a iocb that is about to be freed, so we had to add it. Additionally it also synced out written data in the error case, which has been changed to operate like the other callers.
- gfs2 also used generic_write_sync to implement a crude version of fallocate. It has been switched to use an open coded variant instead.

Signed-off-by Christoph Hellwig <[email protected]>

---
  fs/block_dev.c     |  8 +-------
  fs/btrfs/file.c    |  7 ++-----
  fs/cifs/file.c     |  8 +-------
  fs/direct-io.c     |  8 ++------
  fs/ext4/file.c     |  8 +-------
  fs/gfs2/file.c     |  9 +++++++--
  fs/ntfs/file.c     |  8 ++------
  fs/udf/file.c      | 11 ++---------
  fs/xfs/xfs_file.c  |  8 +-------
  include/linux/fs.h |  8 +-------
  mm/filemap.c       | 30 ++++++++++++++++++++----------
  11 files changed, 40 insertions(+), 73 deletions(-)

fs/block_dev.c +1 -7

index cc9d411..c529b1c 100644
@@ -1568,18 +1568,12 @@ static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
   */
  ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
  {
-	struct file *file = iocb->ki_filp;
  	struct blk_plug plug;
  	ssize_t ret;
 
  	blk_start_plug(&plug);
  	ret = __generic_file_write_iter(iocb, from);
-	if (ret > 0) {
-		ssize_t err;
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
+	ret = generic_write_sync(iocb, ret);
  	blk_finish_plug(&plug);
  	return ret;
  }

fs/btrfs/file.c +2 -5

index a18ceab..4f4a6f7 100644
@@ -1820,11 +1820,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
  	 */
  	BTRFS_I(inode)->last_trans = root->fs_info->generation + 1;
  	BTRFS_I(inode)->last_sub_trans = root->log_transid;
-	if (num_written > 0) {
-		err = generic_write_sync(file, pos, num_written);
-		if (err < 0)
-			num_written = err;
-	}
+
+	num_written = generic_write_sync(iocb, num_written);
 
  	if (sync)
  		atomic_dec(&BTRFS_I(inode)->sync_writers);

fs/cifs/file.c +1 -7

index c485afa..32359de 100644
@@ -2706,13 +2706,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
  		rc = __generic_file_write_iter(iocb, from);
  		mutex_unlock(&inode->i_mutex);
 
-		if (rc > 0) {
-			ssize_t err;
-
-			err = generic_write_sync(file, iocb->ki_pos - rc, rc);
-			if (err < 0)
-				rc = err;
-		}
+		rc = generic_write_sync(iocb, rc);
  	} else {
  		mutex_unlock(&inode->i_mutex);
  	}

fs/direct-io.c +2 -6

index e181b6b..b72ac83 100644
@@ -257,12 +257,8 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret,
  	inode_dio_done(dio->inode);
  	if (is_async) {
  		if (dio->rw & WRITE) {
-			int err;
-
-			err = generic_write_sync(dio->iocb->ki_filp, offset,
-						 transferred);
-			if (err < 0 && ret > 0)
-				ret = err;
+			dio->iocb->ki_pos = offset + transferred;
+			ret = generic_write_sync(dio->iocb, ret);
  		}
 
  		aio_complete(dio->iocb, ret, 0);

fs/ext4/file.c +1 -7

index aca7b24..79b000c 100644
@@ -175,13 +175,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
  	ret = __generic_file_write_iter(iocb, from);
  	mutex_unlock(&inode->i_mutex);
 
-	if (ret > 0) {
-		ssize_t err;
-
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
+	ret = generic_write_sync(iocb, ret);
  	if (o_direct)
  		blk_finish_plug(&plug);
 

fs/gfs2/file.c +7 -2

index 80dd44d..3fafeca 100644
@@ -895,8 +895,13 @@ retry:
  		gfs2_quota_unlock(ip);
  	}
 
-	if (error == 0)
-		error = generic_write_sync(file, pos, count);
+	if (error)
+		goto out_unlock;
+
+	if ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host)) {
+		error = vfs_fsync_range(file, pos, pos + count - 1,
+			       (file->f_flags & __O_SYNC) ? 0 : 1);
+	}
  	goto out_unlock;
 
  out_trans_fail:

fs/ntfs/file.c +2 -6

index 643faa4..4f3d664 100644
@@ -2127,12 +2127,8 @@ static ssize_t ntfs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
  	mutex_lock(&inode->i_mutex);
  	ret = ntfs_file_aio_write_nolock(iocb, iov, nr_segs, &iocb->ki_pos);
  	mutex_unlock(&inode->i_mutex);
-	if (ret > 0) {
-		int err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
-	return ret;
+
+	return generic_write_sync(iocb, ret);
  }
 
  /**

fs/udf/file.c +2 -9

index bb15771..1cdabd0 100644
@@ -155,16 +155,9 @@ static ssize_t udf_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
  	retval = __generic_file_write_iter(iocb, from);
  	mutex_unlock(&inode->i_mutex);
 
-	if (retval > 0) {
-		ssize_t err;
-
+	if (retval > 0)
  		mark_inode_dirty(inode);
-		err = generic_write_sync(file, iocb->ki_pos - retval, retval);
-		if (err < 0)
-			retval = err;
-	}
-
-	return retval;
+	return generic_write_sync(iocb, retval);
  }
 
  long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)

fs/xfs/xfs_file.c +1 -7

index 0655915..a8cab66 100644
@@ -792,14 +792,8 @@ xfs_file_write_iter(
  		ret = xfs_file_buffered_aio_write(iocb, from);
 
  	if (ret > 0) {
-		ssize_t err;
-
  		XFS_STATS_ADD(xs_write_bytes, ret);
-
-		/* Handle various SYNC-type writes */
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
+		ret = generic_write_sync(iocb, ret);
  	}
  	return ret;
  }

include/linux/fs.h +1 -7

index eaebd99..7d0e116 100644
@@ -2242,13 +2242,7 @@ extern int filemap_fdatawrite_range(struct address_space *mapping,
  extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
  			   int datasync);
  extern int vfs_fsync(struct file *file, int datasync);
-static inline int generic_write_sync(struct file *file, loff_t pos, loff_t count)
-{
-	if (!(file->f_flags & O_DSYNC) && !IS_SYNC(file->f_mapping->host))
-		return 0;
-	return vfs_fsync_range(file, pos, pos + count - 1,
-			       (file->f_flags & __O_SYNC) ? 0 : 1);
-}
+extern int generic_write_sync(struct kiocb *iocb, loff_t count);
  extern void emergency_sync(void);
  extern void emergency_remount(void);
  #ifdef CONFIG_BLOCK

mm/filemap.c +20 -10

index 09d3af3..6107058 100644
@@ -2664,6 +2664,24 @@ out:
  }
  EXPORT_SYMBOL(__generic_file_write_iter);
 
+int generic_write_sync(struct kiocb *iocb, loff_t count)
+{
+	struct file *file = iocb->ki_filp;
+
+	if (count > 0 &&
+	    ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host))) {
+		bool fdatasync = !(file->f_flags & __O_SYNC);
+		ssize_t ret = 0;
+
+		ret = vfs_fsync_range(file, iocb->ki_pos - count,
+				iocb->ki_pos - 1, fdatasync);
+		if (ret < 0)
+			return ret;
+	}
+	return count;
+}
+EXPORT_SYMBOL(generic_write_sync);
+
  /**
   * generic_file_write_iter - write data to a file
   * @iocb:	IO state structure

@@ -2675,22 +2693,14 @@ EXPORT_SYMBOL(__generic_file_write_iter);
   */
  ssize_t generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
  {
-	struct file *file = iocb->ki_filp;
-	struct inode *inode = file->f_mapping->host;
+	struct inode *inode = iocb->ki_filp->f_mapping->host;
  	ssize_t ret;
 
  	mutex_lock(&inode->i_mutex);
  	ret = __generic_file_write_iter(iocb, from);
  	mutex_unlock(&inode->i_mutex);
 
-	if (ret > 0) {
-		ssize_t err;
-
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
-	return ret;
+	return generic_write_sync(iocb, ret);
  }
  EXPORT_SYMBOL(generic_file_write_iter);
 

From: Christoph Hellwig <[email protected]>

Clean up the generic_write_sync by just passing an iocb and a bytes written / negative errno argument. In addition to simplifying the callers this also prepares for passing a per-operation O_DSYNC flag. Two callers didn't quite fit that scheme:

- dio_complete didn't both to update ki_pos as we don't need it on a iocb that is about to be freed, so we had to add it. Additionally it also synced out written data in the error case, which has been changed to operate like the other callers.
- gfs2 also used generic_write_sync to implement a crude version of fallocate. It has been switched to use an open coded variant instead.

Signed-off-by Christoph Hellwig <[email protected]>

---

fs/block_dev.c     |  8 +-------
fs/btrfs/file.c    |  7 ++-----
fs/cifs/file.c     |  8 +-------
fs/direct-io.c     |  8 ++------
fs/ext4/file.c     |  8 +-------
fs/gfs2/file.c     |  9 +++++++--
fs/ntfs/file.c     |  8 ++------
fs/udf/file.c      | 11 ++---------
fs/xfs/xfs_file.c  |  8 +-------
include/linux/fs.h |  8 +-------
mm/filemap.c       | 30 ++++++++++++++++++++----------
11 files changed, 40 insertions(+), 73 deletions(-)

fs/block_dev.c +1 -7

index cc9d411..c529b1c 100644
@@ -1568,18 +1568,12 @@ static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 */
 ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	struct file *file = iocb->ki_filp;
 	struct blk_plug plug;
 	ssize_t ret;
 
 	blk_start_plug(&plug);
 	ret = __generic_file_write_iter(iocb, from);
-	if (ret > 0) {
-		ssize_t err;
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
+	ret = generic_write_sync(iocb, ret);
 	blk_finish_plug(&plug);
 	return ret;
 }

fs/btrfs/file.c +2 -5

index a18ceab..4f4a6f7 100644
@@ -1820,11 +1820,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	 */
 	BTRFS_I(inode)->last_trans = root->fs_info->generation + 1;
 	BTRFS_I(inode)->last_sub_trans = root->log_transid;
-	if (num_written > 0) {
-		err = generic_write_sync(file, pos, num_written);
-		if (err < 0)
-			num_written = err;
-	}
+
+	num_written = generic_write_sync(iocb, num_written);
 
 	if (sync)
 		atomic_dec(&BTRFS_I(inode)->sync_writers);

fs/cifs/file.c +1 -7

index c485afa..32359de 100644
@@ -2706,13 +2706,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 		rc = __generic_file_write_iter(iocb, from);
 		mutex_unlock(&inode->i_mutex);
 
-		if (rc > 0) {
-			ssize_t err;
-
-			err = generic_write_sync(file, iocb->ki_pos - rc, rc);
-			if (err < 0)
-				rc = err;
-		}
+		rc = generic_write_sync(iocb, rc);
 	} else {
 		mutex_unlock(&inode->i_mutex);
 	}

fs/direct-io.c +2 -6

index e181b6b..b72ac83 100644
@@ -257,12 +257,8 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret,
 	inode_dio_done(dio->inode);
 	if (is_async) {
 		if (dio->rw & WRITE) {
-			int err;
-
-			err = generic_write_sync(dio->iocb->ki_filp, offset,
-						 transferred);
-			if (err < 0 && ret > 0)
-				ret = err;
+			dio->iocb->ki_pos = offset + transferred;
+			ret = generic_write_sync(dio->iocb, ret);
 		}
 
 		aio_complete(dio->iocb, ret, 0);

fs/ext4/file.c +1 -7

index aca7b24..79b000c 100644
@@ -175,13 +175,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	ret = __generic_file_write_iter(iocb, from);
 	mutex_unlock(&inode->i_mutex);
 
-	if (ret > 0) {
-		ssize_t err;
-
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
+	ret = generic_write_sync(iocb, ret);
 	if (o_direct)
 		blk_finish_plug(&plug);
 

fs/gfs2/file.c +7 -2

index 80dd44d..3fafeca 100644
@@ -895,8 +895,13 @@ retry:
 		gfs2_quota_unlock(ip);
 	}
 
-	if (error == 0)
-		error = generic_write_sync(file, pos, count);
+	if (error)
+		goto out_unlock;
+
+	if ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host)) {
+		error = vfs_fsync_range(file, pos, pos + count - 1,
+			       (file->f_flags & __O_SYNC) ? 0 : 1);
+	}
 	goto out_unlock;
 

out_trans_fail:

fs/ntfs/file.c +2 -6

index 643faa4..4f3d664 100644
@@ -2127,12 +2127,8 @@ static ssize_t ntfs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
 	mutex_lock(&inode->i_mutex);
 	ret = ntfs_file_aio_write_nolock(iocb, iov, nr_segs, &iocb->ki_pos);
 	mutex_unlock(&inode->i_mutex);
-	if (ret > 0) {
-		int err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
-	return ret;
+
+	return generic_write_sync(iocb, ret);
 }
 
 /**

fs/udf/file.c +2 -9

index bb15771..1cdabd0 100644
@@ -155,16 +155,9 @@ static ssize_t udf_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	retval = __generic_file_write_iter(iocb, from);
 	mutex_unlock(&inode->i_mutex);
 
-	if (retval > 0) {
-		ssize_t err;
-
+	if (retval > 0)
 		mark_inode_dirty(inode);
-		err = generic_write_sync(file, iocb->ki_pos - retval, retval);
-		if (err < 0)
-			retval = err;
-	}
-
-	return retval;
+	return generic_write_sync(iocb, retval);
 }
 
 long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)

fs/xfs/xfs_file.c +1 -7

index 0655915..a8cab66 100644
@@ -792,14 +792,8 @@ xfs_file_write_iter(
 		ret = xfs_file_buffered_aio_write(iocb, from);
 
 	if (ret > 0) {
-		ssize_t err;
-
 		XFS_STATS_ADD(xs_write_bytes, ret);
-
-		/* Handle various SYNC-type writes */
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
+		ret = generic_write_sync(iocb, ret);
 	}
 	return ret;
 }

include/linux/fs.h +1 -7

index eaebd99..7d0e116 100644
@@ -2242,13 +2242,7 @@ extern int filemap_fdatawrite_range(struct address_space *mapping,
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
 			   int datasync);
 extern int vfs_fsync(struct file *file, int datasync);
-static inline int generic_write_sync(struct file *file, loff_t pos, loff_t count)
-{
-	if (!(file->f_flags & O_DSYNC) && !IS_SYNC(file->f_mapping->host))
-		return 0;
-	return vfs_fsync_range(file, pos, pos + count - 1,
-			       (file->f_flags & __O_SYNC) ? 0 : 1);
-}
+extern int generic_write_sync(struct kiocb *iocb, loff_t count);
 extern void emergency_sync(void);
 extern void emergency_remount(void);
 #ifdef CONFIG_BLOCK

mm/filemap.c +0 -0

index 09d3af3..6107058 100644
@@ -2664,6 +2664,24 @@ out:
 }

mm/filemap.c +12 -10

+
+		ret = vfs_fsync_range(file, iocb->ki_pos - count,
+				iocb->ki_pos - 1, fdatasync);
+		if (ret < 0)
+			return ret;
+	}
+	return count;
+}
+EXPORT_SYMBOL(generic_write_sync);
+
 /**
 * generic_file_write_iter - write data to a file
 * @iocb:	IO state structure

@@ -2675,22 +2693,14 @@ EXPORT_SYMBOL(__generic_file_write_iter);
 */
 ssize_t generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	struct file *file = iocb->ki_filp;
-	struct inode *inode = file->f_mapping->host;
+	struct inode *inode = iocb->ki_filp->f_mapping->host;
 	ssize_t ret;
 
 	mutex_lock(&inode->i_mutex);
 	ret = __generic_file_write_iter(iocb, from);
 	mutex_unlock(&inode->i_mutex);
 
-	if (ret > 0) {
-		ssize_t err;
-
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
-	return ret;
+	return generic_write_sync(iocb, ret);
 }
 EXPORT_SYMBOL(generic_file_write_iter);

Hi,

Quoted 240 lines · Milosz Tanski

From: Christoph Hellwig <[email protected]>

Clean up the generic_write_sync by just passing an iocb and a bytes written / negative errno argument. In addition to simplifying the callers this also prepares for passing a per-operation O_DSYNC flag. Two callers didn't quite fit that scheme:

- dio_complete didn't both to update ki_pos as we don't need it on a iocb that is about to be freed, so we had to add it. Additionally it also synced out written data in the error case, which has been changed to operate like the other callers.
- gfs2 also used generic_write_sync to implement a crude version of fallocate. It has been switched to use an open coded variant instead.

Signed-off-by Christoph Hellwig <[email protected]>

---

fs/block_dev.c     |  8 +-------
fs/btrfs/file.c    |  7 ++-----
fs/cifs/file.c     |  8 +-------
fs/direct-io.c     |  8 ++------
fs/ext4/file.c     |  8 +-------
fs/gfs2/file.c     |  9 +++++++--
fs/ntfs/file.c     |  8 ++------
fs/udf/file.c      | 11 ++---------
fs/xfs/xfs_file.c  |  8 +-------
include/linux/fs.h |  8 +-------
mm/filemap.c       | 30 ++++++++++++++++++++----------
11 files changed, 40 insertions(+), 73 deletions(-)

fs/block_dev.c +1 -7

index cc9d411..c529b1c 100644
@@ -1568,18 +1568,12 @@ static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 */
 ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-     struct file *file = iocb->ki_filp;
      struct blk_plug plug;
      ssize_t ret;
 
      blk_start_plug(&plug);
      ret = __generic_file_write_iter(iocb, from);
-     if (ret > 0) {
-             ssize_t err;
-             err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-             if (err < 0)
-                     ret = err;
-     }
+     ret = generic_write_sync(iocb, ret);
      blk_finish_plug(&plug);
      return ret;
 }

fs/btrfs/file.c +2 -5

index a18ceab..4f4a6f7 100644
@@ -1820,11 +1820,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
       */
      BTRFS_I(inode)->last_trans = root->fs_info->generation + 1;
      BTRFS_I(inode)->last_sub_trans = root->log_transid;
-     if (num_written > 0) {
-             err = generic_write_sync(file, pos, num_written);
-             if (err < 0)
-                     num_written = err;
-     }
+
+     num_written = generic_write_sync(iocb, num_written);
 
      if (sync)
              atomic_dec(&BTRFS_I(inode)->sync_writers);

fs/cifs/file.c +1 -7

index c485afa..32359de 100644
@@ -2706,13 +2706,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
              rc = __generic_file_write_iter(iocb, from);
              mutex_unlock(&inode->i_mutex);
 
-             if (rc > 0) {
-                     ssize_t err;
-
-                     err = generic_write_sync(file, iocb->ki_pos - rc, rc);
-                     if (err < 0)
-                             rc = err;
-             }
+             rc = generic_write_sync(iocb, rc);
      } else {
              mutex_unlock(&inode->i_mutex);
      }

fs/direct-io.c +2 -6

index e181b6b..b72ac83 100644
@@ -257,12 +257,8 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret,
      inode_dio_done(dio->inode);
      if (is_async) {
              if (dio->rw & WRITE) {
-                     int err;
-
-                     err = generic_write_sync(dio->iocb->ki_filp, offset,
-                                              transferred);
-                     if (err < 0 && ret > 0)
-                             ret = err;
+                     dio->iocb->ki_pos = offset + transferred;
+                     ret = generic_write_sync(dio->iocb, ret);
              }
 
              aio_complete(dio->iocb, ret, 0);

fs/ext4/file.c +1 -7

index aca7b24..79b000c 100644
@@ -175,13 +175,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
      ret = __generic_file_write_iter(iocb, from);
      mutex_unlock(&inode->i_mutex);
 
-     if (ret > 0) {
-             ssize_t err;
-
-             err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-             if (err < 0)
-                     ret = err;
-     }
+     ret = generic_write_sync(iocb, ret);
      if (o_direct)
              blk_finish_plug(&plug);
 

fs/gfs2/file.c +7 -2

index 80dd44d..3fafeca 100644
@@ -895,8 +895,13 @@ retry:
              gfs2_quota_unlock(ip);
      }
 
-     if (error == 0)
-             error = generic_write_sync(file, pos, count);
+     if (error)
+             goto out_unlock;
+
+     if ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host)) {
+             error = vfs_fsync_range(file, pos, pos + count - 1,
+                            (file->f_flags & __O_SYNC) ? 0 : 1);
+     }
      goto out_unlock;
 

out_trans_fail:

fs/ntfs/file.c +2 -6

index 643faa4..4f3d664 100644
@@ -2127,12 +2127,8 @@ static ssize_t ntfs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
      mutex_lock(&inode->i_mutex);
      ret = ntfs_file_aio_write_nolock(iocb, iov, nr_segs, &iocb->ki_pos);
      mutex_unlock(&inode->i_mutex);
-     if (ret > 0) {
-             int err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-             if (err < 0)
-                     ret = err;
-     }
-     return ret;
+
+     return generic_write_sync(iocb, ret);
 }
 
 /**

fs/udf/file.c +2 -9

index bb15771..1cdabd0 100644
@@ -155,16 +155,9 @@ static ssize_t udf_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
      retval = __generic_file_write_iter(iocb, from);
      mutex_unlock(&inode->i_mutex);
 
-     if (retval > 0) {
-             ssize_t err;
-
+     if (retval > 0)
              mark_inode_dirty(inode);
-             err = generic_write_sync(file, iocb->ki_pos - retval, retval);
-             if (err < 0)
-                     retval = err;
-     }
-
-     return retval;
+     return generic_write_sync(iocb, retval);
 }
 
 long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)

fs/xfs/xfs_file.c +1 -7

index 0655915..a8cab66 100644
@@ -792,14 +792,8 @@ xfs_file_write_iter(
              ret = xfs_file_buffered_aio_write(iocb, from);
 
      if (ret > 0) {
-             ssize_t err;
-
              XFS_STATS_ADD(xs_write_bytes, ret);
-
-             /* Handle various SYNC-type writes */
-             err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-             if (err < 0)
-                     ret = err;
+             ret = generic_write_sync(iocb, ret);
      }
      return ret;
 }

include/linux/fs.h +1 -7

index eaebd99..7d0e116 100644
@@ -2242,13 +2242,7 @@ extern int filemap_fdatawrite_range(struct address_space *mapping,
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
                         int datasync);
 extern int vfs_fsync(struct file *file, int datasync);
-static inline int generic_write_sync(struct file *file, loff_t pos, loff_t count)
-{
-     if (!(file->f_flags & O_DSYNC) && !IS_SYNC(file->f_mapping->host))
-             return 0;
-     return vfs_fsync_range(file, pos, pos + count - 1,
-                            (file->f_flags & __O_SYNC) ? 0 : 1);
-}
+extern int generic_write_sync(struct kiocb *iocb, loff_t count);
 extern void emergency_sync(void);
 extern void emergency_remount(void);
 #ifdef CONFIG_BLOCK

mm/filemap.c +0 -0

index 09d3af3..6107058 100644
@@ -2664,6 +2664,24 @@ out:
 }
 EXPORT_SYMBOL(__generic_file_write_iter);
 

mm/filemap.c out: · line 2674

Quoted 8 lines

mm/filemap.c +8 -0

+int generic_write_sync(struct kiocb *iocb, loff_t count)
+{
+     struct file *file = iocb->ki_filp;
+
+     if (count > 0 &&
+         ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host))) {
+             bool fdatasync = !(file->f_flags & __O_SYNC);
+             ssize_t ret = 0;

That "= 0" is pointless. "ret" is overwritten unconditionally on the following line...

Other than that the NTFS bits are:

Acked-by Anton Altaparmakov <[email protected]>

Best regards,

Anton

Quoted 27 lines

mm/filemap.c +11 -3

+
+             ret = vfs_fsync_range(file, iocb->ki_pos - count,
+                             iocb->ki_pos - 1, fdatasync);
+             if (ret < 0)
+                     return ret;
+     }
+     return count;
+}
+EXPORT_SYMBOL(generic_write_sync);
+
 /**
 * generic_file_write_iter - write data to a file
 * @iocb:     IO state structure

@@ -2675,22 +2693,14 @@ EXPORT_SYMBOL(__generic_file_write_iter);
 */
 ssize_t generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-     struct file *file = iocb->ki_filp;
-     struct inode *inode = file->f_mapping->host;
+     struct inode *inode = iocb->ki_filp->f_mapping->host;
      ssize_t ret;
 
      mutex_lock(&inode->i_mutex);
      ret = __generic_file_write_iter(iocb, from);
      mutex_unlock(&inode->i_mutex);
 
-     if (ret > 0) {

mm/filemap.c EXPORT_SYMBOL(__generic_file_write_iter); · line 2705

Quoted 10 lines

mm/filemap.c +1 -7

-             ssize_t err;
-
-             err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-             if (err < 0)
-                     ret = err;
-     }
-     return ret;
+     return generic_write_sync(iocb, ret);
 }
 EXPORT_SYMBOL(generic_file_write_iter);

--
Anton Altaparmakov <[email protected]> (replace at with @) University of Cambridge Information Services, Roger Needham Building 7 JJ Thomson Avenue, Cambridge, CB3 0RB, UK

From: Christoph Hellwig <[email protected]>

Clean up the generic_write_sync by just passing an iocb and a bytes written / negative errno argument. In addition to simplifying the callers this also prepares for passing a per-operation O_DSYNC flag. Two callers didn't quite fit that scheme:

- dio_complete didn't both to update ki_pos as we don't need it on a iocb that is about to be freed, so we had to add it. Additionally it also synced out written data in the error case, which has been changed to operate like the other callers.
- gfs2 also used generic_write_sync to implement a crude version of fallocate. It has been switched to use an open coded variant instead.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/block_dev.c     |  8 +-------
 fs/btrfs/file.c    |  7 ++-----
 fs/cifs/file.c     |  8 +-------
 fs/direct-io.c     |  8 ++------
 fs/ext4/file.c     |  8 +-------
 fs/gfs2/file.c     |  9 +++++++--
 fs/ntfs/file.c     |  8 ++------
 fs/udf/file.c      | 11 ++---------
 fs/xfs/xfs_file.c  |  8 +-------
 include/linux/fs.h |  8 +-------
 mm/filemap.c       | 30 ++++++++++++++++++++----------
 11 files changed, 40 insertions(+), 73 deletions(-)

fs/block_dev.c +1 -7

index cc9d411..c529b1c 100644
@@ -1568,18 +1568,12 @@ static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
  */
 ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	struct file *file = iocb->ki_filp;
 	struct blk_plug plug;
 	ssize_t ret;
 
 	blk_start_plug(&plug);
 	ret = __generic_file_write_iter(iocb, from);
-	if (ret > 0) {
-		ssize_t err;
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
+	ret = generic_write_sync(iocb, ret);
 	blk_finish_plug(&plug);
 	return ret;
 }

fs/btrfs/file.c +2 -5

index a18ceab..4f4a6f7 100644
@@ -1820,11 +1820,8 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	 */
 	BTRFS_I(inode)->last_trans = root->fs_info->generation + 1;
 	BTRFS_I(inode)->last_sub_trans = root->log_transid;
-	if (num_written > 0) {
-		err = generic_write_sync(file, pos, num_written);
-		if (err < 0)
-			num_written = err;
-	}
+
+	num_written = generic_write_sync(iocb, num_written);
 
 	if (sync)
 		atomic_dec(&BTRFS_I(inode)->sync_writers);

fs/cifs/file.c +1 -7

index c485afa..32359de 100644
@@ -2706,13 +2706,7 @@ cifs_writev(struct kiocb *iocb, struct iov_iter *from)
 		rc = __generic_file_write_iter(iocb, from);
 		mutex_unlock(&inode->i_mutex);
 
-		if (rc > 0) {
-			ssize_t err;
-
-			err = generic_write_sync(file, iocb->ki_pos - rc, rc);
-			if (err < 0)
-				rc = err;
-		}
+		rc = generic_write_sync(iocb, rc);
 	} else {
 		mutex_unlock(&inode->i_mutex);
 	}

fs/direct-io.c +2 -6

index e181b6b..b72ac83 100644
@@ -257,12 +257,8 @@ static ssize_t dio_complete(struct dio *dio, loff_t offset, ssize_t ret,
 	inode_dio_done(dio->inode);
 	if (is_async) {
 		if (dio->rw & WRITE) {
-			int err;
-
-			err = generic_write_sync(dio->iocb->ki_filp, offset,
-						 transferred);
-			if (err < 0 && ret > 0)
-				ret = err;
+			dio->iocb->ki_pos = offset + transferred;
+			ret = generic_write_sync(dio->iocb, ret);
 		}
 
 		aio_complete(dio->iocb, ret, 0);

fs/ext4/file.c +1 -7

index aca7b24..79b000c 100644
@@ -175,13 +175,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	ret = __generic_file_write_iter(iocb, from);
 	mutex_unlock(&inode->i_mutex);
 
-	if (ret > 0) {
-		ssize_t err;
-
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
+	ret = generic_write_sync(iocb, ret);
 	if (o_direct)
 		blk_finish_plug(&plug);
 

fs/gfs2/file.c +7 -2

index 80dd44d..3fafeca 100644
@@ -895,8 +895,13 @@ retry:
 		gfs2_quota_unlock(ip);
 	}
 
-	if (error == 0)
-		error = generic_write_sync(file, pos, count);
+	if (error)
+		goto out_unlock;
+
+	if ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host)) {
+		error = vfs_fsync_range(file, pos, pos + count - 1,
+			       (file->f_flags & __O_SYNC) ? 0 : 1);
+	}
 	goto out_unlock;
 
 out_trans_fail:

fs/ntfs/file.c +2 -6

index 643faa4..4f3d664 100644
@@ -2127,12 +2127,8 @@ static ssize_t ntfs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
 	mutex_lock(&inode->i_mutex);
 	ret = ntfs_file_aio_write_nolock(iocb, iov, nr_segs, &iocb->ki_pos);
 	mutex_unlock(&inode->i_mutex);
-	if (ret > 0) {
-		int err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
-	return ret;
+
+	return generic_write_sync(iocb, ret);
 }
 
 /**

fs/udf/file.c +2 -9

index bb15771..1cdabd0 100644
@@ -155,16 +155,9 @@ static ssize_t udf_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	retval = __generic_file_write_iter(iocb, from);
 	mutex_unlock(&inode->i_mutex);
 
-	if (retval > 0) {
-		ssize_t err;
-
+	if (retval > 0)
 		mark_inode_dirty(inode);
-		err = generic_write_sync(file, iocb->ki_pos - retval, retval);
-		if (err < 0)
-			retval = err;
-	}
-
-	return retval;
+	return generic_write_sync(iocb, retval);
 }
 
 long udf_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)

fs/xfs/xfs_file.c +1 -7

index 0655915..a8cab66 100644
@@ -792,14 +792,8 @@ xfs_file_write_iter(
 		ret = xfs_file_buffered_aio_write(iocb, from);
 
 	if (ret > 0) {
-		ssize_t err;
-
 		XFS_STATS_ADD(xs_write_bytes, ret);
-
-		/* Handle various SYNC-type writes */
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
+		ret = generic_write_sync(iocb, ret);
 	}
 	return ret;
 }

include/linux/fs.h +1 -7

index eaebd99..7d0e116 100644
@@ -2242,13 +2242,7 @@ extern int filemap_fdatawrite_range(struct address_space *mapping,
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
 			   int datasync);
 extern int vfs_fsync(struct file *file, int datasync);
-static inline int generic_write_sync(struct file *file, loff_t pos, loff_t count)
-{
-	if (!(file->f_flags & O_DSYNC) && !IS_SYNC(file->f_mapping->host))
-		return 0;
-	return vfs_fsync_range(file, pos, pos + count - 1,
-			       (file->f_flags & __O_SYNC) ? 0 : 1);
-}
+extern int generic_write_sync(struct kiocb *iocb, loff_t count);
 extern void emergency_sync(void);
 extern void emergency_remount(void);
 #ifdef CONFIG_BLOCK

mm/filemap.c +20 -10

index 09d3af3..6107058 100644
@@ -2664,6 +2664,24 @@ out:
 }
 EXPORT_SYMBOL(__generic_file_write_iter);
 
+int generic_write_sync(struct kiocb *iocb, loff_t count)
+{
+	struct file *file = iocb->ki_filp;
+
+	if (count > 0 &&
+	    ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host))) {
+		bool fdatasync = !(file->f_flags & __O_SYNC);
+		ssize_t ret = 0;
+
+		ret = vfs_fsync_range(file, iocb->ki_pos - count,
+				iocb->ki_pos - 1, fdatasync);
+		if (ret < 0)
+			return ret;
+	}
+	return count;
+}
+EXPORT_SYMBOL(generic_write_sync);
+
 /**
  * generic_file_write_iter - write data to a file
  * @iocb:	IO state structure

@@ -2675,22 +2693,14 @@ EXPORT_SYMBOL(__generic_file_write_iter);
  */
 ssize_t generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	struct file *file = iocb->ki_filp;
-	struct inode *inode = file->f_mapping->host;
+	struct inode *inode = iocb->ki_filp->f_mapping->host;
 	ssize_t ret;
 
 	mutex_lock(&inode->i_mutex);
 	ret = __generic_file_write_iter(iocb, from);
 	mutex_unlock(&inode->i_mutex);
 
-	if (ret > 0) {
-		ssize_t err;
-
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
-	return ret;
+	return generic_write_sync(iocb, ret);
 }
 EXPORT_SYMBOL(generic_file_write_iter);
 

Signature · 6 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/read_write.c static ssize_t do_readv_writev(int type, struct file *file, · line 848

Quoted 9 lines · Milosz Tanski

fs/read_write.c +7 -2

-		if (type == READ && (flags & RWF_NONBLOCK))
-			return -EAGAIN;
+		if (type == READ) {
+			if (flags & RWF_NONBLOCK)
+				return -EAGAIN;
+		} else {
+			if (flags & RWF_DSYNC)
+				return -EINVAL;
+		}

Minor nit, but I'd rather read something that looks like this:

	if (type == READ && (flags & RWF_NONBLOCK))
		return -EAGAIN;
	else if (type == WRITE && (flags & RWF_DSYNC))
		return -EINVAL;

I won't lose sleep over it, though.

Reviewed-by Jeff Moyer <[email protected]>

Quoted 7 lines · Jeff Moyer

Minor nit, but I'd rather read something that looks like this:

      if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;
      else if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

But your version is less logically efficient for the case where "type == READ" is true and "flags & RWF_NONBLOCK" is false because your version then has to do the "if (type == WRITE" check before discovering it does not need to take that branch either, whilst the original version does not have to do such a test at all.

Quoted 3 lines · Anton Altaparmakov

On 7 Nov 2014, at 01:46, Jeff Moyer <[email protected]> wrote:
Minor nit, but I'd rather read something that looks like this:

Quoted 4 lines

      if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;
      else if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

But your version is less logically efficient for the case where "type == READ" is true and "flags & RWF_NONBLOCK" is false because your version then has to do the "if (type == WRITE" check before discovering it does not need to take that branch either, whilst the original version does not have to do such a test at all.

Seriously?

Just focus on the code readability/maintainability which makes the code most easily understood/obvious to a new pair of eyes, and leave such micro-optimizations to the compiler..

Thanks

Hi,

Quoted 12 lines · Anand Avati

Quoted 7 lines · Anton Altaparmakov

On 7 Nov 2014, at 01:46, Jeff Moyer <[email protected]> wrote:
Minor nit, but I'd rather read something that looks like this:

      if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;
      else if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

But your version is less logically efficient for the case where "type == READ" is true and "flags & RWF_NONBLOCK" is false because your version then has to do the "if (type == WRITE" check before discovering it does not need to take that branch either, whilst the original version does not have to do such a test at all.

Quoted 1 line

Seriously?

Of course seriously.

Quoted 1 line

Just focus on the code readability/maintainability which makes the code most easily understood/obvious to a new pair of eyes, and leave such micro-optimizations to the compiler..

The original version is more readable (IMO) and this is not a micro-optimization. It is people like you who are responsible for the fact that we need faster and faster computers to cope with the inefficient/poor code being written more and more...

Quoted 14 lines · Anton Altaparmakov

Hi,

Quoted 12 lines · Anand Avati

Quoted 7 lines · Anton Altaparmakov

On 7 Nov 2014, at 01:46, Jeff Moyer <[email protected]> wrote:
Minor nit, but I'd rather read something that looks like this:

      if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;
      else if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

But your version is less logically efficient for the case where "type == READ" is true and "flags & RWF_NONBLOCK" is false because your version then has to do the "if (type == WRITE" check before discovering it does not need to take that branch either, whilst the original version does not have to do such a test at all.

Quoted 8 lines

Quoted 1 line

Seriously?

Of course seriously.

Quoted 1 line

Just focus on the code readability/maintainability which makes the code most easily understood/obvious to a new pair of eyes, and leave such micro-optimizations to the compiler..

The original version is more readable (IMO) and this is not a micro-optimization. It is people like you who are responsible for the fact that we need faster and faster computers to cope with the inefficient/poor code being written more and more...

Your original version needs me to know that type can only be either READ or WRITE (and not, for instance, READONLY or READWRITE or some other random special case) and it rings alarm bells when I first see it. If you want to keep the micro optimization, you need an assertion to acknowledge the potential bug and a comment to make the code obvious:

+ assert(type == READ || type == WRITE);
+ if (type == READ) {
+ if (flags & RWF_NONBLOCK)
+ return -EAGAIN;
+ } else { /* WRITE */
+ if (flags & RWF_DSYNC)
+ return -EINVAL;
+ }

but since what's really happening here is two separate and independent error checks, Jeff's version is still better, even if it does take an extra couple of nanoseconds.

Actually I'd probably write:

       if (type == READ && (flags & RWF_NONBLOCK))
              return -EAGAIN;

       if (type == WRITE && (flags & RWF_DSYNC))
              return -EINVAL;

(no 'else' since the code will never be reached if the first test is true).

--
Roger Willcocks <[email protected]>

From: Christoph Hellwig <[email protected]>

With the new read/write with flags syscalls we can support a flag to enable O_DSYNC semantics on a per-operation basis. This ?s useful to implement protocols like SMB, NFS or SCSI that have such per-operation flags.

Example program below:

cat > pwritev2.c << EOF

        (off_t) val,                              \
        (off_t) ((((uint64_t) (val)) >> (sizeof (long) * 4)) >> (sizeof (long) * 4))

static ssize_t
pwritev2(int fd, const struct iovec *iov, int iovcnt, off_t offset, int flags) {
return syscall(__NR_pwritev2, fd, iov, iovcnt, LO_HI_LONG(offset),
flags);
}

int main(int argc, char **argv)

{
	int fd = open(argv[1], O_WRONLY|O_CREAT|O_TRUNC, 0666);
	char buf[1024];
	struct iovec iov = { .iov_base = buf, .iov_len = 1024 };
	int ret;

        if (fd < 0) {
                perror("open");
                return 0;
        }

	memset(buf, 0xfe, sizeof(buf));

	ret = pwritev2(fd, &iov, 1, 0, RWF_DSYNC);
	if (ret < 0)
		perror("pwritev2");
	else
		printf("ret = %d\n", ret);

	return 0;
}

EOF

Signed-off-by Christoph Hellwig <[email protected]>

[[email protected]: added flag check to compat_do_readv_writev()]

Signed-off-by Milosz Tanski <[email protected]>

---
 fs/ceph/file.c     |  4 +++-
 fs/fuse/file.c     |  2 ++
 fs/nfs/file.c      | 10 ++++++----
 fs/ocfs2/file.c    |  6 ++++--
 fs/read_write.c    | 20 +++++++++++++++-----
 include/linux/fs.h |  3 ++-
 mm/filemap.c       |  4 +++-
 7 files changed, 35 insertions(+), 14 deletions(-)

fs/ceph/file.c +3 -1

index b798b5c..2d4e15a 100644
@@ -983,7 +983,9 @@ retry_snap:
 	ceph_put_cap_refs(ci, got);
 
 	if (written >= 0 &&
-	    ((file->f_flags & O_SYNC) || IS_SYNC(file->f_mapping->host) ||
+	    ((file->f_flags & O_SYNC) ||
+	     IS_SYNC(file->f_mapping->host) ||
+	     (iocb->ki_rwflags & RWF_DSYNC) ||
 	     ceph_osdmap_flag(osdc->osdmap, CEPH_OSDMAP_NEARFULL))) {
 		err = vfs_fsync_range(file, pos, pos + written - 1, 1);
 		if (err < 0)

fs/fuse/file.c +2 -0

index caa8d95..bb4fb23 100644
@@ -1248,6 +1248,8 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		written += written_buffered;
 		iocb->ki_pos = pos + written_buffered;
 	} else {
+		if (iocb->ki_rwflags & RWF_DSYNC)
+			return -EINVAL;
 		written = fuse_perform_write(file, mapping, from, pos);
 		if (written >= 0)
 			iocb->ki_pos = pos + written;

fs/nfs/file.c +6 -4

index aa9046f..c59b0b7 100644
@@ -652,13 +652,15 @@ static const struct vm_operations_struct nfs_file_vm_ops = {
 	.remap_pages = generic_file_remap_pages,
 };
 
-static int nfs_need_sync_write(struct file *filp, struct inode *inode)
+static int nfs_need_sync_write(struct kiocb *iocb, struct inode *inode)
 {
 	struct nfs_open_context *ctx;
 
-	if (IS_SYNC(inode) || (filp->f_flags & O_DSYNC))
+	if (IS_SYNC(inode) ||
+	    (iocb->ki_filp->f_flags & O_DSYNC) ||
+	    (iocb->ki_rwflags & RWF_DSYNC))
 		return 1;
-	ctx = nfs_file_open_context(filp);
+	ctx = nfs_file_open_context(iocb->ki_filp);
 	if (test_bit(NFS_CONTEXT_ERROR_WRITE, &ctx->flags) ||
 	    nfs_ctx_key_to_expire(ctx))
 		return 1;

@@ -705,7 +707,7 @@ ssize_t nfs_file_write(struct kiocb *iocb, struct iov_iter *from)
 		written = result;
 
 	/* Return error values for O_DSYNC and IS_SYNC() */
-	if (result >= 0 && nfs_need_sync_write(file, inode)) {
+	if (result >= 0 && nfs_need_sync_write(iocb, inode)) {
 		int err = vfs_fsync(file, 0);
 		if (err < 0)
 			result = err;

fs/ocfs2/file.c +4 -2

index bb66ca4..8f9a86b 100644
@@ -2374,8 +2374,10 @@ out_dio:
 	/* buffered aio wouldn't have proper lock coverage today */
 	BUG_ON(ret == -EIOCBQUEUED && !(file->f_flags & O_DIRECT));
 
-	if (((file->f_flags & O_DSYNC) && !direct_io) || IS_SYNC(inode) ||
-	    ((file->f_flags & O_DIRECT) && !direct_io)) {
+	if (((file->f_flags & O_DSYNC) && !direct_io) ||
+	    IS_SYNC(inode) ||
+	    ((file->f_flags & O_DIRECT) && !direct_io) ||
+	    (iocb->ki_rwflags & RWF_DSYNC)) {
 		ret = filemap_fdatawrite_range(file->f_mapping, *ppos,
 					       *ppos + count - 1);
 		if (ret < 0)

fs/read_write.c +15 -5

index cba7d4c..3443265 100644
@@ -839,8 +839,13 @@ static ssize_t do_readv_writev(int type, struct file *file,
 		ret = do_iter_readv_writev(file, type, iov, nr_segs, tot_len,
 						pos, iter_fn, flags);
 	} else {
-		if (type == READ && (flags & RWF_NONBLOCK))
-			return -EAGAIN;
+		if (type == READ) {
+			if (flags & RWF_NONBLOCK)
+				return -EAGAIN;
+		} else {
+			if (flags & RWF_DSYNC)
+				return -EINVAL;
+		}
 
 		if (fnv)
 			ret = do_sync_readv_writev(file, iov, nr_segs, tot_len,

@@ -888,7 +893,7 @@ ssize_t vfs_writev(struct file *file, const struct iovec __user *vec,
 		return -EBADF;
 	if (!(file->f_mode & FMODE_CAN_WRITE))
 		return -EINVAL;
-	if (flags & ~0)
+	if (flags & ~RWF_DSYNC)
 		return -EINVAL;
 
 	return do_readv_writev(WRITE, file, vec, vlen, pos, flags);

@@ -1080,8 +1085,13 @@ static ssize_t compat_do_readv_writev(int type, struct file *file,
 		ret = do_iter_readv_writev(file, type, iov, nr_segs, tot_len,
 						pos, iter_fn, flags);
 	} else {
-		if (type == READ && (flags & RWF_NONBLOCK))
-			return -EAGAIN;
+		if (type == READ) {
+			if (flags & RWF_NONBLOCK)
+				return -EAGAIN;
+		} else {
+			if (flags & RWF_DSYNC)
+				return -EINVAL;
+		}
 
 		if (fnv)
 			ret = do_sync_readv_writev(file, iov, nr_segs, tot_len,

include/linux/fs.h +2 -1

index 7d0e116..7786b88 100644
@@ -1460,7 +1460,8 @@ struct block_device_operations;
 #define HAVE_UNLOCKED_IOCTL 1
 
 /* These flags are used for the readv/writev syscalls with flags. */
-#define RWF_NONBLOCK 0x00000001
+#define RWF_NONBLOCK	0x00000001
+#define RWF_DSYNC	0x00000002
 
 struct iov_iter;
 

mm/filemap.c +3 -1

index 6107058..4fbef99 100644
@@ -2669,7 +2669,9 @@ int generic_write_sync(struct kiocb *iocb, loff_t count)
 	struct file *file = iocb->ki_filp;
 
 	if (count > 0 &&
-	    ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host))) {
+	    ((file->f_flags & O_DSYNC) ||
+	     (iocb->ki_rwflags & RWF_DSYNC) ||
+	     IS_SYNC(file->f_mapping->host))) {
 		bool fdatasync = !(file->f_flags & __O_SYNC);
 		ssize_t ret = 0;
 

Signature · 6 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

This series looks good, do you also have a man page sniplet to document the new syscalls?

This patcheset introduces an ability to perform a non-blocking read from regular files in buffered IO mode. This works by only for those filesystems that have data in the page cache.

It does this by introducing new syscalls new syscalls preadv2/pwritev2. These new syscalls behave like the network sendmsg, recvmsg syscalls that accept an extra flag argument (RWF_NONBLOCK).

It's a very common patern today (samba, libuv, etc..) use a large threadpool to perform buffered IO operations. They submit the work form another thread that performs network IO and epoll or other threads that perform CPU work. This leads to increased latency for processing, esp. in the case of data that's already cached in the page cache.

With the new interface the applications will now be able to fetch the data in their network / cpu bound thread(s) and only defer to a threadpool if it's not there. In our own application (VLDB) we've observed a decrease in latency for "fast" request by avoiding unnecessary queuing and having to swap out current tasks in IO bound work threads.

Version 5 highlight:
- XFS support for RWF_NONBLOCK. from Christoph.
- RWF_DSYNC flag and support for pwritev2, from Christoph.
- Implemented compat syscalls, per. Jeff.
- Missing nfs, ceph changes from older patchset.

Version 4 highlight:
- Updated for 3.18-rc1.
- Performance data from our application.
- First stab at man page with Jeff's help. Patch is in-reply to.

RFC Version 3 highlights:
- Down to 2 syscalls from 4; can user fp or argument position.
- RWF_NONBLOCK value flag is not the same O_NONBLOCK, per Jeff.

RFC Version 2 highlights:
- Put the flags argument into kiocb (less noise), per. Al Viro
- O_DIRECT checking early in the process, per. Jeff Moyer
- Resolved duplicate (c&p) code in syscall code, per. Jeff
- Included perf data in thread cover letter, per. Jeff
- Created a new flag (not O_NONBLOCK) for readv2, perf Jeff

Some perf data generated using fio comparing the posix aio engine to a version of the posix AIO engine that attempts to performs "fast" reads before submitting the operations to the queue. This workflow is on ext4 partition on raid0 (test / build-rig.) Simulating our database access patern workload using 16kb read accesses. Our database uses a home-spun posix aio like queue (samba does the same thing.)

f1: ~73% rand read over mostly cached data (zipf med-size dataset)
f2: ~18% rand read over mostly un-cached data (uniform large-dataset)
f3: ~9% seq-read over large dataset

before:

f1:

    bw (KB  /s): min=   11, max= 9088, per=0.56%, avg=969.54, stdev=827.99
    lat (msec) : 50=0.01%, 100=1.06%, 250=5.88%, 500=4.08%, 750=12.48%
    lat (msec) : 1000=17.27%, 2000=49.86%, >=2000=9.42%
f2:
    bw (KB  /s): min=    2, max= 1882, per=0.16%, avg=273.28, stdev=220.26
    lat (msec) : 250=5.65%, 500=3.31%, 750=15.64%, 1000=24.59%, 2000=46.56%
    lat (msec) : >=2000=4.33%
f3:
    bw (KB  /s): min=    0, max=265568, per=99.95%, avg=174575.10,
                 stdev=34526.89
    lat (usec) : 2=0.01%, 4=0.01%, 10=0.02%, 20=0.27%, 50=10.82%
    lat (usec) : 100=50.34%, 250=5.05%, 500=7.12%, 750=6.60%, 1000=4.55%
    lat (msec) : 2=8.73%, 4=3.49%, 10=1.83%, 20=0.89%, 50=0.22%
    lat (msec) : 100=0.05%, 250=0.02%, 500=0.01%
total:
   READ: io=102365MB, aggrb=174669KB/s, minb=240KB/s, maxb=173599KB/s,
         mint=600001msec, maxt=600113msec

after (with fast read using preadv2 before submit):

f1:

    bw (KB  /s): min=    3, max=14897, per=1.28%, avg=2276.69, stdev=2930.39
    lat (usec) : 2=70.63%, 4=0.01%
    lat (msec) : 250=0.20%, 500=2.26%, 750=1.18%, 2000=0.22%, >=2000=25.53%
f2:
    bw (KB  /s): min=    2, max= 2362, per=0.14%, avg=249.83, stdev=222.00
    lat (msec) : 250=6.35%, 500=1.78%, 750=9.29%, 1000=20.49%, 2000=52.18%
    lat (msec) : >=2000=9.99%
f3:
    bw (KB  /s): min=    1, max=245448, per=100.00%, avg=177366.50,
                 stdev=35995.60
    lat (usec) : 2=64.04%, 4=0.01%, 10=0.01%, 20=0.06%, 50=0.43%
    lat (usec) : 100=0.20%, 250=1.27%, 500=2.93%, 750=3.93%, 1000=7.35%
    lat (msec) : 2=14.27%, 4=2.88%, 10=1.54%, 20=0.81%, 50=0.22%
    lat (msec) : 100=0.05%, 250=0.02%
total:
   READ: io=103941MB, aggrb=177339KB/s, minb=213KB/s, maxb=176375KB/s,
         mint=600020msec, maxt=600178msec

Interpreting the results you can see total bandwidth stays the same but overall request latency is decreased in f1 (random, mostly cached) and f3 (sequential) workloads. There is a slight bump in latency for since it's random data that's unlikely to be cached but we're always trying "fast read".

In our application we have starting keeping track of "fast read" hits/misses and for files / requests that have a lot hit ratio we don't do "fast reads" mostly getting rid of extra latency in the uncached cases. In our real world work load we were able to reduce average response time by 20 to 30% (depends on amount of IO done by request).

I've performed other benchmarks and I have no observed any perf regressions in any of the normal (old) code paths.

I have co-developed these changes with Christoph Hellwig.

Christoph Hellwig (3):
xfs: add RWF_NONBLOCK support
fs: pass iocb to generic_write_sync
fs: add a flag for per-operation O_DSYNC semantics

Milosz Tanski (4):
vfs: Prepare for adding a new preadv/pwritev with user flags.
vfs: Define new syscalls preadv2,pwritev2
x86: wire up preadv2 and pwritev2
vfs: RWF_NONBLOCK flag for preadv2

 arch/x86/syscalls/syscall_32.tbl  |   2 +
 arch/x86/syscalls/syscall_64.tbl  |   2 +
 drivers/target/target_core_file.c |   6 +-
 fs/block_dev.c                    |   8 +-
 fs/btrfs/file.c                   |   7 +-
 fs/ceph/file.c                    |   6 +-
 fs/cifs/file.c                    |  14 +--
 fs/direct-io.c                    |   8 +-
 fs/ext4/file.c                    |   8 +-
 fs/fuse/file.c                    |   2 +
 fs/gfs2/file.c                    |   9 +-
 fs/nfs/file.c                     |  15 ++-
 fs/nfsd/vfs.c                     |   4 +-
 fs/ntfs/file.c                    |   8 +-
 fs/ocfs2/file.c                   |  12 +-
 fs/pipe.c                         |   3 +-
 fs/read_write.c                   | 239 +++++++++++++++++++++++++++++---------
 fs/splice.c                       |   2 +-
 fs/udf/file.c                     |  11 +-
 fs/xfs/xfs_file.c                 |  36 ++++--
 include/linux/aio.h               |   2 +
 include/linux/compat.h            |   6 +
 include/linux/fs.h                |  16 ++-
 include/linux/syscalls.h          |   6 +
 include/uapi/asm-generic/unistd.h |   6 +-
 mm/filemap.c                      |  55 +++++++--
 mm/shmem.c                        |   4 +
 27 files changed, 346 insertions(+), 151 deletions(-)

--
1.9.1