Re: [PATCH 1/6] sort out blockdev_direct_IO variants

[Jan Kara <jack@suse.cz>]

On Sun 30-05-10 22:49:42, Christoph Hellwig wrote:
> Move the call to vmtruncate to get rid of accessive blocks to the callers
> in prepearation of the new truncate calling sequence.  This was only done
> for DIO_LOCKING filesystems, so the __blockdev_direct_IO_newtrunc variant
> was not needed anyway.  Get rid of blockdev_direct_IO_no_locking and
> it's _newtrunc variant while at it as just opencoding the two additional
> paramters is shorted than the name suffix.
  Hmm, I've noticed that you didn't add truncate code to btrfs. I see that
it already does some recovery after direct IO error but I'd like to make
sure that it won't be broken by your patches... Chris?

								Honza
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/fs/block_dev.c
> ===================================================================
> --- linux-2.6.orig/fs/block_dev.c	2010-05-30 15:10:19.692004346 +0200
> +++ linux-2.6/fs/block_dev.c	2010-05-30 15:15:00.753273377 +0200
> @@ -172,9 +172,8 @@ blkdev_direct_IO(int rw, struct kiocb *i
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file->f_mapping->host;
>  
> -	return blockdev_direct_IO_no_locking_newtrunc(rw, iocb, inode,
> -				I_BDEV(inode), iov, offset, nr_segs,
> -				blkdev_get_blocks, NULL);
> +	return __blockdev_direct_IO(rw, iocb, inode, I_BDEV(inode), iov, offset,
> +				    nr_segs, blkdev_get_blocks, NULL, NULL, 0);
>  }
>  
>  int __sync_blockdev(struct block_device *bdev, int wait)
> Index: linux-2.6/fs/direct-io.c
> ===================================================================
> --- linux-2.6.orig/fs/direct-io.c	2010-05-30 15:10:19.711004136 +0200
> +++ linux-2.6/fs/direct-io.c	2010-05-30 15:15:00.754273587 +0200
> @@ -1134,8 +1134,27 @@ direct_io_worker(int rw, struct kiocb *i
>  	return ret;
>  }
>  
> +/*
> + * This is a library function for use by filesystem drivers.
> + *
> + * The locking rules are governed by the flags parameter:
> + *  - if the flags value contains DIO_LOCKING we use a fancy locking
> + *    scheme for dumb filesystems.
> + *    For writes this function is called under i_mutex and returns with
> + *    i_mutex held, for reads, i_mutex is not held on entry, but it is
> + *    taken and dropped again before returning.
> + *    For reads and writes i_alloc_sem is taken in shared mode and released
> + *    on I/O completion (which may happen asynchronously after returning to
> + *    the caller).
> + *
> + *  - if the flags value does NOT contain DIO_LOCKING we don't use any
> + *    internal locking but rather rely on the filesystem to synchronize
> + *    direct I/O reads/writes versus each other and truncate.
> + *    For reads and writes both i_mutex and i_alloc_sem are not held on
> + *    entry and are never taken.
> + */
>  ssize_t
> -__blockdev_direct_IO_newtrunc(int rw, struct kiocb *iocb, struct inode *inode,
> +__blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
>  	struct block_device *bdev, const struct iovec *iov, loff_t offset, 
>  	unsigned long nr_segs, get_block_t get_block, dio_iodone_t end_io,
>  	dio_submit_t submit_io,	int flags)
> @@ -1231,57 +1250,4 @@ __blockdev_direct_IO_newtrunc(int rw, st
>  out:
>  	return retval;
>  }
> -EXPORT_SYMBOL(__blockdev_direct_IO_newtrunc);
> -
> -/*
> - * This is a library function for use by filesystem drivers.
> - *
> - * The locking rules are governed by the flags parameter:
> - *  - if the flags value contains DIO_LOCKING we use a fancy locking
> - *    scheme for dumb filesystems.
> - *    For writes this function is called under i_mutex and returns with
> - *    i_mutex held, for reads, i_mutex is not held on entry, but it is
> - *    taken and dropped again before returning.
> - *    For reads and writes i_alloc_sem is taken in shared mode and released
> - *    on I/O completion (which may happen asynchronously after returning to
> - *    the caller).
> - *
> - *  - if the flags value does NOT contain DIO_LOCKING we don't use any
> - *    internal locking but rather rely on the filesystem to synchronize
> - *    direct I/O reads/writes versus each other and truncate.
> - *    For reads and writes both i_mutex and i_alloc_sem are not held on
> - *    entry and are never taken.
> - */
> -ssize_t
> -__blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
> -	struct block_device *bdev, const struct iovec *iov, loff_t offset,
> -	unsigned long nr_segs, get_block_t get_block, dio_iodone_t end_io,
> -	dio_submit_t submit_io,	int flags)
> -{
> -	ssize_t retval;
> -
> -	retval = __blockdev_direct_IO_newtrunc(rw, iocb, inode, bdev, iov,
> -			offset, nr_segs, get_block, end_io, submit_io, flags);
> -	/*
> -	 * In case of error extending write may have instantiated a few
> -	 * blocks outside i_size. Trim these off again for DIO_LOCKING.
> -	 * NOTE: DIO_NO_LOCK/DIO_OWN_LOCK callers have to handle this in
> -	 * their own manner. This is a further example of where the old
> -	 * truncate sequence is inadequate.
> -	 *
> -	 * NOTE: filesystems with their own locking have to handle this
> -	 * on their own.
> -	 */
> -	if (flags & DIO_LOCKING) {
> -		if (unlikely((rw & WRITE) && retval < 0)) {
> -			loff_t isize = i_size_read(inode);
> -			loff_t end = offset + iov_length(iov, nr_segs);
> -
> -			if (end > isize)
> -				vmtruncate(inode, isize);
> -		}
> -	}
> -
> -	return retval;
> -}
>  EXPORT_SYMBOL(__blockdev_direct_IO);
> Index: linux-2.6/fs/ext4/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/ext4/inode.c	2010-05-30 15:10:19.719003158 +0200
> +++ linux-2.6/fs/ext4/inode.c	2010-05-30 15:15:00.760003927 +0200
> @@ -3545,15 +3545,24 @@ static ssize_t ext4_ind_direct_IO(int rw
>  
>  retry:
>  	if (rw == READ && ext4_should_dioread_nolock(inode))
> -		ret = blockdev_direct_IO_no_locking(rw, iocb, inode,
> +		ret = __blockdev_direct_IO(rw, iocb, inode,
>  				 inode->i_sb->s_bdev, iov,
>  				 offset, nr_segs,
> -				 ext4_get_block, NULL);
> -	else
> +				 ext4_get_block, NULL, NULL, 0);
> +	else {
>  		ret = blockdev_direct_IO(rw, iocb, inode,
>  				 inode->i_sb->s_bdev, iov,
>  				 offset, nr_segs,
>  				 ext4_get_block, NULL);
> +
> +		if (unlikely((rw & WRITE) && ret < 0)) {
> +			loff_t isize = i_size_read(inode);
> +			loff_t end = offset + iov_length(iov, nr_segs);
> +
> +			if (end > isize)
> +				vmtruncate(inode, isize);
> +		}
> +	}
>  	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>  		goto retry;
>  
> Index: linux-2.6/fs/gfs2/aops.c
> ===================================================================
> --- linux-2.6.orig/fs/gfs2/aops.c	2010-05-30 15:10:19.727004485 +0200
> +++ linux-2.6/fs/gfs2/aops.c	2010-05-30 15:15:00.768051419 +0200
> @@ -1047,9 +1047,9 @@ static ssize_t gfs2_direct_IO(int rw, st
>  	if (rv != 1)
>  		goto out; /* dio not valid, fall back to buffered i/o */
>  
> -	rv = blockdev_direct_IO_no_locking(rw, iocb, inode, inode->i_sb->s_bdev,
> -					   iov, offset, nr_segs,
> -					   gfs2_get_block_direct, NULL);
> +	rv = __blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
> +				  offset, nr_segs, gfs2_get_block_direct,
> +				  NULL, NULL, 0);
>  out:
>  	gfs2_glock_dq_m(1, &gh);
>  	gfs2_holder_uninit(&gh);
> Index: linux-2.6/fs/ocfs2/aops.c
> ===================================================================
> --- linux-2.6.orig/fs/ocfs2/aops.c	2010-05-30 15:10:19.741004625 +0200
> +++ linux-2.6/fs/ocfs2/aops.c	2010-05-30 15:15:00.776005603 +0200
> @@ -669,11 +669,10 @@ static ssize_t ocfs2_direct_IO(int rw,
>  	if (i_size_read(inode) <= offset)
>  		return 0;
>  
> -	ret = blockdev_direct_IO_no_locking(rw, iocb, inode,
> -					    inode->i_sb->s_bdev, iov, offset,
> -					    nr_segs,
> -					    ocfs2_direct_IO_get_blocks,
> -					    ocfs2_dio_end_io);
> +	ret = __blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev,
> +				   iov, offset, nr_segs,
> +				   ocfs2_direct_IO_get_blocks,
> +				   ocfs2_dio_end_io, NULL, 0);
>  
>  	mlog_exit(ret);
>  	return ret;
> Index: linux-2.6/fs/xfs/linux-2.6/xfs_aops.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/linux-2.6/xfs_aops.c	2010-05-30 15:10:19.752024390 +0200
> +++ linux-2.6/fs/xfs/linux-2.6/xfs_aops.c	2010-05-30 15:15:00.781005533 +0200
> @@ -1658,10 +1658,9 @@ xfs_vm_direct_IO(
>  	iocb->private = xfs_alloc_ioend(inode, rw == WRITE ?
>  					IO_UNWRITTEN : IO_READ);
>  
> -	ret = blockdev_direct_IO_no_locking(rw, iocb, inode, bdev, iov,
> -					    offset, nr_segs,
> -					    xfs_get_blocks_direct,
> -					    xfs_end_io_direct);
> +	ret = __blockdev_direct_IO(rw, iocb, inode, bdev, iov, offset,
> +				   nr_segs, xfs_get_blocks_direct,
> +				   xfs_end_io_direct, NULL, 0);
>  
>  	if (unlikely(ret != -EIOCBQUEUED && iocb->private))
>  		xfs_destroy_ioend(iocb->private);
> Index: linux-2.6/include/linux/fs.h
> ===================================================================
> --- linux-2.6.orig/include/linux/fs.h	2010-05-30 15:10:19.861004207 +0200
> +++ linux-2.6/include/linux/fs.h	2010-05-30 15:15:00.787005812 +0200
> @@ -2255,16 +2255,6 @@ static inline int xip_truncate_page(stru
>  struct bio;
>  typedef void (dio_submit_t)(int rw, struct bio *bio, struct inode *inode,
>  			    loff_t file_offset);
> -void dio_end_io(struct bio *bio, int error);
> -
> -ssize_t __blockdev_direct_IO_newtrunc(int rw, struct kiocb *iocb, struct inode *inode,
> -	struct block_device *bdev, const struct iovec *iov, loff_t offset,
> -	unsigned long nr_segs, get_block_t get_block, dio_iodone_t end_io,
> -	dio_submit_t submit_io, int lock_type);
> -ssize_t __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
> -	struct block_device *bdev, const struct iovec *iov, loff_t offset,
> -	unsigned long nr_segs, get_block_t get_block, dio_iodone_t end_io,
> -	dio_submit_t submit_io,	int lock_type);
>  
>  enum {
>  	/* need locking between buffered and direct access */
> @@ -2274,24 +2264,13 @@ enum {
>  	DIO_SKIP_HOLES	= 0x02,
>  };
>  
> -static inline ssize_t blockdev_direct_IO_newtrunc(int rw, struct kiocb *iocb,
> -	struct inode *inode, struct block_device *bdev, const struct iovec *iov,
> -	loff_t offset, unsigned long nr_segs, get_block_t get_block,
> -	dio_iodone_t end_io)
> -{
> -	return __blockdev_direct_IO_newtrunc(rw, iocb, inode, bdev, iov, offset,
> -				    nr_segs, get_block, end_io, NULL,
> -				    DIO_LOCKING | DIO_SKIP_HOLES);
> -}
> +void dio_end_io(struct bio *bio, int error);
> +
> +ssize_t __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
> +	struct block_device *bdev, const struct iovec *iov, loff_t offset,
> +	unsigned long nr_segs, get_block_t get_block, dio_iodone_t end_io,
> +	dio_submit_t submit_io,	int flags);
>  
> -static inline ssize_t blockdev_direct_IO_no_locking_newtrunc(int rw, struct kiocb *iocb,
> -	struct inode *inode, struct block_device *bdev, const struct iovec *iov,
> -	loff_t offset, unsigned long nr_segs, get_block_t get_block,
> -	dio_iodone_t end_io)
> -{
> -	return __blockdev_direct_IO_newtrunc(rw, iocb, inode, bdev, iov, offset,
> -				nr_segs, get_block, end_io, NULL, 0);
> -}
>  static inline ssize_t blockdev_direct_IO(int rw, struct kiocb *iocb,
>  	struct inode *inode, struct block_device *bdev, const struct iovec *iov,
>  	loff_t offset, unsigned long nr_segs, get_block_t get_block,
> @@ -2301,15 +2280,6 @@ static inline ssize_t blockdev_direct_IO
>  				    nr_segs, get_block, end_io, NULL,
>  				    DIO_LOCKING | DIO_SKIP_HOLES);
>  }
> -
> -static inline ssize_t blockdev_direct_IO_no_locking(int rw, struct kiocb *iocb,
> -	struct inode *inode, struct block_device *bdev, const struct iovec *iov,
> -	loff_t offset, unsigned long nr_segs, get_block_t get_block,
> -	dio_iodone_t end_io)
> -{
> -	return __blockdev_direct_IO(rw, iocb, inode, bdev, iov, offset,
> -				    nr_segs, get_block, end_io, NULL, 0);
> -}
>  #endif
>  
>  extern const struct file_operations generic_ro_fops;
> Index: linux-2.6/fs/ext2/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/ext2/inode.c	2010-05-30 15:10:26.785274006 +0200
> +++ linux-2.6/fs/ext2/inode.c	2010-05-30 15:15:00.792005673 +0200
> @@ -831,7 +831,7 @@ ext2_direct_IO(int rw, struct kiocb *ioc
>  	struct inode *inode = mapping->host;
>  	ssize_t ret;
>  
> -	ret = blockdev_direct_IO_newtrunc(rw, iocb, inode, inode->i_sb->s_bdev,
> +	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev,
>  				iov, offset, nr_segs, ext2_get_block, NULL);
>  	if (ret < 0 && (rw & WRITE))
>  		ext2_write_failed(mapping, offset + iov_length(iov, nr_segs));
> Index: linux-2.6/fs/ext3/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/ext3/inode.c	2010-05-30 15:10:19.774004346 +0200
> +++ linux-2.6/fs/ext3/inode.c	2010-05-30 15:15:00.798005533 +0200
> @@ -1785,6 +1785,17 @@ retry:
>  	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
>  				 offset, nr_segs,
>  				 ext3_get_block, NULL);
> +	/*
> +	 * In case of error extending write may have instantiated a few
> +	 * blocks outside i_size. Trim these off again.
> +	 */
> +	if (unlikely((rw & WRITE) && ret < 0)) {
> +		loff_t isize = i_size_read(inode);
> +		loff_t end = offset + iov_length(iov, nr_segs);
> +
> +		if (end > isize)
> +			vmtruncate(inode, isize);
> +	}
>  	if (ret == -ENOSPC && ext3_should_retry_alloc(inode->i_sb, &retries))
>  		goto retry;
>  
> Index: linux-2.6/fs/fat/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/fat/inode.c	2010-05-30 15:10:19.795006092 +0200
> +++ linux-2.6/fs/fat/inode.c	2010-05-30 15:15:00.806067692 +0200
> @@ -212,8 +212,8 @@ static ssize_t fat_direct_IO(int rw, str
>  	 * FAT need to use the DIO_LOCKING for avoiding the race
>  	 * condition of fat_get_block() and ->truncate().
>  	 */
> -	ret = blockdev_direct_IO_newtrunc(rw, iocb, inode, inode->i_sb->s_bdev,
> -				iov, offset, nr_segs, fat_get_block, NULL);
> +	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev,
> +				 iov, offset, nr_segs, fat_get_block, NULL);
>  	if (ret < 0 && (rw & WRITE))
>  		fat_write_failed(mapping, offset + iov_length(iov, nr_segs));
>  
> Index: linux-2.6/fs/hfs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/hfs/inode.c	2010-05-30 15:10:19.808003788 +0200
> +++ linux-2.6/fs/hfs/inode.c	2010-05-30 15:15:00.810051978 +0200
> @@ -112,9 +112,24 @@ static ssize_t hfs_direct_IO(int rw, str
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file->f_path.dentry->d_inode->i_mapping->host;
> +	ssize_t ret;
>  
> -	return blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
> +	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
>  				  offset, nr_segs, hfs_get_block, NULL);
> +
> +	/*
> +	 * In case of error extending write may have instantiated a few
> +	 * blocks outside i_size. Trim these off again.
> +	 */
> +	if (unlikely((rw & WRITE) && ret < 0)) {
> +		loff_t isize = i_size_read(inode);
> +		loff_t end = offset + iov_length(iov, nr_segs);
> +
> +		if (end > isize)
> +			vmtruncate(inode, isize);
> +	}
> +
> +	return ret;
>  }
>  
>  static int hfs_writepages(struct address_space *mapping,
> Index: linux-2.6/fs/hfsplus/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/hfsplus/inode.c	2010-05-30 15:10:19.816011470 +0200
> +++ linux-2.6/fs/hfsplus/inode.c	2010-05-30 15:15:00.817051489 +0200
> @@ -105,9 +105,24 @@ static ssize_t hfsplus_direct_IO(int rw,
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file->f_path.dentry->d_inode->i_mapping->host;
> +	ssize_t ret;
>  
> -	return blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
> +	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
>  				  offset, nr_segs, hfsplus_get_block, NULL);
> +
> +	/*
> +	 * In case of error extending write may have instantiated a few
> +	 * blocks outside i_size. Trim these off again.
> +	 */
> +	if (unlikely((rw & WRITE) && ret < 0)) {
> +		loff_t isize = i_size_read(inode);
> +		loff_t end = offset + iov_length(iov, nr_segs);
> +
> +		if (end > isize)
> +			vmtruncate(inode, isize);
> +	}
> +
> +	return ret;
>  }
>  
>  static int hfsplus_writepages(struct address_space *mapping,
> Index: linux-2.6/fs/jfs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/jfs/inode.c	2010-05-30 15:10:19.826004346 +0200
> +++ linux-2.6/fs/jfs/inode.c	2010-05-30 15:15:00.824005463 +0200
> @@ -317,9 +317,24 @@ static ssize_t jfs_direct_IO(int rw, str
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file->f_mapping->host;
> +	ssize_t ret;
>  
> -	return blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
> +	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
>  				offset, nr_segs, jfs_get_block, NULL);
> +
> +	/*
> +	 * In case of error extending write may have instantiated a few
> +	 * blocks outside i_size. Trim these off again.
> +	 */
> +	if (unlikely((rw & WRITE) && ret < 0)) {
> +		loff_t isize = i_size_read(inode);
> +		loff_t end = offset + iov_length(iov, nr_segs);
> +
> +		if (end > isize)
> +			vmtruncate(inode, isize);
> +	}
> +
> +	return ret;
>  }
>  
>  const struct address_space_operations jfs_aops = {
> Index: linux-2.6/fs/nilfs2/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/nilfs2/inode.c	2010-05-30 15:10:19.837004346 +0200
> +++ linux-2.6/fs/nilfs2/inode.c	2010-05-30 15:15:00.830005533 +0200
> @@ -237,6 +237,19 @@ nilfs_direct_IO(int rw, struct kiocb *io
>  	/* Needs synchronization with the cleaner */
>  	size = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
>  				  offset, nr_segs, nilfs_get_block, NULL);
> +
> +	/*
> +	 * In case of error extending write may have instantiated a few
> +	 * blocks outside i_size. Trim these off again.
> +	 */
> +	if (unlikely((rw & WRITE) && size < 0)) {
> +		loff_t isize = i_size_read(inode);
> +		loff_t end = offset + iov_length(iov, nr_segs);
> +
> +		if (end > isize)
> +			vmtruncate(inode, isize);
> +	}
> +
>  	return size;
>  }
>  
> Index: linux-2.6/fs/reiserfs/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/reiserfs/inode.c	2010-05-30 15:10:19.848012727 +0200
> +++ linux-2.6/fs/reiserfs/inode.c	2010-05-30 15:15:00.835005812 +0200
> @@ -3059,10 +3059,25 @@ static ssize_t reiserfs_direct_IO(int rw
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file->f_mapping->host;
> +	ssize_t ret;
>  
> -	return blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
> +	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
>  				  offset, nr_segs,
>  				  reiserfs_get_blocks_direct_io, NULL);
> +
> +	/*
> +	 * In case of error extending write may have instantiated a few
> +	 * blocks outside i_size. Trim these off again.
> +	 */
> +	if (unlikely((rw & WRITE) && ret < 0)) {
> +		loff_t isize = i_size_read(inode);
> +		loff_t end = offset + iov_length(iov, nr_segs);
> +
> +		if (end > isize)
> +			vmtruncate(inode, isize);
> +	}
> +
> +	return ret;
>  }
>  
>  int reiserfs_setattr(struct dentry *dentry, struct iattr *attr)
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR