Re: [PATCH v5 6/7] fs: pass iocb to generic_write_sync

[Jan Kara]

On Wed 05-11-14 16:14:52, Milosz Tanski wrote:
> From: Christoph Hellwig <hch@xxxxxx>
> 
> 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 <hch@xxxxxx>
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  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(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index cc9d411..c529b1c 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -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;
>  }
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index a18ceab..4f4a6f7 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -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);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index c485afa..32359de 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -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);
>  	}
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index e181b6b..b72ac83 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -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);
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index aca7b24..79b000c 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -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);
>  
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 80dd44d..3fafeca 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -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:
> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index 643faa4..4f3d664 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -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);
>  }
>  
>  /**
> diff --git a/fs/udf/file.c b/fs/udf/file.c
> index bb15771..1cdabd0 100644
> --- a/fs/udf/file.c
> +++ b/fs/udf/file.c
> @@ -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)
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 0655915..a8cab66 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -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;
>  }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index eaebd99..7d0e116 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -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
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 09d3af3..6107058 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -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);
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/